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

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

On Mon, 16 Oct 2023 16:13:16 +0800
Xiao Ni <xni@redhat.com> wrote:

> On Fri, Oct 13, 2023 at 7:59 PM Mariusz Tkaczyk
> <mariusz.tkaczyk@linux.intel.com> wrote:
> >
> > On Fri, 13 Oct 2023 18:59:21 +0800
> > Xiao Ni <xni@redhat.com> wrote:
> >  
> > > On Fri, Oct 13, 2023 at 5:31 PM Mariusz Tkaczyk
> > > <mariusz.tkaczyk@linux.intel.com> wrote:  
> > > >
> > > > 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.  
> > >
> > > 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.  
> 
> Hi Mariusz
> 
> I just noticed the kernel version should be 3.14 rather than 5.4. In
> kernel 3.14 (20d0189b1012 block: Introduce new bio_split()) introduces
> this problem. So 5.4 is a typo error?
> 
> Regards
> Xiao
> 
Hi Xiao,
5.4 is a kernel where Neil introduced RAID0_LAYOUT_SUPPORT:
"Since Linux 5.4 a layout is needed for RAID0 arrays with
varying device sizes."

3.14 is a kernel when regression came but it seems that we fixed it in
5.4. I think that we can set it safely starting from 5.4.

Thanks,
Mariusz