[Ocfs2-devel] [PATCH] ocfs2/dlm: return DLM_CANCELGRANT if the lock is on granted list and the operation is canceled
Changwei Ge
ge.changwei at h3c.com
Thu Feb 14 23:56:21 PST 2019
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()
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.
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 at huawei.com>
>>>>>>>>>> Reviewed-by: Yiwen Jiang<jiangyiwen at 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
>>>>>>
>>>>>
>>>> .
>>>>
>>>
>> .
>>
>
More information about the Ocfs2-devel
mailing list