Re: [PATCH 1/2] md: Inform udev about device removal when stopping

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 3908 bytes --]

On Wed, Feb 17 2016, Sebastian Parschauer wrote:

> On 16.02.2016 21:43, NeilBrown wrote:
>> On Wed, Feb 17 2016, Shaohua Li wrote:
>> 
>>> On Tue, Feb 16, 2016 at 03:44:36PM +0100, Sebastian Parschauer wrote:
>>>> When stopping an MD device, then its device node /dev/mdX may still
>>>> exist afterwards or it is recreated by udev. The next open() call
>>>> can lead to creation of an inoperable MD device. The reason for
>>>> this is that a change event (KOBJ_CHANGE) is announced to udev.
>>>> So announce a removal event (KOBJ_REMOVE) to udev instead.
>>>>
>>>> A change is likely also required in mdadm because of the support
>>>> for kernels prior to 2.6.28.
>>>
>>> I didn't follow why we need the change. Shouldn't the KOBJ_REMOVE event be sent
>>> automatically when gendisk is deleted?
>>> mddev_put()->mddev_delayed_delete()->md_free()->del_gendisk().
>>>
>>> Thanks,
>>> Shaohua
>> 
>> For a bit of context: this KOBJ_CHANGE event was added in Oct 2008
>> 
>> Commit: 934d9c23b4c7 ("md: destroy partitions and notify udev when md array is stopped.")
>> 
>> At the time, md devices weren't getting removed at all.
>> Now they are (I figured out the locking), though they can still come
>> back.
>> 
>> There are still two stages.  The array is stopped, and then the block
>> device is destroyed.  It is theoretically possible to stop the array
>> without destroying the block device, though I don't think that happens
>> in practice.
>> 
>> So this KOBJ_CHANGE is, I think, technically correct (change from
>> "active" to "inactive")  but probably isn't needed any more - not to the
>> extent it was at the time.
>> 
>> There are some annoying races with caused by udev responding (belatedly)
>> to events by running programs that open s/dev/mdXX and so automatically
>> re-creates the md device.
>> The real problem here is not the event or the delays in udev.  It is the
>> fact that opening /dev/mdXX transparently creates a device.
>> 
>> The only way (I know of) to really avoid these races is to use named
>> arrays.
>> Put
>>    CREATE names=yes
>> 
>> in mdadm.conf.  Then md arrays will be created by writing a name to a
>> magic file in /sys.  The arrays have a minor number >=512 and are not
>> auto-re-created if the device node is re-opened before udev unlinks it.
>> 
>> So: the patch might be safe, and might solve a particular problem, but
>> it is really just a bandaid.  The best fix is "CREATE named=yes" (and
>> use named like "md_home", not "md4".
>
> Older mdadm versions like 3.2.6 have really bad scaling issues as they
> search the whole /dev directory with map_dev() for the correct device
> and we've hit further issues with the symlinks in /dev/md/. This is why
> we've decided to go for the /dev/mdX devices directly as then also the
> minor number is clear.

Why would anyone care about the minor number?

with 'name=yes', the entries in /dev are e.g. "md_foo" - no symlinks in
/dev (the exact same symlinked are in /dev/md).

If there are scaling issues, we should try to fix them.  Please report
details.

>
> I remember custom commits:
> * dev_open: add parameter 'do_map_dev'
> * mdopen: don't do 'map_dev' in 'create_mddev' if devname is /dev/mdX

Have you posted these?  Please do.

>
> I did a further test: If mdadm and the kernel don't send any uevent when
> stopping, then it also works. Might be the best solution.

I'm glad it works for your test cases, but that doesn't necessarily
means it is correct or sufficient.
I'm not exactly against removing the uevent, but I wouldn't be surprised
if that ends up causing a regression for someone who does things
differently to you.
And as I have said, I think there are other situation, maybe less
common, where udev can get bogged down and end up handling change events after
the array has been destroyed.

NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]