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

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

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.  

> 
> 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

Correct! I'm accepting risk in extraordinary cases. In general, kernel is bumped
not downgraded.

> 
> 
> > 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?
> 
Not sure if I get your question correctly. This property is supported now so
why we should? It is already there so we are safe to set it.

This comment is about code optimization here:
> > > -                     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;

I think that we can check:
if (di->dev_size != di2->dev_size || di->data_offset != di2->data_offset)
    raid0_need_layout = 1;

but I could be wrong here, it is zoned raid, I don't have experience in this
area. It is existing code so you don't need to dig into. You can left it as is.

Isn't the size of members saved somewhere, do we need to count it?

Mariusz