Re: [PATCH] md/raid1: always set MD_RECOVERY_INTR flag in raid1 error handler to avoid potential data corruption
From: NeilBrown <hidden>
Date: 2014-07-29 02:44:15
Subsystem:
software raid (multiple disks) support, the rest · Maintainers:
Song Liu, Yu Kuai, Linus Torvalds
On Mon, 28 Jul 2014 16:09:33 +0800 jiao hui [off-list ref] wrote:
quoted hunk ↗ jump to hunk
quoted
From 1fdbfb8552c00af55d11d7a63cdafbdf1749ff63 Mon Sep 17 00:00:00 2001From: Jiao Hui <redacted> 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 [off-list ref] --- drivers/md/raid1.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c index aacf6bf..51d06eb 100644 --- a/drivers/md/raid1.c +++ b/drivers/md/raid1.c@@ -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);
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
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 56e24c072b62..4f007a410f4b 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c@@ -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;
Attachments
- signature.asc [application/pgp-signature] 828 bytes