[PATCH 0/2] New timeout while waiting for mdmon

[PATCH 0/2] New timeout while waiting for mdmon

[Kinga Stefaniuk]

This series of patches contains adding new timeout
which is needed to have mdmon started completely.

Kinga Stefaniuk (2):
  util.c: change devnm to const in mdmon functions
  Wait for mdmon when it is stared via systemd

 Assemble.c |  4 ++--
 Grow.c     |  7 ++++---
 mdadm.h    |  6 ++++--
 util.c     | 33 +++++++++++++++++++++++++++++++--
 4 files changed, 41 insertions(+), 9 deletions(-)

-- 
2.35.3

[PATCH 1/2] util.c: change devnm to const in mdmon functions

[Kinga Stefaniuk]

Devnm shall not be changed inside mdmon_running()
and mdmon_pid() functions, change this parameter to const.

Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com>
---
 mdadm.h | 4 ++--
 util.c  | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/mdadm.h b/mdadm.h
index ae2106a2f28f..af4c484afdf7 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1767,8 +1767,8 @@ extern int is_subarray_active(char *subarray, char *devname);
 extern int open_subarray(char *dev, char *subarray, struct supertype *st, int quiet);
 extern struct superswitch *version_to_superswitch(char *vers);
 
-extern int mdmon_running(char *devnm);
-extern int mdmon_pid(char *devnm);
+extern int mdmon_running(const char *devnm);
+extern int mdmon_pid(const char *devnm);
 extern int check_env(char *name);
 extern __u32 random32(void);
 extern void random_uuid(__u8 *buf);
diff --git a/util.c b/util.c
index 9e8370450a8d..65056a19e2cd 100644
--- a/util.c
+++ b/util.c
@@ -1891,7 +1891,7 @@ unsigned long long min_recovery_start(struct mdinfo *array)
 	return recovery_start;
 }
 
-int mdmon_pid(char *devnm)
+int mdmon_pid(const char *devnm)
 {
 	char path[100];
 	char pid[10];
@@ -1911,7 +1911,7 @@ int mdmon_pid(char *devnm)
 	return atoi(pid);
 }
 
-int mdmon_running(char *devnm)
+int mdmon_running(const char *devnm)
 {
 	int pid = mdmon_pid(devnm);
 	if (pid <= 0)
-- 
2.35.3

[PATCH 2/2] Wait for mdmon when it is stared via systemd

[Kinga Stefaniuk]

When mdmon is being started it may need few seconds to start.
For now, we didn't wait for it. Introduce wait_for_mdmon()
function, which waits up to 5 seconds for mdmon to start completely.

Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com>
---
 Assemble.c |  4 ++--
 Grow.c     |  7 ++++---
 mdadm.h    |  2 ++
 util.c     | 29 +++++++++++++++++++++++++++++
 4 files changed, 37 insertions(+), 5 deletions(-)

diff --git a/Assemble.c b/Assemble.c
index f6c5b99e25e2..9cb1747df0a3 100644
--- a/Assemble.c
+++ b/Assemble.c
@@ -2175,8 +2175,8 @@ int assemble_container_content(struct supertype *st, int mdfd,
 			if (!mdmon_running(st->container_devnm))
 				start_mdmon(st->container_devnm);
 			ping_monitor(st->container_devnm);
-			if (mdmon_running(st->container_devnm) &&
-			    st->update_tail == NULL)
+			if (wait_for_mdmon(st->container_devnm) == MDADM_STATUS_SUCCESS &&
+			    !st->update_tail)
 				st->update_tail = &st->updates;
 		}
 
diff --git a/Grow.c b/Grow.c
index 074f19956e17..0e44fae4891e 100644
--- a/Grow.c
+++ b/Grow.c
@@ -2085,7 +2085,7 @@ int Grow_reshape(char *devname, int fd,
 			if (!mdmon_running(st->container_devnm))
 				start_mdmon(st->container_devnm);
 			ping_monitor(container);
-			if (mdmon_running(st->container_devnm) == false) {
+			if (wait_for_mdmon(st->container_devnm) != MDADM_STATUS_SUCCESS) {
 				pr_err("No mdmon found. Grow cannot continue.\n");
 				goto release;
 			}
@@ -3176,7 +3176,8 @@ static int reshape_array(char *container, int fd, char *devname,
 			if (!mdmon_running(container))
 				start_mdmon(container);
 			ping_monitor(container);
-			if (mdmon_running(container) && st->update_tail == NULL)
+			if (wait_for_mdmon(container) == MDADM_STATUS_SUCCESS &&
+			    !st->update_tail)
 				st->update_tail = &st->updates;
 		}
 	}
@@ -5140,7 +5141,7 @@ int Grow_continue_command(char *devname, int fd,
 			start_mdmon(container);
 		ping_monitor(container);
 
-		if (mdmon_running(container) == false) {
+		if (wait_for_mdmon(container) != MDADM_STATUS_SUCCESS) {
 			pr_err("No mdmon found. Grow cannot continue.\n");
 			ret_val = 1;
 			goto Grow_continue_command_exit;
diff --git a/mdadm.h b/mdadm.h
index af4c484afdf7..9b8fb3f6f8d8 100644
--- a/mdadm.h
+++ b/mdadm.h
@@ -1769,6 +1769,8 @@ extern struct superswitch *version_to_superswitch(char *vers);
 
 extern int mdmon_running(const char *devnm);
 extern int mdmon_pid(const char *devnm);
+extern mdadm_status_t wait_for_mdmon(const char *devnm);
+
 extern int check_env(char *name);
 extern __u32 random32(void);
 extern void random_uuid(__u8 *buf);
diff --git a/util.c b/util.c
index 65056a19e2cd..df12cf2bb2b1 100644
--- a/util.c
+++ b/util.c
@@ -1921,6 +1921,35 @@ int mdmon_running(const char *devnm)
 	return 0;
 }
 
+/*
+ * wait_for_mdmon() - Waits for mdmon within specified time.
+ * @devnm: Device for which mdmon should start.
+ *
+ * Function waits for mdmon to start. It may need few seconds
+ * to start, we set timeout to 5, it should be sufficient.
+ * Do not wait if mdmon has been started.
+ *
+ * Return: MDADM_STATUS_SUCCESS if mdmon is running, error code otherwise.
+ */
+mdadm_status_t wait_for_mdmon(const char *devnm)
+{
+	const time_t mdmon_timeout = 5;
+	time_t start_time = time(0);
+
+	if (mdmon_running(devnm))
+		return MDADM_STATUS_SUCCESS;
+
+	pr_info("Waiting for mdmon to start\n");
+	while (time(0) - start_time < mdmon_timeout) {
+		sleep_for(0, MSEC_TO_NSEC(200), true);
+		if (mdmon_running(devnm))
+			return MDADM_STATUS_SUCCESS;
+	};
+
+	pr_err("Timeout waiting for mdmon\n");
+	return MDADM_STATUS_ERROR;
+}
+
 int start_mdmon(char *devnm)
 {
 	int i;
-- 
2.35.3

Re: [PATCH 0/2] New timeout while waiting for mdmon

[Paul E Luse]

On Tue,  7 May 2024 05:38:54 +0200
Kinga Stefaniuk <kinga.stefaniuk@intel.com> wrote:

> This series of patches contains adding new timeout
> which is needed to have mdmon started completely.
> 

Thanks Kinga!  What is the end user experience w/o this patch? (ie what
negative impact does this patch address? mystery hang?  missing events?)

-Paul

> Kinga Stefaniuk (2):
>   util.c: change devnm to const in mdmon functions
>   Wait for mdmon when it is stared via systemd
> 
>  Assemble.c |  4 ++--
>  Grow.c     |  7 ++++---
>  mdadm.h    |  6 ++++--
>  util.c     | 33 +++++++++++++++++++++++++++++++--
>  4 files changed, 41 insertions(+), 9 deletions(-)
> 

Re: [PATCH 0/2] New timeout while waiting for mdmon

[Kinga Tanska]

On Mon, 6 May 2024 21:28:59 -0700
Paul E Luse <paul.e.luse@linux.intel.com> wrote:

> On Tue,  7 May 2024 05:38:54 +0200
> Kinga Stefaniuk <kinga.stefaniuk@intel.com> wrote:
> 
> > This series of patches contains adding new timeout
> > which is needed to have mdmon started completely.
> >   
> 
> Thanks Kinga!  What is the end user experience w/o this patch? (ie
> what negative impact does this patch address? mystery hang?  missing
> events?)
> 
> -Paul
> 
> > Kinga Stefaniuk (2):
> >   util.c: change devnm to const in mdmon functions
> >   Wait for mdmon when it is stared via systemd
> > 
> >  Assemble.c |  4 ++--
> >  Grow.c     |  7 ++++---
> >  mdadm.h    |  6 ++++--
> >  util.c     | 33 +++++++++++++++++++++++++++++++--
> >  4 files changed, 41 insertions(+), 9 deletions(-)
> >   
> 
> 

Hi Paul,

we have an issue for R0 - if grow is run for R0 to n-number of drives,
R0 has to move to R4, then mdmon is started for it. After that, mdadm
finishes --grow command, and systemd runs --grow-continue to have this
reshape continued for prepared array. With new kernels, we noticed that
R4 has not enough time to has mdmon started during this process, and for
this reason, the next command, --grow-continue failed.
Another problem is reboot during resync, sometimes mdmon has not
enough time to start too, and resync has been not continued after
reboot.
That's why I've proposed timeout which will address this problem.

Kinga

Re: [PATCH 0/2] New timeout while waiting for mdmon

[Mariusz Tkaczyk]

On Tue,  7 May 2024 05:38:54 +0200
Kinga Stefaniuk <kinga.stefaniuk@intel.com> wrote:

> This series of patches contains adding new timeout
> which is needed to have mdmon started completely.
> 
> Kinga Stefaniuk (2):
>   util.c: change devnm to const in mdmon functions
>   Wait for mdmon when it is stared via systemd
> 
>  Assemble.c |  4 ++--
>  Grow.c     |  7 ++++---
>  mdadm.h    |  6 ++++--
>  util.c     | 33 +++++++++++++++++++++++++++++++--
>  4 files changed, 41 insertions(+), 9 deletions(-)
> 

Applied! 

Thanks,
Mariusz

Re: [PATCH 2/2] Wait for mdmon when it is stared via systemd

[Paul Menzel]

Dear Kinga,


Thank you for the patch. There is a small typo in the summary: star*t*ed.

Am 07.05.24 um 05:38 schrieb Kinga Stefaniuk:
> When mdmon is being started it may need few seconds to start.
> For now, we didn't wait for it. Introduce wait_for_mdmon()
> function, which waits up to 5 seconds for mdmon to start completely.
> 
> Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com>
> ---
>   Assemble.c |  4 ++--
>   Grow.c     |  7 ++++---
>   mdadm.h    |  2 ++
>   util.c     | 29 +++++++++++++++++++++++++++++
>   4 files changed, 37 insertions(+), 5 deletions(-)
> 
> diff --git a/Assemble.c b/Assemble.c
> index f6c5b99e25e2..9cb1747df0a3 100644
> --- a/Assemble.c
> +++ b/Assemble.c
> @@ -2175,8 +2175,8 @@ int assemble_container_content(struct supertype *st, int mdfd,
>   			if (!mdmon_running(st->container_devnm))
>   				start_mdmon(st->container_devnm);
>   			ping_monitor(st->container_devnm);
> -			if (mdmon_running(st->container_devnm) &&
> -			    st->update_tail == NULL)
> +			if (wait_for_mdmon(st->container_devnm) == MDADM_STATUS_SUCCESS &&
> +			    !st->update_tail)
>   				st->update_tail = &st->updates;
>   		}
>   
> diff --git a/Grow.c b/Grow.c
> index 074f19956e17..0e44fae4891e 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -2085,7 +2085,7 @@ int Grow_reshape(char *devname, int fd,
>   			if (!mdmon_running(st->container_devnm))
>   				start_mdmon(st->container_devnm);
>   			ping_monitor(container);
> -			if (mdmon_running(st->container_devnm) == false) {
> +			if (wait_for_mdmon(st->container_devnm) != MDADM_STATUS_SUCCESS) {
>   				pr_err("No mdmon found. Grow cannot continue.\n");
>   				goto release;
>   			}
> @@ -3176,7 +3176,8 @@ static int reshape_array(char *container, int fd, char *devname,
>   			if (!mdmon_running(container))
>   				start_mdmon(container);
>   			ping_monitor(container);
> -			if (mdmon_running(container) && st->update_tail == NULL)
> +			if (wait_for_mdmon(container) == MDADM_STATUS_SUCCESS &&
> +			    !st->update_tail)
>   				st->update_tail = &st->updates;
>   		}
>   	}
> @@ -5140,7 +5141,7 @@ int Grow_continue_command(char *devname, int fd,
>   			start_mdmon(container);
>   		ping_monitor(container);
>   
> -		if (mdmon_running(container) == false) {
> +		if (wait_for_mdmon(container) != MDADM_STATUS_SUCCESS) {
>   			pr_err("No mdmon found. Grow cannot continue.\n");
>   			ret_val = 1;
>   			goto Grow_continue_command_exit;
> diff --git a/mdadm.h b/mdadm.h
> index af4c484afdf7..9b8fb3f6f8d8 100644
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -1769,6 +1769,8 @@ extern struct superswitch *version_to_superswitch(char *vers);
>   
>   extern int mdmon_running(const char *devnm);
>   extern int mdmon_pid(const char *devnm);
> +extern mdadm_status_t wait_for_mdmon(const char *devnm);
> +
>   extern int check_env(char *name);
>   extern __u32 random32(void);
>   extern void random_uuid(__u8 *buf);
> diff --git a/util.c b/util.c
> index 65056a19e2cd..df12cf2bb2b1 100644
> --- a/util.c
> +++ b/util.c
> @@ -1921,6 +1921,35 @@ int mdmon_running(const char *devnm)
>   	return 0;
>   }
>   
> +/*
> + * wait_for_mdmon() - Waits for mdmon within specified time.
> + * @devnm: Device for which mdmon should start.
> + *
> + * Function waits for mdmon to start. It may need few seconds
> + * to start, we set timeout to 5, it should be sufficient.
> + * Do not wait if mdmon has been started.
> + *
> + * Return: MDADM_STATUS_SUCCESS if mdmon is running, error code otherwise.
> + */
> +mdadm_status_t wait_for_mdmon(const char *devnm)
> +{
> +	const time_t mdmon_timeout = 5;
> +	time_t start_time = time(0);
> +
> +	if (mdmon_running(devnm))
> +		return MDADM_STATUS_SUCCESS;
> +
> +	pr_info("Waiting for mdmon to start\n");
> +	while (time(0) - start_time < mdmon_timeout) {
> +		sleep_for(0, MSEC_TO_NSEC(200), true);
> +		if (mdmon_running(devnm))
> +			return MDADM_STATUS_SUCCESS;
> +	};
> +
> +	pr_err("Timeout waiting for mdmon\n");

Please print the timeout limit.

> +	return MDADM_STATUS_ERROR;
> +}
> +
>   int start_mdmon(char *devnm)
>   {
>   	int i;

Doesn’t systemd have some interface sd_ on how to notify about a 
successful start?


Kind nregards,

Paul

Re: [PATCH 2/2] Wait for mdmon when it is stared via systemd

[Kinga Stefaniuk]

On Tue, 14 May 2024 11:17:16 +0200
Paul Menzel <pmenzel@molgen.mpg.de> wrote:

> Dear Kinga,
> 
> 
> Thank you for the patch. There is a small typo in the summary:
> star*t*ed.
> 
> Am 07.05.24 um 05:38 schrieb Kinga Stefaniuk:
> > When mdmon is being started it may need few seconds to start.
> > For now, we didn't wait for it. Introduce wait_for_mdmon()
> > function, which waits up to 5 seconds for mdmon to start completely.
> > 
> > Signed-off-by: Kinga Stefaniuk <kinga.stefaniuk@intel.com>
> > ---
> >   Assemble.c |  4 ++--
> >   Grow.c     |  7 ++++---
> >   mdadm.h    |  2 ++
> >   util.c     | 29 +++++++++++++++++++++++++++++
> >   4 files changed, 37 insertions(+), 5 deletions(-)
> > 
> > diff --git a/Assemble.c b/Assemble.c
> > index f6c5b99e25e2..9cb1747df0a3 100644
> > --- a/Assemble.c
> > +++ b/Assemble.c
> > @@ -2175,8 +2175,8 @@ int assemble_container_content(struct
> > supertype *st, int mdfd, if (!mdmon_running(st->container_devnm))
> >   				start_mdmon(st->container_devnm);
> >   			ping_monitor(st->container_devnm);
> > -			if (mdmon_running(st->container_devnm) &&
> > -			    st->update_tail == NULL)
> > +			if (wait_for_mdmon(st->container_devnm) ==
> > MDADM_STATUS_SUCCESS &&
> > +			    !st->update_tail)
> >   				st->update_tail = &st->updates;
> >   		}
> >   
> > diff --git a/Grow.c b/Grow.c
> > index 074f19956e17..0e44fae4891e 100644
> > --- a/Grow.c
> > +++ b/Grow.c
> > @@ -2085,7 +2085,7 @@ int Grow_reshape(char *devname, int fd,
> >   			if (!mdmon_running(st->container_devnm))
> >   				start_mdmon(st->container_devnm);
> >   			ping_monitor(container);
> > -			if (mdmon_running(st->container_devnm) ==
> > false) {
> > +			if (wait_for_mdmon(st->container_devnm) !=
> > MDADM_STATUS_SUCCESS) { pr_err("No mdmon found. Grow cannot
> > continue.\n"); goto release;
> >   			}
> > @@ -3176,7 +3176,8 @@ static int reshape_array(char *container, int
> > fd, char *devname, if (!mdmon_running(container))
> >   				start_mdmon(container);
> >   			ping_monitor(container);
> > -			if (mdmon_running(container) &&
> > st->update_tail == NULL)
> > +			if (wait_for_mdmon(container) ==
> > MDADM_STATUS_SUCCESS &&
> > +			    !st->update_tail)
> >   				st->update_tail = &st->updates;
> >   		}
> >   	}
> > @@ -5140,7 +5141,7 @@ int Grow_continue_command(char *devname, int
> > fd, start_mdmon(container);
> >   		ping_monitor(container);
> >   
> > -		if (mdmon_running(container) == false) {
> > +		if (wait_for_mdmon(container) !=
> > MDADM_STATUS_SUCCESS) { pr_err("No mdmon found. Grow cannot
> > continue.\n"); ret_val = 1;
> >   			goto Grow_continue_command_exit;
> > diff --git a/mdadm.h b/mdadm.h
> > index af4c484afdf7..9b8fb3f6f8d8 100644
> > --- a/mdadm.h
> > +++ b/mdadm.h
> > @@ -1769,6 +1769,8 @@ extern struct superswitch
> > *version_to_superswitch(char *vers); 
> >   extern int mdmon_running(const char *devnm);
> >   extern int mdmon_pid(const char *devnm);
> > +extern mdadm_status_t wait_for_mdmon(const char *devnm);
> > +
> >   extern int check_env(char *name);
> >   extern __u32 random32(void);
> >   extern void random_uuid(__u8 *buf);
> > diff --git a/util.c b/util.c
> > index 65056a19e2cd..df12cf2bb2b1 100644
> > --- a/util.c
> > +++ b/util.c
> > @@ -1921,6 +1921,35 @@ int mdmon_running(const char *devnm)
> >   	return 0;
> >   }
> >   
> > +/*
> > + * wait_for_mdmon() - Waits for mdmon within specified time.
> > + * @devnm: Device for which mdmon should start.
> > + *
> > + * Function waits for mdmon to start. It may need few seconds
> > + * to start, we set timeout to 5, it should be sufficient.
> > + * Do not wait if mdmon has been started.
> > + *
> > + * Return: MDADM_STATUS_SUCCESS if mdmon is running, error code
> > otherwise.
> > + */
> > +mdadm_status_t wait_for_mdmon(const char *devnm)
> > +{
> > +	const time_t mdmon_timeout = 5;
> > +	time_t start_time = time(0);
> > +
> > +	if (mdmon_running(devnm))
> > +		return MDADM_STATUS_SUCCESS;
> > +
> > +	pr_info("Waiting for mdmon to start\n");
> > +	while (time(0) - start_time < mdmon_timeout) {
> > +		sleep_for(0, MSEC_TO_NSEC(200), true);
> > +		if (mdmon_running(devnm))
> > +			return MDADM_STATUS_SUCCESS;
> > +	};
> > +
> > +	pr_err("Timeout waiting for mdmon\n");  
> 
> Please print the timeout limit.
> 
> > +	return MDADM_STATUS_ERROR;
> > +}
> > +
> >   int start_mdmon(char *devnm)
> >   {
> >   	int i;  
> 
> Doesn’t systemd have some interface sd_ on how to notify about a 
> successful start?
> 
> 
> Kind nregards,
> 
> Paul
> 

Hi Paul,

mdadm has its own mechanism to verify if mdmon is running and using it
we are not limited only to systemd, so it's better to use this way.

Thanks,
Kinga