[PATCH 0/4] mdmonitor improvements

[PATCH 0/4] mdmonitor improvements

[Mariusz Tkaczyk]

This patchset is targeting issues observed across distributions:
- polling on a wrong resource when mdstat is empty
- eventing for external containers
- dealing with udev and mdadm
- quiet fail if other instance is running

Mdmonitor is started automitically if needed by udev. This patchset
introduces mdmonitor stoping if no redundant array presents.

Blazej Kucman (2):
  mdmonitor: set small delay once
  Check if other Monitor instance running before fork.

Mariusz Tkaczyk (2):
  Monitor: refresh mdstat fd after select
  Monitor: stop notifing about containers.

 Monitor.c | 83 ++++++++++++++++++++++++++++++++++++++++---------------
 mdadm.h   |  2 +-
 mdstat.c  | 20 +++++++++++---
 3 files changed, 77 insertions(+), 28 deletions(-)

-- 
2.25.0

[PATCH 3/4] mdmonitor: set small delay once

[Mariusz Tkaczyk]

From: Blazej Kucman <blazej.kucman@intel.com>

If mdmonitor is awakened by event, set small delay once
to deal with udev and mdadm.

Signed-off-by: Blazej Kucman <blazej.kucman@intel.com>
Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com>
---
 Monitor.c | 14 +++++++++++++-
 mdadm.h   |  2 +-
 mdstat.c  | 18 +++++++++++++++---
 3 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/Monitor.c b/Monitor.c
index aed7a692..0fb4f77d 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -128,6 +128,7 @@ 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();
@@ -249,7 +250,18 @@ int Monitor(struct mddev_dev *devlist,
 				break;
 			}
 			else {
-				mdstat_wait(c->delay);
+				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 (wait_result != 0) {
+					if (c->delay > 5)
+						delay_for_event = 5;
+				} else
+					delay_for_event = c->delay;
+
 				mdstat_close();
 			}
 		}
diff --git a/mdadm.h b/mdadm.h
index 399478b8..4961c0f9 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -628,7 +628,7 @@ struct mdstat_ent {
 extern struct mdstat_ent *mdstat_read(int hold, int start);
 extern void mdstat_close(void);
 extern void free_mdstat(struct mdstat_ent *ms);
-extern void mdstat_wait(int seconds);
+extern int mdstat_wait(int seconds);
 extern void mdstat_wait_fd(int fd, const sigset_t *sigmask);
 extern int mddev_busy(char *devnm);
 extern struct mdstat_ent *mdstat_by_component(char *name);
diff --git a/mdstat.c b/mdstat.c
index 48559e64..dd96cca7 100644
--- a/mdstat.c
+++ b/mdstat.c
@@ -302,7 +302,17 @@ void mdstat_close(void)
 	mdstat_fd = -1;
 }
 
-void mdstat_wait(int seconds)
+/*
+ * function: mdstat_wait
+ * Description: Function waits for event on mdstat.
+ * Parameters:
+ *		seconds - timeout for waiting
+ * Returns:
+ *		> 0 - detected event
+ *		0 - timeout
+ *		< 0 - detected error
+ */
+int mdstat_wait(int seconds)
 {
 	fd_set fds;
 	struct timeval tm;
@@ -312,10 +322,12 @@ void mdstat_wait(int seconds)
 		FD_SET(mdstat_fd, &fds);
 		maxfd = mdstat_fd;
 	} else
-		return;
+		return -1;
+
 	tm.tv_sec = seconds;
 	tm.tv_usec = 0;
-	select(maxfd + 1, NULL, NULL, &fds, &tm);
+
+	return select(maxfd + 1, NULL, NULL, &fds, &tm);
 }
 
 void mdstat_wait_fd(int fd, const sigset_t *sigmask)
-- 
2.25.0

[PATCH 4/4] Check if other Monitor instance running before fork.

[Mariusz Tkaczyk]

From: Blazej Kucman <blazej.kucman@intel.com>

Make error message visible to the user.

Signed-off-by: Blazej Kucman <blazej.kucman@intel.com>
Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com>
---
 Monitor.c | 44 ++++++++++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 16 deletions(-)

diff --git a/Monitor.c b/Monitor.c
index 0fb4f77d..7fd48084 100644
--- a/Monitor.c
+++ b/Monitor.c
@@ -63,6 +63,7 @@ struct alert_info {
 };
 static int make_daemon(char *pidfile);
 static int check_one_sharer(int scan);
+static void write_autorebuild_pid(void);
 static void alert(char *event, char *dev, char *disc, struct alert_info *info);
 static int check_array(struct state *st, struct mdstat_ent *mdstat,
 		       int test, struct alert_info *info,
@@ -153,6 +154,11 @@ int Monitor(struct mddev_dev *devlist,
 	info.mailfrom = mailfrom;
 	info.dosyslog = dosyslog;
 
+	if (share){
+		if (check_one_sharer(c->scan))
+			return 1;
+	}
+
 	if (daemonise) {
 		int rv = make_daemon(pidfile);
 		if (rv >= 0)
@@ -160,8 +166,7 @@ int Monitor(struct mddev_dev *devlist,
 	}
 
 	if (share)
-		if (check_one_sharer(c->scan))
-			return 1;
+		write_autorebuild_pid();
 
 	if (devlist == NULL) {
 		mdlist = conf_get_ident(NULL);
@@ -328,8 +333,8 @@ static int check_one_sharer(int scan)
 	int pid;
 	FILE *comm_fp;
 	FILE *fp;
-	char comm_path[100];
-	char path[100];
+	char comm_path[PATH_MAX];
+	char path[PATH_MAX];
 	char comm[20];
 
 	sprintf(path, "%s/autorebuild.pid", MDMON_DIR);
@@ -356,21 +361,28 @@ static int check_one_sharer(int scan)
 		}
 		fclose(fp);
 	}
-	if (scan) {
-		if (mkdir(MDMON_DIR, S_IRWXU) < 0 && errno != EEXIST) {
+	return 0;
+}
+
+static void write_autorebuild_pid()
+{
+	char path[PATH_MAX];
+	int pid;
+	FILE *fp;
+	sprintf(path, "%s/autorebuild.pid", MDMON_DIR);
+
+	if (mkdir(MDMON_DIR, S_IRWXU) < 0 && errno != EEXIST) {
+		pr_err("Can't create autorebuild.pid file\n");
+	} else {
+		fp = fopen(path, "w");
+		if (!fp)
 			pr_err("Can't create autorebuild.pid file\n");
-		} else {
-			fp = fopen(path, "w");
-			if (!fp)
-				pr_err("Cannot create autorebuild.pidfile\n");
-			else {
-				pid = getpid();
-				fprintf(fp, "%d\n", pid);
-				fclose(fp);
-			}
+		else {
+			pid = getpid();
+			fprintf(fp, "%d\n", pid);
+			fclose(fp);
 		}
 	}
-	return 0;
 }
 
 static void alert(char *event, char *dev, char *disc, struct alert_info *info)
-- 
2.25.0

Re: [PATCH 3/4] mdmonitor: set small delay once

[Nix]

On 9 Sep 2020, Mariusz Tkaczyk uttered the following:

> +				/*
> +				 * If mdmonitor is awaken by event, set small delay once
> +				 * to deal with udev and mdadm.
> +				 */
> +				if (wait_result != 0) {
> +					if (c->delay > 5)
> +						delay_for_event = 5;
> +				} else
> +					delay_for_event = c->delay;

This is racy: if any delay is needed, any finite delay value will
now and then be too short.

I think this should be fixed by arranging for mdmonitor to be signalled
when udev or whatever has finished whatever it's doing. (udev has lots
of ways it could be asked to do that.)

RE: [PATCH 0/4] mdmonitor improvements

[Tkaczyk, Mariusz]

Hi Jes,
Do you receive whole patchset?
If not let me know. I will send it again.
I don't know why two patches were lost, I cannot find them anywhere on linux-raid.

Thanks,
Mariusz

-----Original Message-----
From: linux-raid-owner@vger.kernel.org <linux-raid-owner@vger.kernel.org> On Behalf Of Mariusz Tkaczyk
Sent: Wednesday, September 9, 2020 10:31 AM
To: jes@trained-monkey.org
Cc: linux-raid@vger.kernel.org
Subject: [PATCH 0/4] mdmonitor improvements

This patchset is targeting issues observed across distributions:
- polling on a wrong resource when mdstat is empty
- eventing for external containers
- dealing with udev and mdadm
- quiet fail if other instance is running

Mdmonitor is started automitically if needed by udev. This patchset introduces mdmonitor stoping if no redundant array presents.

Blazej Kucman (2):
  mdmonitor: set small delay once
  Check if other Monitor instance running before fork.

Mariusz Tkaczyk (2):
  Monitor: refresh mdstat fd after select
  Monitor: stop notifing about containers.

 Monitor.c | 83 ++++++++++++++++++++++++++++++++++++++++---------------
 mdadm.h   |  2 +-
 mdstat.c  | 20 +++++++++++---
 3 files changed, 77 insertions(+), 28 deletions(-)

--
2.25.0

Re: [PATCH 0/4] mdmonitor improvements

[Jes Sorensen]

On 9/29/20 10:34 AM, Tkaczyk, Mariusz wrote:
> Hi Jes,
> Do you receive whole patchset?
> If not let me know. I will send it again.
> I don't know why two patches were lost, I cannot find them anywhere on linux-raid.
> 
> Thanks,
> Mariusz

Hi Mariusz,

I got four patches. Sorry I have been completely swamped with other
stuff at work the last couple of months, so I have gotten behind on this.

I'll try and go through them now.

Thanks,
Jes

> -----Original Message-----
> From: linux-raid-owner@vger.kernel.org <linux-raid-owner@vger.kernel.org> On Behalf Of Mariusz Tkaczyk
> Sent: Wednesday, September 9, 2020 10:31 AM
> To: jes@trained-monkey.org
> Cc: linux-raid@vger.kernel.org
> Subject: [PATCH 0/4] mdmonitor improvements
> 
> This patchset is targeting issues observed across distributions:
> - polling on a wrong resource when mdstat is empty
> - eventing for external containers
> - dealing with udev and mdadm
> - quiet fail if other instance is running
> 
> Mdmonitor is started automitically if needed by udev. This patchset introduces mdmonitor stoping if no redundant array presents.
> 
> Blazej Kucman (2):
>   mdmonitor: set small delay once
>   Check if other Monitor instance running before fork.
> 
> Mariusz Tkaczyk (2):
>   Monitor: refresh mdstat fd after select
>   Monitor: stop notifing about containers.
> 
>  Monitor.c | 83 ++++++++++++++++++++++++++++++++++++++++---------------
>  mdadm.h   |  2 +-
>  mdstat.c  | 20 +++++++++++---
>  3 files changed, 77 insertions(+), 28 deletions(-)
> 
> --
> 2.25.0
> 

Re: [PATCH 1/4] Monitor: refresh mdstat fd after select

[Jes Sorensen]

On 9/9/20 4:31 AM, Mariusz Tkaczyk wrote:
> After 52209d6ee118 ("Monitor: release /proc/mdstat fd when no arrays
> present") mdstat fd is closed if mdstat is empty or cannot be opened.
> It causes that monitor is not able to select on mdstat. Select
> doesn't fail because it gets valid descriptor to a different resource.
> As a result any new event will be unnoticed until timeout (delay).
> 
> Refresh mdstat after wake up, don't poll on wrong resource.
> 
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com>
> ---
>  Monitor.c | 6 +++---
>  mdstat.c  | 4 ++--
>  2 files changed, 5 insertions(+), 5 deletions(-)

Applied!

Thanks,
Jes

Re: [PATCH 2/4] Monitor: stop notifing about containers.

[Jes Sorensen]

On 9/9/20 4:31 AM, Mariusz Tkaczyk wrote:
> Stop reporting any events from container but still track them,
> it is important for spare migration.
> Stop mdmonitor if no redundant array is presented in mdstat.
> There is nothing to follow.
> 
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com>
> ---
>  Monitor.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)

Applied!

Thanks,
Jes

Re: [PATCH 3/4] mdmonitor: set small delay once

[Jes Sorensen]

On 9/15/20 7:33 AM, Nix wrote:
> On 9 Sep 2020, Mariusz Tkaczyk uttered the following:
> 
>> +				/*
>> +				 * If mdmonitor is awaken by event, set small delay once
>> +				 * to deal with udev and mdadm.
>> +				 */
>> +				if (wait_result != 0) {
>> +					if (c->delay > 5)
>> +						delay_for_event = 5;
>> +				} else
>> +					delay_for_event = c->delay;
> 
> This is racy: if any delay is needed, any finite delay value will
> now and then be too short.
> 
> I think this should be fixed by arranging for mdmonitor to be signalled
> when udev or whatever has finished whatever it's doing. (udev has lots
> of ways it could be asked to do that.)
> 

Hi

I have applied this patch for now, since it is better than what we had.
However, I agree it would be better to do this using udev or other
signalling, so please have a look at that.

Cheers,
Jes

Re: [PATCH 4/4] Check if other Monitor instance running before fork.

[Jes Sorensen]

On 9/9/20 4:31 AM, Mariusz Tkaczyk wrote:
> From: Blazej Kucman <blazej.kucman@intel.com>
> 
> Make error message visible to the user.
> 
> Signed-off-by: Blazej Kucman <blazej.kucman@intel.com>
> Signed-off-by: Mariusz Tkaczyk <mariusz.tkaczyk@intel.com>
> ---
>  Monitor.c | 44 ++++++++++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 16 deletions(-)

Applied!

Thanks,
Jes