[PATCH] md: Drop sending a change uevent when stopping

[PATCH] md: Drop sending a change uevent when stopping

[Sebastian Parschauer]

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 sent to udev which
races against the remove event (KOBJ_REMOVE) from md_free().
So drop sending the change event.

A change is likely also required in mdadm as many versions send the
change event to udev as well.

Signed-off-by: Sebastian Parschauer <sebastian.riemer@profitbricks.com>
---
 drivers/md/md.c |    1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index e55e6cf..464627b 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -5671,7 +5671,6 @@ static int do_md_stop(struct mddev *mddev, int mode,
 		export_array(mddev);
 
 		md_clean(mddev);
-		kobject_uevent(&disk_to_dev(mddev->gendisk)->kobj, KOBJ_CHANGE);
 		if (mddev->hold_active == UNTIL_STOP)
 			mddev->hold_active = 0;
 	}
-- 
1.7.9.5

Re: [PATCH] md: Drop sending a change uevent when stopping

[Shaohua Li]

On Wed, Feb 17, 2016 at 05:25:00PM +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 sent to udev which
> races against the remove event (KOBJ_REMOVE) from md_free().
> So drop sending the change event.
> 
> A change is likely also required in mdadm as many versions send the
> change event to udev as well.

Makes sense, it's unlikely we need the CHANGE event. Applied.

Thanks,
Shaohua

Re: [PATCH] md: Drop sending a change uevent when stopping

[NeilBrown]

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

On Thu, Feb 18 2016, Shaohua Li wrote:

> On Wed, Feb 17, 2016 at 05:25:00PM +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 sent to udev which
>> races against the remove event (KOBJ_REMOVE) from md_free().
>> So drop sending the change event.
>> 
>> A change is likely also required in mdadm as many versions send the
>> change event to udev as well.
>
> Makes sense, it's unlikely we need the CHANGE event. Applied.
>
> Thanks,
> Shaohua

It would be worth checking, but I think that with this change, you can
write
  "inactive" to /sys/block/mdXXX/md/array_state
and the array will become inactive, but no uevent will be generated,
which isn't good.
Maybe send the uevent that was just removed from the 'inactive' case of
array_state_store() instead.

(But I still think this is just a bandaid and doesn't provide any
guarantees that there will be no races with udev)

NeilBrown

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

Re: [PATCH] md: Drop sending a change uevent when stopping

[Shaohua Li]

On Thu, Feb 18, 2016 at 08:29:28AM +1100, Neil Brown wrote:
> On Thu, Feb 18 2016, Shaohua Li wrote:
> 
> > On Wed, Feb 17, 2016 at 05:25:00PM +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 sent to udev which
> >> races against the remove event (KOBJ_REMOVE) from md_free().
> >> So drop sending the change event.
> >> 
> >> A change is likely also required in mdadm as many versions send the
> >> change event to udev as well.
> >
> > Makes sense, it's unlikely we need the CHANGE event. Applied.
> >
> > Thanks,
> > Shaohua
> 
> It would be worth checking, but I think that with this change, you can
> write
>   "inactive" to /sys/block/mdXXX/md/array_state
> and the array will become inactive, but no uevent will be generated,
> which isn't good.
> Maybe send the uevent that was just removed from the 'inactive' case of
> array_state_store() instead.

with 'inactive', the mode == 2, do_md_stop() doesn't send the event either, so
the behavior isn't changed.

> (But I still think this is just a bandaid and doesn't provide any
> guarantees that there will be no races with udev)

that's correct. I'd expect races in other CHNAGE/REMOVE cases are very rare.

Thanks,
Shaohua

Re: [PATCH] md: Drop sending a change uevent when stopping

[Hannes Reinecke]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/17/2016 10:29 PM, NeilBrown wrote:
> On Thu, Feb 18 2016, Shaohua Li wrote:
> 
>> On Wed, Feb 17, 2016 at 05:25:00PM +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 sent to udev which races against the
>>> remove event (KOBJ_REMOVE) from md_free(). So drop sending
>>> the change event.
>>> 
>>> A change is likely also required in mdadm as many versions
>>> send the change event to udev as well.
>> 
>> Makes sense, it's unlikely we need the CHANGE event.
>> Applied.
>> 
>> Thanks, Shaohua
> 
> It would be worth checking, but I think that with this change,
> you can write "inactive" to /sys/block/mdXXX/md/array_state and
> the array will become inactive, but no uevent will be
> generated, which isn't good. Maybe send the uevent that was
> just removed from the 'inactive' case of array_state_store()
> instead.
> 
> (But I still think this is just a bandaid and doesn't provide
> any guarantees that there will be no races with udev)
> 
Thing is, _none_ of the other subsystems will ever send a uevent
when it becomes inactive.
(Would be pretty pointless, too, as what exactly is one supposed
to do here?)
The current usage has it that CHANGE events are only ever sent if
a device becomes active.

Cheers,

Hannes
- -- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBAgAGBQJWxWiUAAoJEGz4yi9OyKjPUhUP+gJXhNTCYTbLNzR7LYcPQplY
rqcALLhZIDt8inveiSaPVXs5F1VCQsT87qS6JtD3EBSU64eWVq0+xowxKStoyjPl
/MaBFQs7yxJCdf5Enx0/hKPN3MYuQT2nf5EiB461mlnfxLZKUEgwKbDK6+6HqToI
x0rtUFv4JpVDd9HFY3PNqZjGtQTMrbXMVxsBtefIiYPeyaPpYU9Zo0qM+17CmJIr
J3JdXOjHluengKcdi1O6GDvshUiysWP/ukG/q7If4JxpomKS5ljOn5MHcCgBl/CL
UwjUuSmZ9e5ZKyIIxU2oMAFRYqLCGX5Fw5Q90YG7UOZQ3ODbYPJfR7d61OSjomYt
j0bME+QXkdoxOkwlG7EwSU8fG6dv4H55RxrrFcu4ZBl6TRo3jpCOCrq+kY1XapF4
NGeY0j3vBQ4ZziMNodelg+KZaBrCSFGu+cn7uqEsrOJ+N4e7gJv28trPiPzE4Hiz
07buvtTNEYQuqUNvR9MiuDCubnHy0imaA+3fb0orXZOllhYFAMfUVxpOsJvO/ySg
XVSrpcm7BnU/4i6sNXJUpZdTrmWPi8kvZU9avlZFwL0CrVtdhjkoGOu20Ou7H53g
nR3wXQWEybd8yK62lqb3g7one/JnEeVupb69aSiaJnAt7kLZE2Dcg5QVFPAXOIDU
eqYQYG60/85zdeLcqzlM
=e02n
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] md: Drop sending a change uevent when stopping

[NeilBrown]

[-- Attachment #1.1: Type: text/plain, Size: 1872 bytes --]

On Thu, Feb 18 2016, Hannes Reinecke wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 02/17/2016 10:29 PM, NeilBrown wrote:
>> On Thu, Feb 18 2016, Shaohua Li wrote:
>> 
>>> On Wed, Feb 17, 2016 at 05:25:00PM +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 sent to udev which races against the
>>>> remove event (KOBJ_REMOVE) from md_free(). So drop sending
>>>> the change event.
>>>> 
>>>> A change is likely also required in mdadm as many versions
>>>> send the change event to udev as well.
>>> 
>>> Makes sense, it's unlikely we need the CHANGE event.
>>> Applied.
>>> 
>>> Thanks, Shaohua
>> 
>> It would be worth checking, but I think that with this change,
>> you can write "inactive" to /sys/block/mdXXX/md/array_state and
>> the array will become inactive, but no uevent will be
>> generated, which isn't good. Maybe send the uevent that was
>> just removed from the 'inactive' case of array_state_store()
>> instead.
>> 
>> (But I still think this is just a bandaid and doesn't provide
>> any guarantees that there will be no races with udev)
>> 
> Thing is, _none_ of the other subsystems will ever send a uevent
> when it becomes inactive.

A CDROM drive does when you eject the media.


> (Would be pretty pointless, too, as what exactly is one supposed
> to do here?)

Lazy-unmount the filesystem?
If the array was part of another array, mark the slot in that array as
'faulty' ?

> The current usage has it that CHANGE events are only ever sent if
> a device becomes active.

"mostly" but not "only ever".

Thanks,
NeilBrown

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

[-- Attachment #2: Type: text/plain, Size: 172 bytes --]

_______________________________________________
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [PATCH] md: Drop sending a change uevent when stopping

[Shaohua Li]

On Fri, Feb 19, 2016 at 10:06:20AM +1100, Neil Brown wrote:
> On Thu, Feb 18 2016, Hannes Reinecke wrote:
> 
> > -----BEGIN PGP SIGNED MESSAGE-----
> > Hash: SHA1
> >
> > On 02/17/2016 10:29 PM, NeilBrown wrote:
> >> On Thu, Feb 18 2016, Shaohua Li wrote:
> >> 
> >>> On Wed, Feb 17, 2016 at 05:25:00PM +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 sent to udev which races against the
> >>>> remove event (KOBJ_REMOVE) from md_free(). So drop sending
> >>>> the change event.
> >>>> 
> >>>> A change is likely also required in mdadm as many versions
> >>>> send the change event to udev as well.
> >>> 
> >>> Makes sense, it's unlikely we need the CHANGE event.
> >>> Applied.
> >>> 
> >>> Thanks, Shaohua
> >> 
> >> It would be worth checking, but I think that with this change,
> >> you can write "inactive" to /sys/block/mdXXX/md/array_state and
> >> the array will become inactive, but no uevent will be
> >> generated, which isn't good. Maybe send the uevent that was
> >> just removed from the 'inactive' case of array_state_store()
> >> instead.
> >> 
> >> (But I still think this is just a bandaid and doesn't provide
> >> any guarantees that there will be no races with udev)
> >> 
> > Thing is, _none_ of the other subsystems will ever send a uevent
> > when it becomes inactive.
> 
> A CDROM drive does when you eject the media.
> 
> 
> > (Would be pretty pointless, too, as what exactly is one supposed
> > to do here?)
> 
> Lazy-unmount the filesystem?
> If the array was part of another array, mark the slot in that array as
> 'faulty' ?
> 
> > The current usage has it that CHANGE events are only ever sent if
> > a device becomes active.
> 
> "mostly" but not "only ever".

Neil,

did you mean I should drop the patch?

I really doubt there is any difference with/without the CHANGE event giving a
REMOVE event will pop up soon. But this could be userspace aware, I'm not
totally sure.

Thanks,
Shaohua