md/raid1: always set MD_RECOVERY_INTR flag in raid1 error handler to avoid potential data corruption

PATCH In review jiao hui 2 participants 5 messages Jul 28, 2014 – Jul 30, 2014 MD RAID

Share & tools

jiao hui

author

2014-07-28 08:09 UTC #0 lore raw

PATCH 1/1 md/raid1: always set MD_RECOVERY_INTR flag in raid1 error handler to avoid potential data corruption

From 1fdbfb8552c00af55d11d7a63cdafbdf1749ff63 Mon Sep 17 00:00:00 2001
From: Jiao Hui <[email protected]>
Date: Mon, 28 Jul 2014 11:57:20 +0800
Subject: [PATCH] md/raid1: always set MD_RECOVERY_INTR flag in raid1 error handler to avoid potential data corruption

In the recovery of raid1 with bitmap, if a bitmap bit has a NEEDED or RESYNC flag, actual resync io will happen. The sync_thread check each rdev, if any rdev is missing or has a FAULTY flag, the array is still_degraded, then the bitmap bit NEEDED flag not cleared. Otherwise, we cleared NEEDED flag and set RESYNC flag. The RESYNC flag cleared in bitmap_cond_end_sync or bitmap_close_sync.

If the only disk which is being recovered fails again when raid1 recovery is in progress. The resync_thread can't find a non-In_sync disk to write, then the remaining recovery skipped. RAID1 error handler only set MD_RECOVERY_INTR flag when a In_sync disk fails. But the disk being reocvered is non-In_sync, then md_do_sync can't got the INTR singal to break, and the mddev->curr_resync is uptodated to max_sectors (mddev->dev_sectors). When raid1 personality tries to finish resync process, no bitmap bit with RESYNC flag can set back to NEEDED flag, and bitmap_close_sync clear the RESYNC flag. When the disk is added back, the area from the offset of last recovery to the end of bitmap-chunk is skipped by resync_thread forever.

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

---
 drivers/md/raid1.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

drivers/md/raid1.c +4 -4

index aacf6bf..51d06eb 100644
@@ -1391,16 +1391,16 @@ static void error(struct mddev *mddev, struct md_rdev *rdev)
 		return;
 	}
 	set_bit(Blocked, &rdev->flags);
+	/*
+	 * if recovery is running, make sure it aborts.
+	 */
+	set_bit(MD_RECOVERY_INTR, &mddev->recovery);
 	if (test_and_clear_bit(In_sync, &rdev->flags)) {
 		unsigned long flags;
 		spin_lock_irqsave(&conf->device_lock, flags);
 		mddev->degraded++;
 		set_bit(Faulty, &rdev->flags);
 		spin_unlock_irqrestore(&conf->device_lock, flags);
-		/*
-		 * if recovery is running, make sure it aborts.
-		 */
-		set_bit(MD_RECOVERY_INTR, &mddev->recovery);
 	} else
 		set_bit(Faulty, &rdev->flags);
 	set_bit(MD_CHANGE_DEVS, &mddev->flags);

Signature · 1 line

1.8.3.1

jiao hui

author

2014-07-28 08:23 UTC #1 lore raw

I reproduce this issue almost each time with a raid1 with considerable large bitmap-chunk， such as 64MB。
I make this patch on Centos7.0。
Any comments are welcome.

Quoted 56 lines · jiao hui

From 1fdbfb8552c00af55d11d7a63cdafbdf1749ff63 Mon Sep 17 00:00:00 2001
From: Jiao Hui <[email protected]>
Date: Mon, 28 Jul 2014 11:57:20 +0800
Subject: [PATCH] md/raid1: always set MD_RECOVERY_INTR flag in raid1 error handler to avoid potential data corruption
In the recovery of raid1 with bitmap, if a bitmap bit has a NEEDED or RESYNC flag, actual resync io will happen. The sync_thread check each rdev, if any rdev is missing or has a FAULTY flag, the array is still_degraded, then the bitmap bit NEEDED flag not cleared. Otherwise, we cleared NEEDED flag and set RESYNC flag. The RESYNC flag cleared in bitmap_cond_end_sync or bitmap_close_sync.
If the only disk which is being recovered fails again when raid1 recovery is in progress. The resync_thread can't find a non-In_sync disk to write, then the remaining recovery skipped. RAID1 error handler only set MD_RECOVERY_INTR flag when a In_sync disk fails. But the disk being reocvered is non-In_sync, then md_do_sync can't got the INTR singal to break, and the mddev->curr_resync is uptodated to max_sectors (mddev->dev_sectors). When raid1 personality tries to finish resync process, no bitmap bit with RESYNC flag can set back to NEEDED flag, and bitmap_close_sync clear the RESYNC flag. When the disk is added back, the area from the offset of last recovery to the end of bitmap-chunk is skipped by resync_thread forever.
Signed-off-by JiaoHui <[email protected]>
---
 drivers/md/raid1.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/md/raid1.c +4 -5
index aacf6bf..51d06eb 100644
@@ -1391,16 +1391,16 @@ static void error(struct mddev *mddev, struct md_rdev *rdev)
                return;
        }
        set_bit(Blocked, &rdev->flags);
+       /*
+        * if recovery is running, make sure it aborts.
+        */
+       set_bit(MD_RECOVERY_INTR, &mddev->recovery);
        if (test_and_clear_bit(In_sync, &rdev->flags)) {
                unsigned long flags;
                spin_lock_irqsave(&conf->device_lock, flags);
                mddev->degraded++;
                set_bit(Faulty, &rdev->flags);
                spin_unlock_irqrestore(&conf->device_lock, flags);
-               /*
-                * if recovery is running, make sure it aborts.
-                */
-               set_bit(MD_RECOVERY_INTR, &mddev->recovery);
        } else
                set_bit(Faulty, &rdev->flags);
        set_bit(MD_CHANGE_DEVS, &mddev->flags);
--
1.8.3.1

Quoted 4 lines

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

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

NeilBrown

2014-07-29 02:44 UTC #2 lore raw

In reply to jiao hui

Quoted 41 lines · On Mon, 28 Jul 2014 16:09:33 +0800 jiao hui <[email protected]> wrote:

Quoted 1 line
From 1fdbfb8552c00af55d11d7a63cdafbdf1749ff63 Mon Sep 17 00:00:00 2001
From: Jiao Hui <[email protected]>
Date: Mon, 28 Jul 2014 11:57:20 +0800
Subject: [PATCH] md/raid1: always set MD_RECOVERY_INTR flag in raid1 error handler to avoid potential data corruption
In the recovery of raid1 with bitmap, if a bitmap bit has a NEEDED or RESYNC flag, actual resync io will happen. The sync_thread check each rdev, if any rdev is missing or has a FAULTY flag, the array is still_degraded, then the bitmap bit NEEDED flag not cleared. Otherwise, we cleared NEEDED flag and set RESYNC flag. The RESYNC flag cleared in bitmap_cond_end_sync or bitmap_close_sync.
If the only disk which is being recovered fails again when raid1 recovery is in progress. The resync_thread can't find a non-In_sync disk to write, then the remaining recovery skipped. RAID1 error handler only set MD_RECOVERY_INTR flag when a In_sync disk fails. But the disk being reocvered is non-In_sync, then md_do_sync can't got the INTR singal to break, and the mddev->curr_resync is uptodated to max_sectors (mddev->dev_sectors). When raid1 personality tries to finish resync process, no bitmap bit with RESYNC flag can set back to NEEDED flag, and bitmap_close_sync clear the RESYNC flag. When the disk is added back, the area from the offset of last recovery to the end of bitmap-chunk is skipped by resync_thread forever.
Signed-off-by JiaoHui <[email protected]>
---
 drivers/md/raid1.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/md/raid1.c +4 -0
index aacf6bf..51d06eb 100644
@@ -1391,16 +1391,16 @@ static void error(struct mddev *mddev, struct md_rdev *rdev)
 		return;
 	}
 	set_bit(Blocked, &rdev->flags);
+	/*
+	 * if recovery is running, make sure it aborts.
+	 */
+	set_bit(MD_RECOVERY_INTR, &mddev->recovery);
 	if (test_and_clear_bit(In_sync, &rdev->flags)) {
 		unsigned long flags;
 		spin_lock_irqsave(&conf->device_lock, flags);

drivers/md/raid1.c static void error(struct mddev *mddev, struct md_rdev *rdev) · line 1406

Quoted 10 lines

drivers/md/raid1.c +0 -4

 		mddev->degraded++;
 		set_bit(Faulty, &rdev->flags);
 		spin_unlock_irqrestore(&conf->device_lock, flags);
-		/*
-		 * if recovery is running, make sure it aborts.
-		 */
-		set_bit(MD_RECOVERY_INTR, &mddev->recovery);
 	} else
 		set_bit(Faulty, &rdev->flags);
 	set_bit(MD_CHANGE_DEVS, &mddev->flags);

Hi,
thanks for the report and the patch.

If the recovery process gets a write error it will abort the current bitmap region by calling bitmap_end_sync() in end_sync_write().
However you are talking about a different situation where a normal IO write gets and error and fails a drive. Then the recovery aborts without aborting the current bitmap region.

I think I would rather fix the bug by calling end_sync_write() at the place where the recovery decides to abort, as in the following patch. Would you be able to test it please and confirm that it works?

A similar fix will probably be needed for raid10.

Thanks,
NeilBrown

drivers/md/raid1.c +3 -1

index 56e24c072b62..4f007a410f4b 100644
@@ -2668,9 +2668,11 @@ static sector_t sync_request(struct mddev *mddev, sector_t sector_nr, int *skipp
 
 	if (write_targets == 0 || read_targets == 0) {
 		/* There is nowhere to write, so all non-sync
-		 * drives must be failed - so we are finished
+		 * drives must be failed - so we are finished.
+		 * But abort the current bitmap region though.
 		 */
 		sector_t rv;
+		bitmap_end_sync(mddev->bitmap, sector_nr, &sync_blocks, 1);
 		if (min_bad > 0)
 			max_sector = sector_nr + min_bad;
 		rv = max_sector - sector_nr;

jiao hui

author

2014-07-29 06:50 UTC #3 lore raw

Hi neil,

The patch works. I test it on Centos 7.0 for fifty rounds， no consistency issue found。

Best Regards.
jiaohui

Quoted 80 lines · NeilBrown

Quoted 41 lines · On Mon, 28 Jul 2014 16:09:33 +0800 jiao hui <[email protected]> wrote:
Quoted 1 line
From 1fdbfb8552c00af55d11d7a63cdafbdf1749ff63 Mon Sep 17 00:00:00 2001
From: Jiao Hui <[email protected]>
Date: Mon, 28 Jul 2014 11:57:20 +0800
Subject: [PATCH] md/raid1: always set MD_RECOVERY_INTR flag in raid1 error handler to avoid potential data corruption
In the recovery of raid1 with bitmap, if a bitmap bit has a NEEDED or RESYNC flag, actual resync io will happen. The sync_thread check each rdev, if any rdev is missing or has a FAULTY flag, the array is still_degraded, then the bitmap bit NEEDED flag not cleared. Otherwise, we cleared NEEDED flag and set RESYNC flag. The RESYNC flag cleared in bitmap_cond_end_sync or bitmap_close_sync.
If the only disk which is being recovered fails again when raid1 recovery is in progress. The resync_thread can't find a non-In_sync disk to write, then the remaining recovery skipped. RAID1 error handler only set MD_RECOVERY_INTR flag when a In_sync disk fails. But the disk being reocvered is non-In_sync, then md_do_sync can't got the INTR singal to break, and the mddev->curr_resync is uptodated to max_sectors (mddev->dev_sectors). When raid1 personality tries to finish resync process, no bitmap bit with RESYNC flag can set back to NEEDED flag, and bitmap_close_sync clear the RESYNC flag. When the disk is added back, the area from the offset of last recovery to the end of bitmap-chunk is skipped by resync_thread forever.
Signed-off-by JiaoHui <[email protected]>
---
 drivers/md/raid1.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/md/raid1.c +4 -0
index aacf6bf..51d06eb 100644
@@ -1391,16 +1391,16 @@ static void error(struct mddev *mddev, struct md_rdev *rdev)
              return;
      }
      set_bit(Blocked, &rdev->flags);
+     /*
+      * if recovery is running, make sure it aborts.
+      */
+     set_bit(MD_RECOVERY_INTR, &mddev->recovery);
      if (test_and_clear_bit(In_sync, &rdev->flags)) {
              unsigned long flags;
              spin_lock_irqsave(&conf->device_lock, flags);
drivers/md/raid1.c static void error(struct mddev *mddev, struct md_rdev *rdev) · line 1406
Quoted 10 lines
drivers/md/raid1.c +0 -4
              mddev->degraded++;
              set_bit(Faulty, &rdev->flags);
              spin_unlock_irqrestore(&conf->device_lock, flags);
-             /*
-              * if recovery is running, make sure it aborts.
-              */
-             set_bit(MD_RECOVERY_INTR, &mddev->recovery);
      } else
              set_bit(Faulty, &rdev->flags);
      set_bit(MD_CHANGE_DEVS, &mddev->flags);
Hi,
thanks for the report and the patch.
If the recovery process gets a write error it will abort the current bitmap region by calling bitmap_end_sync() in end_sync_write().
However you are talking about a different situation where a normal IO write gets and error and fails a drive. Then the recovery aborts without aborting the current bitmap region.
I think I would rather fix the bug by calling end_sync_write() at the place where the recovery decides to abort, as in the following patch. Would you be able to test it please and confirm that it works?
A similar fix will probably be needed for raid10.
Thanks,
NeilBrown
drivers/md/raid1.c +0 -0
index 56e24c072b62..4f007a410f4b 100644
@@ -2668,9 +2668,11 @@ static sector_t sync_request(struct mddev *mddev, sector_t sector_nr, int *skipp
 
        if (write_targets == 0 || read_targets == 0) {

drivers/md/raid1.c static sector_t sync_request(struct mddev *mddev, sector_t sector_nr, int *skipp · line 2678

Quoted 10 lines

drivers/md/raid1.c +3 -1

                /* There is nowhere to write, so all non-sync
-                * drives must be failed - so we are finished
+                * drives must be failed - so we are finished.
+                * But abort the current bitmap region though.
                 */
                sector_t rv;
+               bitmap_end_sync(mddev->bitmap, sector_nr, &sync_blocks, 1);
                if (min_bad > 0)
                        max_sector = sector_nr + min_bad;
                rv = max_sector - sector_nr;

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

NeilBrown

2014-07-30 03:39 UTC #4 lore raw

Quoted 8 lines · On Tue, 29 Jul 2014 14:50:04 +0800 jiao hui <[email protected]> wrote:

Hi neil,
The patch works. I test it on Centos 7.0 for fifty rounds， no consistency issue found。
Best Regards.
jiaohui

Thanks for testing.

I looked again and compared with raid10 and decided that your fix actually was better. If you are recovering two drives at once and only one fails, you patch will do the right thing, but mine won't.

I'll be submitting the following.

Thanks,
NeilBrown

From b628438e59827e710df20c27fea680cbe1870272 Mon Sep 17 00:00:00 2001
From: NeilBrown <[email protected]>
Date: Wed, 30 Jul 2014 13:24:50 +1000
Subject: [PATCH] md/raid1,raid10: always abort recover on write error.

Currently we don't abort recovery on a write error if the write error to the recovering device was triggerd by normal IO (as opposed to recovery IO).

This means that for one bitmap region, the recovery might write to the recovering device for a few sectors, then not bother for subsequent sectors (as it never writes to failed devices). In this case the bitmap bit will be cleared, but it really shouldn't.

The result is that if the recovering device fails and is then re-added (after fixing whatever hardware problem triggerred the failure), the second recovery won't redo the region it was in the middle of, so some of the device will not be recovered properly.

If we abort the recovery, the region being processes will be cancelled (bit not cleared) and the whole region will be retried.

As the bug can result in data corruption the patch is suitable for -stable. For kernels prior to 3.11 there is a conflict in raid10.c which will require care.

Original-from: jiao hui <[email protected]>

Reported-and-tested-by jiao hui <[email protected]>

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

Cc [email protected]

drivers/md/raid1.c +4 -4

index 56e24c072b62..d7690f86fdb9 100644
@@ -1501,12 +1501,12 @@ static void error(struct mddev *mddev, struct md_rdev *rdev)
 		mddev->degraded++;
 		set_bit(Faulty, &rdev->flags);
 		spin_unlock_irqrestore(&conf->device_lock, flags);
-		/*
-		 * if recovery is running, make sure it aborts.
-		 */
-		set_bit(MD_RECOVERY_INTR, &mddev->recovery);
 	} else
 		set_bit(Faulty, &rdev->flags);
+	/*
+	 * if recovery is running, make sure it aborts.
+	 */
+	set_bit(MD_RECOVERY_INTR, &mddev->recovery);
 	set_bit(MD_CHANGE_DEVS, &mddev->flags);
 	printk(KERN_ALERT
 	       "md/raid1:%s: Disk failure on %s, disabling device.\n"

drivers/md/raid10.c +5 -6

index cb882aae9e20..b08c18871323 100644
@@ -1684,13 +1684,12 @@ static void error(struct mddev *mddev, struct md_rdev *rdev)
 		spin_unlock_irqrestore(&conf->device_lock, flags);
 		return;
 	}
-	if (test_and_clear_bit(In_sync, &rdev->flags)) {
+	if (test_and_clear_bit(In_sync, &rdev->flags))
 		mddev->degraded++;
-			/*
-		 * if recovery is running, make sure it aborts.
-		 */
-		set_bit(MD_RECOVERY_INTR, &mddev->recovery);
-	}
+	/*
+	 * If recovery is running, make sure it aborts.
+	 */
+	set_bit(MD_RECOVERY_INTR, &mddev->recovery);
 	set_bit(Blocked, &rdev->flags);
 	set_bit(Faulty, &rdev->flags);
 	set_bit(MD_CHANGE_DEVS, &mddev->flags);