Ocfs2 devel - May 2022 - [PATCH 1/2] ocfs2: dlmfs: not clear USER_LOCK_ATTACHED when destroy lock

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.
Could you please send a new version with update description?

Thanks,
Joseph

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