md/raid5:Fix recover/replace stop if handle stipe failed

PATCH majianpeng 2 participants 9 messages Mar 14, 2012 – Mar 28, 2012 MD RAID

Share & tools

majianpeng

author

2012-03-14 07:07 UTC #0 lore raw

From 849df9f6422972452b99a2c2d08d005437a52d72 Mon Sep 17 00:00:00 2001
From: majianpeng <[email protected]>
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 <[email protected]>

---
 drivers/md/raid5.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

drivers/md/raid5.c +6 -2

index 360f2b9..55193ef 100644
@@ -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);
 	}
 }

Signature · 6 lines

1.7.5.4

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

NeilBrown

2012-03-14 07:33 UTC #1 lore raw

Quoted 9 lines · On Wed, 14 Mar 2012 15:07:55 +0800 "majianpeng" <[email protected]> wrote:

Quoted 1 line
From 849df9f6422972452b99a2c2d08d005437a52d72 Mon Sep 17 00:00:00 2001
From: majianpeng <[email protected]>
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.

Quoted 34 lines

Signed-off-by majianpeng <[email protected]>

---
 drivers/md/raid5.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

drivers/md/raid5.c +5 -2

index 360f2b9..55193ef 100644
@@ -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
 	 */

drivers/md/raid5.c handle_failed_sync(struct r5conf *conf, struct stripe_head *sh, · line 2510

Quoted 4 lines

drivers/md/raid5.c +1 -0

@@ -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

Quoted 3 lines

drivers/md/raid5.c +0 -0
 	}
 }
 

majianpeng

author

2012-03-14 09:27 UTC #2 lore raw

In reply to majianpeng

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

Quoted 9 lines · On Wed, 14 Mar 2012 15:07:55 +0800 "majianpeng" <[email protected]> wrote:

Quoted 1 line
From 849df9f6422972452b99a2c2d08d005437a52d72 Mon Sep 17 00:00:00 2001
From: majianpeng <[email protected]>
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.

Quoted 34 lines

Signed-off-by majianpeng <[email protected]>

---
 drivers/md/raid5.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

drivers/md/raid5.c +5 -2

index 360f2b9..55193ef 100644
@@ -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
 	 */

drivers/md/raid5.c handle_failed_sync(struct r5conf *conf, struct stripe_head *sh, · line 2510

Quoted 4 lines

drivers/md/raid5.c +1 -0

@@ -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

Quoted 3 lines

drivers/md/raid5.c +0 -0
 	}
 }
 

NeilBrown

2012-03-27 03:26 UTC #3 lore raw

Quoted 5 lines · On Wed, 14 Mar 2012 17:27:44 +0800 "majianpeng" <[email protected]> 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

drivers/md/raid5.c +29 ~~-26~~

index 23ac880..2186e0e 100644
@@ -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
 				 */

majianpeng

author

2012-03-27 07:33 UTC #4 lore raw

I tested and corrected some bug.

drivers/md/raid5.c +31 ~~-27~~

index 23ac880..f8480f8 100644
@@ -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

Quoted 5 lines · On Wed, 14 Mar 2012 17:27:44 +0800 "majianpeng" <[email protected]> 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

drivers/md/raid5.c +29 ~~-26~~

75 lines not on this page · raw message

NeilBrown

2012-03-28 01:40 UTC #5 lore raw

Quoted 1 line · On Tue, 27 Mar 2012 15:33:25 +0800 "majianpeng" <[email protected]> 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:

drivers/md/raid5.c handle_failed_sync(struct r5conf *conf, struct stripe_head *sh, · line 2480

Quoted 4 lines

        /* There is nothing more to do for sync/check/repair.

drivers/md/raid5.c +3 -0

+        * 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

majianpeng

author

2012-03-28 01:45 UTC #6 lore raw

In reply to majianpeng

Quoted 2 lines

Quoted 2 lines
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

NeilBrown

2012-03-28 02:14 UTC #7 lore raw

Quoted 5 lines · On Wed, 28 Mar 2012 09:45:40 +0800 "majianpeng" <[email protected]> wrote:

Quoted 2 lines
Quoted 2 lines
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

Quoted 10 lines

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

majianpeng

author

2012-03-28 02:25 UTC #8 lore raw

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

Quoted 5 lines · On Wed, 28 Mar 2012 09:45:40 +0800 "majianpeng" <[email protected]> wrote:

Quoted 2 lines
Quoted 2 lines
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

Quoted 10 lines

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