Re: [QUESTION] How to fix the race of "mdadm --add" and "mdadm mdadm --incremental --export"

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

On Wed, 15 Mar 2023 16:01:26 +0100
Martin Wilck <mwilck@suse.com> wrote:

> On Wed, 2023-03-15 at 22:57 +0800, Li Xiao Keng wrote:
> > 
> > 
> > On 2023/3/15 22:14, Martin Wilck wrote:  
> > > On Wed, 2023-03-15 at 21:10 +0800, Li Xiao Keng wrote:  
> > > > >   
> > > >   I test move close() after ioctl(). The reason of EBUSY is that
> > > > mdadm
> > > > open(sdf) with O_EXCL. So fd should be closed before ioctl. When
> > > > I
> > > > remove
> > > > O_EXCL, ioctl() will return success.  
> > > 
> > > This makes sense. I suppose mdadm must use O_EXCL if it modifies
> > > RAID
> > > meta data, otherwise data corruption is just too likely. It is also
> > > impossible to drop the O_EXCL flag with fcntl() without closing the
> > > fd.
> > > 
> > > So, if mdadm must close the fd before calling ioctl(), the race can
> > > hardly be avoided. The close() will cause a uevent, and nothing
> > > prevents the udev rules from running before the ioctl() returns.
> > >   
> >   Now I find that close() cause a change udev. Is it necessary to
> > import
> > "mdadm --incremental --export" when change udev cause? Can we ignore
> > it?  
> 
> Normally this is what you want to happen if a change uevent for a MD
> member device is processed.
> 
> The case you're looking at is the exception, as another instance of
> mdamn is handling the device right at this point in time.
> 
> Martin
> 
Hi,
Code snipped from Incremental, devname seems to be our disk:

	/* 4/ Check if array exists.
	 */
	if (map_lock(&map))
		pr_err("failed to get exclusive lock on mapfile\n");
	/* Now check we can get O_EXCL.  If not, probably "mdadm -A" has
	 * taken over
	 */
	dfd = dev_open(devname, O_RDONLY|O_EXCL);

The map_lock in --add should resolve your issue. It seems to be simplest
option. I we cannot lock udev effectively then it seems to be the best one.
We need to control adding the device because external metadata is quite
different and for example in IMSM the initial metadata doesn't point to
container wanted by user. Hope it clarifies all objections here.

I don't see any blocker from using locking mechanism for --add.

Thanks,
Mariusz