Re: [PATCH 2/9] md: override md superblock recovery_offset for cache device

[NeilBrown <neilb@suse.com>]

On Wed, 29 Jul 2015 17:38:42 -0700 Shaohua Li <shli@fb.com> wrote:

> Cache device stores data in a log structure. We need record the log
> start. Here we override md superblock recovery_offset for this purpose.
> This field of a cache device is meaningless otherwise.
> 
> Signed-off-by: Shaohua Li <shli@fb.com>
> ---
>  drivers/md/md.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index fd84f16..9861f34 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -1664,6 +1664,7 @@ static int super_1_validate(struct mddev *mddev, struct md_rdev *rdev)
>  				  "cache feature, ignoring the device\n");
>  				return -EINVAL;
>  			}
> +			rdev->recovery_offset = le64_to_cpu(sb->recovery_offset);
>  			set_bit(WriteCache, &rdev->flags);
>  			break;
>  		default:
> @@ -1830,6 +1831,9 @@ static void super_1_sync(struct mddev *mddev, struct md_rdev *rdev)
>  			sb->dev_roles[i] = cpu_to_le16(0xffff);
>  	}
>  
> +	if (test_bit(WriteCache, &rdev->flags))
> +		sb->recovery_offset = cpu_to_le64(rdev->recovery_offset);
> +

I would much rather this was a little earlier in the function,
near where recovery_offset is set  for non-InSync devices.
i.e. immediately after:

	if (rdev->raid_disk >= 0 &&
	    !test_bit(In_sync, &rdev->flags)) {
		sb->feature_map |=
			cpu_to_le32(MD_FEATURE_RECOVERY_OFFSET);
		sb->recovery_offset =
			cpu_to_le64(rdev->recovery_offset);
		if (rdev->saved_raid_disk >= 0 && mddev->bitmap)
			sb->feature_map |=
				cpu_to_le32(MD_FEATURE_RECOVERY_BITMAP);
	}

Maybe we could always copy recovery_offset from rdev to sb, and then
just set whichever feature flags are relevant - if any.  Or maybe not.

Thanks,
NeilBrown


>  	sb->sb_csum = calc_sb_1_csum(sb);
>  }
>