Re: md: raid5 resync corrects read errors on data block - is this correct?
flat view
From: NeilBrown <hidden>
Date: 2012-09-25 06:57:02
On Thu, 20 Sep 2012 11:26:50 +0300 Alexander Lyakas [off-list ref] wrote:
Hi Neil, you are completely right. I got confused between mddev->recovery_cp and sb->resync_offset; the latter may become 0 due to in-flight WRITEs and not due to resync. Looking at the code again, I see that recovery_cp is totally one-way from sb->resync_offset to MaxSector (except for explicit loading via sysfs). Also recovery_cp is not relevant to "check" and "repair". So recovery_cp is pretty simple after all. Below is V2 patch. (I have also to credit it to somebody else, because he was the one that said - just do rcw while you are resyncing). Thanks, Alex. -----------------quoted
From cc3e2bfcf2fd2c69180577949425d69de88706bb Mon Sep 17 00:00:00 2001From: Alex Lyakas <redacted> Date: Thu, 13 Sep 2012 18:55:00 +0300 Subject: [PATCH] When RAID5 is dirty, force reconstruct-write instead of read-modify-write. Signed-off-by: Alex Lyakas <redacted> Signed-off-by: Yair Hershko <redacted>
Signed-off-by has a very specific meaning - it isn't just a way of giving recredit. If Yair wrote some of the code, this is fine. If not, then something like "Suggest-by:" might be more appropriate. Should I change it to that. applied, thanks. NeilBrown
quoted hunk ↗ jump to hunk
diff --git a/ubuntu_kmodules/Ubuntu-3.2.0-25.40/drivers/md/raid5.cb/ubuntu_kmodules/Ubuntu-3.2.0-25.40/drivers/md/raid5.c index 5332202..9fdd5e3 100644--- a/ubuntu_kmodules/Ubuntu-3.2.0-25.40/drivers/md/raid5.c +++ b/ubuntu_kmodules/Ubuntu-3.2.0-25.40/drivers/md/raid5.c@@ -2555,12 +2555,24 @@ static void handle_stripe_dirtying(struct r5conf *conf, int disks) { int rmw = 0, rcw = 0, i; - if (conf->max_degraded == 2) { - /* RAID6 requires 'rcw' in current implementation - * Calculate the real rcw later - for now fake it + sector_t recovery_cp = conf->mddev->recovery_cp; + + /* RAID6 requires 'rcw' in current implementation. + * Otherwise, check whether resync is now happening or should start. + * If yes, then the array is dirty (after unclean shutdown or + * initial creation), so parity in some stripes might be inconsistent. + * In this case, we need to always do reconstruct-write, to ensure + * that in case of drive failure or read-error correction, we + * generate correct data from the parity. + */ + if (conf->max_degraded == 2 || + (recovery_cp < MaxSector && sh->sector >= recovery_cp)) { + /* Calculate the real rcw later - for now make it * look like rcw is cheaper */ rcw = 1; rmw = 2; + pr_debug("force RCW max_degraded=%u, recovery_cp=%lush->sector=%lu\n", + conf->max_degraded, recovery_cp, sh->sector); } else for (i = disks; i--; ) { /* would I have to read this buffer for read_modify_write */ struct r5dev *dev = &sh->dev[i]; On Wed, Sep 19, 2012 at 8:59 AM, NeilBrown [off-list ref] wrote:quoted
On Mon, 17 Sep 2012 14:15:16 +0300 Alexander Lyakas [off-list ref] wrote:quoted
Hi Neil, below is a bit less-ugly version of the patch. Thanks, Alex.quoted
From 05cf800d623bf558c99d542cf8bf083c85b7e5d5 Mon Sep 17 00:00:00 2001From: Alex Lyakas <redacted> Date: Thu, 13 Sep 2012 18:55:00 +0300 Subject: [PATCH] When RAID5 is dirty, force reconstruct-write instead of read-modify-write. Signed-off-by: Alex Lyakas <redacted> Signed-off-by: Yair Hershko <redacted>diff --git a/ubuntu_kmodules/Ubuntu-3.2.0-25.40/drivers/md/raid5.cb/ubuntu_kmodules/Ubuntu-3.2.0-25.40/drivers/md/raid5.c index 5332202..0702785 100644--- a/ubuntu_kmodules/Ubuntu-3.2.0-25.40/drivers/md/raid5.c +++ b/ubuntu_kmodules/Ubuntu-3.2.0-25.40/drivers/md/raid5.c@@ -2555,12 +2555,36 @@ static void handle_stripe_dirtying(struct r5conf *conf, int disks) { int rmw = 0, rcw = 0, i; - if (conf->max_degraded == 2) { - /* RAID6 requires 'rcw' in current implementation - * Calculate the real rcw later - for now fake it + sector_t recovery_cp = conf->mddev->recovery_cp; + unsigned long recovery = conf->mddev->recovery; + int needed = test_bit(MD_RECOVERY_NEEDED, &recovery); + int resyncing = test_bit(MD_RECOVERY_SYNC, &recovery) && + !test_bit(MD_RECOVERY_REQUESTED, &recovery) && + !test_bit(MD_RECOVERY_CHECK, &recovery); + int transitional = test_bit(MD_RECOVERY_RUNNING, &recovery) && + !test_bit(MD_RECOVERY_SYNC, &recovery) && + !test_bit(MD_RECOVERY_RECOVER, &recovery) && + !test_bit(MD_RECOVERY_DONE, &recovery) && + !test_bit(MD_RECOVERY_RESHAPE, &recovery);Thanks Alex, however I don't understand why you want to test all of these bits. Isn't it enough just to check ->recovery_cp ??quoted
+ + /* RAID6 requires 'rcw' in current implementation. + * Otherwise, attempt to check whether resync is now happening + * or should start. + * If yes, then the array is dirty (after unclean shutdown or + * initial creation), so parity in some stripes might be inconsistent. + * In this case, we need to always do reconstruct-write, to ensure + * that in case of drive failure or read-error correction, we + * generate correct data from the parity. + */ + if (conf->max_degraded == 2 || + (recovery_cp < MaxSector && sh->sector >= recovery_cp && + (needed || resyncing || transitional))) { + /* Calculate the real rcw later - for now fake it * look like rcw is cheaperAlso, we should probably fix this comment. s/fake/make/ Thanks, NeilBrownquoted
*/ rcw = 1; rmw = 2; + pr_debug("force RCW max_degraded=%u, recovery_cp=%lu sh->sector=%lu recovery=0x%lx\n", + conf->max_degraded, recovery_cp, sh->sector, recovery); } else for (i = disks; i--; ) { /* would I have to read this buffer for read_modify_write */ struct r5dev *dev = &sh->dev[i];
Attachments
- signature.asc [application/pgp-signature] 828 bytes