DM RAID: Fix for ineffective "sync" directive

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().

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 <[email protected]>

Index: linux-upstream/drivers/md/dm-raid.c
===================================================================

drivers/md/dm-raid.c +4 -0

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

Quoted 13 lines · On Thu, 23 Aug 2012 10:31:54 -0500 Jonathan Brassow <[email protected]> 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??).

Quoted 38 lines · NeilBrown

Quoted 13 lines · On Thu, 23 Aug 2012 10:31:54 -0500 Jonathan Brassow <[email protected]> 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'.

Quoted 3 lines

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??).

For me, the 'sync' and 'nosync' args are not just a vestige of when we didn't have metadata; but also a form of continuity with dm-raid1. The 'sync' option has - for device-mapper - always been a way to reset the bitmap and force a resync.

Neil,

I've taken your suggestion to not muck around with the BITMAP_STALE, and instead simply clear the superblocks as a whole when the "sync" directive is given.

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.

The fix is to skip over the loading of the superblocks when "sync" is given, causing new superblocks to be written that will force the array to go through initialization (i.e. synchronization).

Signed-off-by Jonathan Brassow <[email protected]>

Index: linux-upstream/drivers/md/dm-raid.c
===================================================================

drivers/md/dm-raid.c +6 -0

@@ -960,6 +960,19 @@ static int analyse_superblocks(struct dm
 
 	freshest = NULL;
 	rdev_for_each_safe(rdev, tmp, mddev) {
+		/*
+		 * Skipping super_load due to DMPF_SYNC will cause
+		 * the array to undergo initialization again as
+		 * though it were new.  This is the intended effect
+		 * of the "sync" directive.
+		 *