All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] mdmonitor: check if udev has finished events processing
@ 2021-01-14 14:14 Oleksandr Shchirskyi
  2021-03-08 15:23 ` Jes Sorensen
  0 siblings, 1 reply; 9+ messages in thread
From: Oleksandr Shchirskyi @ 2021-01-14 14:14 UTC (permalink / raw)
  To: jes; +Cc: linux-raid

If mdmonitor is awaken by event, wait for udev to finish
events processing, to eliminate the race between udev and mdadm
when spare has been added and need to be moved by mdmonitor

Signed-off-by: Oleksandr Shchirskyi <oleksandr.shchirskyi@intel.com>
---
 Makefile  |  2 +-
 Monitor.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++---------
 2 files changed, 63 insertions(+), 12 deletions(-)

diff --git a/Makefile b/Makefile
index 4cd4c9d8..1e6e1e12 100644
--- a/Makefile
+++ b/Makefile
@@ -119,7 +119,7 @@ endif
 # If you want a static binary, you might uncomment these
 # LDFLAGS = -static
 # STRIP = -s
-LDLIBS=-ldl
+LDLIBS=-ldl -ludev
 
 INSTALL = /usr/bin/install
 DESTDIR =
diff --git a/Monitor.c b/Monitor.c
index 3f3005b8..4c39145b 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -29,6 +29,7 @@
 #include	<signal.h>
 #include	<limits.h>
 #include	<syslog.h>
+#include	<libudev.h>
 
 struct state {
 	char *devname;
@@ -72,6 +73,7 @@ static int add_new_arrays(struct mdstat_ent *mdstat, struct state **statelist,
 			  int test, struct alert_info *info);
 static void try_spare_migration(struct state *statelist, struct alert_info *info);
 static void link_containers_with_subarrays(struct state *list);
+static int check_udev_activity(void);
 
 int Monitor(struct mddev_dev *devlist,
 	    char *mailaddr, char *alert_cmd,
@@ -129,7 +131,6 @@ int Monitor(struct mddev_dev *devlist,
 	char *mailfrom;
 	struct alert_info info;
 	struct mddev_ident *mdlist;
-	int delay_for_event = c->delay;
 
 	if (!mailaddr) {
 		mailaddr = conf_get_mailaddr();
@@ -244,7 +245,7 @@ int Monitor(struct mddev_dev *devlist,
 
 		/* If an array has active < raid && spare == 0 && spare_group != NULL
 		 * Look for another array with spare > 0 and active == raid and same spare_group
-		 *  if found, choose a device and hotremove/hotadd
+		 * if found, choose a device and hotremove/hotadd
 		 */
 		if (share && anydegraded)
 			try_spare_migration(statelist, &info);
@@ -255,17 +256,12 @@ int Monitor(struct mddev_dev *devlist,
 				break;
 			}
 			else {
-				int wait_result = mdstat_wait(delay_for_event);
-
 				/*
-				 * If mdmonitor is awaken by event, set small delay once
-				 * to deal with udev and mdadm.
+				 * If mdmonitor is awaken by event, check for udev activity
+				 * to wait for udev to finish new devices processing.
 				 */
-				if (wait_result != 0) {
-					if (c->delay > 5)
-						delay_for_event = 5;
-				} else
-					delay_for_event = c->delay;
+				if (mdstat_wait(c->delay) && check_udev_activity())
+					pr_err("Error while waiting for UDEV to complete new devices processing\n");
 
 				mdstat_close();
 			}
@@ -1037,6 +1033,61 @@ static void link_containers_with_subarrays(struct state *list)
 				}
 }
 
+
+/* function: check_udev_activity
+ * Description: Function waits for udev to finish
+ * events processing.
+ * Returns:
+ *		1 - detected error while opening udev
+ *		2 - timeout
+ *		0 - successfull completion
+ */
+static int check_udev_activity(void)
+{
+	struct udev *udev = NULL;
+	struct udev_queue *udev_queue = NULL;
+	int timeout_cnt = 30;
+	int rc = 0;
+
+	// in rare cases systemd may not have udev
+	// in such cases just exit with rc 0
+	if (!use_udev())
+		goto out;
+
+	udev = udev_new();
+	if (!udev) {
+		rc = 1;
+		goto out;
+	}
+
+	udev_queue = udev_queue_new(udev);
+	if (!udev_queue) {
+		rc = 1;
+		goto out;
+	}
+
+	if (udev_queue_get_queue_is_empty(udev_queue))
+		goto out;
+
+	while (!udev_queue_get_queue_is_empty(udev_queue)) {
+		sleep(1);
+
+		if (timeout_cnt)
+			timeout_cnt--;
+		else {
+			rc = 2;
+			goto out;
+		}
+	}
+
+out:
+	if (udev_queue)
+		udev_queue_unref(udev_queue);
+	if (udev)
+		udev_unref(udev);
+	return rc;
+}
+
 /* Not really Monitor but ... */
 int Wait(char *dev)
 {
-- 
2.27.0

---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
 


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] mdmonitor: check if udev has finished events processing
  2021-01-14 14:14 [PATCH] mdmonitor: check if udev has finished events processing Oleksandr Shchirskyi
@ 2021-03-08 15:23 ` Jes Sorensen
  2021-03-09  9:01   ` Tkaczyk, Mariusz
  0 siblings, 1 reply; 9+ messages in thread
From: Jes Sorensen @ 2021-03-08 15:23 UTC (permalink / raw)
  To: Oleksandr Shchirskyi; +Cc: linux-raid

On 1/14/21 9:14 AM, Oleksandr Shchirskyi wrote:
> If mdmonitor is awaken by event, wait for udev to finish
> events processing, to eliminate the race between udev and mdadm
> when spare has been added and need to be moved by mdmonitor
> 
> Signed-off-by: Oleksandr Shchirskyi <oleksandr.shchirskyi@intel.com>
> ---
>  Makefile  |  2 +-
>  Monitor.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++---------
>  2 files changed, 63 insertions(+), 12 deletions(-)

Hi,

I think it is reasonable to require libudev in 2021, so I have applied
this. However if someone feels there is a reason to not have this build
requirement, I will also accept a patch to make this dependency optional.

Second, I took the liberty to replace the ugly C++ style // comments
with proper /* */ ones.

Thanks,
Jes


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mdmonitor: check if udev has finished events processing
  2021-03-08 15:23 ` Jes Sorensen
@ 2021-03-09  9:01   ` Tkaczyk, Mariusz
  2021-03-09  9:13     ` Wols Lists
  0 siblings, 1 reply; 9+ messages in thread
From: Tkaczyk, Mariusz @ 2021-03-09  9:01 UTC (permalink / raw)
  To: Jes Sorensen, Oleksandr Shchirskyi; +Cc: linux-raid

On 08.03.2021 16:23, Jes Sorensen wrote:

> I think it is reasonable to require libudev in 2021, so I have applied
> this. However if someone feels there is a reason to not have this build
> requirement, I will also accept a patch to make this dependency optional.

Hi Jes,

If community agrees for adding this dependency, I think that is a good
time to drop all legacy code for handling cases if udev is not available.
This code is dead, we cannot compile mdadm without libudev.

Do you agree?

Mariusz

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mdmonitor: check if udev has finished events processing
  2021-03-09  9:01   ` Tkaczyk, Mariusz
@ 2021-03-09  9:13     ` Wols Lists
  2021-03-09  9:45       ` Michael Fritscher
  0 siblings, 1 reply; 9+ messages in thread
From: Wols Lists @ 2021-03-09  9:13 UTC (permalink / raw)
  To: Tkaczyk, Mariusz, Jes Sorensen, Oleksandr Shchirskyi; +Cc: linux-raid

On 09/03/21 09:01, Tkaczyk, Mariusz wrote:
> On 08.03.2021 16:23, Jes Sorensen wrote:
> 
>> I think it is reasonable to require libudev in 2021, so I have applied
>> this. However if someone feels there is a reason to not have this build
>> requirement, I will also accept a patch to make this dependency optional.
> 
> Hi Jes,
> 
> If community agrees for adding this dependency, I think that is a good
> time to drop all legacy code for handling cases if udev is not available.
> This code is dead, we cannot compile mdadm without libudev.
> 
Is udev part of systemd? Are there alternate implementations for the
anti-systemd-holdouts? Iirc you don't need systemd itself to have udev,
but it might provoke a few screams ...

My current (gentoo) system is OpenRC, but that's still on KDE4 and
hasn't been updated in a couple of years (don't ask why). My new system
is currently being built and is gentoo/systemd, but it's clear the
anti-systemd sentiment is still strong ...

Cheers,
Wol


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mdmonitor: check if udev has finished events processing
  2021-03-09  9:13     ` Wols Lists
@ 2021-03-09  9:45       ` Michael Fritscher
  2021-03-09 11:17         ` Tkaczyk, Mariusz
  0 siblings, 1 reply; 9+ messages in thread
From: Michael Fritscher @ 2021-03-09  9:45 UTC (permalink / raw)
  To: Wols Lists, Tkaczyk, Mariusz, Jes Sorensen, Oleksandr Shchirskyi
  Cc: linux-raid

Am 09.03.21 um 10:13 schrieb Wols Lists:
> Is udev part of systemd? Are there alternate implementations for the
> anti-systemd-holdouts? Iirc you don't need systemd itself to have udev,
> but it might provoke a few screams ...
> 
> My current (gentoo) system is OpenRC, but that's still on KDE4 and
> hasn't been updated in a couple of years (don't ask why). My new system
> is currently being built and is gentoo/systemd, but it's clear the
> anti-systemd sentiment is still strong ...
> 
> Cheers,
> Wol
> 

Good day,

there is e.g. eudev ( https://wiki.gentoo.org/wiki/Project:Eudev ) with
the explicit target to be used without systemd.

Best regards,
Michael Fritscher

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mdmonitor: check if udev has finished events processing
  2021-03-09  9:45       ` Michael Fritscher
@ 2021-03-09 11:17         ` Tkaczyk, Mariusz
  2021-03-09 14:52           ` Jes Sorensen
  0 siblings, 1 reply; 9+ messages in thread
From: Tkaczyk, Mariusz @ 2021-03-09 11:17 UTC (permalink / raw)
  To: Michael Fritscher, Wols Lists, Jes Sorensen, Oleksandr Shchirskyi
  Cc: linux-raid

On 09.03.2021 10:45, Michael Fritscher wrote:
> Am 09.03.21 um 10:13 schrieb Wols Lists:
>> Is udev part of systemd? Are there alternate implementations for the
>> anti-systemd-holdouts? Iirc you don't need systemd itself to have udev,
>> but it might provoke a few screams ...
>>
>> My current (gentoo) system is OpenRC, but that's still on KDE4 and
>> hasn't been updated in a couple of years (don't ask why). My new system
>> is currently being built and is gentoo/systemd, but it's clear the
>> anti-systemd sentiment is still strong ...
>>
>> Cheers,
>> Wol
>>
> 
> Good day,
> 
> there is e.g. eudev ( https://wiki.gentoo.org/wiki/Project:Eudev ) with
> the explicit target to be used without systemd.
> 
> Best regards,
> Michael Fritscher
> 

It is a udev replacement and offers similar functionality.
I'm wondering on configuration without udev (systemd or other forks).
Is it still a case?

Thanks,
Mariusz

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mdmonitor: check if udev has finished events processing
  2021-03-09 11:17         ` Tkaczyk, Mariusz
@ 2021-03-09 14:52           ` Jes Sorensen
  2021-03-10 16:46             ` Tkaczyk, Mariusz
  0 siblings, 1 reply; 9+ messages in thread
From: Jes Sorensen @ 2021-03-09 14:52 UTC (permalink / raw)
  To: Tkaczyk, Mariusz, Michael Fritscher, Wols Lists, Oleksandr Shchirskyi
  Cc: linux-raid

On 3/9/21 6:17 AM, Tkaczyk, Mariusz wrote:
> On 09.03.2021 10:45, Michael Fritscher wrote:
>> Am 09.03.21 um 10:13 schrieb Wols Lists:
>>> Is udev part of systemd? Are there alternate implementations for the
>>> anti-systemd-holdouts? Iirc you don't need systemd itself to have udev,
>>> but it might provoke a few screams ...
>>>
>>> My current (gentoo) system is OpenRC, but that's still on KDE4 and
>>> hasn't been updated in a couple of years (don't ask why). My new system
>>> is currently being built and is gentoo/systemd, but it's clear the
>>> anti-systemd sentiment is still strong ...
>>>
>>> Cheers,
>>> Wol
>>>
>>
>> Good day,
>>
>> there is e.g. eudev ( https://wiki.gentoo.org/wiki/Project:Eudev ) with
>> the explicit target to be used without systemd.
> 
> It is a udev replacement and offers similar functionality.
> I'm wondering on configuration without udev (systemd or other forks).
> Is it still a case?

This is my main concern, small embedded devices that don't use systemd.
I've never been a big systemd fan, but it's how the chips have fallen,
so I am not overly worried about a couple of fanatics.

If eudev can do the trick, that would be great.

Cheers,
Jes

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mdmonitor: check if udev has finished events processing
  2021-03-09 14:52           ` Jes Sorensen
@ 2021-03-10 16:46             ` Tkaczyk, Mariusz
  2021-03-11 12:58               ` Jes Sorensen
  0 siblings, 1 reply; 9+ messages in thread
From: Tkaczyk, Mariusz @ 2021-03-10 16:46 UTC (permalink / raw)
  To: Jes Sorensen, Michael Fritscher, Wols Lists, Oleksandr Shchirskyi
  Cc: linux-raid, artur.paszkiewicz

On 09.03.2021 15:52, Jes Sorensen wrote:
> On 3/9/21 6:17 AM, Tkaczyk, Mariusz wrote:
>> On 09.03.2021 10:45, Michael Fritscher wrote:
>>> Am 09.03.21 um 10:13 schrieb Wols Lists:
>>>> Is udev part of systemd? Are there alternate implementations for the
>>>> anti-systemd-holdouts? Iirc you don't need systemd itself to have udev,
>>>> but it might provoke a few screams ...
>>>>
>>>> My current (gentoo) system is OpenRC, but that's still on KDE4 and
>>>> hasn't been updated in a couple of years (don't ask why). My new system
>>>> is currently being built and is gentoo/systemd, but it's clear the
>>>> anti-systemd sentiment is still strong ...
>>>>
>>>> Cheers,
>>>> Wol
>>>>
>>>
>>> Good day,
>>>
>>> there is e.g. eudev ( https://wiki.gentoo.org/wiki/Project:Eudev ) with
>>> the explicit target to be used without systemd.
>>
>> It is a udev replacement and offers similar functionality.
>> I'm wondering on configuration without udev (systemd or other forks).
>> Is it still a case?
> 
> This is my main concern, small embedded devices that don't use systemd.
> I've never been a big systemd fan, but it's how the chips have fallen,
> so I am not overly worried about a couple of fanatics.
> 
> If eudev can do the trick, that would be great.
> 
> Cheers,
> Jes
> 
Hello,

Mdadm is in use in openwrt without udev (thanks to Artur for research).
They are using hotplug scripts:
https://openwrt.org/docs/guide-user/base-system/hotplug#coldplug
To provide compatibility with libudev dependency they wrote small shim:
https://openwrt.org/packages/pkgdata/libudev-fbsd
Unfortunately, not all libudev functions are defined, mdadm compilation
there might be problematic:
https://openwrt.org/packages/pkgdata_lede17_1/mdadm

Now, it makes sense to define libudev as optional dependency.
It should be done before new release, Intel will do that.

Anyway, I still think that we should drop udev detection from mdadm.
I there is no systemd-udevd, we should expect other tool to provide
similar functionality, like hotplug scripts, eudev.
IMO mdadm doesn't need to create any link.

Mariusz


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] mdmonitor: check if udev has finished events processing
  2021-03-10 16:46             ` Tkaczyk, Mariusz
@ 2021-03-11 12:58               ` Jes Sorensen
  0 siblings, 0 replies; 9+ messages in thread
From: Jes Sorensen @ 2021-03-11 12:58 UTC (permalink / raw)
  To: Tkaczyk, Mariusz, Michael Fritscher, Wols Lists, Oleksandr Shchirskyi
  Cc: linux-raid, artur.paszkiewicz

On 3/10/21 11:46 AM, Tkaczyk, Mariusz wrote:
> On 09.03.2021 15:52, Jes Sorensen wrote:
>> This is my main concern, small embedded devices that don't use systemd.
>> I've never been a big systemd fan, but it's how the chips have fallen,
>> so I am not overly worried about a couple of fanatics.
>>
>> If eudev can do the trick, that would be great.
>>
>> Cheers,
>> Jes
>>
> Hello,
> 
> Mdadm is in use in openwrt without udev (thanks to Artur for research).
> They are using hotplug scripts:
> https://openwrt.org/docs/guide-user/base-system/hotplug#coldplug
> To provide compatibility with libudev dependency they wrote small shim:
> https://openwrt.org/packages/pkgdata/libudev-fbsd
> Unfortunately, not all libudev functions are defined, mdadm compilation
> there might be problematic:
> https://openwrt.org/packages/pkgdata_lede17_1/mdadm
> 
> Now, it makes sense to define libudev as optional dependency.
> It should be done before new release, Intel will do that.
> 
> Anyway, I still think that we should drop udev detection from mdadm.
> I there is no systemd-udevd, we should expect other tool to provide
> similar functionality, like hotplug scripts, eudev.
> IMO mdadm doesn't need to create any link.

Thanks for checking into this and taking care of it, appreciate it!

Cheers,
Jes


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-03-11 12:59 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-14 14:14 [PATCH] mdmonitor: check if udev has finished events processing Oleksandr Shchirskyi
2021-03-08 15:23 ` Jes Sorensen
2021-03-09  9:01   ` Tkaczyk, Mariusz
2021-03-09  9:13     ` Wols Lists
2021-03-09  9:45       ` Michael Fritscher
2021-03-09 11:17         ` Tkaczyk, Mariusz
2021-03-09 14:52           ` Jes Sorensen
2021-03-10 16:46             ` Tkaczyk, Mariusz
2021-03-11 12:58               ` Jes Sorensen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.