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

[piaojun <piaojun@huawei.com>]

Hi Changwei,

On 2019/2/15 17:27, Changwei Ge wrote:
> On 2019/2/15 17:20, piaojun wrote:
>> Hi Changwei,
>>
>> On 2019/2/15 15:56, Changwei Ge wrote:
>>> Hi Jun
>>>
>>> I just read the code around unlock/cancel.
>>>
>>> On 2019/2/15 15:35, piaojun wrote:
>>>> Hi Changwei,
>>>>
>>>> On 2019/2/15 15:06, Changwei Ge wrote:
>>>>> Hi Jun,
>>>>>
>>>>> On 2019/2/15 14:51, piaojun wrote:
>>>>>> Hi Changwei,
>>>>>>
>>>>>> On 2019/2/14 18:13, Changwei Ge wrote:
>>>>>>> On 2019/2/14 17:09, piaojun wrote:
>>>>>>>> Hi Changwei,
>>>>>>>>
>>>>>>>> The problem can't be solved completely if clear ::cancel_pending in
>>>>>>>> dlm_proxy_ast_handler, as AST will come at anytime just before
>>>>>>>
>>>>>>> So how about also add check here bere setting ::cancel_pending in dlmunlock_common() before invoking dlm_send_remote_unlock_request().
>>>>>>> If already on grant list just return DLM_CANCELGRANT
>>>>>>>
>>>>>>> Then a further reference code might look like:
>>>>>>>
>>>>>>> root at ubuntu16:/home/chge/linux[master]# git diff
>>>>>>> diff --git a/fs/ocfs2/dlm/dlmast.c b/fs/ocfs2/dlm/dlmast.c
>>>>>>> index 39831fc..812843b 100644
>>>>>>> --- a/fs/ocfs2/dlm/dlmast.c
>>>>>>> +++ b/fs/ocfs2/dlm/dlmast.c
>>>>>>> @@ -372,8 +372,11 @@ int dlm_proxy_ast_handler(struct o2net_msg *msg, u32 len, void *data,
>>>>>>>             head = &res->converting;
>>>>>>>             lock = NULL;
>>>>>>>             list_for_each_entry(lock, head, list) {
>>>>>>> -               if (lock->ml.cookie == cookie)
>>>>>>> +               if (lock->ml.cookie == cookie) {
>>>>>>> +                       if (lock->cancel_pending)
>>>>>>> +                               lock->cancel_pending = 0;
>>>>>>>                             goto do_ast;
>>>>>>> +               }
>>>>>>>             }
>>>>>>>
>>>>>>>             /* if not on convert, try blocked for ast, granted for bast */
>>>>>>> diff --git a/fs/ocfs2/dlm/dlmunlock.c b/fs/ocfs2/dlm/dlmunlock.c
>>>>>>> index c8e9b70..b4728b5 100644
>>>>>>> --- a/fs/ocfs2/dlm/dlmunlock.c
>>>>>>> +++ b/fs/ocfs2/dlm/dlmunlock.c
>>>>>>> @@ -174,9 +174,14 @@ static enum dlm_status dlmunlock_common(struct dlm_ctxt *dlm,
>>>>>>>             if (!master_node) {
>>>>>>>                     owner = res->owner;
>>>>>>>                     /* drop locks and send message */
>>>>>>> -               if (flags & LKM_CANCEL)
>>>>>>> +               if (flags & LKM_CANCEL) {
>>>>>>> +                       if (dlm_lock_on_list(&res->granted, lock)) {
>>>>>>> +                               status = DLM_CANCELGRANT;
>>>>>>> +                               goto leave;
>>>
>>> I found that above code should be useless.
>>> As upstream code already take it into consideration that AST has come before cancellation.
>>> In dlm_get_cancel_actions()
>>> 	'''
>>> 	} else if (dlm_lock_on_list(&res->granted, lock)) {
>>> 		/* too late, already granted. */
>>> 		status = DLM_CANCELGRANT;
>>> 		*actions = DLM_UNLOCK_CALL_AST;
>>> 	} else {
>>> 	'''
>>>
>>>>>>
>>>>>> If master dead and then lockres is moved to granted list in
>>>>>> dlm_move_lockres_to_recovery_list, the OCFS2_LOCK_BUSY is not cleared.
>>>>>
>>>>> OCFS2_LOCK_BUSY  should be cleared in ocfs2_locking_ast() since previous locking AST has come back(moving lock to grant list).
>>>>> That's why we return DLM_CANCELGRANT to caller to avoid calling AST. Otherwise ast() will be called twice, which is obviously a BUG.
>>>>
>>>> I mean master is already dead and ast won't come. So the
>>>> OCFS2_LOCK_BUSY is not cleared. And there are two cases that lockres is
>>>> moved to grant list:
>>>> 1. AST comes back and OCFS2_LOCK_BUSY is cleared.
>>>> 2. AST does not come back and OCFS2_LOCK_BUSY remains, recovery process
>>>>      move it to grant list. In this case, we need do AST for it.
>>>
>>> For point 2, ocfs2 can handle it, so you still don't have to worry. It's no problem.
>>> In dlmconvert_remote()
>>
>> What I worry about is that OCFS2_LOCK_BUSY won't be cleared if remote
>> master is dead, as OCFS2_LOCK_BUSY is cleared in two cases:
>> 1. remote AST come, clear it in dlm_proxy_ast_handler.
>> 2. remote AST does not come when master dead, clear it in
>>     o2dlm_unlock_ast_wrapper.
> 
> Please don't worry about point 2.
> Like my previous e-mail, after dlm recovery picking up a new master for corresponding lock resource.
> dlmlock() -> convert will retry and send request to new master.
> Eventually, this locking(convert) will succeed and ast() will be called for ocfs2 layer to clear OCFS2_LOCK_BUSY.

Perhaps you misunderstand my meaning. The AST I meant is for dlmunlock,
not for dlmlock, so it won't cause retry sending request to new master.

> 
> Thanks,
> Changwei
> 
>>
>> For case 2, if DLM_CANCELGRANT is set, o2dlm_unlock_ast_wrapper just
>> return and won't clear OCFS2_LOCK_BUSY. So in this case, I think we
>> should do AST again for it.
>>
>>>
>>>
>>> 341         if (status != DLM_NORMAL) {
>>> 342                 if (status != DLM_NOTQUEUED)
>>> 343                         dlm_error(status);
>>> 344                 dlm_revert_pending_convert(res, lock);
>>> 345         } else if (!lock->convert_pending) {
>>> 346                 mlog(0, "%s: res %.*s, owner died and lock has been moved back "
>>> 347                                 "to granted list, retry convert.\n",
>>> 348                                 dlm->name, res->lockname.len, res->lockname.name);
>>> 349                 status = DLM_RECOVERING;
>>> 350         }
>>>
>>> Thus, dlmlock() will wait until RECOVERY is done.
>>> And for ocfs2 layer, it's apparent, it doesn't have to be aware of what happened to DLM.
>>
>> I do not think the waiting can solve this problem.
>>
>>>
>>> Thanks,
>>> Changwei
>>>
>>>
>>>>
>>>> Perhaps we need do AST again for it.
>>>>
>>>>>
>>>>>> And this will cause stuck problem when ocfs2_drop_lock. The reason is
>>>>>> that unlock ast won't be done when DLM_CANCELGRANT is set. So I think
>>>>>
>>>>> With above elaboration, you don't have to worry the hang issue anymore.
>>>>>
>>>>> Thanks,
>>>>> Changwei
>>>>>
>>>>>> we need distinguish all the cases of moving lockres to grant list.
>>>>>>
>>>>>>> +                       }
>>>>>>> +
>>>>>>>                             lock->cancel_pending = 1;
>>>>>>> -               else
>>>>>>> +               } else
>>>>>>>                             lock->unlock_pending = 1;
>>>>>>>                     spin_unlock(&lock->spinlock);
>>>>>>>                     spin_unlock(&res->spinlock);
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Changwei
>>>>>>>
>>>>>>>> ::cancel_pendig is set. If there is not any other better solutions,
>>>>>>>> could we accept this patch? This bug is very harmful.
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>> Jun
>>>>>>>>
>>>>>>>> On 2018/12/8 18:05, wangjian wrote:
>>>>>>>>> Hi Changwei,
>>>>>>>>>
>>>>>>>>> I understand your idea. But we should be aware that the cancel_convert process and
>>>>>>>>> other processes (accepting the AST process, the recovery process) are asynchronous.
>>>>>>>>> For example, according to your idea, check if the lock is in the grant queue before
>>>>>>>>> calling the dlm_send_remote_unlock_request function in the dlm_proxy_ast_handler function.
>>>>>>>>> Then decide whether to clear cancel_pending. But if the AST does not come at this time,
>>>>>>>>> the check passes and cancel_pendig will not be cleared. Then AST immediately came over again,
>>>>>>>>> which also led to a bug. I personally think that for asynchronous processes we can't guarantee
>>>>>>>>> the speed of execution of each process. All we can do is to avoid the BUG scene.
>>>>>>>>>
>>>>>>>>> As for the question you said ("If you remove the BUG check why you still call dlm_commit_pending_cancel()
>>>>>>>>> to move the lock back to grant on matter it's on converting list or not?").
>>>>>>>>> I think we should first check if the lock is in the grant queue
>>>>>>>>> (at this time, dlm->spinlock and res->spinlock have been added), then decide whether to call
>>>>>>>>> dlm_commit_pending_cancel function.
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Jian
>>>>>>>>>
>>>>>>>>> On 12/7/2018 11:12 AM, Changwei Ge wrote:
>>>>>>>>>> Hi Jian,
>>>>>>>>>>
>>>>>>>>>> I suppose that the situation you described truly exists.
>>>>>>>>>> But the way you fix the issue is not in my favor.
>>>>>>>>>>
>>>>>>>>>> If you remove the BUG check why you still call dlm_commit_pending_cancel() to
>>>>>>>>>> move the lock back to grant on matter it's on converting list or not?
>>>>>>>>>>
>>>>>>>>>> So how about keeping the BUG check in dlm_move_lockres_to_recovery_list().
>>>>>>>>>> If the locking _ast_ comes back very fast just check ::cancel_pending in dlm_proxy_ast_handler() and clear it.
>>>>>>>>>> Then with the logic checking if the lock is on grant list (in dlmunlock_common() more less like your current method)
>>>>>>>>>> or not we can easily tell if the cancellation succeeds or not.
>>>>>>>>>>
>>>>>>>>>> That complies the original dlm design, which I think is better and easier for maintainers to understand.
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Changwei
>>>>>>>>>>
>>>>>>>>>> On 2018/12/6 20:06, wangjian wrote:
>>>>>>>>>>> Hi Changwei,
>>>>>>>>>>>
>>>>>>>>>>> I don't fully agree with your point of view. In my opinion, the lock cancellation process and the lock
>>>>>>>>>>> conversion process are asynchronous. We can't guarantee that the lock must be in the convert list
>>>>>>>>>>> during the lock conversion process, otherwise this BUG will not happen.
>>>>>>>>>>> So, I think this is a meaningless BUG.
>>>>>>>>>>>
>>>>>>>>>>> Thanks,
>>>>>>>>>>> Jian
>>>>>>>>>>>
>>>>>>>>>>> On 12/5/2018 9:49 AM, Changwei Ge wrote:
>>>>>>>>>>>> Hi Jian,
>>>>>>>>>>>>
>>>>>>>>>>>> I suppose you can't just remove the BUG_ON() check.
>>>>>>>>>>>> If you remove it, below code violates the original logic.
>>>>>>>>>>>>
>>>>>>>>>>>> '''
>>>>>>>>>>>> 2141                                 dlm_commit_pending_cancel(res, lock);
>>>>>>>>>>>> '''
>>>>>>>>>>>>
>>>>>>>>>>>> What's more important is *locking cancel* must be against a *locking conversion* progress.
>>>>>>>>>>>> So it makes sense to check if this lock is on converting list.
>>>>>>>>>>>>
>>>>>>>>>>>> So I have to NACK to this patch.
>>>>>>>>>>>>
>>>>>>>>>>>> Thanks,
>>>>>>>>>>>> Changwei
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> On 2018/12/3 20:23, wangjian wrote:
>>>>>>>>>>>>> In the dlm_move_lockres_to_recovery_list function, if the lock
>>>>>>>>>>>>> is in the granted queue and cancel_pending is set, it will
>>>>>>>>>>>>> encounter a BUG. I think this is a meaningless BUG,
>>>>>>>>>>>>> so be prepared to remove it. A scenario that causes
>>>>>>>>>>>>> this BUG will be given below.
>>>>>>>>>>>>>
>>>>>>>>>>>>> At the beginning, Node 1 is the master and has NL lock,
>>>>>>>>>>>>> Node 2 has PR lock, Node 3 has PR lock too.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Node 1          Node 2          Node 3
>>>>>>>>>>>>>                   want to get EX lock.
>>>>>>>>>>>>>
>>>>>>>>>>>>>                                   want to get EX lock.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Node 3 hinder
>>>>>>>>>>>>> Node 2 to get
>>>>>>>>>>>>> EX lock, send
>>>>>>>>>>>>> Node 3 a BAST.
>>>>>>>>>>>>>
>>>>>>>>>>>>>                                   receive BAST from
>>>>>>>>>>>>>                                   Node 1. downconvert
>>>>>>>>>>>>>                                   thread begin to
>>>>>>>>>>>>>                                   cancel PR to EX conversion.
>>>>>>>>>>>>>                                   In dlmunlock_common function,
>>>>>>>>>>>>>                                   downconvert thread has set
>>>>>>>>>>>>>                                   lock->cancel_pending,
>>>>>>>>>>>>>                                   but did not enter
>>>>>>>>>>>>>                                   dlm_send_remote_unlock_request
>>>>>>>>>>>>>                                   function.
>>>>>>>>>>>>>
>>>>>>>>>>>>>                   Node2 dies because
>>>>>>>>>>>>>                   the host is powered down.
>>>>>>>>>>>>>
>>>>>>>>>>>>> In recovery process,
>>>>>>>>>>>>> clean the lock that
>>>>>>>>>>>>> related to Node2.
>>>>>>>>>>>>> then finish Node 3
>>>>>>>>>>>>> PR to EX request.
>>>>>>>>>>>>> give Node 3 a AST.
>>>>>>>>>>>>>
>>>>>>>>>>>>>                                   receive AST from Node 1.
>>>>>>>>>>>>>                                   change lock level to EX,
>>>>>>>>>>>>>                                   move lock to granted list.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Node1 dies because
>>>>>>>>>>>>> the host is powered down.
>>>>>>>>>>>>>
>>>>>>>>>>>>>                                   In dlm_move_lockres_to_recovery_list
>>>>>>>>>>>>>                                   function. the lock is in the
>>>>>>>>>>>>>                                   granted queue and cancel_pending
>>>>>>>>>>>>>                                   is set. BUG_ON.
>>>>>>>>>>>>>
>>>>>>>>>>>>> But after clearing this BUG, process will encounter
>>>>>>>>>>>>> the second BUG in the ocfs2_unlock_ast function.
>>>>>>>>>>>>> Here is a scenario that will cause the second BUG
>>>>>>>>>>>>> in ocfs2_unlock_ast as follows:
>>>>>>>>>>>>>
>>>>>>>>>>>>> At the beginning, Node 1 is the master and has NL lock,
>>>>>>>>>>>>> Node 2 has PR lock, Node 3 has PR lock too.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Node 1          Node 2          Node 3
>>>>>>>>>>>>>                   want to get EX lock.
>>>>>>>>>>>>>
>>>>>>>>>>>>>                                   want to get EX lock.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Node 3 hinder
>>>>>>>>>>>>> Node 2 to get
>>>>>>>>>>>>> EX lock, send
>>>>>>>>>>>>> Node 3 a BAST.
>>>>>>>>>>>>>
>>>>>>>>>>>>>                                   receive BAST from
>>>>>>>>>>>>>                                   Node 1. downconvert
>>>>>>>>>>>>>                                   thread begin to
>>>>>>>>>>>>>                                   cancel PR to EX conversion.
>>>>>>>>>>>>>                                   In dlmunlock_common function,
>>>>>>>>>>>>>                                   downconvert thread has released
>>>>>>>>>>>>>                                   lock->spinlock and res->spinlock,
>>>>>>>>>>>>>                                   but did not enter
>>>>>>>>>>>>>                                   dlm_send_remote_unlock_request
>>>>>>>>>>>>>                                   function.
>>>>>>>>>>>>>
>>>>>>>>>>>>>                   Node2 dies because
>>>>>>>>>>>>>                   the host is powered down.
>>>>>>>>>>>>>
>>>>>>>>>>>>> In recovery process,
>>>>>>>>>>>>> clean the lock that
>>>>>>>>>>>>> related to Node2.
>>>>>>>>>>>>> then finish Node 3
>>>>>>>>>>>>> PR to EX request.
>>>>>>>>>>>>> give Node 3 a AST.
>>>>>>>>>>>>>
>>>>>>>>>>>>>                                   receive AST from Node 1.
>>>>>>>>>>>>>                                   change lock level to EX,
>>>>>>>>>>>>>                                   move lock to granted list,
>>>>>>>>>>>>>                                   set lockres->l_unlock_action
>>>>>>>>>>>>>                                   as OCFS2_UNLOCK_INVALID
>>>>>>>>>>>>>                                   in ocfs2_locking_ast function.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Node2 dies because
>>>>>>>>>>>>> the host is powered down.
>>>>>>>>>>>>>
>>>>>>>>>>>>>                                   Node 3 realize that Node 1
>>>>>>>>>>>>>                                   is dead, remove Node 1 from
>>>>>>>>>>>>>                                   domain_map. downconvert thread
>>>>>>>>>>>>>                                   get DLM_NORMAL from
>>>>>>>>>>>>>                                   dlm_send_remote_unlock_request
>>>>>>>>>>>>>                                   function and set *call_ast as 1.
>>>>>>>>>>>>>                                   Then downconvert thread meet
>>>>>>>>>>>>>                                   BUG in ocfs2_unlock_ast function.
>>>>>>>>>>>>>
>>>>>>>>>>>>> To avoid meet the second BUG, function dlmunlock_common shuold
>>>>>>>>>>>>> return DLM_CANCELGRANT if the lock is on granted list and
>>>>>>>>>>>>> the operation is canceled.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Signed-off-by: Jian Wang<wangjian161@huawei.com>
>>>>>>>>>>>>> Reviewed-by: Yiwen Jiang<jiangyiwen@huawei.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>        fs/ocfs2/dlm/dlmrecovery.c | 1 -
>>>>>>>>>>>>>        fs/ocfs2/dlm/dlmunlock.c   | 5 +++++
>>>>>>>>>>>>>        2 files changed, 5 insertions(+), 1 deletion(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/fs/ocfs2/dlm/dlmrecovery.c b/fs/ocfs2/dlm/dlmrecovery.c
>>>>>>>>>>>>> index 802636d..7489652 100644
>>>>>>>>>>>>> --- a/fs/ocfs2/dlm/dlmrecovery.c
>>>>>>>>>>>>> +++ b/fs/ocfs2/dlm/dlmrecovery.c
>>>>>>>>>>>>> @@ -2134,7 +2134,6 @@ void dlm_move_lockres_to_recovery_list(struct dlm_ctxt *dlm,
>>>>>>>>>>>>>        				 * if this had completed successfully
>>>>>>>>>>>>>        				 * before sending this lock state to the
>>>>>>>>>>>>>        				 * new master */
>>>>>>>>>>>>> -				BUG_ON(i != DLM_CONVERTING_LIST);
>>>>>>>>>>>>>        				mlog(0, "node died with cancel pending "
>>>>>>>>>>>>>        				     "on %.*s. move back to granted list.\n",
>>>>>>>>>>>>>        				     res->lockname.len, res->lockname.name);
>>>>>>>>>>>>> diff --git a/fs/ocfs2/dlm/dlmunlock.c b/fs/ocfs2/dlm/dlmunlock.c
>>>>>>>>>>>>> index 63d701c..505bb6c 100644
>>>>>>>>>>>>> --- a/fs/ocfs2/dlm/dlmunlock.c
>>>>>>>>>>>>> +++ b/fs/ocfs2/dlm/dlmunlock.c
>>>>>>>>>>>>> @@ -183,6 +183,11 @@ static enum dlm_status dlmunlock_common(struct dlm_ctxt *dlm,
>>>>>>>>>>>>>        							flags, owner);
>>>>>>>>>>>>>        		spin_lock(&res->spinlock);
>>>>>>>>>>>>>        		spin_lock(&lock->spinlock);
>>>>>>>>>>>>> +
>>>>>>>>>>>>> +		if ((flags & LKM_CANCEL) &&
>>>>>>>>>>>>> +				dlm_lock_on_list(&res->granted, lock))
>>>>>>>>>>>>> +			status = DLM_CANCELGRANT;
>>>>>>>>>>>>> +
>>>>>>>>>>>>>        		/* if the master told us the lock was already granted,
>>>>>>>>>>>>>        		 * let the ast handle all of these actions */
>>>>>>>>>>>>>        		if (status == DLM_CANCELGRANT) {
>>>>>>>>>>>>> -- 
>>>>>>>>>>>>> 1.8.3.1
>>>>>>>>>>>>>
>>>>>>>>>>>> .
>>>>>>>>>>>>
>>>>>>>>>> .
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> _______________________________________________
>>>>>>>>> Ocfs2-devel mailing list
>>>>>>>>> Ocfs2-devel at oss.oracle.com
>>>>>>>>> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>>>>>>>>>
>>>>>>>>
>>>>>>> .
>>>>>>>
>>>>>>
>>>>> .
>>>>>
>>>>
>>> .
>>>
>>
> .
>