Ocfs2 devel - Feb 2019 - [PATCH] ocfs2/dlm: return DLM_CANCELGRANT if the lock is on granted list and the operation is canceled

Hi Changwei,

On 2019/2/21 14:46, Changwei Ge wrote:
> Hi jun
> Good afternoon.
> 
>>>>> If AST doesn't manage to get back to requested node,
why must flag OCFS2_LOCK_BUSY be cleared in o2dlm_unlock_ast_wrapper?
>>>>>
>>>>> Yes, OCFS2_LOCK_BUSY can be cleared it either
o2dlm_unlock_ast_wrapper() or o2dlm_lock_ast_wrapper() with o2cb stack applied.
>>>>>
>>>>> If we return DLM_CANCELGRANT from ocfs2/dlm to dlm, then we
must know that AST has ever come back or master node has moved the lock to grant
list itself, thus we clear flag OCFS2_LOCK_BUSY in o2dlm_lock_ast_wrapper().
>>>>> Otherwise we ascertain that we can stop current ongoing
locking procedure and must clear OCFS2_LOCK_BUSY in o2dlm_unlock_ast_wrapper()
(*synchronized*).
>>>>>
>>>>> Let's summarize this, OCFS2_LOCK_BUSY should be cleared
whether by locking success or cancellation success.
>>>>>
>>>>> And my way already check if the lock is granted then return
DLM_CANCELGRANT or not.
>>>>>
>>>>
>>>> OCFS2_LOCK_BUSY won't be cleared if DLM_CANCELGRANT is set
in
>>>> o2dlm_unlock_ast_wrapper, and that's what I'm concerned
about:
>>>
>>> But we already *ascertain* that previous locking request has been
*granted* before deciding to return DLM_CANCELGRANT during cancellation to
o2dlm_unlock_ast_wrapper().
>>>
>>> If above condition stands, o2dlm_lock_ast_wrapper() must will be or
have been called, which also clears OCFS2_LOCK_BUSY.
>>>
>>
>> 1. Node1 already has PR lock, and wants to get ex.
> Well, a locking up-conversion procedure.
> 
>> 2. Node1 receive BAST and do unlock, here OCFS2_LOCK_BUSY is set.
> Because there are two concurrent up-conversion, which conflict, so one of
them must be canceled!
> 
>> 3. Node1 can not receive the AST for unlock as master dead.
> So here you mean the lock can't be granted.
> 
>> 4. Then o2dlm_unlock_ast_wrapper will be called rather than
o2dlm_lock_ast_wrapper.
> Then the cancellation succeeds as the master dies.
> 
>> 5. Actually the *granted* lock request has nothing to do with
OCFS2_LOCK_BUSY.
> Yes, o2dlm_lock_ast_wrapper will not clear OCFS2_LOCK_BUSY.
> 
> But my suggestion was not against above timing sequence.
> Did you misunderstand my suggestion?
> And the original logic of Jian's patch also returns DLM_CANCELGRANT.
Yes, Jian's last patch can't solve the problem either, and I think we
should find another solution for it. I'm considering deleting the check
for DLM_CANCELGRANT, and clear OCFS2_LOCK_BUSY in the following process.

Thanks,
Jun
> 
> Thanks,
> Changwei
> 
> 
>   
> 
> .
>

Hi Jun,

On 2019/2/22 11:16, piaojun wrote:
> Hi Changwei,
> 
> On 2019/2/21 14:46, Changwei Ge wrote:
>> Hi jun
>> Good afternoon.
>>
>>>>>> If AST doesn't manage to get back to requested
node, why must flag OCFS2_LOCK_BUSY be cleared in o2dlm_unlock_ast_wrapper?
>>>>>>
>>>>>> Yes, OCFS2_LOCK_BUSY can be cleared it either
o2dlm_unlock_ast_wrapper() or o2dlm_lock_ast_wrapper() with o2cb stack applied.
>>>>>>
>>>>>> If we return DLM_CANCELGRANT from ocfs2/dlm to dlm,
then we must know that AST has ever come back or master node has moved the lock
to grant list itself, thus we clear flag OCFS2_LOCK_BUSY in
o2dlm_lock_ast_wrapper().
>>>>>> Otherwise we ascertain that we can stop current ongoing
locking procedure and must clear OCFS2_LOCK_BUSY in o2dlm_unlock_ast_wrapper()
(*synchronized*).
>>>>>>
>>>>>> Let's summarize this, OCFS2_LOCK_BUSY should be
cleared whether by locking success or cancellation success.
>>>>>>
>>>>>> And my way already check if the lock is granted then
return DLM_CANCELGRANT or not.
>>>>>>
>>>>>
>>>>> OCFS2_LOCK_BUSY won't be cleared if DLM_CANCELGRANT is
set in
>>>>> o2dlm_unlock_ast_wrapper, and that's what I'm
concerned about:
>>>>
>>>> But we already *ascertain* that previous locking request has
been *granted* before deciding to return DLM_CANCELGRANT during cancellation to
o2dlm_unlock_ast_wrapper().
>>>>
>>>> If above condition stands, o2dlm_lock_ast_wrapper() must will
be or have been called, which also clears OCFS2_LOCK_BUSY.
>>>>
>>>
>>> 1. Node1 already has PR lock, and wants to get ex.
>> Well, a locking up-conversion procedure.
>>
>>> 2. Node1 receive BAST and do unlock, here OCFS2_LOCK_BUSY is set.
>> Because there are two concurrent up-conversion, which conflict, so one
of them must be canceled!
>>
>>> 3. Node1 can not receive the AST for unlock as master dead.
>> So here you mean the lock can't be granted.
>>
>>> 4. Then o2dlm_unlock_ast_wrapper will be called rather than
o2dlm_lock_ast_wrapper.
>> Then the cancellation succeeds as the master dies.
>>
>>> 5. Actually the *granted* lock request has nothing to do with
OCFS2_LOCK_BUSY.
>> Yes, o2dlm_lock_ast_wrapper will not clear OCFS2_LOCK_BUSY.
>>
>> But my suggestion was not against above timing sequence.
>> Did you misunderstand my suggestion?
>> And the original logic of Jian's patch also returns
DLM_CANCELGRANT.
> 
> Yes, Jian's last patch can't solve the problem either, and I think
we
> should find another solution for it. I'm considering deleting the check
> for DLM_CANCELGRANT, and clear OCFS2_LOCK_BUSY in the following process.
If Jian's patch can't fix the issue either, I am going to ask Andrew to
drop this patch.
Hopefully, Jian or you can help post another patch for it. Then we can have
another round of discussion.

Thanks,
Changwei
> 
> Thanks,
> Jun
> 
>>
>> Thanks,
>> Changwei
>>
>>
>>    
>>
>> .
>>
>