Junxiao Bi
2022-May-17 16:12 UTC
[Ocfs2-devel] [PATCH 1/2] ocfs2: dlmfs: not clear USER_LOCK_ATTACHED when destroy lock
On 5/16/22 6:58 PM, Joseph Qi wrote:> > On 5/17/22 12:30 AM, Junxiao Bi wrote: >> On 5/15/22 7:57 AM, Joseph Qi wrote: >>> On 5/14/22 12:27 AM, Junxiao Bi wrote: >>>> On 5/12/22 7:05 PM, Joseph Qi wrote: >>>> >>>>> On 5/11/22 7:22 AM, Junxiao Bi wrote: >>>>>> The following function is the only place that check USER_LOCK_ATTACHED, >>>>>> this flag is set when lock request is granted through user_bast() and >>>>>> only the following function will clear it. >>>>>> >>>>> user_ast? >>>> Good catch, that's a typo, should be user_ast. >>>>>> Checking of this flag here is to make sure ocfs2_dlm_unlock is not >>>>>> issued if this lock is never granted. For example, lock file is created >>>>>> and then get removed, open file never happens. >>>>>> >>>>>> Clearing the flag here is not necessary because this is the only function >>>>>> that checks it, if another flow is executing user_dlm_destroy_lock(), it >>>>>> will bail out at the beginning because of USER_LOCK_IN_TEARDOWN and never >>>>>> check USER_LOCK_ATTACHED. >>>>>> Drop the clear, so we don't need take care it for the following >>>>>> error handling patch. >>>>>> >>>>> Seems it depends on initializing lockres every time, but it seems this >>>>> is not true for directory now. >>>> Sorry, i didn't get this. Can you elaborate this? >>>> >>> lockres may be reused and if we don't reinitialized, the left flag can >>> cause unexpected behavior. >> I don't know how it could get reused since it's going to be removed. Anyway USER_LOCK_IN_TEARDOWN is still set in lockres. All the flow will bail out because of this flag. >> > dlmfs_inode_private is allocated from kmem_cache. > The case I'm thinking about is, calling user_dlm_destroy_lock() without > a valid ast comming before. So checking USER_LOCK_ATTACHED here may be > incorrect. > But look more closer, it seems that lockres is unused for directories. > So it won't be a real issue.Yes, lock is only for file, not direcotry.> Could you please send a new version with update description?Sorry, little confused, which part of description needs update? Thanks, Junxiao.> > Thanks, > Joseph
Joseph Qi
2022-May-18 01:54 UTC
[Ocfs2-devel] [PATCH 1/2] ocfs2: dlmfs: not clear USER_LOCK_ATTACHED when destroy lock
On 5/18/22 12:12 AM, Junxiao Bi wrote:> > On 5/16/22 6:58 PM, Joseph Qi wrote: >> >> On 5/17/22 12:30 AM, Junxiao Bi wrote: >>> On 5/15/22 7:57 AM, Joseph Qi wrote: >>>> On 5/14/22 12:27 AM, Junxiao Bi wrote: >>>>> On 5/12/22 7:05 PM, Joseph Qi wrote: >>>>> >>>>>> On 5/11/22 7:22 AM, Junxiao Bi wrote: >>>>>>> The following function is the only place that check USER_LOCK_ATTACHED, >>>>>>> this flag is set when lock request is granted through user_bast() and >>>>>>> only the following function will clear it. >>>>>>> >>>>>> user_ast? >>>>> Good catch, that's a typo, should be user_ast. >>>>>>> Checking of this flag here is to make sure ocfs2_dlm_unlock is not >>>>>>> issued if this lock is never granted. For example, lock file is created >>>>>>> and then get removed, open file never happens. >>>>>>> >>>>>>> Clearing the flag here is not necessary because this is the only function >>>>>>> that checks it, if another flow is executing user_dlm_destroy_lock(), it >>>>>>> will bail out at the beginning because of USER_LOCK_IN_TEARDOWN and never >>>>>>> check USER_LOCK_ATTACHED. >>>>>>> Drop the clear, so we don't need take care it for the following >>>>>>> error handling patch. >>>>>>> >>>>>> Seems it depends on initializing lockres every time, but it seems this >>>>>> is not true for directory now. >>>>> Sorry, i didn't get this. Can you elaborate this? >>>>> >>>> lockres may be reused and if we don't reinitialized, the left flag can >>>> cause unexpected behavior. >>> I don't know how it could get reused since it's going to be removed. Anyway USER_LOCK_IN_TEARDOWN is still set in lockres. All the flow will bail out because of this flag. >>> >> dlmfs_inode_private is allocated from kmem_cache. >> The case I'm thinking about is, calling user_dlm_destroy_lock() without >> a valid ast comming before. So checking USER_LOCK_ATTACHED here may be >> incorrect. >> But look more closer, it seems that lockres is unused for directories. >> So it won't be a real issue. > Yes, lock is only for file, not direcotry. >> Could you please send a new version with update description? > > Sorry, little confused, which part of description needs update? >The typo that user_ast() is for granting lock request. And better to include the information we discussed above.