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

[PATCH v2] 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>
---

Was patch 4/4 of "multipath-tools: Yet Another PR fix"
Changes in v2 (assuming we agree to use this patch at all):
added a semi-colon after the "skip" label to make clang happy.

 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                   | 62 +++++++++++++++++++----------
 multipathd/main.h                   |  3 +-
 multipathd/multipathd.8.in          |  6 +++
 10 files changed, 133 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..f00444b4 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -630,28 +630,50 @@ 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:
+			; /* a statement must follow a label on pre C23 clang */
 		}
 	}
 }
@@ -739,7 +761,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 +1435,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 +2680,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 +3341,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 +4392,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 +4416,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 v2] libmpathpersist: fix code for skipping multipathd path registration

[Martin Wilck]

On Mon, 2025-12-08 at 16:12 -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>

Minor comments below.

Regards,
Martin



> ---
> 
> Was patch 4/4 of "multipath-tools: Yet Another PR fix"
> Changes in v2 (assuming we agree to use this patch at all):
> added a semi-colon after the "skip" label to make clang happy.
> 
>  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                   | 62 +++++++++++++++++++--------
> --
>  multipathd/main.h                   |  3 +-
>  multipathd/multipathd.8.in          |  6 +++
>  10 files changed, 133 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);

Nitpick: in the commit description you said the list was comma-
separated, here it looks like space-separated.

Also, I'd prefer to use pp->dev_t here unless there are good reasons
against that.

> +			}
> +		}
> +	}
> +	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..f00444b4 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -630,28 +630,50 @@ 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) {

As this command is not likely to be sent by human users, do we need to
accept both dev and dev_t here?

> +						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:
> +			; /* a statement must follow a label on pre
> C23 clang */
>  		}
>  	}
>  }
> @@ -739,7 +761,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 +1435,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 +2680,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 +3341,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 +4392,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 +4416,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.
>  .

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

[Benjamin Marzinski]

On Tue, Dec 09, 2025 at 01:01:27PM +0100, Martin Wilck wrote:
> On Mon, 2025-12-08 at 16:12 -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>
> 
> Minor comments below.
> 
> Regards,
> Martin
> 
> 
> 
> > ---
> > 
> > Was patch 4/4 of "multipath-tools: Yet Another PR fix"
> > Changes in v2 (assuming we agree to use this patch at all):
> > added a semi-colon after the "skip" label to make clang happy.
> > 
> >  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                   | 62 +++++++++++++++++++--------
> > --
> >  multipathd/main.h                   |  3 +-
> >  multipathd/multipathd.8.in          |  6 +++
> >  10 files changed, 133 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);
> 
> Nitpick: in the commit description you said the list was comma-
> separated, here it looks like space-separated.

Oops. I'll fix that.
 
> Also, I'd prefer to use pp->dev_t here unless there are good reasons
> against that.

Sure. I have no preference. 

> > +			}
> > +		}
> > +	}
> > +	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..f00444b4 100644
> > --- a/multipathd/main.c
> > +++ b/multipathd/main.c
> > @@ -630,28 +630,50 @@ 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) {
> 
> As this command is not likely to be sent by human users, do we need to
> accept both dev and dev_t here?

Probably not. I can make it just use dev_t.

-Ben
 
> > +						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:
> > +			; /* a statement must follow a label on pre
> > C23 clang */
> >  		}
> >  	}
> >  }
> > @@ -739,7 +761,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 +1435,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 +2680,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 +3341,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 +4392,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 +4416,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.
> >  .