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

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

[Hannes Reinecke]

Hi Sebastian,

while it's true in general that a 'change' event should not be sent
when a device is being removed or deleted, sending a 'remove' event
from userspace is most definitely wrong, too.
'remove' events should be generated from the kernel whenever a
device disappears from sysfs; it should never be generated from
userspace (as the device will still be present in sysfs).

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)
--
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 2/2] Manage: Inform udev about device removal when stopping

[Sebastian Parschauer]

On 16.02.2016 16:47, Hannes Reinecke wrote:
> Hi Sebastian,
> 
> while it's true in general that a 'change' event should not be sent
> when a device is being removed or deleted, sending a 'remove' event
> from userspace is most definitely wrong, too.
> 'remove' events should be generated from the kernel whenever a
> device disappears from sysfs; it should never be generated from
> userspace (as the device will still be present in sysfs).

Hi Hannes,

thanks for your comment! Only few people actually stop md devices
without rebooting. As a hotfix for running systems it is definitely the
best solution for us at PB to do it in mdadm. Mdadm knows in this
situation that stopping worked.

But I'm also fine with dropping the support for kernels prior to 2.6.28
and the event sending in mdadm completely for the mainline. Sending the
kernel patch to linux-stable is kind of useless if mdadm still sends the
change event.
So your vote seems to be dropping the udev event sending in mdadm and
going for the kernel change.

I'll wait for other comments or votes for the preferred solution.

Cheers,
Sebastian

[PATCH 0/2] md/mdadm: Inform udev about device removal 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 announced to udev.
So announce a removal event (KOBJ_REMOVE) to udev instead.

Because of the support for kernels prior to 2.6.28, this change is
required in mdadm and the kernel. The udev event from mdadm
overrides the one from the kernel.

Sebastian Parschauer (2):
  md: Inform udev about device removal when stopping
  Manage: Inform udev about device removal when stopping

 drivers/md/md.c |    2 +-
 Manage.c        |    6 +++---
 2 files changed, 4 insertions(+), 4 deletions(-)

[PATCH 2/2] Manage: Inform udev about device removal 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 announced to udev.
So announce a removal event (KOBJ_REMOVE) to udev instead.

This also overrides the change event sent by the kernel.

Signed-off-by: Sebastian Parschauer <sebastian.riemer@profitbricks.com>
---
 Manage.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Manage.c b/Manage.c
index 7e1b94b..bc89764 100644
--- a/Manage.c
+++ b/Manage.c
@@ -494,13 +494,13 @@ done:
 		goto out;
 	}
 	/* prior to 2.6.28, KOBJ_CHANGE was not sent when an md array
-	 * was stopped, so We'll do it here just to be sure.  Drop any
-	 * partitions as well...
+	 * was stopped, it should be KOBJ_REMOVE instead, so we set the
+	 * remove event here just to be sure. Drop any partitions as well...
 	 */
 	if (fd >= 0)
 		ioctl(fd, BLKRRPART, 0);
 	if (mdi)
-		sysfs_uevent(mdi, "change");
+		sysfs_uevent(mdi, "remove");
 
 	if (devnm[0] && use_udev()) {
 		struct map_ent *mp = map_by_devnm(&map, devnm);
-- 
1.7.9.5

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

[Jes Sorensen]

Sebastian Parschauer <sebastian.riemer@profitbricks.com> writes:
> 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.
>
> This also overrides the change event sent by the kernel.
>
> Signed-off-by: Sebastian Parschauer <sebastian.riemer@profitbricks.com>
> ---
>  Manage.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Manage.c b/Manage.c
> index 7e1b94b..bc89764 100644
> --- a/Manage.c
> +++ b/Manage.c
> @@ -494,13 +494,13 @@ done:
>  		goto out;
>  	}
>  	/* prior to 2.6.28, KOBJ_CHANGE was not sent when an md array
> -	 * was stopped, so We'll do it here just to be sure.  Drop any
> -	 * partitions as well...
> +	 * was stopped, it should be KOBJ_REMOVE instead, so we set the
> +	 * remove event here just to be sure. Drop any partitions as well...
>  	 */
>  	if (fd >= 0)
>  		ioctl(fd, BLKRRPART, 0);
>  	if (mdi)
> -		sysfs_uevent(mdi, "change");
> +		sysfs_uevent(mdi, "remove");

I am a little concerned about this change. You assume the kernel and
mdadm will be updated in sync, which is unlikely to happen. I believe
you need to match the kernel version and send the corresponding event
currectly for this to work correctly?

Jes

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

[Sebastian Parschauer]

On 16.02.2016 18:41, Jes Sorensen wrote:
> Sebastian Parschauer <sebastian.riemer@profitbricks.com> writes:
>> 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.
>>
>> This also overrides the change event sent by the kernel.
>>
>> Signed-off-by: Sebastian Parschauer <sebastian.riemer@profitbricks.com>
>> ---
>>  Manage.c |    6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/Manage.c b/Manage.c
>> index 7e1b94b..bc89764 100644
>> --- a/Manage.c
>> +++ b/Manage.c
>> @@ -494,13 +494,13 @@ done:
>>  		goto out;
>>  	}
>>  	/* prior to 2.6.28, KOBJ_CHANGE was not sent when an md array
>> -	 * was stopped, so We'll do it here just to be sure.  Drop any
>> -	 * partitions as well...
>> +	 * was stopped, it should be KOBJ_REMOVE instead, so we set the
>> +	 * remove event here just to be sure. Drop any partitions as well...
>>  	 */
>>  	if (fd >= 0)
>>  		ioctl(fd, BLKRRPART, 0);
>>  	if (mdi)
>> -		sysfs_uevent(mdi, "change");
>> +		sysfs_uevent(mdi, "remove");
> 
> I am a little concerned about this change. You assume the kernel and
> mdadm will be updated in sync, which is unlikely to happen. I believe
> you need to match the kernel version and send the corresponding event
> currectly for this to work correctly?

The worst thing that can happen is that the kernel sends the change
event after the remove event. Then it is the current situation again.
From my tests mdadm does enough other stuff in between. Udev is able to
handle receiving two remove events from my testing. Multiple mdadm
instances can't run in parallel any ways. So userspace around it needs
some serialization for it any ways. So also stopping an MD device and
assembling a new one with the same minor number shouldn't race.

I still prefer this solution here. But if you decide to drop the udev
event sending in mdadm, then I'm also fine with that.

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

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

[Hannes Reinecke]

On 02/16/2016 07:03 PM, Sebastian Parschauer wrote:
> On 16.02.2016 18:41, Jes Sorensen wrote:
>> Sebastian Parschauer <sebastian.riemer@profitbricks.com> writes:
>>> 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.
>>>
>>> This also overrides the change event sent by the kernel.
>>>
>>> Signed-off-by: Sebastian Parschauer <sebastian.riemer@profitbricks.com>
>>> ---
>>>   Manage.c |    6 +++---
>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Manage.c b/Manage.c
>>> index 7e1b94b..bc89764 100644
>>> --- a/Manage.c
>>> +++ b/Manage.c
>>> @@ -494,13 +494,13 @@ done:
>>>   		goto out;
>>>   	}
>>>   	/* prior to 2.6.28, KOBJ_CHANGE was not sent when an md array
>>> -	 * was stopped, so We'll do it here just to be sure.  Drop any
>>> -	 * partitions as well...
>>> +	 * was stopped, it should be KOBJ_REMOVE instead, so we set the
>>> +	 * remove event here just to be sure. Drop any partitions as well...
>>>   	 */
>>>   	if (fd >= 0)
>>>   		ioctl(fd, BLKRRPART, 0);
>>>   	if (mdi)
>>> -		sysfs_uevent(mdi, "change");
>>> +		sysfs_uevent(mdi, "remove");
>>
>> I am a little concerned about this change. You assume the kernel and
>> mdadm will be updated in sync, which is unlikely to happen. I believe
>> you need to match the kernel version and send the corresponding event
>> currectly for this to work correctly?
>
> The worst thing that can happen is that the kernel sends the change
> event after the remove event. Then it is the current situation again.
>  From my tests mdadm does enough other stuff in between. Udev is able to
> handle receiving two remove events from my testing. Multiple mdadm
> instances can't run in parallel any ways. So userspace around it needs
> some serialization for it any ways. So also stopping an MD device and
> assembling a new one with the same minor number shouldn't race.
>
> I still prefer this solution here. But if you decide to drop the udev
> event sending in mdadm, then I'm also fine with that.
>
I strongly prefer removing the udev event generation altogether.
As this appears to be a carry-over from older kernels, it looks to me as 
being an incomplete conversion:
At one point udev introduced 'ONLINE' and 'OFFLINE' events, which were 
supposed to be used for this kind of scenario.
(ONLINE being a companion to 'ADD', and 'OFFLINE' being the companion to 
'DELETE'). However, later the 'ONLINE' got modified to 'CHANGE', and the 
'OFFLINE' got dropped completely.
Or that was the plan.
So it looks as if the conversion to 'CHANGE' got applied to the 
'OFFLINE' event, too.
Hence I strongly recommend to drop it completely, and let the kernel or 
the MD module decide if and when a uevent should be send.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
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 2/2] Manage: Inform udev about device removal when stopping

[Jes Sorensen]

Hannes Reinecke <hare@suse.de> writes:
> On 02/16/2016 07:03 PM, Sebastian Parschauer wrote:
>> On 16.02.2016 18:41, Jes Sorensen wrote:
>>> Sebastian Parschauer <sebastian.riemer@profitbricks.com> writes:
>>>> 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.
>>>>
>>>> This also overrides the change event sent by the kernel.
>>>>
>>>> Signed-off-by: Sebastian Parschauer <sebastian.riemer@profitbricks.com>
>>>> ---
>>>>   Manage.c |    6 +++---
>>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/Manage.c b/Manage.c
>>>> index 7e1b94b..bc89764 100644
>>>> --- a/Manage.c
>>>> +++ b/Manage.c
>>>> @@ -494,13 +494,13 @@ done:
>>>>   		goto out;
>>>>   	}
>>>>   	/* prior to 2.6.28, KOBJ_CHANGE was not sent when an md array
>>>> -	 * was stopped, so We'll do it here just to be sure.  Drop any
>>>> -	 * partitions as well...
>>>> +	 * was stopped, it should be KOBJ_REMOVE instead, so we set the
>>>> +	 * remove event here just to be sure. Drop any partitions as well...
>>>>   	 */
>>>>   	if (fd >= 0)
>>>>   		ioctl(fd, BLKRRPART, 0);
>>>>   	if (mdi)
>>>> -		sysfs_uevent(mdi, "change");
>>>> +		sysfs_uevent(mdi, "remove");
>>>
>>> I am a little concerned about this change. You assume the kernel and
>>> mdadm will be updated in sync, which is unlikely to happen. I believe
>>> you need to match the kernel version and send the corresponding event
>>> currectly for this to work correctly?
>>
>> The worst thing that can happen is that the kernel sends the change
>> event after the remove event. Then it is the current situation again.
>>  From my tests mdadm does enough other stuff in between. Udev is able to
>> handle receiving two remove events from my testing. Multiple mdadm
>> instances can't run in parallel any ways. So userspace around it needs
>> some serialization for it any ways. So also stopping an MD device and
>> assembling a new one with the same minor number shouldn't race.
>>
>> I still prefer this solution here. But if you decide to drop the udev
>> event sending in mdadm, then I'm also fine with that.
>>
> I strongly prefer removing the udev event generation altogether.
> As this appears to be a carry-over from older kernels, it looks to me
> as being an incomplete conversion:
> At one point udev introduced 'ONLINE' and 'OFFLINE' events, which were
> supposed to be used for this kind of scenario.
> (ONLINE being a companion to 'ADD', and 'OFFLINE' being the companion
> to 'DELETE'). However, later the 'ONLINE' got modified to 'CHANGE',
> and the 'OFFLINE' got dropped completely.
> Or that was the plan.
> So it looks as if the conversion to 'CHANGE' got applied to the
> 'OFFLINE' event, too.
> Hence I strongly recommend to drop it completely, and let the kernel
> or the MD module decide if and when a uevent should be send.

I am totally fine with this, however we should make mdadm fail if run
against a pre-2.6.28 kernel then.

Cheers,
Jes

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

[NeilBrown]

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

On Wed, Feb 17 2016, Jes Sorensen wrote:

> Hannes Reinecke <hare@suse.de> writes:
>> On 02/16/2016 07:03 PM, Sebastian Parschauer wrote:
>>> On 16.02.2016 18:41, Jes Sorensen wrote:
>>>> Sebastian Parschauer <sebastian.riemer@profitbricks.com> writes:
>>>>> 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.
>>>>>
>>>>> This also overrides the change event sent by the kernel.
>>>>>
>>>>> Signed-off-by: Sebastian Parschauer <sebastian.riemer@profitbricks.com>
>>>>> ---
>>>>>   Manage.c |    6 +++---
>>>>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/Manage.c b/Manage.c
>>>>> index 7e1b94b..bc89764 100644
>>>>> --- a/Manage.c
>>>>> +++ b/Manage.c
>>>>> @@ -494,13 +494,13 @@ done:
>>>>>   		goto out;
>>>>>   	}
>>>>>   	/* prior to 2.6.28, KOBJ_CHANGE was not sent when an md array
>>>>> -	 * was stopped, so We'll do it here just to be sure.  Drop any
>>>>> -	 * partitions as well...
>>>>> +	 * was stopped, it should be KOBJ_REMOVE instead, so we set the
>>>>> +	 * remove event here just to be sure. Drop any partitions as well...
>>>>>   	 */
>>>>>   	if (fd >= 0)
>>>>>   		ioctl(fd, BLKRRPART, 0);
>>>>>   	if (mdi)
>>>>> -		sysfs_uevent(mdi, "change");
>>>>> +		sysfs_uevent(mdi, "remove");
>>>>
>>>> I am a little concerned about this change. You assume the kernel and
>>>> mdadm will be updated in sync, which is unlikely to happen. I believe
>>>> you need to match the kernel version and send the corresponding event
>>>> currectly for this to work correctly?
>>>
>>> The worst thing that can happen is that the kernel sends the change
>>> event after the remove event. Then it is the current situation again.
>>>  From my tests mdadm does enough other stuff in between. Udev is able to
>>> handle receiving two remove events from my testing. Multiple mdadm
>>> instances can't run in parallel any ways. So userspace around it needs
>>> some serialization for it any ways. So also stopping an MD device and
>>> assembling a new one with the same minor number shouldn't race.
>>>
>>> I still prefer this solution here. But if you decide to drop the udev
>>> event sending in mdadm, then I'm also fine with that.
>>>
>> I strongly prefer removing the udev event generation altogether.
>> As this appears to be a carry-over from older kernels, it looks to me
>> as being an incomplete conversion:
>> At one point udev introduced 'ONLINE' and 'OFFLINE' events, which were
>> supposed to be used for this kind of scenario.
>> (ONLINE being a companion to 'ADD', and 'OFFLINE' being the companion
>> to 'DELETE'). However, later the 'ONLINE' got modified to 'CHANGE',
>> and the 'OFFLINE' got dropped completely.
>> Or that was the plan.
>> So it looks as if the conversion to 'CHANGE' got applied to the
>> 'OFFLINE' event, too.
>> Hence I strongly recommend to drop it completely, and let the kernel
>> or the MD module decide if and when a uevent should be send.
>
> I am totally fine with this, however we should make mdadm fail if run
> against a pre-2.6.28 kernel then.
>
> Cheers,
> Jes

I would suggest protecting the

	if (fd >= 0)
		ioctl(fd, BLKRRPART, 0);
	if (mdi)
		sysfs_uevent(mdi, "change");

code with

   if (get_linux_version() < 2006028)

That should be completely safe - 2.6.28 and later do this (if needed).

NeilBrown

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

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

[Jes Sorensen]

NeilBrown <neilb@suse.com> writes:
> On Wed, Feb 17 2016, Jes Sorensen wrote:
>
>> Hannes Reinecke <hare@suse.de> writes:
>>> On 02/16/2016 07:03 PM, Sebastian Parschauer wrote:
>>>> The worst thing that can happen is that the kernel sends the change
>>>> event after the remove event. Then it is the current situation again.
>>>>  From my tests mdadm does enough other stuff in between. Udev is able to
>>>> handle receiving two remove events from my testing. Multiple mdadm
>>>> instances can't run in parallel any ways. So userspace around it needs
>>>> some serialization for it any ways. So also stopping an MD device and
>>>> assembling a new one with the same minor number shouldn't race.
>>>>
>>>> I still prefer this solution here. But if you decide to drop the udev
>>>> event sending in mdadm, then I'm also fine with that.
>>>>
>>> I strongly prefer removing the udev event generation altogether.
>>> As this appears to be a carry-over from older kernels, it looks to me
>>> as being an incomplete conversion:
>>> At one point udev introduced 'ONLINE' and 'OFFLINE' events, which were
>>> supposed to be used for this kind of scenario.
>>> (ONLINE being a companion to 'ADD', and 'OFFLINE' being the companion
>>> to 'DELETE'). However, later the 'ONLINE' got modified to 'CHANGE',
>>> and the 'OFFLINE' got dropped completely.
>>> Or that was the plan.
>>> So it looks as if the conversion to 'CHANGE' got applied to the
>>> 'OFFLINE' event, too.
>>> Hence I strongly recommend to drop it completely, and let the kernel
>>> or the MD module decide if and when a uevent should be send.
>>
>> I am totally fine with this, however we should make mdadm fail if run
>> against a pre-2.6.28 kernel then.
>>
>> Cheers,
>> Jes
>
> I would suggest protecting the
>
> 	if (fd >= 0)
> 		ioctl(fd, BLKRRPART, 0);
> 	if (mdi)
> 		sysfs_uevent(mdi, "change");
>
> code with
>
>    if (get_linux_version() < 2006028)
>
> That should be completely safe - 2.6.28 and later do this (if needed).

Seems a better fix to me. I much prefer the duplicated events.

Sebastian, does this patch resolve the problem for you? If nobody
hollors, I will push this into mdadm.

Cheers,
Jes

commit 7856fa44b8f0bc217a6bbcb5f7c51b2f03717655
Author: Jes Sorensen <Jes.Sorensen@redhat.com>
Date:   Tue Feb 16 16:58:36 2016 -0500

    Manage.c: Only issue change events for kernels older then 2.6.28
    
    2.6.28+ kernels handle this themselves and issuing the event here can
    cause a race.
    
    Reported-by: Sebastian Parschauer <sebastian.riemer@profitbricks.com>
    Suggested-by: NeilBrown <neilb@suse.com>
    Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>

diff --git a/Manage.c b/Manage.c
index 7e1b94b..eae96e1 100644
--- a/Manage.c
+++ b/Manage.c
@@ -493,14 +493,17 @@ done:
 		rv = 1;
 		goto out;
 	}
-	/* prior to 2.6.28, KOBJ_CHANGE was not sent when an md array
-	 * was stopped, so We'll do it here just to be sure.  Drop any
-	 * partitions as well...
-	 */
-	if (fd >= 0)
-		ioctl(fd, BLKRRPART, 0);
-	if (mdi)
-		sysfs_uevent(mdi, "change");
+
+	if (get_linux_version() < 2006028) {
+		/* prior to 2.6.28, KOBJ_CHANGE was not sent when an md array
+		 * was stopped, so We'll do it here just to be sure.  Drop any
+		 * partitions as well...
+		 */
+		if (fd >= 0)
+			ioctl(fd, BLKRRPART, 0);
+		if (mdi)
+			sysfs_uevent(mdi, "change");
+	}
 
 	if (devnm[0] && use_udev()) {
 		struct map_ent *mp = map_by_devnm(&map, devnm);

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

[Sebastian Parschauer]

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

On 16.02.2016 23:02, Jes Sorensen wrote:
> NeilBrown <neilb@suse.com> writes:
>> On Wed, Feb 17 2016, Jes Sorensen wrote:
>>
>>> Hannes Reinecke <hare@suse.de> writes:
>>>> On 02/16/2016 07:03 PM, Sebastian Parschauer wrote:
>>>>> The worst thing that can happen is that the kernel sends the change
>>>>> event after the remove event. Then it is the current situation again.
>>>>>  From my tests mdadm does enough other stuff in between. Udev is able to
>>>>> handle receiving two remove events from my testing. Multiple mdadm
>>>>> instances can't run in parallel any ways. So userspace around it needs
>>>>> some serialization for it any ways. So also stopping an MD device and
>>>>> assembling a new one with the same minor number shouldn't race.
>>>>>
>>>>> I still prefer this solution here. But if you decide to drop the udev
>>>>> event sending in mdadm, then I'm also fine with that.
>>>>>
>>>> I strongly prefer removing the udev event generation altogether.
>>>> As this appears to be a carry-over from older kernels, it looks to me
>>>> as being an incomplete conversion:
>>>> At one point udev introduced 'ONLINE' and 'OFFLINE' events, which were
>>>> supposed to be used for this kind of scenario.
>>>> (ONLINE being a companion to 'ADD', and 'OFFLINE' being the companion
>>>> to 'DELETE'). However, later the 'ONLINE' got modified to 'CHANGE',
>>>> and the 'OFFLINE' got dropped completely.
>>>> Or that was the plan.
>>>> So it looks as if the conversion to 'CHANGE' got applied to the
>>>> 'OFFLINE' event, too.
>>>> Hence I strongly recommend to drop it completely, and let the kernel
>>>> or the MD module decide if and when a uevent should be send.
>>>
>>> I am totally fine with this, however we should make mdadm fail if run
>>> against a pre-2.6.28 kernel then.
>>>
>>> Cheers,
>>> Jes
>>
>> I would suggest protecting the
>>
>> 	if (fd >= 0)
>> 		ioctl(fd, BLKRRPART, 0);
>> 	if (mdi)
>> 		sysfs_uevent(mdi, "change");
>>
>> code with
>>
>>    if (get_linux_version() < 2006028)
>>
>> That should be completely safe - 2.6.28 and later do this (if needed).
> 
> Seems a better fix to me. I much prefer the duplicated events.
> 
> Sebastian, does this patch resolve the problem for you? If nobody
> hollors, I will push this into mdadm.

I've tested this on top of mdadm-3.4 and with vanilla kernel 4.5.0-rc3.
The device nodes are still there. I guess this is because of the change
event from the kernel. This confirms my former tests.

Run 1, 2: one of two device nodes gone
Run 3: both device nodes gone
Run 4: both device nodes are there (again)

It only works if the kernel sends the remove event instead of the change
event as well. So I'm only fine with this if the kernel change gets
accepted as well.

I create two MD devices (md0 and md1) on top of LVM. Then I check if the
creation worked, stop the devices and check the devices again. For a
clean state afterwards, I clean up everything and check devices again in
my script. I have 5s sleep between each action to get final states.

I've attached my test script.

> commit 7856fa44b8f0bc217a6bbcb5f7c51b2f03717655
> Author: Jes Sorensen <Jes.Sorensen@redhat.com>
> Date:   Tue Feb 16 16:58:36 2016 -0500
> 
>     Manage.c: Only issue change events for kernels older then 2.6.28

Shouldn't the prefix be "Manage:" or "Manage/stop:"? Here is a typo.
Should be "than" instead of "then" here.

>     
>     2.6.28+ kernels handle this themselves and issuing the event here can
>     cause a race.
>     
>     Reported-by: Sebastian Parschauer <sebastian.riemer@profitbricks.com>
>     Suggested-by: NeilBrown <neilb@suse.com>
>     Signed-off-by: Jes Sorensen <Jes.Sorensen@redhat.com>
> 
> diff --git a/Manage.c b/Manage.c
> index 7e1b94b..eae96e1 100644
> --- a/Manage.c
> +++ b/Manage.c
> @@ -493,14 +493,17 @@ done:
>  		rv = 1;
>  		goto out;
>  	}
> -	/* prior to 2.6.28, KOBJ_CHANGE was not sent when an md array
> -	 * was stopped, so We'll do it here just to be sure.  Drop any
> -	 * partitions as well...
> -	 */
> -	if (fd >= 0)
> -		ioctl(fd, BLKRRPART, 0);
> -	if (mdi)
> -		sysfs_uevent(mdi, "change");
> +
> +	if (get_linux_version() < 2006028) {
> +		/* prior to 2.6.28, KOBJ_CHANGE was not sent when an md array
> +		 * was stopped, so We'll do it here just to be sure.  Drop any
> +		 * partitions as well...
> +		 */
> +		if (fd >= 0)
> +			ioctl(fd, BLKRRPART, 0);
> +		if (mdi)
> +			sysfs_uevent(mdi, "change");
> +	}
>  
>  	if (devnm[0] && use_udev()) {
>  		struct map_ent *mp = map_by_devnm(&map, devnm);
> 


[-- Attachment #2: test_mdadm_stop.sh.txt --]
[-- Type: text/plain, Size: 836 bytes --]

#!/bin/bash

NUM_VOL=2

remove_devices() {
    for ((i=0;i<${NUM_VOL};i++)); do
        rm -fv /dev/md${i}
    done
}

check_devices() {
    ls -l /dev/disk/by-id/md*
    ls -d /dev/md*
    ls -d /sys/block/md*
    cat /proc/mdstat
}

stop_devices() {
    for ((i=0;i<${NUM_VOL};i++)); do
        mdadm --stop /dev/md${i} 2>&1
    done
}

create_devices() {
    for ((i=0;i<${NUM_VOL};i++)); do
        mdadm --zero-superblock /dev/dm-${i}
        mdadm -C /dev/md${i} -l 1 -n 2 -e 1.2 \
            --run /dev/dm-${i} missing
    done
}

####### MAIN #######

create_devices

sleep 5

echo "==== CREATE CHECK ===="
check_devices

sleep 5

stop_devices

sleep 5

echo "==== CHECK 1 ===="
check_devices

stop_devices
remove_devices

sleep 5

echo "==== CHECK 2 ===="
check_devices

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

[Hannes Reinecke]

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

On 02/16/2016 09:46 PM, NeilBrown wrote:
> On Wed, Feb 17 2016, Jes Sorensen wrote:
> 
>> Hannes Reinecke <hare@suse.de> writes:
>>> On 02/16/2016 07:03 PM, Sebastian Parschauer wrote:
>>>> On 16.02.2016 18:41, Jes Sorensen wrote:
>>>>> Sebastian Parschauer
>>>>> <sebastian.riemer@profitbricks.com> writes:
>>>>>> 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.
>>>>>> 
>>>>>> This also overrides the change event sent by the
>>>>>> kernel.
>>>>>> 
>>>>>> Signed-off-by: Sebastian Parschauer
>>>>>> <sebastian.riemer@profitbricks.com> --- Manage.c |
>>>>>> 6 +++--- 1 file changed, 3 insertions(+), 3
>>>>>> deletions(-)
>>>>>> 
>>>>>> diff --git a/Manage.c b/Manage.c index
>>>>>> 7e1b94b..bc89764 100644 --- a/Manage.c +++
>>>>>> b/Manage.c @@ -494,13 +494,13 @@ done: goto out; } /*
>>>>>> prior to 2.6.28, KOBJ_CHANGE was not sent when an md
>>>>>> array -	 * was stopped, so We'll do it here just to
>>>>>> be sure.  Drop any -	 * partitions as well... +	 *
>>>>>> was stopped, it should be KOBJ_REMOVE instead, so we
>>>>>> set the +	 * remove event here just to be sure. Drop
>>>>>> any partitions as well... */ if (fd >= 0) ioctl(fd,
>>>>>> BLKRRPART, 0); if (mdi) -		sysfs_uevent(mdi,
>>>>>> "change"); +		sysfs_uevent(mdi, "remove");
>>>>> 
>>>>> I am a little concerned about this change. You assume
>>>>> the kernel and mdadm will be updated in sync, which is
>>>>> unlikely to happen. I believe you need to match the
>>>>> kernel version and send the corresponding event 
>>>>> currectly for this to work correctly?
>>>> 
>>>> The worst thing that can happen is that the kernel sends
>>>> the change event after the remove event. Then it is the
>>>> current situation again. From my tests mdadm does enough
>>>> other stuff in between. Udev is able to handle receiving
>>>> two remove events from my testing. Multiple mdadm 
>>>> instances can't run in parallel any ways. So userspace
>>>> around it needs some serialization for it any ways. So
>>>> also stopping an MD device and assembling a new one with
>>>> the same minor number shouldn't race.
>>>> 
>>>> I still prefer this solution here. But if you decide to
>>>> drop the udev event sending in mdadm, then I'm also fine
>>>> with that.
>>>> 
>>> I strongly prefer removing the udev event generation
>>> altogether. As this appears to be a carry-over from older
>>> kernels, it looks to me as being an incomplete conversion: 
>>> At one point udev introduced 'ONLINE' and 'OFFLINE' events,
>>> which were supposed to be used for this kind of scenario. 
>>> (ONLINE being a companion to 'ADD', and 'OFFLINE' being the
>>> companion to 'DELETE'). However, later the 'ONLINE' got
>>> modified to 'CHANGE', and the 'OFFLINE' got dropped
>>> completely. Or that was the plan. So it looks as if the
>>> conversion to 'CHANGE' got applied to the 'OFFLINE' event,
>>> too. Hence I strongly recommend to drop it completely, and
>>> let the kernel or the MD module decide if and when a uevent
>>> should be send.
>> 
>> I am totally fine with this, however we should make mdadm
>> fail if run against a pre-2.6.28 kernel then.
>> 
>> Cheers, Jes
> 
> I would suggest protecting the
> 
> if (fd >= 0) ioctl(fd, BLKRRPART, 0); if (mdi) 
> sysfs_uevent(mdi, "change");
> 
> code with
> 
> if (get_linux_version() < 2006028)
> 
> That should be completely safe - 2.6.28 and later do this (if
> needed).
> 
+1.

Yes, this is the best solution.

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

iQIcBAEBAgAGBQJWxBs6AAoJEGz4yi9OyKjP8a0P/RqXuKmFFOcBXnST1f1ZWq24
gExfzeo8VAQicWi/CBFu+lumePOiypzKfP0NfHw4PDGPYuLQQq6OXRmiQCqhWz/p
EBRZ9a8NyUIjUpra2j66IiMGwh1KGYl9AeIZmvGTNVkNcOySKdJxLWkFnI6wLwvU
YmUez04UFxEleynt5c00ZYvioYnVchVDNEc4/8yTG6jAUg4+6Q7tTw8vvR3wko+K
vmDbUyUz+q8R5tyTjCKB/KWgMPnMUv+wYoZx+jWtpRyUO6a76U9T5if+ZKk4EUkb
NBHy76L6/YxvOhJqAuX9dMiPDADgHVzD5mnzTSzt9HF/ArXBXtEMcaMxhLPYpLTU
lY7FQqzQ5sGQKbo0nm3EHrLjIP1bWe3BKaniyFQG39wlzdhhFGCzJzHnl1KwSnu6
gy+/AuQSibvxUQhD/ZO4+AJjMq1sXLwRlwwPFr/pI8wrcIqFIUuZG0JjY9NY2UQ2
+povgSj4UnXpRS7BKjvmN/VyUIbnXzf8cpB8w2qwxI5nSwKgjhSNz+o3NeCDQqpw
u2E0MIciPDKXb2GnfPA2+Depm8VfcL9uaRNbHmnV9shIlRsQLB9/IzzVA5Cf5T3f
GA9pHLKEM2LHAWqPmUVIghzyTTj5CXsZrH2GdJVyNTc1bymDBKmQbbiO+IVIHg45
XnJ7L15O6ZTXEW1WMNYT
=okXv
-----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 2/2] Manage: Inform udev about device removal when stopping

[Jes Sorensen]

Hannes Reinecke <hare@suse.de> writes:
> On 02/16/2016 09:46 PM, NeilBrown wrote:
>> On Wed, Feb 17 2016, Jes Sorensen wrote:
>>> 
>>> I am totally fine with this, however we should make mdadm
>>> fail if run against a pre-2.6.28 kernel then.
>>> 
>>> Cheers, Jes
>> 
>> I would suggest protecting the
>> 
>> if (fd >= 0) ioctl(fd, BLKRRPART, 0); if (mdi) 
>> sysfs_uevent(mdi, "change");
>> 
>> code with
>> 
>> if (get_linux_version() < 2006028)
>> 
>> That should be completely safe - 2.6.28 and later do this (if
>> needed).
>> 
> +1.
>
> Yes, this is the best solution.

Sebastian indicates it only works if the kernel patch he submitted is
applied too - should we tweak the mdadm version check to match the next
upstream kernel, or stick with it as is here?

Jes

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

[Sebastian Parschauer]

On 17.02.2016 14:06, Jes Sorensen wrote:
> Hannes Reinecke <hare@suse.de> writes:
>> On 02/16/2016 09:46 PM, NeilBrown wrote:
>>> On Wed, Feb 17 2016, Jes Sorensen wrote:
>>>>
>>>> I am totally fine with this, however we should make mdadm
>>>> fail if run against a pre-2.6.28 kernel then.
>>>>
>>>> Cheers, Jes
>>>
>>> I would suggest protecting the
>>>
>>> if (fd >= 0) ioctl(fd, BLKRRPART, 0); if (mdi) 
>>> sysfs_uevent(mdi, "change");
>>>
>>> code with
>>>
>>> if (get_linux_version() < 2006028)
>>>
>>> That should be completely safe - 2.6.28 and later do this (if
>>> needed).
>>>
>> +1.
>>
>> Yes, this is the best solution.
> 
> Sebastian indicates it only works if the kernel patch he submitted is
> applied too - should we tweak the mdadm version check to match the next
> upstream kernel, or stick with it as is here?

Sorry, it also works if dropping the sending of the change event in the
kernel as well. This seems to be the preferred solution so far. So for
kernels still sending the change event, the problem is not fixed this
way. But your mdadm commit also doesn't make it worse.

Cheers,
Sebastian

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

[Jes Sorensen]

Sebastian Parschauer <sebastian.riemer@profitbricks.com> writes:
> On 17.02.2016 14:06, Jes Sorensen wrote:
>> Hannes Reinecke <hare@suse.de> writes:
>>> On 02/16/2016 09:46 PM, NeilBrown wrote:
>>>> On Wed, Feb 17 2016, Jes Sorensen wrote:
>>>>>
>>>>> I am totally fine with this, however we should make mdadm
>>>>> fail if run against a pre-2.6.28 kernel then.
>>>>>
>>>>> Cheers, Jes
>>>>
>>>> I would suggest protecting the
>>>>
>>>> if (fd >= 0) ioctl(fd, BLKRRPART, 0); if (mdi) 
>>>> sysfs_uevent(mdi, "change");
>>>>
>>>> code with
>>>>
>>>> if (get_linux_version() < 2006028)
>>>>
>>>> That should be completely safe - 2.6.28 and later do this (if
>>>> needed).
>>>>
>>> +1.
>>>
>>> Yes, this is the best solution.
>> 
>> Sebastian indicates it only works if the kernel patch he submitted is
>> applied too - should we tweak the mdadm version check to match the next
>> upstream kernel, or stick with it as is here?
>
> Sorry, it also works if dropping the sending of the change event in the
> kernel as well. This seems to be the preferred solution so far. So for
> kernels still sending the change event, the problem is not fixed this
> way. But your mdadm commit also doesn't make it worse.

Since there is pretty broad agreement on this approach, I have pushed
the fix out for mdadm.

Jes