Re: [PATCH 1/1] mdadm/super1: Add MD_FEATURE_RAID0_LAYOUT if sb->layout is set

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Wed, 11 Oct 2023 21:05:22 +0800
Xiao Ni <xni@redhat.com> wrote:

> In kernel space super_1_validate sets mddev->layout to -1 if
> MD_FEATURE_RAID0_LAYOUT is not set. MD_FEATURE_RAID0_LAYOUT is set in mdadm
> write_init_super1. Now only raid with more than one zone can set this bit.
> But for raid0 with same size member disks, it doesn't set this bit. The
> layout is *unknown* when running mdadm -D command. In fact it should be
> RAID0_ORIG_LAYOUT which gets from default_layout.
> 
> So set MD_FEATURE_RAID0_LAYOUT when sb->layout has value.
> 
> Fixes: 329dfc28debb ('Create: add support for RAID0 layouts.')
> Signed-off-by: Xiao Ni <xni@redhat.com>
> ---
>  super1.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
> 
> diff --git a/super1.c b/super1.c
> index 856b02082662..f29751b4a5c7 100644
> --- a/super1.c
> +++ b/super1.c
> @@ -1978,26 +1978,10 @@ static int write_init_super1(struct supertype *st)
>  	unsigned long long sb_offset;
>  	unsigned long long data_offset;
>  	long bm_offset;
> -	int raid0_need_layout = 0;
>  
> -	for (di = st->info; di; di = di->next) {
> +	for (di = st->info; di; di = di->next)
>  		if (di->disk.state & (1 << MD_DISK_JOURNAL))
>  			sb->feature_map |= __cpu_to_le32(MD_FEATURE_JOURNAL);
> -		if (sb->level == 0 && sb->layout != 0) {
> -			struct devinfo *di2 = st->info;
> -			unsigned long long s1, s2;
> -			s1 = di->dev_size;
> -			if (di->data_offset != INVALID_SECTORS)
> -				s1 -= di->data_offset;
> -			s1 /= __le32_to_cpu(sb->chunksize);
> -			s2 = di2->dev_size;
> -			if (di2->data_offset != INVALID_SECTORS)
> -				s2 -= di2->data_offset;
> -			s2 /= __le32_to_cpu(sb->chunksize);
> -			if (s1 != s2)
> -				raid0_need_layout = 1;
> -		}
> -	}
>  
>  	for (di = st->info; di; di = di->next) {
>  		if (di->disk.state & (1 << MD_DISK_FAULTY))
> @@ -2139,8 +2123,7 @@ static int write_init_super1(struct supertype *st)
>  			sb->bblog_offset = 0;
>  		}
>  
> -		/* RAID0 needs a layout if devices aren't all the same size
> */
> -		if (raid0_need_layout)
> +		if (sb->level == 0 && sb->layout)
>  			sb->feature_map |=
> __cpu_to_le32(MD_FEATURE_RAID0_LAYOUT); 
>  		sb->sb_csum = calc_sb_1_csum(sb);
Hi Xiao,

I read Neil patch:
https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=329dfc28de

For sure Neil has a purpose to make it this way. I think that because it breaks
creation when layout is not supported by kernel. Neil wanted to keep possible
largest compatibility so it sets layout feature only if it is necessary.
Your change forces layout bit to be always used. Can you test this change on
kernel without raid0_layout support? I expect regression for same dev size raid
arrays.

I think that before we will set layout bit we should check kernel
version, it must be higher than 5.4. In the future we would remove this check.
So, it forces the calculations made by Neil back but I think that we can simply
compare dev_size and data_offset between members.

Thanks,
Mariusz