Junxiao Bi
2016-Jan-04 08:20 UTC
[Ocfs2-devel] [PATCH] ocfs2/dlm: ignore cleaning the migration mle that is inuse
Hi Jiufei, On 12/31/2015 03:15 PM, xuejiufei wrote:> Hi Junxiao, > On 2015/12/31 11:05, Junxiao Bi wrote: >> On 12/30/2015 05:56 PM, xuejiufei wrote: >>> Hi Junxiao, >>> You are right. But it may happen that mle->woken is set to 1 in >>> dlm_clean_migration_mle() just after atomic_read() in >>> dlm_migrate_lockres(). Actually we trigger this BUG when dlm_send_one_lockres() >>> return error. >> Yes, that's possible because of that 5s timeout wakeup, I think this >> timeout is useless and can be removed? >>> >>> And I think dlm_migrate_lockres() should not set owner to target and return 0 >>> when mle->woken is set to 1 in dlm_clean_migration_mle(). This is another problem? >> Yes, it should be fixed, or the lockres owner will be set to a down node >> wrongly. Can the following fix these two issue? >> > It can not fix the first issue when dlm_send_one_lockres() or > dlm_mark_lockres_migrating() return error, right?Right. This is for second issue. Any way please consider these two issue, I think they can be fixed in one patch since both happened when target node down during migrate. You can make your own one or merge this patch to yours if you like. Thanks, Junxiao.> >> >> diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c >> index 84f2f8079466..d0380ea62340 100644 >> --- a/fs/ocfs2/dlm/dlmmaster.c >> +++ b/fs/ocfs2/dlm/dlmmaster.c >> @@ -2618,39 +2618,22 @@ fail: >> >> >> /* wait for new node to assert master */ >> - while (1) { >> - ret = wait_event_interruptible_timeout(mle->wq, >> - (atomic_read(&mle->woken) == 1), >> - msecs_to_jiffies(5000)); >> - >> - if (ret >= 0) { >> - if (atomic_read(&mle->woken) == 1 || >> - res->owner == target) >> - break; >> - >> - mlog(0, "%s:%.*s: timed out during migration\n", >> - dlm->name, res->lockname.len, res->lockname.name); >> - /* avoid hang during shutdown when migrating lockres >> - * to a node which also goes down */ >> - if (dlm_is_node_dead(dlm, target)) { >> - mlog(0, "%s:%.*s: expected migration " >> - "target %u is no longer up, restarting\n", >> - dlm->name, res->lockname.len, >> - res->lockname.name, target); >> - ret = -EINVAL; >> - /* migration failed, detach and clean up mle */ >> - dlm_mle_detach_hb_events(dlm, mle); >> - dlm_put_mle(mle); >> - dlm_put_mle_inuse(mle); >> - spin_lock(&res->spinlock); >> - res->state &= ~DLM_LOCK_RES_MIGRATING; >> - wake = 1; >> - spin_unlock(&res->spinlock); >> - goto leave; >> - } >> - } else >> - mlog(0, "%s:%.*s: caught signal during migration\n", >> - dlm->name, res->lockname.len, res->lockname.name); >> + wait_event(mle->wq, ((atomic_read(&mle->woken) == 1) || >> + (atomic_read(&mle->woken) == 2))); >> + >> + /* migrate target down */ >> + if (atomic_read(&mle->woken) == 2) { >> + mlog(0, "%s:%.*s: expected migration " >> + "target %u is no longer up, restarting\n", >> + dlm->name, res->lockname.len, >> + res->lockname.name, target); >> + ret = -EINVAL; >> + dlm_put_mle_inuse(mle); >> + spin_lock(&res->spinlock); >> + res->state &= ~DLM_LOCK_RES_MIGRATING; >> + wake = 1; >> + spin_unlock(&res->spinlock); >> + goto leave; >> } >> >> /* all done, set the owner, clear the flag */ >> @@ -3227,7 +3210,7 @@ static void dlm_clean_migration_mle(struct >> dlm_ctxt *dlm, >> >> spin_lock(&mle->spinlock); >> __dlm_unlink_mle(dlm, mle); >> - atomic_set(&mle->woken, 1); >> + atomic_set(&mle->woken, 2); >> spin_unlock(&mle->spinlock); >> >> wake_up(&mle->wq); >> >> Thanks, >> Junxiao. >> >> >>> >>> Thanks >>> Jiufei. >>> >>> On 2015/12/30 10:52, Junxiao Bi wrote: >>>> Hi Jiufei, >>>> >>>> When target node down, mle is cleared from >>>> dlm_do_local_recovery_cleanup()->dlm_clean_master_list()->dlm_clean_migration_mle()? >>>> mle->woken is set to 1 in dlm_clean_migration_mle(), so the code to >>>> detect target node down(if (dlm_is_node_dead(dlm, target))) will never >>>> be run in dlm_migrate_lockres()? >>>> >>>> >>>> 2621 while (1) { >>>> 2622 ret = wait_event_interruptible_timeout(mle->wq, >>>> 2623 (atomic_read(&mle->woken) >>>> == 1), >>>> 2624 msecs_to_jiffies(5000)); >>>> 2625 >>>> 2626 if (ret >= 0) { >>>> 2627 if (atomic_read(&mle->woken) == 1 || >>>> 2628 res->owner == target) >>>> 2629 break; >>>> 2630 >>>> 2631 mlog(0, "%s:%.*s: timed out during >>>> migration\n", >>>> 2632 dlm->name, res->lockname.len, >>>> res->lockname.name); >>>> 2633 /* avoid hang during shutdown when >>>> migrating lockres >>>> 2634 * to a node which also goes down */ >>>> 2635 if (dlm_is_node_dead(dlm, target)) { >>>> 2636 mlog(0, "%s:%.*s: expected migration " >>>> 2637 "target %u is no longer up, >>>> restarting\n", >>>> 2638 dlm->name, res->lockname.len, >>>> 2639 res->lockname.name, target); >>>> 2640 ret = -EINVAL; >>>> 2641 /* migration failed, detach and >>>> clean up mle */ >>>> 2642 dlm_mle_detach_hb_events(dlm, mle); >>>> 2643 dlm_put_mle(mle); >>>> 2644 dlm_put_mle_inuse(mle); >>>> 2645 spin_lock(&res->spinlock); >>>> 2646 res->state &= ~DLM_LOCK_RES_MIGRATING; >>>> 2647 wake = 1; >>>> 2648 spin_unlock(&res->spinlock); >>>> 2649 goto leave; >>>> 2650 } >>>> 2651 } else >>>> 2652 mlog(0, "%s:%.*s: caught signal during >>>> migration\n", >>>> 2653 dlm->name, res->lockname.len, >>>> res->lockname.name); >>>> 2654 } >>>> >>>> >>>> Thanks, >>>> Junxiao. >>>> On 12/28/2015 03:44 PM, xuejiufei wrote: >>>>> We have found that migration source will trigger a BUG that the >>>>> refcount of mle is already zero before put when the target is >>>>> down during migration. The situation is as follows: >>>>> >>>>> dlm_migrate_lockres >>>>> dlm_add_migration_mle >>>>> dlm_mark_lockres_migrating >>>>> dlm_get_mle_inuse >>>>> <<<<<< Now the refcount of the mle is 2. >>>>> dlm_send_one_lockres and wait for the target to become the >>>>> new master. >>>>> <<<<<< o2hb detect the target down and clean the migration >>>>> mle. Now the refcount is 1. >>>>> >>>>> dlm_migrate_lockres woken, and put the mle twice when found >>>>> the target goes down which trigger the BUG with the following >>>>> message: >>>>> "ERROR: bad mle: ". >>>>> >>>>> Signed-off-by: Jiufei Xue <xuejiufei at huawei.com> >>>>> Reviewed-by: Joseph Qi <joseph.qi at huawei.com> >>>>> --- >>>>> fs/ocfs2/dlm/dlmmaster.c | 26 +++++++++++++++----------- >>>>> 1 file changed, 15 insertions(+), 11 deletions(-) >>>>> >>>>> diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c >>>>> index 936e11b..b713140 100644 >>>>> --- a/fs/ocfs2/dlm/dlmmaster.c >>>>> +++ b/fs/ocfs2/dlm/dlmmaster.c >>>>> @@ -2519,6 +2519,11 @@ static int dlm_migrate_lockres(struct dlm_ctxt *dlm, >>>>> spin_lock(&dlm->master_lock); >>>>> ret = dlm_add_migration_mle(dlm, res, mle, &oldmle, name, >>>>> namelen, target, dlm->node_num); >>>>> + /* get an extra reference on the mle. >>>>> + * otherwise the assert_master from the new >>>>> + * master will destroy this. >>>>> + */ >>>>> + dlm_get_mle_inuse(mle); >>>>> spin_unlock(&dlm->master_lock); >>>>> spin_unlock(&dlm->spinlock); >>>>> >>>>> @@ -2554,6 +2559,7 @@ fail: >>>>> if (mle_added) { >>>>> dlm_mle_detach_hb_events(dlm, mle); >>>>> dlm_put_mle(mle); >>>>> + dlm_put_mle_inuse(mle); >>>>> } else if (mle) { >>>>> kmem_cache_free(dlm_mle_cache, mle); >>>>> mle = NULL; >>>>> @@ -2571,17 +2577,6 @@ fail: >>>>> * ensure that all assert_master work is flushed. */ >>>>> flush_workqueue(dlm->dlm_worker); >>>>> >>>>> - /* get an extra reference on the mle. >>>>> - * otherwise the assert_master from the new >>>>> - * master will destroy this. >>>>> - * also, make sure that all callers of dlm_get_mle >>>>> - * take both dlm->spinlock and dlm->master_lock */ >>>>> - spin_lock(&dlm->spinlock); >>>>> - spin_lock(&dlm->master_lock); >>>>> - dlm_get_mle_inuse(mle); >>>>> - spin_unlock(&dlm->master_lock); >>>>> - spin_unlock(&dlm->spinlock); >>>>> - >>>>> /* notify new node and send all lock state */ >>>>> /* call send_one_lockres with migration flag. >>>>> * this serves as notice to the target node that a >>>>> @@ -3312,6 +3307,15 @@ top: >>>>> mle->new_master != dead_node) >>>>> continue; >>>>> >>>>> + if (mle->new_master == dead_node && mle->inuse) { >>>>> + mlog(ML_NOTICE, "%s: target %u died during " >>>>> + "migration from %u, the MLE is " >>>>> + "still keep used, ignore it!\n", >>>>> + dlm->name, dead_node, >>>>> + mle->master); >>>>> + continue; >>>>> + } >>>>> + >>>>> /* If we have reached this point, this mle needs to be >>>>> * removed from the list and freed. */ >>>>> dlm_clean_migration_mle(dlm, mle); >>>>> >>>> >>>> >>>> . >>>> >>> >> >> >> . >> >
xuejiufei
2016-Jan-06 00:57 UTC
[Ocfs2-devel] [PATCH] ocfs2/dlm: ignore cleaning the migration mle that is inuse
Hi Junxiao, Function dlm_clean_migration_mle() will not be called after the first patch applied. So the issue that the owner of lockres will be set to a down node is not exist. Thanks, Jiufei On 2016/1/4 16:20, Junxiao Bi wrote:> Hi Jiufei, > > On 12/31/2015 03:15 PM, xuejiufei wrote: >> Hi Junxiao, >> On 2015/12/31 11:05, Junxiao Bi wrote: >>> On 12/30/2015 05:56 PM, xuejiufei wrote: >>>> Hi Junxiao, >>>> You are right. But it may happen that mle->woken is set to 1 in >>>> dlm_clean_migration_mle() just after atomic_read() in >>>> dlm_migrate_lockres(). Actually we trigger this BUG when dlm_send_one_lockres() >>>> return error. >>> Yes, that's possible because of that 5s timeout wakeup, I think this >>> timeout is useless and can be removed? >>>> >>>> And I think dlm_migrate_lockres() should not set owner to target and return 0 >>>> when mle->woken is set to 1 in dlm_clean_migration_mle(). This is another problem? >>> Yes, it should be fixed, or the lockres owner will be set to a down node >>> wrongly. Can the following fix these two issue? >>> >> It can not fix the first issue when dlm_send_one_lockres() or >> dlm_mark_lockres_migrating() return error, right? > Right. This is for second issue. Any way please consider these two > issue, I think they can be fixed in one patch since both happened when > target node down during migrate. You can make your own one or merge this > patch to yours if you like. > > Thanks, > Junxiao. >> >>> >>> diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c >>> index 84f2f8079466..d0380ea62340 100644 >>> --- a/fs/ocfs2/dlm/dlmmaster.c >>> +++ b/fs/ocfs2/dlm/dlmmaster.c >>> @@ -2618,39 +2618,22 @@ fail: >>> >>> >>> /* wait for new node to assert master */ >>> - while (1) { >>> - ret = wait_event_interruptible_timeout(mle->wq, >>> - (atomic_read(&mle->woken) == 1), >>> - msecs_to_jiffies(5000)); >>> - >>> - if (ret >= 0) { >>> - if (atomic_read(&mle->woken) == 1 || >>> - res->owner == target) >>> - break; >>> - >>> - mlog(0, "%s:%.*s: timed out during migration\n", >>> - dlm->name, res->lockname.len, res->lockname.name); >>> - /* avoid hang during shutdown when migrating lockres >>> - * to a node which also goes down */ >>> - if (dlm_is_node_dead(dlm, target)) { >>> - mlog(0, "%s:%.*s: expected migration " >>> - "target %u is no longer up, restarting\n", >>> - dlm->name, res->lockname.len, >>> - res->lockname.name, target); >>> - ret = -EINVAL; >>> - /* migration failed, detach and clean up mle */ >>> - dlm_mle_detach_hb_events(dlm, mle); >>> - dlm_put_mle(mle); >>> - dlm_put_mle_inuse(mle); >>> - spin_lock(&res->spinlock); >>> - res->state &= ~DLM_LOCK_RES_MIGRATING; >>> - wake = 1; >>> - spin_unlock(&res->spinlock); >>> - goto leave; >>> - } >>> - } else >>> - mlog(0, "%s:%.*s: caught signal during migration\n", >>> - dlm->name, res->lockname.len, res->lockname.name); >>> + wait_event(mle->wq, ((atomic_read(&mle->woken) == 1) || >>> + (atomic_read(&mle->woken) == 2))); >>> + >>> + /* migrate target down */ >>> + if (atomic_read(&mle->woken) == 2) { >>> + mlog(0, "%s:%.*s: expected migration " >>> + "target %u is no longer up, restarting\n", >>> + dlm->name, res->lockname.len, >>> + res->lockname.name, target); >>> + ret = -EINVAL; >>> + dlm_put_mle_inuse(mle); >>> + spin_lock(&res->spinlock); >>> + res->state &= ~DLM_LOCK_RES_MIGRATING; >>> + wake = 1; >>> + spin_unlock(&res->spinlock); >>> + goto leave; >>> } >>> >>> /* all done, set the owner, clear the flag */ >>> @@ -3227,7 +3210,7 @@ static void dlm_clean_migration_mle(struct >>> dlm_ctxt *dlm, >>> >>> spin_lock(&mle->spinlock); >>> __dlm_unlink_mle(dlm, mle); >>> - atomic_set(&mle->woken, 1); >>> + atomic_set(&mle->woken, 2); >>> spin_unlock(&mle->spinlock); >>> >>> wake_up(&mle->wq); >>> >>> Thanks, >>> Junxiao. >>> >>> >>>> >>>> Thanks >>>> Jiufei. >>>> >>>> On 2015/12/30 10:52, Junxiao Bi wrote: >>>>> Hi Jiufei, >>>>> >>>>> When target node down, mle is cleared from >>>>> dlm_do_local_recovery_cleanup()->dlm_clean_master_list()->dlm_clean_migration_mle()? >>>>> mle->woken is set to 1 in dlm_clean_migration_mle(), so the code to >>>>> detect target node down(if (dlm_is_node_dead(dlm, target))) will never >>>>> be run in dlm_migrate_lockres()? >>>>> >>>>> >>>>> 2621 while (1) { >>>>> 2622 ret = wait_event_interruptible_timeout(mle->wq, >>>>> 2623 (atomic_read(&mle->woken) >>>>> == 1), >>>>> 2624 msecs_to_jiffies(5000)); >>>>> 2625 >>>>> 2626 if (ret >= 0) { >>>>> 2627 if (atomic_read(&mle->woken) == 1 || >>>>> 2628 res->owner == target) >>>>> 2629 break; >>>>> 2630 >>>>> 2631 mlog(0, "%s:%.*s: timed out during >>>>> migration\n", >>>>> 2632 dlm->name, res->lockname.len, >>>>> res->lockname.name); >>>>> 2633 /* avoid hang during shutdown when >>>>> migrating lockres >>>>> 2634 * to a node which also goes down */ >>>>> 2635 if (dlm_is_node_dead(dlm, target)) { >>>>> 2636 mlog(0, "%s:%.*s: expected migration " >>>>> 2637 "target %u is no longer up, >>>>> restarting\n", >>>>> 2638 dlm->name, res->lockname.len, >>>>> 2639 res->lockname.name, target); >>>>> 2640 ret = -EINVAL; >>>>> 2641 /* migration failed, detach and >>>>> clean up mle */ >>>>> 2642 dlm_mle_detach_hb_events(dlm, mle); >>>>> 2643 dlm_put_mle(mle); >>>>> 2644 dlm_put_mle_inuse(mle); >>>>> 2645 spin_lock(&res->spinlock); >>>>> 2646 res->state &= ~DLM_LOCK_RES_MIGRATING; >>>>> 2647 wake = 1; >>>>> 2648 spin_unlock(&res->spinlock); >>>>> 2649 goto leave; >>>>> 2650 } >>>>> 2651 } else >>>>> 2652 mlog(0, "%s:%.*s: caught signal during >>>>> migration\n", >>>>> 2653 dlm->name, res->lockname.len, >>>>> res->lockname.name); >>>>> 2654 } >>>>> >>>>> >>>>> Thanks, >>>>> Junxiao. >>>>> On 12/28/2015 03:44 PM, xuejiufei wrote: >>>>>> We have found that migration source will trigger a BUG that the >>>>>> refcount of mle is already zero before put when the target is >>>>>> down during migration. The situation is as follows: >>>>>> >>>>>> dlm_migrate_lockres >>>>>> dlm_add_migration_mle >>>>>> dlm_mark_lockres_migrating >>>>>> dlm_get_mle_inuse >>>>>> <<<<<< Now the refcount of the mle is 2. >>>>>> dlm_send_one_lockres and wait for the target to become the >>>>>> new master. >>>>>> <<<<<< o2hb detect the target down and clean the migration >>>>>> mle. Now the refcount is 1. >>>>>> >>>>>> dlm_migrate_lockres woken, and put the mle twice when found >>>>>> the target goes down which trigger the BUG with the following >>>>>> message: >>>>>> "ERROR: bad mle: ". >>>>>> >>>>>> Signed-off-by: Jiufei Xue <xuejiufei at huawei.com> >>>>>> Reviewed-by: Joseph Qi <joseph.qi at huawei.com> >>>>>> --- >>>>>> fs/ocfs2/dlm/dlmmaster.c | 26 +++++++++++++++----------- >>>>>> 1 file changed, 15 insertions(+), 11 deletions(-) >>>>>> >>>>>> diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c >>>>>> index 936e11b..b713140 100644 >>>>>> --- a/fs/ocfs2/dlm/dlmmaster.c >>>>>> +++ b/fs/ocfs2/dlm/dlmmaster.c >>>>>> @@ -2519,6 +2519,11 @@ static int dlm_migrate_lockres(struct dlm_ctxt *dlm, >>>>>> spin_lock(&dlm->master_lock); >>>>>> ret = dlm_add_migration_mle(dlm, res, mle, &oldmle, name, >>>>>> namelen, target, dlm->node_num); >>>>>> + /* get an extra reference on the mle. >>>>>> + * otherwise the assert_master from the new >>>>>> + * master will destroy this. >>>>>> + */ >>>>>> + dlm_get_mle_inuse(mle); >>>>>> spin_unlock(&dlm->master_lock); >>>>>> spin_unlock(&dlm->spinlock); >>>>>> >>>>>> @@ -2554,6 +2559,7 @@ fail: >>>>>> if (mle_added) { >>>>>> dlm_mle_detach_hb_events(dlm, mle); >>>>>> dlm_put_mle(mle); >>>>>> + dlm_put_mle_inuse(mle); >>>>>> } else if (mle) { >>>>>> kmem_cache_free(dlm_mle_cache, mle); >>>>>> mle = NULL; >>>>>> @@ -2571,17 +2577,6 @@ fail: >>>>>> * ensure that all assert_master work is flushed. */ >>>>>> flush_workqueue(dlm->dlm_worker); >>>>>> >>>>>> - /* get an extra reference on the mle. >>>>>> - * otherwise the assert_master from the new >>>>>> - * master will destroy this. >>>>>> - * also, make sure that all callers of dlm_get_mle >>>>>> - * take both dlm->spinlock and dlm->master_lock */ >>>>>> - spin_lock(&dlm->spinlock); >>>>>> - spin_lock(&dlm->master_lock); >>>>>> - dlm_get_mle_inuse(mle); >>>>>> - spin_unlock(&dlm->master_lock); >>>>>> - spin_unlock(&dlm->spinlock); >>>>>> - >>>>>> /* notify new node and send all lock state */ >>>>>> /* call send_one_lockres with migration flag. >>>>>> * this serves as notice to the target node that a >>>>>> @@ -3312,6 +3307,15 @@ top: >>>>>> mle->new_master != dead_node) >>>>>> continue; >>>>>> >>>>>> + if (mle->new_master == dead_node && mle->inuse) { >>>>>> + mlog(ML_NOTICE, "%s: target %u died during " >>>>>> + "migration from %u, the MLE is " >>>>>> + "still keep used, ignore it!\n", >>>>>> + dlm->name, dead_node, >>>>>> + mle->master); >>>>>> + continue; >>>>>> + } >>>>>> + >>>>>> /* If we have reached this point, this mle needs to be >>>>>> * removed from the list and freed. */ >>>>>> dlm_clean_migration_mle(dlm, mle); >>>>>> >>>>> >>>>> >>>>> . >>>>> >>>> >>> >>> >>> . >>> >> > > > . >