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

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?

Thanks,

Junxiao.
>
> Thanks,
> Joseph
>
>> int user_dlm_destroy_lock(struct user_lock_res *lockres)
>> {
>>      ...
>>
>>      status = 0;
>>      if (!(lockres->l_flags & USER_LOCK_ATTACHED)) {
>>          spin_unlock(&lockres->l_lock);
>>          goto bail;
>>      }
>>
>>      lockres->l_flags &= ~USER_LOCK_ATTACHED;
>>      lockres->l_flags |= USER_LOCK_BUSY;
>>      spin_unlock(&lockres->l_lock);
>>
>> 	status = ocfs2_dlm_unlock(conn, &lockres->l_lksb,
DLM_LKF_VALBLK);
>>      if (status) {
>>          user_log_dlm_error("ocfs2_dlm_unlock", status,
lockres);
>>          goto bail;
>>      }
>> 	...
>> }
>>
>> Cc: <stable at vger.kernel.org>
>> Signed-off-by: Junxiao Bi <junxiao.bi at oracle.com>
>> ---
>>   fs/ocfs2/dlmfs/userdlm.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/fs/ocfs2/dlmfs/userdlm.c b/fs/ocfs2/dlmfs/userdlm.c
>> index 29f183a15798..af0be612589c 100644
>> --- a/fs/ocfs2/dlmfs/userdlm.c
>> +++ b/fs/ocfs2/dlmfs/userdlm.c
>> @@ -619,7 +619,6 @@ int user_dlm_destroy_lock(struct user_lock_res
*lockres)
>>   		goto bail;
>>   	}
>>   
>> -	lockres->l_flags &= ~USER_LOCK_ATTACHED;
>>   	lockres->l_flags |= USER_LOCK_BUSY;
>>   	spin_unlock(&lockres->l_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