piaojun
2019-Feb-15 09:19 UTC
[Ocfs2-devel] [PATCH] ocfs2/dlm: return DLM_CANCELGRANT if the lock is on granted list and the operation is canceled
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. 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 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 >>>>>>> >>>>>> >>>>> . >>>>> >>>> >>> . >>> >> > . >
Changwei Ge
2019-Feb-15 09:27 UTC
[Ocfs2-devel] [PATCH] ocfs2/dlm: return DLM_CANCELGRANT if the lock is on granted list and the operation is canceled
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. 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 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 >>>>>>>> >>>>>>> >>>>>> . >>>>>> >>>>> >>>> . >>>> >>> >> . >> >