Skip to contentSkip to conversation

ocfs2/dlm: ignore cleaning the migration mle that is inuse

PATCH New xuejiufei 2 participants 8 messages Dec 28, 2015 – Jan 7, 2016 1 message not in the archive OCFS2

lore
Share & tools
lore

A message that is not in the archive (sent off-list or not yet received): [email protected]

xuejiufei

author
#1 lore raw

PATCH 1/1 ocfs2/dlm: ignore cleaning the migration mle that is inuse

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: ".

---
 fs/ocfs2/dlm/dlmmaster.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)
fs/ocfs2/dlm/dlmmaster.c +15 -11
index 936e11b..b713140 100644
@@ -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);
Signature · 1 line
1.8.4.3
#2 lore raw

PATCH 1/1 ocfs2/dlm: ignore cleaning the migration mle that is inuse

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.

Quoted 84 lines · xuejiufei

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: ".

---
 fs/ocfs2/dlm/dlmmaster.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)
fs/ocfs2/dlm/dlmmaster.c +15 -11
index 936e11b..b713140 100644
@@ -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);
author
#3 lore raw

PATCH 1/1 ocfs2/dlm: ignore cleaning the migration mle that is inuse

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.

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?

Thanks
Jiufei.

Quoted 144 lines · Junxiao Bi

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.

Quoted 74 lines · xuejiufei

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: ".

---
 fs/ocfs2/dlm/dlmmaster.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)
fs/ocfs2/dlm/dlmmaster.c +8 -11
index 936e11b..b713140 100644
@@ -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 "

fs/ocfs2/dlm/dlmmaster.c top: · line 3321

Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +7 -0
+						"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);

.

#4 lore raw

PATCH 1/1 ocfs2/dlm: ignore cleaning the migration mle that is inuse

Quoted 5 lines · xuejiufei

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?

Quoted 3 lines

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?

fs/ocfs2/dlm/dlmmaster.c +16 -33
index 84f2f8079466..d0380ea62340 100644
@@ -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.

Quoted 150 lines

Thanks
Jiufei.

Quoted 144 lines · Junxiao Bi

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.

Quoted 74 lines · xuejiufei

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: ".

---
 fs/ocfs2/dlm/dlmmaster.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)
fs/ocfs2/dlm/dlmmaster.c +8 -11
index 936e11b..b713140 100644
@@ -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 "

fs/ocfs2/dlm/dlmmaster.c top: · line 3321

Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +7 -0
+						"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);

.

author
#5 lore raw

PATCH 1/1 ocfs2/dlm: ignore cleaning the migration mle that is inuse

Hi Junxiao,

Quoted 6 lines · Junxiao Bi
Quoted 5 lines · xuejiufei

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.

Quoted 8 lines

Yes, that's possible because of that 5s timeout wakeup, I think this timeout is useless and can be removed?

Quoted 3 lines

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?

Quoted 230 lines
fs/ocfs2/dlm/dlmmaster.c +17 -34
index 84f2f8079466..d0380ea62340 100644
@@ -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.

Quoted 150 lines

Thanks
Jiufei.

Quoted 144 lines · Junxiao Bi

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.

Quoted 74 lines · xuejiufei

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: ".

---
 fs/ocfs2/dlm/dlmmaster.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)
fs/ocfs2/dlm/dlmmaster.c +8 -11
index 936e11b..b713140 100644
@@ -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 "

fs/ocfs2/dlm/dlmmaster.c top: · line 3321

Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +7 -0
+						"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);

.

.

#6 lore raw

PATCH 1/1 ocfs2/dlm: ignore cleaning the migration mle that is inuse

Hi Jiufei,

Quoted 11 lines · xuejiufei

Hi Junxiao,

Quoted 9 lines · Junxiao Bi
Quoted 5 lines · xuejiufei

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?

Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

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.

Quoted 232 lines
Quoted 231 lines
fs/ocfs2/dlm/dlmmaster.c +17 -34
index 84f2f8079466..d0380ea62340 100644
@@ -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.

Quoted 150 lines

Thanks
Jiufei.

Quoted 144 lines · Junxiao Bi

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.

Quoted 74 lines · xuejiufei

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: ".

---
 fs/ocfs2/dlm/dlmmaster.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)
fs/ocfs2/dlm/dlmmaster.c +8 -11
index 936e11b..b713140 100644
@@ -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 "

fs/ocfs2/dlm/dlmmaster.c top: · line 3321

Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +7 -0
+						"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);

.

.

author
#7 lore raw

PATCH 1/1 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

Quoted 264 lines · Junxiao Bi

Hi Jiufei,

Quoted 11 lines · xuejiufei

Hi Junxiao,

Quoted 9 lines · Junxiao Bi
Quoted 5 lines · xuejiufei

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?

Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

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.

Quoted 229 lines
Quoted 229 lines
fs/ocfs2/dlm/dlmmaster.c +17 -34
index 84f2f8079466..d0380ea62340 100644
@@ -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.

Quoted 150 lines

Thanks
Jiufei.

Quoted 144 lines · Junxiao Bi

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.

Quoted 74 lines · xuejiufei

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: ".

---
 fs/ocfs2/dlm/dlmmaster.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)
fs/ocfs2/dlm/dlmmaster.c +8 -11
index 936e11b..b713140 100644
@@ -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 "

fs/ocfs2/dlm/dlmmaster.c top: · line 3321

Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +7 -0
+						"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);

.

Quoted 3 lines
Quoted 2 lines

.

.

#8 lore raw

PATCH 1/1 ocfs2/dlm: ignore cleaning the migration mle that is inuse

Quoted 4 lines · xuejiufei

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.

Right. Thank you for the explanation.

Thanks,
Junxiao.

Quoted 270 lines

Thanks,
Jiufei

Quoted 264 lines · Junxiao Bi

Hi Jiufei,

Quoted 11 lines · xuejiufei

Hi Junxiao,

Quoted 9 lines · Junxiao Bi
Quoted 5 lines · xuejiufei

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?

Quoted 1 line
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

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.

Quoted 229 lines
Quoted 229 lines
fs/ocfs2/dlm/dlmmaster.c +17 -34
index 84f2f8079466..d0380ea62340 100644
@@ -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.

Quoted 150 lines

Thanks
Jiufei.

Quoted 144 lines · Junxiao Bi

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.

Quoted 74 lines · xuejiufei

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: ".

---
 fs/ocfs2/dlm/dlmmaster.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)
fs/ocfs2/dlm/dlmmaster.c +8 -11
index 936e11b..b713140 100644
@@ -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 "

fs/ocfs2/dlm/dlmmaster.c top: · line 3321

Quoted 10 lines
fs/ocfs2/dlm/dlmmaster.c +7 -0
+						"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);

.

Quoted 3 lines
Quoted 2 lines

.

.