[PATCH v3 0/5] fix set_no_path_retry and cleanup commands

[PATCH v3 0/5] fix set_no_path_retry and cleanup commands

[Benjamin Marzinski]

This patchset combines my patch to disable queueing in set_no_path_retry
when recovery mode has timed out with my pathset to cleanup the
interactive commands. This has been significantly changed in v3

Differences from v2 (based on suggestions by Martin Wilck):
0001: This completely new patch that Martin suggested. Martin, I've
      made you the Author. Feel free to change my commit message if
      you'd like.
0002: This is similar to patch 0001 from v2, except the rational has
      changed. It is still possible to get in a situation where a
      multipath device is queueing after timeout out in recovery, so we
      still need to handle this. Since we won't have to worry about
      multipathd flip-flopping the queueing state on reloads, I pulled
      this check outside the PATH_PENDING check, as Martin suggested.
0003: This is the same as patches 0002-0004 from v2. It made no sense
      to merge 0002 and 0004 but not 0003. In would just result in
      making changes only to undo them in the next patch.
0004: This is instead of my 0005 patch from v2. In order for
      disabling and restoring queuing to always work correctly,
      multipathd needs to track the current queueing state.
0005: This is a new patch based on some of Martin's comments on
      v2 patch 0005.

Differences from v1 (based on suggestions by Martin Wilck):
0001: added comments explaining why cleaning up the queueing state is
      unnecessary in update_queue_mode_{add|del}_path().
patches 0002-0005 were previously 0001-0004

Benjamin Marzinski (4):
  multipathd: Make sure to disable queueing if recovery has failed.
  multipathd: don't modify the multipath device on show commands
  libmultipath: keep track of queueing state in features
  multipathd: only restore queueing if it has been disabled first

Martin Wilck (1):
  libmultipath: avoid temporarily enabling queueing on reload

 libmultipath/configure.c   |  4 +--
 libmultipath/devmapper.c   | 23 +++++++++++++++---
 libmultipath/devmapper.h   |  2 +-
 libmultipath/dmparser.c    | 14 +++++++++--
 libmultipath/structs_vec.c | 34 +++++++++++++++++++++-----
 multipathd/cli_handlers.c  | 50 +++++++++++++++++---------------------
 multipathd/dmevents.c      |  2 +-
 multipathd/main.c          | 33 +++++++++++++------------
 multipathd/main.h          |  7 +++---
 multipathd/multipathd.8.in | 12 +++++++--
 multipathd/waiter.c        |  2 +-
 11 files changed, 116 insertions(+), 67 deletions(-)

-- 
2.41.0

[PATCH v3 1/5] libmultipath: avoid temporarily enabling queueing on reload

[Benjamin Marzinski]

From: Martin Wilck <mwilck@suse.com>

Instead of always enabling queueing when a map is reloaded with
no_path_retry set to a positive number, check if the map has timed out
in recovery mode, and only enable queueing if it has not. This saves
multipathd from having to disable queueing on the map immediately after
the reload.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/dmparser.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/libmultipath/dmparser.c b/libmultipath/dmparser.c
index 3b37a926..3d85e6ee 100644
--- a/libmultipath/dmparser.c
+++ b/libmultipath/dmparser.c
@@ -60,9 +60,19 @@ int assemble_map(struct multipath *mp, char **params)
 	nr_priority_groups = VECTOR_SIZE(mp->pg);
 	initial_pg_nr = (nr_priority_groups ? mp->bestpg : 0);
 
-	if (mp->no_path_retry != NO_PATH_RETRY_UNDEF  &&
-	    mp->no_path_retry != NO_PATH_RETRY_FAIL) {
+	switch (mp->no_path_retry) {
+	case NO_PATH_RETRY_UNDEF:
+	case NO_PATH_RETRY_FAIL:
+		break;
+	default:
+		/* don't enable queueing if no_path_retry has timed out */
+		if (mp->in_recovery && mp->retry_tick == 0 &&
+		    count_active_paths(mp) == 0)
+			break;
+		/* fallthrough */
+	case NO_PATH_RETRY_QUEUE:
 		add_feature(&mp->features, no_path_retry);
+		break;
 	}
 	if (mp->retain_hwhandler == RETAIN_HWHANDLER_ON &&
 	    get_linux_version_code() < KERNEL_VERSION(4, 3, 0))
-- 
2.41.0

[PATCH v3 2/5] multipathd: Make sure to disable queueing if recovery has failed.

[Benjamin Marzinski]

If a multipath device has no_path_retry set to a number and has lost all
paths, gone into recovery mode, and timed out, it will disable
queue_if_no_paths. After that, if the device is reloaded by multipath
outside of multipathd, it will re-enable queuieng on the device. When
multipathd later calls set_no_path_retry() to update the queueing state,
it will not disable queue_if_no_paths, since the device is still in the
recovery state, so it believes no work needs to be done. The device will
remain in the recovery state, with retry_ticks at 0, and queueing
enabled, even though there are no usable paths.

To fix this, in set_no_path_retry(), if no_path_retry is set to a number
and the device is queueing but it is in recovery mode and out of
retries with no usable paths, manually disable queue_if_no_path.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs_vec.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 0e8a46e7..66bc264c 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -627,8 +627,19 @@ void set_no_path_retry(struct multipath *mpp)
 			    !mpp->in_recovery)
 				dm_queue_if_no_path(mpp->alias, 1);
 			leave_recovery_mode(mpp);
-		} else if (pathcount(mpp, PATH_PENDING) == 0)
-			enter_recovery_mode(mpp);
+		} else {
+			/*
+			 * If in_recovery is set, enter_recovery_mode does
+			 * nothing. If the device is already in recovery
+			 * mode and has already timed out, manually call
+			 * dm_queue_if_no_path to stop it from queueing.
+			 */
+			if ((!mpp->features || is_queueing) &&
+			    mpp->in_recovery && mpp->retry_tick == 0)
+				dm_queue_if_no_path(mpp->alias, 0);
+			if (pathcount(mpp, PATH_PENDING) == 0)
+				enter_recovery_mode(mpp);
+		}
 		break;
 	}
 }
@@ -774,6 +785,11 @@ int verify_paths(struct multipath *mpp)
  *   -1 (FAIL)  : fail_if_no_path
  *    0 (UNDEF) : nothing
  *   >0         : queue_if_no_path enabled, turned off after polling n times
+ *
+ * Since this will only be called when fail_path(), update_multipath(), or
+ * io_err_stat_handle_pathfail() are failing a previously active path, the
+ * device cannot already be in recovery mode, so there will never be a need
+ * to disable queueing here.
  */
 void update_queue_mode_del_path(struct multipath *mpp)
 {
@@ -787,6 +803,12 @@ void update_queue_mode_del_path(struct multipath *mpp)
 	condlog(2, "%s: remaining active paths: %d", mpp->alias, active);
 }
 
+/*
+ * Since this will only be called from check_path() -> reinstate_path() after
+ * the queueing state has been updated in set_no_path_retry, this does not
+ * need to worry about modifying the queueing state except when actually
+ * leaving recovery mode.
+ */
 void update_queue_mode_add_path(struct multipath *mpp)
 {
 	int active = count_active_paths(mpp);
-- 
2.41.0

[PATCH v3 3/5] multipathd: don't modify the multipath device on show commands

[Benjamin Marzinski]

The commands to show a multipath device are supposed to return its
current state without updating it. Even when reset is 0,
update_multipath() still can update the device.

To fix this, split __setup_multipath() into two functions:
refresh_multipath(), that updates the table and status, and
setup_multipath(), which works as before but now calls
refresh_multipath(). Make the multipathd show commands call
refresh_multipath() instead of update_multipath().

With the show commands calling refresh_multipath(), all callers of
update_multipath() set the reset argument, so remove it and always call
setup_multipath() from update_multipath().

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/cli_handlers.c | 10 +++++-----
 multipathd/dmevents.c     |  2 +-
 multipathd/main.c         | 25 ++++++++++++++-----------
 multipathd/main.h         |  7 +++----
 multipathd/waiter.c       |  2 +-
 5 files changed, 24 insertions(+), 22 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 420d75df..b1dff202 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -76,7 +76,7 @@ static int
 show_map_topology (struct strbuf *reply, struct multipath *mpp,
 		   struct vectors *vecs, const fieldwidth_t *width)
 {
-	if (update_multipath(vecs, mpp->alias, 0))
+	if (refresh_multipath(vecs, mpp))
 		return 1;
 
 	if (snprint_multipath_topology(reply, mpp, 2, width) < 0)
@@ -98,7 +98,7 @@ show_maps_topology (struct strbuf *reply, struct vectors * vecs)
 	foreign_path_layout(p_width);
 
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
-		if (update_multipath(vecs, mpp->alias, 0)) {
+		if (refresh_multipath(vecs, mpp)) {
 			i--;
 			continue;
 		}
@@ -118,7 +118,7 @@ show_maps_json (struct strbuf *reply, struct vectors * vecs)
 	struct multipath * mpp;
 
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
-		if (update_multipath(vecs, mpp->alias, 0)) {
+		if (refresh_multipath(vecs, mpp)) {
 			return 1;
 		}
 	}
@@ -133,7 +133,7 @@ static int
 show_map_json (struct strbuf *reply, struct multipath * mpp,
 	       struct vectors * vecs)
 {
-	if (update_multipath(vecs, mpp->alias, 0))
+	if (refresh_multipath(vecs, mpp))
 		return 1;
 
 	if (snprint_multipath_map_json(reply, mpp) < 0)
@@ -365,7 +365,7 @@ show_maps (struct strbuf *reply, struct vectors *vecs, char *style,
 		return 1;
 
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
-		if (update_multipath(vecs, mpp->alias, 0)) {
+		if (refresh_multipath(vecs, mpp)) {
 			i--;
 			continue;
 		}
diff --git a/multipathd/dmevents.c b/multipathd/dmevents.c
index 3a859691..5657000c 100644
--- a/multipathd/dmevents.c
+++ b/multipathd/dmevents.c
@@ -361,7 +361,7 @@ static int dmevent_loop (void)
 		if (curr_dev.action == EVENT_REMOVE)
 			remove_map_by_alias(curr_dev.name, waiter->vecs);
 		else
-			r = update_multipath(waiter->vecs, curr_dev.name, 1);
+			r = update_multipath(waiter->vecs, curr_dev.name);
 		pthread_cleanup_pop(1);
 
 		if (r) {
diff --git a/multipathd/main.c b/multipathd/main.c
index 230c9d10..c7476ff0 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -500,8 +500,7 @@ remove_maps_and_stop_waiters(struct vectors *vecs)
 	remove_maps(vecs);
 }
 
-int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
-		      int reset)
+int refresh_multipath(struct vectors *vecs, struct multipath *mpp)
 {
 	if (dm_get_info(mpp->alias, &mpp->dmi)) {
 		/* Error accessing table */
@@ -513,20 +512,24 @@ int __setup_multipath(struct vectors *vecs, struct multipath *mpp,
 		condlog(0, "%s: failed to setup multipath", mpp->alias);
 		goto out;
 	}
-
-	if (reset) {
-		set_no_path_retry(mpp);
-		if (VECTOR_SIZE(mpp->paths) != 0)
-			dm_cancel_deferred_remove(mpp);
-	}
-
 	return 0;
 out:
 	remove_map_and_stop_waiter(mpp, vecs);
 	return 1;
 }
 
-int update_multipath (struct vectors *vecs, char *mapname, int reset)
+int setup_multipath(struct vectors *vecs, struct multipath *mpp)
+{
+	if (refresh_multipath(vecs, mpp) != 0)
+		return 1;
+
+	set_no_path_retry(mpp);
+	if (VECTOR_SIZE(mpp->paths) != 0)
+		dm_cancel_deferred_remove(mpp);
+	return 0;
+}
+
+int update_multipath (struct vectors *vecs, char *mapname)
 {
 	struct multipath *mpp;
 	struct pathgroup  *pgp;
@@ -540,7 +543,7 @@ int update_multipath (struct vectors *vecs, char *mapname, int reset)
 		return 2;
 	}
 
-	if (__setup_multipath(vecs, mpp, reset))
+	if (setup_multipath(vecs, mpp))
 		return 1; /* mpp freed in setup_multipath */
 
 	/*
diff --git a/multipathd/main.h b/multipathd/main.h
index 8a178c0b..194f8776 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -43,10 +43,9 @@ int ev_remove_map (char *, char *, int, struct vectors *);
 int flush_map(struct multipath *, struct vectors *, int);
 
 void handle_signals(bool);
-int __setup_multipath (struct vectors * vecs, struct multipath * mpp,
-		       int reset);
-#define setup_multipath(vecs, mpp) __setup_multipath(vecs, mpp, 1)
-int update_multipath (struct vectors *vecs, char *mapname, int reset);
+int refresh_multipath(struct vectors * vecs, struct multipath * mpp);
+int setup_multipath(struct vectors * vecs, struct multipath * mpp);
+int update_multipath(struct vectors *vecs, char *mapname);
 int reload_and_sync_map(struct multipath *mpp, struct vectors *vecs);
 
 void handle_path_wwid_change(struct path *pp, struct vectors *vecs);
diff --git a/multipathd/waiter.c b/multipathd/waiter.c
index 52793698..d1f344b6 100644
--- a/multipathd/waiter.c
+++ b/multipathd/waiter.c
@@ -156,7 +156,7 @@ static int waiteventloop (struct event_thread *waiter)
 		pthread_cleanup_push(cleanup_lock, &waiter->vecs->lock);
 		lock(&waiter->vecs->lock);
 		pthread_testcancel();
-		r = update_multipath(waiter->vecs, waiter->mapname, 1);
+		r = update_multipath(waiter->vecs, waiter->mapname);
 		lock_cleanup_pop(waiter->vecs->lock);
 
 		if (r) {
-- 
2.41.0

[PATCH v3 4/5] libmultipath: keep track of queueing state in features

[Benjamin Marzinski]

Make multipathd update mpp->features when in enables or disables
queuing. This patch handles all the cases except failed removes by
dm_suspend_and_flush_map(), which is never called by multipathd.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/configure.c   |  4 +---
 libmultipath/devmapper.c   | 23 +++++++++++++++++++----
 libmultipath/devmapper.h   |  2 +-
 libmultipath/structs_vec.c | 10 +++++-----
 multipathd/main.c          |  8 ++++----
 5 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/libmultipath/configure.c b/libmultipath/configure.c
index d8094903..f6813c00 100644
--- a/libmultipath/configure.c
+++ b/libmultipath/configure.c
@@ -1240,9 +1240,7 @@ int coalesce_paths (struct vectors *vecs, vector mpvec, char *refwwid,
 			    mpp->no_path_retry != NO_PATH_RETRY_FAIL)
 				condlog(3, "%s: multipathd not running, unset "
 					"queue_if_no_path feature", mpp->alias);
-			if (!dm_queue_if_no_path(mpp->alias, 0))
-				remove_feature(&mpp->features,
-					       "queue_if_no_path");
+			dm_queue_if_no_path(mpp, 0);
 		}
 
 		if (!is_daemon && mpp->action != ACT_NOTHING)
diff --git a/libmultipath/devmapper.c b/libmultipath/devmapper.c
index 9be82f4e..36b9d274 100644
--- a/libmultipath/devmapper.c
+++ b/libmultipath/devmapper.c
@@ -56,6 +56,7 @@ static int dm_cancel_remove_partmaps(const char * mapname);
 static int do_foreach_partmaps(const char * mapname,
 			       int (*partmap_func)(const char *, void *),
 			       void *data);
+static int _dm_queue_if_no_path(const char *mapname, int enable);
 
 #ifndef LIBDM_API_COOKIE
 static inline int dm_task_set_cookie(struct dm_task *dmt, uint32_t *c, int a)
@@ -1085,7 +1086,7 @@ int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove,
 	if (need_suspend &&
 	    dm_get_map(mapname, &mapsize, &params) == DMP_OK &&
 	    strstr(params, "queue_if_no_path")) {
-		if (!dm_queue_if_no_path(mapname, 0))
+		if (!_dm_queue_if_no_path(mapname, 0))
 			queue_if_no_path = 1;
 		else
 			/* Leave queue_if_no_path alone if unset failed */
@@ -1134,7 +1135,7 @@ int _dm_flush_map (const char * mapname, int need_sync, int deferred_remove,
 	} while (retries-- > 0);
 
 	if (queue_if_no_path == 1)
-		dm_queue_if_no_path(mapname, 1);
+		_dm_queue_if_no_path(mapname, 1);
 
 	return 1;
 }
@@ -1252,8 +1253,8 @@ dm_reinstate_path(const char * mapname, char * path)
 	return dm_message(mapname, message);
 }
 
-int
-dm_queue_if_no_path(const char *mapname, int enable)
+static int
+_dm_queue_if_no_path(const char *mapname, int enable)
 {
 	char *message;
 
@@ -1265,6 +1266,20 @@ dm_queue_if_no_path(const char *mapname, int enable)
 	return dm_message(mapname, message);
 }
 
+int dm_queue_if_no_path(struct multipath *mpp, int enable)
+{
+	int r;
+	static const char no_path_retry[] = "queue_if_no_path";
+
+	if ((r = _dm_queue_if_no_path(mpp->alias, enable)) == 0) {
+		if (enable)
+			add_feature(&mpp->features, no_path_retry);
+		else
+			remove_feature(&mpp->features, no_path_retry);
+	}
+	return r;
+}
+
 static int
 dm_groupmsg (const char * msg, const char * mapname, int index)
 {
diff --git a/libmultipath/devmapper.h b/libmultipath/devmapper.h
index 42f8eccd..d1790df5 100644
--- a/libmultipath/devmapper.h
+++ b/libmultipath/devmapper.h
@@ -58,7 +58,7 @@ int dm_cancel_deferred_remove(struct multipath *mpp);
 int dm_flush_maps (int need_suspend, int retries);
 int dm_fail_path(const char * mapname, char * path);
 int dm_reinstate_path(const char * mapname, char * path);
-int dm_queue_if_no_path(const char *mapname, int enable);
+int dm_queue_if_no_path(struct multipath *mpp, int enable);
 int dm_switchgroup(const char * mapname, int index);
 int dm_enablegroup(const char * mapname, int index);
 int dm_disablegroup(const char * mapname, int index);
diff --git a/libmultipath/structs_vec.c b/libmultipath/structs_vec.c
index 66bc264c..665286aa 100644
--- a/libmultipath/structs_vec.c
+++ b/libmultipath/structs_vec.c
@@ -593,7 +593,7 @@ static void leave_recovery_mode(struct multipath *mpp)
 	 */
 	if (recovery && (mpp->no_path_retry == NO_PATH_RETRY_QUEUE ||
 			 mpp->no_path_retry > 0)) {
-		dm_queue_if_no_path(mpp->alias, 1);
+		dm_queue_if_no_path(mpp, 1);
 		condlog(2, "%s: queue_if_no_path enabled", mpp->alias);
 		condlog(1, "%s: Recovered to normal mode", mpp->alias);
 	}
@@ -611,11 +611,11 @@ void set_no_path_retry(struct multipath *mpp)
 		break;
 	case NO_PATH_RETRY_FAIL:
 		if (!mpp->features || is_queueing)
-			dm_queue_if_no_path(mpp->alias, 0);
+			dm_queue_if_no_path(mpp, 0);
 		break;
 	case NO_PATH_RETRY_QUEUE:
 		if (!mpp->features || !is_queueing)
-			dm_queue_if_no_path(mpp->alias, 1);
+			dm_queue_if_no_path(mpp, 1);
 		break;
 	default:
 		if (count_active_paths(mpp) > 0) {
@@ -625,7 +625,7 @@ void set_no_path_retry(struct multipath *mpp)
 			 */
 			if ((!mpp->features || !is_queueing) &&
 			    !mpp->in_recovery)
-				dm_queue_if_no_path(mpp->alias, 1);
+				dm_queue_if_no_path(mpp, 1);
 			leave_recovery_mode(mpp);
 		} else {
 			/*
@@ -636,7 +636,7 @@ void set_no_path_retry(struct multipath *mpp)
 			 */
 			if ((!mpp->features || is_queueing) &&
 			    mpp->in_recovery && mpp->retry_tick == 0)
-				dm_queue_if_no_path(mpp->alias, 0);
+				dm_queue_if_no_path(mpp, 0);
 			if (pathcount(mpp, PATH_PENDING) == 0)
 				enter_recovery_mode(mpp);
 		}
diff --git a/multipathd/main.c b/multipathd/main.c
index c7476ff0..de1c9058 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -596,7 +596,7 @@ flush_map_nopaths(struct multipath *mpp, struct vectors *vecs) {
 		mpp->no_path_retry = NO_PATH_RETRY_FAIL;
 		mpp->disable_queueing = 1;
 		mpp->stat_map_failures++;
-		dm_queue_if_no_path(mpp->alias, 0);
+		dm_queue_if_no_path(mpp, 0);
 	}
 	if (!flush_map(mpp, vecs, 1)) {
 		condlog(2, "%s: removed map after removing all paths", alias);
@@ -930,7 +930,7 @@ uev_remove_map (struct uevent * uev, struct vectors * vecs)
 		goto out;
 	}
 
-	dm_queue_if_no_path(alias, 0);
+	dm_queue_if_no_path(mpp, 0);
 	remove_map_and_stop_waiter(mpp, vecs);
 out:
 	lock_cleanup_pop(vecs->lock);
@@ -2094,7 +2094,7 @@ retry_count_tick(vector mpvec)
 			condlog(4, "%s: Retrying.. No active path", mpp->alias);
 			if(--mpp->retry_tick == 0) {
 				mpp->stat_map_failures++;
-				dm_queue_if_no_path(mpp->alias, 0);
+				dm_queue_if_no_path(mpp, 0);
 				condlog(2, "%s: Disable queueing", mpp->alias);
 			}
 		}
@@ -3260,7 +3260,7 @@ static void cleanup_maps(struct vectors *vecs)
 	put_multipath_config(conf);
 	if (queue_without_daemon == QUE_NO_DAEMON_OFF)
 		vector_foreach_slot(vecs->mpvec, mpp, i)
-			dm_queue_if_no_path(mpp->alias, 0);
+			dm_queue_if_no_path(mpp, 0);
 	remove_maps_and_stop_waiters(vecs);
 	vecs->mpvec = NULL;
 }
-- 
2.41.0

[PATCH v3 5/5] multipathd: only restore queueing if it has been disabled first

[Benjamin Marzinski]

Make the restorequeueing command only do something if disablequeueing
has first been run on the map. Also update the man page to explain
what restorequeuing actually does.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/cli_handlers.c  | 40 ++++++++++++++++----------------------
 multipathd/multipathd.8.in | 12 ++++++++++--
 2 files changed, 27 insertions(+), 25 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index b1dff202..cf448b67 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -928,22 +928,15 @@ cli_restore_queueing(void *v, struct strbuf *reply, void *data)
 		return 1;
 	}
 
-	mpp->disable_queueing = 0;
-	conf = get_multipath_config();
-	pthread_cleanup_push(put_multipath_config, conf);
-	select_no_path_retry(conf, mpp);
-	pthread_cleanup_pop(1);
-
-	/*
-	 * Don't call set_no_path_retry() for the NO_PATH_RETRY_FAIL case.
-	 * That would disable queueing when "restorequeueing" is called,
-	 * and the code never behaved that way. Users might not expect it.
-	 * In almost all cases, queueing will be disabled anyway when we
-	 * are here.
-	 */
-	if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
-	    mpp->no_path_retry != NO_PATH_RETRY_FAIL)
+	if (mpp->disable_queueing) {
+		mpp->disable_queueing = 0;
+		conf = get_multipath_config();
+		pthread_cleanup_push(put_multipath_config, conf);
+		select_no_path_retry(conf, mpp);
+		pthread_cleanup_pop(1);
 		set_no_path_retry(mpp);
+	} else
+		condlog(2, "%s: queueing not disabled. Nothing to do", mapname);
 
 	return 0;
 }
@@ -957,15 +950,16 @@ cli_restore_all_queueing(void *v, struct strbuf *reply, void *data)
 
 	condlog(2, "restore queueing (operator)");
 	vector_foreach_slot(vecs->mpvec, mpp, i) {
-		mpp->disable_queueing = 0;
-		struct config *conf = get_multipath_config();
-		pthread_cleanup_push(put_multipath_config, conf);
-		select_no_path_retry(conf, mpp);
-		pthread_cleanup_pop(1);
-		/* See comment in cli_restore_queueing() */
-		if (mpp->no_path_retry != NO_PATH_RETRY_UNDEF &&
-		    mpp->no_path_retry != NO_PATH_RETRY_FAIL)
+		if (mpp->disable_queueing) {
+			mpp->disable_queueing = 0;
+			struct config *conf = get_multipath_config();
+			pthread_cleanup_push(put_multipath_config, conf);
+			select_no_path_retry(conf, mpp);
+			pthread_cleanup_pop(1);
 			set_no_path_retry(mpp);
+		} else
+			condlog(2, "%s: queueing not disabled. Nothing to do",
+				mpp->alias);
 	}
 	return 0;
 }
diff --git a/multipathd/multipathd.8.in b/multipathd/multipathd.8.in
index e98c27fd..f1cab3ff 100644
--- a/multipathd/multipathd.8.in
+++ b/multipathd/multipathd.8.in
@@ -251,7 +251,11 @@ Disable queueing on all multipath devices.
 .
 .TP
 .B restorequeueing maps|multipaths
-Restore queueing on all multipath devices.
+Restore queueing to the configured \fIno_path_retry\fR setting on all multipath
+devices whose queueing has been previously disabled by the \fIdisablequeueing\fR
+command. \fBNote:\fR If \fIno_path_path_retry\fR is set to queue for a limited
+number of retries after all paths have failed, this will not enable queueing if
+there are no active paths.
 .
 .TP
 .B disablequeueing map|multipath $map
@@ -259,7 +263,11 @@ Disable queuing on multipathed map $map.
 .
 .TP
 .B restorequeueing map|multipath $map
-Restore queuing on multipahted map $map.
+restore queueing to the configured \fIno_path_retry\fR setting on multipathed
+map $map whose queueing has been previously disabled by the
+\fIdisablequeueing\fR command. \fBNote:\fR If \fIno_path_path_retry\fR is set to
+queue for a limited number of retries after all paths have failed, this will not
+enable queueing if there are no active paths.
 .
 .TP
 .B forcequeueing daemon
-- 
2.41.0

Re: [PATCH v3 0/5] fix set_no_path_retry and cleanup commands

[Martin Wilck]

On Tue, 2023-12-19 at 17:51 -0500, Benjamin Marzinski wrote:
> This patchset combines my patch to disable queueing in
> set_no_path_retry
> when recovery mode has timed out with my pathset to cleanup the
> interactive commands. This has been significantly changed in v3
> 
> Differences from v2 (based on suggestions by Martin Wilck):
> 0001: This completely new patch that Martin suggested. Martin, I've
>       made you the Author. Feel free to change my commit message if
>       you'd like.
> 0002: This is similar to patch 0001 from v2, except the rational has
>       changed. It is still possible to get in a situation where a
>       multipath device is queueing after timeout out in recovery, so
> we
>       still need to handle this. Since we won't have to worry about
>       multipathd flip-flopping the queueing state on reloads, I
> pulled
>       this check outside the PATH_PENDING check, as Martin suggested.
> 0003: This is the same as patches 0002-0004 from v2. It made no sense
>       to merge 0002 and 0004 but not 0003. In would just result in
>       making changes only to undo them in the next patch.
> 0004: This is instead of my 0005 patch from v2. In order for
>       disabling and restoring queuing to always work correctly,
>       multipathd needs to track the current queueing state.
> 0005: This is a new patch based on some of Martin's comments on
>       v2 patch 0005.
> 
> Differences from v1 (based on suggestions by Martin Wilck):
> 0001: added comments explaining why cleaning up the queueing state is
>       unnecessary in update_queue_mode_{add|del}_path().
> patches 0002-0005 were previously 0001-0004
> 
> Benjamin Marzinski (4):
>   multipathd: Make sure to disable queueing if recovery has failed.
>   multipathd: don't modify the multipath device on show commands
>   libmultipath: keep track of queueing state in features
>   multipathd: only restore queueing if it has been disabled first
> 
> Martin Wilck (1):
>   libmultipath: avoid temporarily enabling queueing on reload
> 
>  libmultipath/configure.c   |  4 +--
>  libmultipath/devmapper.c   | 23 +++++++++++++++---
>  libmultipath/devmapper.h   |  2 +-
>  libmultipath/dmparser.c    | 14 +++++++++--
>  libmultipath/structs_vec.c | 34 +++++++++++++++++++++-----
>  multipathd/cli_handlers.c  | 50 +++++++++++++++++-------------------
> --
>  multipathd/dmevents.c      |  2 +-
>  multipathd/main.c          | 33 +++++++++++++------------
>  multipathd/main.h          |  7 +++---
>  multipathd/multipathd.8.in | 12 +++++++--
>  multipathd/waiter.c        |  2 +-
>  11 files changed, 116 insertions(+), 67 deletions(-)
> 

For the set:

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