On Fri, Oct 13, 2023 at 5:31 PM Mariusz Tkaczyk
<mariusz.tkaczyk@xxxxxxxxxxxxxxx> wrote:
>
> On Wed, 11 Oct 2023 21:05:22 +0800
> Xiao Ni <xni@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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.
Hi Mariusz
Thanks for pointing out this. I only think the kernel which supports
MD_FEATURE_RAID0_LAYOUT
>
> 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.
Let me check if I understand right:
It needs to check raid0_need_layout when <= kernel 5.4. It can set
MD_FEATURE_RAID0_LAYOUT for all raid0 > kernel 5.4
> So, it forces the calculations made by Neil back but I think that we can simply
> compare dev_size and data_offset between members.
We don't need to consider the compatibility anymore in future?
Best Regards
Xiao
>
> Thanks,
> Mariusz
>
