linux-raid.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* md/raid5:Fix recover/replace stop if handle stipe failed
@ 2012-03-14  7:07 majianpeng
  2012-03-14  7:33 ` NeilBrown
  2012-03-14  9:27 ` majianpeng
  0 siblings, 2 replies; 9+ messages in thread
From: majianpeng @ 2012-03-14  7:07 UTC (permalink / raw)
  To: Neil Brown; +Cc: linux-raid

From 849df9f6422972452b99a2c2d08d005437a52d72 Mon Sep 17 00:00:00 2001
From: majianpeng <majianpeng@gmail.com>
Date: Wed, 14 Mar 2012 14:41:07 +0800
Subject: [PATCH] md/raid5:Fix recover/replace stop if handle stipe failed. 
 If handled stipe failed when recover/replace,should not first
 call md_done_sync(conf->mddev, STRIPE_SECTORS, 0).Beacause
 this set MD_RECOVERY_INTR and will terminate the
 recover/replace. And the sync_thread will repeatly start
 and stop.


Signed-off-by: majianpeng <majianpeng@gmail.com>
---
 drivers/md/raid5.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 360f2b9..55193ef 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2472,7 +2472,6 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
 	int abort = 0;
 	int i;
 
-	md_done_sync(conf->mddev, STRIPE_SECTORS, 0);
 	clear_bit(STRIPE_SYNCING, &sh->state);
 	s->syncing = 0;
 	s->replacing = 0;
@@ -2480,8 +2479,12 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
 	 * For recover/replace we need to record a bad block on all
 	 * non-sync devices, or abort the recovery
 	 */
-	if (!test_bit(MD_RECOVERY_RECOVER, &conf->mddev->recovery))
+	if (!test_bit(MD_RECOVERY_RECOVER, &conf->mddev->recovery)) {
+		md_done_sync(conf->mddev, STRIPE_SECTORS, 0);
 		return;
+	} else
+		md_done_sync(conf->mddev, STRIPE_SECTORS, 1);
+
 	/* During recovery devices cannot be removed, so locking and
 	 * refcounting of rdevs is not needed
 	 */
@@ -2504,6 +2507,7 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
 	if (abort) {
 		conf->recovery_disabled = conf->mddev->recovery_disabled;
 		set_bit(MD_RECOVERY_INTR, &conf->mddev->recovery);
+		md_wakeup_thread(conf->mddev->thread);
 	}
 }
 
-- 
1.7.5.4

 				
--------------
majianpeng
2012-03-14


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: md/raid5:Fix recover/replace stop if handle stipe failed
  2012-03-14  7:07 md/raid5:Fix recover/replace stop if handle stipe failed majianpeng
@ 2012-03-14  7:33 ` NeilBrown
  2012-03-14  9:27 ` majianpeng
  1 sibling, 0 replies; 9+ messages in thread
From: NeilBrown @ 2012-03-14  7:33 UTC (permalink / raw)
  To: majianpeng; +Cc: linux-raid

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

On Wed, 14 Mar 2012 15:07:55 +0800 "majianpeng" <majianpeng@gmail.com> wrote:

> >From 849df9f6422972452b99a2c2d08d005437a52d72 Mon Sep 17 00:00:00 2001
> From: majianpeng <majianpeng@gmail.com>
> Date: Wed, 14 Mar 2012 14:41:07 +0800
> Subject: [PATCH] md/raid5:Fix recover/replace stop if handle stipe failed. 
>  If handled stipe failed when recover/replace,should not first
>  call md_done_sync(conf->mddev, STRIPE_SECTORS, 0).Beacause
>  this set MD_RECOVERY_INTR and will terminate the
>  recover/replace. And the sync_thread will repeatly start
>  and stop.

I disagree.  It is safer to stop and then (if all seems to be working) to
start again.  We will start up exactly were we left of so there is little
cost, and I think it make the code safer.


> 
> 
> Signed-off-by: majianpeng <majianpeng@gmail.com>
> ---
>  drivers/md/raid5.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 360f2b9..55193ef 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -2472,7 +2472,6 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
>  	int abort = 0;
>  	int i;
>  
> -	md_done_sync(conf->mddev, STRIPE_SECTORS, 0);
>  	clear_bit(STRIPE_SYNCING, &sh->state);
>  	s->syncing = 0;
>  	s->replacing = 0;
> @@ -2480,8 +2479,12 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
>  	 * For recover/replace we need to record a bad block on all
>  	 * non-sync devices, or abort the recovery
>  	 */
> -	if (!test_bit(MD_RECOVERY_RECOVER, &conf->mddev->recovery))
> +	if (!test_bit(MD_RECOVERY_RECOVER, &conf->mddev->recovery)) {
> +		md_done_sync(conf->mddev, STRIPE_SECTORS, 0);
>  		return;
> +	} else
> +		md_done_sync(conf->mddev, STRIPE_SECTORS, 1);
> +
>  	/* During recovery devices cannot be removed, so locking and
>  	 * refcounting of rdevs is not needed
>  	 */
> @@ -2504,6 +2507,7 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
>  	if (abort) {
>  		conf->recovery_disabled = conf->mddev->recovery_disabled;
>  		set_bit(MD_RECOVERY_INTR, &conf->mddev->recovery);
> +		md_wakeup_thread(conf->mddev->thread);

This change seems unrelated to the above changes.

It isn't needed as this function is called only by the thread that you are
waking up, so it cannot be asleep.

Thanks,
NeilBrown



>  	}
>  }
>  


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Re: md/raid5:Fix recover/replace stop if handle stipe failed
  2012-03-14  7:07 md/raid5:Fix recover/replace stop if handle stipe failed majianpeng
  2012-03-14  7:33 ` NeilBrown
@ 2012-03-14  9:27 ` majianpeng
  2012-03-27  3:26   ` NeilBrown
  1 sibling, 1 reply; 9+ messages in thread
From: majianpeng @ 2012-03-14  9:27 UTC (permalink / raw)
  To: NeilBrown; +Cc: linux-raid

I created a raid5 using three disks and disk0 add bad blocks.I set faulty disk2 and remov disk2 and readd disk2.
It seems to recover well and set disk2 badblocks  as disk0.
But the md0_resync repeatly stop and start.
The recovery_start of disk2 all the same .

In function md_do_sync()
sectors = mddev->pers->sync_request(mddev, j, &skipped,
						  currspeed < speed_min(mddev));
		if (sectors == 0) {
			set_bit(MD_RECOVERY_INTR, &mddev->recovery);
			goto out;
		}

		if (!skipped) { /* actual IO requested */
			io_sectors += sectors;
			atomic_add(sectors, &mddev->recovery_active);
		}

		if (test_bit(MD_RECOVERY_INTR, &mddev->recovery))
			break;

		j += sectors;
		if (j>1) mddev->curr_resync = j;

If  'if (test_bit(MD_RECOVERY_INTR, &mddev->recovery))' is ok ,then j does not add sectors so curr_resync does not change.
The sparedisk recovery_start not change.


------------------				 
majianpeng
2012-03-14

-------------------------------------------------------------
发件人:NeilBrown
发送日期:2012-03-14 15:33:53
收件人:majianpeng
抄送:linux-raid
主题:Re: md/raid5:Fix recover/replace stop if handle stipe failed

On Wed, 14 Mar 2012 15:07:55 +0800 "majianpeng" <majianpeng@gmail.com> wrote:

> >From 849df9f6422972452b99a2c2d08d005437a52d72 Mon Sep 17 00:00:00 2001
> From: majianpeng <majianpeng@gmail.com>
> Date: Wed, 14 Mar 2012 14:41:07 +0800
> Subject: [PATCH] md/raid5:Fix recover/replace stop if handle stipe failed. 
>  If handled stipe failed when recover/replace,should not first
>  call md_done_sync(conf->mddev, STRIPE_SECTORS, 0).Beacause
>  this set MD_RECOVERY_INTR and will terminate the
>  recover/replace. And the sync_thread will repeatly start
>  and stop.

I disagree.  It is safer to stop and then (if all seems to be working) to
start again.  We will start up exactly were we left of so there is little
cost, and I think it make the code safer.


> 
> 
> Signed-off-by: majianpeng <majianpeng@gmail.com>
> ---
>  drivers/md/raid5.c |    8 ++++++--
>  1 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
> index 360f2b9..55193ef 100644
> --- a/drivers/md/raid5.c
> +++ b/drivers/md/raid5.c
> @@ -2472,7 +2472,6 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
>  	int abort = 0;
>  	int i;
>  
> -	md_done_sync(conf->mddev, STRIPE_SECTORS, 0);
>  	clear_bit(STRIPE_SYNCING, &sh->state);
>  	s->syncing = 0;
>  	s->replacing = 0;
> @@ -2480,8 +2479,12 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
>  	 * For recover/replace we need to record a bad block on all
>  	 * non-sync devices, or abort the recovery
>  	 */
> -	if (!test_bit(MD_RECOVERY_RECOVER, &conf->mddev->recovery))
> +	if (!test_bit(MD_RECOVERY_RECOVER, &conf->mddev->recovery)) {
> +		md_done_sync(conf->mddev, STRIPE_SECTORS, 0);
>  		return;
> +	} else
> +		md_done_sync(conf->mddev, STRIPE_SECTORS, 1);
> +
>  	/* During recovery devices cannot be removed, so locking and
>  	 * refcounting of rdevs is not needed
>  	 */
> @@ -2504,6 +2507,7 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
>  	if (abort) {
>  		conf->recovery_disabled = conf->mddev->recovery_disabled;
>  		set_bit(MD_RECOVERY_INTR, &conf->mddev->recovery);
> +		md_wakeup_thread(conf->mddev->thread);

This change seems unrelated to the above changes.

It isn't needed as this function is called only by the thread that you are
waking up, so it cannot be asleep.

Thanks,
NeilBrown



>  	}
>  }
>  


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: md/raid5:Fix recover/replace stop if handle stipe failed
  2012-03-14  9:27 ` majianpeng
@ 2012-03-27  3:26   ` NeilBrown
  2012-03-27  7:33     ` majianpeng
  0 siblings, 1 reply; 9+ messages in thread
From: NeilBrown @ 2012-03-27  3:26 UTC (permalink / raw)
  To: majianpeng; +Cc: linux-raid

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

On Wed, 14 Mar 2012 17:27:44 +0800 "majianpeng" <majianpeng@gmail.com> wrote:

> I created a raid5 using three disks and disk0 add bad blocks.I set faulty disk2 and remov disk2 and readd disk2.
> It seems to recover well and set disk2 badblocks  as disk0.
> But the md0_resync repeatly stop and start.
> The recovery_start of disk2 all the same .
> 

Thanks for the extra details (and sorry for the delay in replying).

There certainly is something wrong with handling bad blocks during recovery.

I think this patch should fix it.  Are you able to test it and confirm?

Thanks,
NeilBrown


diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 23ac880..2186e0e 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2471,39 +2471,41 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
 	int abort = 0;
 	int i;
 
-	md_done_sync(conf->mddev, STRIPE_SECTORS, 0);
 	clear_bit(STRIPE_SYNCING, &sh->state);
 	s->syncing = 0;
 	s->replacing = 0;
 	/* There is nothing more to do for sync/check/repair.
+	 * Don't even need to abort as that is handled elsewhere
+	 * if needed, and not always wanted e.g. if there is a known
+	 * bad block here.
 	 * For recover/replace we need to record a bad block on all
 	 * non-sync devices, or abort the recovery
 	 */
-	if (!test_bit(MD_RECOVERY_RECOVER, &conf->mddev->recovery))
-		return;
-	/* During recovery devices cannot be removed, so locking and
-	 * refcounting of rdevs is not needed
-	 */
-	for (i = 0; i < conf->raid_disks; i++) {
-		struct md_rdev *rdev = conf->disks[i].rdev;
-		if (rdev
-		    && !test_bit(Faulty, &rdev->flags)
-		    && !test_bit(In_sync, &rdev->flags)
-		    && !rdev_set_badblocks(rdev, sh->sector,
-					   STRIPE_SECTORS, 0))
-			abort = 1;
-		rdev = conf->disks[i].replacement;
-		if (rdev
-		    && !test_bit(Faulty, &rdev->flags)
-		    && !test_bit(In_sync, &rdev->flags)
-		    && !rdev_set_badblocks(rdev, sh->sector,
-					   STRIPE_SECTORS, 0))
-			abort = 1;
-	}
-	if (abort) {
-		conf->recovery_disabled = conf->mddev->recovery_disabled;
-		set_bit(MD_RECOVERY_INTR, &conf->mddev->recovery);
+	if (test_bit(MD_RECOVERY_RECOVER, &conf->mddev->recovery)) {
+		/* During recovery devices cannot be removed, so
+		 * locking and refcounting of rdevs is not needed
+		 */
+		for (i = 0; i < conf->raid_disks; i++) {
+			struct md_rdev *rdev = conf->disks[i].rdev;
+			if (rdev
+			    && !test_bit(Faulty, &rdev->flags)
+			    && !test_bit(In_sync, &rdev->flags)
+			    && !rdev_set_badblocks(rdev, sh->sector,
+						   STRIPE_SECTORS, 0))
+				abort = 1;
+			rdev = conf->disks[i].replacement;
+			if (rdev
+			    && !test_bit(Faulty, &rdev->flags)
+			    && !test_bit(In_sync, &rdev->flags)
+			    && !rdev_set_badblocks(rdev, sh->sector,
+						   STRIPE_SECTORS, 0))
+				abort = 1;
+		}
+		if (abort)
+			conf->recovery_disabled =
+				conf->mddev->recovery_disabled;
 	}
+	md_done_sync(conf->mddev, STRIPE_SECTORS, !abort);
 }
 
 static int want_replace(struct stripe_head *sh, int disk_idx)
@@ -3203,7 +3205,8 @@ static void analyse_stripe(struct stripe_head *sh, struct stripe_head_state *s)
 			/* Not in-sync */;
 		else if (is_bad) {
 			/* also not in-sync */
-			if (!test_bit(WriteErrorSeen, &rdev->flags)) {
+			if (!test_bit(WriteErrorSeen, &rdev->flags) &&
+			    test_bit(R5_UPTODATE, &sh->devs[i].flags)) {
 				/* treat as in-sync, but with a read error
 				 * which we can now try to correct
 				 */

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: Re: md/raid5:Fix recover/replace stop if handle stipe failed
  2012-03-27  3:26   ` NeilBrown
@ 2012-03-27  7:33     ` majianpeng
  2012-03-28  1:40       ` NeilBrown
  2012-03-28  1:45       ` majianpeng
  0 siblings, 2 replies; 9+ messages in thread
From: majianpeng @ 2012-03-27  7:33 UTC (permalink / raw)
  To: NeilBrown; +Cc: linux-raid

I tested and corrected some bug.

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 23ac880..f8480f8 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2471,39 +2471,42 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
        int abort = 0;
        int i;
 
-       md_done_sync(conf->mddev, STRIPE_SECTORS, 0);
        clear_bit(STRIPE_SYNCING, &sh->state);
        s->syncing = 0;
        s->replacing = 0;
        /* There is nothing more to do for sync/check/repair.
+        * Don't even need to abort as that is handled elsewhere
+        * if needed, and not always wanted e.g. if there is a known
+        * bad block here.
         * For recover/replace we need to record a bad block on all
         * non-sync devices, or abort the recovery
         */
-       if (!test_bit(MD_RECOVERY_RECOVER, &conf->mddev->recovery))
-               return;
-       /* During recovery devices cannot be removed, so locking and
-        * refcounting of rdevs is not needed
-        */
-       for (i = 0; i < conf->raid_disks; i++) {
-               struct md_rdev *rdev = conf->disks[i].rdev;
-               if (rdev
-                   && !test_bit(Faulty, &rdev->flags)
-                   && !test_bit(In_sync, &rdev->flags)
-                   && !rdev_set_badblocks(rdev, sh->sector,
-                                          STRIPE_SECTORS, 0))
-                       abort = 1;
-               rdev = conf->disks[i].replacement;
-               if (rdev
-                   && !test_bit(Faulty, &rdev->flags)
-                   && !test_bit(In_sync, &rdev->flags)
-                   && !rdev_set_badblocks(rdev, sh->sector,
-                                          STRIPE_SECTORS, 0))
-                       abort = 1;
-       }
-       if (abort) {
-               conf->recovery_disabled = conf->mddev->recovery_disabled;
-               set_bit(MD_RECOVERY_INTR, &conf->mddev->recovery);
-       }
+       if (test_bit(MD_RECOVERY_RECOVER, &conf->mddev->recovery)) {
+               /* During recovery devices cannot be removed, so
+                * locking and refcounting of rdevs is not needed
+                */
+               for (i = 0; i < conf->raid_disks; i++) {
+                       struct md_rdev *rdev = conf->disks[i].rdev;
+                       if (rdev
+                           && !test_bit(Faulty, &rdev->flags)
+                           && !test_bit(In_sync, &rdev->flags)
+                           && !rdev_set_badblocks(rdev, sh->sector,
+                                                  STRIPE_SECTORS, 0))
+                               abort = 1;
+                       rdev = conf->disks[i].replacement;
+                       if (rdev
+                           && !test_bit(Faulty, &rdev->flags)
+                           && !test_bit(In_sync, &rdev->flags)
+                           && !rdev_set_badblocks(rdev, sh->sector,
+                                                  STRIPE_SECTORS, 0))
+                               abort = 1;
+               }
+               if (abort)
+                       conf->recovery_disabled =
+                               conf->mddev->recovery_disabled;
+       } else
+               abort = 1;
+       md_done_sync(conf->mddev, STRIPE_SECTORS, !abort);
 }
 
 static int want_replace(struct stripe_head *sh, int disk_idx)
@@ -3203,7 +3206,8 @@ static void analyse_stripe(struct stripe_head *sh, struct stripe_head_state *s)
                        /* Not in-sync */;
                else if (is_bad) {
                        /* also not in-sync */
-                       if (!test_bit(WriteErrorSeen, &rdev->flags)) {
+                       if (!test_bit(WriteErrorSeen, &rdev->flags) &&
+                           test_bit(R5_UPTODATE, &dev->flags)) {
                                /* treat as in-sync, but with a read error
                                 * which we can now try to correct
                                 */


------------------				 
majianpeng
2012-03-27

-------------------------------------------------------------
发件人:NeilBrown
发送日期:2012-03-27 11:26:50
收件人:majianpeng
抄送:linux-raid
主题:Re: md/raid5:Fix recover/replace stop if handle stipe failed

On Wed, 14 Mar 2012 17:27:44 +0800 "majianpeng" <majianpeng@gmail.com> wrote:

> I created a raid5 using three disks and disk0 add bad blocks.I set faulty disk2 and remov disk2 and readd disk2.
> It seems to recover well and set disk2 badblocks  as disk0.
> But the md0_resync repeatly stop and start.
> The recovery_start of disk2 all the same .
> 

Thanks for the extra details (and sorry for the delay in replying).

There certainly is something wrong with handling bad blocks during recovery.

I think this patch should fix it.  Are you able to test it and confirm?

Thanks,
NeilBrown


diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 23ac880..2186e0e 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c
@@ -2471,39 +2471,41 @@ handle_failed_sync(struct r5conf *conf, struct stripe_head *sh,
 	int abort = 0;
 	int i;
 
-	md_done_sync(conf->mddev, STRIPE_SECTORS, 0);
 	clear_bit(STRIPE_SYNCING, &sh->state);
 	s->syncing = 0;
 	s->replacing = 0;
 	/* There is nothing more to do for sync/check/repair.
+	 * Don't even need to abort as that is handled elsewhere
+	 * if needed, and not always wanted e.g. if there is a known
+	 * bad block here.
 	 * For recover/replace we need to record a bad block on all
 	 * non-sync devices, or abort the recovery
 	 */
-	if (!test_bit(MD_RECOVERY_RECOVER, &conf->mddev->recovery))
-		return;
-	/* During recovery devices cannot be removed, so locking and
-	 * refcounting of rdevs is not needed
-	 */
-	for (i = 0; i < conf->raid_disks; i++) {
-		struct md_rdev *rdev = conf->disks[i].rdev;
-		if (rdev
-		    && !test_bit(Faulty, &rdev->flags)
-		    && !test_bit(In_sync, &rdev->flags)
-		    && !rdev_set_badblocks(rdev, sh->sector,
-					   STRIPE_SECTORS, 0))
-			abort = 1;
-		rdev = conf->disks[i].replacement;
-		if (rdev
-		    && !test_bit(Faulty, &rdev->flags)
-		    && !test_bit(In_sync, &rdev->flags)
-		    && !rdev_set_badblocks(rdev, sh->sector,
-					   STRIPE_SECTORS, 0))
-			abort = 1;
-	}
-	if (abort) {
-		conf->recovery_disabled = conf->mddev->recovery_disabled;
-		set_bit(MD_RECOVERY_INTR, &conf->mddev->recovery);
+	if (test_bit(MD_RECOVERY_RECOVER, &conf->mddev->recovery)) {
+		/* During recovery devices cannot be removed, so
+		 * locking and refcounting of rdevs is not needed
+		 */
+		for (i = 0; i < conf->raid_disks; i++) {
+			struct md_rdev *rdev = conf->disks[i].rdev;
+			if (rdev
+			    && !test_bit(Faulty, &rdev->flags)
+			    && !test_bit(In_sync, &rdev->flags)
+			    && !rdev_set_badblocks(rdev, sh->sector,
+						   STRIPE_SECTORS, 0))
+				abort = 1;
+			rdev = conf->disks[i].replacement;
+			if (rdev
+			    && !test_bit(Faulty, &rdev->flags)
+			    && !test_bit(In_sync, &rdev->flags)
+			    && !rdev_set_badblocks(rdev, sh->sector,
+						   STRIPE_SECTORS, 0))
+				abort = 1;
+		}
+		if (abort)
+			conf->recovery_disabled =
+				conf->mddev->recovery_disabled;
 	}
+	md_done_sync(conf->mddev, STRIPE_SECTORS, !abort);
 }
 
 static int want_replace(struct stripe_head *sh, int disk_idx)
@@ -3203,7 +3205,8 @@ static void analyse_stripe(struct stripe_head *sh, struct stripe_head_state *s)
 			/* Not in-sync */;
 		else if (is_bad) {
 			/* also not in-sync */
-			if (!test_bit(WriteErrorSeen, &rdev->flags)) {
+			if (!test_bit(WriteErrorSeen, &rdev->flags) &&
+			    test_bit(R5_UPTODATE, &sh->devs[i].flags)) {
 				/* treat as in-sync, but with a read error
 				 * which we can now try to correct
 				 */

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: md/raid5:Fix recover/replace stop if handle stipe failed
  2012-03-27  7:33     ` majianpeng
@ 2012-03-28  1:40       ` NeilBrown
  2012-03-28  1:45       ` majianpeng
  1 sibling, 0 replies; 9+ messages in thread
From: NeilBrown @ 2012-03-28  1:40 UTC (permalink / raw)
  To: majianpeng; +Cc: linux-raid

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

On Tue, 27 Mar 2012 15:33:25 +0800 "majianpeng" <majianpeng@gmail.com> wrote:

> I tested and corrected some bug.

Thanks.
I added the compile-error fix.

However I don't agree that we need to set 'abort = 1' in the non-RECOVERY
case.

As the comment that I added says:

>         /* There is nothing more to do for sync/check/repair.
> +        * Don't even need to abort as that is handled elsewhere
> +        * if needed, and not always wanted e.g. if there is a known
> +        * bad block here.

Thanks,
NeilBrown


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Re: md/raid5:Fix recover/replace stop if handle stipe failed
  2012-03-27  7:33     ` majianpeng
  2012-03-28  1:40       ` NeilBrown
@ 2012-03-28  1:45       ` majianpeng
  2012-03-28  2:14         ` NeilBrown
  1 sibling, 1 reply; 9+ messages in thread
From: majianpeng @ 2012-03-28  1:45 UTC (permalink / raw)
  To: NeilBrown; +Cc: linux-raid

>>However I don't agree that we need to set 'abort = 1' in the non-RECOVERY
>>case.
But the origial code is:
md_done_sync(conf->mddev, STRIPE_SECTORS, 0)
This mean you will interrunt the resync.

------------------				 
majianpeng
2012-03-28



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: md/raid5:Fix recover/replace stop if handle stipe failed
  2012-03-28  1:45       ` majianpeng
@ 2012-03-28  2:14         ` NeilBrown
  2012-03-28  2:25           ` majianpeng
  0 siblings, 1 reply; 9+ messages in thread
From: NeilBrown @ 2012-03-28  2:14 UTC (permalink / raw)
  To: majianpeng; +Cc: linux-raid

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

On Wed, 28 Mar 2012 09:45:40 +0800 "majianpeng" <majianpeng@gmail.com> wrote:

> >>However I don't agree that we need to set 'abort = 1' in the non-RECOVERY
> >>case.
> But the origial code is:
> md_done_sync(conf->mddev, STRIPE_SECTORS, 0)
> This mean you will interrunt the resync.

Correct.  But I now think the original code was wrong - or at least
unnecessary and addition of known-bad-blocks has made it wrong.

Thanks,
NeilBrown


> 
> ------------------				 
> majianpeng
> 2012-03-28
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Re: md/raid5:Fix recover/replace stop if handle stipe failed
  2012-03-28  2:14         ` NeilBrown
@ 2012-03-28  2:25           ` majianpeng
  0 siblings, 0 replies; 9+ messages in thread
From: majianpeng @ 2012-03-28  2:25 UTC (permalink / raw)
  To: NeilBrown; +Cc: linux-raid

I see.Thanks

------------------				 
majianpeng
2012-03-28

-------------------------------------------------------------
发件人:NeilBrown
发送日期:2012-03-28 10:15:00
收件人:majianpeng
抄送:linux-raid
主题:Re: md/raid5:Fix recover/replace stop if handle stipe failed

On Wed, 28 Mar 2012 09:45:40 +0800 "majianpeng" <majianpeng@gmail.com> wrote:

> >>However I don't agree that we need to set 'abort = 1' in the non-RECOVERY
> >>case.
> But the origial code is:
> md_done_sync(conf->mddev, STRIPE_SECTORS, 0)
> This mean you will interrunt the resync.

Correct.  But I now think the original code was wrong - or at least
unnecessary and addition of known-bad-blocks has made it wrong.

Thanks,
NeilBrown


> 
> ------------------				 
> majianpeng
> 2012-03-28
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2012-03-28  2:25 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-03-14  7:07 md/raid5:Fix recover/replace stop if handle stipe failed majianpeng
2012-03-14  7:33 ` NeilBrown
2012-03-14  9:27 ` majianpeng
2012-03-27  3:26   ` NeilBrown
2012-03-27  7:33     ` majianpeng
2012-03-28  1:40       ` NeilBrown
2012-03-28  1:45       ` majianpeng
2012-03-28  2:14         ` NeilBrown
2012-03-28  2:25           ` majianpeng

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).