Message-ID: <20230322075111.000064bc@linux.intel.com>
Date:   Wed, 22 Mar 2023 07:51:11 +0100
From:   Mariusz Tkaczyk <mariusz.tkaczyk@...ux.intel.com>
To:     "NeilBrown" <neilb@...e.de>
Cc:     "Xiao Ni" <xni@...hat.com>,
        "Jes Sorensen" <jes@...ined-monkey.org>,
        "Song Liu" <song@...nel.org>,
        "Linux regressions mailing list" <regressions@...ts.linux.dev>,
        "linux-raid" <linux-raid@...r.kernel.org>,
        "LKML" <linux-kernel@...r.kernel.org>,
        "Nikolay Kichukov" <hijacker@...um.net>,
        Logan Gunthorpe <logang@...tatee.com>
Subject: Re: [PATCH - mdadm] mdopen: always try create_named_array()

On Wed, 22 Mar 2023 14:24:49 +1100
"NeilBrown" <neilb@...e.de> wrote:

> On Wed, 22 Mar 2023, Xiao Ni wrote:
> > On Tue, Mar 14, 2023 at 8:08 AM NeilBrown <neilb@...e.de> wrote:  
> > >
> > >
> > > mdopen() will use create_named_array() to ask the kernel to create the
> > > given md array, but only if it is given a number or name.
> > > If it is NOT given a name and is required to choose one itself using
> > > find_free_devnm() it does NOT use create_named_array().
> > >
> > > On kernels with CONFIG_BLOCK_LEGACY_AUTOLOAD not set, this can result in
> > > failure to assemble an array.  This can particularly seen when the
> > > "name" of the array begins with a host name different to the name of the
> > > host running the command.
> > >
> > > So add the missing call to create_named_array().
> > >
> > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217074  
> > 
> > Hi Neil
> > 
> > I have two questions, hope you can help to understand the function
> > create_mddev better.
> > 
> > Frist, from the comment7 of the bug you mentioned:
> > 
> > There are two different sorts names.  Note that you almost
> > acknowledged this by writing "name for my md device node" while the
> > documentation only talks about names for "md devices", not for "md
> > device nodes".
> > 
> > There are
> > 1/ there are names in /dev or /dev/md/ (device nodes)
> > 2/ there are names that appear in /proc/mdstat and in /sys/block/ (devices)
> > 
> > Thanks for the clarification. But it looks like it doesn't work like
> > what you said.
> > For example:
> > mdadm -CR /dev/md/root -l0 -n2 /dev/sda /dev/sdc --name=test
> > cat /proc/mdstat
> > Personalities : [raid0]
> > md127 : active raid0 sdc[1] sda[0]
> >       3906764800 blocks super 1.2 512k chunks
> > cd /sys/block/md127/md/
> > 
> > In /proc/mdstat and /sys/block, they all use md127 rather than the
> > name(root)  
> 
> Try again with "CREATE names=yes" in /etc/mdadm.conf.
> 
> mdadm generally tries to keep:
>   - the names in /dev/
>   - the names in /dev/md/
>   - the names in /proc/mdstat
>   - the names stored in the metadata
> 
> in sync.  It can only do this when:
>  - you enabled "names=yes"
>  - you don't confuse it by specifying a device name (/dev/md/root) that
>    is different from the metadata names "test".
> 
> If you don't have "names=yes" then the name in /proc/mdstat and the name
> in /dev/md* will be numeric.  The name in /dev/md/ and the name in the
> metadata can be different and will usually be the same.
> 
> If you explicitly give a different name with --name= than the device
> name then obviously they will be different.  If you then stop the array
> and restart with "mdadm -As" or "mdadm -I /dev/sda; mdadm -I /dev/sdb"
> then mdadm will create a name in /dev/md/ that matches the name in the
> metadata.
> 
> > 
> > Before this patch,  it creates a symbol link with the name root rather than
> > test ll /dev/md/root
> > lrwxrwxrwx. 1 root root 8 Mar 21 22:35 /dev/md/root -> ../md127  
> 
> That is what you asked it to do.
> 
> > 
> > So "test" which is specified by --name looks like it has little usage.
> >   
> 
> It is stored in the metadata.  You can see it in --examine output.  If
> you reassemble the array without specifying a device name, it will use
> the name "test".
> 
> > 
> > By the way, after this patch, the symbol link /dev/md/root can't be
> > created anymore.
> > Is it a regression problem?  
> 
> I cannot reproduce any problem like that.  Please provide a sequence of
> steps so that I can try to duplicate it.

Hi,
It is not caused by this patch. Regression is caused by:
https://git.kernel.org/pub/scm/utils/mdadm/mdadm.git/commit/?id=8a4ce2c053866ac97feb436c4c85a54446ee0016

We noticed that yesterday.

In our case, udev fails to create link, timeout happens. This is caused by
missing MD_DEVNAME property in --detail --export. At that is all I know for now.
Work in progress.

Steps:
#mdadm -CR imsm -e imsm -n4 /dev/nvme[0-3]n1
#mdadm -CR vol -l5 -n4 /dev/nvme[0-3]n1 --assume-clean

Thanks,
Mariusz

> 
> > 
> > Second, are there possibilities that the arguments "dev" and "name" of
> > function create_mddev
> > are null at the same time?  
> 
> No.  For Build or Create, dev is never NULL.  For Assemble and
> Incremental, name is never NULL.
> 
> 
> > After some tests, I found dev can't be null when creating a raid
> > device. It can be checked before
> > calling create_mddev. And we must get a name after creating a raid
> > device. So when assembling
> > a raid device, the name must not be null. So the dev and name can't be
> > null at the same time, right?  
> 
> Correct.
> 
> NeilBrown
> 
> 
> > 
> > Best Regards
> > Xiao
> > 
> >   
> > > Signed-off-by: NeilBrown <neilb@...e.de>
> > > ---
> > >  mdopen.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/mdopen.c b/mdopen.c
> > > index d18c931996d2..810f79a3d19a 100644
> > > --- a/mdopen.c
> > > +++ b/mdopen.c
> > > @@ -370,6 +370,7 @@ int create_mddev(char *dev, char *name, int autof,
> > > int trustworthy, }
> > >                 if (block_udev)
> > >                         udev_block(devnm);
> > > +               create_named_array(devnm);
> > >         }
> > >
> > >         sprintf(devname, "/dev/%s", devnm);
> > > --
> > > 2.39.2
> > >  
> > 
> >   
> 

Powered by blists - more mailing lists