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

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

[majianpeng]

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 <majianpeng@gmail.com>
---
 drivers/md/raid5.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index d267672..ed63261 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -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)
-- 
1.7.5.4

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2680 bytes --]

On Mon, 18 Jun 2012 11:13:48 +0800 majianpeng <majianpeng@gmail.com> 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 <majianpeng@gmail.com>
> ---
>  drivers/md/raid5.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index d267672..ed63261 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -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 <neilb@suse.de>
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 <majianpeng@gmail.com>
Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index befadb4..e23cd59 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -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--; ) {

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

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

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

On Wed, 27 Jun 2012 13:45:13 +1000 NeilBrown <neilb@suse.de> 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.
> 
....
> +		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


>  
>  	if (s.handle_bad_blocks)
>  		for (i = disks; i--; ) {


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]