[dm-devel] [PATCH v2 00/11] multipathd: fix __delayed_reconfig logic

[dm-devel] [PATCH v2 00/11] multipathd: fix __delayed_reconfig logic

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Hi Ben, hi Christophe, hi Guozhonghua,

here's my new take at the race condition issue reported by Guozhonghua.
This patch set is meant as an alternative to Ben's recent series
"fix looping when reconfigure is delayed". I believe that by removing
the special case from __post_config_state(), the state change logic
becomes somewhat easier to understand again.

I hope I got it right this time.

Changes v1->v2:

 04: reset __delayed_reconfig if no delay needed (Ben)
     only call __post_config_state in IDLE state (Ben)
 06/07: unblock reconfigure if maps are removed, as suggested by Ben
 08-11: minor logging fixes I found useful

Regards
Martin

Martin Wilck (11):
  multipathd: child(): remove superfluous if condition
  multipathd: set reload_type in when calling reconfigure()
  multipathd: avoid busy loop in child() for delayed reconfigure
  multipathd: reset __delayed_reconfig from ev_add_map()
  multipathd: remove volatile qualifier from running_state
  libmultipath: add callback for remove_map()
  multipathd: use remove_map_callback for delayed reconfigure
  libmultipath: warn only once about deprecated options
  multipathd: improve logging of reconfigure()
  multipathd: log state changes
  multipathd: remove unhelpful startup / shutdown messages

 libmultipath/dict.c               |  15 ++-
 libmultipath/libmultipath.version |   3 +-
 libmultipath/structs_vec.c        |   6 ++
 multipathd/main.c                 | 172 ++++++++++++++++--------------
 4 files changed, 109 insertions(+), 87 deletions(-)

-- 
2.35.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 01/11] multipathd: child(): remove superfluous if condition

[mwilck]

From: Martin Wilck <mwilck@suse.com>

No need to test for state == DAEMON_CONFIGURE at this point, we
know that this is the case.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index f2c0b28..1c8839d 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -3395,6 +3395,8 @@ child (__attribute__((unused)) void *param)
 	pthread_attr_destroy(&misc_attr);
 
 	while (1) {
+		int rc = 0;
+
 		pthread_cleanup_push(config_cleanup, NULL);
 		pthread_mutex_lock(&config_lock);
 		while (running_state != DAEMON_CONFIGURE &&
@@ -3404,23 +3406,21 @@ child (__attribute__((unused)) void *param)
 		pthread_cleanup_pop(1);
 		if (state == DAEMON_SHUTDOWN)
 			break;
-		if (state == DAEMON_CONFIGURE) {
-			int rc = 0;
 
-			pthread_cleanup_push(cleanup_lock, &vecs->lock);
-			lock(&vecs->lock);
-			pthread_testcancel();
-			if (!need_to_delay_reconfig(vecs))
-				rc = reconfigure(vecs);
-			else
-				enable_delayed_reconfig();
-			lock_cleanup_pop(vecs->lock);
-			if (!rc)
-				post_config_state(DAEMON_IDLE);
-			else {
-				condlog(0, "fatal error applying configuration - aborting");
-				exit_daemon();
-			}
+		/* handle DAEMON_CONFIGURE */
+		pthread_cleanup_push(cleanup_lock, &vecs->lock);
+		lock(&vecs->lock);
+		pthread_testcancel();
+		if (!need_to_delay_reconfig(vecs))
+			rc = reconfigure(vecs);
+		else
+			enable_delayed_reconfig();
+		lock_cleanup_pop(vecs->lock);
+		if (!rc)
+			post_config_state(DAEMON_IDLE);
+		else {
+			condlog(0, "fatal error applying configuration - aborting");
+			exit_daemon();
 		}
 	}
 
-- 
2.35.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 01/11] multipathd: child(): remove superfluous if condition

[Benjamin Marzinski]

On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
>
> From: Martin Wilck <mwilck@suse.com>
>
> No need to test for state == DAEMON_CONFIGURE at this point, we
> know that this is the case.
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/main.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index f2c0b28..1c8839d 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -3395,6 +3395,8 @@ child (__attribute__((unused)) void *param)
>         pthread_attr_destroy(&misc_attr);
>
>         while (1) {
> +               int rc = 0;
> +
>                 pthread_cleanup_push(config_cleanup, NULL);
>                 pthread_mutex_lock(&config_lock);
>                 while (running_state != DAEMON_CONFIGURE &&
> @@ -3404,23 +3406,21 @@ child (__attribute__((unused)) void *param)
>                 pthread_cleanup_pop(1);
>                 if (state == DAEMON_SHUTDOWN)
>                         break;
> -               if (state == DAEMON_CONFIGURE) {
> -                       int rc = 0;
>
> -                       pthread_cleanup_push(cleanup_lock, &vecs->lock);
> -                       lock(&vecs->lock);
> -                       pthread_testcancel();
> -                       if (!need_to_delay_reconfig(vecs))
> -                               rc = reconfigure(vecs);
> -                       else
> -                               enable_delayed_reconfig();
> -                       lock_cleanup_pop(vecs->lock);
> -                       if (!rc)
> -                               post_config_state(DAEMON_IDLE);
> -                       else {
> -                               condlog(0, "fatal error applying configuration - aborting");
> -                               exit_daemon();
> -                       }
> +               /* handle DAEMON_CONFIGURE */
> +               pthread_cleanup_push(cleanup_lock, &vecs->lock);
> +               lock(&vecs->lock);
> +               pthread_testcancel();
> +               if (!need_to_delay_reconfig(vecs))
> +                       rc = reconfigure(vecs);
> +               else
> +                       enable_delayed_reconfig();
> +               lock_cleanup_pop(vecs->lock);
> +               if (!rc)
> +                       post_config_state(DAEMON_IDLE);
> +               else {
> +                       condlog(0, "fatal error applying configuration - aborting");
> +                       exit_daemon();
>                 }
>         }
>
> --
> 2.35.1
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 02/11] multipathd: set reload_type in when calling reconfigure()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Only set reload_type (and reset reconfigure_pending) immediately
before we actually call reconfigure(). This allows us to get rid of
the reload_type global variable, and makes sure that reconfigure()
is called with the reload type that was last requested.

While at it, convert configure() and reconfigure() to static functions.

Signed-off-by: Martin Wilck <mwilck@suse.com>
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 35 +++++++++++++++++------------------
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 1c8839d..7ecf3bd 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -287,14 +287,10 @@ enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
 
 /* Don't access this variable without holding config_lock */
 static enum force_reload_types reconfigure_pending = FORCE_RELOAD_NONE;
-/* Only set while changing to DAEMON_CONFIGURE, and only access while
- * reconfiguring or scheduling a delayed reconfig in DAEMON_CONFIGURE */
-static volatile enum force_reload_types reload_type = FORCE_RELOAD_NONE;
 
 static void enable_delayed_reconfig(void)
 {
 	pthread_mutex_lock(&config_lock);
-	reconfigure_pending = reload_type;
 	__delayed_reconfig = true;
 	pthread_mutex_unlock(&config_lock);
 }
@@ -324,11 +320,6 @@ static void __post_config_state(enum daemon_status state)
 			old_state = DAEMON_IDLE;
 			state = DAEMON_CONFIGURE;
 		}
-		if (state == DAEMON_CONFIGURE) {
-			reload_type = (reconfigure_pending == FORCE_RELOAD_YES) ? FORCE_RELOAD_YES : FORCE_RELOAD_WEAK;
-			reconfigure_pending = FORCE_RELOAD_NONE;
-			__delayed_reconfig = false;
-		}
 		running_state = state;
 		pthread_cond_broadcast(&config_cond);
 		do_sd_notify(old_state, state);
@@ -2714,8 +2705,8 @@ checkerloop (void *ap)
 	return NULL;
 }
 
-int
-configure (struct vectors * vecs)
+static int
+configure (struct vectors * vecs, enum force_reload_types reload_type)
 {
 	struct multipath * mpp;
 	struct path * pp;
@@ -2846,8 +2837,8 @@ void rcu_free_config(struct rcu_head *head)
 	free_config(conf);
 }
 
-int
-reconfigure (struct vectors * vecs)
+static int
+reconfigure (struct vectors *vecs, enum force_reload_types reload_type)
 {
 	struct config * old, *conf;
 	int old_marginal_pathgroups;
@@ -2894,8 +2885,7 @@ reconfigure (struct vectors * vecs)
 #ifdef FPIN_EVENT_HANDLER
 	fpin_clean_marginal_dev_list(NULL);
 #endif
-	configure(vecs);
-
+	configure(vecs, reload_type);
 
 	return 0;
 }
@@ -3411,9 +3401,18 @@ child (__attribute__((unused)) void *param)
 		pthread_cleanup_push(cleanup_lock, &vecs->lock);
 		lock(&vecs->lock);
 		pthread_testcancel();
-		if (!need_to_delay_reconfig(vecs))
-			rc = reconfigure(vecs);
-		else
+		if (!need_to_delay_reconfig(vecs)) {
+			enum force_reload_types reload_type;
+
+			pthread_mutex_lock(&config_lock);
+			reload_type = reconfigure_pending == FORCE_RELOAD_YES ?
+				FORCE_RELOAD_YES : FORCE_RELOAD_WEAK;
+			reconfigure_pending = FORCE_RELOAD_NONE;
+			__delayed_reconfig = false;
+			pthread_mutex_unlock(&config_lock);
+
+			rc = reconfigure(vecs, reload_type);
+		} else
 			enable_delayed_reconfig();
 		lock_cleanup_pop(vecs->lock);
 		if (!rc)
-- 
2.35.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 03/11] multipathd: avoid busy loop in child() for delayed reconfigure

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If need_to_delay_reconfig() returned true, the logic introduced
by 250708c ("multipathd: improve delayed reconfigure") and
4b104e6 ("multipathd: pass in the type of reconfigure") could cause
child() to run in a tight loop, switching back and forth between
DAEMON_CONFIGURE and DAEMON_IDLE states without actually calling
reconfigure().

Move the logic to postpone reconfigure() calls from __post_config_state()
into child(), entirely, to avoid this situation. This means that child()
needs to poll for reconfigure_pending besides running_state changes.

Fixes: 250708c ("multipathd: improve delayed reconfigure")
Fixes: 4b104e6 ("multipathd: pass in the type of reconfigure")
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 48 +++++++++++++++++++----------------------------
 1 file changed, 19 insertions(+), 29 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 7ecf3bd..d033a9a 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -288,38 +288,12 @@ enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
 /* Don't access this variable without holding config_lock */
 static enum force_reload_types reconfigure_pending = FORCE_RELOAD_NONE;
 
-static void enable_delayed_reconfig(void)
-{
-	pthread_mutex_lock(&config_lock);
-	__delayed_reconfig = true;
-	pthread_mutex_unlock(&config_lock);
-}
-
 /* must be called with config_lock held */
 static void __post_config_state(enum daemon_status state)
 {
 	if (state != running_state && running_state != DAEMON_SHUTDOWN) {
 		enum daemon_status old_state = running_state;
 
-		/*
-		 * Handle a pending reconfigure request.
-		 * DAEMON_IDLE is set from child() after reconfigure(),
-		 * or from checkerloop() after completing checkers.
-		 * In either case, child() will see DAEMON_CONFIGURE
-		 * again and start another reconfigure cycle.
-		 */
-		if (reconfigure_pending != FORCE_RELOAD_NONE &&
-		    state == DAEMON_IDLE &&
-		    (old_state == DAEMON_CONFIGURE ||
-		     old_state == DAEMON_RUNNING)) {
-			/*
-			 * notify systemd of transient idle state, lest systemd
-			 * thinks the reload lasts forever.
-			 */
-			do_sd_notify(old_state, DAEMON_IDLE);
-			old_state = DAEMON_IDLE;
-			state = DAEMON_CONFIGURE;
-		}
 		running_state = state;
 		pthread_cond_broadcast(&config_cond);
 		do_sd_notify(old_state, state);
@@ -3390,8 +3364,21 @@ child (__attribute__((unused)) void *param)
 		pthread_cleanup_push(config_cleanup, NULL);
 		pthread_mutex_lock(&config_lock);
 		while (running_state != DAEMON_CONFIGURE &&
-		       running_state != DAEMON_SHUTDOWN)
+		       running_state != DAEMON_SHUTDOWN &&
+		       /*
+			* Check if another reconfigure request was scheduled
+			* while we last ran reconfigure().
+			* We have to test __delayed_reconfig here
+			* to avoid a busy loop
+			*/
+		       (reconfigure_pending == FORCE_RELOAD_NONE
+			 || __delayed_reconfig))
 			pthread_cond_wait(&config_cond, &config_lock);
+
+		if (running_state != DAEMON_CONFIGURE &&
+		    running_state != DAEMON_SHUTDOWN)
+			/* This sets running_state to DAEMON_CONFIGURE */
+			__post_config_state(DAEMON_CONFIGURE);
 		state = running_state;
 		pthread_cleanup_pop(1);
 		if (state == DAEMON_SHUTDOWN)
@@ -3412,8 +3399,11 @@ child (__attribute__((unused)) void *param)
 			pthread_mutex_unlock(&config_lock);
 
 			rc = reconfigure(vecs, reload_type);
-		} else
-			enable_delayed_reconfig();
+		} else {
+			pthread_mutex_lock(&config_lock);
+			__delayed_reconfig = true;
+			pthread_mutex_unlock(&config_lock);
+		}
 		lock_cleanup_pop(vecs->lock);
 		if (!rc)
 			post_config_state(DAEMON_IDLE);
-- 
2.35.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 03/11] multipathd: avoid busy loop in child() for delayed reconfigure

[Benjamin Marzinski]

On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
>
> From: Martin Wilck <mwilck@suse.com>
>
> If need_to_delay_reconfig() returned true, the logic introduced
> by 250708c ("multipathd: improve delayed reconfigure") and
> 4b104e6 ("multipathd: pass in the type of reconfigure") could cause
> child() to run in a tight loop, switching back and forth between
> DAEMON_CONFIGURE and DAEMON_IDLE states without actually calling
> reconfigure().
>
> Move the logic to postpone reconfigure() calls from __post_config_state()
> into child(), entirely, to avoid this situation. This means that child()
> needs to poll for reconfigure_pending besides running_state changes.
>
> Fixes: 250708c ("multipathd: improve delayed reconfigure")
> Fixes: 4b104e6 ("multipathd: pass in the type of reconfigure")
> Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/main.c | 48 +++++++++++++++++++----------------------------
>  1 file changed, 19 insertions(+), 29 deletions(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 7ecf3bd..d033a9a 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -288,38 +288,12 @@ enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
>  /* Don't access this variable without holding config_lock */
>  static enum force_reload_types reconfigure_pending = FORCE_RELOAD_NONE;
>
> -static void enable_delayed_reconfig(void)
> -{
> -       pthread_mutex_lock(&config_lock);
> -       __delayed_reconfig = true;
> -       pthread_mutex_unlock(&config_lock);
> -}
> -
>  /* must be called with config_lock held */
>  static void __post_config_state(enum daemon_status state)
>  {
>         if (state != running_state && running_state != DAEMON_SHUTDOWN) {
>                 enum daemon_status old_state = running_state;
>
> -               /*
> -                * Handle a pending reconfigure request.
> -                * DAEMON_IDLE is set from child() after reconfigure(),
> -                * or from checkerloop() after completing checkers.
> -                * In either case, child() will see DAEMON_CONFIGURE
> -                * again and start another reconfigure cycle.
> -                */
> -               if (reconfigure_pending != FORCE_RELOAD_NONE &&
> -                   state == DAEMON_IDLE &&
> -                   (old_state == DAEMON_CONFIGURE ||
> -                    old_state == DAEMON_RUNNING)) {
> -                       /*
> -                        * notify systemd of transient idle state, lest systemd
> -                        * thinks the reload lasts forever.
> -                        */
> -                       do_sd_notify(old_state, DAEMON_IDLE);
> -                       old_state = DAEMON_IDLE;
> -                       state = DAEMON_CONFIGURE;
> -               }
>                 running_state = state;
>                 pthread_cond_broadcast(&config_cond);
>                 do_sd_notify(old_state, state);
> @@ -3390,8 +3364,21 @@ child (__attribute__((unused)) void *param)
>                 pthread_cleanup_push(config_cleanup, NULL);
>                 pthread_mutex_lock(&config_lock);
>                 while (running_state != DAEMON_CONFIGURE &&
> -                      running_state != DAEMON_SHUTDOWN)
> +                      running_state != DAEMON_SHUTDOWN &&
> +                      /*
> +                       * Check if another reconfigure request was scheduled
> +                       * while we last ran reconfigure().
> +                       * We have to test __delayed_reconfig here
> +                       * to avoid a busy loop
> +                       */
> +                      (reconfigure_pending == FORCE_RELOAD_NONE
> +                        || __delayed_reconfig))
>                         pthread_cond_wait(&config_cond, &config_lock);
> +
> +               if (running_state != DAEMON_CONFIGURE &&
> +                   running_state != DAEMON_SHUTDOWN)
> +                       /* This sets running_state to DAEMON_CONFIGURE */
> +                       __post_config_state(DAEMON_CONFIGURE);
>                 state = running_state;
>                 pthread_cleanup_pop(1);
>                 if (state == DAEMON_SHUTDOWN)
> @@ -3412,8 +3399,11 @@ child (__attribute__((unused)) void *param)
>                         pthread_mutex_unlock(&config_lock);
>
>                         rc = reconfigure(vecs, reload_type);
> -               } else
> -                       enable_delayed_reconfig();
> +               } else {
> +                       pthread_mutex_lock(&config_lock);
> +                       __delayed_reconfig = true;
> +                       pthread_mutex_unlock(&config_lock);
> +               }
>                 lock_cleanup_pop(vecs->lock);
>                 if (!rc)
>                         post_config_state(DAEMON_IDLE);
> --
> 2.35.1
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 04/11] multipathd: reset __delayed_reconfig from ev_add_map()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

With the previous patch, one race condition between child() and
the uevent handler (ev_add_map()) remains: 1. child() sets
__delayed_reconfig, 2. ev_add_map() calls schedule_reconfigure() and
thus DAEMON_CONFIGURE, 3. child() sets DAEMON_IDLE. This would cause
the pending reconfigure request to be missed.

To fix this, reset __delayed_reconfig immediately from ev_add_map()
(and likewise, missing_uev_wait_tick()). This way the wait loop in
child() terminates on the reconfigure_pending condition.

Also, these schedule_reconfigure() callers don't really request a
reconfigure() call; they just unblock processing previously requested
reconfigure() calls. Add a new helper, unblock_reconfigure(), that
does just that.

Suggested-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 45 +++++++++++++++++++++++++--------------------
 1 file changed, 25 insertions(+), 20 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index d033a9a..1454a18 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -155,16 +155,6 @@ int should_exit(void)
 	return get_running_state() == DAEMON_SHUTDOWN;
 }
 
-static bool get_delayed_reconfig(void)
-{
-	bool val;
-
-	pthread_mutex_lock(&config_lock);
-	val = __delayed_reconfig;
-	pthread_mutex_unlock(&config_lock);
-	return val;
-}
-
 /*
  * global copy of vecs for use in sig handlers
  */
@@ -308,6 +298,27 @@ void post_config_state(enum daemon_status state)
 	pthread_cleanup_pop(1);
 }
 
+static bool unblock_reconfigure(void)
+{
+	bool was_delayed;
+
+	pthread_mutex_lock(&config_lock);
+	was_delayed = __delayed_reconfig;
+	if (was_delayed) {
+		__delayed_reconfig = false;
+		/*
+		 * In IDLE state, make sure child() is woken up
+		 * Otherwise it will wake up when state switches to IDLE
+		 */
+		if (running_state == DAEMON_IDLE)
+			__post_config_state(DAEMON_CONFIGURE);
+	}
+	pthread_mutex_unlock(&config_lock);
+	if (was_delayed)
+		condlog(3, "unblocked delayed reconfigure");
+	return was_delayed;
+}
+
 void schedule_reconfigure(enum force_reload_types requested_type)
 {
 	pthread_mutex_lock(&config_lock);
@@ -790,12 +801,9 @@ ev_add_map (char * dev, const char * alias, struct vectors * vecs)
 		dm_get_info(mpp->alias, &mpp->dmi);
 		if (mpp->wait_for_udev) {
 			mpp->wait_for_udev = 0;
-			if (get_delayed_reconfig() &&
-			    !need_to_delay_reconfig(vecs)) {
-				condlog(2, "reconfigure (delayed)");
-				schedule_reconfigure(FORCE_RELOAD_WEAK);
+			if (!need_to_delay_reconfig(vecs) &&
+			    unblock_reconfigure())
 				return 0;
-			}
 		}
 		/*
 		 * Not really an error -- we generate our own uevent
@@ -1899,11 +1907,8 @@ missing_uev_wait_tick(struct vectors *vecs)
 		}
 	}
 
-	if (timed_out && get_delayed_reconfig() &&
-	    !need_to_delay_reconfig(vecs)) {
-		condlog(2, "reconfigure (delayed)");
-		schedule_reconfigure(FORCE_RELOAD_WEAK);
-	}
+	if (timed_out && !need_to_delay_reconfig(vecs))
+		unblock_reconfigure();
 }
 
 static void
-- 
2.35.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 04/11] multipathd: reset __delayed_reconfig from ev_add_map()

[Benjamin Marzinski]

On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
>
> From: Martin Wilck <mwilck@suse.com>
>
> With the previous patch, one race condition between child() and
> the uevent handler (ev_add_map()) remains: 1. child() sets
> __delayed_reconfig, 2. ev_add_map() calls schedule_reconfigure() and
> thus DAEMON_CONFIGURE, 3. child() sets DAEMON_IDLE. This would cause
> the pending reconfigure request to be missed.
>
> To fix this, reset __delayed_reconfig immediately from ev_add_map()
> (and likewise, missing_uev_wait_tick()). This way the wait loop in
> child() terminates on the reconfigure_pending condition.
>
> Also, these schedule_reconfigure() callers don't really request a
> reconfigure() call; they just unblock processing previously requested
> reconfigure() calls. Add a new helper, unblock_reconfigure(), that
> does just that.
>
> Suggested-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  multipathd/main.c | 45 +++++++++++++++++++++++++--------------------
>  1 file changed, 25 insertions(+), 20 deletions(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index d033a9a..1454a18 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -155,16 +155,6 @@ int should_exit(void)
>         return get_running_state() == DAEMON_SHUTDOWN;
>  }
>
> -static bool get_delayed_reconfig(void)
> -{
> -       bool val;
> -
> -       pthread_mutex_lock(&config_lock);
> -       val = __delayed_reconfig;
> -       pthread_mutex_unlock(&config_lock);
> -       return val;
> -}
> -
>  /*
>   * global copy of vecs for use in sig handlers
>   */
> @@ -308,6 +298,27 @@ void post_config_state(enum daemon_status state)
>         pthread_cleanup_pop(1);
>  }
>
> +static bool unblock_reconfigure(void)
> +{
> +       bool was_delayed;
> +
> +       pthread_mutex_lock(&config_lock);
> +       was_delayed = __delayed_reconfig;
> +       if (was_delayed) {
> +               __delayed_reconfig = false;
> +               /*
> +                * In IDLE state, make sure child() is woken up
> +                * Otherwise it will wake up when state switches to IDLE
> +                */
> +               if (running_state == DAEMON_IDLE)
> +                       __post_config_state(DAEMON_CONFIGURE);
> +       }
> +       pthread_mutex_unlock(&config_lock);
> +       if (was_delayed)
> +               condlog(3, "unblocked delayed reconfigure");
> +       return was_delayed;
> +}
> +
>  void schedule_reconfigure(enum force_reload_types requested_type)
>  {
>         pthread_mutex_lock(&config_lock);
> @@ -790,12 +801,9 @@ ev_add_map (char * dev, const char * alias, struct vectors * vecs)
>                 dm_get_info(mpp->alias, &mpp->dmi);
>                 if (mpp->wait_for_udev) {
>                         mpp->wait_for_udev = 0;
> -                       if (get_delayed_reconfig() &&
> -                           !need_to_delay_reconfig(vecs)) {
> -                               condlog(2, "reconfigure (delayed)");
> -                               schedule_reconfigure(FORCE_RELOAD_WEAK);
> +                       if (!need_to_delay_reconfig(vecs) &&
> +                           unblock_reconfigure())
>                                 return 0;
> -                       }
>                 }
>                 /*
>                  * Not really an error -- we generate our own uevent
> @@ -1899,11 +1907,8 @@ missing_uev_wait_tick(struct vectors *vecs)
>                 }
>         }
>
> -       if (timed_out && get_delayed_reconfig() &&
> -           !need_to_delay_reconfig(vecs)) {
> -               condlog(2, "reconfigure (delayed)");
> -               schedule_reconfigure(FORCE_RELOAD_WEAK);
> -       }
> +       if (timed_out && !need_to_delay_reconfig(vecs))
> +               unblock_reconfigure();
>  }
>
>  static void
> --
> 2.35.1
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 05/11] multipathd: remove volatile qualifier from running_state

[mwilck]

From: Martin Wilck <mwilck@suse.com>

running_state is only accessed while holding the config_lock, the
volatile qualifier is superfluous.

Suggested-by: Benjamin Marzinski <bmarzins@redhat.com>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 1454a18..f3b8eb4 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -127,7 +127,7 @@ static int poll_dmevents = 0;
 static int poll_dmevents = 1;
 #endif
 /* Don't access this variable without holding config_lock */
-static volatile enum daemon_status running_state = DAEMON_INIT;
+static enum daemon_status running_state = DAEMON_INIT;
 /* Don't access this variable without holding config_lock */
 static bool __delayed_reconfig;
 pid_t daemon_pid;
-- 
2.35.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 05/11] multipathd: remove volatile qualifier from running_state

[Benjamin Marzinski]

On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
>
> From: Martin Wilck <mwilck@suse.com>
>
> running_state is only accessed while holding the config_lock, the
> volatile qualifier is superfluous.
>
> Suggested-by: Benjamin Marzinski <bmarzins@redhat.com>
> Signed-off-by: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> ---
>  multipathd/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 1454a18..f3b8eb4 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -127,7 +127,7 @@ static int poll_dmevents = 0;
>  static int poll_dmevents = 1;
>  #endif
>  /* Don't access this variable without holding config_lock */
> -static volatile enum daemon_status running_state = DAEMON_INIT;
> +static enum daemon_status running_state = DAEMON_INIT;
>  /* Don't access this variable without holding config_lock */
>  static bool __delayed_reconfig;
>  pid_t daemon_pid;
> --
> 2.35.1
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 06/11] libmultipath: add callback for remove_map()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

This callback is be used by multipathd to unblock pending
reconfigure requests if a map is removed that multipathd is currently
waiting for.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/libmultipath.version | 3 ++-
 libmultipath/structs_vec.c        | 6 ++++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index 216f0ee..8132df7 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -31,7 +31,7 @@
  *   The new version inherits the previous ones.
  */
 
-LIBMULTIPATH_14.0.0 {
+LIBMULTIPATH_14.1.0 {
 global:
 	/* symbols referenced by multipath and multipathd */
 	add_foreign;
@@ -164,6 +164,7 @@ global:
 	remember_wwid;
 	remove_map;
 	remove_map_by_alias;
+	remove_map_callback;
 	remove_maps;
 	remove_wwid;
 	replace_wwids;
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 6c23df8..a69f064 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -336,11 +336,17 @@ void set_path_removed(struct path *pp)
 	pp->initialized = INIT_REMOVED;
 }
 
+void remove_map_callback(struct multipath *mpp __attribute__((unused)))
+{
+}
+
 void
 remove_map(struct multipath *mpp, vector pathvec, vector mpvec)
 {
 	int i;
 
+	remove_map_callback(mpp);
+
 	free_pathvec(mpp->paths, KEEP_PATHS);
 	free_pgvec(mpp->pg, KEEP_PATHS);
 	mpp->paths = mpp->pg = NULL;
-- 
2.35.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 06/11] libmultipath: add callback for remove_map()

[Benjamin Marzinski]

On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
>
> From: Martin Wilck <mwilck@suse.com>
>
> This callback is be used by multipathd to unblock pending
> reconfigure requests if a map is removed that multipathd is currently
> waiting for.
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  libmultipath/libmultipath.version | 3 ++-
>  libmultipath/structs_vec.c        | 6 ++++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
> index 216f0ee..8132df7 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -31,7 +31,7 @@
>   *   The new version inherits the previous ones.
>   */
>
> -LIBMULTIPATH_14.0.0 {
> +LIBMULTIPATH_14.1.0 {
>  global:
>         /* symbols referenced by multipath and multipathd */
>         add_foreign;
> @@ -164,6 +164,7 @@ global:
>         remember_wwid;
>         remove_map;
>         remove_map_by_alias;
> +       remove_map_callback;
>         remove_maps;
>         remove_wwid;
>         replace_wwids;
> diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
> index 6c23df8..a69f064 100644
> --- a/libmultipath/structs_vec.c
> +++ b/libmultipath/structs_vec.c
> @@ -336,11 +336,17 @@ void set_path_removed(struct path *pp)
>         pp->initialized = INIT_REMOVED;
>  }
>
> +void remove_map_callback(struct multipath *mpp __attribute__((unused)))
> +{
> +}
> +

Does this work? I thought that unless you specifically declared the
symbol weak, the call in remove_map() would have already gotten
resolved to point to the existing remove_map_callback() when the
shared library was getting created.  Is it because the function is
empty? Am I just misunderstanding something?

-Ben

>  void
>  remove_map(struct multipath *mpp, vector pathvec, vector mpvec)
>  {
>         int i;
>
> +       remove_map_callback(mpp);
> +
>         free_pathvec(mpp->paths, KEEP_PATHS);
>         free_pgvec(mpp->pg, KEEP_PATHS);
>         mpp->paths = mpp->pg = NULL;
> --
> 2.35.1
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 06/11] libmultipath: add callback for remove_map()

[Martin Wilck]

On Mon, 2022-03-21 at 19:28 -0500, Benjamin Marzinski wrote:
> On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
> > 
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > This callback is be used by multipathd to unblock pending
> > reconfigure requests if a map is removed that multipathd is
> > currently
> > waiting for.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  libmultipath/libmultipath.version | 3 ++-
> >  libmultipath/structs_vec.c        | 6 ++++++
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libmultipath/libmultipath.version
> > b/libmultipath/libmultipath.version
> > index 216f0ee..8132df7 100644
> > --- a/libmultipath/libmultipath.version
> > +++ b/libmultipath/libmultipath.version
> > @@ -31,7 +31,7 @@
> >   *   The new version inherits the previous ones.
> >   */
> > 
> > -LIBMULTIPATH_14.0.0 {
> > +LIBMULTIPATH_14.1.0 {
> >  global:
> >         /* symbols referenced by multipath and multipathd */
> >         add_foreign;
> > @@ -164,6 +164,7 @@ global:
> >         remember_wwid;
> >         remove_map;
> >         remove_map_by_alias;
> > +       remove_map_callback;
> >         remove_maps;
> >         remove_wwid;
> >         replace_wwids;
> > diff --git a/libmultipath/structs_vec.c
> > b/libmultipath/structs_vec.c
> > index 6c23df8..a69f064 100644
> > --- a/libmultipath/structs_vec.c
> > +++ b/libmultipath/structs_vec.c
> > @@ -336,11 +336,17 @@ void set_path_removed(struct path *pp)
> >         pp->initialized = INIT_REMOVED;
> >  }
> > 
> > +void remove_map_callback(struct multipath *mpp
> > __attribute__((unused)))
> > +{
> > +}
> > +
> 
> Does this work? I thought that unless you specifically declared the
> symbol weak, the call in remove_map() would have already gotten
> resolved to point to the existing remove_map_callback() when the
> shared library was getting created.  Is it because the function is
> empty? Am I just misunderstanding something?

This works because I added the symbol to libmultipath.version,
assigning it "global" visibility. To be consistent, we could do the
same thing with get_multipath_config() et al., but I didn't want to
change that just now.

We (or actually, users and distro integrators) have to be somewhat
careful with adding linker flags. As discussed e.g. in
https://github.com/opensvc/multipath-tools/issues/26
flags like "-Bsymbolic-functions" would mess this up, because this flag
overrides the settings from our linker script. But declaring the symbol
"weak" wouldn't protect against -Bsymbolic mess-up, either.

I had a long discussion with our toolchain experts about this, which
lead to the conclusion above. I am pretty positive about it. 
Feel free to ask the RH experts, too ;-)

Regards
Martin


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 06/11] libmultipath: add callback for remove_map()

[Benjamin Marzinski]

On Tue, Mar 22, 2022 at 3:35 AM Martin Wilck <mwilck@suse.com> wrote:
>
> On Mon, 2022-03-21 at 19:28 -0500, Benjamin Marzinski wrote:
> > On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
> > >
> > > From: Martin Wilck <mwilck@suse.com>
> > >
> > > This callback is be used by multipathd to unblock pending
> > > reconfigure requests if a map is removed that multipathd is
> > > currently
> > > waiting for.
> > >
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  libmultipath/libmultipath.version | 3 ++-
> > >  libmultipath/structs_vec.c        | 6 ++++++
> > >  2 files changed, 8 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/libmultipath/libmultipath.version
> > > b/libmultipath/libmultipath.version
> > > index 216f0ee..8132df7 100644
> > > --- a/libmultipath/libmultipath.version
> > > +++ b/libmultipath/libmultipath.version
> > > @@ -31,7 +31,7 @@
> > >   *   The new version inherits the previous ones.
> > >   */
> > >
> > > -LIBMULTIPATH_14.0.0 {
> > > +LIBMULTIPATH_14.1.0 {
> > >  global:
> > >         /* symbols referenced by multipath and multipathd */
> > >         add_foreign;
> > > @@ -164,6 +164,7 @@ global:
> > >         remember_wwid;
> > >         remove_map;
> > >         remove_map_by_alias;
> > > +       remove_map_callback;
> > >         remove_maps;
> > >         remove_wwid;
> > >         replace_wwids;
> > > diff --git a/libmultipath/structs_vec.c
> > > b/libmultipath/structs_vec.c
> > > index 6c23df8..a69f064 100644
> > > --- a/libmultipath/structs_vec.c
> > > +++ b/libmultipath/structs_vec.c
> > > @@ -336,11 +336,17 @@ void set_path_removed(struct path *pp)
> > >         pp->initialized = INIT_REMOVED;
> > >  }
> > >
> > > +void remove_map_callback(struct multipath *mpp
> > > __attribute__((unused)))
> > > +{
> > > +}
> > > +
> >
> > Does this work? I thought that unless you specifically declared the
> > symbol weak, the call in remove_map() would have already gotten
> > resolved to point to the existing remove_map_callback() when the
> > shared library was getting created.  Is it because the function is
> > empty? Am I just misunderstanding something?
>
> This works because I added the symbol to libmultipath.version,
> assigning it "global" visibility. To be consistent, we could do the
> same thing with get_multipath_config() et al., but I didn't want to
> change that just now.

So all of the exported symbols from libmultipath are weak? Good to know.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>


> We (or actually, users and distro integrators) have to be somewhat
> careful with adding linker flags. As discussed e.g. in
> https://github.com/opensvc/multipath-tools/issues/26
> flags like "-Bsymbolic-functions" would mess this up, because this flag
> overrides the settings from our linker script. But declaring the symbol
> "weak" wouldn't protect against -Bsymbolic mess-up, either.
>
> I had a long discussion with our toolchain experts about this, which
> lead to the conclusion above. I am pretty positive about it.
> Feel free to ask the RH experts, too ;-)
>
> Regards
> Martin
>
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 06/11] libmultipath: add callback for remove_map()

[Martin Wilck]

On Tue, 2022-03-22 at 09:59 -0500, Benjamin Marzinski wrote:
> 
> So all of the exported symbols from libmultipath are weak? Good to
> know.

No, "weak" has a special meaning. For dynamic linking, it's default
behavior to resolve symbols by name and use the first definition
encountered. This means that a symbol in a shared library will always
be overridden by a symbol of the same name in the main executable.

"When resolving symbolic references, the dynamic linker examines the
symbol tables with a breadth-first search. That is, it first looks at
the symbol table of the executable program itself, then at the symbol
tables of the DT_NEEDED entries (in order), then at the second level
DT_NEEDED entries, and so on." ([1], III, p. 2-12). The "weak"
attribute only matters during static linking ([1], III, p. 1-5). See
also the description of LD_DYNAMIC_WEAK in ld.so(8) [2].

I didn't understand this clearly before. And the documentation, in
particular the explanation of the "weak" attribute in the gcc docs, is
misleading, because it doesn't explain the difference between static
and dynamic linking [3].

Regards
Martin

[1] https://refspecs.linuxfoundation.org/elf/elf.pdf
[2] https://man7.org/linux/man-pages/man8/ld.so.8.html

[3] https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html#Common-Function-Attributes


--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 07/11] multipathd: use remove_map_callback for delayed reconfigure

[mwilck]

From: Martin Wilck <mwilck@suse.com>

If multipathd needs to delay reconfigure() because it's waiting for a map
creation uevent, it can happen that child() isn't woken up if the map
being waited for is removed before the uevent arrives. Fix this by
unblocking reconfigure() with the remove_map_callback() function.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/multipathd/main.c b/multipathd/main.c
index f3b8eb4..4721cd8 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -319,6 +319,17 @@ static bool unblock_reconfigure(void)
 	return was_delayed;
 }
 
+/*
+ * Make sure child() is woken up when a map is removed that multipathd
+ * is currently waiting for.
+ * Overrides libmultipath's weak symbol by the same name
+ */
+void remove_map_callback(struct multipath *mpp)
+{
+	if (mpp->wait_for_udev > 0)
+		unblock_reconfigure();
+}
+
 void schedule_reconfigure(enum force_reload_types requested_type)
 {
 	pthread_mutex_lock(&config_lock);
-- 
2.35.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 07/11] multipathd: use remove_map_callback for delayed reconfigure

[Benjamin Marzinski]

On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
>
> From: Martin Wilck <mwilck@suse.com>
>
> If multipathd needs to delay reconfigure() because it's waiting for a map
> creation uevent, it can happen that child() isn't woken up if the map
> being waited for is removed before the uevent arrives. Fix this by
> unblocking reconfigure() with the remove_map_callback() function.
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index f3b8eb4..4721cd8 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -319,6 +319,17 @@ static bool unblock_reconfigure(void)
>         return was_delayed;
>  }
>
> +/*
> + * Make sure child() is woken up when a map is removed that multipathd
> + * is currently waiting for.
> + * Overrides libmultipath's weak symbol by the same name
> + */
> +void remove_map_callback(struct multipath *mpp)
> +{
> +       if (mpp->wait_for_udev > 0)

Is there a reason why you don't decrement wait_for_udev, and check
need_to_delay_reconfig() like in ev_add_map()? I realize that it
doesn't hurt anything to unblock the reconfigure even if there are
other devices that need a delay. The main thread will notice that it
still needs to delay. I'm just wondering why we do it differently
here?

-Ben

> +               unblock_reconfigure();
> +}
> +
>  void schedule_reconfigure(enum force_reload_types requested_type)
>  {
>         pthread_mutex_lock(&config_lock);
> --
> 2.35.1
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 07/11] multipathd: use remove_map_callback for delayed reconfigure

[Martin Wilck]

On Mon, 2022-03-21 at 19:34 -0500, Benjamin Marzinski wrote:
> On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
> > 
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > If multipathd needs to delay reconfigure() because it's waiting for
> > a map
> > creation uevent, it can happen that child() isn't woken up if the
> > map
> > being waited for is removed before the uevent arrives. Fix this by
> > unblocking reconfigure() with the remove_map_callback() function.
> > 
> > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > ---
> >  multipathd/main.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index f3b8eb4..4721cd8 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -319,6 +319,17 @@ static bool unblock_reconfigure(void)
> >         return was_delayed;
> >  }
> > 
> > +/*
> > + * Make sure child() is woken up when a map is removed that
> > multipathd
> > + * is currently waiting for.
> > + * Overrides libmultipath's weak symbol by the same name
> > + */
> > +void remove_map_callback(struct multipath *mpp)
> > +{
> > +       if (mpp->wait_for_udev > 0)
> 
> Is there a reason why you don't decrement wait_for_udev, and check
> need_to_delay_reconfig() like in ev_add_map()? I realize that it
> doesn't hurt anything to unblock the reconfigure even if there are
> other devices that need a delay. The main thread will notice that it
> still needs to delay. I'm just wondering why we do it differently
> here?

The main reason was to keep it simple. need_to_delay_reconfig() needs
to be passed a "vecs" pointer, and requires the vecs lock to be
held. remove_map() is deep down in the call stack and has lots of
direct and indirect callers. It can be called with mpvec == NULL and
(in theory) also with pathvec == NULL, in which case it simply frees
the memory obtained by the map, without unlinking itself or its members
from any vectors. In that case it *could* be called without the vecs
lock held (AFAICS, that's not the case today, but the function could be
used this way, e.g. in an error handling code path).

I thought it was easier and safer not to have to assert that every
current and future caller holds the vecs lock, in particular because
this is called indirectly from libmultipath, the function that grabs
the lock is usually high up in the call stack.

I had indeed pondered whether I should remove the call to
need_to_delay_reconfig() from the ev_add_map(), too. I decided against
it, because I realized that waking up child() for nothing is not
cheap,as child() needs to grab the vecs lock just for calling
need_to_delay_reconfig(). We should avoid this for the common case of 
an uevent which we are waiting for. 
The remove_map() code path, OTOH, is a corner case (map being removed
while in the process of being added). We need to cover it, but we know
that this code path will be rarely executed in practice, and thus
unlikely to cause vecs lock contention.

I hope this makes sense.

Regards
Martin

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 07/11] multipathd: use remove_map_callback for delayed reconfigure

[Benjamin Marzinski]

On Tue, Mar 22, 2022 at 4:08 AM Martin Wilck <mwilck@suse.com> wrote:
>
> On Mon, 2022-03-21 at 19:34 -0500, Benjamin Marzinski wrote:
> > On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
> > >
> > > From: Martin Wilck <mwilck@suse.com>
> > >
> > > If multipathd needs to delay reconfigure() because it's waiting for
> > > a map
> > > creation uevent, it can happen that child() isn't woken up if the
> > > map
> > > being waited for is removed before the uevent arrives. Fix this by
> > > unblocking reconfigure() with the remove_map_callback() function.
> > >
> > > Signed-off-by: Martin Wilck <mwilck@suse.com>
> > > ---
> > >  multipathd/main.c | 11 +++++++++++
> > >  1 file changed, 11 insertions(+)
> > >
> > > diff --git a/multipathd/main.c b/multipathd/main.c
> > > index f3b8eb4..4721cd8 100644
> > > --- a/multipathd/main.c
> > > +++ b/multipathd/main.c
> > > @@ -319,6 +319,17 @@ static bool unblock_reconfigure(void)
> > >         return was_delayed;
> > >  }
> > >
> > > +/*
> > > + * Make sure child() is woken up when a map is removed that
> > > multipathd
> > > + * is currently waiting for.
> > > + * Overrides libmultipath's weak symbol by the same name
> > > + */
> > > +void remove_map_callback(struct multipath *mpp)
> > > +{
> > > +       if (mpp->wait_for_udev > 0)
> >
> > Is there a reason why you don't decrement wait_for_udev, and check
> > need_to_delay_reconfig() like in ev_add_map()? I realize that it
> > doesn't hurt anything to unblock the reconfigure even if there are
> > other devices that need a delay. The main thread will notice that it
> > still needs to delay. I'm just wondering why we do it differently
> > here?
>
> The main reason was to keep it simple. need_to_delay_reconfig() needs
> to be passed a "vecs" pointer, and requires the vecs lock to be
> held. remove_map() is deep down in the call stack and has lots of
> direct and indirect callers. It can be called with mpvec == NULL and
> (in theory) also with pathvec == NULL, in which case it simply frees
> the memory obtained by the map, without unlinking itself or its members
> from any vectors. In that case it *could* be called without the vecs
> lock held (AFAICS, that's not the case today, but the function could be
> used this way, e.g. in an error handling code path).
>
> I thought it was easier and safer not to have to assert that every
> current and future caller holds the vecs lock, in particular because
> this is called indirectly from libmultipath, the function that grabs
> the lock is usually high up in the call stack.
>
> I had indeed pondered whether I should remove the call to
> need_to_delay_reconfig() from the ev_add_map(), too. I decided against
> it, because I realized that waking up child() for nothing is not
> cheap,as child() needs to grab the vecs lock just for calling
> need_to_delay_reconfig(). We should avoid this for the common case of
> an uevent which we are waiting for.
> The remove_map() code path, OTOH, is a corner case (map being removed
> while in the process of being added). We need to cover it, but we know
> that this code path will be rarely executed in practice, and thus
> unlikely to cause vecs lock contention.
>
> I hope this makes sense.

Yeah. Whenever a map is getting removed from vecs->mpvec, the vecs
lock better be held. But since a map could be getting removed from
some other vector of maps, we can't use (mpvec != NULL) to be sure
that the vecs lock must be held.

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

>
> Regards
> Martin
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 08/11] libmultipath: warn only once about deprecated options

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 libmultipath/dict.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/libmultipath/dict.c b/libmultipath/dict.c
index 2af9764..26cbe78 100644
--- a/libmultipath/dict.c
+++ b/libmultipath/dict.c
@@ -279,7 +279,11 @@ static int								\
 def_ ## option ## _handler (struct config *conf, vector strvec,		\
 			    const char *file, int line_nr)		\
 {									\
-	condlog(2, "%s line %d, \"" #option "\" is deprecated and will be disabled in a future release", file, line_nr);				\
+	static bool warned;						\
+	if (!warned) {							\
+		condlog(2, "%s line %d, \"" #option "\" is deprecated and will be disabled in a future release", file, line_nr); \
+		warned = true;						\
+	}								\
 	return function (strvec, &conf->option, file, line_nr);		\
 }
 
@@ -829,14 +833,19 @@ static int
 def_config_dir_handler(struct config *conf, vector strvec, const char *file,
 		       int line_nr)
 {
+	static bool warned;
+
 	/* this is only valid in the main config file */
 	if (conf->processed_main_config) {
 		condlog(1, "%s line %d, config_dir option only valid in /etc/multipath.conf",
 			file, line_nr);
 		return 0;
 	}
-	condlog(2, "%s line %d, \"config_dir\" is deprecated and will be disabled in a future release",
-		file, line_nr);
+	if (!warned) {
+		condlog(2, "%s line %d, \"config_dir\" is deprecated and will be disabled in a future release",
+			file, line_nr);
+		warned = true;
+	}
 	return set_path(strvec, &conf->config_dir, file, line_nr);
 }
 declare_def_snprint(config_dir, print_str)
-- 
2.35.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 08/11] libmultipath: warn only once about deprecated options

[Benjamin Marzinski]

On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
>
> From: Martin Wilck <mwilck@suse.com>
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> ---
>  libmultipath/dict.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
>
> diff --git a/libmultipath/dict.c b/libmultipath/dict.c
> index 2af9764..26cbe78 100644
> --- a/libmultipath/dict.c
> +++ b/libmultipath/dict.c
> @@ -279,7 +279,11 @@ static int                                                         \
>  def_ ## option ## _handler (struct config *conf, vector strvec,                \
>                             const char *file, int line_nr)              \
>  {                                                                      \
> -       condlog(2, "%s line %d, \"" #option "\" is deprecated and will be disabled in a future release", file, line_nr);                                \
> +       static bool warned;                                             \
> +       if (!warned) {                                                  \
> +               condlog(2, "%s line %d, \"" #option "\" is deprecated and will be disabled in a future release", file, line_nr); \
> +               warned = true;                                          \
> +       }                                                               \
>         return function (strvec, &conf->option, file, line_nr);         \
>  }
>
> @@ -829,14 +833,19 @@ static int
>  def_config_dir_handler(struct config *conf, vector strvec, const char *file,
>                        int line_nr)
>  {
> +       static bool warned;
> +
>         /* this is only valid in the main config file */
>         if (conf->processed_main_config) {
>                 condlog(1, "%s line %d, config_dir option only valid in /etc/multipath.conf",
>                         file, line_nr);
>                 return 0;
>         }
> -       condlog(2, "%s line %d, \"config_dir\" is deprecated and will be disabled in a future release",
> -               file, line_nr);
> +       if (!warned) {
> +               condlog(2, "%s line %d, \"config_dir\" is deprecated and will be disabled in a future release",
> +                       file, line_nr);
> +               warned = true;
> +       }
>         return set_path(strvec, &conf->config_dir, file, line_nr);
>  }
>  declare_def_snprint(config_dir, print_str)
> --
> 2.35.1
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 09/11] multipathd: improve logging of reconfigure()

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Print a message when reconfigure() actually starts doing something,
and when a reconfigure() call is delayed.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/multipathd/main.c b/multipathd/main.c
index 4721cd8..e841ba8 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2840,6 +2840,7 @@ reconfigure (struct vectors *vecs, enum force_reload_types reload_type)
 	if (verbosity)
 		libmp_verbosity = verbosity;
 	setlogmask(LOG_UPTO(libmp_verbosity + 3));
+	condlog(2, "%s: setting up paths and maps", __func__);
 
 	/*
 	 * free old map and path vectors ... they use old conf state
@@ -3419,6 +3420,7 @@ child (__attribute__((unused)) void *param)
 			pthread_mutex_lock(&config_lock);
 			__delayed_reconfig = true;
 			pthread_mutex_unlock(&config_lock);
+			condlog(3, "delaying reconfigure()");
 		}
 		lock_cleanup_pop(vecs->lock);
 		if (!rc)
-- 
2.35.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 09/11] multipathd: improve logging of reconfigure()

[Benjamin Marzinski]

On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
>
> From: Martin Wilck <mwilck@suse.com>
>
> Print a message when reconfigure() actually starts doing something,
> and when a reconfigure() call is delayed.
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> ---
>  multipathd/main.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 4721cd8..e841ba8 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2840,6 +2840,7 @@ reconfigure (struct vectors *vecs, enum force_reload_types reload_type)
>         if (verbosity)
>                 libmp_verbosity = verbosity;
>         setlogmask(LOG_UPTO(libmp_verbosity + 3));
> +       condlog(2, "%s: setting up paths and maps", __func__);
>
>         /*
>          * free old map and path vectors ... they use old conf state
> @@ -3419,6 +3420,7 @@ child (__attribute__((unused)) void *param)
>                         pthread_mutex_lock(&config_lock);
>                         __delayed_reconfig = true;
>                         pthread_mutex_unlock(&config_lock);
> +                       condlog(3, "delaying reconfigure()");
>                 }
>                 lock_cleanup_pop(vecs->lock);
>                 if (!rc)
> --
> 2.35.1
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 10/11] multipathd: log state changes

[mwilck]

From: Martin Wilck <mwilck@suse.com>

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/multipathd/main.c b/multipathd/main.c
index e841ba8..4b4fa9c 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -287,6 +287,8 @@ static void __post_config_state(enum daemon_status state)
 		running_state = state;
 		pthread_cond_broadcast(&config_cond);
 		do_sd_notify(old_state, state);
+		condlog(4, "daemon state %s -> %s",
+			daemon_status_msg[old_state], daemon_status_msg[state]);
 	}
 }
 
-- 
2.35.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 10/11] multipathd: log state changes

[Benjamin Marzinski]

On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
>
> From: Martin Wilck <mwilck@suse.com>
>
> Signed-off-by: Martin Wilck <mwilck@suse.com>

Reviewed-by: Benjamin Marzinski <bmarzins@redhat.com>

> ---
>  multipathd/main.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index e841ba8..4b4fa9c 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -287,6 +287,8 @@ static void __post_config_state(enum daemon_status state)
>                 running_state = state;
>                 pthread_cond_broadcast(&config_cond);
>                 do_sd_notify(old_state, state);
> +               condlog(4, "daemon state %s -> %s",
> +                       daemon_status_msg[old_state], daemon_status_msg[state]);
>         }
>  }
>
> --
> 2.35.1
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH v2 11/11] multipathd: remove unhelpful startup / shutdown messages

[mwilck]

From: Martin Wilck <mwilck@suse.com>

These messages are noisy in the system log without being actually helpful.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 multipathd/main.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 4b4fa9c..6e6635b 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2560,7 +2560,6 @@ checkerloop (void *ap)
 	rcu_register_thread();
 	mlockall(MCL_CURRENT | MCL_FUTURE);
 	vecs = (struct vectors *)ap;
-	condlog(2, "path checkers start up");
 
 	/* Tweak start time for initial path check */
 	get_monotonic_time(&last_time);
@@ -3241,8 +3240,7 @@ child (__attribute__((unused)) void *param)
 
 	post_config_state(DAEMON_START);
 
-	condlog(2, "--------start up--------");
-	condlog(2, "read " DEFAULT_CONFIGFILE);
+	condlog(3, "read " DEFAULT_CONFIGFILE);
 
 	if (verbosity)
 		libmp_verbosity = verbosity;
@@ -3435,7 +3433,6 @@ child (__attribute__((unused)) void *param)
 
 	exit_code = 0;
 failed:
-	condlog(2, "--------shut down-------");
 	/* All cleanup is done in the cleanup_child() exit handler */
 	return sd_notify_exit(exit_code);
 }
-- 
2.35.1

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 11/11] multipathd: remove unhelpful startup / shutdown messages

[Benjamin Marzinski]

On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
>
> From: Martin Wilck <mwilck@suse.com>
>
> These messages are noisy in the system log without being actually helpful.

I've actually found the "start up" and "shut down" messages useful a
number of times, for tracking when multipathd starts up and shuts
down. Since people generally run multipathd constantly, they rarely
appear more than a couple of times per boot. I would prefer if they
could stay.  I'm fine with removing the others.

-Ben

> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  multipathd/main.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 4b4fa9c..6e6635b 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -2560,7 +2560,6 @@ checkerloop (void *ap)
>         rcu_register_thread();
>         mlockall(MCL_CURRENT | MCL_FUTURE);
>         vecs = (struct vectors *)ap;
> -       condlog(2, "path checkers start up");
>
>         /* Tweak start time for initial path check */
>         get_monotonic_time(&last_time);
> @@ -3241,8 +3240,7 @@ child (__attribute__((unused)) void *param)
>
>         post_config_state(DAEMON_START);
>
> -       condlog(2, "--------start up--------");
> -       condlog(2, "read " DEFAULT_CONFIGFILE);
> +       condlog(3, "read " DEFAULT_CONFIGFILE);
>
>         if (verbosity)
>                 libmp_verbosity = verbosity;
> @@ -3435,7 +3433,6 @@ child (__attribute__((unused)) void *param)
>
>         exit_code = 0;
>  failed:
> -       condlog(2, "--------shut down-------");
>         /* All cleanup is done in the cleanup_child() exit handler */
>         return sd_notify_exit(exit_code);
>  }
> --
> 2.35.1
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 11/11] multipathd: remove unhelpful startup / shutdown messages

[Martin Wilck]

On Mon, 2022-03-21 at 19:43 -0500, Benjamin Marzinski wrote:
> On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
> > 
> > From: Martin Wilck <mwilck@suse.com>
> > 
> > These messages are noisy in the system log without being actually
> > helpful.
> 
> I've actually found the "start up" and "shut down" messages useful a
> number of times, for tracking when multipathd starts up and shuts
> down. 

Makes sense ;-)

Currently we see the following messages for multipathd startup and
shutdown:

Mar 11 09:30:00 bremer systemd[1]: Starting Device-Mapper Multipath Device Controller.
Mar 11 09:30:01 bremer multipathd[363]: --------start up--------
Mar 11 09:30:01 bremer systemd[1]: Started Device-Mapper Multipath Device Controller.
Mar 11 09:30:01 bremer multipathd[363]: read /etc/multipath.conf
Mar 11 09:30:01 bremer multipathd[363]: path checkers start up
...
Mar 11 09:30:52 bremer systemd[1]: Stopping Device-Mapper Multipath Device Controller...
Mar 11 09:30:52 bremer multipathd[363]: exit (signal)
Mar 11 09:30:52 bremer multipathd[363]: --------shut down-------
Mar 11 09:30:52 bremer systemd[1]: Stopped Device-Mapper Multipath Device Controller.

To my taste, this is too much. Of course, not everyone is using
systemd. Without systemd and with the part of my patch you acked, we'd
be down from 9 to 3 messages. IMO either the "exit" message or the
"shut down" message could be hidden at -v2. I suppose we could decrease
the verbosity level of handle_signals() to -v3 instead. Would you agree
with that?

> Since people generally run multipathd constantly, they rarely
> appear more than a couple of times per boot. I would prefer if they
> could stay.  I'm fine with removing the others.

Ok, fine with me. Do you insist on the "--------", too? It's mainly
that which bothers me. If you look at the typical boot messages of
contemporary Linux servers, no other daemon uses this strong emphasis
for an informational message. The informational value would be higher
if we printed a detailed version number including HEAD commit ID, like
other daemons do.

Martin

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [dm-devel] [PATCH v2 11/11] multipathd: remove unhelpful startup / shutdown messages

[Benjamin Marzinski]

On Tue, Mar 22, 2022 at 4:30 AM Martin Wilck <mwilck@suse.com> wrote:
>
> On Mon, 2022-03-21 at 19:43 -0500, Benjamin Marzinski wrote:
> > On Fri, Mar 18, 2022 at 5:33 PM <mwilck@suse.com> wrote:
> > >
> > > From: Martin Wilck <mwilck@suse.com>
> > >
> > > These messages are noisy in the system log without being actually
> > > helpful.
> >
> > I've actually found the "start up" and "shut down" messages useful a
> > number of times, for tracking when multipathd starts up and shuts
> > down.
>
> Makes sense ;-)
>
> Currently we see the following messages for multipathd startup and
> shutdown:
>
> Mar 11 09:30:00 bremer systemd[1]: Starting Device-Mapper Multipath Device Controller.
> Mar 11 09:30:01 bremer multipathd[363]: --------start up--------
> Mar 11 09:30:01 bremer systemd[1]: Started Device-Mapper Multipath Device Controller.
> Mar 11 09:30:01 bremer multipathd[363]: read /etc/multipath.conf
> Mar 11 09:30:01 bremer multipathd[363]: path checkers start up
> ...
> Mar 11 09:30:52 bremer systemd[1]: Stopping Device-Mapper Multipath Device Controller...
> Mar 11 09:30:52 bremer multipathd[363]: exit (signal)
> Mar 11 09:30:52 bremer multipathd[363]: --------shut down-------
> Mar 11 09:30:52 bremer systemd[1]: Stopped Device-Mapper Multipath Device Controller.
>
> To my taste, this is too much. Of course, not everyone is using
> systemd. Without systemd and with the part of my patch you acked, we'd
> be down from 9 to 3 messages. IMO either the "exit" message or the
> "shut down" message could be hidden at -v2. I suppose we could decrease
> the verbosity level of handle_signals() to -v3 instead. Would you agree
> with that?

Sure.

> > Since people generally run multipathd constantly, they rarely
> > appear more than a couple of times per boot. I would prefer if they
> > could stay.  I'm fine with removing the others.
>
> Ok, fine with me. Do you insist on the "--------", too? It's mainly
> that which bothers me. If you look at the typical boot messages of
> contemporary Linux servers, no other daemon uses this strong emphasis
> for an informational message. The informational value would be higher
> if we printed a detailed version number including HEAD commit ID, like
> other daemons do.

I'm fine with changing what the messages look like, I'd just like
something to stay there.

-Ben

> Martin
>

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel