Skip to contentSkip to conversation

md/raid5:Add "BlockedBadBlocks" flag when waitting rdev to be unlocked.

PATCH In review majianpeng 2 participants 3 messages Jun 18, 2012 – Jun 27, 2012 MD RAID

lore
Share & tools
lore

majianpeng

author
#0 lore raw

PATCH 1/1 md/raid5:Add "BlockedBadBlocks" flag when waitting rdev to be unlocked.

If rdev became blocked because the unack badblocks, it did not exec md_wait_for_blocked_rdev in handle_stripe().So the rdev->nr_pending did not decrease.So rdev did not remove because the wrong nr_pending.

Signed-off-by majianpeng <[email protected]>
---
 drivers/md/raid5.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/md/raid5.c +2 -1
index d267672..ed63261 100644
@@ -3582,7 +3582,8 @@ static void handle_stripe(struct stripe_head *sh)
 
 finish:
 	/* wait for this device to become unblocked */
-	if (conf->mddev->external && unlikely(s.blocked_rdev))
+	if (unlikely(s.blocked_rdev) && (conf->mddev->external ||
+		test_bit(BlockedBadBlocks, &(s.blocked_rdev->flags))))
 		md_wait_for_blocked_rdev(s.blocked_rdev, conf->mddev);
 
 	if (s.handle_bad_blocks)
Signature · 1 line
1.7.5.4
#1 lore raw
Quoted 13 lines · On Mon, 18 Jun 2012 11:13:48 +0800 majianpeng <[email protected]> wrote:

If rdev became blocked because the unack badblocks, it did not exec md_wait_for_blocked_rdev in handle_stripe().So the rdev->nr_pending did not decrease.So rdev did not remove because the wrong nr_pending.

Signed-off-by majianpeng <[email protected]>
---
 drivers/md/raid5.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)
drivers/md/raid5.c +0 -0
index d267672..ed63261 100644

drivers/md/raid5.c static void handle_stripe(struct stripe_head *sh) · line 3589

Quoted 9 lines
drivers/md/raid5.c +2 -1
@@ -3582,7 +3582,8 @@ static void handle_stripe(struct stripe_head *sh)
 
 finish:
 	/* wait for this device to become unblocked */
-	if (conf->mddev->external && unlikely(s.blocked_rdev))
+	if (unlikely(s.blocked_rdev) && (conf->mddev->external ||
+		test_bit(BlockedBadBlocks, &(s.blocked_rdev->flags))))
 		md_wait_for_blocked_rdev(s.blocked_rdev, conf->mddev);
 
 	if (s.handle_bad_blocks)

Thanks for finding this. However I don't think your patch is quite correct. It would re-introduce a hang fixed by commit 43220aa0f22cd3ce5b3.

I've applied the following instead.

Thanks,
NeilBrown

From 0cee6aeb02b1ef947be62bb455f64720ecba2b4c Mon Sep 17 00:00:00 2001
From: NeilBrown <[email protected]>
Date: Wed, 27 Jun 2012 13:43:54 +1000
Subject: [PATCH] md/raid5: fix refcount problem when blocked_rdev is set.

commit 43220aa0f22cd3ce5b30246d50ccd696d119edea
md/raid5: fix a hang on device failure.

fixed a hang, but introduced a refcounting in balance so that if the presence of bad-blocks ever caused an rdev to be 'blocked' we would increment the refcount on the rdev and never decrement it.

So added the needed rdev_dec_pending when md_wait_for_blocked_rdev is not called.

Reported-by majianpeng <[email protected]>
Signed-off-by NeilBrown <[email protected]>
drivers/md/raid5.c +11 -2
index befadb4..e23cd59 100644
@@ -3588,8 +3588,17 @@ static void handle_stripe(struct stripe_head *sh)
 
 finish:
 	/* wait for this device to become unblocked */
-	if (conf->mddev->external && unlikely(s.blocked_rdev))
-		md_wait_for_blocked_rdev(s.blocked_rdev, conf->mddev);
+	if (unlikely(s.blocked_rdev)) {
+		if (conf->mddev->external)
+			md_wait_for_blocked_rdev(s.blocked_rdev,
+						 conf->mddev);
+		else
+			/* Internal metadata will immediately
+			 * be written by raid5d, so we don't
+			 * need to wait here.
+			 */
+			rdev_dec_pending(rdev, mddev);
+	}
 
 	if (s.handle_bad_blocks)
 		for (i = disks; i--; ) {
#2 lore raw
Quoted 5 lines · On Wed, 27 Jun 2012 13:45:13 +1000 NeilBrown <[email protected]> wrote:

Thanks for finding this. However I don't think your patch is quite correct. It would re-introduce a hang fixed by commit 43220aa0f22cd3ce5b3.

I've applied the following instead.

....

drivers/md/raid5.c static void handle_stripe(struct stripe_head *sh) · line 3601

Quoted 7 lines
drivers/md/raid5.c +7 -0
+		else
+			/* Internal metadata will immediately
+			 * be written by raid5d, so we don't
+			 * need to wait here.
+			 */
+			rdev_dec_pending(rdev, mddev);
+	}

... which is clearly wrong. That last line is now

                        rdev_dec_pending(s.blocked_rdev, conf->mddev);

NeilBrown

Quoted 3 lines
drivers/md/raid5.c +0 -0
 	if (s.handle_bad_blocks)
 		for (i = disks; i--; ) {