Re: [PATCH 2/2 v2] md: set write-intent bit first in sync-bitmap for rwm in non-insync region.

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 6693 bytes --]

On Mon, 22 Jul 2013 14:07:51 +0800 Robin Dong <robin.k.dong@gmail.com> wrote:

> From: Robin Dong <sanbai@taobao.com>
> 
> 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 <sanbai@taobao.com>
> Cc: NeilBrown <neilb@suse.de>
> ---
>  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?


>  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



> +
>  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);


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]