Re: [PATCH 2/2 v2] md: set write-intent bit first in sync-bitmap for rwm in non-insync region.
From: NeilBrown <hidden>
Date: 2013-07-23 05:36:21
On Mon, 22 Jul 2013 14:07:51 +0800 Robin Dong [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: Robin Dong <redacted> If part of the RAID456 array is not in sync, then a read-modify-write cycle will update incorrect parity to new incorrect parity. If you subsequently get a drive failure, any data on that drive in the not-in-sync region will be lost. To avoid this, we: 1. In first bitmap_endwrite to a not-in-sync region, set the write-intent bit and wake up resync thread. 2. When resync finishes, set the sync bit and clear write-intent bit. Signed-off-by: Robin Dong <redacted> Cc: NeilBrown <redacted> --- drivers/md/bitmap.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++++- drivers/md/bitmap.h | 2 ++ drivers/md/raid5.c | 11 +++++++++++ 3 files changed, 63 insertions(+), 1 deletions(-)diff --git a/drivers/md/bitmap.c b/drivers/md/bitmap.c index 0d894af..b4967c4 100644 --- a/drivers/md/bitmap.c +++ b/drivers/md/bitmap.c@@ -1447,6 +1447,24 @@ int bitmap_startwrite(struct bitmap *bitmap, sector_t offset, unsigned long sect } EXPORT_SYMBOL(bitmap_startwrite); +void syncbitmap_endwrite(struct bitmap *bitmap, sector_t offset, + bitmap_counter_t *bmc) +{ + struct mddev *mddev = bitmap->mddev; + + if (mddev->level >= 4 && + offset > mddev->curr_resync_completed) { + *bmc |= NEEDED_MASK; + if (!test_bit(MD_RECOVERY_RUNNING, &mddev->recovery)) { + set_bit(MD_RECOVERY_NEEDED, &mddev->recovery); + set_bit(MD_RECOVERY_SYNC, &mddev->recovery); + md_wakeup_thread(mddev->sync_thread); + sysfs_notify_dirent_safe(mddev->sysfs_action); + } + } else + syncbitmap_file_set_bit(bitmap, offset); +} +
I don't like the test for "level >= 4" here. This would cause RAID10 to be treated differently to RAID1 and that isn't right. Can we just have the raid personality pass a flag (or maybe set a flag in bitmap_info) to say if it wants auto-resync or not?
quoted hunk ↗ jump to hunk
void bitmap_endwrite(struct bitmap *bitmap, sector_t offset, unsigned long sectors, int success, int behind) {@@ -1479,7 +1497,8 @@ void bitmap_endwrite(struct bitmap *bitmap, sector_t offset, unsigned long secto sysfs_notify_dirent_safe(bitmap->sysfs_can_clear); } - syncbitmap_file_set_bit(bitmap, offset); + if (bitmap->mddev->bitmap_info.sync_bitmap) + syncbitmap_endwrite(bitmap, offset, bmc); if (!success && !NEEDED(*bmc)) *bmc |= NEEDED_MASK;@@ -1502,6 +1521,36 @@ void bitmap_endwrite(struct bitmap *bitmap, sector_t offset, unsigned long secto } EXPORT_SYMBOL(bitmap_endwrite); +void bitmap_in_synced(struct bitmap *bitmap, sector_t offset, + unsigned long sectors) +{ + while (sectors) { + sector_t blocks; + unsigned long flags; + bitmap_counter_t *bmc; + + spin_lock_irqsave(&bitmap->counts.lock, flags); + bmc = bitmap_get_counter(&bitmap->counts, offset, &blocks, 0); + if (!bmc) { + spin_unlock_irqrestore(&bitmap->counts.lock, flags); + return; + } + + if (NEEDED(*bmc) || (!NEEDED(*bmc) && RESYNC(*bmc))) { + *bmc &= ~NEEDED_MASK; + syncbitmap_file_set_bit(bitmap, offset); + } + + spin_unlock_irqrestore(&bitmap->counts.lock, flags); + offset += blocks; + if (sectors > blocks) + sectors -= blocks; + else + sectors = 0; + } +} +EXPORT_SYMBOL(bitmap_in_synced);
This function is confusing me. (maybe a comment at the top to explain it
would help???)
I think to get clarity we need a clear statement of which bitmap has priority.
I would expect:
write-intent sync bitmap meaning
0 0 chunk is unused. No data here.
0 1 chunk is clean and in sync. data is safe.
1 0 Data might be here, full sync is needed.
1 1 Data is here, full sync is needed.
So the syncbitmap bit can safely be set when the write-intent bit is set, but
must be set before it is cleared.
So I would always set the syncbitmap bit when setting the write-intent bit
(so the "1 0" row never exists). For RAID5 if the syncbitmap bit wasn't set,
set set the NEEDED_MASK at the same time and trigger a resync. Then the
write-intent bit will not get cleared until the resync has happened.
So this special clearing of the NEEDED_MASK should not be needed.
Does that make sense?
Thanks,
NeilBrown
quoted hunk ↗ jump to hunk
+ static int __bitmap_start_sync(struct bitmap *bitmap, sector_t offset, sector_t *blocks, int degraded) {diff --git a/drivers/md/bitmap.h b/drivers/md/bitmap.h index ddc30da..6d22d8b 100644 --- a/drivers/md/bitmap.h +++ b/drivers/md/bitmap.h@@ -253,6 +253,8 @@ int bitmap_startwrite(struct bitmap *bitmap, sector_t offset, unsigned long sectors, int behind); void bitmap_endwrite(struct bitmap *bitmap, sector_t offset, unsigned long sectors, int success, int behind); +void bitmap_in_synced(struct bitmap *bitmap, sector_t offset, + unsigned long sectors); int bitmap_start_sync(struct bitmap *bitmap, sector_t offset, sector_t *blocks, int degraded); void bitmap_end_sync(struct bitmap *bitmap, sector_t offset, sector_t *blocks, int aborted);diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c index 2bf094a..bb7de94 100644 --- a/drivers/md/raid5.c +++ b/drivers/md/raid5.c@@ -3622,6 +3622,8 @@ static void handle_stripe(struct stripe_head *sh) if ((s.syncing || s.replacing) && s.locked == 0 && test_bit(STRIPE_INSYNC, &sh->state)) { md_done_sync(conf->mddev, STRIPE_SECTORS, 1); + bitmap_in_synced(conf->mddev->bitmap, sh->sector, + STRIPE_SECTORS); clear_bit(STRIPE_SYNCING, &sh->state); if (test_and_clear_bit(R5_Overlap, &sh->dev[sh->pd_idx].flags)) wake_up(&conf->wait_for_overlap);@@ -4690,6 +4692,15 @@ static inline sector_t sync_request(struct mddev *mddev, sector_t sector_nr, int return sync_blocks * STRIPE_SECTORS; /* keep things rounded to whole stripes */ } + if (!test_bit(MD_RECOVERY_REQUESTED, &mddev->recovery) && + conf->fullsync && + !syncbitmap_start_sync(mddev->bitmap, sector_nr, &sync_blocks) && + sync_blocks >= STRIPE_SECTORS) { + sync_blocks /= STRIPE_SECTORS; + *skipped = 1; + return sync_blocks * STRIPE_SECTORS; + } + bitmap_cond_end_sync(mddev->bitmap, sector_nr); sh = get_active_stripe(conf, sector_nr, 0, 1, 0);
Attachments
- signature.asc [application/pgp-signature] 828 bytes