[PATCH 0/4] multipath-tools: Yet Another PR fix

[PATCH 0/4] multipath-tools: Yet Another PR fix

[Benjamin Marzinski]

I ran into some cases where checking the number of registered keys made
multipathd think all paths had been registered, when they hadn't. The
last two patches in this patchset rework that code. The first two are
just fixes to other issues I happened to notice while working on this
code.

Benjamin Marzinski (4):
  libmpathpersist: fix register retry status checking
  multipath-tools man pages: fix multipathd commands keyword ordering
  multipathd: remember number of registered keys when ioctl fails
  libmpathpersist: fix code for skipping multipathd path registration

 libmpathpersist/mpath_persist_int.c | 20 ++++----
 libmpathpersist/mpath_updatepr.c    | 48 ++++++++++++++-----
 libmpathpersist/mpathpr.h           |  4 +-
 multipathd/callbacks.c              |  1 +
 multipathd/cli.c                    |  1 +
 multipathd/cli.h                    |  2 +
 multipathd/cli_handlers.c           | 39 ++++++++++++++--
 multipathd/main.c                   | 72 +++++++++++++++++++----------
 multipathd/main.h                   |  3 +-
 multipathd/multipathd.8.in          | 24 ++++++----
 10 files changed, 155 insertions(+), 59 deletions(-)

-- 
2.50.1

[PATCH 1/4] libmpathpersist: fix register retry status checking

[Benjamin Marzinski]

If there libmpathpersist failed to create a thread to retry the register
and ignore command, mpath_prout_reg should fail. Instead, the code was
simply ignoring the failed threads. Fix that.

Fixes: 2a4ca250 ("libmpathpersist: change how reservation conflicts are handled")
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index ea8d65ea..91c53419 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -536,19 +536,19 @@ static int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 			}
 		}
 		for (i = 0; i < count; i++) {
-			if (thread[i].param.status != MPATH_PR_SKIP &&
-			    thread[i].param.status != MPATH_PR_THREAD_ERROR) {
+			if (thread[i].param.status == MPATH_PR_SKIP)
+				continue;
+			if (thread[i].param.status != MPATH_PR_THREAD_ERROR) {
 				rc = pthread_join(thread[i].id, NULL);
 				if (rc) {
 					condlog(3, "%s: failed to join thread while retrying %d",
 						mpp->wwid, i);
 				}
-				if (thread[i].param.status ==
-				    MPATH_PR_RETRYABLE_ERROR)
-					retryable_error = true;
-				else if (status == MPATH_PR_SUCCESS)
-					status = thread[i].param.status;
 			}
+			if (thread[i].param.status == MPATH_PR_RETRYABLE_ERROR)
+				retryable_error = true;
+			else if (status == MPATH_PR_SUCCESS)
+				status = thread[i].param.status;
 		}
 		need_retry = false;
 	}
-- 
2.50.1

[PATCH 2/4] multipath-tools man pages: fix multipathd commands keyword ordering

[Benjamin Marzinski]

multipathd now has a fixed ordering of the keywords. Specifically, verbs
must come first. Fix the man page to reflect the ordering.

Fixes: f812466f6 ("multipathd: more robust command parsing")
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/multipathd.8.in | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/multipathd/multipathd.8.in b/multipathd/multipathd.8.in
index 8815e099..7ffb8d4c 100644
--- a/multipathd/multipathd.8.in
+++ b/multipathd/multipathd.8.in
@@ -344,48 +344,48 @@ will not be disabled when the daemon stops.
 Restores configured queue_without_daemon mode.
 .
 .TP
-.B map|multipath $map setprstatus
+.B setprstatus map|multipath $map
 Enable persistent reservation management on $map.
 .
 .TP
-.B map|multipath $map unsetprstatus
+.B unsetprstatus map|multipath $map
 Disable persistent reservation management on $map.
 .
 .TP
-.B map|multipath $map getprstatus
+.B getprstatus map|multipath $map
 Get the current persistent reservation management status of $map.
 .
 .TP
-.B map|multipath $map getprkey
+.B getprkey map|multipath $map
 Get the current persistent reservation key associated with $map.
 .
 .TP
-.B map|multipath $map setprkey key $key
+.B setprkey map|multipath $map key $key
 Set the persistent reservation key associated with $map to $key in the
 \fIprkeys_file\fR. This key will only be used by multipathd if
 \fIreservation_key\fR is set to \fBfile\fR in \fI@CONFIGFILE@\fR.
 .
 .TP
-.B map|multipath $map unsetprkey
+.B unsetprkey map|multipath $map
 Remove the persistent reservation key associated with $map from the
 \fIprkeys_file\fR. This will only unset the key used by multipathd if
 \fIreservation_key\fR is set to \fBfile\fR in \fI@CONFIGFILE@\fR.
 .
 .TP
-.B path $path setmarginal
+.B setmarginal path $path
 move $path to a marginal pathgroup. The path will remain in the marginal
 path group until \fIunsetmarginal\fR is called. This command will only
 work if \fImarginal_pathgroups\fR is enabled and there is no Shaky paths
 detection method configured (see the multipath.conf man page for details).
 .
 .TP
-.B path $path unsetmarginal
+.B unsetmarginal path $path
 return marginal path $path to its normal pathgroup. This command will only
 work if \fImarginal_pathgroups\fR is enabled and there is no Shaky paths
 detection method configured (see the multipath.conf man page for details).
 .
 .TP
-.B map $map unsetmarginal
+.B unsetmarginal map $map
 return all marginal paths in $map to their normal pathgroups. This command
 will only work if \fImarginal_pathgroups\fR is enabled and there is no Shaky
 paths detection method configured (see the multipath.conf man page for details).
-- 
2.50.1

[PATCH 3/4] multipathd: remember number of registered keys when ioctl fails

[Benjamin Marzinski]

If prin_do_scsi_ioctl() fails in update_map_pr() for some reason other
than Persistent Reservations not being supported, It shouldn't clear the
number of registered keys, since there's no reason to think that it has
changed. Similarly, if update_map_pr() fails in mpath_pr_event_handle(),
don't assume that the nr_keys_needed was cleared. Just return whatever
the value is now. This saves multipathd from doing pointless calls to
update_map_pr(), if one of the paths is failing.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/main.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/multipathd/main.c b/multipathd/main.c
index 4c86f31e..a7650639 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -4278,7 +4278,9 @@ void unset_pr(struct multipath *mpp)
  * The number of found keys must be at least as large as *nr_keys,
  * and if MPATH_PR_SUCCESS is returned and mpp->prflag is PR_SET after
  * the call, *nr_keys will be set to the number of found keys. Otherwise
- * it will be set to 0.
+ * if mpp->prflag is PR_UNSET it will be set to 0. If MPATH_PR_SUCCESS
+ * is not returned and mpp->prflag is not PR_UNSET, nr_keys will not be
+ * changed.
  */
 static int update_map_pr(struct multipath *mpp, struct path *pp, unsigned int *nr_keys)
 {
@@ -4307,11 +4309,12 @@ static int update_map_pr(struct multipath *mpp, struct path *pp, unsigned int *n
 
 	ret = prin_do_scsi_ioctl(pp->dev, MPATH_PRIN_RKEY_SA, &resp, 0);
 	if (ret != MPATH_PR_SUCCESS) {
-		if (ret == MPATH_PR_ILLEGAL_REQ)
+		if (ret == MPATH_PR_ILLEGAL_REQ) {
 			unset_pr(mpp);
+			*nr_keys = 0;
+		}
 		condlog(0, "%s : pr in read keys service action failed Error=%d",
 			mpp->alias, ret);
-		*nr_keys = 0;
 		return ret;
 	}
 
@@ -4427,7 +4430,7 @@ retry:
 			clear_reg ? "Clearing" : "Setting", pp->dev, ret);
 	} else if (!clear_reg) {
 		if (update_map_pr(mpp, pp, &nr_keys_needed) != MPATH_PR_SUCCESS)
-			return 0;
+			return nr_keys_needed;
 		if (mpp->prflag != PR_SET) {
 			memset(&param, 0, sizeof(param));
 			clear_reg = true;
-- 
2.50.1

[PATCH 4/4] libmpathpersist: fix code for skipping multipathd path registration

[Benjamin Marzinski]

When libmpathpersist notifies multipathd that a key has been registered,
cli_setprstatus() calls pr_register_active_paths() with a flag to let it
know that the paths are likely already registered, and it can skip
re-registering them, as long as the number of active paths matches the
number of registered keys. This shortcut can fail, causing multipathd to
not register needed paths, if either a path becomes usable and another
becomes unusable while libmpathpersist is running or if there already
were registered keys for I_T Nexus's that don't correspond to path
devices.

To make this shortcut work in cases like that, this commit adds a new
multipathd command "setprstatus map <map> pathlist <pathlist>", where
<pathlist> is a quoted, comma separated list of scsi path devices.
libmpathpersist will send out the list of paths it registered the key
on. pr_register_active_paths() will skip calling mpath_pr_event_handle()
for paths on that list.

In order to deal with the possiblity of a preempt occuring while
libmpathpersist was running, the code still needs to check that it has
the expected number of keys.

Fixes: f7d6cd17 ("multipathd: Fix race while registering PR key")
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c |  6 +--
 libmpathpersist/mpath_updatepr.c    | 48 +++++++++++++++++------
 libmpathpersist/mpathpr.h           |  4 +-
 multipathd/callbacks.c              |  1 +
 multipathd/cli.c                    |  1 +
 multipathd/cli.h                    |  2 +
 multipathd/cli_handlers.c           | 39 ++++++++++++++++--
 multipathd/main.c                   | 61 +++++++++++++++++++----------
 multipathd/main.h                   |  3 +-
 multipathd/multipathd.8.in          |  6 +++
 10 files changed, 132 insertions(+), 39 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 91c53419..14f7ba83 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -1001,12 +1001,12 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	case MPATH_PROUT_REG_SA:
 	case MPATH_PROUT_REG_IGN_SA:
 		if (unregistering)
-			update_prflag(mpp->alias, 0);
+			update_prflag(mpp, 0);
 		else
-			update_prflag(mpp->alias, 1);
+			update_prflag(mpp, 1);
 		break;
 	case MPATH_PROUT_CLEAR_SA:
-		update_prflag(mpp->alias, 0);
+		update_prflag(mpp, 0);
 		if (mpp->prkey_source == PRKEY_SOURCE_FILE)
 			update_prkey(mpp->alias, 0);
 		break;
diff --git a/libmpathpersist/mpath_updatepr.c b/libmpathpersist/mpath_updatepr.c
index bd8ed2be..bc9c79e2 100644
--- a/libmpathpersist/mpath_updatepr.c
+++ b/libmpathpersist/mpath_updatepr.c
@@ -20,8 +20,9 @@
 #include "uxsock.h"
 #include "mpathpr.h"
 #include "structs.h"
+#include "strbuf.h"
 
-static char *do_pr(char *alias, char *str)
+static char *do_pr(char *alias, const char *str)
 {
 	int fd;
 	char *reply;
@@ -51,24 +52,27 @@ static char *do_pr(char *alias, char *str)
 	return reply;
 }
 
-static int do_update_pr(char *alias, char *cmd, char *key)
+static int do_update_pr(char *alias, char *cmd, const char *data)
 {
-	char str[256];
+	STRBUF_ON_STACK(buf);
 	char *reply = NULL;
 	int ret = -1;
 
-	if (key)
-		snprintf(str, sizeof(str), "%s map %s key %s", cmd, alias, key);
+	if (data)
+		print_strbuf(&buf, "%s map %s %s %s", cmd, alias,
+			     strcmp(cmd, "setprkey") ? "pathlist" : "key",
+			     data);
 	else
-		snprintf(str, sizeof(str), "%s map %s", cmd, alias);
+		print_strbuf(&buf, "%s map %s", cmd, alias);
 
-	reply = do_pr(alias, str);
+	reply = do_pr(alias, get_strbuf_str(&buf));
 	if (reply) {
 		if (strncmp(reply, "ok", 2) == 0)
 			ret = 0;
 		else
 			ret = -1;
-		condlog(ret ? 0 : 4, "%s: message=%s reply=%s", alias, str, reply);
+		condlog(ret ? 0 : 4, "%s: message=%s reply=%s", alias,
+			get_strbuf_str(&buf), reply);
 	}
 
 	free(reply);
@@ -106,9 +110,31 @@ int get_prhold(char *mapname)
 	return do_get_pr(mapname, "getprhold");
 }
 
-int update_prflag(char *mapname, int set) {
-	return do_update_pr(mapname, (set)? "setprstatus" : "unsetprstatus",
-			    NULL);
+int update_prflag(struct multipath *mpp, int set)
+{
+	STRBUF_ON_STACK(buf);
+	int i, j;
+	bool first = true;
+	struct pathgroup *pgp = NULL;
+	struct path *pp = NULL;
+
+	if (!set)
+		return do_update_pr(mpp->alias, "unsetprstatus", NULL);
+
+	append_strbuf_str(&buf, "\"");
+	vector_foreach_slot (mpp->pg, pgp, j) {
+		vector_foreach_slot (pgp->paths, pp, i) {
+			if (pp->state == PATH_UP || pp->state == PATH_GHOST) {
+				if (first) {
+					append_strbuf_str(&buf, pp->dev);
+					first = false;
+				} else
+					print_strbuf(&buf, " %s", pp->dev);
+			}
+		}
+	}
+	append_strbuf_str(&buf, "\"");
+	return do_update_pr(mpp->alias, "setprstatus", get_strbuf_str(&buf));
 }
 
 int update_prhold(char *mapname, bool set)
diff --git a/libmpathpersist/mpathpr.h b/libmpathpersist/mpathpr.h
index 99d6b82f..adbfc865 100644
--- a/libmpathpersist/mpathpr.h
+++ b/libmpathpersist/mpathpr.h
@@ -1,12 +1,14 @@
 #ifndef MPATHPR_H_INCLUDED
 #define MPATHPR_H_INCLUDED
 
+#include "structs.h"
+
 /*
  * This header file contains symbols that are only used by
  * libmpathpersist internally.
  */
 
-int update_prflag(char *mapname, int set);
+int update_prflag(struct multipath *mpp, int set);
 int update_prkey_flags(char *mapname, uint64_t prkey, uint8_t sa_flags);
 int get_prflag(char *mapname);
 int get_prhold(char *mapname);
diff --git a/multipathd/callbacks.c b/multipathd/callbacks.c
index b6b57f45..1129855b 100644
--- a/multipathd/callbacks.c
+++ b/multipathd/callbacks.c
@@ -59,6 +59,7 @@ void init_handler_callbacks(void)
 	set_unlocked_handler_callback(VRB_SHUTDOWN, HANDLER(cli_shutdown));
 	set_handler_callback(VRB_GETPRSTATUS | Q1_MAP, HANDLER(cli_getprstatus));
 	set_handler_callback(VRB_SETPRSTATUS | Q1_MAP, HANDLER(cli_setprstatus));
+	set_handler_callback(VRB_SETPRSTATUS | Q1_MAP | Q2_PATHLIST, HANDLER(cli_setprstatus_list));
 	set_handler_callback(VRB_UNSETPRSTATUS | Q1_MAP, HANDLER(cli_unsetprstatus));
 	set_handler_callback(VRB_FORCEQ | Q1_DAEMON, HANDLER(cli_force_no_daemon_q));
 	set_handler_callback(VRB_RESTOREQ | Q1_DAEMON, HANDLER(cli_restore_no_daemon_q));
diff --git a/multipathd/cli.c b/multipathd/cli.c
index d0e6cebc..0d679c86 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -227,6 +227,7 @@ load_keys (void)
 	r += add_key(keys, "getprhold", VRB_GETPRHOLD, 0);
 	r += add_key(keys, "setprhold", VRB_SETPRHOLD, 0);
 	r += add_key(keys, "unsetprhold", VRB_UNSETPRHOLD, 0);
+	r += add_key(keys, "pathlist", KEY_PATHLIST, 1);
 
 	if (r) {
 		free_keys(keys);
diff --git a/multipathd/cli.h b/multipathd/cli.h
index 5a943a45..3e607389 100644
--- a/multipathd/cli.h
+++ b/multipathd/cli.h
@@ -62,6 +62,7 @@ enum {
 	KEY_LOCAL		= 81,
 	KEY_GROUP		= 82,
 	KEY_KEY			= 83,
+	KEY_PATHLIST		= 84,
 };
 
 /*
@@ -94,6 +95,7 @@ enum {
 	Q2_LOCAL		= KEY_LOCAL << 16,
 	Q2_GROUP		= KEY_GROUP << 16,
 	Q2_KEY			= KEY_KEY << 16,
+	Q2_PATHLIST		= KEY_PATHLIST << 16,
 
 	/* byte 3: qualifier 3 */
 	Q3_FMT			= KEY_FMT << 24,
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 2812d01e..5770dcec 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -31,6 +31,7 @@
 #include "foreign.h"
 #include "strbuf.h"
 #include "cli_handlers.h"
+#include <ctype.h>
 
 static struct path *
 find_path_by_str(const struct vector_s *pathvec, const char *str,
@@ -1276,8 +1277,8 @@ cli_getprstatus (void * v, struct strbuf *reply, void * data)
 	return 0;
 }
 
-static int
-cli_setprstatus(void * v, struct strbuf *reply, void * data)
+static int do_setprstatus(void * v, struct strbuf *reply, void * data,
+			  const struct vector_s *registered_paths)
 {
 	struct multipath * mpp;
 	struct vectors * vecs = (struct vectors *)data;
@@ -1291,7 +1292,7 @@ cli_setprstatus(void * v, struct strbuf *reply, void * data)
 
 	if (mpp->prflag != PR_SET) {
 		set_pr(mpp);
-		pr_register_active_paths(mpp, true);
+		pr_register_active_paths(mpp, registered_paths);
 		if (mpp->prflag == PR_SET)
 			condlog(2, "%s: prflag set", param);
 		else
@@ -1302,6 +1303,38 @@ cli_setprstatus(void * v, struct strbuf *reply, void * data)
 	return 0;
 }
 
+static int
+cli_setprstatus(void * v, struct strbuf *reply, void * data)
+{
+	return do_setprstatus(v, reply, data, NULL);
+}
+
+static int
+cli_setprstatus_list(void * v, struct strbuf *reply, void * data)
+{
+	int r;
+	struct vector_s registered_paths = { .allocated = 0 };
+	char *ptr = get_keyparam(v, KEY_PATHLIST);
+
+	while (isspace(*ptr))
+		ptr++;
+	while (*ptr) {
+		if (!vector_alloc_slot(&registered_paths)) {
+			r = -ENOMEM;
+			goto out;
+		}
+		vector_set_slot(&registered_paths, ptr);
+		while (*ptr && !isspace(*ptr))
+			ptr++;
+		while (isspace(*ptr))
+			*ptr++ = '\0';
+	}
+	r = do_setprstatus(v, reply, data, &registered_paths);
+out:
+	vector_reset(&registered_paths);
+	return r;
+}
+
 static int
 cli_unsetprstatus(void * v, struct strbuf *reply, void * data)
 {
diff --git a/multipathd/main.c b/multipathd/main.c
index a7650639..2cd5c952 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -630,28 +630,49 @@ flush_map_nopaths(struct multipath *mpp, struct vectors *vecs) {
 	return true;
 }
 
-void pr_register_active_paths(struct multipath *mpp, bool check_nr_active)
+/*
+ * If reg_paths in non-NULL, it is a vector of paths that libmpathpersist
+ * registered. If the number of registered keys is smaller than the number
+ * of registered paths, then likely a preempt that occurred while
+ * libmpathpersist was registering the key. As long as there are still some
+ * registered keys, treat the preempt as happening first, and make sure to
+ * register keys on all the paths. If the number of registered keys is at
+ * least as large as the number of registered paths, then no preempt happened,
+ * and multipathd does not need to re-register the paths that libmpathpersist
+ * handled
+ */
+void pr_register_active_paths(struct multipath *mpp,
+			      const struct vector_s *reg_paths)
 {
-	unsigned int i, j, nr_keys = 0;
-	unsigned int nr_active = 0;
+	unsigned int i, j, k, nr_keys = 0;
+	unsigned int wanted_nr = VECTOR_SIZE(reg_paths);
 	struct path *pp;
 	struct pathgroup *pgp;
-
-	if (check_nr_active) {
-		nr_active = count_active_paths(mpp);
-		if (!nr_active)
-			return;
-	}
+	char *pathname;
 
 	vector_foreach_slot (mpp->pg, pgp, i) {
 		vector_foreach_slot (pgp->paths, pp, j) {
 			if (mpp->prflag == PR_UNSET)
 				return;
-			if (pp->state == PATH_UP || pp->state == PATH_GHOST) {
-				nr_keys = mpath_pr_event_handle(pp, nr_keys, nr_active);
-				if (check_nr_active && nr_keys == nr_active)
-					return;
+			if (pp->state != PATH_UP && pp->state != PATH_GHOST)
+				continue;
+			if (wanted_nr && nr_keys) {
+				vector_foreach_slot(reg_paths, pathname, k) {
+					if (strcmp(pp->dev, pathname) == 0 ||
+					    strcmp(pp->dev_t, pathname) == 0) {
+						goto skip;
+					}
+				}
+			}
+			nr_keys = mpath_pr_event_handle(pp, nr_keys, wanted_nr);
+			if (nr_keys && nr_keys < wanted_nr) {
+				/*
+				 * Incorrect number of registered keys. Need
+				 * to register all devices
+				 */
+				wanted_nr = 0;
 			}
+skip:
 		}
 	}
 }
@@ -739,7 +760,7 @@ fail:
 
 	sync_map_state(mpp, false);
 
-	pr_register_active_paths(mpp, false);
+	pr_register_active_paths(mpp, NULL);
 
 	if (VECTOR_SIZE(offline_paths) != 0)
 		handle_orphaned_offline_paths(offline_paths);
@@ -1413,7 +1434,7 @@ rescan:
 
 	if (retries >= 0) {
 		if ((mpp->prflag == PR_SET && prflag != PR_SET) || start_waiter)
-			pr_register_active_paths(mpp, false);
+			pr_register_active_paths(mpp, NULL);
 		condlog(2, "%s [%s]: path added to devmap %s",
 			pp->dev, pp->dev_t, mpp->alias);
 		return 0;
@@ -2658,7 +2679,7 @@ update_path_state (struct vectors * vecs, struct path * pp)
 				"reservation registration", pp->dev);
 			mpath_pr_event_handle(pp, 0, 0);
 			if (pp->mpp->prflag == PR_SET && prflag != PR_SET)
-				pr_register_active_paths(pp->mpp, false);
+				pr_register_active_paths(pp->mpp, NULL);
 		}
 
 		/*
@@ -3319,7 +3340,7 @@ configure (struct vectors * vecs, enum force_reload_types reload_type)
 	vector_foreach_slot(mpvec, mpp, i){
 		if (remember_wwid(mpp->wwid) == 1)
 			trigger_paths_udev_change(mpp, true);
-		pr_register_active_paths(mpp, false);
+		pr_register_active_paths(mpp, NULL);
 	}
 
 	/*
@@ -4370,8 +4391,8 @@ static int update_map_pr(struct multipath *mpp, struct path *pp, unsigned int *n
  *
  * nr_keys_wanted: Only used if nr_keys_needed is 0, so we don't know how
  * many keys we currently have. If nr_keys_wanted in non-zero and the
- * number of keys found by the initial call to update_map_pr() matches it,
- * exit early, since we have all the keys we are expecting.
+ * number of keys found by the initial call to update_map_pr() is at least
+ * as large as it, exit early, since we have all the keys we are expecting.
  *
  * The function returns the number of keys that are registered or 0 if
  * it's unknown.
@@ -4394,7 +4415,7 @@ mpath_pr_event_handle(struct path *pp, unsigned int nr_keys_needed,
 		nr_keys_needed = 1;
 		if (update_map_pr(mpp, pp, &nr_keys_needed) != MPATH_PR_SUCCESS)
 			return 0;
-		if (nr_keys_wanted && nr_keys_wanted == nr_keys_needed)
+		if (nr_keys_wanted && nr_keys_wanted <= nr_keys_needed)
 			return nr_keys_needed;
 	}
 
diff --git a/multipathd/main.h b/multipathd/main.h
index 6d60ee81..85b533cd 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -54,5 +54,6 @@ int resize_map(struct multipath *mpp, unsigned long long size,
 	       struct vectors *vecs);
 void set_pr(struct multipath *mpp);
 void unset_pr(struct multipath *mpp);
-void pr_register_active_paths(struct multipath *mpp, bool check_active_nr);
+void pr_register_active_paths(struct multipath *mpp,
+			      const struct vector_s *registered_paths);
 #endif /* MAIN_H_INCLUDED */
diff --git a/multipathd/multipathd.8.in b/multipathd/multipathd.8.in
index 7ffb8d4c..38a243e3 100644
--- a/multipathd/multipathd.8.in
+++ b/multipathd/multipathd.8.in
@@ -348,6 +348,12 @@ Restores configured queue_without_daemon mode.
 Enable persistent reservation management on $map.
 .
 .TP
+.B setprstatus map|multipath $map pathlist $pathlist
+Enable persistent reservation management on $map, and notify multipathd of
+the paths that have been registered, so it doesn't attempt to re-register
+them.
+.
+.TP
 .B unsetprstatus map|multipath $map
 Disable persistent reservation management on $map.
 .
-- 
2.50.1

Re: [PATCH 4/4] libmpathpersist: fix code for skipping multipathd path registration

[Martin Wilck]

On Mon, 2025-12-01 at 22:02 -0500, Benjamin Marzinski wrote:
> When libmpathpersist notifies multipathd that a key has been
> registered,
> cli_setprstatus() calls pr_register_active_paths() with a flag to let
> it
> know that the paths are likely already registered, and it can skip
> re-registering them, as long as the number of active paths matches
> the
> number of registered keys. This shortcut can fail, causing multipathd
> to
> not register needed paths, if either a path becomes usable and
> another
> becomes unusable while libmpathpersist is running or if there already
> were registered keys for I_T Nexus's that don't correspond to path
> devices.
> 
> To make this shortcut work in cases like that, this commit adds a new
> multipathd command "setprstatus map <map> pathlist <pathlist>", where
> <pathlist> is a quoted, comma separated list of scsi path devices.
> libmpathpersist will send out the list of paths it registered the key
> on. pr_register_active_paths() will skip calling
> mpath_pr_event_handle()
> for paths on that list.
> 
> In order to deal with the possiblity of a preempt occuring while
> libmpathpersist was running, the code still needs to check that it
> has
> the expected number of keys.
> 
> Fixes: f7d6cd17 ("multipathd: Fix race while registering PR key")
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmpathpersist/mpath_persist_int.c |  6 +--
>  libmpathpersist/mpath_updatepr.c    | 48 +++++++++++++++++------
>  libmpathpersist/mpathpr.h           |  4 +-
>  multipathd/callbacks.c              |  1 +
>  multipathd/cli.c                    |  1 +
>  multipathd/cli.h                    |  2 +
>  multipathd/cli_handlers.c           | 39 ++++++++++++++++--
>  multipathd/main.c                   | 61 +++++++++++++++++++--------
> --
>  multipathd/main.h                   |  3 +-
>  multipathd/multipathd.8.in          |  6 +++
>  10 files changed, 132 insertions(+), 39 deletions(-)

I may be missing something, but this is quite a lot of additional
complexity just for the shortcut. Have you considered just not taking
the shortcut in the first place?

Martin

Re: [PATCH 0/4] multipath-tools: Yet Another PR fix

[Martin Wilck]

On Mon, 2025-12-01 at 22:02 -0500, Benjamin Marzinski wrote:
> I ran into some cases where checking the number of registered keys
> made
> multipathd think all paths had been registered, when they hadn't. The
> last two patches in this patchset rework that code. The first two are
> just fixes to other issues I happened to notice while working on this
> code.
> 
> Benjamin Marzinski (4):
>   libmpathpersist: fix register retry status checking
>   multipath-tools man pages: fix multipathd commands keyword ordering
>   multipathd: remember number of registered keys when ioctl fails
>   libmpathpersist: fix code for skipping multipathd path registration
> 
>  libmpathpersist/mpath_persist_int.c | 20 ++++----
>  libmpathpersist/mpath_updatepr.c    | 48 ++++++++++++++-----
>  libmpathpersist/mpathpr.h           |  4 +-
>  multipathd/callbacks.c              |  1 +
>  multipathd/cli.c                    |  1 +
>  multipathd/cli.h                    |  2 +
>  multipathd/cli_handlers.c           | 39 ++++++++++++++--
>  multipathd/main.c                   | 72 +++++++++++++++++++--------
> --
>  multipathd/main.h                   |  3 +-
>  multipathd/multipathd.8.in          | 24 ++++++----
>  10 files changed, 155 insertions(+), 59 deletions(-)

For the series, except 4/4:
Reviewed-by: Martin Wilck <mwilck@suse.com>

Re: [PATCH 4/4] libmpathpersist: fix code for skipping multipathd path registration

[Martin Wilck]

On Mon, 2025-12-01 at 22:02 -0500, Benjamin Marzinski wrote:
> When libmpathpersist notifies multipathd that a key has been
> registered,
> cli_setprstatus() calls pr_register_active_paths() with a flag to let
> it
> know that the paths are likely already registered, and it can skip
> re-registering them, as long as the number of active paths matches
> the
> number of registered keys. This shortcut can fail, causing multipathd
> to
> not register needed paths, if either a path becomes usable and
> another
> becomes unusable while libmpathpersist is running or if there already
> were registered keys for I_T Nexus's that don't correspond to path
> devices.
> 
> To make this shortcut work in cases like that, this commit adds a new
> multipathd command "setprstatus map <map> pathlist <pathlist>", where
> <pathlist> is a quoted, comma separated list of scsi path devices.
> libmpathpersist will send out the list of paths it registered the key
> on. pr_register_active_paths() will skip calling
> mpath_pr_event_handle()
> for paths on that list.
> 
> In order to deal with the possiblity of a preempt occuring while
> libmpathpersist was running, the code still needs to check that it
> has
> the expected number of keys.
> 
> Fixes: f7d6cd17 ("multipathd: Fix race while registering PR key")
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmpathpersist/mpath_persist_int.c |  6 +--
>  libmpathpersist/mpath_updatepr.c    | 48 +++++++++++++++++------
>  libmpathpersist/mpathpr.h           |  4 +-
>  multipathd/callbacks.c              |  1 +
>  multipathd/cli.c                    |  1 +
>  multipathd/cli.h                    |  2 +
>  multipathd/cli_handlers.c           | 39 ++++++++++++++++--
>  multipathd/main.c                   | 61 +++++++++++++++++++--------
> --
>  multipathd/main.h                   |  3 +-
>  multipathd/multipathd.8.in          |  6 +++
>  10 files changed, 132 insertions(+), 39 deletions(-)
> 
> 
> diff --git a/multipathd/main.c b/multipathd/main.c
> index a7650639..2cd5c952 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -630,28 +630,49 @@ flush_map_nopaths(struct multipath *mpp, struct
> vectors *vecs) {
>  	return true;
>  }
>  
> -void pr_register_active_paths(struct multipath *mpp, bool
> check_nr_active)
> +/*
> + * If reg_paths in non-NULL, it is a vector of paths that
> libmpathpersist
> + * registered. If the number of registered keys is smaller than the
> number
> + * of registered paths, then likely a preempt that occurred while
> + * libmpathpersist was registering the key. As long as there are
> still some
> + * registered keys, treat the preempt as happening first, and make
> sure to
> + * register keys on all the paths. If the number of registered keys
> is at
> + * least as large as the number of registered paths, then no preempt
> happened,
> + * and multipathd does not need to re-register the paths that
> libmpathpersist
> + * handled
> + */
> +void pr_register_active_paths(struct multipath *mpp,
> +			      const struct vector_s *reg_paths)
>  {
> -	unsigned int i, j, nr_keys = 0;
> -	unsigned int nr_active = 0;
> +	unsigned int i, j, k, nr_keys = 0;
> +	unsigned int wanted_nr = VECTOR_SIZE(reg_paths);
>  	struct path *pp;
>  	struct pathgroup *pgp;
> -
> -	if (check_nr_active) {
> -		nr_active = count_active_paths(mpp);
> -		if (!nr_active)
> -			return;
> -	}
> +	char *pathname;
>  
>  	vector_foreach_slot (mpp->pg, pgp, i) {
>  		vector_foreach_slot (pgp->paths, pp, j) {
>  			if (mpp->prflag == PR_UNSET)
>  				return;
> -			if (pp->state == PATH_UP || pp->state ==
> PATH_GHOST) {
> -				nr_keys = mpath_pr_event_handle(pp,
> nr_keys, nr_active);
> -				if (check_nr_active && nr_keys ==
> nr_active)
> -					return;
> +			if (pp->state != PATH_UP && pp->state !=
> PATH_GHOST)
> +				continue;
> +			if (wanted_nr && nr_keys) {
> +				vector_foreach_slot(reg_paths,
> pathname, k) {
> +					if (strcmp(pp->dev,
> pathname) == 0 ||
> +					    strcmp(pp->dev_t,
> pathname) == 0) {
> +						goto skip;
> +					}
> +				}
> +			}
> +			nr_keys = mpath_pr_event_handle(pp, nr_keys,
> wanted_nr);
> +			if (nr_keys && nr_keys < wanted_nr) {
> +				/*
> +				 * Incorrect number of registered
> keys. Need
> +				 * to register all devices
> +				 */
> +				wanted_nr = 0;
>  			}
> +skip:

This fails on Debian Sid with this error messsage:

main.c:677:3: error: label at end of compound statement is a C23
extension [-Werror,-Wc23-extensions]
  677 |                 }
      |            


[1] https://github.com/openSUSE/multipath-tools/actions/runs/20031717354/job/57442300023

Martin

Re: [PATCH 4/4] libmpathpersist: fix code for skipping multipathd path registration

[Benjamin Marzinski]

On Mon, Dec 08, 2025 at 03:46:56PM +0100, Martin Wilck wrote:
> On Mon, 2025-12-01 at 22:02 -0500, Benjamin Marzinski wrote:
> > When libmpathpersist notifies multipathd that a key has been
> > registered,
> > cli_setprstatus() calls pr_register_active_paths() with a flag to let
> > it
> > know that the paths are likely already registered, and it can skip
> > re-registering them, as long as the number of active paths matches
> > the
> > number of registered keys. This shortcut can fail, causing multipathd
> > to
> > not register needed paths, if either a path becomes usable and
> > another
> > becomes unusable while libmpathpersist is running or if there already
> > were registered keys for I_T Nexus's that don't correspond to path
> > devices.
> > 
> > To make this shortcut work in cases like that, this commit adds a new
> > multipathd command "setprstatus map <map> pathlist <pathlist>", where
> > <pathlist> is a quoted, comma separated list of scsi path devices.
> > libmpathpersist will send out the list of paths it registered the key
> > on. pr_register_active_paths() will skip calling
> > mpath_pr_event_handle()
> > for paths on that list.
> > 
> > In order to deal with the possiblity of a preempt occuring while
> > libmpathpersist was running, the code still needs to check that it
> > has
> > the expected number of keys.
> > 
> > Fixes: f7d6cd17 ("multipathd: Fix race while registering PR key")
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmpathpersist/mpath_persist_int.c |  6 +--
> >  libmpathpersist/mpath_updatepr.c    | 48 +++++++++++++++++------
> >  libmpathpersist/mpathpr.h           |  4 +-
> >  multipathd/callbacks.c              |  1 +
> >  multipathd/cli.c                    |  1 +
> >  multipathd/cli.h                    |  2 +
> >  multipathd/cli_handlers.c           | 39 ++++++++++++++++--
> >  multipathd/main.c                   | 61 +++++++++++++++++++--------
> > --
> >  multipathd/main.h                   |  3 +-
> >  multipathd/multipathd.8.in          |  6 +++
> >  10 files changed, 132 insertions(+), 39 deletions(-)
> 
> I may be missing something, but this is quite a lot of additional
> complexity just for the shortcut. Have you considered just not taking
> the shortcut in the first place?

Yep. But that means that whenever you register a multipath device with
mpathpersist, multipathd will (almost always pointlessly) re-register
all the paths, just to make sure it doesn't hit these super rare corner
cases. So this added complexity keeps multipathd from running unnecesary
scsi PR commands, and should be quicker. All multipathd does is break
the new cli message up into pathnames, and then has
pr_register_active_paths() check the paths against the pathanames, and
skip them if they match and the number of keys is as big as expected
(and it needs to check the number of keys regardless of whether it uses
the shortcut).

I thought briefly about pulling all the path registration work into
multipathd, but it ends up making multipathd even more complex than this
shortcut, while not saving much complexity from libmpathpersist. If we
wanted to make multipathd handle all of libmpathpersists work, that
would be a different story, but I'm still not sold on the idea of doing
that.

That being said, we can drop a chunk of code if we just force
multipathd to automatically reregister the paths. So if you think that
the reduced work is not worth the added complexity, I'm o.k. with
pulling the shortcut out completely.

-Ben
 
> Martin

Re: [PATCH 4/4] libmpathpersist: fix code for skipping multipathd path registration

[Martin Wilck]

On Mon, 2025-12-08 at 15:50 -0500, Benjamin Marzinski wrote:
> On Mon, Dec 08, 2025 at 03:46:56PM +0100, Martin Wilck wrote:
> > 
> > I may be missing something, but this is quite a lot of additional
> > complexity just for the shortcut. Have you considered just not
> > taking
> > the shortcut in the first place?
> 
> Yep. But that means that whenever you register a multipath device
> with
> mpathpersist, multipathd will (almost always pointlessly) re-register
> all the paths, just to make sure it doesn't hit these super rare
> corner
> cases. So this added complexity keeps multipathd from running
> unnecesary
> scsi PR commands, and should be quicker. All multipathd does is break
> the new cli message up into pathnames, and then has
> pr_register_active_paths() check the paths against the pathanames,
> and
> skip them if they match and the number of keys is as big as expected
> (and it needs to check the number of keys regardless of whether it
> uses
> the shortcut).
> 
> I thought briefly about pulling all the path registration work into
> multipathd, but it ends up making multipathd even more complex than
> this
> shortcut, while not saving much complexity from libmpathpersist. If
> we
> wanted to make multipathd handle all of libmpathpersists work, that
> would be a different story, but I'm still not sold on the idea of
> doing
> that.
> 
> That being said, we can drop a chunk of code if we just force
> multipathd to automatically reregister the paths. So if you think
> that
> the reduced work is not worth the added complexity, I'm o.k. with
> pulling the shortcut out completely.

I admit I am somewhat intimidated by the complexity into which the PR
code has grown. Therefore I would be glad to avoid extra complexity if
possible. But for that very reason, I don't feel competent to judge
whether this complexity is "worth" it, IOW how badly performance would
be affected by the extra register commands, in particular during high
IO load. I suppose it's not so much a matter of "performance", rather
of latency between running mpathpersist and the completion of the task
in multipathd, which might be critical in certain situations.

This is why I wrote "have you considered". If you, from the vast
exprience that you gained in the course of your recent work, think that
this extra complexity is worth it, fine with me. I'll go over the patch
once more.

Martin