[dm-devel] [PATCH 0/4] Add "reconfigure all" multipath command

[dm-devel] [PATCH 0/4] Add "reconfigure all" multipath command

[Benjamin Marzinski]

This patchset is supposed to replace Martin's

multipathd: add "force_reconfigure" option

patch from his uxlsnr overhaul patchset. It also makes the default
reconfigure be a weak reconfigure, but instead of adding a configuration
option to control this, it adds a new multipathd command,
"reconfigure all", to do a full reconfigure. The HUP signal is left
doing only weak reconfigures.
 
In order to keep from having two states that are handled nearly
identically, the code adds an extra variable to track the type of
configuration that was selected, but this could easily be switch to
use a new DAEMON_CONFIGURE_ALL state instead.
 
The final patch, that added the new command, is meant to apply on top of
Martin's changed client handler code. I can send one that works with the
current client handler code, if people would rather review that.

Benjamin Marzinski (4):
  multipathd: move delayed_reconfig out of struct config
  multipathd: remove reconfigure from header file.
  multipathd: pass in the type of reconfigure
  multipathd: add "reconfigure all" command.

 libmultipath/config.h     |  1 -
 libmultipath/configure.c  |  2 +-
 multipath/main.c          |  2 +-
 multipathd/cli.c          |  1 +
 multipathd/cli.h          |  2 +
 multipathd/cli_handlers.c | 12 +++++-
 multipathd/main.c         | 91 +++++++++++++++++++++++----------------
 multipathd/main.h         |  3 +-
 8 files changed, 70 insertions(+), 44 deletions(-)

-- 
2.17.2

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

[dm-devel] [PATCH 1/4] multipathd: move delayed_reconfig out of struct config

[Benjamin Marzinski]

delayed_reconfig was inside the config struct, but it wasn't updated
during an RCU write section, so there's no synchronization on it.
Instead, pull it out of the config structure, and use the config_lock
to synchronize it.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/config.h |  1 -
 multipathd/main.c     | 45 ++++++++++++++++++++++++++++---------------
 2 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/libmultipath/config.h b/libmultipath/config.h
index 933fe0d1..c73389b5 100644
--- a/libmultipath/config.h
+++ b/libmultipath/config.h
@@ -178,7 +178,6 @@ struct config {
 	int strict_timing;
 	int retrigger_tries;
 	int retrigger_delay;
-	int delayed_reconfig;
 	int uev_wait_timeout;
 	int skip_kpartx;
 	int remove_retries;
diff --git a/multipathd/main.c b/multipathd/main.c
index 2b416af9..1ead0904 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -127,6 +127,8 @@ static int poll_dmevents = 1;
 #endif
 /* Don't access this variable without holding config_lock */
 static volatile enum daemon_status running_state = DAEMON_INIT;
+/* Don't access this variable without holding config_lock */
+static bool __delayed_reconfig;
 pid_t daemon_pid;
 static pthread_mutex_t config_lock = PTHREAD_MUTEX_INITIALIZER;
 static pthread_cond_t config_cond;
@@ -150,6 +152,23 @@ 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;
+}
+
+static void set_delayed_reconfig(bool val)
+{
+	pthread_mutex_lock(&config_lock);
+	__delayed_reconfig = val;
+	pthread_mutex_unlock(&config_lock);
+}
+
 /*
  * global copy of vecs for use in sig handlers
  */
@@ -297,8 +316,10 @@ static void __post_config_state(enum daemon_status state)
 			old_state = DAEMON_IDLE;
 			state = DAEMON_CONFIGURE;
 		}
-		if (reconfigure_pending && state == DAEMON_CONFIGURE)
+		if (state == DAEMON_CONFIGURE) {
 			reconfigure_pending = false;
+			__delayed_reconfig = false;
+		}
 		running_state = state;
 		pthread_cond_broadcast(&config_cond);
 		do_sd_notify(old_state, state);
@@ -765,7 +786,7 @@ int
 ev_add_map (char * dev, const char * alias, struct vectors * vecs)
 {
 	struct multipath * mpp;
-	int delayed_reconfig, reassign_maps;
+	int reassign_maps;
 	struct config *conf;
 
 	if (dm_is_mpath(alias) != 1) {
@@ -784,12 +805,11 @@ ev_add_map (char * dev, const char * alias, struct vectors * vecs)
 				return 1;
 		}
 		conf = get_multipath_config();
-		delayed_reconfig = conf->delayed_reconfig;
 		reassign_maps = conf->reassign_maps;
 		put_multipath_config(conf);
 		if (mpp->wait_for_udev) {
 			mpp->wait_for_udev = 0;
-			if (delayed_reconfig &&
+			if (get_delayed_reconfig() &&
 			    !need_to_delay_reconfig(vecs)) {
 				condlog(2, "reconfigure (delayed)");
 				schedule_reconfigure();
@@ -1791,8 +1811,7 @@ missing_uev_wait_tick(struct vectors *vecs)
 {
 	struct multipath * mpp;
 	unsigned int i;
-	int timed_out = 0, delayed_reconfig;
-	struct config *conf;
+	int timed_out = 0;
 
 	vector_foreach_slot (vecs->mpvec, mpp, i) {
 		if (mpp->wait_for_udev && --mpp->uev_wait_tick <= 0) {
@@ -1808,10 +1827,7 @@ missing_uev_wait_tick(struct vectors *vecs)
 		}
 	}
 
-	conf = get_multipath_config();
-	delayed_reconfig = conf->delayed_reconfig;
-	put_multipath_config(conf);
-	if (timed_out && delayed_reconfig &&
+	if (timed_out && get_delayed_reconfig() &&
 	    !need_to_delay_reconfig(vecs)) {
 		condlog(2, "reconfigure (delayed)");
 		schedule_reconfigure();
@@ -3257,13 +3273,10 @@ child (__attribute__((unused)) void *param)
 			pthread_cleanup_push(cleanup_lock, &vecs->lock);
 			lock(&vecs->lock);
 			pthread_testcancel();
-			if (!need_to_delay_reconfig(vecs)) {
+			if (!need_to_delay_reconfig(vecs))
 				reconfigure(vecs);
-			} else {
-				conf = get_multipath_config();
-				conf->delayed_reconfig = 1;
-				put_multipath_config(conf);
-			}
+			else
+				set_delayed_reconfig(true);
 			lock_cleanup_pop(vecs->lock);
 			post_config_state(DAEMON_IDLE);
 		}
-- 
2.17.2

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

[dm-devel] [PATCH 2/4] multipathd: remove reconfigure from header file.

[Benjamin Marzinski]

Only multipathd/main.c uses it.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/multipathd/main.h b/multipathd/main.h
index 23ce919e..a1697a74 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -39,7 +39,6 @@ enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
 					    unsigned long ms);
 void schedule_reconfigure(void);
 int need_to_delay_reconfig (struct vectors *);
-int reconfigure (struct vectors *);
 int ev_add_path (struct path *, struct vectors *, int);
 int ev_remove_path (struct path *, struct vectors *, int);
 int ev_add_map (char *, const char *, struct vectors *);
-- 
2.17.2

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

[dm-devel] [PATCH 3/4] multipathd: pass in the type of reconfigure

[Benjamin Marzinski]

This code doesn't actually change how multipathd reconfigures. It still
does a weak reconfigure at the start, and full reconfigures later. But
now schedule_reconfigure() takes the type of reconfigure to do, and that
gets passed down to reconfigure(). If a full reconfigure has already
been requested but not started, weak reconfigure requests will be
upgraded. A future patch will enable users to control what kind of
reconfigures happen.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c  |  2 +-
 multipathd/cli_handlers.c |  2 +-
 multipathd/main.c         | 62 ++++++++++++++++++++-------------------
 multipathd/main.h         |  2 +-
 4 files changed, 35 insertions(+), 33 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index 7edb355b..eb8ec1bd 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1098,7 +1098,7 @@ out:
  * FORCE_RELOAD_NONE: existing maps aren't touched at all
  * FORCE_RELOAD_YES: all maps are rebuilt from scratch and (re)loaded in DM
  * FORCE_RELOAD_WEAK: existing maps are compared to the current conf and only
- * reloaded in DM if there's a difference. This is useful during startup.
+ * reloaded in DM if there's a difference. This is normally sufficient.
  */
 int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
 		    int force_reload, enum mpath_cmds cmd)
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index f59db3ab..b12a4e7e 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1012,7 +1012,7 @@ cli_reconfigure(void * v, struct strbuf *reply, void * data)
 {
 	condlog(2, "reconfigure (operator)");
 
-	schedule_reconfigure();
+	schedule_reconfigure(true);
 	return 0;
 }
 
diff --git a/multipathd/main.c b/multipathd/main.c
index 1ead0904..5c831e8d 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -162,13 +162,6 @@ static bool get_delayed_reconfig(void)
 	return val;
 }
 
-static void set_delayed_reconfig(bool val)
-{
-	pthread_mutex_lock(&config_lock);
-	__delayed_reconfig = val;
-	pthread_mutex_unlock(&config_lock);
-}
-
 /*
  * global copy of vecs for use in sig handlers
  */
@@ -290,7 +283,18 @@ enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
 }
 
 /* Don't access this variable without holding config_lock */
-static bool reconfigure_pending;
+static enum force_reload_types reconfigure_pending = FORCE_RELOAD_NONE;
+/* Only set while changing to DAEMON_CONFIGURE, and only access while
+ * reconfiguring in DAEMON_CONFIGURE */
+static volatile enum force_reload_types reload_type = FORCE_RELOAD_NONE;
+
+static void enable_delayed_reconfig(enum force_reload_types type)
+{
+	pthread_mutex_lock(&config_lock);
+	reconfigure_pending = type;
+	__delayed_reconfig = true;
+	pthread_mutex_unlock(&config_lock);
+}
 
 /* must be called with config_lock held */
 static void __post_config_state(enum daemon_status state)
@@ -305,7 +309,8 @@ static void __post_config_state(enum daemon_status state)
 		 * In either case, child() will see DAEMON_CONFIGURE
 		 * again and start another reconfigure cycle.
 		 */
-		if (reconfigure_pending && state == DAEMON_IDLE &&
+		if (reconfigure_pending != FORCE_RELOAD_NONE &&
+		    state == DAEMON_IDLE &&
 		    (old_state == DAEMON_CONFIGURE ||
 		     old_state == DAEMON_RUNNING)) {
 			/*
@@ -317,7 +322,8 @@ static void __post_config_state(enum daemon_status state)
 			state = DAEMON_CONFIGURE;
 		}
 		if (state == DAEMON_CONFIGURE) {
-			reconfigure_pending = false;
+			reload_type = (reconfigure_pending == FORCE_RELOAD_YES) ? FORCE_RELOAD_YES : FORCE_RELOAD_WEAK;
+			reconfigure_pending = FORCE_RELOAD_NONE;
 			__delayed_reconfig = false;
 		}
 		running_state = state;
@@ -334,20 +340,25 @@ void post_config_state(enum daemon_status state)
 	pthread_cleanup_pop(1);
 }
 
-void schedule_reconfigure(void)
+void schedule_reconfigure(bool force_reload_yes)
 {
 	pthread_mutex_lock(&config_lock);
 	pthread_cleanup_push(config_cleanup, NULL);
+	enum force_reload_types type;
+
+	type = (reconfigure_pending == FORCE_RELOAD_YES || force_reload_yes) ?
+	       FORCE_RELOAD_YES : FORCE_RELOAD_WEAK;
 	switch (running_state)
 	{
 	case DAEMON_SHUTDOWN:
 		break;
 	case DAEMON_IDLE:
+		reconfigure_pending = type;
 		__post_config_state(DAEMON_CONFIGURE);
 		break;
 	case DAEMON_CONFIGURE:
 	case DAEMON_RUNNING:
-		reconfigure_pending = true;
+		reconfigure_pending = type;
 		break;
 	default:
 		break;
@@ -812,7 +823,7 @@ ev_add_map (char * dev, const char * alias, struct vectors * vecs)
 			if (get_delayed_reconfig() &&
 			    !need_to_delay_reconfig(vecs)) {
 				condlog(2, "reconfigure (delayed)");
-				schedule_reconfigure();
+				schedule_reconfigure(false);
 				return 0;
 			}
 		}
@@ -1830,7 +1841,7 @@ missing_uev_wait_tick(struct vectors *vecs)
 	if (timed_out && get_delayed_reconfig() &&
 	    !need_to_delay_reconfig(vecs)) {
 		condlog(2, "reconfigure (delayed)");
-		schedule_reconfigure();
+		schedule_reconfigure(false);
 	}
 }
 
@@ -2588,14 +2599,13 @@ checkerloop (void *ap)
 }
 
 int
-configure (struct vectors * vecs)
+configure (struct vectors * vecs, enum force_reload_types type)
 {
 	struct multipath * mpp;
 	struct path * pp;
 	vector mpvec;
 	int i, ret;
 	struct config *conf;
-	static int force_reload = FORCE_RELOAD_WEAK;
 
 	if (!vecs->pathvec && !(vecs->pathvec = vector_alloc())) {
 		condlog(0, "couldn't allocate path vec in configure");
@@ -2643,15 +2653,7 @@ configure (struct vectors * vecs)
 	if (should_exit())
 		goto fail;
 
-	/*
-	 * create new set of maps & push changed ones into dm
-	 * In the first call, use FORCE_RELOAD_WEAK to avoid making
-	 * superfluous ACT_RELOAD ioctls. Later calls are done
-	 * with FORCE_RELOAD_YES.
-	 */
-	ret = coalesce_paths(vecs, mpvec, NULL, force_reload, CMD_NONE);
-	if (force_reload == FORCE_RELOAD_WEAK)
-		force_reload = FORCE_RELOAD_YES;
+	ret = coalesce_paths(vecs, mpvec, NULL, type, CMD_NONE);
 	if (ret != CP_OK) {
 		condlog(0, "configure failed while coalescing paths");
 		goto fail;
@@ -2729,7 +2731,7 @@ void rcu_free_config(struct rcu_head *head)
 }
 
 int
-reconfigure (struct vectors * vecs)
+reconfigure (struct vectors * vecs, enum force_reload_types type)
 {
 	struct config * old, *conf;
 
@@ -2763,7 +2765,7 @@ reconfigure (struct vectors * vecs)
 	rcu_assign_pointer(multipath_conf, conf);
 	call_rcu(&old->rcu, rcu_free_config);
 
-	configure(vecs);
+	configure(vecs, type);
 
 
 	return 0;
@@ -2815,7 +2817,7 @@ handle_signals(bool nonfatal)
 		return;
 	if (reconfig_sig) {
 		condlog(2, "reconfigure (signal)");
-		schedule_reconfigure();
+		schedule_reconfigure(true);
 	}
 	if (log_reset_sig) {
 		condlog(2, "reset log (signal)");
@@ -3274,9 +3276,9 @@ child (__attribute__((unused)) void *param)
 			lock(&vecs->lock);
 			pthread_testcancel();
 			if (!need_to_delay_reconfig(vecs))
-				reconfigure(vecs);
+				reconfigure(vecs, reload_type);
 			else
-				set_delayed_reconfig(true);
+				enable_delayed_reconfig(reload_type);
 			lock_cleanup_pop(vecs->lock);
 			post_config_state(DAEMON_IDLE);
 		}
diff --git a/multipathd/main.h b/multipathd/main.h
index a1697a74..c8a1ce92 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -37,7 +37,7 @@ void exit_daemon(void);
 const char * daemon_status(void);
 enum daemon_status wait_for_state_change_if(enum daemon_status oldstate,
 					    unsigned long ms);
-void schedule_reconfigure(void);
+void schedule_reconfigure(bool reconfig_all);
 int need_to_delay_reconfig (struct vectors *);
 int ev_add_path (struct path *, struct vectors *, int);
 int ev_remove_path (struct path *, struct vectors *, int);
-- 
2.17.2

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

[dm-devel] [PATCH 4/4] multipathd: add "reconfigure all" command.

[Benjamin Marzinski]

With this commit, multipathd no longer defaults to full reconfigures for
the "reconfigure" command and the HUP signal. The default is a weak
reconfigure. A new command, "reconfigure all", has been added to do
a full reconfigure.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipath/main.c          |  2 +-
 multipathd/cli.c          |  1 +
 multipathd/cli.h          |  2 ++
 multipathd/cli_handlers.c | 10 ++++++++++
 multipathd/main.c         |  2 +-
 5 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/multipath/main.c b/multipath/main.c
index 65ece830..7b9797ec 100644
--- a/multipath/main.c
+++ b/multipath/main.c
@@ -759,7 +759,7 @@ int delegate_to_multipathd(enum mpath_cmds cmd,
 		return NOT_DELEGATED;
 
 	if (cmd == CMD_CREATE && conf->force_reload == FORCE_RELOAD_YES) {
-		p += snprintf(p, n, "reconfigure");
+		p += snprintf(p, n, "reconfigure all");
 	}
 	else if (cmd == CMD_FLUSH_ONE && dev && dev_type == DEV_DEVMAP) {
 		p += snprintf(p, n, "del map %s", dev);
diff --git a/multipathd/cli.c b/multipathd/cli.c
index 414f6608..d9308bdf 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -212,6 +212,7 @@ load_keys (void)
 	r += add_key(keys, "local", LOCAL, 0);
 	r += add_key(keys, "setmarginal", SETMARGINAL, 0);
 	r += add_key(keys, "unsetmarginal", UNSETMARGINAL, 0);
+	r += add_key(keys, "all", ALL, 0);
 
 
 	if (r) {
diff --git a/multipathd/cli.h b/multipathd/cli.h
index fcb6af00..bba705e8 100644
--- a/multipathd/cli.h
+++ b/multipathd/cli.h
@@ -47,6 +47,7 @@ enum {
 	__LOCAL,			/* 40 */
 	__SETMARGINAL,
 	__UNSETMARGINAL,
+	__ALL,
 };
 
 #define LIST		(1ULL << __LIST)
@@ -93,6 +94,7 @@ enum {
 #define LOCAL		(1ULL << __LOCAL)
 #define SETMARGINAL	(1ULL << __SETMARGINAL)
 #define UNSETMARGINAL	(1ULL << __UNSETMARGINAL)
+#define ALL		(1ULL << __ALL)
 
 #define INITIAL_REPLY_LEN	1200
 
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index b12a4e7e..58b9916c 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1012,6 +1012,15 @@ cli_reconfigure(void * v, struct strbuf *reply, void * data)
 {
 	condlog(2, "reconfigure (operator)");
 
+	schedule_reconfigure(false);
+	return 0;
+}
+
+int
+cli_reconfigure_all(void * v, struct strbuf *reply, void * data)
+{
+	condlog(2, "reconfigure all (operator)");
+
 	schedule_reconfigure(true);
 	return 0;
 }
@@ -1497,6 +1506,7 @@ void init_handler_callbacks(void)
 	set_handler_callback(DEL+MAPS, cli_del_maps);
 	set_handler_callback(SWITCH+MAP+GROUP, cli_switch_group);
 	set_unlocked_handler_callback(RECONFIGURE, cli_reconfigure);
+	set_unlocked_handler_callback(RECONFIGURE+ALL, cli_reconfigure_all);
 	set_handler_callback(SUSPEND+MAP, cli_suspend);
 	set_handler_callback(RESUME+MAP, cli_resume);
 	set_handler_callback(RESIZE+MAP, cli_resize);
diff --git a/multipathd/main.c b/multipathd/main.c
index 5c831e8d..cc4a4a5d 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2817,7 +2817,7 @@ handle_signals(bool nonfatal)
 		return;
 	if (reconfig_sig) {
 		condlog(2, "reconfigure (signal)");
-		schedule_reconfigure(true);
+		schedule_reconfigure(false);
 	}
 	if (log_reset_sig) {
 		condlog(2, "reset log (signal)");
-- 
2.17.2

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

Re: [dm-devel] [PATCH 0/4] Add "reconfigure all" multipath command

[Xose Vazquez Perez]

On 9/21/21 01:21, Benjamin Marzinski wrote:

> This patchset is supposed to replace Martin's
> 
> multipathd: add "force_reconfigure" option
> 
> patch from his uxlsnr overhaul patchset. It also makes the default
> reconfigure be a weak reconfigure, but instead of adding a configuration
> option to control this, it adds a new multipathd command,
> "reconfigure all", to do a full reconfigure. The HUP signal is left
> doing only weak reconfigures.
>   
> In order to keep from having two states that are handled nearly
> identically, the code adds an extra variable to track the type of
> configuration that was selected, but this could easily be switch to
> use a new DAEMON_CONFIGURE_ALL state instead.
>   
> The final patch, that added the new command, is meant to apply on top of
> Martin's changed client handler code. I can send one that works with the
> current client handler code, if people would rather review that.

This change is going to affect some places, raw search:

$ git grep reconfigure
libdmmp/libdmmp.c:      snprintf(cmd, _IPC_MAX_CMD_LEN, "%s", "reconfigure");
libmultipath/configure.c:        * If we are in this code path (startup or forced reconfigure),
libmultipath/foreign.h:  * don't support it. "multipathd reconfigure" starts foreign device
libmultipath/foreign.h:  * This is called if multipathd reconfigures itself.
multipath/main.c:               p += snprintf(p, n, "reconfigure");
multipathd/cli.c:       r += add_key(keys, "reconfigure", RECONFIGURE, 0);
multipathd/cli_handlers.c:cli_reconfigure(void * v, char ** reply, int * len, void * data)
multipathd/cli_handlers.c:      condlog(2, "reconfigure (operator)");
multipathd/cli_handlers.h:int cli_reconfigure(void * v, char ** reply, int * len, void * data);
multipathd/main.c:                              condlog(2, "reconfigure (delayed)");
multipathd/main.c:      set_unlocked_handler_callback(RECONFIGURE, cli_reconfigure);
multipathd/main.c:              condlog(2, "reconfigure (delayed)");
multipathd/main.c:reconfigure (struct vectors * vecs)
multipathd/main.c:              condlog(2, "reconfigure (signal)");
multipathd/main.c:                              reconfigure(vecs);
multipathd/main.h:int reconfigure (struct vectors *);
multipathd/multipathd.8:happens, it will reconfigure the multipath map the path belongs to, so that this
multipathd/multipathd.8:.B reconfigure
multipathd/multipathd.service:ExecReload=/sbin/multipathd reconfigure
multipathd/uxlsnr.c:     * do it once per reconfigure */

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

Re: [dm-devel] [PATCH 0/4] Add "reconfigure all" multipath command

[Benjamin Marzinski]

On Fri, Sep 24, 2021 at 10:44:46PM +0200, Xose Vazquez Perez wrote:
> On 9/21/21 01:21, Benjamin Marzinski wrote:
> 
> >This patchset is supposed to replace Martin's
> >
> >multipathd: add "force_reconfigure" option
> >
> >patch from his uxlsnr overhaul patchset. It also makes the default
> >reconfigure be a weak reconfigure, but instead of adding a configuration
> >option to control this, it adds a new multipathd command,
> >"reconfigure all", to do a full reconfigure. The HUP signal is left
> >doing only weak reconfigures.
> >In order to keep from having two states that are handled nearly
> >identically, the code adds an extra variable to track the type of
> >configuration that was selected, but this could easily be switch to
> >use a new DAEMON_CONFIGURE_ALL state instead.
> >The final patch, that added the new command, is meant to apply on top of
> >Martin's changed client handler code. I can send one that works with the
> >current client handler code, if people would rather review that.
> 
> This change is going to affect some places, raw search:

Yes. I specifically broke the code that actually changes how multipathd
operates from a user' point of view into a seperate patch (4/4) because
distributions might need to revert in, if they want to pull in recent
upstream changes, but don't what this kind of change in multipathd's
behavior.

I admit, this patchset needs to include documentation to mention the
changed behavior. I'll add that.  But I'm not sure what to make of the
list below.  I don't see any code in it that I didn't think about.  We
can disagree as to whether, for instance, dmmp_reconfig() should do a
full or a weak reconfig. But without some more information, I'm not sure
what you are asking of me.

-Ben

> $ git grep reconfigure
> libdmmp/libdmmp.c:      snprintf(cmd, _IPC_MAX_CMD_LEN, "%s", "reconfigure");
> libmultipath/configure.c:        * If we are in this code path (startup or forced reconfigure),
> libmultipath/foreign.h:  * don't support it. "multipathd reconfigure" starts foreign device
> libmultipath/foreign.h:  * This is called if multipathd reconfigures itself.
> multipath/main.c:               p += snprintf(p, n, "reconfigure");
> multipathd/cli.c:       r += add_key(keys, "reconfigure", RECONFIGURE, 0);
> multipathd/cli_handlers.c:cli_reconfigure(void * v, char ** reply, int * len, void * data)
> multipathd/cli_handlers.c:      condlog(2, "reconfigure (operator)");
> multipathd/cli_handlers.h:int cli_reconfigure(void * v, char ** reply, int * len, void * data);
> multipathd/main.c:                              condlog(2, "reconfigure (delayed)");
> multipathd/main.c:      set_unlocked_handler_callback(RECONFIGURE, cli_reconfigure);
> multipathd/main.c:              condlog(2, "reconfigure (delayed)");
> multipathd/main.c:reconfigure (struct vectors * vecs)
> multipathd/main.c:              condlog(2, "reconfigure (signal)");
> multipathd/main.c:                              reconfigure(vecs);
> multipathd/main.h:int reconfigure (struct vectors *);
> multipathd/multipathd.8:happens, it will reconfigure the multipath map the path belongs to, so that this
> multipathd/multipathd.8:.B reconfigure
> multipathd/multipathd.service:ExecReload=/sbin/multipathd reconfigure
> multipathd/uxlsnr.c:     * do it once per reconfigure */

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

Re: [dm-devel] [PATCH 0/4] Add "reconfigure all" multipath command

[Martin Wilck]

On Mon, 2021-09-27 at 10:11 -0500, Benjamin Marzinski wrote:
> On Fri, Sep 24, 2021 at 10:44:46PM +0200, Xose Vazquez Perez wrote:
> > On 9/21/21 01:21, Benjamin Marzinski wrote:
> > 
> > > This patchset is supposed to replace Martin's
> > > 
> > > multipathd: add "force_reconfigure" option
> > > 
> > > patch from his uxlsnr overhaul patchset. It also makes the
> > > default
> > > reconfigure be a weak reconfigure, but instead of adding a
> > > configuration
> > > option to control this, it adds a new multipathd command,
> > > "reconfigure all", to do a full reconfigure. The HUP signal is
> > > left
> > > doing only weak reconfigures.
> > > In order to keep from having two states that are handled nearly
> > > identically, the code adds an extra variable to track the type of
> > > configuration that was selected, but this could easily be switch
> > > to
> > > use a new DAEMON_CONFIGURE_ALL state instead.
> > > The final patch, that added the new command, is meant to apply on
> > > top of
> > > Martin's changed client handler code. I can send one that works
> > > with the
> > > current client handler code, if people would rather review that.
> > 
> > This change is going to affect some places, raw search:
> 
> Yes. I specifically broke the code that actually changes how
> multipathd
> operates from a user' point of view into a seperate patch (4/4)
> because
> distributions might need to revert in, if they want to pull in recent
> upstream changes, but don't what this kind of change in multipathd's
> behavior.
> 
> I admit, this patchset needs to include documentation to mention the
> changed behavior. I'll add that. 

Well, the idea is that there is actually no difference between "weak"
and "hard" reconfigure in terms of the end result. If a change must be
applied to reconcile kernel state and user settings, "weak" reconfigure
will do it. The documentation should express that and avoid stipulating
doubt among users. The main difference is that "hard" reconfigure
always executes a reload operation, which comes down to a
suspend/reload/resume, and thus a) is slow and b) unnecessarily
interrupts IO on the map for a few fractions of a second.

My personal PoV is that we should consider it a bug if a user reports a
situation where a "hard" reconfigure has a different outcome than a
"weak" one. Of course distros need to think twice when any defaults
change, and therefore the way Ben split the patch set makes a lot of
sense. Yet if we had serious doubts that "weak" reconfigure works, we
shouldn't switch to it upstream, either. I personally don't have such
doubts any more. Xose, if I'm missing something, let me know.

Cheers,
Martin



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

Re: [dm-devel] [PATCH 1/4] multipathd: move delayed_reconfig out of struct config

[Martin Wilck]

On Mon, 2021-09-20 at 18:21 -0500, Benjamin Marzinski wrote:
> delayed_reconfig was inside the config struct, but it wasn't updated
> during an RCU write section, so there's no synchronization on it.
> Instead, pull it out of the config structure, and use the config_lock
> to synchronize it.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>


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

Re: [dm-devel] [PATCH 2/4] multipathd: remove reconfigure from header file.

[Martin Wilck]

On Mon, 2021-09-20 at 18:21 -0500, Benjamin Marzinski wrote:
> Only multipathd/main.c uses it.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>


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

Re: [dm-devel] [PATCH 3/4] multipathd: pass in the type of reconfigure

[Martin Wilck]

On Mon, 2021-09-20 at 18:21 -0500, Benjamin Marzinski wrote:
> This code doesn't actually change how multipathd reconfigures. It still
> does a weak reconfigure at the start, and full reconfigures later.

AFAICS it does change this, at least for the cases where reconfigure()
is called from ev_add_map() and missing_uev_wait_tick, where you call
schedule_reconfigure(false). The old code was using a static variable
and thus always doing a hard reconfigure when called more than once.

I'm find with this change (the old behavior was arguably wrong), but
the commit message should be clarified in this respect.

> But
> now schedule_reconfigure() takes the type of reconfigure to do, and
> that
> gets passed down to reconfigure(). If a full reconfigure has already
> been requested but not started, weak reconfigure requests will be
> upgraded. A future patch will enable users to control what kind of
> reconfigures happen.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmultipath/configure.c  |  2 +-
>  multipathd/cli_handlers.c |  2 +-
>  multipathd/main.c         | 62 ++++++++++++++++++++-------------------
>  multipathd/main.h         |  2 +-
>  4 files changed, 35 insertions(+), 33 deletions(-)
> 
> diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> index 7edb355b..eb8ec1bd 100644
> --- a/libmultipath/configure.c
> +++ b/libmultipath/configure.c
> @@ -1098,7 +1098,7 @@ out:
>   * FORCE_RELOAD_NONE: existing maps aren't touched at all
>   * FORCE_RELOAD_YES: all maps are rebuilt from scratch and (re)loaded
> in DM
>   * FORCE_RELOAD_WEAK: existing maps are compared to the current conf
> and only
> - * reloaded in DM if there's a difference. This is useful during
> startup.
> + * reloaded in DM if there's a difference. This is normally
> sufficient.
>   */
>  int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
>                     int force_reload, enum mpath_cmds cmd)
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index f59db3ab..b12a4e7e 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -1012,7 +1012,7 @@ cli_reconfigure(void * v, struct strbuf *reply,
> void * data)
>  {
>         condlog(2, "reconfigure (operator)");
>  
> -       schedule_reconfigure();
> +       schedule_reconfigure(true);
>         return 0;
>  }
>  
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 1ead0904..5c831e8d 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -162,13 +162,6 @@ static bool get_delayed_reconfig(void)
>         return val;
>  }
>  
> -static void set_delayed_reconfig(bool val)
> -{
> -       pthread_mutex_lock(&config_lock);
> -       __delayed_reconfig = val;
> -       pthread_mutex_unlock(&config_lock);
> -}
> -
>  /*
>   * global copy of vecs for use in sig handlers
>   */
> @@ -290,7 +283,18 @@ enum daemon_status wait_for_state_change_if(enum
> daemon_status oldstate,
>  }
>  
>  /* Don't access this variable without holding config_lock */
> -static bool reconfigure_pending;
> +static enum force_reload_types reconfigure_pending =
> FORCE_RELOAD_NONE;
> +/* Only set while changing to DAEMON_CONFIGURE, and only access while
> + * reconfiguring in DAEMON_CONFIGURE */
> +static volatile enum force_reload_types reload_type =
> FORCE_RELOAD_NONE;
> +
> +static void enable_delayed_reconfig(enum force_reload_types type)
> +{
> +       pthread_mutex_lock(&config_lock);
> +       reconfigure_pending = type;
> +       __delayed_reconfig = true;
> +       pthread_mutex_unlock(&config_lock);
> +}
>  
>  /* must be called with config_lock held */
>  static void __post_config_state(enum daemon_status state)
> @@ -305,7 +309,8 @@ static void __post_config_state(enum daemon_status
> state)
>                  * In either case, child() will see DAEMON_CONFIGURE
>                  * again and start another reconfigure cycle.
>                  */
> -               if (reconfigure_pending && state == DAEMON_IDLE &&
> +               if (reconfigure_pending != FORCE_RELOAD_NONE &&
> +                   state == DAEMON_IDLE &&
>                     (old_state == DAEMON_CONFIGURE ||
>                      old_state == DAEMON_RUNNING)) {
>                         /*
> @@ -317,7 +322,8 @@ static void __post_config_state(enum daemon_status
> state)
>                         state = DAEMON_CONFIGURE;
>                 }
>                 if (state == DAEMON_CONFIGURE) {
> -                       reconfigure_pending = false;
> +                       reload_type = (reconfigure_pending ==
> FORCE_RELOAD_YES) ? FORCE_RELOAD_YES : FORCE_RELOAD_WEAK;
> +                       reconfigure_pending = FORCE_RELOAD_NONE;
>                         __delayed_reconfig = false;
>                 }
>                 running_state = state;
> @@ -334,20 +340,25 @@ void post_config_state(enum daemon_status state)
>         pthread_cleanup_pop(1);
>  }
>  
> -void schedule_reconfigure(void)
> +void schedule_reconfigure(bool force_reload_yes)
>  {
>         pthread_mutex_lock(&config_lock);
>         pthread_cleanup_push(config_cleanup, NULL);
> +       enum force_reload_types type;
> +
> +       type = (reconfigure_pending == FORCE_RELOAD_YES ||
> force_reload_yes) ?
> +              FORCE_RELOAD_YES : FORCE_RELOAD_WEAK;
>         switch (running_state)
>         {
>         case DAEMON_SHUTDOWN:
>                 break;
>         case DAEMON_IDLE:
> +               reconfigure_pending = type;
>                 __post_config_state(DAEMON_CONFIGURE);
>                 break;
>         case DAEMON_CONFIGURE:
>         case DAEMON_RUNNING:
> -               reconfigure_pending = true;
> +               reconfigure_pending = type;
>                 break;
>         default:
>                 break;
> @@ -812,7 +823,7 @@ ev_add_map (char * dev, const char * alias, struct
> vectors * vecs)
>                         if (get_delayed_reconfig() &&
>                             !need_to_delay_reconfig(vecs)) {
>                                 condlog(2, "reconfigure (delayed)");
> -                               schedule_reconfigure();
> +                               schedule_reconfigure(false);
>                                 return 0;
>                         }
>                 }
> @@ -1830,7 +1841,7 @@ missing_uev_wait_tick(struct vectors *vecs)
>         if (timed_out && get_delayed_reconfig() &&
>             !need_to_delay_reconfig(vecs)) {
>                 condlog(2, "reconfigure (delayed)");
> -               schedule_reconfigure();
> +               schedule_reconfigure(false);
>         }
>  }
>  
> @@ -2588,14 +2599,13 @@ checkerloop (void *ap)
>  }
>  
>  int
> -configure (struct vectors * vecs)
> +configure (struct vectors * vecs, enum force_reload_types type)
>  {
>         struct multipath * mpp;
>         struct path * pp;
>         vector mpvec;
>         int i, ret;
>         struct config *conf;
> -       static int force_reload = FORCE_RELOAD_WEAK;
>  
>         if (!vecs->pathvec && !(vecs->pathvec = vector_alloc())) {
>                 condlog(0, "couldn't allocate path vec in configure");
> @@ -2643,15 +2653,7 @@ configure (struct vectors * vecs)
>         if (should_exit())
>                 goto fail;
>  
> -       /*
> -        * create new set of maps & push changed ones into dm
> -        * In the first call, use FORCE_RELOAD_WEAK to avoid making
> -        * superfluous ACT_RELOAD ioctls. Later calls are done
> -        * with FORCE_RELOAD_YES.
> -        */
> -       ret = coalesce_paths(vecs, mpvec, NULL, force_reload,
> CMD_NONE);
> -       if (force_reload == FORCE_RELOAD_WEAK)
> -               force_reload = FORCE_RELOAD_YES;
> +       ret = coalesce_paths(vecs, mpvec, NULL, type, CMD_NONE);


Can we please rename this variable from "type" to "reload_type" here,
for the sake of readability? Likewise, in reconfigure()?

>         if (ret != CP_OK) {
>                 condlog(0, "configure failed while coalescing paths");
>                 goto fail;
> @@ -2729,7 +2731,7 @@ void rcu_free_config(struct rcu_head *head)
>  }
>  
>  int
> -reconfigure (struct vectors * vecs)
> +reconfigure (struct vectors * vecs, enum force_reload_types type)
>  {
>         struct config * old, *conf;
>  
> @@ -2763,7 +2765,7 @@ reconfigure (struct vectors * vecs)
>         rcu_assign_pointer(multipath_conf, conf);
>         call_rcu(&old->rcu, rcu_free_config);
>  
> -       configure(vecs);
> +       configure(vecs, type);
>  
>  
>         return 0;
> @@ -2815,7 +2817,7 @@ handle_signals(bool nonfatal)
>                 return;
>         if (reconfig_sig) {
>                 condlog(2, "reconfigure (signal)");
> -               schedule_reconfigure();
> +               schedule_reconfigure(true);
>         }
>         if (log_reset_sig) {
>                 condlog(2, "reset log (signal)");
> @@ -3274,9 +3276,9 @@ child (__attribute__((unused)) void *param)
>                         lock(&vecs->lock);
>                         pthread_testcancel();
>                         if (!need_to_delay_reconfig(vecs))
> -                               reconfigure(vecs);
> +                               reconfigure(vecs, reload_type);
>                         else
> -                               set_delayed_reconfig(true);
> +                               enable_delayed_reconfig(reload_type);
>                         lock_cleanup_pop(vecs->lock);
>                         post_config_state(DAEMON_IDLE);
>                 }
> diff --git a/multipathd/main.h b/multipathd/main.h
> index a1697a74..c8a1ce92 100644
> --- a/multipathd/main.h
> +++ b/multipathd/main.h
> @@ -37,7 +37,7 @@ void exit_daemon(void);
>  const char * daemon_status(void);
>  enum daemon_status wait_for_state_change_if(enum daemon_status
> oldstate,
>                                             unsigned long ms);
> -void schedule_reconfigure(void);
> +void schedule_reconfigure(bool reconfig_all);

For consistency, please consider using an "enum force_reload_types" as
argument to schedule_reconfigure() rather than a boolean.

Otherwise, the patch looks good.

Cheers
Martin


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

Re: [dm-devel] [PATCH 1/4] multipathd: move delayed_reconfig out of struct config

[Martin Wilck]

On Mon, 2021-11-15 at 17:10 +0100, Martin Wilck wrote:
> On Mon, 2021-09-20 at 18:21 -0500, Benjamin Marzinski wrote:
> > delayed_reconfig was inside the config struct, but it wasn't updated
> > during an RCU write section, so there's no synchronization on it.
> > Instead, pull it out of the config structure, and use the config_lock
> > to synchronize it.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> 
> Reviewed-by: Martin Wilck <mwilck@suse.com>
> 

I forgot: this patch changes the ABI of libmpathpersist and
libmultipath. Not in a bad way, because "struct config" is opaque to
users of libmpathpersist, but for staying consistent, we should adapt
the library versions.

See https://github.com/openSUSE/multipath-tools/actions/runs/1455114454
(open the "abi-test" artifact to check the details).

Regards
Martin




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

Re: [dm-devel] [PATCH 4/4] multipathd: add "reconfigure all" command.

[Martin Wilck]

On Mon, 2021-09-20 at 18:21 -0500, Benjamin Marzinski wrote:
> With this commit, multipathd no longer defaults to full reconfigures
> for
> the "reconfigure" command and the HUP signal. The default is a weak
> reconfigure. A new command, "reconfigure all", has been added to do
> a full reconfigure.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Reviewed-by: Martin Wilck <mwilck@suse.com>



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

Re: [dm-devel] [PATCH 1/4] multipathd: move delayed_reconfig out of struct config

[Benjamin Marzinski]

On Mon, Nov 15, 2021 at 04:27:40PM +0000, Martin Wilck wrote:
> On Mon, 2021-11-15 at 17:10 +0100, Martin Wilck wrote:
> > On Mon, 2021-09-20 at 18:21 -0500, Benjamin Marzinski wrote:
> > > delayed_reconfig was inside the config struct, but it wasn't updated
> > > during an RCU write section, so there's no synchronization on it.
> > > Instead, pull it out of the config structure, and use the config_lock
> > > to synchronize it.
> > > 
> > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > 
> > Reviewed-by: Martin Wilck <mwilck@suse.com>
> > 
> 
> I forgot: this patch changes the ABI of libmpathpersist and
> libmultipath. Not in a bad way, because "struct config" is opaque to
> users of libmpathpersist, but for staying consistent, we should adapt
> the library versions.

Oops. Yeah, I can bump the version.

-Ben
 
> See https://github.com/openSUSE/multipath-tools/actions/runs/1455114454
> (open the "abi-test" artifact to check the details).
> 
> Regards
> Martin
> 
> 

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

Re: [dm-devel] [PATCH 3/4] multipathd: pass in the type of reconfigure

[Benjamin Marzinski]

On Mon, Nov 15, 2021 at 04:23:54PM +0000, Martin Wilck wrote:
> On Mon, 2021-09-20 at 18:21 -0500, Benjamin Marzinski wrote:
> > This code doesn't actually change how multipathd reconfigures. It still
> > does a weak reconfigure at the start, and full reconfigures later.
> 
> AFAICS it does change this, at least for the cases where reconfigure()
> is called from ev_add_map() and missing_uev_wait_tick, where you call
> schedule_reconfigure(false). The old code was using a static variable
> and thus always doing a hard reconfigure when called more than once.
> 
> I'm find with this change (the old behavior was arguably wrong), but
> the commit message should be clarified in this respect.

Sure.

-Ben

> 
> > But
> > now schedule_reconfigure() takes the type of reconfigure to do, and
> > that
> > gets passed down to reconfigure(). If a full reconfigure has already
> > been requested but not started, weak reconfigure requests will be
> > upgraded. A future patch will enable users to control what kind of
> > reconfigures happen.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmultipath/configure.c  |  2 +-
> >  multipathd/cli_handlers.c |  2 +-
> >  multipathd/main.c         | 62 ++++++++++++++++++++-------------------
> >  multipathd/main.h         |  2 +-
> >  4 files changed, 35 insertions(+), 33 deletions(-)
> > 
> > diff --git a/libmultipath/configure.c b/libmultipath/configure.c
> > index 7edb355b..eb8ec1bd 100644
> > --- a/libmultipath/configure.c
> > +++ b/libmultipath/configure.c
> > @@ -1098,7 +1098,7 @@ out:
> >   * FORCE_RELOAD_NONE: existing maps aren't touched at all
> >   * FORCE_RELOAD_YES: all maps are rebuilt from scratch and (re)loaded
> > in DM
> >   * FORCE_RELOAD_WEAK: existing maps are compared to the current conf
> > and only
> > - * reloaded in DM if there's a difference. This is useful during
> > startup.
> > + * reloaded in DM if there's a difference. This is normally
> > sufficient.
> >   */
> >  int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
> >                     int force_reload, enum mpath_cmds cmd)
> > diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> > index f59db3ab..b12a4e7e 100644
> > --- a/multipathd/cli_handlers.c
> > +++ b/multipathd/cli_handlers.c
> > @@ -1012,7 +1012,7 @@ cli_reconfigure(void * v, struct strbuf *reply,
> > void * data)
> >  {
> >         condlog(2, "reconfigure (operator)");
> >  
> > -       schedule_reconfigure();
> > +       schedule_reconfigure(true);
> >         return 0;
> >  }
> >  
> > diff --git a/multipathd/main.c b/multipathd/main.c
> > index 1ead0904..5c831e8d 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -162,13 +162,6 @@ static bool get_delayed_reconfig(void)
> >         return val;
> >  }
> >  
> > -static void set_delayed_reconfig(bool val)
> > -{
> > -       pthread_mutex_lock(&config_lock);
> > -       __delayed_reconfig = val;
> > -       pthread_mutex_unlock(&config_lock);
> > -}
> > -
> >  /*
> >   * global copy of vecs for use in sig handlers
> >   */
> > @@ -290,7 +283,18 @@ enum daemon_status wait_for_state_change_if(enum
> > daemon_status oldstate,
> >  }
> >  
> >  /* Don't access this variable without holding config_lock */
> > -static bool reconfigure_pending;
> > +static enum force_reload_types reconfigure_pending =
> > FORCE_RELOAD_NONE;
> > +/* Only set while changing to DAEMON_CONFIGURE, and only access while
> > + * reconfiguring in DAEMON_CONFIGURE */
> > +static volatile enum force_reload_types reload_type =
> > FORCE_RELOAD_NONE;
> > +
> > +static void enable_delayed_reconfig(enum force_reload_types type)
> > +{
> > +       pthread_mutex_lock(&config_lock);
> > +       reconfigure_pending = type;
> > +       __delayed_reconfig = true;
> > +       pthread_mutex_unlock(&config_lock);
> > +}
> >  
> >  /* must be called with config_lock held */
> >  static void __post_config_state(enum daemon_status state)
> > @@ -305,7 +309,8 @@ static void __post_config_state(enum daemon_status
> > state)
> >                  * In either case, child() will see DAEMON_CONFIGURE
> >                  * again and start another reconfigure cycle.
> >                  */
> > -               if (reconfigure_pending && state == DAEMON_IDLE &&
> > +               if (reconfigure_pending != FORCE_RELOAD_NONE &&
> > +                   state == DAEMON_IDLE &&
> >                     (old_state == DAEMON_CONFIGURE ||
> >                      old_state == DAEMON_RUNNING)) {
> >                         /*
> > @@ -317,7 +322,8 @@ static void __post_config_state(enum daemon_status
> > state)
> >                         state = DAEMON_CONFIGURE;
> >                 }
> >                 if (state == DAEMON_CONFIGURE) {
> > -                       reconfigure_pending = false;
> > +                       reload_type = (reconfigure_pending ==
> > FORCE_RELOAD_YES) ? FORCE_RELOAD_YES : FORCE_RELOAD_WEAK;
> > +                       reconfigure_pending = FORCE_RELOAD_NONE;
> >                         __delayed_reconfig = false;
> >                 }
> >                 running_state = state;
> > @@ -334,20 +340,25 @@ void post_config_state(enum daemon_status state)
> >         pthread_cleanup_pop(1);
> >  }
> >  
> > -void schedule_reconfigure(void)
> > +void schedule_reconfigure(bool force_reload_yes)
> >  {
> >         pthread_mutex_lock(&config_lock);
> >         pthread_cleanup_push(config_cleanup, NULL);
> > +       enum force_reload_types type;
> > +
> > +       type = (reconfigure_pending == FORCE_RELOAD_YES ||
> > force_reload_yes) ?
> > +              FORCE_RELOAD_YES : FORCE_RELOAD_WEAK;
> >         switch (running_state)
> >         {
> >         case DAEMON_SHUTDOWN:
> >                 break;
> >         case DAEMON_IDLE:
> > +               reconfigure_pending = type;
> >                 __post_config_state(DAEMON_CONFIGURE);
> >                 break;
> >         case DAEMON_CONFIGURE:
> >         case DAEMON_RUNNING:
> > -               reconfigure_pending = true;
> > +               reconfigure_pending = type;
> >                 break;
> >         default:
> >                 break;
> > @@ -812,7 +823,7 @@ ev_add_map (char * dev, const char * alias, struct
> > vectors * vecs)
> >                         if (get_delayed_reconfig() &&
> >                             !need_to_delay_reconfig(vecs)) {
> >                                 condlog(2, "reconfigure (delayed)");
> > -                               schedule_reconfigure();
> > +                               schedule_reconfigure(false);
> >                                 return 0;
> >                         }
> >                 }
> > @@ -1830,7 +1841,7 @@ missing_uev_wait_tick(struct vectors *vecs)
> >         if (timed_out && get_delayed_reconfig() &&
> >             !need_to_delay_reconfig(vecs)) {
> >                 condlog(2, "reconfigure (delayed)");
> > -               schedule_reconfigure();
> > +               schedule_reconfigure(false);
> >         }
> >  }
> >  
> > @@ -2588,14 +2599,13 @@ checkerloop (void *ap)
> >  }
> >  
> >  int
> > -configure (struct vectors * vecs)
> > +configure (struct vectors * vecs, enum force_reload_types type)
> >  {
> >         struct multipath * mpp;
> >         struct path * pp;
> >         vector mpvec;
> >         int i, ret;
> >         struct config *conf;
> > -       static int force_reload = FORCE_RELOAD_WEAK;
> >  
> >         if (!vecs->pathvec && !(vecs->pathvec = vector_alloc())) {
> >                 condlog(0, "couldn't allocate path vec in configure");
> > @@ -2643,15 +2653,7 @@ configure (struct vectors * vecs)
> >         if (should_exit())
> >                 goto fail;
> >  
> > -       /*
> > -        * create new set of maps & push changed ones into dm
> > -        * In the first call, use FORCE_RELOAD_WEAK to avoid making
> > -        * superfluous ACT_RELOAD ioctls. Later calls are done
> > -        * with FORCE_RELOAD_YES.
> > -        */
> > -       ret = coalesce_paths(vecs, mpvec, NULL, force_reload,
> > CMD_NONE);
> > -       if (force_reload == FORCE_RELOAD_WEAK)
> > -               force_reload = FORCE_RELOAD_YES;
> > +       ret = coalesce_paths(vecs, mpvec, NULL, type, CMD_NONE);
> 
> 
> Can we please rename this variable from "type" to "reload_type" here,
> for the sake of readability? Likewise, in reconfigure()?
> 
> >         if (ret != CP_OK) {
> >                 condlog(0, "configure failed while coalescing paths");
> >                 goto fail;
> > @@ -2729,7 +2731,7 @@ void rcu_free_config(struct rcu_head *head)
> >  }
> >  
> >  int
> > -reconfigure (struct vectors * vecs)
> > +reconfigure (struct vectors * vecs, enum force_reload_types type)
> >  {
> >         struct config * old, *conf;
> >  
> > @@ -2763,7 +2765,7 @@ reconfigure (struct vectors * vecs)
> >         rcu_assign_pointer(multipath_conf, conf);
> >         call_rcu(&old->rcu, rcu_free_config);
> >  
> > -       configure(vecs);
> > +       configure(vecs, type);
> >  
> >  
> >         return 0;
> > @@ -2815,7 +2817,7 @@ handle_signals(bool nonfatal)
> >                 return;
> >         if (reconfig_sig) {
> >                 condlog(2, "reconfigure (signal)");
> > -               schedule_reconfigure();
> > +               schedule_reconfigure(true);
> >         }
> >         if (log_reset_sig) {
> >                 condlog(2, "reset log (signal)");
> > @@ -3274,9 +3276,9 @@ child (__attribute__((unused)) void *param)
> >                         lock(&vecs->lock);
> >                         pthread_testcancel();
> >                         if (!need_to_delay_reconfig(vecs))
> > -                               reconfigure(vecs);
> > +                               reconfigure(vecs, reload_type);
> >                         else
> > -                               set_delayed_reconfig(true);
> > +                               enable_delayed_reconfig(reload_type);
> >                         lock_cleanup_pop(vecs->lock);
> >                         post_config_state(DAEMON_IDLE);
> >                 }
> > diff --git a/multipathd/main.h b/multipathd/main.h
> > index a1697a74..c8a1ce92 100644
> > --- a/multipathd/main.h
> > +++ b/multipathd/main.h
> > @@ -37,7 +37,7 @@ void exit_daemon(void);
> >  const char * daemon_status(void);
> >  enum daemon_status wait_for_state_change_if(enum daemon_status
> > oldstate,
> >                                             unsigned long ms);
> > -void schedule_reconfigure(void);
> > +void schedule_reconfigure(bool reconfig_all);
> 
> For consistency, please consider using an "enum force_reload_types" as
> argument to schedule_reconfigure() rather than a boolean.
> 
> Otherwise, the patch looks good.
> 
> Cheers
> Martin

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