Re: [PATCH mdadm] mdadm: Don't open md device for CREATE and ASSEMBLE

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

On Fri, 15 Jul 2022 10:20:26 +0800
Guoqing Jiang <guoqing.jiang@linux.dev> wrote:

> On 7/15/22 6:37 AM, Logan Gunthorpe wrote:
> > The mdadm command tries to open the md device for most modes, first
> > thing, no matter what. When running to create or assemble an array,
> > in most cases, the md device will not exist, the open call will fail
> > and everything will proceed correctly.  
> 
> I guess the BUILD mode doesn't need to create md as well since commit
> 7f91af49ad09 ("Delay creation of array devices for assemble/build/create").
> 
> > However, when running tests, a create or assembly command may be run
> > shortly after stopping an array and the old md device file may still
> > be around. Then, if create_on_open is set in the kernel, a new md
> > device will be created when mdadm does its initial open.
> >
> > When mdadm gets around to creating the new device with the new_array
> > parameter it issues this error:
> >
> >     mdadm: Fail to create md0 when using
> >     /sys/module/md_mod/parameters/new_array, fallback to creation via node
> >
> > This is because an mddev was already created by the kernel with the
> > earlier open() call and thus the new one being created will fail with
> > EEXIST. The mdadm command will still successfully be created due to
> > falling back to the node creation method. However, the error message
> > itself will fail any test that's running it.
> >
> > This issue is a race condition that is very rare, but a recent change
> > in the kernel caused this to happen more frequently: about 1 in 50
> > times.
> >
> > To fix this, don't bother trying to open the md device for CREATE and
> > ASSEMBLE commands, as the file descriptor will never be used anyway
> > even if it is successfully openned.
Hi All,

This is not a fix, it just reduces race probability because file descriptor
will be opened later. I normal environment issue is not likely to happen
because user doesn't create and stop arrays in loop. Issue here
should be resolved in tests, perhaps wait should be sufficient.

I tried to resolve it in the past by adding completion to md driver and force
mdadm --stop command to wait for sysfs clean up but I have never finished it.
IMO it is a better way, wait for device to be fully removed by MD during stop.
Thoughts?

One objection I have here is that error handling is changed, so it could be
harmful change for users. 
> >
> > Side note: it would be nice to disable create_on_open as well to help
> > solve this, but it seems the work for this was never finished. By default,
> > mdadm will create using the old node interface when a name is specified
> > unless the user specifically puts names=yes in a config file, which
> > doesn't seem to be vcreate_on_openery common yet.  
> 
> Hmm, 'create_on_open' is default to true, not sure if there is any side 
> effect
> after change to false.

I'm slowly working on this. The change is not simple, starting from terrible
create_mddev code and char *mddev and char *name rules , ending with hidden
references which we are not aware off (it is common to create temp node and
open mddevice in mdadm) and other references in systemd (because of that, udev
is avoiding to open device).
This also indicate that we should drop partition discover in kernel and use
udev in the future, especially for external metadata (where RO -> RW transition
happens during assembly). But this is a separate problem.

Thanks,
Mariusz
> 
> > Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
> > ---
> >   mdadm.c | 3 +--
> >   1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/mdadm.c b/mdadm.c
> > index 56722ed997a2..3b886b5c0639 100644
> > --- a/mdadm.c
> > +++ b/mdadm.c
> > @@ -1347,8 +1347,7 @@ int main(int argc, char *argv[])
> >   	 * an md device.  We check that here and open it.
> >   	 */
> >   
> > -	if (mode == MANAGE || mode == BUILD || mode == CREATE ||
> > -	    mode == GROW || (mode == ASSEMBLE && ! c.scan)) {
> > +	if (mode == MANAGE || mode == BUILD || mode == GROW) {
> >   		if (devs_found < 1) {
> >   			pr_err("an md device must be given in this
> > mode\n"); exit(2);  
> 
> I think it is a reasonable change.
> 
> Thanks,
> Guoqing