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

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.

Thanks,
Joseph

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.

Thanks,

Junxiao.
>
> Thanks,
> Joseph