Re: [PATCH] DM RAID: Fix for ineffective "sync" directive

[NeilBrown <neilb@suse.de>]

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

On Thu, 23 Aug 2012 10:31:54 -0500 Jonathan Brassow <jbrassow@redhat.com>
wrote:

> Neil,
> 
> There are two ways I see to fix the problem of the "sync" table argument
> not causing the array to re-sync.  We could set mddev->events to
> UINT_MAX and force an mddev/bitmap event mismatch - causing the bitmap
> code to set BITMAP_STALE itself.  Otherwise, we can do as I have below
> and set BITMAP_STALE ourselves.  The real difference is whether this is
> done before or after md_run().  The first solution can be done before
> md_run(); the second cannot because the bitmap is allocated during
> md_run().  Either solution works and correctly sets the bitmap to stale
> before bitmap_load() is called.  So, rather than having to think about
> issues that come with setting events to UINT_MAX, I chose to set the
> bitmap stale after md_run().

I'm not at all sure this is the right way to look at the issue.

The only reason that we have 'sync' and 'nosync' table args is because
originally we weren't storing any metadata on disk.  The 'in-sync?' state of
the array is important metadata, so it needed to be given to md via a table
option.

If you have a bitmap, then you are storing this information in metadata, so
including it as a table option is redundant and - as you noticed - ignored.

If you want to assert that the data in the bitmap is incorrect, then I think
the best approach is to change it so that it is correct.
i.e. if you want to assert that an array is not in-sync and should be fully
'synced', then the right thing to do is to write lots of '1's to the bitmap
before trying to assemble the array.

However I'm not violently against the patch - though I would be against
fiddling with event_count. It does seem unpleasantly asymmetric though.
The 'sync' option says 'assume all bits in the bitmap are set', but the
'nosync' option doesn't say 'assume all bits in the bitmap are clear'.

Is simply writing to the bitmap when you want to force some behaviour an
option, or do you really really want "sync" to mean "please ignore the
bitmap"?  (or maybe an "ignorebitmap" option is what we want??).

NeilBrown



> 
>  brassow
> 
> DM RAID: Fix for "sync" directive ineffectiveness
> 
> There are two table arguments that can be given to a DM RAID target that
> control whether the array is forced to (re)synchronize or skip initialization:
> "sync" and "nosync".  When "sync" is given, we set mddev->recovery_cp to 0
> in order to cause the device to resynchronize.  This is insufficient if there
> is a bitmap in use, because the array will simply look at the bitmap and see
> that there is no recovery necessary.
> 
> Once md_run() is called and creates the bitmap, we set the bitmap as stale.
> When the bitmap is loaded during the resume phase, a full resync will
> result.
> 
> Signed-off-by: Jonathan Brassow <jbrassow@redhat.com>
> 
> Index: linux-upstream/drivers/md/dm-raid.c
> ===================================================================
> --- linux-upstream.orig/drivers/md/dm-raid.c
> +++ linux-upstream/drivers/md/dm-raid.c
> @@ -1123,6 +1123,10 @@ static int raid_ctr(struct dm_target *ti
>  		ret = -EINVAL;
>  		goto size_mismatch;
>  	}
> +
> +	if (rs->print_flags & DMPF_SYNC)
> +		set_bit(BITMAP_STALE, &(rs->md.bitmap->flags));
> +
>  	rs->callbacks.congested_fn = raid_is_congested;
>  	dm_table_add_target_callbacks(ti->table, &rs->callbacks);
>  
> 


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