[PATCH 00/15] Improve mpathpersist's unavailable path handling

[PATCH 00/15] Improve mpathpersist's unavailable path handling

[Benjamin Marzinski]

A problem that mpathpersist has with making SCSI Persistent Resevations
to a multipath device work like they do to individual SCSI devices is
that some of the paths to a multipath device might be down or missing
when the mpathpersist commands are run. Multipath handles registering a
new key pretty well. If paths are unavailable at the time of the
command, the key is registered when they later become available.  But if
the multipath device is also holding a reservation on one of its paths,
things get trickier.

If a persistent reservation is being held by an unsuable path of a
multipath device (the path can either be down or completely removed),
libmpathpersist can't change it just by forwarding the regular
persistent reservation commands. This can cause problems both for the
RELEASE command and the REGISTER and REGISTER AND IGNORE commands if
they are used to change from one key to another. If the path holding the
reservation is unavailable, the reservation won't be released or have
its key changed, as expected. I wish the problem of having a reservation
key changed while it is holding the reservation was simply a theoretical
one, but there are enterprise users of multipath that need this
capability.

This patchset deals with both of these problems. libmpathpersist always
had code to handle releasing a reservation held by an unavailable path,
but the existing method is broken. It relies on poorly supported
optional features of SCSI Persistent Reservations: the READ FULL STATUS
command and specifying Initiator Ports with the REGISTER command
(SIP_C). Also, fixing its current issues would additionally require
supporting the All Target Ports option (ATP_C). This existing workaround
has been redesigned to use the PREEMPT command instead. Key changes
where the path holding the reservation is unavailable were not
previously handled by libmpathpersist. This patchset also handles them
using the PREEMPT command.

patches 0001-0008 are simply cleanups an prep work.
patches 0009-0010 redesign the RELEASE command workaround
patch 0011 creates a workaround for the REGISTER command
patch 0012 makes this work for the REGISTER AND IGNORE command as well
patches 0013-0014 are more prep work
patch 0015 Adds a safety check before preempting a key, so that only
           devices that are holding a reservation will do the
           preemption

These workarounds depend on the fact that managing scsi persistent
reservations for multipath devices has never worked correctly if
multiple nodes use the same persistent reservation key for the same
device. multipathd needs to be able to register the key on new paths,
but the key could have been preempted since it was registered for the
multipath device. To make sure that multipath isn't automatically
registering paths for a device that has been preempted, multipathd
checks that the configured key has already been registered by other
paths of the device.  Unfortunately, there is no way to verify that the
other paths to the multipath device on this node are the ones holding
the keys in question. If another node is using the same key for the
device, those registered keys could belong to paths on the other node,
and there could be no registered keys from this node (likely due to
being preempted). If multipathd registered the key for this new path
because it saw another node's keys, then a preempted node would suddenly
gain access to the storage.

As far as the workarounds from this patchset go, if there was another
node using the same key, then preempting the key in these workarounds
would preempt the other node as well. The safety check in patch 0015
tries to make sure that this only happens when the current is holding
the reservation, but this isn't foolproof.

I should also note that while working on this patchset, I noticed a
number of other issues with the persistent reservation code, mostly
involving changing a registered key. I'll be fixing them in a future
patchset.

Benjamin Marzinski (15):
  multipathd: remove thread from mpath_pr_event_handle
  libmpathpersist: remove uneeded wrapper function.
  libmpathpersist: reduce log level for persistent reservation checking
  libmpathpersist: remove pointless update_map_pr ret value code
  multipathd: use update_map_pr in mpath_pr_event_handle
  libmpathpersist: limit changing prflag in update_map_pr
  multipathd: Don't call update_map_pr unnecessarily
  libmpathpersist: remove useless function send_prout_activepath
  limpathpersist: redesign failed release workaround
  libmpathpersist: fail the release if all threads fail
  limpathpersist: Handle changing key corner case
  libmapthpersist: Handle REGISTER AND IGNORE changing key corner case
  libmultipath: rename prflag_value enums
  libmpathpersist: use a switch statement for prout command finalizing
  libmpathpersist: Add safety check for preempting on key change

 libmpathpersist/libmpathpersist.version |   2 +-
 libmpathpersist/mpath_persist_int.c     | 505 ++++++++++++++----------
 libmpathpersist/mpath_persist_int.h     |   2 +-
 libmpathpersist/mpath_updatepr.c        |  74 +++-
 libmpathpersist/mpathpr.h               |   3 +
 libmultipath/libmultipath.version       |   1 +
 libmultipath/structs.h                  |  10 +-
 multipathd/callbacks.c                  |   3 +
 multipathd/cli.c                        |   4 +-
 multipathd/cli.h                        |   3 +
 multipathd/cli_handlers.c               |  70 +++-
 multipathd/main.c                       | 149 +++----
 12 files changed, 483 insertions(+), 343 deletions(-)

-- 
2.48.1

[PATCH 01/15] multipathd: remove thread from mpath_pr_event_handle

[Benjamin Marzinski]

mpath_pr_event_handle() creates a separate thread to do the persistent
reservation work, but it doesn't take any advantage of the work being
done in another thread. Merge mpath_pr_event_handle() and
mpath_pr_event_handler_fn() into a single function with no separate
thread.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index 2d5c146d..46eef222 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -89,8 +89,7 @@
 #define CMDSIZE 160
 #define MSG_SIZE 32
 
-int mpath_pr_event_handle(struct path *pp);
-void * mpath_pr_event_handler_fn (void * );
+static void mpath_pr_event_handle(struct path *pp);
 
 #define LOG_MSG(lvl, pp)					\
 do {								\
@@ -4229,17 +4228,24 @@ main (int argc, char *argv[])
 		return (child(NULL));
 }
 
-void *  mpath_pr_event_handler_fn (void * pathp )
+static void mpath_pr_event_handle(struct path *pp)
 {
 	struct multipath * mpp;
 	unsigned int i;
 	int ret, isFound;
-	struct path * pp = (struct path *)pathp;
 	struct prout_param_descriptor *param;
 	struct prin_resp *resp;
 
-	rcu_register_thread();
 	mpp = pp->mpp;
+	if (pp->bus != SYSFS_BUS_SCSI) {
+		mpp->prflag = PRFLAG_UNSET;
+		return;
+	}
+
+	if (!get_be64(mpp->reservation_key)) {
+		mpp->prflag = PRFLAG_UNSET;
+		return;
+	}
 
 	resp = mpath_alloc_prin_response(MPATH_PRIN_RKEY_SA);
 	if (!resp){
@@ -4306,38 +4312,4 @@ void *  mpath_pr_event_handler_fn (void * pathp )
 out:
 	if (resp)
 		free(resp);
-	rcu_unregister_thread();
-	return NULL;
-}
-
-int mpath_pr_event_handle(struct path *pp)
-{
-	pthread_t thread;
-	int rc;
-	pthread_attr_t attr;
-	struct multipath * mpp;
-
-	if (pp->bus != SYSFS_BUS_SCSI)
-		goto no_pr;
-
-	mpp = pp->mpp;
-
-	if (!get_be64(mpp->reservation_key))
-		goto no_pr;
-
-	pthread_attr_init(&attr);
-	pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE);
-
-	rc = pthread_create(&thread, NULL , mpath_pr_event_handler_fn, pp);
-	if (rc) {
-		condlog(0, "%s: ERROR; return code from pthread_create() is %d", pp->dev, rc);
-		return -1;
-	}
-	pthread_attr_destroy(&attr);
-	rc = pthread_join(thread, NULL);
-	return 0;
-
-no_pr:
-	pp->mpp->prflag = PRFLAG_UNSET;
-	return 0;
 }
-- 
2.48.1

[PATCH 02/15] libmpathpersist: remove uneeded wrapper function.

[Benjamin Marzinski]

mpath_send_prin_activepath() just calls prin_do_scsi_ioctl() with exactly
the same arguments that it is passed, and returns the same return. remove
it.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c | 15 ++-------------
 1 file changed, 2 insertions(+), 13 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 9ed9f5a7..612bbed9 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -49,17 +49,6 @@ struct threadinfo {
 	struct prout_param param;
 };
 
-static int mpath_send_prin_activepath (char * dev, int rq_servact,
-				struct prin_resp * resp, int noisy)
-{
-
-	int rc;
-
-	rc = prin_do_scsi_ioctl(dev, rq_servact, resp,  noisy);
-
-	return (rc);
-}
-
 static int mpath_prin_activepath (struct multipath *mpp, int rq_servact,
 	struct prin_resp * resp, int noisy)
 {
@@ -80,8 +69,8 @@ static int mpath_prin_activepath (struct multipath *mpp, int rq_servact,
 
 			condlog(3, "%s: sending pr in command to %s ",
 				mpp->wwid, pp->dev);
-			ret = mpath_send_prin_activepath(pp->dev, rq_servact,
-							 resp, noisy);
+			ret = prin_do_scsi_ioctl(pp->dev, rq_servact, resp,
+						 noisy);
 			switch(ret)
 			{
 				case MPATH_PR_SUCCESS:
-- 
2.48.1

[PATCH 03/15] libmpathpersist: reduce log level for persistent reservation checking

[Benjamin Marzinski]

Move logging of minor expected behavior to INFO level. Modify the log
level of some messages by whether or not mpp->prflag changed values.

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

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 612bbed9..4172167a 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -60,7 +60,7 @@ static int mpath_prin_activepath (struct multipath *mpp, int rq_servact,
 		vector_foreach_slot (pgp->paths, pp, i){
 			if (!((pp->state == PATH_UP) ||
 			      (pp->state == PATH_GHOST))){
-				condlog(2, "%s: %s not available. Skip.",
+				condlog(3, "%s: %s not available. Skip.",
 					mpp->wwid, pp->dev);
 				condlog(3, "%s: status = %d.",
 					mpp->wwid, pp->state);
@@ -726,13 +726,13 @@ int update_map_pr(struct multipath *mpp)
 	struct prin_resp *resp;
 	unsigned int i;
 	int ret = MPATH_PR_OTHER, isFound;
+	bool was_set = (mpp->prflag == PRFLAG_SET);
 
 	if (!get_be64(mpp->reservation_key))
 	{
 		/* Nothing to do. Assuming pr mgmt feature is disabled*/
 		mpp->prflag = PRFLAG_UNSET;
-		condlog(4, "%s: reservation_key not set in multipath.conf",
-			mpp->alias);
+		condlog(was_set ? 2 : 4, "%s: reservation_key not set in multipath.conf", mpp->alias);
 		return MPATH_PR_SUCCESS;
 	}
 
@@ -744,7 +744,7 @@ int update_map_pr(struct multipath *mpp)
 	}
 	if (count_active_paths(mpp) == 0)
 	{
-		condlog(0,"%s: No available paths to check pr status",
+		condlog(2, "%s: No available paths to check pr status",
 			mpp->alias);
 		goto out;
 	}
@@ -761,22 +761,24 @@ int update_map_pr(struct multipath *mpp)
 
 	if (resp->prin_descriptor.prin_readkeys.additional_length == 0 )
 	{
-		condlog(3,"%s: No key found. Device may not be registered. ", mpp->alias);
+		condlog(was_set ? 1 : 3, "%s: No key found. Device may not be registered. ", mpp->alias);
 		goto out;
 	}
 
-	condlog(2, "%s: Multipath  reservation_key: 0x%" PRIx64 " ", mpp->alias,
+	condlog(3, "%s: Multipath reservation_key: 0x%" PRIx64 " ", mpp->alias,
 		get_be64(mpp->reservation_key));
 
 	isFound =0;
 	for (i = 0; i < resp->prin_descriptor.prin_readkeys.additional_length/8; i++ )
 	{
-		condlog(2, "%s: PR IN READKEYS[%d]  reservation key:", mpp->alias, i);
-		dumpHex((char *)&resp->prin_descriptor.prin_readkeys.key_list[i*8], 8 , 1);
+		if (libmp_verbosity >= 3) {
+			condlog(3, "%s: PR IN READKEYS[%d] reservation key:",
+				mpp->alias, i);
+			dumpHex((char *)&resp->prin_descriptor.prin_readkeys.key_list[i * 8], 8, 1);
+		}
 
-		if (!memcmp(&mpp->reservation_key, &resp->prin_descriptor.prin_readkeys.key_list[i*8], 8))
-		{
-			condlog(2, "%s: reservation key found in pr in readkeys response", mpp->alias);
+		if (!memcmp(&mpp->reservation_key, &resp->prin_descriptor.prin_readkeys.key_list[i * 8], 8)) {
+			condlog(3, "%s: reservation key found in pr in readkeys response", mpp->alias);
 			isFound =1;
 		}
 	}
@@ -784,7 +786,7 @@ int update_map_pr(struct multipath *mpp)
 	if (isFound)
 	{
 		mpp->prflag = PRFLAG_SET;
-		condlog(2, "%s: prflag flag set.", mpp->alias );
+		condlog(was_set ? 3 : 2, "%s: prflag flag set.", mpp->alias );
 	}
 
 out:
-- 
2.48.1

[PATCH 04/15] libmpathpersist: remove pointless update_map_pr ret value code

[Benjamin Marzinski]

Don't set ret to MPATH_PR_SUCCESS, after the code just made sure that
it was already set to MPATH_PR_SUCCESS.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 4172167a..f2c81fa0 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -757,8 +757,6 @@ int update_map_pr(struct multipath *mpp)
 		goto out;
 	}
 
-	ret = MPATH_PR_SUCCESS;
-
 	if (resp->prin_descriptor.prin_readkeys.additional_length == 0 )
 	{
 		condlog(was_set ? 1 : 3, "%s: No key found. Device may not be registered. ", mpp->alias);
-- 
2.48.1

[PATCH 05/15] multipathd: use update_map_pr in mpath_pr_event_handle

[Benjamin Marzinski]

Clean up the duplicate code in mpath_pr_event_handle() and
update_map_pr() by making update_map_pr() take an optional path device
to use for its check, instead of checking all path devices and make
mpath_pr_event_handle() call update_map_pr() to do its checking.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/libmpathpersist.version |  2 +-
 libmpathpersist/mpath_persist_int.c     | 21 +++++---
 libmpathpersist/mpath_persist_int.h     |  2 +-
 multipathd/main.c                       | 68 ++++---------------------
 4 files changed, 25 insertions(+), 68 deletions(-)

diff --git a/libmpathpersist/libmpathpersist.version b/libmpathpersist/libmpathpersist.version
index eed66aa0..b31237db 100644
--- a/libmpathpersist/libmpathpersist.version
+++ b/libmpathpersist/libmpathpersist.version
@@ -34,7 +34,7 @@ global:
 	mpath_persistent_reserve_out__;
 } LIBMPATHPERSIST_2.1.0;
 
-__LIBMPATHPERSIST_INT_1.1.0 {
+__LIBMPATHPERSIST_INT_2.0.0 {
 	/* Internal use by multipath-tools */
 	dumpHex;
 	mpath_alloc_prin_response;
diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index f2c81fa0..92c3ea7e 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -720,7 +720,7 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	return ret;
 }
 
-int update_map_pr(struct multipath *mpp)
+int update_map_pr(struct multipath *mpp, struct path *pp)
 {
 	int noisy=0;
 	struct prin_resp *resp;
@@ -733,7 +733,7 @@ int update_map_pr(struct multipath *mpp)
 		/* Nothing to do. Assuming pr mgmt feature is disabled*/
 		mpp->prflag = PRFLAG_UNSET;
 		condlog(was_set ? 2 : 4, "%s: reservation_key not set in multipath.conf", mpp->alias);
-		return MPATH_PR_SUCCESS;
+		return MPATH_PR_SKIP;
 	}
 
 	resp = mpath_alloc_prin_response(MPATH_PRIN_RKEY_SA);
@@ -742,14 +742,18 @@ int update_map_pr(struct multipath *mpp)
 		condlog(0,"%s : failed to alloc resp in update_map_pr", mpp->alias);
 		return MPATH_PR_OTHER;
 	}
-	if (count_active_paths(mpp) == 0)
-	{
+	if (!pp && count_active_paths(mpp) == 0) {
 		condlog(2, "%s: No available paths to check pr status",
 			mpp->alias);
 		goto out;
 	}
 	mpp->prflag = PRFLAG_UNSET;
-	ret = mpath_prin_activepath(mpp, MPATH_PRIN_RKEY_SA, resp, noisy);
+	if (pp)
+		ret = prin_do_scsi_ioctl(pp->dev, MPATH_PRIN_RKEY_SA, resp,
+					 noisy);
+	else
+		ret = mpath_prin_activepath(mpp, MPATH_PRIN_RKEY_SA, resp,
+					    noisy);
 
 	if (ret != MPATH_PR_SUCCESS )
 	{
@@ -784,8 +788,11 @@ int update_map_pr(struct multipath *mpp)
 	if (isFound)
 	{
 		mpp->prflag = PRFLAG_SET;
-		condlog(was_set ? 3 : 2, "%s: prflag flag set.", mpp->alias );
-	}
+		condlog(was_set ? 3 : 2, "%s: key found. prflag set.",
+			mpp->alias);
+	} else
+		condlog(was_set ? 1 : 3, "%s: key not found. prflag unset.",
+			mpp->alias);
 
 out:
 	free(resp);
diff --git a/libmpathpersist/mpath_persist_int.h b/libmpathpersist/mpath_persist_int.h
index 7819823c..7b32c1e2 100644
--- a/libmpathpersist/mpath_persist_int.h
+++ b/libmpathpersist/mpath_persist_int.h
@@ -20,6 +20,6 @@ int prin_do_scsi_ioctl(char * dev, int rq_servact, struct prin_resp * resp, int
 int prout_do_scsi_ioctl( char * dev, int rq_servact, int rq_scope,
 			 unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy);
 void dumpHex(const char* , int len, int no_ascii);
-int update_map_pr(struct multipath *mpp);
+int update_map_pr(struct multipath *mpp, struct path *pp);
 
 #endif /* MPATH_PERSIST_INT_H_INCLUDED */
diff --git a/multipathd/main.c b/multipathd/main.c
index 46eef222..ace278f1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -727,7 +727,7 @@ fail:
 	sync_map_state(mpp, false);
 
 	if (mpp->prflag != PRFLAG_SET)
-		update_map_pr(mpp);
+		update_map_pr(mpp, NULL);
 	if (mpp->prflag == PRFLAG_SET)
 		pr_register_active_paths(mpp);
 
@@ -1403,7 +1403,7 @@ rescan:
 
 	if (retries >= 0) {
 		if (start_waiter)
-			update_map_pr(mpp);
+			update_map_pr(mpp, NULL);
 		if (mpp->prflag == PRFLAG_SET && prflag != PRFLAG_SET)
 				pr_register_active_paths(mpp);
 		condlog(2, "%s [%s]: path added to devmap %s",
@@ -3312,7 +3312,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);
-		update_map_pr(mpp);
+		update_map_pr(mpp, NULL);
 		if (mpp->prflag == PRFLAG_SET)
 			pr_register_active_paths(mpp);
 	}
@@ -4230,70 +4230,24 @@ main (int argc, char *argv[])
 
 static void mpath_pr_event_handle(struct path *pp)
 {
-	struct multipath * mpp;
-	unsigned int i;
-	int ret, isFound;
+	struct multipath *mpp = pp->mpp;
+	int ret;
 	struct prout_param_descriptor *param;
-	struct prin_resp *resp;
 
-	mpp = pp->mpp;
 	if (pp->bus != SYSFS_BUS_SCSI) {
 		mpp->prflag = PRFLAG_UNSET;
 		return;
 	}
 
-	if (!get_be64(mpp->reservation_key)) {
-		mpp->prflag = PRFLAG_UNSET;
+	if (update_map_pr(mpp, pp) != MPATH_PR_SUCCESS)
 		return;
-	}
-
-	resp = mpath_alloc_prin_response(MPATH_PRIN_RKEY_SA);
-	if (!resp){
-		condlog(0,"%s Alloc failed for prin response", pp->dev);
-		goto out;
-	}
-
-	mpp->prflag = PRFLAG_UNSET;
-	ret = prin_do_scsi_ioctl(pp->dev, MPATH_PRIN_RKEY_SA, resp, 0);
-	if (ret != MPATH_PR_SUCCESS )
-	{
-		condlog(0,"%s : pr in read keys service action failed. Error=%d", pp->dev, ret);
-		goto out;
-	}
-
-	condlog(3, " event pr=%d addlen=%d",resp->prin_descriptor.prin_readkeys.prgeneration,
-			resp->prin_descriptor.prin_readkeys.additional_length );
-
-	if (resp->prin_descriptor.prin_readkeys.additional_length == 0 )
-	{
-		condlog(1, "%s: No key found. Device may not be registered.", pp->dev);
-		goto out;
-	}
-	condlog(2, "Multipath  reservation_key: 0x%" PRIx64 " ",
-		get_be64(mpp->reservation_key));
 
-	isFound =0;
-	for (i = 0; i < resp->prin_descriptor.prin_readkeys.additional_length/8; i++ )
-	{
-		condlog(2, "PR IN READKEYS[%d]  reservation key:",i);
-		dumpHex((char *)&resp->prin_descriptor.prin_readkeys.key_list[i*8], 8 , -1);
-		if (!memcmp(&mpp->reservation_key, &resp->prin_descriptor.prin_readkeys.key_list[i*8], 8))
-		{
-			condlog(2, "%s: pr key found in prin readkeys response", mpp->alias);
-			isFound =1;
-			break;
-		}
-	}
-	if (!isFound)
-	{
-		condlog(0, "%s: Either device not registered or ", pp->dev);
-		condlog(0, "host is not authorised for registration. Skip path");
-		goto out;
-	}
+	if (mpp->prflag != PRFLAG_SET)
+		return;
 
 	param = (struct prout_param_descriptor *)calloc(1, sizeof(struct prout_param_descriptor));
 	if (!param)
-		goto out;
+		return;
 
 	param->sa_flags = mpp->sa_flags;
 	memcpy(param->sa_key, &mpp->reservation_key, 8);
@@ -4306,10 +4260,6 @@ static void mpath_pr_event_handle(struct path *pp)
 	{
 		condlog(0,"%s: Reservation registration failed. Error: %d", pp->dev, ret);
 	}
-	mpp->prflag = PRFLAG_SET;
 
 	free(param);
-out:
-	if (resp)
-		free(resp);
 }
-- 
2.48.1

[PATCH 06/15] libmpathpersist: limit changing prflag in update_map_pr

[Benjamin Marzinski]

Do not unset mpp->prflag in update_map_pr() unless the device doesn't
support persistent reservations or it successfully reads the registered
keys and discovers that the device's key is not there. Also, nothing in
the libmpathpersist ever returns MPATH_PR_SENSE_INVALID_OP. It instead
returns MPATH_PR_ILLEGAL_REQ if the device doesn't support persistent
reservations, so check for that instead.

Also, do not even check for the registered keys in update_map_pr() if
mpp->prflag is unset. Otherwise, multipathd will set mpp->prflag if the
key was unregistered (and thus prflag was unset) while a path is down
(and thus could not have its key unregistered) and then that path comes
back up. I should note that the above issue can only occur if multipath
is defining PR keys in /etc/multipath.conf, instead of the prkeys file.

If a device has no registered keys, it must unset prflag, since that
means it's no longer registered. Possibly its registration was removed
by a CLEAR or PREEMPT action. But if a device has a registered key but
it is supposed to be unregistered, it should remain unregistered, since
that key is likely one that libmpathpersist could not unregister at the
time.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 92c3ea7e..e312d577 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -74,7 +74,7 @@ static int mpath_prin_activepath (struct multipath *mpp, int rq_servact,
 			switch(ret)
 			{
 				case MPATH_PR_SUCCESS:
-				case MPATH_PR_SENSE_INVALID_OP:
+				case MPATH_PR_ILLEGAL_REQ:
 					return ret;
 				default:
 					continue;
@@ -728,6 +728,10 @@ int update_map_pr(struct multipath *mpp, struct path *pp)
 	int ret = MPATH_PR_OTHER, isFound;
 	bool was_set = (mpp->prflag == PRFLAG_SET);
 
+	/* If pr is explicitly unset, it must be manually set */
+	if (mpp->prflag == PRFLAG_UNSET)
+		return MPATH_PR_SKIP;
+
 	if (!get_be64(mpp->reservation_key))
 	{
 		/* Nothing to do. Assuming pr mgmt feature is disabled*/
@@ -747,7 +751,6 @@ int update_map_pr(struct multipath *mpp, struct path *pp)
 			mpp->alias);
 		goto out;
 	}
-	mpp->prflag = PRFLAG_UNSET;
 	if (pp)
 		ret = prin_do_scsi_ioctl(pp->dev, MPATH_PRIN_RKEY_SA, resp,
 					 noisy);
@@ -757,9 +760,12 @@ int update_map_pr(struct multipath *mpp, struct path *pp)
 
 	if (ret != MPATH_PR_SUCCESS )
 	{
+		if (ret == MPATH_PR_ILLEGAL_REQ)
+			mpp->prflag = PRFLAG_UNSET;
 		condlog(0,"%s : pr in read keys service action failed Error=%d", mpp->alias, ret);
 		goto out;
 	}
+	mpp->prflag = PRFLAG_UNSET;
 
 	if (resp->prin_descriptor.prin_readkeys.additional_length == 0 )
 	{
-- 
2.48.1

[PATCH 07/15] multipathd: Don't call update_map_pr unnecessarily

[Benjamin Marzinski]

None of the calls to update_map_pr() outside of mpath_pr_event_handle()
add any benefit. When update_map_pr() is called without a path, it tries
to read the pr keys list on each usable path until it succeeds, and then
checks the keys to see if they include the configured key.

In all cases where update_map_pr() is called outside of
mpath_pr_event_handle(), after it is called, pr_register_active_paths()
is called if a matching key was found. pr_register_active_paths() calls
mpath_pr_event_handle() on each usable path, which calls update_map_pr()
with a path, so it only checks that path. If a matching key is found, it
registers a key on the current path. The result is that after
pr_register_active_paths() is called, update_map_pr() will be called for
each usable path, just like update_map_pr() did. So calling
update_map_pr() first doesn't change the results for multipathd, it just
adds duplicate work, so remove those calls.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index ace278f1..02cfe8d1 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -637,6 +637,8 @@ pr_register_active_paths(struct multipath *mpp)
 
 	vector_foreach_slot (mpp->pg, pgp, i) {
 		vector_foreach_slot (pgp->paths, pp, j) {
+			if (mpp->prflag == PRFLAG_UNSET)
+				return;
 			if ((pp->state == PATH_UP) || (pp->state == PATH_GHOST))
 				mpath_pr_event_handle(pp);
 		}
@@ -726,10 +728,7 @@ fail:
 
 	sync_map_state(mpp, false);
 
-	if (mpp->prflag != PRFLAG_SET)
-		update_map_pr(mpp, NULL);
-	if (mpp->prflag == PRFLAG_SET)
-		pr_register_active_paths(mpp);
+	pr_register_active_paths(mpp);
 
 	if (VECTOR_SIZE(offline_paths) != 0)
 		handle_orphaned_offline_paths(offline_paths);
@@ -1402,10 +1401,9 @@ rescan:
 	sync_map_state(mpp, false);
 
 	if (retries >= 0) {
-		if (start_waiter)
-			update_map_pr(mpp, NULL);
-		if (mpp->prflag == PRFLAG_SET && prflag != PRFLAG_SET)
-				pr_register_active_paths(mpp);
+		if ((mpp->prflag == PRFLAG_SET && prflag != PRFLAG_SET) ||
+		    start_waiter)
+			pr_register_active_paths(mpp);
 		condlog(2, "%s [%s]: path added to devmap %s",
 			pp->dev, pp->dev_t, mpp->alias);
 		return 0;
@@ -3312,9 +3310,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);
-		update_map_pr(mpp, NULL);
-		if (mpp->prflag == PRFLAG_SET)
-			pr_register_active_paths(mpp);
+		pr_register_active_paths(mpp);
 	}
 
 	/*
-- 
2.48.1

[PATCH 08/15] libmpathpersist: remove useless function send_prout_activepath

[Benjamin Marzinski]

send_prout_activepath() creates a single separate thread that just calls
prout_do_scsi_ioctl() and it doesn't take any advantage of the work
being done in another thread. Remove the function and call
prout_do_scsi_ioctl() directly.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c | 39 ++---------------------------
 1 file changed, 2 insertions(+), 37 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index e312d577..13829742 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -364,40 +364,6 @@ static int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 	return (status == MPATH_PR_RETRYABLE_ERROR) ? MPATH_PR_OTHER : status;
 }
 
-static int send_prout_activepath(char *dev, int rq_servact, int rq_scope,
-				 unsigned int rq_type,
-				 struct prout_param_descriptor * paramp, int noisy)
-{
-	struct prout_param param;
-	param.rq_servact = rq_servact;
-	param.rq_scope  = rq_scope;
-	param.rq_type   = rq_type;
-	param.paramp    = paramp;
-	param.noisy = noisy;
-	param.status = -1;
-
-	pthread_t thread;
-	pthread_attr_t attr;
-	int rc;
-
-	memset(&thread, 0, sizeof(thread));
-	strlcpy(param.dev, dev, FILE_NAME_SIZE);
-	/* Initialize and set thread joinable attribute */
-	pthread_attr_init(&attr);
-	pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_JOINABLE);
-
-	rc = pthread_create(&thread, &attr, mpath_prout_pthread_fn, (void *)(&param));
-	if (rc){
-		condlog (3, "%s: failed to create thread %d", dev, rc);
-		return MPATH_PR_THREAD_ERROR;
-	}
-	/* Free attribute and wait for the other threads */
-	pthread_attr_destroy(&attr);
-	rc = pthread_join(thread, NULL);
-
-	return (param.status);
-}
-
 static int mpath_prout_common(struct multipath *mpp,int rq_servact, int rq_scope,
 			      unsigned int rq_type,
 			      struct prout_param_descriptor* paramp, int noisy)
@@ -417,9 +383,8 @@ static int mpath_prout_common(struct multipath *mpp,int rq_servact, int rq_scope
 
 			condlog (3, "%s: sending pr out command to %s", mpp->wwid, pp->dev);
 			found = true;
-			ret = send_prout_activepath(pp->dev, rq_servact,
-						    rq_scope, rq_type,
-						    paramp, noisy);
+			ret = prout_do_scsi_ioctl(pp->dev, rq_servact, rq_scope,
+						  rq_type, paramp, noisy);
 			if (ret != MPATH_PR_RETRYABLE_ERROR)
 				return ret;
 		}
-- 
2.48.1

[PATCH 09/15] limpathpersist: redesign failed release workaround

[Benjamin Marzinski]

The workaround for releasing a reservation held by a failed path
(clearing all persistent reservation data and then reregistering all the
keys) has several problems. It requires devices that support the READ
FULL STATUS command and are capable of specifying TransportIDs with
REGISTRATION commands (SIP_C), neither of which are mandatory to support
SCSI Persistent Reservations. Non SIP_C devices will be left with no
registered keys. Also, not all cleared keys are registered, just the
ones going to the Target Port receiving the REGISTRATION command. To
reregister all the keys, the code would have to also use the "All Target
Ports" flag to register keys on different Target Ports, but this could
end up registering keys on paths they aren't supposed to be on (for
instance if one of the registered keys was only there because the path
was down and it couldn't be released).

The redesign avoids these issues by only using mandatory Persistent
Reservation commands, without extra optional parameters or flags, and
only effects the keys of the multipath device it is being issued on.
The new workaround is:
1. Suspend the multipath device to prevent I/O
2. Preempt the key to move the reservation to an available path. This
   also removes the registered keys from every path except the path
   issuing the PREEMPT command. Since the device is suspended, not I/O
   can go to these unregisted paths and fail.
3. Release the reservation on the path that now holds it.
4. Resume the device (since it no longer matters that most of the paths
   no longer have a registered key)
5. Reregister the keys on all the paths.

If steps 3 or 4 fail, the code will attempt to reregister the keys, and
then attempt (or possibly re-attempt) the resume.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c | 173 ++++++++++++----------------
 libmultipath/libmultipath.version   |   1 +
 2 files changed, 76 insertions(+), 98 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 13829742..7fb08b2e 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -366,7 +366,8 @@ static int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 
 static int mpath_prout_common(struct multipath *mpp,int rq_servact, int rq_scope,
 			      unsigned int rq_type,
-			      struct prout_param_descriptor* paramp, int noisy)
+			      struct prout_param_descriptor* paramp, int noisy,
+			      struct path **pptr)
 {
 	int i,j, ret;
 	struct pathgroup *pgp = NULL;
@@ -385,6 +386,8 @@ static int mpath_prout_common(struct multipath *mpp,int rq_servact, int rq_scope
 			found = true;
 			ret = prout_do_scsi_ioctl(pp->dev, rq_servact, rq_scope,
 						  rq_type, paramp, noisy);
+			if (ret == MPATH_PR_SUCCESS && pptr)
+				*pptr = pp;
 			if (ret != MPATH_PR_RETRYABLE_ERROR)
 				return ret;
 		}
@@ -405,14 +408,12 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 	struct path *pp = NULL;
 	int active_pathcount = 0;
 	pthread_attr_t attr;
-	int rc, found = 0;
+	int rc;
 	int count = 0;
 	int status = MPATH_PR_SUCCESS;
-	struct prin_resp resp;
-	struct prout_param_descriptor *pamp = NULL;
-	struct prin_resp *pr_buff;
-	int length;
-	struct transportid *pptr = NULL;
+	struct prin_resp resp = {0};
+	uint16_t udev_flags = (mpp->skip_kpartx) ? MPATH_UDEV_NO_KPARTX_FLAG : 0;
+	bool did_resume = false;
 
 	if (!mpp)
 		return MPATH_PR_DMMP_ERROR;
@@ -502,104 +503,78 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 	}
 
 	condlog (2, "%s: Path holding reservation is not available.", mpp->wwid);
-
-	pr_buff =  mpath_alloc_prin_response(MPATH_PRIN_RFSTAT_SA);
-	if (!pr_buff){
-		condlog (0, "%s: failed to  alloc pr in response buffer.", mpp->wwid);
+	/*
+	 * Cannot free the reservation because the path that is holding it
+	 * is not usable. Workaround this by:
+	 * 1. Suspending the device
+	 * 2. Preempting the reservation to move it to a usable path
+	 *    (this removes the registered keys on all paths except the
+	 *    preempting one. Since the device is suspended, no IO can
+	 *    go to these unregistered paths and fail).
+	 * 3. Releasing the reservation on the path that now holds it.
+	 * 4. Resuming the device (since it no longer matters that most of
+	 *    that paths no longer have a registered key)
+	 * 5. Reregistering keys on all the paths
+	 */
+
+	if (!dm_simplecmd_noflush(DM_DEVICE_SUSPEND, mpp->alias, 0)) {
+		condlog(0, "%s: release: failed to suspend dm device.", mpp->wwid);
 		return MPATH_PR_OTHER;
 	}
 
-	status = mpath_prin_activepath (mpp, MPATH_PRIN_RFSTAT_SA, pr_buff, noisy);
-
-	if (status != MPATH_PR_SUCCESS){
-		condlog (0,  "%s: pr in read full status command failed.",  mpp->wwid);
-		goto out;
-	}
-
-	num = pr_buff->prin_descriptor.prin_readfd.number_of_descriptor;
-	if (0 == num){
-		goto out;
-	}
-	length = sizeof (struct prout_param_descriptor) + (sizeof (struct transportid *));
-
-	pamp = (struct prout_param_descriptor *)malloc (length);
-	if (!pamp){
-		condlog (0, "%s: failed to alloc pr out parameter.", mpp->wwid);
-		goto out;
-	}
-
-	memset(pamp, 0, length);
-
-	pamp->trnptid_list[0] = (struct transportid *) malloc (sizeof (struct transportid));
-	if (!pamp->trnptid_list[0]){
-		condlog (0, "%s: failed to alloc pr out transportid.", mpp->wwid);
-		goto out1;
-	}
-	pptr = pamp->trnptid_list[0];
-
-	if (get_be64(mpp->reservation_key)){
-		memcpy (pamp->key, &mpp->reservation_key, 8);
-		condlog (3, "%s: reservation key set.", mpp->wwid);
+	memset(paramp, 0, sizeof(*paramp));
+	memcpy(paramp->key, &mpp->reservation_key, 8);
+	memcpy(paramp->sa_key, &mpp->reservation_key, 8);
+	status = mpath_prout_common(mpp, MPATH_PROUT_PREE_SA, rq_scope, rq_type,
+				    paramp, noisy, &pp);
+	if (status != MPATH_PR_SUCCESS) {
+		condlog(0, "%s: release: pr preempt command failed.", mpp->wwid);
+		goto fail_resume;
 	}
 
-	status = mpath_prout_common (mpp, MPATH_PROUT_CLEAR_SA,
-				     rq_scope, rq_type, pamp, noisy);
-
-	if (status) {
-		condlog(0, "%s: failed to send CLEAR_SA", mpp->wwid);
-		goto out2;
+	memset(paramp, 0, sizeof(*paramp));
+	memcpy(paramp->key, &mpp->reservation_key, 8);
+	status = prout_do_scsi_ioctl(pp->dev, MPATH_PROUT_REL_SA, rq_scope,
+				     rq_type, paramp, noisy);
+	if (status != MPATH_PR_SUCCESS) {
+		condlog(0, "%s: release on alternate path failed.", mpp->wwid);
+		goto out_reregister;
 	}
 
-	pamp->num_transportid = 1;
-
-	for (i = 0; i < num; i++){
-		if (get_be64(mpp->reservation_key) &&
-			memcmp(pr_buff->prin_descriptor.prin_readfd.descriptors[i]->key,
-			       &mpp->reservation_key, 8)){
-			/*register with transport id*/
-			memset(pamp, 0, length);
-			pamp->trnptid_list[0] = pptr;
-			memset (pamp->trnptid_list[0], 0, sizeof (struct transportid));
-			memcpy (pamp->sa_key,
-					pr_buff->prin_descriptor.prin_readfd.descriptors[i]->key, 8);
-			pamp->sa_flags = MPATH_F_SPEC_I_PT_MASK;
-			pamp->num_transportid = 1;
-
-			memcpy (pamp->trnptid_list[0],
-					&pr_buff->prin_descriptor.prin_readfd.descriptors[i]->trnptid,
-					sizeof (struct transportid));
-			status = mpath_prout_common (mpp, MPATH_PROUT_REG_SA, 0, rq_type,
-					pamp, noisy);
-
-			pamp->sa_flags = 0;
-			memcpy (pamp->key, pr_buff->prin_descriptor.prin_readfd.descriptors[i]->key, 8);
-			memset (pamp->sa_key, 0, 8);
-			pamp->num_transportid = 0;
-			status = mpath_prout_common (mpp, MPATH_PROUT_REG_SA, 0, rq_type,
-					pamp, noisy);
-		}
-		else
-		{
-			if (get_be64(mpp->reservation_key))
-				found = 1;
-		}
-
-
-	}
-
-	if (found){
-		memset (pamp, 0, length);
-		memcpy (pamp->sa_key, &mpp->reservation_key, 8);
-		memset (pamp->key, 0, 8);
-		status = mpath_prout_reg(mpp, MPATH_PROUT_REG_SA, rq_scope, rq_type, pamp, noisy);
+	if (!dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias, udev_flags)) {
+		condlog(0, "%s release: failed to resume dm device.", mpp->wwid);
+		/*
+		 * leave status set to MPATH_PR_SUCCESS, we will have another
+		 * chance to resume the device.
+		 */
+		goto out_reregister;
+	}
+	did_resume = true;
+
+out_reregister:
+	memset(paramp, 0, sizeof(*paramp));
+	memcpy(paramp->sa_key, &mpp->reservation_key, 8);
+	rc = mpath_prout_reg(mpp, MPATH_PROUT_REG_IGN_SA, rq_scope, rq_type,
+			     paramp, noisy);
+	if (rc != MPATH_PR_SUCCESS)
+		condlog(0, "%s: release: failed to reregister paths.", mpp->wwid);
+
+	/*
+	 * If we failed releasing the reservation or resuming earlier
+	 * try resuming now. Otherwise, return with the reregistering status
+	 * This means we will report failure, even though the resevation
+	 * has been released, since the keys were not reregistered.
+	 */
+	if (did_resume)
+		return rc;
+	else if (status == MPATH_PR_SUCCESS)
+		status = rc;
+fail_resume:
+	if (!dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias, udev_flags)) {
+		condlog(0, "%s: release: failed to resume dm device.", mpp->wwid);
+		if (status == MPATH_PR_SUCCESS)
+			status = MPATH_PR_OTHER;
 	}
-
-out2:
-	free(pptr);
-out1:
-	free (pamp);
-out:
-	free (pr_buff);
 	return (status == MPATH_PR_RETRYABLE_ERROR) ? MPATH_PR_OTHER : status;
 }
 
@@ -620,6 +595,7 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	mpp->mpe = find_mpe(conf->mptable, mpp->wwid);
 	select_reservation_key(conf, mpp);
 	select_all_tg_pt(conf, mpp);
+	select_skip_kpartx(conf, mpp);
 	put_multipath_config(conf);
 
 	memcpy(&prkey, paramp->sa_key, 8);
@@ -661,7 +637,8 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	case MPATH_PROUT_PREE_SA :
 	case MPATH_PROUT_PREE_AB_SA :
 	case MPATH_PROUT_CLEAR_SA:
-		ret = mpath_prout_common(mpp, rq_servact, rq_scope, rq_type, paramp, noisy);
+		ret = mpath_prout_common(mpp, rq_servact, rq_scope, rq_type,
+					 paramp, noisy, NULL);
 		break;
 	case MPATH_PROUT_REL_SA:
 		ret = mpath_prout_rel(mpp, rq_servact, rq_scope, rq_type, paramp, noisy);
diff --git a/libmultipath/libmultipath.version b/libmultipath/libmultipath.version
index a6718355..2f47b3ad 100644
--- a/libmultipath/libmultipath.version
+++ b/libmultipath/libmultipath.version
@@ -249,4 +249,5 @@ local:
 LIBMULTIPATH_29.1.0 {
 global:
 	can_recheck_wwid;
+	select_skip_kpartx;
 } LIBMULTIPATH_29.0.0;
-- 
2.48.1

[PATCH 10/15] libmpathpersist: fail the release if all threads fail

[Benjamin Marzinski]

If none of the threads succeeds in issuing the release, simply return
failure, instead of trying the workaround.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 7fb08b2e..ad5a4ee7 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -475,15 +475,21 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 		}
 	}
 
+	rc = MPATH_PR_DMMP_ERROR;
 	for (i = 0; i < count; i++){
 		/*  check thread status here and return the status */
 
-		if (thread[i].param.status == MPATH_PR_RESERV_CONFLICT)
+		if (thread[i].param.status == MPATH_PR_SUCCESS)
+			rc = MPATH_PR_SUCCESS;
+		else if (thread[i].param.status == MPATH_PR_RESERV_CONFLICT)
 			status = MPATH_PR_RESERV_CONFLICT;
-		else if (status == MPATH_PR_SUCCESS
-				&& thread[i].param.status != MPATH_PR_RESERV_CONFLICT)
+		else if (status == MPATH_PR_SUCCESS)
 			status = thread[i].param.status;
 	}
+	if (rc != MPATH_PR_SUCCESS) {
+		condlog(0, "%s: all threads failed to release reservation.", mpp->wwid);
+		return status;
+	}
 
 	status = mpath_prin_activepath (mpp, MPATH_PRIN_RRES_SA, &resp, noisy);
 	if (status != MPATH_PR_SUCCESS){
-- 
2.48.1

[PATCH 11/15] limpathpersist: Handle changing key corner case

[Benjamin Marzinski]

When you change the reservation key of a registered multipath device,
some of paths might be down or even deleted since you originally
registered the key. libmpathpersist won't be able to change these
registrations. This can be a problem if the path that is down is holding
the reservation. Other nodes will assume that the reservation is now
under your new key, when it is still under the old one. If they try to
preempt the reservation, they will succeed. But they will just
unregister all the paths with the new key. They won't take over the
reservation, and if the path holding it comes back up, it will still be
able to write to the device.

To avoid this, after libmpathpersist changes the key of a registered
device, it now checks to see if its old key is still holding the
reservation. If it is, limpathpersist preempts the reservation.
Currently this only works on REGISTER commands, not REGISTER AND IGNORE
ones. A future patch will add that capability. This patch also moves
mpath_prout_common() up, without changing it at all.

I should note that this relies on the fact that libmpathpersist has
never worked correctly if two nodes use the same reservation key for the
same device. If another node used the same key, it could be the one
holding the reservation, and preempting the reservation would deregister
it. However, multipathd has always relied on the fact that two nodes
will not use the same registration key to know if it is safe to register
a key on a path that was just added or just came back up. If there is
already a registered key matching the configured key, multipathd assumes
that the device has not been preempted, and registers the key on the
path. If there are no keys matching the configured key, multipathd
assumes that the device has been preempted, and won't register a key.
Again, if another node shared the same key, multipathd could registered
paths on a node that had been preempted.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c | 124 ++++++++++++++++++++--------
 1 file changed, 90 insertions(+), 34 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index ad5a4ee7..ca972c2b 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -221,6 +221,94 @@ static void *mpath_prout_pthread_fn(void *p)
 	pthread_exit(NULL);
 }
 
+static int mpath_prout_common(struct multipath *mpp,int rq_servact, int rq_scope,
+			      unsigned int rq_type,
+			      struct prout_param_descriptor* paramp, int noisy,
+			      struct path **pptr)
+{
+	int i,j, ret;
+	struct pathgroup *pgp = NULL;
+	struct path *pp = NULL;
+	bool found = false;
+
+	vector_foreach_slot (mpp->pg, pgp, j){
+		vector_foreach_slot (pgp->paths, pp, i){
+			if (!((pp->state == PATH_UP) || (pp->state == PATH_GHOST))){
+				condlog (1, "%s: %s path not up. Skip",
+					 mpp->wwid, pp->dev);
+				continue;
+			}
+
+			condlog (3, "%s: sending pr out command to %s", mpp->wwid, pp->dev);
+			found = true;
+			ret = prout_do_scsi_ioctl(pp->dev, rq_servact, rq_scope,
+						  rq_type, paramp, noisy);
+			if (ret == MPATH_PR_SUCCESS && pptr)
+				*pptr = pp;
+			if (ret != MPATH_PR_RETRYABLE_ERROR)
+				return ret;
+		}
+	}
+	if (found)
+		return MPATH_PR_OTHER;
+	condlog (0, "%s: no path available", mpp->wwid);
+	return MPATH_PR_DMMP_ERROR;
+}
+
+/*
+ * If you are changing the key registered to a device, and that device is
+ * holding the reservation on a path that couldn't get its key updated,
+ * either because it is down or no longer part of the multipath device,
+ * you need to preempt the reservation to a usable path with the new key
+ */
+void preempt_missing_path(struct multipath *mpp, uint8_t *key, uint8_t *sa_key,
+			  int noisy)
+{
+	uint8_t zero[8] = {0};
+	struct prin_resp resp = {0};
+	int rq_scope;
+	unsigned int rq_type;
+	struct prout_param_descriptor paramp = {0};
+	int status;
+
+	/*
+	 * If you previously didn't have a key registered or you didn't
+	 * switch to a different key, there's no need to preempt. Also, you
+	 * can't preempt if you no longer have a registered key
+	 */
+	if (memcmp(key, zero, 8) == 0 || memcmp(sa_key, zero, 8) == 0 ||
+	    memcmp(key, sa_key, 8) == 0)
+		return;
+
+	status = mpath_prin_activepath(mpp, MPATH_PRIN_RRES_SA, &resp, noisy);
+	if (status != MPATH_PR_SUCCESS) {
+		condlog(0, "%s: register: pr in read reservation command failed.", mpp->wwid);
+		return;
+	}
+	/* If there is no reservation, there's nothing to preempt */
+	if (!resp.prin_descriptor.prin_readresv.additional_length)
+		return;
+	/*
+	 * If there reservation is not held by the old key, you don't
+	 * want to preempt it
+	 */
+	if (memcmp(key, resp.prin_descriptor.prin_readresv.key, 8) != 0)
+		return;
+	/* Assume this key is being held by an inaccessable path on this
+	 * node. libmpathpersist has never worked if multiple nodes share
+	 * the same reservation key for a device
+	 */
+	rq_type = resp.prin_descriptor.prin_readresv.scope_type & MPATH_PR_TYPE_MASK;
+	rq_scope = (resp.prin_descriptor.prin_readresv.scope_type & MPATH_PR_SCOPE_MASK) >> 4;
+	memcpy(paramp.key, sa_key, 8);
+	memcpy(paramp.sa_key, key, 8);
+	status = mpath_prout_common(mpp, MPATH_PROUT_PREE_SA, rq_scope, rq_type,
+				    &paramp, noisy, NULL);
+	if (status != MPATH_PR_SUCCESS)
+		condlog(0, "%s: register: pr preempt command failed.",
+			mpp->wwid);
+}
+
 static int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 			   unsigned int rq_type,
 			   struct prout_param_descriptor * paramp, int noisy)
@@ -361,43 +449,11 @@ static int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 	}
 
 	pthread_attr_destroy(&attr);
+	if (status == MPATH_PR_SUCCESS)
+		preempt_missing_path(mpp, paramp->key, paramp->sa_key, noisy);
 	return (status == MPATH_PR_RETRYABLE_ERROR) ? MPATH_PR_OTHER : status;
 }
 
-static int mpath_prout_common(struct multipath *mpp,int rq_servact, int rq_scope,
-			      unsigned int rq_type,
-			      struct prout_param_descriptor* paramp, int noisy,
-			      struct path **pptr)
-{
-	int i,j, ret;
-	struct pathgroup *pgp = NULL;
-	struct path *pp = NULL;
-	bool found = false;
-
-	vector_foreach_slot (mpp->pg, pgp, j){
-		vector_foreach_slot (pgp->paths, pp, i){
-			if (!((pp->state == PATH_UP) || (pp->state == PATH_GHOST))){
-				condlog (1, "%s: %s path not up. Skip",
-					 mpp->wwid, pp->dev);
-				continue;
-			}
-
-			condlog (3, "%s: sending pr out command to %s", mpp->wwid, pp->dev);
-			found = true;
-			ret = prout_do_scsi_ioctl(pp->dev, rq_servact, rq_scope,
-						  rq_type, paramp, noisy);
-			if (ret == MPATH_PR_SUCCESS && pptr)
-				*pptr = pp;
-			if (ret != MPATH_PR_RETRYABLE_ERROR)
-				return ret;
-		}
-	}
-	if (found)
-		return MPATH_PR_OTHER;
-	condlog (0, "%s: no path available", mpp->wwid);
-	return MPATH_PR_DMMP_ERROR;
-}
-
 static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 			   unsigned int rq_type,
 			   struct prout_param_descriptor * paramp, int noisy)
-- 
2.48.1

[PATCH 12/15] libmapthpersist: Handle REGISTER AND IGNORE changing key corner case

[Benjamin Marzinski]

When you use REGISTER to change the reservation key of a registered
multipath device, where the path holding the reservation is down,
libmpathpersist will PREEMPT the failed path to move the reservation to
a usable path, so the reservation key will be updated. However, when you
use REGISTER AND IGNORE, you don't pass in the old key, so
libmpathpersist can't use it to check against the key holding the
reservation, which is necessary to know if it needs to PREEMPT.

Since the SCSI spec says that devices must ignore any passed-in key on
REGISTER AND IGNORE, libmpathpersist can still use it to pass in the old
key value. But unlike with REGISTER, the command won't fail if device
isn't actually registered with the old key, so libmpathpersist needs to
check that itself. To do this, it calls update_map_pr() just like
multipathd does to check that the device is registered before
registering new paths.  However, libmpathpersist doesn't track the
persistent reservation state like multipathd, so it needs to ask
multipathd if it thinks the device is registered, using the "get
prstatus map <map>" command.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c | 20 ++++++++++
 libmpathpersist/mpath_updatepr.c    | 59 +++++++++++++++++++++--------
 libmpathpersist/mpathpr.h           |  1 +
 3 files changed, 65 insertions(+), 15 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index ca972c2b..ee9706c6 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -640,6 +640,23 @@ fail_resume:
 	return (status == MPATH_PR_RETRYABLE_ERROR) ? MPATH_PR_OTHER : status;
 }
 
+/*
+ * for MPATH_PROUT_REG_IGN_SA, we use the ignored paramp->key to store the
+ * currently registered key.
+ */
+static void set_ignored_key(struct multipath *mpp, uint8_t *key)
+{
+	memset(key, 0, 8);
+	if (!get_be64(mpp->reservation_key))
+		return;
+	if (get_prflag(mpp->alias) == PRFLAG_UNSET)
+		return;
+	update_map_pr(mpp, NULL);
+	if (mpp->prflag != PRFLAG_SET)
+		return;
+	memcpy(key, &mpp->reservation_key, 8);
+}
+
 int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 				    int rq_servact, int rq_scope, unsigned int rq_type,
 				    struct prout_param_descriptor *paramp, int noisy)
@@ -660,6 +677,9 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	select_skip_kpartx(conf, mpp);
 	put_multipath_config(conf);
 
+	if (rq_servact == MPATH_PROUT_REG_IGN_SA)
+		set_ignored_key(mpp, paramp->key);
+
 	memcpy(&prkey, paramp->sa_key, 8);
 	if (mpp->prkey_source == PRKEY_SOURCE_FILE && prkey &&
 	    (rq_servact == MPATH_PROUT_REG_IGN_SA ||
diff --git a/libmpathpersist/mpath_updatepr.c b/libmpathpersist/mpath_updatepr.c
index 36bd777e..7cf82f50 100644
--- a/libmpathpersist/mpath_updatepr.c
+++ b/libmpathpersist/mpath_updatepr.c
@@ -19,14 +19,12 @@
 #include "config.h"
 #include "uxsock.h"
 #include "mpathpr.h"
+#include "structs.h"
 
-
-static int do_update_pr(char *alias, char *cmd, char *key)
+static char *do_pr(char *alias, char *str)
 {
 	int fd;
-	char str[256];
 	char *reply;
-	int ret = 0;
 	int timeout;
 	struct config *conf;
 
@@ -37,24 +35,35 @@ static int do_update_pr(char *alias, char *cmd, char *key)
 	fd = mpath_connect();
 	if (fd == -1) {
 		condlog (0, "ux socket connect error");
-		return -1;
+		return NULL;
 	}
 
-	if (key)
-		snprintf(str,sizeof(str),"%s map %s key %s", cmd, alias, key);
-	else
-		snprintf(str,sizeof(str),"%s map %s", cmd, alias);
 	condlog (2, "%s: pr message=%s", alias, str);
 	if (send_packet(fd, str) != 0) {
 		condlog(2, "%s: message=%s send error=%d", alias, str, errno);
 		mpath_disconnect(fd);
-		return -1;
+		return NULL;
 	}
-	ret = recv_packet(fd, &reply, timeout);
-	if (ret < 0) {
+	if (recv_packet(fd, &reply, timeout) < 0)
 		condlog(2, "%s: message=%s recv error=%d", alias, str, errno);
-		ret = -1;
-	} else {
+
+	mpath_disconnect(fd);
+	return reply;
+}
+
+static int do_update_pr(char *alias, char *cmd, char *key)
+{
+	char str[256];
+	char *reply = NULL;
+	int ret = -1;
+
+	if (key)
+		snprintf(str, sizeof(str), "%s map %s key %s", cmd, alias, key);
+	else
+		snprintf(str, sizeof(str), "%s map %s", cmd, alias);
+
+	reply = do_pr(alias, str);
+	if (reply) {
 		condlog (2, "%s: message=%s reply=%s", alias, str, reply);
 		if (reply && strncmp(reply,"ok", 2) == 0)
 			ret = 0;
@@ -63,10 +72,30 @@ static int do_update_pr(char *alias, char *cmd, char *key)
 	}
 
 	free(reply);
-	mpath_disconnect(fd);
 	return ret;
 }
 
+int get_prflag(char *mapname)
+{
+	char str[256];
+	char *reply;
+	int prflag;
+
+	snprintf(str, sizeof(str), "getprstatus map %s", mapname);
+	reply = do_pr(mapname, str);
+	if (!reply)
+		prflag = PRFLAG_UNKNOWN;
+	else if (strncmp(reply, "unset", 5) == 0)
+		prflag = PRFLAG_UNSET;
+	else if (strncmp(reply, "set", 3) == 0)
+		prflag = PRFLAG_SET;
+	else
+		prflag = PRFLAG_UNKNOWN;
+
+	free(reply);
+	return prflag;
+}
+
 int update_prflag(char *mapname, int set) {
 	return do_update_pr(mapname, (set)? "setprstatus" : "unsetprstatus",
 			    NULL);
diff --git a/libmpathpersist/mpathpr.h b/libmpathpersist/mpathpr.h
index cc1a5673..912957e5 100644
--- a/libmpathpersist/mpathpr.h
+++ b/libmpathpersist/mpathpr.h
@@ -8,6 +8,7 @@
 
 int update_prflag(char *mapname, int set);
 int update_prkey_flags(char *mapname, uint64_t prkey, uint8_t sa_flags);
+int get_prflag(char *mapname);
 #define update_prkey(mapname, prkey) update_prkey_flags(mapname, prkey, 0)
 
 #endif
-- 
2.48.1

[PATCH 13/15] libmultipath: rename prflag_value enums

[Benjamin Marzinski]

These will also be used by another variable, so make their name more
generic.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c | 16 ++++++++--------
 libmpathpersist/mpath_updatepr.c    |  8 ++++----
 libmultipath/structs.h              |  9 ++++-----
 multipathd/cli_handlers.c           | 22 +++++++++++-----------
 multipathd/main.c                   | 16 +++++++---------
 5 files changed, 34 insertions(+), 37 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index ee9706c6..1bb62531 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -649,10 +649,10 @@ static void set_ignored_key(struct multipath *mpp, uint8_t *key)
 	memset(key, 0, 8);
 	if (!get_be64(mpp->reservation_key))
 		return;
-	if (get_prflag(mpp->alias) == PRFLAG_UNSET)
+	if (get_prflag(mpp->alias) == PR_UNSET)
 		return;
 	update_map_pr(mpp, NULL);
-	if (mpp->prflag != PRFLAG_SET)
+	if (mpp->prflag != PR_SET)
 		return;
 	memcpy(key, &mpp->reservation_key, 8);
 }
@@ -750,16 +750,16 @@ int update_map_pr(struct multipath *mpp, struct path *pp)
 	struct prin_resp *resp;
 	unsigned int i;
 	int ret = MPATH_PR_OTHER, isFound;
-	bool was_set = (mpp->prflag == PRFLAG_SET);
+	bool was_set = (mpp->prflag == PR_SET);
 
 	/* If pr is explicitly unset, it must be manually set */
-	if (mpp->prflag == PRFLAG_UNSET)
+	if (mpp->prflag == PR_UNSET)
 		return MPATH_PR_SKIP;
 
 	if (!get_be64(mpp->reservation_key))
 	{
 		/* Nothing to do. Assuming pr mgmt feature is disabled*/
-		mpp->prflag = PRFLAG_UNSET;
+		mpp->prflag = PR_UNSET;
 		condlog(was_set ? 2 : 4, "%s: reservation_key not set in multipath.conf", mpp->alias);
 		return MPATH_PR_SKIP;
 	}
@@ -785,11 +785,11 @@ int update_map_pr(struct multipath *mpp, struct path *pp)
 	if (ret != MPATH_PR_SUCCESS )
 	{
 		if (ret == MPATH_PR_ILLEGAL_REQ)
-			mpp->prflag = PRFLAG_UNSET;
+			mpp->prflag = PR_UNSET;
 		condlog(0,"%s : pr in read keys service action failed Error=%d", mpp->alias, ret);
 		goto out;
 	}
-	mpp->prflag = PRFLAG_UNSET;
+	mpp->prflag = PR_UNSET;
 
 	if (resp->prin_descriptor.prin_readkeys.additional_length == 0 )
 	{
@@ -817,7 +817,7 @@ int update_map_pr(struct multipath *mpp, struct path *pp)
 
 	if (isFound)
 	{
-		mpp->prflag = PRFLAG_SET;
+		mpp->prflag = PR_SET;
 		condlog(was_set ? 3 : 2, "%s: key found. prflag set.",
 			mpp->alias);
 	} else
diff --git a/libmpathpersist/mpath_updatepr.c b/libmpathpersist/mpath_updatepr.c
index 7cf82f50..c8fbe4a2 100644
--- a/libmpathpersist/mpath_updatepr.c
+++ b/libmpathpersist/mpath_updatepr.c
@@ -84,13 +84,13 @@ int get_prflag(char *mapname)
 	snprintf(str, sizeof(str), "getprstatus map %s", mapname);
 	reply = do_pr(mapname, str);
 	if (!reply)
-		prflag = PRFLAG_UNKNOWN;
+		prflag = PR_UNKNOWN;
 	else if (strncmp(reply, "unset", 5) == 0)
-		prflag = PRFLAG_UNSET;
+		prflag = PR_UNSET;
 	else if (strncmp(reply, "set", 3) == 0)
-		prflag = PRFLAG_SET;
+		prflag = PR_SET;
 	else
-		prflag = PRFLAG_UNKNOWN;
+		prflag = PR_UNKNOWN;
 
 	free(reply);
 	return prflag;
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 39d1c71c..6c427d6f 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -424,11 +424,10 @@ struct path {
 
 typedef int (pgpolicyfn) (struct multipath *, vector);
 
-
-enum prflag_value {
-	PRFLAG_UNKNOWN,
-	PRFLAG_UNSET,
-	PRFLAG_SET,
+enum pr_value {
+	PR_UNKNOWN,
+	PR_UNSET,
+	PR_SET,
 };
 
 enum prio_update_type {
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index fcb87757..a92b07ce 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1250,14 +1250,15 @@ cli_shutdown (void * v, struct strbuf *reply, void * data)
 	return 0;
 }
 
+static const char *const pr_str[] = {
+	[PR_UNKNOWN] = "unknown\n",
+	[PR_UNSET] = "unset\n",
+	[PR_SET] = "set\n",
+};
+
 static int
 cli_getprstatus (void * v, struct strbuf *reply, void * data)
 {
-	static const char * const prflag_str[] = {
-		[PRFLAG_UNKNOWN] = "unknown\n",
-		[PRFLAG_UNSET] = "unset\n",
-		[PRFLAG_SET] = "set\n",
-	};
 	struct multipath * mpp;
 	struct vectors * vecs = (struct vectors *)data;
 	char * param = get_keyparam(v, KEY_MAP);
@@ -1268,7 +1269,7 @@ cli_getprstatus (void * v, struct strbuf *reply, void * data)
 	if (!mpp)
 		return -ENODEV;
 
-	append_strbuf_str(reply, prflag_str[mpp->prflag]);
+	append_strbuf_str(reply, pr_str[mpp->prflag]);
 
 	condlog(3, "%s: reply = %s", param, get_strbuf_str(reply));
 
@@ -1288,12 +1289,11 @@ cli_setprstatus(void * v, struct strbuf *reply, void * data)
 	if (!mpp)
 		return -ENODEV;
 
-	if (mpp->prflag != PRFLAG_SET) {
-		mpp->prflag = PRFLAG_SET;
+	if (mpp->prflag != PR_SET) {
+		mpp->prflag = PR_SET;
 		condlog(2, "%s: prflag set", param);
 	}
 
-
 	return 0;
 }
 
@@ -1310,8 +1310,8 @@ cli_unsetprstatus(void * v, struct strbuf *reply, void * data)
 	if (!mpp)
 		return -ENODEV;
 
-	if (mpp->prflag != PRFLAG_UNSET) {
-		mpp->prflag = PRFLAG_UNSET;
+	if (mpp->prflag != PR_UNSET) {
+		mpp->prflag = PR_UNSET;
 		condlog(2, "%s: prflag unset", param);
 	}
 
diff --git a/multipathd/main.c b/multipathd/main.c
index 02cfe8d1..582af880 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -637,7 +637,7 @@ pr_register_active_paths(struct multipath *mpp)
 
 	vector_foreach_slot (mpp->pg, pgp, i) {
 		vector_foreach_slot (pgp->paths, pp, j) {
-			if (mpp->prflag == PRFLAG_UNSET)
+			if (mpp->prflag == PR_UNSET)
 				return;
 			if ((pp->state == PATH_UP) || (pp->state == PATH_GHOST))
 				mpath_pr_event_handle(pp);
@@ -1273,7 +1273,7 @@ ev_add_path (struct path * pp, struct vectors * vecs, int need_do_map)
 	int start_waiter = 0;
 	int ret;
 	int ro;
-	unsigned char prflag = PRFLAG_UNSET;
+	unsigned char prflag = PR_UNSET;
 
 	/*
 	 * need path UID to go any further
@@ -1401,8 +1401,7 @@ rescan:
 	sync_map_state(mpp, false);
 
 	if (retries >= 0) {
-		if ((mpp->prflag == PRFLAG_SET && prflag != PRFLAG_SET) ||
-		    start_waiter)
+		if ((mpp->prflag == PR_SET && prflag != PR_SET) || start_waiter)
 			pr_register_active_paths(mpp);
 		condlog(2, "%s [%s]: path added to devmap %s",
 			pp->dev, pp->dev_t, mpp->alias);
@@ -2639,7 +2638,7 @@ update_path_state (struct vectors * vecs, struct path * pp)
 
 		/* newstate == PATH_UP || newstate == PATH_GHOST */
 
-		if (pp->mpp->prflag != PRFLAG_UNSET) {
+		if (pp->mpp->prflag != PR_UNSET) {
 			int prflag = pp->mpp->prflag;
 			/*
 			 * Check Persistent Reservation.
@@ -2647,8 +2646,7 @@ update_path_state (struct vectors * vecs, struct path * pp)
 			condlog(2, "%s: checking persistent "
 				"reservation registration", pp->dev);
 			mpath_pr_event_handle(pp);
-			if (pp->mpp->prflag == PRFLAG_SET &&
-			    prflag != PRFLAG_SET)
+			if (pp->mpp->prflag == PR_SET && prflag != PR_SET)
 				pr_register_active_paths(pp->mpp);
 		}
 
@@ -4231,14 +4229,14 @@ static void mpath_pr_event_handle(struct path *pp)
 	struct prout_param_descriptor *param;
 
 	if (pp->bus != SYSFS_BUS_SCSI) {
-		mpp->prflag = PRFLAG_UNSET;
+		mpp->prflag = PR_UNSET;
 		return;
 	}
 
 	if (update_map_pr(mpp, pp) != MPATH_PR_SUCCESS)
 		return;
 
-	if (mpp->prflag != PRFLAG_SET)
+	if (mpp->prflag != PR_SET)
 		return;
 
 	param = (struct prout_param_descriptor *)calloc(1, sizeof(struct prout_param_descriptor));
-- 
2.48.1

[PATCH 14/15] libmpathpersist: use a switch statement for prout command finalizing

[Benjamin Marzinski]

Change the code at the end of do_mpath_persistent_reserve_out() to
use a switch statement instead of multiple if statements. A future
patch will add more actions here, and a switch statement looks cleaner.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 1bb62531..ae0defc2 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -729,15 +729,19 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 		return MPATH_PR_OTHER;
 	}
 
-	if ((ret == MPATH_PR_SUCCESS) && ((rq_servact == MPATH_PROUT_REG_SA) ||
-				(rq_servact ==  MPATH_PROUT_REG_IGN_SA)))
-	{
+	if (ret != MPATH_PR_SUCCESS)
+		return ret;
+
+	switch (rq_servact) {
+	case MPATH_PROUT_REG_SA:
+	case MPATH_PROUT_REG_IGN_SA:
 		if (prkey == 0) {
 			update_prflag(mpp->alias, 0);
 			update_prkey(mpp->alias, 0);
 		} else
 			update_prflag(mpp->alias, 1);
-	} else if ((ret == MPATH_PR_SUCCESS) && (rq_servact == MPATH_PROUT_CLEAR_SA)) {
+		break;
+	case MPATH_PROUT_CLEAR_SA:
 		update_prflag(mpp->alias, 0);
 		update_prkey(mpp->alias, 0);
 	}
-- 
2.48.1

[PATCH 15/15] libmpathpersist: Add safety check for preempting on key change

[Benjamin Marzinski]

When a reservation key is changed from one non-zero value to another,
libmpathpersist checks if the old key is still holding the reservation,
and preempts it if it is. This was only safe if two nodes never share
the same key. If a node uses the same key as another node that is
holding the reservation, and switches keys so that it no longer matches,
it will end up preempting the reservation. This is clearly unexpected
behavior, and it can come about by a simple accident of registering a
device with the wrong key, and then immediately fixing it.

To handle this, add code to track if a device is the reservation holder
to multipathd. multipathd now has three new commands "getprhold",
"setprhold", and "unsetprhold". These commands work like the equivalent
*prstatus commands. libmpathpersist calls setprhold on RESERVE commands
and PREEMPT commands when the preempted key is holding the reservation
and unsetprhold on RELEASE commands. Also, calling unsetprflag causes
prhold to be unset as well, so CLEAR commands and REGISTER commands with
a 0x0 service action key will also unset prhold. libmpathpersist() will
also unset prhold if it notices that the device cannot be holding a
reservation in preempt_missing_path().

When a new multipath device is created, its initial prhold state is
PR_UNKNOWN until it checks the current reservation, just like with
prflag. If multipathd ever finds that a device's registration has been
preempted or cleared in update_map_pr(), it unsets prhold, just like
with prflag.

Now, before libmpathpersist preempts a reservation when changing keys
it also checks if multipathd thinks that the device is holding
the reservation. If it does not, then libmpathpersist won't preempt
the key. It will assume that another node is holding the reservation
with the same key.

I should note that this safety check only stops a node not holding the
reservation from preempting the node holding the reservation. If the
node holding the reservation changes its key, but it fails to change the
resevation key, because that path is down or gone, it will still issue
the preempt to move the reservation to a usable path, even if another
node is using the same key. This will remove the registrations for that
other node. It also will not work correctly if multipathd stops tracking
a device for some reason. It's only a best-effort safety check.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c | 87 ++++++++++++++++++++++++-----
 libmpathpersist/mpath_updatepr.c    | 19 ++++++-
 libmpathpersist/mpathpr.h           |  2 +
 libmultipath/structs.h              |  1 +
 multipathd/callbacks.c              |  3 +
 multipathd/cli.c                    |  4 +-
 multipathd/cli.h                    |  3 +
 multipathd/cli_handlers.c           | 48 ++++++++++++++++
 multipathd/main.c                   | 33 ++++++++++-
 9 files changed, 182 insertions(+), 18 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index ae0defc2..91f0d018 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -260,6 +260,10 @@ static int mpath_prout_common(struct multipath *mpp,int rq_servact, int rq_scope
  * holding the reservation on a path that couldn't get its key updated,
  * either because it is down or no longer part of the multipath device,
  * you need to preempt the reservation to a usable path with the new key
+ *
+ * Also, it's possible that the reservation was preempted, and the device
+ * is being re-registered. If it appears that is the case, clear
+ * mpp->prhold in multipathd.
  */
 void preempt_missing_path(struct multipath *mpp, uint8_t *key, uint8_t *sa_key,
 			  int noisy)
@@ -272,12 +276,19 @@ void preempt_missing_path(struct multipath *mpp, uint8_t *key, uint8_t *sa_key,
 	int status;
 
 	/*
-	 * If you previously didn't have a key registered or you didn't
-	 * switch to a different key, there's no need to preempt. Also, you
-	 * can't preempt if you no longer have a registered key
+	 * If you previously didn't have a key registered, you can't
+	 * be holding the reservation. Also, you can't preempt if you
+	 * no longer have a registered key
 	 */
-	if (memcmp(key, zero, 8) == 0 || memcmp(sa_key, zero, 8) == 0 ||
-	    memcmp(key, sa_key, 8) == 0)
+	if (memcmp(key, zero, 8) == 0 || memcmp(sa_key, zero, 8) == 0) {
+		update_prhold(mpp->alias, false);
+		return;
+	}
+	/*
+	 * If you didn't switch to a different key, there is no need to
+	 * preempt.
+	 */
+	if (memcmp(key, sa_key, 8) == 0)
 		return;
 
 	status = mpath_prin_activepath(mpp, MPATH_PRIN_RRES_SA, &resp, noisy);
@@ -286,13 +297,29 @@ void preempt_missing_path(struct multipath *mpp, uint8_t *key, uint8_t *sa_key,
 		return;
 	}
 	/* If there is no reservation, there's nothing to preempt */
-	if (!resp.prin_descriptor.prin_readresv.additional_length)
+	if (!resp.prin_descriptor.prin_readresv.additional_length) {
+		update_prhold(mpp->alias, false);
 		return;
+	}
 	/*
 	 * If there reservation is not held by the old key, you don't
 	 * want to preempt it
 	 */
-	if (memcmp(key, resp.prin_descriptor.prin_readresv.key, 8) != 0)
+	if (memcmp(key, resp.prin_descriptor.prin_readresv.key, 8) != 0) {
+		/*
+		 * If reseravation key doesn't match either the old or
+		 * the new key, then clear prhold.
+		 */
+		if (memcmp(sa_key, resp.prin_descriptor.prin_readresv.key, 8) != 0)
+			update_prhold(mpp->alias, false);
+		return;
+	}
+
+	/*
+	 * If multipathd doesn't think it is holding the reservation, don't
+	 * preempt it
+	 */
+	if (get_prhold(mpp->alias) != PR_SET)
 		return;
 	/* Assume this key is being held by an inaccessable path on this
 	 * node. libmpathpersist has never worked if multiple nodes share
@@ -640,19 +667,38 @@ fail_resume:
 	return (status == MPATH_PR_RETRYABLE_ERROR) ? MPATH_PR_OTHER : status;
 }
 
+static int reservation_key_matches(struct multipath *mpp, uint8_t *key,
+				   int noisy)
+{
+	struct prin_resp resp = {0};
+	int status;
+
+	status = mpath_prin_activepath(mpp, MPATH_PRIN_RRES_SA, &resp, noisy);
+	if (status != MPATH_PR_SUCCESS) {
+		condlog(0, "%s: pr in read reservation command failed.",
+			mpp->wwid);
+		return YNU_UNDEF;
+	}
+	if (!resp.prin_descriptor.prin_readresv.additional_length)
+		return YNU_NO;
+	if (memcmp(key, resp.prin_descriptor.prin_readresv.key, 8) == 0)
+		return YNU_YES;
+	return YNU_NO;
+}
+
 /*
  * for MPATH_PROUT_REG_IGN_SA, we use the ignored paramp->key to store the
- * currently registered key.
+ * currently registered key for use in preempt_missing_path(), but only if
+ * the key is holding the reservation.
  */
 static void set_ignored_key(struct multipath *mpp, uint8_t *key)
 {
 	memset(key, 0, 8);
 	if (!get_be64(mpp->reservation_key))
 		return;
-	if (get_prflag(mpp->alias) == PR_UNSET)
+	if (get_prhold(mpp->alias) == PR_UNSET)
 		return;
-	update_map_pr(mpp, NULL);
-	if (mpp->prflag != PR_SET)
+	if (reservation_key_matches(mpp, key, 0) == YNU_NO)
 		return;
 	memcpy(key, &mpp->reservation_key, 8);
 }
@@ -665,6 +711,7 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	int ret;
 	uint64_t prkey;
 	struct config *conf;
+	bool preempting_reservation = false;
 
 	ret = mpath_get_map(curmp, pathvec, fd, &mpp);
 	if (ret != MPATH_PR_SUCCESS)
@@ -715,9 +762,12 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	case MPATH_PROUT_REG_IGN_SA:
 		ret= mpath_prout_reg(mpp, rq_servact, rq_scope, rq_type, paramp, noisy);
 		break;
-	case MPATH_PROUT_RES_SA :
-	case MPATH_PROUT_PREE_SA :
-	case MPATH_PROUT_PREE_AB_SA :
+	case MPATH_PROUT_PREE_SA:
+	case MPATH_PROUT_PREE_AB_SA:
+		if (reservation_key_matches(mpp, paramp->sa_key, noisy) == YNU_YES)
+			preempting_reservation = true;
+		/* fallthrough */
+	case MPATH_PROUT_RES_SA:
 	case MPATH_PROUT_CLEAR_SA:
 		ret = mpath_prout_common(mpp, rq_servact, rq_scope, rq_type,
 					 paramp, noisy, NULL);
@@ -744,6 +794,15 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	case MPATH_PROUT_CLEAR_SA:
 		update_prflag(mpp->alias, 0);
 		update_prkey(mpp->alias, 0);
+		break;
+	case MPATH_PROUT_RES_SA:
+	case MPATH_PROUT_REL_SA:
+		update_prhold(mpp->alias, (rq_servact == MPATH_PROUT_RES_SA));
+		break;
+	case MPATH_PROUT_PREE_SA:
+	case MPATH_PROUT_PREE_AB_SA:
+		if (preempting_reservation)
+			update_prhold(mpp->alias, 1);
 	}
 	return ret;
 }
diff --git a/libmpathpersist/mpath_updatepr.c b/libmpathpersist/mpath_updatepr.c
index c8fbe4a2..dd8dd48e 100644
--- a/libmpathpersist/mpath_updatepr.c
+++ b/libmpathpersist/mpath_updatepr.c
@@ -75,13 +75,13 @@ static int do_update_pr(char *alias, char *cmd, char *key)
 	return ret;
 }
 
-int get_prflag(char *mapname)
+static int do_get_pr(char *mapname, const char *cmd)
 {
 	char str[256];
 	char *reply;
 	int prflag;
 
-	snprintf(str, sizeof(str), "getprstatus map %s", mapname);
+	snprintf(str, sizeof(str), "%s map %s", cmd, mapname);
 	reply = do_pr(mapname, str);
 	if (!reply)
 		prflag = PR_UNKNOWN;
@@ -96,11 +96,26 @@ int get_prflag(char *mapname)
 	return prflag;
 }
 
+int get_prflag(char *mapname)
+{
+	return do_get_pr(mapname, "getprstatus");
+}
+
+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_prhold(char *mapname, bool set)
+{
+	return do_update_pr(mapname, (set) ? "setprhold" : "unsetprhold", NULL);
+}
+
 int update_prkey_flags(char *mapname, uint64_t prkey, uint8_t sa_flags) {
 	char str[256];
 
diff --git a/libmpathpersist/mpathpr.h b/libmpathpersist/mpathpr.h
index 912957e5..99d6b82f 100644
--- a/libmpathpersist/mpathpr.h
+++ b/libmpathpersist/mpathpr.h
@@ -9,6 +9,8 @@
 int update_prflag(char *mapname, 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);
+int update_prhold(char *mapname, bool set);
 #define update_prkey(mapname, prkey) update_prkey_flags(mapname, prkey, 0)
 
 #endif
diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 6c427d6f..97093675 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -522,6 +522,7 @@ struct multipath {
 	struct be64 reservation_key;
 	uint8_t sa_flags;
 	int prflag;
+	int prhold;
 	int all_tg_pt;
 	struct gen_multipath generic_mp;
 	bool fpin_must_reload;
diff --git a/multipathd/callbacks.c b/multipathd/callbacks.c
index fb87b280..b6b57f45 100644
--- a/multipathd/callbacks.c
+++ b/multipathd/callbacks.c
@@ -69,4 +69,7 @@ void init_handler_callbacks(void)
 	set_handler_callback(VRB_UNSETMARGINAL | Q1_PATH, HANDLER(cli_unset_marginal));
 	set_handler_callback(VRB_UNSETMARGINAL | Q1_MAP,
 			     HANDLER(cli_unset_all_marginal));
+	set_handler_callback(VRB_GETPRHOLD | Q1_MAP, HANDLER(cli_getprhold));
+	set_handler_callback(VRB_SETPRHOLD | Q1_MAP, HANDLER(cli_setprhold));
+	set_handler_callback(VRB_UNSETPRHOLD | Q1_MAP, HANDLER(cli_unsetprhold));
 }
diff --git a/multipathd/cli.c b/multipathd/cli.c
index 34588290..d0e6cebc 100644
--- a/multipathd/cli.c
+++ b/multipathd/cli.c
@@ -224,7 +224,9 @@ load_keys (void)
 	r += add_key(keys, "setmarginal", VRB_SETMARGINAL, 0);
 	r += add_key(keys, "unsetmarginal", VRB_UNSETMARGINAL, 0);
 	r += add_key(keys, "all", KEY_ALL, 0);
-
+	r += add_key(keys, "getprhold", VRB_GETPRHOLD, 0);
+	r += add_key(keys, "setprhold", VRB_SETPRHOLD, 0);
+	r += add_key(keys, "unsetprhold", VRB_UNSETPRHOLD, 0);
 
 	if (r) {
 		free_keys(keys);
diff --git a/multipathd/cli.h b/multipathd/cli.h
index 9fa50145..5a943a45 100644
--- a/multipathd/cli.h
+++ b/multipathd/cli.h
@@ -38,6 +38,9 @@ enum {
 	VRB_UNSETMARGINAL	= 23,
 	VRB_SHUTDOWN		= 24,
 	VRB_QUIT		= 25,
+	VRB_GETPRHOLD		= 26,
+	VRB_SETPRHOLD		= 27,
+	VRB_UNSETPRHOLD		= 28,
 
 	/* Qualifiers, values must be different from verbs */
 	KEY_PATH		= 65,
diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index a92b07ce..34910c9b 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1314,6 +1314,10 @@ cli_unsetprstatus(void * v, struct strbuf *reply, void * data)
 		mpp->prflag = PR_UNSET;
 		condlog(2, "%s: prflag unset", param);
 	}
+	if (mpp->prhold != PR_UNSET) {
+		mpp->prhold = PR_UNSET;
+		condlog(2, "%s: prhold unset (by clearing prflag)", param);
+	}
 
 	return 0;
 }
@@ -1401,6 +1405,50 @@ cli_setprkey(void * v, struct strbuf *reply, void * data)
 	return ret;
 }
 
+static int do_prhold(struct vectors *vecs, char *param, int prhold)
+{
+	struct multipath *mpp = find_mp_by_str(vecs->mpvec, param);
+
+	if (!mpp)
+		return -ENODEV;
+
+	if (mpp->prhold != prhold) {
+		mpp->prhold = prhold;
+		condlog(2, "%s: prhold %s", param, pr_str[prhold]);
+	}
+
+	return 0;
+}
+
+static int cli_setprhold(void *v, struct strbuf *reply, void *data)
+{
+	return do_prhold((struct vectors *)data, get_keyparam(v, KEY_MAP),
+			 PR_SET);
+}
+
+static int cli_unsetprhold(void *v, struct strbuf *reply, void *data)
+{
+	return do_prhold((struct vectors *)data, get_keyparam(v, KEY_MAP),
+			 PR_UNSET);
+}
+
+static int cli_getprhold(void *v, struct strbuf *reply, void *data)
+{
+	struct multipath *mpp;
+	struct vectors *vecs = (struct vectors *)data;
+	char *param = get_keyparam(v, KEY_MAP);
+
+	param = convert_dev(param, 0);
+
+	mpp = find_mp_by_str(vecs->mpvec, param);
+	if (!mpp)
+		return -ENODEV;
+
+	append_strbuf_str(reply, pr_str[mpp->prhold]);
+	condlog(3, "%s: reply = %s", param, get_strbuf_str(reply));
+	return 0;
+}
+
 static int cli_set_marginal(void * v, struct strbuf *reply, void * data)
 {
 	struct vectors * vecs = (struct vectors *)data;
diff --git a/multipathd/main.c b/multipathd/main.c
index 582af880..d22425f6 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -4222,6 +4222,32 @@ main (int argc, char *argv[])
 		return (child(NULL));
 }
 
+static void check_prhold(struct multipath *mpp, struct path *pp)
+{
+	struct prin_resp resp = {0};
+	int status;
+
+	if (mpp->prflag == PR_UNSET) {
+		mpp->prhold = PR_UNSET;
+		return;
+	}
+	if (mpp->prflag != PR_SET || mpp->prhold != PR_UNKNOWN)
+		return;
+
+	status = prin_do_scsi_ioctl(pp->dev, MPATH_PRIN_RRES_SA, &resp, 0);
+	if (status != MPATH_PR_SUCCESS) {
+		condlog (0, "%s: pr in read reservation command failed: %d",
+			 mpp->wwid, status);
+		return;
+	}
+	mpp->prhold = PR_UNSET;
+	if (!resp.prin_descriptor.prin_readresv.additional_length)
+		return;
+
+	if (memcmp(&mpp->reservation_key, resp.prin_descriptor.prin_readresv.key, 8) == 0)
+		mpp->prhold = PR_SET;
+}
+
 static void mpath_pr_event_handle(struct path *pp)
 {
 	struct multipath *mpp = pp->mpp;
@@ -4233,8 +4259,13 @@ static void mpath_pr_event_handle(struct path *pp)
 		return;
 	}
 
-	if (update_map_pr(mpp, pp) != MPATH_PR_SUCCESS)
+	if (update_map_pr(mpp, pp) != MPATH_PR_SUCCESS) {
+		if (mpp->prflag == PR_UNSET)
+			mpp->prhold = PR_UNSET;
 		return;
+	}
+
+	check_prhold(mpp, pp);
 
 	if (mpp->prflag != PR_SET)
 		return;
-- 
2.48.1

Re: [PATCH 11/15] limpathpersist: Handle changing key corner case

[Martin Wilck]

On Thu, 2025-07-10 at 14:10 -0400, Benjamin Marzinski wrote:
> When you change the reservation key of a registered multipath device,
> some of paths might be down or even deleted since you originally

[...]
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

This is not a full review, just feedback about a failing CI pipeline.

> ---
>  libmpathpersist/mpath_persist_int.c | 124 ++++++++++++++++++++------
> --
>  1 file changed, 90 insertions(+), 34 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist_int.c
> b/libmpathpersist/mpath_persist_int.c
> index ad5a4ee7..ca972c2b 100644
> --- a/libmpathpersist/mpath_persist_int.c
> +++ b/libmpathpersist/mpath_persist_int.c
> 
> +void preempt_missing_path(struct multipath *mpp, uint8_t *key,
> uint8_t *sa_key,
> +			  int noisy)
> +{
> +	uint8_t zero[8] = {0};
> +	struct prin_resp resp = {0};

gcc 4.8 (Debian Jessie) dislikes this syntax.

> mpath_persist_int.c:272:9: error: missing braces around initializer [-Werror=missing-braces]

The same issue appears in patch 7/15 and patch 15/15.
It seems to be a gcc bug
(https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119).
So we could use #pragma to silence the warning, but I'd like to avoid
that, especially because it occurs multiple times.

I don't think we should drop Jessie support just yet.

This works:

	struct prin_resp resp = {{{0}}};

> +	int rq_scope;
> +	unsigned int rq_type;
> +	struct prout_param_descriptor paramp = {0};

Likewise, gcc 4.8 warns about this:

> mpath_persist_int.c:275:9: error: missing initializer for field 'sa_key' of 'struct prout_param_descriptor' [-Werror=missing-field-initializers]

It's not easy to find a syntax that this compiler accepts without
warning. {{0}} does _not_ work here.

The following did the trick for me:

	struct prout_param_descriptor paramp = {.sa_flags = 0};


Regards
Martin

Re: [PATCH 11/15] limpathpersist: Handle changing key corner case

[Martin Wilck]

On Fri, 2025-07-11 at 14:15 +0200, Martin Wilck wrote:
> On Thu, 2025-07-10 at 14:10 -0400, Benjamin Marzinski wrote:
> > When you change the reservation key of a registered multipath
> > device,
> > some of paths might be down or even deleted since you originally
> 
> [...]
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> 
> This is not a full review, just feedback about a failing CI pipeline.
> 
> > ---
> >  libmpathpersist/mpath_persist_int.c | 124 ++++++++++++++++++++----
> > --
> > --
> >  1 file changed, 90 insertions(+), 34 deletions(-)
> > 
> > diff --git a/libmpathpersist/mpath_persist_int.c
> > b/libmpathpersist/mpath_persist_int.c
> > index ad5a4ee7..ca972c2b 100644
> > --- a/libmpathpersist/mpath_persist_int.c
> > +++ b/libmpathpersist/mpath_persist_int.c
> > 
> > +void preempt_missing_path(struct multipath *mpp, uint8_t *key,
> > uint8_t *sa_key,
> > +			  int noisy)
> > +{
> > +	uint8_t zero[8] = {0};
> > +	struct prin_resp resp = {0};
> 
> gcc 4.8 (Debian Jessie) dislikes this syntax.
> 
> > mpath_persist_int.c:272:9: error: missing braces around initializer
> > [-Werror=missing-braces]
> 
> The same issue appears in patch 7/15 and patch 15/15.
> It seems to be a gcc bug
> (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119).
> So we could use #pragma to silence the warning, but I'd like to avoid
> that, especially because it occurs multiple times.
> 
> I don't think we should drop Jessie support just yet.
> 
> This works:
> 
> 	struct prin_resp resp = {{{0}}};

It does, but it causes an error with Jessie's clang compiler (clang
3.5.0).
                                  ^
> mpath_persist_int.c:673:30: error: missing field 'additional_length' initializer [-Werror,-Wmissing-field-initializers]
>         struct prin_resp resp = {{{0}}};
 
Both compilers seem to accept this:

	struct prin_resp resp = {{{.prgeneration = 0}}};

It's getting so awkward that we might as well just use memset.

Martin

Re: [PATCH 11/15] limpathpersist: Handle changing key corner case

[Benjamin Marzinski]

On Fri, Jul 11, 2025 at 04:11:46PM +0200, Martin Wilck wrote:
> On Fri, 2025-07-11 at 14:15 +0200, Martin Wilck wrote:
> > On Thu, 2025-07-10 at 14:10 -0400, Benjamin Marzinski wrote:
> > > When you change the reservation key of a registered multipath
> > > device,
> > > some of paths might be down or even deleted since you originally
> > 
> > [...]
> > > 
> > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > 
> > This is not a full review, just feedback about a failing CI pipeline.
> > 
> > > ---
> > >  libmpathpersist/mpath_persist_int.c | 124 ++++++++++++++++++++----
> > > --
> > > --
> > >  1 file changed, 90 insertions(+), 34 deletions(-)
> > > 
> > > diff --git a/libmpathpersist/mpath_persist_int.c
> > > b/libmpathpersist/mpath_persist_int.c
> > > index ad5a4ee7..ca972c2b 100644
> > > --- a/libmpathpersist/mpath_persist_int.c
> > > +++ b/libmpathpersist/mpath_persist_int.c
> > > 
> > > +void preempt_missing_path(struct multipath *mpp, uint8_t *key,
> > > uint8_t *sa_key,
> > > +			  int noisy)
> > > +{
> > > +	uint8_t zero[8] = {0};
> > > +	struct prin_resp resp = {0};
> > 
> > gcc 4.8 (Debian Jessie) dislikes this syntax.
> > 
> > > mpath_persist_int.c:272:9: error: missing braces around initializer
> > > [-Werror=missing-braces]
> > 
> > The same issue appears in patch 7/15 and patch 15/15.
> > It seems to be a gcc bug
> > (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119).
> > So we could use #pragma to silence the warning, but I'd like to avoid
> > that, especially because it occurs multiple times.
> > 
> > I don't think we should drop Jessie support just yet.
> > 
> > This works:
> > 
> > 	struct prin_resp resp = {{{0}}};
> 
> It does, but it causes an error with Jessie's clang compiler (clang
> 3.5.0).
>                                   ^
> > mpath_persist_int.c:673:30: error: missing field 'additional_length' initializer [-Werror,-Wmissing-field-initializers]
> >         struct prin_resp resp = {{{0}}};
>  
> Both compilers seem to accept this:
> 
> 	struct prin_resp resp = {{{.prgeneration = 0}}};
> 
> It's getting so awkward that we might as well just use memset.

Yeah. picking a member from one of union types to set to 0 is pretty
awkward. I can repost the patches with memsets.

-Ben

> 
> Martin
> 

Re: [PATCH 11/15] limpathpersist: Handle changing key corner case

[Martin Wilck]

On Mon, 2025-07-14 at 12:59 -0400, Benjamin Marzinski wrote:
> On Fri, Jul 11, 2025 at 04:11:46PM +0200, Martin Wilck wrote:
> > On Fri, 2025-07-11 at 14:15 +0200, Martin Wilck wrote:
> > 
> > 
> > It's getting so awkward that we might as well just use memset.
> 
> Yeah. picking a member from one of union types to set to 0 is pretty
> awkward. I can repost the patches with memsets.

Let me finish the review first. I'm working on it.

Martin

Re: [PATCH 03/15] libmpathpersist: reduce log level for persistent reservation checking

[Martin Wilck]

On Thu, 2025-07-10 at 14:10 -0400, Benjamin Marzinski wrote:
> Move logging of minor expected behavior to INFO level. Modify the log
> level of some messages by whether or not mpp->prflag changed values.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmpathpersist/mpath_persist_int.c | 26 ++++++++++++++------------
>  1 file changed, 14 insertions(+), 12 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist_int.c
> b/libmpathpersist/mpath_persist_int.c
> index 612bbed9..4172167a 100644
> --- a/libmpathpersist/mpath_persist_int.c
> +++ b/libmpathpersist/mpath_persist_int.c
>  	}
> @@ -761,22 +761,24 @@ int update_map_pr(struct multipath *mpp)
> 
>  keys.key_list[i*8], 8 , 1);
> +		if (libmp_verbosity >= 3) {
> +			condlog(3, "%s: PR IN READKEYS[%d]
> reservation key:",
> +				mpp->alias, i);
> +			dumpHex((char *)&resp-
> >prin_descriptor.prin_readkeys.key_list[i * 8], 8, 1);
> +		}

I'd suggest to restrict usage of dumpHex() to verbosity 4 and higher.

>  
> -		if (!memcmp(&mpp->reservation_key, &resp-
> >prin_descriptor.prin_readkeys.key_list[i*8], 8))
> -		{
> -			condlog(2, "%s: reservation key found in pr
> in readkeys response", mpp->alias);
> +		if (!memcmp(&mpp->reservation_key, &resp-
> >prin_descriptor.prin_readkeys.key_list[i * 8], 8)) {
> +			condlog(3, "%s: reservation key found in pr
> in readkeys response", mpp->alias);

This one should be logged at verbosity 4 only, too, IMO.

Martin

Re: [PATCH 09/15] limpathpersist: redesign failed release workaround

[Martin Wilck]

Hi Ben,

I like this approach, but I have a few questions.

On Thu, 2025-07-10 at 14:10 -0400, Benjamin Marzinski wrote:
> The workaround for releasing a reservation held by a failed path
> (clearing all persistent reservation data and then reregistering all
> the
> keys) has several problems. It requires devices that support the READ
> FULL STATUS command and are capable of specifying TransportIDs with
> REGISTRATION commands (SIP_C), neither of which are mandatory to
> support
> SCSI Persistent Reservations.

While I have made similar experiences, I wonder where the SCSI specs
state which service actions and which parameters are mandatory for
Persistent Reservation commands.

I've tried to figure it out from SPC-6, but I couldn't.

>  Non SIP_C devices will be left with no
> registered keys. Also, not all cleared keys are registered, just the
> ones going to the Target Port receiving the REGISTRATION command. To
> reregister all the keys, the code would have to also use the "All
> Target
> Ports" flag to register keys on different Target Ports, but this
> could
> end up registering keys on paths they aren't supposed to be on (for
> instance if one of the registered keys was only there because the
> path
> was down and it couldn't be released).
> 
> The redesign avoids these issues by only using mandatory Persistent
> Reservation commands, without extra optional parameters or flags, and
> only effects the keys of the multipath device it is being issued on.
> The new workaround is:
> 1. Suspend the multipath device to prevent I/O
> 2. Preempt the key to move the reservation to an available path. This
>    also removes the registered keys from every path except the path
>    issuing the PREEMPT command. Since the device is suspended, not
> I/O
>    can go to these unregisted paths and fail.
> 3. Release the reservation on the path that now holds it.

Why that? Can't the reservation just be kept?

> 4. Resume the device (since it no longer matters that most of the
> paths
>    no longer have a registered key)
> 5. Reregister the keys on all the paths.

Why not re-register before resuming?

> 
> If steps 3 or 4 fail, the code will attempt to reregister the keys,
> and
> then attempt (or possibly re-attempt) the resume.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmpathpersist/mpath_persist_int.c | 173 ++++++++++++--------------
> --
>  libmultipath/libmultipath.version   |   1 +
>  2 files changed, 76 insertions(+), 98 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist_int.c
> b/libmpathpersist/mpath_persist_int.c
> index 13829742..7fb08b2e 100644
> --- a/libmpathpersist/mpath_persist_int.c
> +++ b/libmpathpersist/mpath_persist_int.c
> @@ -366,7 +366,8 @@ static int mpath_prout_reg(struct multipath
> *mpp,int rq_servact, int rq_scope,
>  
>  static int mpath_prout_common(struct multipath *mpp,int rq_servact,
> int rq_scope,
>  			      unsigned int rq_type,
> -			      struct prout_param_descriptor* paramp,
> int noisy)
> +			      struct prout_param_descriptor* paramp,
> int noisy,
> +			      struct path **pptr)
>  {
>  	int i,j, ret;
>  	struct pathgroup *pgp = NULL;
> @@ -385,6 +386,8 @@ static int mpath_prout_common(struct multipath
> *mpp,int rq_servact, int rq_scope
>  			found = true;
>  			ret = prout_do_scsi_ioctl(pp->dev,
> rq_servact, rq_scope,
>  						  rq_type, paramp,
> noisy);
> +			if (ret == MPATH_PR_SUCCESS && pptr)
> +				*pptr = pp;
>  			if (ret != MPATH_PR_RETRYABLE_ERROR)
>  				return ret;
>  		}
> @@ -405,14 +408,12 @@ static int mpath_prout_rel(struct multipath
> *mpp,int rq_servact, int rq_scope,
>  	struct path *pp = NULL;
>  	int active_pathcount = 0;
>  	pthread_attr_t attr;
> -	int rc, found = 0;
> +	int rc;
>  	int count = 0;
>  	int status = MPATH_PR_SUCCESS;
> -	struct prin_resp resp;
> -	struct prout_param_descriptor *pamp = NULL;
> -	struct prin_resp *pr_buff;
> -	int length;
> -	struct transportid *pptr = NULL;
> +	struct prin_resp resp = {0};
> +	uint16_t udev_flags = (mpp->skip_kpartx) ?
> MPATH_UDEV_NO_KPARTX_FLAG : 0;
> +	bool did_resume = false;
>  
>  	if (!mpp)
>  		return MPATH_PR_DMMP_ERROR;
> @@ -502,104 +503,78 @@ static int mpath_prout_rel(struct multipath
> *mpp,int rq_servact, int rq_scope,
>  	}
>  
>  	condlog (2, "%s: Path holding reservation is not
> available.", mpp->wwid);
> -
> -	pr_buff =  mpath_alloc_prin_response(MPATH_PRIN_RFSTAT_SA);
> -	if (!pr_buff){
> -		condlog (0, "%s: failed to  alloc pr in response
> buffer.", mpp->wwid);
> +	/*
> +	 * Cannot free the reservation because the path that is
> holding it
> +	 * is not usable. Workaround this by:
> +	 * 1. Suspending the device
> +	 * 2. Preempting the reservation to move it to a usable path
> +	 *    (this removes the registered keys on all paths except
> the
> +	 *    preempting one. Since the device is suspended, no IO
> can
> +	 *    go to these unregistered paths and fail).
> +	 * 3. Releasing the reservation on the path that now holds
> it.
> +	 * 4. Resuming the device (since it no longer matters that
> most of
> +	 *    that paths no longer have a registered key)
> +	 * 5. Reregistering keys on all the paths
> +	 */
> +
> +	if (!dm_simplecmd_noflush(DM_DEVICE_SUSPEND, mpp->alias, 0))
> {
> +		condlog(0, "%s: release: failed to suspend dm
> device.",

Why do you use dm_simplecmd_noflush() here? Shouldn't queued IO be
flushed from the dm device to avoid it being sent to paths that are
going to be unregistered?


> mpp->wwid);
>  		return MPATH_PR_OTHER;
>  	}
>  
> -	status = mpath_prin_activepath (mpp, MPATH_PRIN_RFSTAT_SA,
> pr_buff, noisy);
> -
> -	if (status != MPATH_PR_SUCCESS){
> -		condlog (0,  "%s: pr in read full status command
> failed.",  mpp->wwid);
> -		goto out;
> -	}
> -
> -	num = pr_buff-
> >prin_descriptor.prin_readfd.number_of_descriptor;
> -	if (0 == num){
> -		goto out;
> -	}
> -	length = sizeof (struct prout_param_descriptor) + (sizeof
> (struct transportid *));
> -
> -	pamp = (struct prout_param_descriptor *)malloc (length);
> -	if (!pamp){
> -		condlog (0, "%s: failed to alloc pr out parameter.",
> mpp->wwid);
> -		goto out;
> -	}
> -
> -	memset(pamp, 0, length);
> -
> -	pamp->trnptid_list[0] = (struct transportid *) malloc
> (sizeof (struct transportid));
> -	if (!pamp->trnptid_list[0]){
> -		condlog (0, "%s: failed to alloc pr out
> transportid.", mpp->wwid);
> -		goto out1;
> -	}
> -	pptr = pamp->trnptid_list[0];
> -
> -	if (get_be64(mpp->reservation_key)){
> -		memcpy (pamp->key, &mpp->reservation_key, 8);
> -		condlog (3, "%s: reservation key set.", mpp->wwid);
> +	memset(paramp, 0, sizeof(*paramp));
> +	memcpy(paramp->key, &mpp->reservation_key, 8);
> +	memcpy(paramp->sa_key, &mpp->reservation_key, 8);
> +	status = mpath_prout_common(mpp, MPATH_PROUT_PREE_SA,
> rq_scope, rq_type,
> +				    paramp, noisy, &pp);
> +	if (status != MPATH_PR_SUCCESS) {
> +		condlog(0, "%s: release: pr preempt command
> failed.", mpp->wwid);
> +		goto fail_resume;
>  	}
>  
> -	status = mpath_prout_common (mpp, MPATH_PROUT_CLEAR_SA,
> -				     rq_scope, rq_type, pamp,
> noisy);
> -
> -	if (status) {
> -		condlog(0, "%s: failed to send CLEAR_SA", mpp-
> >wwid);
> -		goto out2;
> +	memset(paramp, 0, sizeof(*paramp));
> +	memcpy(paramp->key, &mpp->reservation_key, 8);
> +	status = prout_do_scsi_ioctl(pp->dev, MPATH_PROUT_REL_SA,
> rq_scope,
> +				     rq_type, paramp, noisy);
> +	if (status != MPATH_PR_SUCCESS) {
> +		condlog(0, "%s: release on alternate path failed.",
> mpp->wwid);
> +		goto out_reregister;
>  	}
>  
> -	pamp->num_transportid = 1;
> -
> -	for (i = 0; i < num; i++){
> -		if (get_be64(mpp->reservation_key) &&
> -			memcmp(pr_buff-
> >prin_descriptor.prin_readfd.descriptors[i]->key,
> -			       &mpp->reservation_key, 8)){
> -			/*register with transport id*/
> -			memset(pamp, 0, length);
> -			pamp->trnptid_list[0] = pptr;
> -			memset (pamp->trnptid_list[0], 0, sizeof
> (struct transportid));
> -			memcpy (pamp->sa_key,
> -					pr_buff-
> >prin_descriptor.prin_readfd.descriptors[i]->key, 8);
> -			pamp->sa_flags = MPATH_F_SPEC_I_PT_MASK;
> -			pamp->num_transportid = 1;
> -
> -			memcpy (pamp->trnptid_list[0],
> -					&pr_buff-
> >prin_descriptor.prin_readfd.descriptors[i]->trnptid,
> -					sizeof (struct
> transportid));
> -			status = mpath_prout_common (mpp,
> MPATH_PROUT_REG_SA, 0, rq_type,
> -					pamp, noisy);
> -
> -			pamp->sa_flags = 0;
> -			memcpy (pamp->key, pr_buff-
> >prin_descriptor.prin_readfd.descriptors[i]->key, 8);
> -			memset (pamp->sa_key, 0, 8);
> -			pamp->num_transportid = 0;
> -			status = mpath_prout_common (mpp,
> MPATH_PROUT_REG_SA, 0, rq_type,
> -					pamp, noisy);
> -		}
> -		else
> -		{
> -			if (get_be64(mpp->reservation_key))
> -				found = 1;
> -		}
> -
> -
> -	}
> -
> -	if (found){
> -		memset (pamp, 0, length);
> -		memcpy (pamp->sa_key, &mpp->reservation_key, 8);
> -		memset (pamp->key, 0, 8);
> -		status = mpath_prout_reg(mpp, MPATH_PROUT_REG_SA,
> rq_scope, rq_type, pamp, noisy);
> +	if (!dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias,
> udev_flags)) {
> +		condlog(0, "%s release: failed to resume dm
> device.", mpp->wwid);
> +		/*
> +		 * leave status set to MPATH_PR_SUCCESS, we will
> have another
> +		 * chance to resume the device.
> +		 */
> +		goto out_reregister;
> +	}
> +	did_resume = true;
> +
> +out_reregister:
> +	memset(paramp, 0, sizeof(*paramp));
> +	memcpy(paramp->sa_key, &mpp->reservation_key, 8);
> +	rc = mpath_prout_reg(mpp, MPATH_PROUT_REG_IGN_SA, rq_scope,
> rq_type,
> +			     paramp, noisy);
> +	if (rc != MPATH_PR_SUCCESS)
> +		condlog(0, "%s: release: failed to reregister
> paths.", mpp->wwid);
> +
> +	/*
> +	 * If we failed releasing the reservation or resuming
> earlier
> +	 * try resuming now. Otherwise, return with the
> reregistering status
> +	 * This means we will report failure, even though the
> resevation
> +	 * has been released, since the keys were not reregistered.
> +	 */
> +	if (did_resume)
> +		return rc;
> +	else if (status == MPATH_PR_SUCCESS)
> +		status = rc;
> +fail_resume:
> +	if (!dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias,
> udev_flags)) {
> +		condlog(0, "%s: release: failed to resume dm
> device.", mpp->wwid);
> +		if (status == MPATH_PR_SUCCESS)
> +			status = MPATH_PR_OTHER;
>  	}
> -
> -out2:
> -	free(pptr);
> -out1:
> -	free (pamp);
> -out:
> -	free (pr_buff);
>  	return (status == MPATH_PR_RETRYABLE_ERROR) ? MPATH_PR_OTHER
> : status;
>  }
>  
> @@ -620,6 +595,7 @@ int do_mpath_persistent_reserve_out(vector curmp,
> vector pathvec, int fd,
>  	mpp->mpe = find_mpe(conf->mptable, mpp->wwid);
>  	select_reservation_key(conf, mpp);
>  	select_all_tg_pt(conf, mpp);
> +	select_skip_kpartx(conf, mpp);

I understand that this is because of the DM_DEVICE_RESUME, but it
deserves a comment here as it seems a bit out of place in this code
that deals only with persistent reservations. Actually, we may want to
move this logic to dm_simplecmd().

Thanks,
Martin

>  	put_multipath_config(conf);
>  
>  	memcpy(&prkey, paramp->sa_key, 8);
> @@ -661,7 +637,8 @@ int do_mpath_persistent_reserve_out(vector curmp,
> vector pathvec, int fd,
>  	case MPATH_PROUT_PREE_SA :
>  	case MPATH_PROUT_PREE_AB_SA :
>  	case MPATH_PROUT_CLEAR_SA:
> -		ret = mpath_prout_common(mpp, rq_servact, rq_scope,
> rq_type, paramp, noisy);
> +		ret = mpath_prout_common(mpp, rq_servact, rq_scope,
> rq_type,
> +					 paramp, noisy, NULL);
>  		break;
>  	case MPATH_PROUT_REL_SA:
>  		ret = mpath_prout_rel(mpp, rq_servact, rq_scope,
> rq_type, paramp, noisy);
> diff --git a/libmultipath/libmultipath.version
> b/libmultipath/libmultipath.version
> index a6718355..2f47b3ad 100644
> --- a/libmultipath/libmultipath.version
> +++ b/libmultipath/libmultipath.version
> @@ -249,4 +249,5 @@ local:
>  LIBMULTIPATH_29.1.0 {
>  global:
>  	can_recheck_wwid;
> +	select_skip_kpartx;
>  } LIBMULTIPATH_29.0.0;

Re: [PATCH 10/15] libmpathpersist: fail the release if all threads fail

[Martin Wilck]

On Thu, 2025-07-10 at 14:10 -0400, Benjamin Marzinski wrote:
> If none of the threads succeeds in issuing the release, simply return
> failure, instead of trying the workaround.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmpathpersist/mpath_persist_int.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist_int.c
> b/libmpathpersist/mpath_persist_int.c
> index 7fb08b2e..ad5a4ee7 100644
> --- a/libmpathpersist/mpath_persist_int.c
> +++ b/libmpathpersist/mpath_persist_int.c
> @@ -475,15 +475,21 @@ static int mpath_prout_rel(struct multipath
> *mpp,int rq_servact, int rq_scope,
>  		}
>  	}
>  
> +	rc = MPATH_PR_DMMP_ERROR;

I find the relationship between "status" and "rc" a bit confusing in
this patch. Can we use something like "bool all_failed" instead?

Martin


>  	for (i = 0; i < count; i++){
>  		/*  check thread status here and return the status
> */
>  
> -		if (thread[i].param.status ==
> MPATH_PR_RESERV_CONFLICT)
> +		if (thread[i].param.status == MPATH_PR_SUCCESS)
> +			rc = MPATH_PR_SUCCESS;
> +		else if (thread[i].param.status ==
> MPATH_PR_RESERV_CONFLICT)
>  			status = MPATH_PR_RESERV_CONFLICT;
> -		else if (status == MPATH_PR_SUCCESS
> -				&& thread[i].param.status !=
> MPATH_PR_RESERV_CONFLICT)
> +		else if (status == MPATH_PR_SUCCESS)
>  			status = thread[i].param.status;
>  	}
> +	if (rc != MPATH_PR_SUCCESS) {
> +		condlog(0, "%s: all threads failed to release
> reservation.", mpp->wwid);
> +		return status;
> +	}
>  
>  	status = mpath_prin_activepath (mpp, MPATH_PRIN_RRES_SA,
> &resp, noisy);
>  	if (status != MPATH_PR_SUCCESS){

Re: [PATCH 15/15] libmpathpersist: Add safety check for preempting on key change

[Martin Wilck]

On Thu, 2025-07-10 at 14:11 -0400, Benjamin Marzinski wrote:
> When a reservation key is changed from one non-zero value to another,
> libmpathpersist checks if the old key is still holding the
> reservation,
> and preempts it if it is. This was only safe if two nodes never share
> the same key. If a node uses the same key as another node that is
> holding the reservation, and switches keys so that it no longer
> matches,
> it will end up preempting the reservation. This is clearly unexpected
> behavior, and it can come about by a simple accident of registering a
> device with the wrong key, and then immediately fixing it.
> 
> To handle this, add code to track if a device is the reservation
> holder
> to multipathd. multipathd now has three new commands "getprhold",
> "setprhold", and "unsetprhold". These commands work like the
> equivalent
> *prstatus commands. libmpathpersist calls setprhold on RESERVE
> commands
> and PREEMPT commands when the preempted key is holding the
> reservation
> and unsetprhold on RELEASE commands. Also, calling unsetprflag causes
> prhold to be unset as well, so CLEAR commands and REGISTER commands
> with
> a 0x0 service action key will also unset prhold. libmpathpersist()
> will
> also unset prhold if it notices that the device cannot be holding a
> reservation in preempt_missing_path().
> 
> When a new multipath device is created, its initial prhold state is
> PR_UNKNOWN until it checks the current reservation, just like with
> prflag. If multipathd ever finds that a device's registration has
> been
> preempted or cleared in update_map_pr(), it unsets prhold, just like
> with prflag.
> 
> Now, before libmpathpersist preempts a reservation when changing keys
> it also checks if multipathd thinks that the device is holding
> the reservation. If it does not, then libmpathpersist won't preempt
> the key. It will assume that another node is holding the reservation
> with the same key.
> 
> I should note that this safety check only stops a node not holding
> the
> reservation from preempting the node holding the reservation. If the
> node holding the reservation changes its key, but it fails to change
> the
> resevation key, because that path is down or gone, it will still
> issue
> the preempt to move the reservation to a usable path, even if another
> node is using the same key. This will remove the registrations for
> that
> other node. It also will not work correctly if multipathd stops
> tracking
> a device for some reason. It's only a best-effort safety check.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmpathpersist/mpath_persist_int.c | 87 ++++++++++++++++++++++++---
> --
>  libmpathpersist/mpath_updatepr.c    | 19 ++++++-
>  libmpathpersist/mpathpr.h           |  2 +
>  libmultipath/structs.h              |  1 +
>  multipathd/callbacks.c              |  3 +
>  multipathd/cli.c                    |  4 +-
>  multipathd/cli.h                    |  3 +
>  multipathd/cli_handlers.c           | 48 ++++++++++++++++
>  multipathd/main.c                   | 33 ++++++++++-
>  9 files changed, 182 insertions(+), 18 deletions(-)
> 

Two questions all the way down.


> diff --git a/libmpathpersist/mpath_persist_int.c
> b/libmpathpersist/mpath_persist_int.c
> index ae0defc2..91f0d018 100644
> --- a/libmpathpersist/mpath_persist_int.c
> +++ b/libmpathpersist/mpath_persist_int.c
> @@ -260,6 +260,10 @@ static int mpath_prout_common(struct multipath
> *mpp,int rq_servact, int rq_scope
>   * holding the reservation on a path that couldn't get its key
> updated,
>   * either because it is down or no longer part of the multipath
> device,
>   * you need to preempt the reservation to a usable path with the new
> key
> + *
> + * Also, it's possible that the reservation was preempted, and the
> device
> + * is being re-registered. If it appears that is the case, clear
> + * mpp->prhold in multipathd.
>   */
>  void preempt_missing_path(struct multipath *mpp, uint8_t *key,
> uint8_t *sa_key,
>  			  int noisy)
> @@ -272,12 +276,19 @@ void preempt_missing_path(struct multipath
> *mpp, uint8_t *key, uint8_t *sa_key,
>  	int status;
>  
>  	/*
> -	 * If you previously didn't have a key registered or you
> didn't
> -	 * switch to a different key, there's no need to preempt.
> Also, you
> -	 * can't preempt if you no longer have a registered key
> +	 * If you previously didn't have a key registered, you can't
> +	 * be holding the reservation. Also, you can't preempt if
> you
> +	 * no longer have a registered key
>  	 */
> -	if (memcmp(key, zero, 8) == 0 || memcmp(sa_key, zero, 8) ==
> 0 ||
> -	    memcmp(key, sa_key, 8) == 0)
> +	if (memcmp(key, zero, 8) == 0 || memcmp(sa_key, zero, 8) ==
> 0) {
> +		update_prhold(mpp->alias, false);
> +		return;
> +	}
> +	/*
> +	 * If you didn't switch to a different key, there is no need
> to
> +	 * preempt.
> +	 */
> +	if (memcmp(key, sa_key, 8) == 0)
>  		return;
>  
>  	status = mpath_prin_activepath(mpp, MPATH_PRIN_RRES_SA,
> &resp, noisy);
> @@ -286,13 +297,29 @@ void preempt_missing_path(struct multipath
> *mpp, uint8_t *key, uint8_t *sa_key,
>  		return;
>  	}
>  	/* If there is no reservation, there's nothing to preempt */
> -	if (!resp.prin_descriptor.prin_readresv.additional_length)
> +	if (!resp.prin_descriptor.prin_readresv.additional_length) {
> +		update_prhold(mpp->alias, false);
>  		return;
> +	}
>  	/*
>  	 * If there reservation is not held by the old key, you
> don't
>  	 * want to preempt it
>  	 */
> -	if (memcmp(key, resp.prin_descriptor.prin_readresv.key, 8)
> != 0)
> +	if (memcmp(key, resp.prin_descriptor.prin_readresv.key, 8)
> != 0) {
> +		/*
> +		 * If reseravation key doesn't match either the old
> or
> +		 * the new key, then clear prhold.
> +		 */
> +		if (memcmp(sa_key,
> resp.prin_descriptor.prin_readresv.key, 8) != 0)
> +			update_prhold(mpp->alias, false);
> +		return;
> +	}
> +
> +	/*
> +	 * If multipathd doesn't think it is holding the
> reservation, don't
> +	 * preempt it
> +	 */
> +	if (get_prhold(mpp->alias) != PR_SET)
>  		return;
>  	/* Assume this key is being held by an inaccessable path on
> this
>  	 * node. libmpathpersist has never worked if multiple nodes
> share
> @@ -640,19 +667,38 @@ fail_resume:
>  	return (status == MPATH_PR_RETRYABLE_ERROR) ? MPATH_PR_OTHER
> : status;
>  }
>  
> +static int reservation_key_matches(struct multipath *mpp, uint8_t
> *key,
> +				   int noisy)
> +{
> +	struct prin_resp resp = {0};
> +	int status;
> +
> +	status = mpath_prin_activepath(mpp, MPATH_PRIN_RRES_SA,
> &resp, noisy);
> +	if (status != MPATH_PR_SUCCESS) {
> +		condlog(0, "%s: pr in read reservation command
> failed.",
> +			mpp->wwid);
> +		return YNU_UNDEF;
> +	}
> +	if (!resp.prin_descriptor.prin_readresv.additional_length)
> +		return YNU_NO;
> +	if (memcmp(key, resp.prin_descriptor.prin_readresv.key, 8)
> == 0)
> +		return YNU_YES;
> +	return YNU_NO;
> +}
> +
>  /*
>   * for MPATH_PROUT_REG_IGN_SA, we use the ignored paramp->key to
> store the
> - * currently registered key.
> + * currently registered key for use in preempt_missing_path(), but
> only if
> + * the key is holding the reservation.
>   */
>  static void set_ignored_key(struct multipath *mpp, uint8_t *key)
>  {
>  	memset(key, 0, 8);
>  	if (!get_be64(mpp->reservation_key))
>  		return;
> -	if (get_prflag(mpp->alias) == PR_UNSET)
> +	if (get_prhold(mpp->alias) == PR_UNSET)
>  		return;
> -	update_map_pr(mpp, NULL);
> -	if (mpp->prflag != PR_SET)
> +	if (reservation_key_matches(mpp, key, 0) == YNU_NO)
>  		return;
>  	memcpy(key, &mpp->reservation_key, 8);
>  }
> @@ -665,6 +711,7 @@ int do_mpath_persistent_reserve_out(vector curmp,
> vector pathvec, int fd,
>  	int ret;
>  	uint64_t prkey;
>  	struct config *conf;
> +	bool preempting_reservation = false;
>  
>  	ret = mpath_get_map(curmp, pathvec, fd, &mpp);
>  	if (ret != MPATH_PR_SUCCESS)
> @@ -715,9 +762,12 @@ int do_mpath_persistent_reserve_out(vector
> curmp, vector pathvec, int fd,
>  	case MPATH_PROUT_REG_IGN_SA:
>  		ret= mpath_prout_reg(mpp, rq_servact, rq_scope,
> rq_type, paramp, noisy);
>  		break;
> -	case MPATH_PROUT_RES_SA :
> -	case MPATH_PROUT_PREE_SA :
> -	case MPATH_PROUT_PREE_AB_SA :
> +	case MPATH_PROUT_PREE_SA:
> +	case MPATH_PROUT_PREE_AB_SA:
> +		if (reservation_key_matches(mpp, paramp->sa_key,
> noisy) == YNU_YES)
> +			preempting_reservation = true;
> +		/* fallthrough */
> +	case MPATH_PROUT_RES_SA:
>  	case MPATH_PROUT_CLEAR_SA:
>  		ret = mpath_prout_common(mpp, rq_servact, rq_scope,
> rq_type,
>  					 paramp, noisy, NULL);
> @@ -744,6 +794,15 @@ int do_mpath_persistent_reserve_out(vector
> curmp, vector pathvec, int fd,
>  	case MPATH_PROUT_CLEAR_SA:
>  		update_prflag(mpp->alias, 0);
>  		update_prkey(mpp->alias, 0);
> +		break;
> +	case MPATH_PROUT_RES_SA:
> +	case MPATH_PROUT_REL_SA:
> +		update_prhold(mpp->alias, (rq_servact ==
> MPATH_PROUT_RES_SA));
> +		break;
> +	case MPATH_PROUT_PREE_SA:
> +	case MPATH_PROUT_PREE_AB_SA:
> +		if (preempting_reservation)
> +			update_prhold(mpp->alias, 1);
>  	}
>  	return ret;
>  }
> diff --git a/libmpathpersist/mpath_updatepr.c
> b/libmpathpersist/mpath_updatepr.c
> index c8fbe4a2..dd8dd48e 100644
> --- a/libmpathpersist/mpath_updatepr.c
> +++ b/libmpathpersist/mpath_updatepr.c
> @@ -75,13 +75,13 @@ static int do_update_pr(char *alias, char *cmd,
> char *key)
>  	return ret;
>  }
>  
> -int get_prflag(char *mapname)
> +static int do_get_pr(char *mapname, const char *cmd)
>  {
>  	char str[256];
>  	char *reply;
>  	int prflag;
>  
> -	snprintf(str, sizeof(str), "getprstatus map %s", mapname);
> +	snprintf(str, sizeof(str), "%s map %s", cmd, mapname);
>  	reply = do_pr(mapname, str);
>  	if (!reply)
>  		prflag = PR_UNKNOWN;
> @@ -96,11 +96,26 @@ int get_prflag(char *mapname)
>  	return prflag;
>  }
>  
> +int get_prflag(char *mapname)
> +{
> +	return do_get_pr(mapname, "getprstatus");
> +}
> +
> +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_prhold(char *mapname, bool set)
> +{
> +	return do_update_pr(mapname, (set) ? "setprhold" :
> "unsetprhold", NULL);
> +}
> +
>  int update_prkey_flags(char *mapname, uint64_t prkey, uint8_t
> sa_flags) {
>  	char str[256];
>  
> diff --git a/libmpathpersist/mpathpr.h b/libmpathpersist/mpathpr.h
> index 912957e5..99d6b82f 100644
> --- a/libmpathpersist/mpathpr.h
> +++ b/libmpathpersist/mpathpr.h
> @@ -9,6 +9,8 @@
>  int update_prflag(char *mapname, 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);
> +int update_prhold(char *mapname, bool set);
>  #define update_prkey(mapname, prkey) update_prkey_flags(mapname,
> prkey, 0)
>  
>  #endif
> diff --git a/libmultipath/structs.h b/libmultipath/structs.h
> index 6c427d6f..97093675 100644
> --- a/libmultipath/structs.h
> +++ b/libmultipath/structs.h
> @@ -522,6 +522,7 @@ struct multipath {
>  	struct be64 reservation_key;
>  	uint8_t sa_flags;
>  	int prflag;
> +	int prhold;
>  	int all_tg_pt;
>  	struct gen_multipath generic_mp;
>  	bool fpin_must_reload;
> diff --git a/multipathd/callbacks.c b/multipathd/callbacks.c
> index fb87b280..b6b57f45 100644
> --- a/multipathd/callbacks.c
> +++ b/multipathd/callbacks.c
> @@ -69,4 +69,7 @@ void init_handler_callbacks(void)
>  	set_handler_callback(VRB_UNSETMARGINAL | Q1_PATH,
> HANDLER(cli_unset_marginal));
>  	set_handler_callback(VRB_UNSETMARGINAL | Q1_MAP,
>  			     HANDLER(cli_unset_all_marginal));
> +	set_handler_callback(VRB_GETPRHOLD | Q1_MAP,
> HANDLER(cli_getprhold));
> +	set_handler_callback(VRB_SETPRHOLD | Q1_MAP,
> HANDLER(cli_setprhold));
> +	set_handler_callback(VRB_UNSETPRHOLD | Q1_MAP,
> HANDLER(cli_unsetprhold));
>  }
> diff --git a/multipathd/cli.c b/multipathd/cli.c
> index 34588290..d0e6cebc 100644
> --- a/multipathd/cli.c
> +++ b/multipathd/cli.c
> @@ -224,7 +224,9 @@ load_keys (void)
>  	r += add_key(keys, "setmarginal", VRB_SETMARGINAL, 0);
>  	r += add_key(keys, "unsetmarginal", VRB_UNSETMARGINAL, 0);
>  	r += add_key(keys, "all", KEY_ALL, 0);
> -
> +	r += add_key(keys, "getprhold", VRB_GETPRHOLD, 0);
> +	r += add_key(keys, "setprhold", VRB_SETPRHOLD, 0);
> +	r += add_key(keys, "unsetprhold", VRB_UNSETPRHOLD, 0);
>  
>  	if (r) {
>  		free_keys(keys);
> diff --git a/multipathd/cli.h b/multipathd/cli.h
> index 9fa50145..5a943a45 100644
> --- a/multipathd/cli.h
> +++ b/multipathd/cli.h
> @@ -38,6 +38,9 @@ enum {
>  	VRB_UNSETMARGINAL	= 23,
>  	VRB_SHUTDOWN		= 24,
>  	VRB_QUIT		= 25,
> +	VRB_GETPRHOLD		= 26,
> +	VRB_SETPRHOLD		= 27,
> +	VRB_UNSETPRHOLD		= 28,
>  
>  	/* Qualifiers, values must be different from verbs */
>  	KEY_PATH		= 65,
> diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
> index a92b07ce..34910c9b 100644
> --- a/multipathd/cli_handlers.c
> +++ b/multipathd/cli_handlers.c
> @@ -1314,6 +1314,10 @@ cli_unsetprstatus(void * v, struct strbuf
> *reply, void * data)
>  		mpp->prflag = PR_UNSET;
>  		condlog(2, "%s: prflag unset", param);
>  	}
> +	if (mpp->prhold != PR_UNSET) {
> +		mpp->prhold = PR_UNSET;
> +		condlog(2, "%s: prhold unset (by clearing prflag)",
> param);
> +	}
>  
>  	return 0;
>  }
> @@ -1401,6 +1405,50 @@ cli_setprkey(void * v, struct strbuf *reply,
> void * data)
>  	return ret;
>  }
>  
> +static int do_prhold(struct vectors *vecs, char *param, int prhold)
> +{
> +	struct multipath *mpp = find_mp_by_str(vecs->mpvec, param);
> +
> +	if (!mpp)
> +		return -ENODEV;
> +
> +	if (mpp->prhold != prhold) {
> +		mpp->prhold = prhold;
> +		condlog(2, "%s: prhold %s", param, pr_str[prhold]);
> +	}
> +
> +	return 0;
> +}
> +
> +static int cli_setprhold(void *v, struct strbuf *reply, void *data)
> +{
> +	return do_prhold((struct vectors *)data, get_keyparam(v,
> KEY_MAP),
> +			 PR_SET);
> +}
> +
> +static int cli_unsetprhold(void *v, struct strbuf *reply, void
> *data)
> +{
> +	return do_prhold((struct vectors *)data, get_keyparam(v,
> KEY_MAP),
> +			 PR_UNSET);
> +}
> +
> +static int cli_getprhold(void *v, struct strbuf *reply, void *data)
> +{
> +	struct multipath *mpp;
> +	struct vectors *vecs = (struct vectors *)data;
> +	char *param = get_keyparam(v, KEY_MAP);
> +
> +	param = convert_dev(param, 0);
> +
> +	mpp = find_mp_by_str(vecs->mpvec, param);
> +	if (!mpp)
> +		return -ENODEV;
> +
> +	append_strbuf_str(reply, pr_str[mpp->prhold]);
> +	condlog(3, "%s: reply = %s", param, get_strbuf_str(reply));
> +	return 0;
> +}
> +
>  static int cli_set_marginal(void * v, struct strbuf *reply, void *
> data)
>  {
>  	struct vectors * vecs = (struct vectors *)data;
> diff --git a/multipathd/main.c b/multipathd/main.c
> index 582af880..d22425f6 100644
> --- a/multipathd/main.c
> +++ b/multipathd/main.c
> @@ -4222,6 +4222,32 @@ main (int argc, char *argv[])
>  		return (child(NULL));
>  }
>  
> +static void check_prhold(struct multipath *mpp, struct path *pp)
> +{
> +	struct prin_resp resp = {0};
> +	int status;
> +
> +	if (mpp->prflag == PR_UNSET) {
> +		mpp->prhold = PR_UNSET;
> +		return;
> +	}
> +	if (mpp->prflag != PR_SET || mpp->prhold != PR_UNKNOWN)
> +		return;

How can this situation come to pass? prflag must be PR_UNKNOWN. 
Shouldn't we reset prhold to PR_UNKNOWN as well?

> +
> +	status = prin_do_scsi_ioctl(pp->dev, MPATH_PRIN_RRES_SA,
> &resp, 0);
> +	if (status != MPATH_PR_SUCCESS) {
> +		condlog (0, "%s: pr in read reservation command
> failed: %d",
> +			 mpp->wwid, status);
> +		return;
> +	}
> +	mpp->prhold = PR_UNSET;
> +	if (!resp.prin_descriptor.prin_readresv.additional_length)
> +		return;
> +
> +	if (memcmp(&mpp->reservation_key,
> resp.prin_descriptor.prin_readresv.key, 8) == 0)
> +		mpp->prhold = PR_SET;
> +}
> +
>  static void mpath_pr_event_handle(struct path *pp)
>  {
>  	struct multipath *mpp = pp->mpp;
> @@ -4233,8 +4259,13 @@ static void mpath_pr_event_handle(struct path
> *pp)
>  		return;
>  	}
>  
> -	if (update_map_pr(mpp, pp) != MPATH_PR_SUCCESS)
> +	if (update_map_pr(mpp, pp) != MPATH_PR_SUCCESS) {
> +		if (mpp->prflag == PR_UNSET)
> +			mpp->prhold = PR_UNSET;

Perhaps we should move setting prhold to update_map_pr()?

Martin

>  		return;
> +	}
> +
> +	check_prhold(mpp, pp);
>  
>  	if (mpp->prflag != PR_SET)
>  		return;

Re: [PATCH 00/15] Improve mpathpersist's unavailable path handling

[Martin Wilck]

Hi Ben,

On Thu, 2025-07-10 at 14:10 -0400, Benjamin Marzinski wrote:
> A problem that mpathpersist has with making SCSI Persistent
> Resevations
> to a multipath device work like they do to individual SCSI devices is
> that some of the paths to a multipath device might be down or missing
> when the mpathpersist commands are run. Multipath handles registering
> a
> new key pretty well. If paths are unavailable at the time of the
> command, the key is registered when they later become available.  But
> if
> the multipath device is also holding a reservation on one of its
> paths,
> things get trickier.
> 
> If a persistent reservation is being held by an unsuable path of a
> multipath device (the path can either be down or completely removed),
> libmpathpersist can't change it just by forwarding the regular
> persistent reservation commands. This can cause problems both for the
> RELEASE command and the REGISTER and REGISTER AND IGNORE commands if
> they are used to change from one key to another. If the path holding
> the
> reservation is unavailable, the reservation won't be released or have
> its key changed, as expected. I wish the problem of having a
> reservation
> key changed while it is holding the reservation was simply a
> theoretical
> one, but there are enterprise users of multipath that need this
> capability.
> 
> This patchset deals with both of these problems. libmpathpersist
> always
> had code to handle releasing a reservation held by an unavailable
> path,
> but the existing method is broken. It relies on poorly supported
> optional features of SCSI Persistent Reservations: the READ FULL
> STATUS
> command and specifying Initiator Ports with the REGISTER command
> (SIP_C). Also, fixing its current issues would additionally require
> supporting the All Target Ports option (ATP_C). This existing
> workaround
> has been redesigned to use the PREEMPT command instead. Key changes
> where the path holding the reservation is unavailable were not
> previously handled by libmpathpersist. This patchset also handles
> them
> using the PREEMPT command.
> 
> patches 0001-0008 are simply cleanups an prep work.
> patches 0009-0010 redesign the RELEASE command workaround
> patch 0011 creates a workaround for the REGISTER command
> patch 0012 makes this work for the REGISTER AND IGNORE command as
> well
> patches 0013-0014 are more prep work
> patch 0015 Adds a safety check before preempting a key, so that only
>            devices that are holding a reservation will do the
>            preemption
> 
> These workarounds depend on the fact that managing scsi persistent
> reservations for multipath devices has never worked correctly if
> multiple nodes use the same persistent reservation key for the same
> device. multipathd needs to be able to register the key on new paths,
> but the key could have been preempted since it was registered for the
> multipath device. To make sure that multipath isn't automatically
> registering paths for a device that has been preempted, multipathd
> checks that the configured key has already been registered by other
> paths of the device.  Unfortunately, there is no way to verify that
> the
> other paths to the multipath device on this node are the ones holding
> the keys in question. If another node is using the same key for the
> device, those registered keys could belong to paths on the other
> node,
> and there could be no registered keys from this node (likely due to
> being preempted). If multipathd registered the key for this new path
> because it saw another node's keys, then a preempted node would
> suddenly
> gain access to the storage.
> 
> As far as the workarounds from this patchset go, if there was another
> node using the same key, then preempting the key in these workarounds
> would preempt the other node as well. The safety check in patch 0015
> tries to make sure that this only happens when the current is holding
> the reservation, but this isn't foolproof.
> 
> I should also note that while working on this patchset, I noticed a
> number of other issues with the persistent reservation code, mostly
> involving changing a registered key. I'll be fixing them in a future
> patchset.
> 
> Benjamin Marzinski (15):
>   multipathd: remove thread from mpath_pr_event_handle
>   libmpathpersist: remove uneeded wrapper function.
>   libmpathpersist: reduce log level for persistent reservation
> checking
>   libmpathpersist: remove pointless update_map_pr ret value code
>   multipathd: use update_map_pr in mpath_pr_event_handle
>   libmpathpersist: limit changing prflag in update_map_pr
>   multipathd: Don't call update_map_pr unnecessarily
>   libmpathpersist: remove useless function send_prout_activepath
>   limpathpersist: redesign failed release workaround
>   libmpathpersist: fail the release if all threads fail
>   limpathpersist: Handle changing key corner case
>   libmapthpersist: Handle REGISTER AND IGNORE changing key corner
> case
>   libmultipath: rename prflag_value enums
>   libmpathpersist: use a switch statement for prout command
> finalizing
>   libmpathpersist: Add safety check for preempting on key change
> 
>  libmpathpersist/libmpathpersist.version |   2 +-
>  libmpathpersist/mpath_persist_int.c     | 505 ++++++++++++++--------
> --
>  libmpathpersist/mpath_persist_int.h     |   2 +-
>  libmpathpersist/mpath_updatepr.c        |  74 +++-
>  libmpathpersist/mpathpr.h               |   3 +
>  libmultipath/libmultipath.version       |   1 +
>  libmultipath/structs.h                  |  10 +-
>  multipathd/callbacks.c                  |   3 +
>  multipathd/cli.c                        |   4 +-
>  multipathd/cli.h                        |   3 +
>  multipathd/cli_handlers.c               |  70 +++-
>  multipathd/main.c                       | 149 +++----
>  12 files changed, 483 insertions(+), 343 deletions(-)

For the series, except those patches I've replied to:
Reviewed-by: Martin Wilck <mwilck@suse.com>

I have taken the liberty to apply the changes suggested by the clang-
format style checker and the fixes for the previously mentioned gcc4.8
compile errors in my tip branch. I also already added my Reviewed-by:
tags there.

I should add that while the logic of this patch set makes a lot of
sense to me and I've done my best to do a careful review, I don't feel
that understand the possible corner cases well enough to be able to
confirm that they can't possibly have unwanted side effects. Doing so
would require a lot more testing and deep-diving into the matter than
I've had time for. You are more exprienced with PR scenarios than me. 
Your analysis of the various cases is as solid as it gets, AFAICT.

Thanks
Martin








Martin

[1] https://github.com/openSUSE/multipath-tools/tree/tip

Re: [PATCH 00/15] Improve mpathpersist's unavailable path handling

[Hannes Reinecke]

On 7/10/25 20:10, Benjamin Marzinski wrote:
> A problem that mpathpersist has with making SCSI Persistent Resevations
> to a multipath device work like they do to individual SCSI devices is
> that some of the paths to a multipath device might be down or missing
> when the mpathpersist commands are run. Multipath handles registering a
> new key pretty well. If paths are unavailable at the time of the
> command, the key is registered when they later become available.  But if
> the multipath device is also holding a reservation on one of its paths,
> things get trickier.
> 
> If a persistent reservation is being held by an unsuable path of a
> multipath device (the path can either be down or completely removed),
> libmpathpersist can't change it just by forwarding the regular
> persistent reservation commands. This can cause problems both for the
> RELEASE command and the REGISTER and REGISTER AND IGNORE commands if
> they are used to change from one key to another. If the path holding the
> reservation is unavailable, the reservation won't be released or have
> its key changed, as expected. I wish the problem of having a reservation
> key changed while it is holding the reservation was simply a theoretical
> one, but there are enterprise users of multipath that need this
> capability.
> 
> This patchset deals with both of these problems. libmpathpersist always
> had code to handle releasing a reservation held by an unavailable path,
> but the existing method is broken. It relies on poorly supported
> optional features of SCSI Persistent Reservations: the READ FULL STATUS
> command and specifying Initiator Ports with the REGISTER command
> (SIP_C). Also, fixing its current issues would additionally require
> supporting the All Target Ports option (ATP_C). This existing workaround
> has been redesigned to use the PREEMPT command instead. Key changes
> where the path holding the reservation is unavailable were not
> previously handled by libmpathpersist. This patchset also handles them
> using the PREEMPT command.
> 
I wish we had a testcase for all of that. Persistent reservation
handling is tricky at the best of times, but throwing in multipathing
it really gets into the arcane knowledge area.
Ben, do you have something which we could turn into some blktest
scenarios?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 03/15] libmpathpersist: reduce log level for persistent reservation checking

[Martin Wilck]

On Sun, 2025-08-24 at 14:57 +0200, Martin Wilck wrote:
> On Thu, 2025-07-10 at 14:10 -0400, Benjamin Marzinski wrote:
> > Move logging of minor expected behavior to INFO level. Modify the
> > log
> > level of some messages by whether or not mpp->prflag changed
> > values.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmpathpersist/mpath_persist_int.c | 26 ++++++++++++++-----------
> > -
> >  1 file changed, 14 insertions(+), 12 deletions(-)
> > 
> > diff --git a/libmpathpersist/mpath_persist_int.c
> > b/libmpathpersist/mpath_persist_int.c
> > index 612bbed9..4172167a 100644
> > --- a/libmpathpersist/mpath_persist_int.c
> > +++ b/libmpathpersist/mpath_persist_int.c
> >  	}
> > @@ -761,22 +761,24 @@ int update_map_pr(struct multipath *mpp)
> > 
> >  keys.key_list[i*8], 8 , 1);
> > +		if (libmp_verbosity >= 3) {
> > +			condlog(3, "%s: PR IN READKEYS[%d]
> > reservation key:",
> > +				mpp->alias, i);
> > +			dumpHex((char *)&resp-
> > > prin_descriptor.prin_readkeys.key_list[i * 8], 8, 1);
> > +		}
> 
> I'd suggest to restrict usage of dumpHex() to verbosity 4 and higher.
> 
> >  
> > -		if (!memcmp(&mpp->reservation_key, &resp-
> > > prin_descriptor.prin_readkeys.key_list[i*8], 8))
> > -		{
> > -			condlog(2, "%s: reservation key found in
> > pr
> > in readkeys response", mpp->alias);
> > +		if (!memcmp(&mpp->reservation_key, &resp-
> > > prin_descriptor.prin_readkeys.key_list[i * 8], 8)) {
> > +			condlog(3, "%s: reservation key found in
> > pr
> > in readkeys response", mpp->alias);
> 
> This one should be logged at verbosity 4 only, too, IMO.

You made these changes in 03/14 of your 2nd set.
So this one is fine.

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

Martin

Re: [PATCH 15/15] libmpathpersist: Add safety check for preempting on key change

[Martin Wilck]

On Sun, 2025-08-24 at 23:00 +0200, Martin Wilck wrote:
> On Thu, 2025-07-10 at 14:11 -0400, Benjamin Marzinski wrote:
> > When a reservation key is changed from one non-zero value to
> > another,
> > libmpathpersist checks if the old key is still holding the
> > reservation,
> > and preempts it if it is. This was only safe if two nodes never
> > share
> > the same key. If a node uses the same key as another node that is
> > holding the reservation, and switches keys so that it no longer
> > matches,
> > it will end up preempting the reservation. This is clearly
> > unexpected
> > behavior, and it can come about by a simple accident of registering
> > a
> > device with the wrong key, and then immediately fixing it.
> > 
> > To handle this, add code to track if a device is the reservation
> > holder
> > to multipathd. multipathd now has three new commands "getprhold",
> > "setprhold", and "unsetprhold". These commands work like the
> > equivalent
> > *prstatus commands. libmpathpersist calls setprhold on RESERVE
> > commands
> > and PREEMPT commands when the preempted key is holding the
> > reservation
> > and unsetprhold on RELEASE commands. Also, calling unsetprflag
> > causes
> > prhold to be unset as well, so CLEAR commands and REGISTER commands
> > with
> > a 0x0 service action key will also unset prhold. libmpathpersist()
> > will
> > also unset prhold if it notices that the device cannot be holding a
> > reservation in preempt_missing_path().
> > 
> > When a new multipath device is created, its initial prhold state is
> > PR_UNKNOWN until it checks the current reservation, just like with
> > prflag. If multipathd ever finds that a device's registration has
> > been
> > preempted or cleared in update_map_pr(), it unsets prhold, just
> > like
> > with prflag.
> > 
> > Now, before libmpathpersist preempts a reservation when changing
> > keys
> > it also checks if multipathd thinks that the device is holding
> > the reservation. If it does not, then libmpathpersist won't preempt
> > the key. It will assume that another node is holding the
> > reservation
> > with the same key.
> > 
> > I should note that this safety check only stops a node not holding
> > the
> > reservation from preempting the node holding the reservation. If
> > the
> > node holding the reservation changes its key, but it fails to
> > change
> > the
> > resevation key, because that path is down or gone, it will still
> > issue
> > the preempt to move the reservation to a usable path, even if
> > another
> > node is using the same key. This will remove the registrations for
> > that
> > other node. It also will not work correctly if multipathd stops
> > tracking
> > a device for some reason. It's only a best-effort safety check.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmpathpersist/mpath_persist_int.c | 87 ++++++++++++++++++++++++-
> > --
> > --
> >  libmpathpersist/mpath_updatepr.c    | 19 ++++++-
> >  libmpathpersist/mpathpr.h           |  2 +
> >  libmultipath/structs.h              |  1 +
> >  multipathd/callbacks.c              |  3 +
> >  multipathd/cli.c                    |  4 +-
> >  multipathd/cli.h                    |  3 +
> >  multipathd/cli_handlers.c           | 48 ++++++++++++++++
> >  multipathd/main.c                   | 33 ++++++++++-
> >  9 files changed, 182 insertions(+), 18 deletions(-)
> > 
> 
> Two questions all the way down.
> 
> >  
> > +static void check_prhold(struct multipath *mpp, struct path *pp)
> > +{
> > +	struct prin_resp resp = {0};
> > +	int status;
> > +
> > +	if (mpp->prflag == PR_UNSET) {
> > +		mpp->prhold = PR_UNSET;
> > +		return;
> > +	}
> > +	if (mpp->prflag != PR_SET || mpp->prhold != PR_UNKNOWN)
> > +		return;
> 
> How can this situation come to pass? prflag must be PR_UNKNOWN. 
> Shouldn't we reset prhold to PR_UNKNOWN as well?

Forget this remark. I was confused.

> 
> > +
> > +	status = prin_do_scsi_ioctl(pp->dev, MPATH_PRIN_RRES_SA,
> > &resp, 0);
> > +	if (status != MPATH_PR_SUCCESS) {
> > +		condlog (0, "%s: pr in read reservation command
> > failed: %d",
> > +			 mpp->wwid, status);
> > +		return;
> > +	}
> > +	mpp->prhold = PR_UNSET;
> > +	if (!resp.prin_descriptor.prin_readresv.additional_length)
> > +		return;
> > +
> > +	if (memcmp(&mpp->reservation_key,
> > resp.prin_descriptor.prin_readresv.key, 8) == 0)
> > +		mpp->prhold = PR_SET;
> > +}
> > +
> >  static void mpath_pr_event_handle(struct path *pp)
> >  {
> >  	struct multipath *mpp = pp->mpp;
> > @@ -4233,8 +4259,13 @@ static void mpath_pr_event_handle(struct
> > path
> > *pp)
> >  		return;
> >  	}
> >  
> > -	if (update_map_pr(mpp, pp) != MPATH_PR_SUCCESS)
> > +	if (update_map_pr(mpp, pp) != MPATH_PR_SUCCESS) {
> > +		if (mpp->prflag == PR_UNSET)
> > +			mpp->prhold = PR_UNSET;
> 
> Perhaps we should move setting prhold to update_map_pr()?

You fixed this in your 2nd set.

Martin

Re: [PATCH 00/15] Improve mpathpersist's unavailable path handling

[Benjamin Marzinski]

On Mon, Aug 25, 2025 at 08:38:38AM +0200, Hannes Reinecke wrote:
> On 7/10/25 20:10, Benjamin Marzinski wrote:
> > A problem that mpathpersist has with making SCSI Persistent Resevations
> > to a multipath device work like they do to individual SCSI devices is
> > that some of the paths to a multipath device might be down or missing
> > when the mpathpersist commands are run. Multipath handles registering a
> > new key pretty well. If paths are unavailable at the time of the
> > command, the key is registered when they later become available.  But if
> > the multipath device is also holding a reservation on one of its paths,
> > things get trickier.
> > 
> > If a persistent reservation is being held by an unsuable path of a
> > multipath device (the path can either be down or completely removed),
> > libmpathpersist can't change it just by forwarding the regular
> > persistent reservation commands. This can cause problems both for the
> > RELEASE command and the REGISTER and REGISTER AND IGNORE commands if
> > they are used to change from one key to another. If the path holding the
> > reservation is unavailable, the reservation won't be released or have
> > its key changed, as expected. I wish the problem of having a reservation
> > key changed while it is holding the reservation was simply a theoretical
> > one, but there are enterprise users of multipath that need this
> > capability.
> > 
> > This patchset deals with both of these problems. libmpathpersist always
> > had code to handle releasing a reservation held by an unavailable path,
> > but the existing method is broken. It relies on poorly supported
> > optional features of SCSI Persistent Reservations: the READ FULL STATUS
> > command and specifying Initiator Ports with the REGISTER command
> > (SIP_C). Also, fixing its current issues would additionally require
> > supporting the All Target Ports option (ATP_C). This existing workaround
> > has been redesigned to use the PREEMPT command instead. Key changes
> > where the path holding the reservation is unavailable were not
> > previously handled by libmpathpersist. This patchset also handles them
> > using the PREEMPT command.
> > 
> I wish we had a testcase for all of that. Persistent reservation
> handling is tricky at the best of times, but throwing in multipathing
> it really gets into the arcane knowledge area.
> Ben, do you have something which we could turn into some blktest
> scenarios?

It wouldn't be hard to use the LIO target to setup these scenarios, and
verify that mpathpersist is handling them. The bigger issue is that I'm
still occassionally running into new ones. I've got a couple more
patches to send to deal with them, but what this actually wants (and
what I plan to write after I think I've handled all the issues) is a
test that will write to the devices while randomly failing and restoring
paths and doing various PR commands, both to check that commands succeed
and fail when expected given the state of the devices when they were
run, and that we don't end up with active paths that either don't have
reservations when they should, or do have them when they shouldn't.

I can look at adding something like that to blktest.

-Ben

> 
> Cheers,
> 
> Hannes
> -- 
> Dr. Hannes Reinecke                  Kernel Storage Architect
> hare@suse.de                                +49 911 74053 688
> SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
> HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 09/15] limpathpersist: redesign failed release workaround

[Benjamin Marzinski]

On Sun, Aug 24, 2025 at 05:26:50PM +0200, Martin Wilck wrote:
> Hi Ben,
> 
> I like this approach, but I have a few questions.
> 
> On Thu, 2025-07-10 at 14:10 -0400, Benjamin Marzinski wrote:
> > The workaround for releasing a reservation held by a failed path
> > (clearing all persistent reservation data and then reregistering all
> > the
> > keys) has several problems. It requires devices that support the READ
> > FULL STATUS command and are capable of specifying TransportIDs with
> > REGISTRATION commands (SIP_C), neither of which are mandatory to
> > support
> > SCSI Persistent Reservations.
> 
> While I have made similar experiences, I wonder where the SCSI specs
> state which service actions and which parameters are mandatory for
> Persistent Reservation commands.
> 
> I've tried to figure it out from SPC-6, but I couldn't.

I'm not sure where/if the spec says READ FULL STATUS is optional, but
I've run into multiple devices that support Persistent Reservations but
don't support the READ FULL STATUS service action. You can check a
device's support for specifying transport IDs with REGISTRATION commands
by looking at the SIP_C bit from the REPORT CAPABILITIES data. Most
devices I've checked don't support it.
 
> >  Non SIP_C devices will be left with no
> > registered keys. Also, not all cleared keys are registered, just the
> > ones going to the Target Port receiving the REGISTRATION command. To
> > reregister all the keys, the code would have to also use the "All
> > Target
> > Ports" flag to register keys on different Target Ports, but this
> > could
> > end up registering keys on paths they aren't supposed to be on (for
> > instance if one of the registered keys was only there because the
> > path
> > was down and it couldn't be released).
> > 
> > The redesign avoids these issues by only using mandatory Persistent
> > Reservation commands, without extra optional parameters or flags, and
> > only effects the keys of the multipath device it is being issued on.
> > The new workaround is:
> > 1. Suspend the multipath device to prevent I/O
> > 2. Preempt the key to move the reservation to an available path. This
> >    also removes the registered keys from every path except the path
> >    issuing the PREEMPT command. Since the device is suspended, not
> > I/O
> >    can go to these unregisted paths and fail.
> > 3. Release the reservation on the path that now holds it.
> 
> Why that? Can't the reservation just be kept?

Err... because this is the workaround for when releasing the reservation
fails because it is being held by a failed path. Releasing it is the
whole point.
 
> > 4. Resume the device (since it no longer matters that most of the
> > paths
> >    no longer have a registered key)
> > 5. Reregister the keys on all the paths.
> 
> Why not re-register before resuming?

The idea is that since we dropped the reservation, it doesn't matter
whether or not the paths are holding keys. But I'm going to change this.
We need to run the code for this workaround (with re-registering the key
on all paths before resuming) whenever a multipath device preempts its
own reservation key.  For instance:

# mpathpersist -oPK 0x123abc -S 0x123abc /dev/mapper/mpatha

assuming mpatha is using the key 0x123abc. When you run the equivalent
sg_persist command, say:

# sg_persist -oPK 0x123abc -S 0x123abc /dev/sda

you should expect any other I_T Nexus that had the reservation key
0x123abc to have its key unregistered, while /dev/sda would keep its key
(and hold the reservation, if it was previously held by an I_T Nexus
with the reservation key 0x123abc). That means IO to /dev/sda should
never fail because it issued a PREEMPT.

When mpathpersist runs this command, it unregisters the keys from every
path of the multipath device, aside from the path that issued the PR
command. That means that IO to the multipath device could start failing,
if it is sent to one of the paths that lost its key. To avoid this, when
a multipath device preempts its own key, it needs to suspend and
reregister all its paths before resuming.

I will be sending a patch to do this.

> > 
> > If steps 3 or 4 fail, the code will attempt to reregister the keys,
> > and
> > then attempt (or possibly re-attempt) the resume.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmpathpersist/mpath_persist_int.c | 173 ++++++++++++--------------
> > --
> >  libmultipath/libmultipath.version   |   1 +
> >  2 files changed, 76 insertions(+), 98 deletions(-)
> > 
> > diff --git a/libmpathpersist/mpath_persist_int.c
> > b/libmpathpersist/mpath_persist_int.c
> > index 13829742..7fb08b2e 100644
> > --- a/libmpathpersist/mpath_persist_int.c
> > +++ b/libmpathpersist/mpath_persist_int.c
> > @@ -366,7 +366,8 @@ static int mpath_prout_reg(struct multipath
> > *mpp,int rq_servact, int rq_scope,
> >  
> >  static int mpath_prout_common(struct multipath *mpp,int rq_servact,
> > int rq_scope,
> >  			      unsigned int rq_type,
> > -			      struct prout_param_descriptor* paramp,
> > int noisy)
> > +			      struct prout_param_descriptor* paramp,
> > int noisy,
> > +			      struct path **pptr)
> >  {
> >  	int i,j, ret;
> >  	struct pathgroup *pgp = NULL;
> > @@ -385,6 +386,8 @@ static int mpath_prout_common(struct multipath
> > *mpp,int rq_servact, int rq_scope
> >  			found = true;
> >  			ret = prout_do_scsi_ioctl(pp->dev,
> > rq_servact, rq_scope,
> >  						  rq_type, paramp,
> > noisy);
> > +			if (ret == MPATH_PR_SUCCESS && pptr)
> > +				*pptr = pp;
> >  			if (ret != MPATH_PR_RETRYABLE_ERROR)
> >  				return ret;
> >  		}
> > @@ -405,14 +408,12 @@ static int mpath_prout_rel(struct multipath
> > *mpp,int rq_servact, int rq_scope,
> >  	struct path *pp = NULL;
> >  	int active_pathcount = 0;
> >  	pthread_attr_t attr;
> > -	int rc, found = 0;
> > +	int rc;
> >  	int count = 0;
> >  	int status = MPATH_PR_SUCCESS;
> > -	struct prin_resp resp;
> > -	struct prout_param_descriptor *pamp = NULL;
> > -	struct prin_resp *pr_buff;
> > -	int length;
> > -	struct transportid *pptr = NULL;
> > +	struct prin_resp resp = {0};
> > +	uint16_t udev_flags = (mpp->skip_kpartx) ?
> > MPATH_UDEV_NO_KPARTX_FLAG : 0;
> > +	bool did_resume = false;
> >  
> >  	if (!mpp)
> >  		return MPATH_PR_DMMP_ERROR;
> > @@ -502,104 +503,78 @@ static int mpath_prout_rel(struct multipath
> > *mpp,int rq_servact, int rq_scope,
> >  	}
> >  
> >  	condlog (2, "%s: Path holding reservation is not
> > available.", mpp->wwid);
> > -
> > -	pr_buff =  mpath_alloc_prin_response(MPATH_PRIN_RFSTAT_SA);
> > -	if (!pr_buff){
> > -		condlog (0, "%s: failed to  alloc pr in response
> > buffer.", mpp->wwid);
> > +	/*
> > +	 * Cannot free the reservation because the path that is
> > holding it
> > +	 * is not usable. Workaround this by:
> > +	 * 1. Suspending the device
> > +	 * 2. Preempting the reservation to move it to a usable path
> > +	 *    (this removes the registered keys on all paths except
> > the
> > +	 *    preempting one. Since the device is suspended, no IO
> > can
> > +	 *    go to these unregistered paths and fail).
> > +	 * 3. Releasing the reservation on the path that now holds
> > it.
> > +	 * 4. Resuming the device (since it no longer matters that
> > most of
> > +	 *    that paths no longer have a registered key)
> > +	 * 5. Reregistering keys on all the paths
> > +	 */
> > +
> > +	if (!dm_simplecmd_noflush(DM_DEVICE_SUSPEND, mpp->alias, 0))
> > {
> > +		condlog(0, "%s: release: failed to suspend dm
> > device.",
> 
> Why do you use dm_simplecmd_noflush() here? Shouldn't queued IO be
> flushed from the dm device to avoid it being sent to paths that are
> going to be unregistered?
> 

I'm pretty certain that DM will still flush all the IO from the target
to DM core before suspending, even with dm_simplecmd_noflush() set. In
request based multipath, queued IOs are never stored in the target. In
bio based multipath, they are, but they will get flushed back up to DM
core when suspending and queued there. No IO should happen through the
target after the suspend, until the resume. dm_simplecmd_noflush() just
keeps multipath from failing any IO that it had queueing, and it's only
really necessary when we resize the device, because if we shrink the
device, outstanding IO might be outside the new bounds.
 
> 
> > mpp->wwid);
> >  		return MPATH_PR_OTHER;
> >  	}
> >  
> > -	status = mpath_prin_activepath (mpp, MPATH_PRIN_RFSTAT_SA,
> > pr_buff, noisy);
> > -
> > -	if (status != MPATH_PR_SUCCESS){
> > -		condlog (0,  "%s: pr in read full status command
> > failed.",  mpp->wwid);
> > -		goto out;
> > -	}
> > -
> > -	num = pr_buff-
> > >prin_descriptor.prin_readfd.number_of_descriptor;
> > -	if (0 == num){
> > -		goto out;
> > -	}
> > -	length = sizeof (struct prout_param_descriptor) + (sizeof
> > (struct transportid *));
> > -
> > -	pamp = (struct prout_param_descriptor *)malloc (length);
> > -	if (!pamp){
> > -		condlog (0, "%s: failed to alloc pr out parameter.",
> > mpp->wwid);
> > -		goto out;
> > -	}
> > -
> > -	memset(pamp, 0, length);
> > -
> > -	pamp->trnptid_list[0] = (struct transportid *) malloc
> > (sizeof (struct transportid));
> > -	if (!pamp->trnptid_list[0]){
> > -		condlog (0, "%s: failed to alloc pr out
> > transportid.", mpp->wwid);
> > -		goto out1;
> > -	}
> > -	pptr = pamp->trnptid_list[0];
> > -
> > -	if (get_be64(mpp->reservation_key)){
> > -		memcpy (pamp->key, &mpp->reservation_key, 8);
> > -		condlog (3, "%s: reservation key set.", mpp->wwid);
> > +	memset(paramp, 0, sizeof(*paramp));
> > +	memcpy(paramp->key, &mpp->reservation_key, 8);
> > +	memcpy(paramp->sa_key, &mpp->reservation_key, 8);
> > +	status = mpath_prout_common(mpp, MPATH_PROUT_PREE_SA,
> > rq_scope, rq_type,
> > +				    paramp, noisy, &pp);
> > +	if (status != MPATH_PR_SUCCESS) {
> > +		condlog(0, "%s: release: pr preempt command
> > failed.", mpp->wwid);
> > +		goto fail_resume;
> >  	}
> >  
> > -	status = mpath_prout_common (mpp, MPATH_PROUT_CLEAR_SA,
> > -				     rq_scope, rq_type, pamp,
> > noisy);
> > -
> > -	if (status) {
> > -		condlog(0, "%s: failed to send CLEAR_SA", mpp-
> > >wwid);
> > -		goto out2;
> > +	memset(paramp, 0, sizeof(*paramp));
> > +	memcpy(paramp->key, &mpp->reservation_key, 8);
> > +	status = prout_do_scsi_ioctl(pp->dev, MPATH_PROUT_REL_SA,
> > rq_scope,
> > +				     rq_type, paramp, noisy);
> > +	if (status != MPATH_PR_SUCCESS) {
> > +		condlog(0, "%s: release on alternate path failed.",
> > mpp->wwid);
> > +		goto out_reregister;
> >  	}
> >  
> > -	pamp->num_transportid = 1;
> > -
> > -	for (i = 0; i < num; i++){
> > -		if (get_be64(mpp->reservation_key) &&
> > -			memcmp(pr_buff-
> > >prin_descriptor.prin_readfd.descriptors[i]->key,
> > -			       &mpp->reservation_key, 8)){
> > -			/*register with transport id*/
> > -			memset(pamp, 0, length);
> > -			pamp->trnptid_list[0] = pptr;
> > -			memset (pamp->trnptid_list[0], 0, sizeof
> > (struct transportid));
> > -			memcpy (pamp->sa_key,
> > -					pr_buff-
> > >prin_descriptor.prin_readfd.descriptors[i]->key, 8);
> > -			pamp->sa_flags = MPATH_F_SPEC_I_PT_MASK;
> > -			pamp->num_transportid = 1;
> > -
> > -			memcpy (pamp->trnptid_list[0],
> > -					&pr_buff-
> > >prin_descriptor.prin_readfd.descriptors[i]->trnptid,
> > -					sizeof (struct
> > transportid));
> > -			status = mpath_prout_common (mpp,
> > MPATH_PROUT_REG_SA, 0, rq_type,
> > -					pamp, noisy);
> > -
> > -			pamp->sa_flags = 0;
> > -			memcpy (pamp->key, pr_buff-
> > >prin_descriptor.prin_readfd.descriptors[i]->key, 8);
> > -			memset (pamp->sa_key, 0, 8);
> > -			pamp->num_transportid = 0;
> > -			status = mpath_prout_common (mpp,
> > MPATH_PROUT_REG_SA, 0, rq_type,
> > -					pamp, noisy);
> > -		}
> > -		else
> > -		{
> > -			if (get_be64(mpp->reservation_key))
> > -				found = 1;
> > -		}
> > -
> > -
> > -	}
> > -
> > -	if (found){
> > -		memset (pamp, 0, length);
> > -		memcpy (pamp->sa_key, &mpp->reservation_key, 8);
> > -		memset (pamp->key, 0, 8);
> > -		status = mpath_prout_reg(mpp, MPATH_PROUT_REG_SA,
> > rq_scope, rq_type, pamp, noisy);
> > +	if (!dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias,
> > udev_flags)) {
> > +		condlog(0, "%s release: failed to resume dm
> > device.", mpp->wwid);
> > +		/*
> > +		 * leave status set to MPATH_PR_SUCCESS, we will
> > have another
> > +		 * chance to resume the device.
> > +		 */
> > +		goto out_reregister;
> > +	}
> > +	did_resume = true;
> > +
> > +out_reregister:
> > +	memset(paramp, 0, sizeof(*paramp));
> > +	memcpy(paramp->sa_key, &mpp->reservation_key, 8);
> > +	rc = mpath_prout_reg(mpp, MPATH_PROUT_REG_IGN_SA, rq_scope,
> > rq_type,
> > +			     paramp, noisy);
> > +	if (rc != MPATH_PR_SUCCESS)
> > +		condlog(0, "%s: release: failed to reregister
> > paths.", mpp->wwid);
> > +
> > +	/*
> > +	 * If we failed releasing the reservation or resuming
> > earlier
> > +	 * try resuming now. Otherwise, return with the
> > reregistering status
> > +	 * This means we will report failure, even though the
> > resevation
> > +	 * has been released, since the keys were not reregistered.
> > +	 */
> > +	if (did_resume)
> > +		return rc;
> > +	else if (status == MPATH_PR_SUCCESS)
> > +		status = rc;
> > +fail_resume:
> > +	if (!dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias,
> > udev_flags)) {
> > +		condlog(0, "%s: release: failed to resume dm
> > device.", mpp->wwid);
> > +		if (status == MPATH_PR_SUCCESS)
> > +			status = MPATH_PR_OTHER;
> >  	}
> > -
> > -out2:
> > -	free(pptr);
> > -out1:
> > -	free (pamp);
> > -out:
> > -	free (pr_buff);
> >  	return (status == MPATH_PR_RETRYABLE_ERROR) ? MPATH_PR_OTHER
> > : status;
> >  }
> >  
> > @@ -620,6 +595,7 @@ int do_mpath_persistent_reserve_out(vector curmp,
> > vector pathvec, int fd,
> >  	mpp->mpe = find_mpe(conf->mptable, mpp->wwid);
> >  	select_reservation_key(conf, mpp);
> >  	select_all_tg_pt(conf, mpp);
> > +	select_skip_kpartx(conf, mpp);
> 
> I understand that this is because of the DM_DEVICE_RESUME, but it
> deserves a comment here as it seems a bit out of place in this code
> that deals only with persistent reservations. Actually, we may want to
> move this logic to dm_simplecmd().

Sure. I'll take a look at this, and comment or move it.

-Ben
 
> Thanks,
> Martin
> 
> >  	put_multipath_config(conf);
> >  
> >  	memcpy(&prkey, paramp->sa_key, 8);
> > @@ -661,7 +637,8 @@ int do_mpath_persistent_reserve_out(vector curmp,
> > vector pathvec, int fd,
> >  	case MPATH_PROUT_PREE_SA :
> >  	case MPATH_PROUT_PREE_AB_SA :
> >  	case MPATH_PROUT_CLEAR_SA:
> > -		ret = mpath_prout_common(mpp, rq_servact, rq_scope,
> > rq_type, paramp, noisy);
> > +		ret = mpath_prout_common(mpp, rq_servact, rq_scope,
> > rq_type,
> > +					 paramp, noisy, NULL);
> >  		break;
> >  	case MPATH_PROUT_REL_SA:
> >  		ret = mpath_prout_rel(mpp, rq_servact, rq_scope,
> > rq_type, paramp, noisy);
> > diff --git a/libmultipath/libmultipath.version
> > b/libmultipath/libmultipath.version
> > index a6718355..2f47b3ad 100644
> > --- a/libmultipath/libmultipath.version
> > +++ b/libmultipath/libmultipath.version
> > @@ -249,4 +249,5 @@ local:
> >  LIBMULTIPATH_29.1.0 {
> >  global:
> >  	can_recheck_wwid;
> > +	select_skip_kpartx;
> >  } LIBMULTIPATH_29.0.0;

Re: [PATCH 00/15] Improve mpathpersist's unavailable path handling

[Hannes Reinecke]

On 8/25/25 21:56, Benjamin Marzinski wrote:
> On Mon, Aug 25, 2025 at 08:38:38AM +0200, Hannes Reinecke wrote:
>> On 7/10/25 20:10, Benjamin Marzinski wrote:
[ .. ]>>>
>>> This patchset deals with both of these problems. libmpathpersist always
>>> had code to handle releasing a reservation held by an unavailable path,
>>> but the existing method is broken. It relies on poorly supported
>>> optional features of SCSI Persistent Reservations: the READ FULL STATUS
>>> command and specifying Initiator Ports with the REGISTER command
>>> (SIP_C). Also, fixing its current issues would additionally require
>>> supporting the All Target Ports option (ATP_C). This existing workaround
>>> has been redesigned to use the PREEMPT command instead. Key changes
>>> where the path holding the reservation is unavailable were not
>>> previously handled by libmpathpersist. This patchset also handles them
>>> using the PREEMPT command.
>>>
>> I wish we had a testcase for all of that. Persistent reservation
>> handling is tricky at the best of times, but throwing in multipathing
>> it really gets into the arcane knowledge area.
>> Ben, do you have something which we could turn into some blktest
>> scenarios?
> 
> It wouldn't be hard to use the LIO target to setup these scenarios, and
> verify that mpathpersist is handling them. The bigger issue is that I'm
> still occassionally running into new ones. I've got a couple more
> patches to send to deal with them, but what this actually wants (and
> what I plan to write after I think I've handled all the issues) is a
> test that will write to the devices while randomly failing and restoring
> paths and doing various PR commands, both to check that commands succeed
> and fail when expected given the state of the devices when they were
> run, and that we don't end up with active paths that either don't have
> reservations when they should, or do have them when they shouldn't.
> 
> I can look at adding something like that to blktest.
> 
That would be awesome. I am looking into updating/modifying PR handling
in qemu by using the in-kernel generic PR support, but to validate that
testcases would be really helpful.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH 09/15] limpathpersist: redesign failed release workaround

[Martin Wilck]

On Mon, 2025-08-25 at 20:51 -0400, Benjamin Marzinski wrote:
> On Sun, Aug 24, 2025 at 05:26:50PM +0200, Martin Wilck wrote:
> 
> > 
> > > +	/*
> > > +	 * Cannot free the reservation because the path that is
> > > holding it
> > > +	 * is not usable. Workaround this by:
> > > +	 * 1. Suspending the device
> > > +	 * 2. Preempting the reservation to move it to a usable
> > > path
> > > +	 *    (this removes the registered keys on all paths
> > > except
> > > the
> > > +	 *    preempting one. Since the device is suspended, no
> > > IO
> > > can
> > > +	 *    go to these unregistered paths and fail).
> > > +	 * 3. Releasing the reservation on the path that now
> > > holds
> > > it.
> > > +	 * 4. Resuming the device (since it no longer matters
> > > that
> > > most of
> > > +	 *    that paths no longer have a registered key)
> > > +	 * 5. Reregistering keys on all the paths
> > > +	 */
> > > +
> > > +	if (!dm_simplecmd_noflush(DM_DEVICE_SUSPEND, mpp->alias,
> > > 0))
> > > {
> > > +		condlog(0, "%s: release: failed to suspend dm
> > > device.",
> > 
> > Why do you use dm_simplecmd_noflush() here? Shouldn't queued IO be
> > flushed from the dm device to avoid it being sent to paths that are
> > going to be unregistered?
> > 
> 
> I'm pretty certain that DM will still flush all the IO from the
> target
> to DM core before suspending, even with dm_simplecmd_noflush() set.
> In
> request based multipath, queued IOs are never stored in the target.
> In
> bio based multipath, they are, but they will get flushed back up to
> DM
> core when suspending and queued there. No IO should happen through
> the
> target after the suspend, until the resume. dm_simplecmd_noflush()
> just
> keeps multipath from failing any IO that it had queueing, and it's
> only
> really necessary when we resize the device, because if we shrink the
> device, outstanding IO might be outside the new bounds.

OK, thanks for the clarification. I guess I've never fully understood
the way queueing works in dm.

What about queueing in the path devices? We'll be removing registration
keys, so IO sent by the SCSI layer may end up with RESERVATION CONFLICT
errors. To my understanding, without the DM_NOFLUSH_FLAG the kernel
will freeze the queue and flush everything, as if the device was closed
during shutdown. If DM_NOFLUSH_FLAG is set, this won't happen. What's
preventing the SCSI layer from sending IO while we're modifying the
registrations?

Martin

Re: [PATCH 09/15] limpathpersist: redesign failed release workaround

[Martin Wilck]

On Tue, 2025-08-26 at 10:44 +0200, Martin Wilck wrote:
> On Mon, 2025-08-25 at 20:51 -0400, Benjamin Marzinski wrote:
> > On Sun, Aug 24, 2025 at 05:26:50PM +0200, Martin Wilck wrote:
> > 
> > > 
> > > > +	/*
> > > > +	 * Cannot free the reservation because the path that
> > > > is
> > > > holding it
> > > > +	 * is not usable. Workaround this by:
> > > > +	 * 1. Suspending the device
> > > > +	 * 2. Preempting the reservation to move it to a
> > > > usable
> > > > path
> > > > +	 *    (this removes the registered keys on all paths
> > > > except
> > > > the
> > > > +	 *    preempting one. Since the device is suspended,
> > > > no
> > > > IO
> > > > can
> > > > +	 *    go to these unregistered paths and fail).
> > > > +	 * 3. Releasing the reservation on the path that now
> > > > holds
> > > > it.
> > > > +	 * 4. Resuming the device (since it no longer matters
> > > > that
> > > > most of
> > > > +	 *    that paths no longer have a registered key)
> > > > +	 * 5. Reregistering keys on all the paths
> > > > +	 */
> > > > +
> > > > +	if (!dm_simplecmd_noflush(DM_DEVICE_SUSPEND, mpp-
> > > > >alias,
> > > > 0))
> > > > {
> > > > +		condlog(0, "%s: release: failed to suspend dm
> > > > device.",
> > > 
> > > Why do you use dm_simplecmd_noflush() here? Shouldn't queued IO
> > > be
> > > flushed from the dm device to avoid it being sent to paths that
> > > are
> > > going to be unregistered?
> > > 
> > 
> > I'm pretty certain that DM will still flush all the IO from the
> > target
> > to DM core before suspending, even with dm_simplecmd_noflush() set.
> > In
> > request based multipath, queued IOs are never stored in the target.
> > In
> > bio based multipath, they are, but they will get flushed back up to
> > DM
> > core when suspending and queued there. No IO should happen through
> > the
> > target after the suspend, until the resume. dm_simplecmd_noflush()
> > just
> > keeps multipath from failing any IO that it had queueing, and it's
> > only
> > really necessary when we resize the device, because if we shrink
> > the
> > device, outstanding IO might be outside the new bounds.
> 
> OK, thanks for the clarification. I guess I've never fully understood
> the way queueing works in dm.
> 
> What about queueing in the path devices? We'll be removing
> registration
> keys, so IO sent by the SCSI layer may end up with RESERVATION
> CONFLICT
> errors. To my understanding, without the DM_NOFLUSH_FLAG the kernel
> will freeze the queue and flush everything, as if the device was
> closed
> during shutdown. If DM_NOFLUSH_FLAG is set, this won't happen. What's
> preventing the SCSI layer from sending IO while we're modifying the
> registrations?

I just re-read commit 2e93ccc1933d ("[PATCH] dm: suspend: add noflush
pushback"), which explains that the noflush flag is mainly meant for
handling queue_if_no_path situations, in particular being able to
reload a queueing multipath map with new paths when all paths have
failed. That's exactly what you wrote, queueing IO in generic dm rather
than in the target in order to facilitate map reloads.

Here we're looking at a very different situation. RESERVATION CONFLICT
errors won't cause queueing, because the kernel doesn't classify them
as path errors. If we need to preempt a reservation held by another
host, we'd rather not send IO to the device yet. If we have a
reservation we want to give up, we should make sure to have all
outstanding IO sent to the storage beforehand. In other situations like
just registering keys without reservations being present, flushing
won't be necessary, but it won't hurt either, AFAICS.

I am not sure whether it makes sense to try sending PRIN or PROUT
commands to maps in queueing state. Depending on the nature of the
errors that caused the paths to fail, we may or may not be able to send
this type of commands. PRIN/PROUT must still work for devices in
STANDBY state, but there are other situations where they wouldn't work,
or would even hang.

I tend to think that sending PRIN or PROUT commands to queueing devices
is an extreme corner case. Perhaps we should just refuse to do this 
(realizing that we can't avoid a device entering queueing mode while
we're processing PR commands, but that's an even more extreme corner
case).

If we can assume that the device is not queueing, it might actually be
better leave the NOFLUSH flag clear, making sure that the queue is 
frozen and all IO is actually flushed before changing PR keys or
reservations.

Am I getting something wrong here?

Martin

Re: [PATCH 09/15] limpathpersist: redesign failed release workaround

[Benjamin Marzinski]

On Tue, Aug 26, 2025 at 10:44:22AM +0200, Martin Wilck wrote:
> On Mon, 2025-08-25 at 20:51 -0400, Benjamin Marzinski wrote:
> > On Sun, Aug 24, 2025 at 05:26:50PM +0200, Martin Wilck wrote:
> > 
> > > 
> > > > +	/*
> > > > +	 * Cannot free the reservation because the path that is
> > > > holding it
> > > > +	 * is not usable. Workaround this by:
> > > > +	 * 1. Suspending the device
> > > > +	 * 2. Preempting the reservation to move it to a usable
> > > > path
> > > > +	 *    (this removes the registered keys on all paths
> > > > except
> > > > the
> > > > +	 *    preempting one. Since the device is suspended, no
> > > > IO
> > > > can
> > > > +	 *    go to these unregistered paths and fail).
> > > > +	 * 3. Releasing the reservation on the path that now
> > > > holds
> > > > it.
> > > > +	 * 4. Resuming the device (since it no longer matters
> > > > that
> > > > most of
> > > > +	 *    that paths no longer have a registered key)
> > > > +	 * 5. Reregistering keys on all the paths
> > > > +	 */
> > > > +
> > > > +	if (!dm_simplecmd_noflush(DM_DEVICE_SUSPEND, mpp->alias,
> > > > 0))
> > > > {
> > > > +		condlog(0, "%s: release: failed to suspend dm
> > > > device.",
> > > 
> > > Why do you use dm_simplecmd_noflush() here? Shouldn't queued IO be
> > > flushed from the dm device to avoid it being sent to paths that are
> > > going to be unregistered?
> > > 
> > 
> > I'm pretty certain that DM will still flush all the IO from the
> > target
> > to DM core before suspending, even with dm_simplecmd_noflush() set.
> > In
> > request based multipath, queued IOs are never stored in the target.
> > In
> > bio based multipath, they are, but they will get flushed back up to
> > DM
> > core when suspending and queued there. No IO should happen through
> > the
> > target after the suspend, until the resume. dm_simplecmd_noflush()
> > just
> > keeps multipath from failing any IO that it had queueing, and it's
> > only
> > really necessary when we resize the device, because if we shrink the
> > device, outstanding IO might be outside the new bounds.
> 
> OK, thanks for the clarification. I guess I've never fully understood
> the way queueing works in dm.
> 
> What about queueing in the path devices? We'll be removing registration
> keys, so IO sent by the SCSI layer may end up with RESERVATION CONFLICT
> errors. To my understanding, without the DM_NOFLUSH_FLAG the kernel
> will freeze the queue and flush everything, as if the device was closed
> during shutdown. If DM_NOFLUSH_FLAG is set, this won't happen. What's
> preventing the SCSI layer from sending IO while we're modifying the
> registrations?

In __dm_suspend() we block all new IOs to the dm device here:
https://github.com/torvalds/linux/blob/fab1beda7597fac1cecc01707d55eadb6bbe773c/drivers/md/dm.c#L2955-L2966

Once we know that no new IOs are getting sent to the target, we wait for
all the IOs that were send to the target to get completed by calling
dm_wait_for_completion() here:

https://github.com/torvalds/linux/blob/fab1beda7597fac1cecc01707d55eadb6bbe773c/drivers/md/dm.c#L2973

Any IOs that are currently being sent inside the multipath target will
get handled either while getting mapped or when ending the path IO by
multipath_clone_and_map(), __multipath_map_bio(), multipath_end_io(), or
multipath_end_io_bio(), which will complete the IOs or send them back to
DM core for queueing there (which also satisfies dm_wait_for_completion).

So by the time the suspend command returns, there won't be any IOs in
flight for the the SCSI layer to send to the target, and there can't be
new ones coming in through DM until we resume.

-Ben 
 
> Martin

Re: [PATCH 09/15] limpathpersist: redesign failed release workaround

[Martin Wilck]

On Tue, 2025-08-26 at 15:36 -0400, Benjamin Marzinski wrote:
> On Tue, Aug 26, 2025 at 10:44:22AM +0200, Martin Wilck wrote:
> > On Mon, 2025-08-25 at 20:51 -0400, Benjamin Marzinski wrote:
> > > On Sun, Aug 24, 2025 at 05:26:50PM +0200, Martin Wilck wrote:
> > > 
> > > > 
> > > > > +	/*
> > > > > +	 * Cannot free the reservation because the path that
> > > > > is
> > > > > holding it
> > > > > +	 * is not usable. Workaround this by:
> > > > > +	 * 1. Suspending the device
> > > > > +	 * 2. Preempting the reservation to move it to a
> > > > > usable
> > > > > path
> > > > > +	 *    (this removes the registered keys on all paths
> > > > > except
> > > > > the
> > > > > +	 *    preempting one. Since the device is suspended,
> > > > > no
> > > > > IO
> > > > > can
> > > > > +	 *    go to these unregistered paths and fail).
> > > > > +	 * 3. Releasing the reservation on the path that now
> > > > > holds
> > > > > it.
> > > > > +	 * 4. Resuming the device (since it no longer
> > > > > matters
> > > > > that
> > > > > most of
> > > > > +	 *    that paths no longer have a registered key)
> > > > > +	 * 5. Reregistering keys on all the paths
> > > > > +	 */
> > > > > +
> > > > > +	if (!dm_simplecmd_noflush(DM_DEVICE_SUSPEND, mpp-
> > > > > >alias,
> > > > > 0))
> > > > > {
> > > > > +		condlog(0, "%s: release: failed to suspend
> > > > > dm
> > > > > device.",
> > > > 
> > > > Why do you use dm_simplecmd_noflush() here? Shouldn't queued IO
> > > > be
> > > > flushed from the dm device to avoid it being sent to paths that
> > > > are
> > > > going to be unregistered?
> > > > 
> > > 
> > > I'm pretty certain that DM will still flush all the IO from the
> > > target
> > > to DM core before suspending, even with dm_simplecmd_noflush()
> > > set.
> > > In
> > > request based multipath, queued IOs are never stored in the
> > > target.
> > > In
> > > bio based multipath, they are, but they will get flushed back up
> > > to
> > > DM
> > > core when suspending and queued there. No IO should happen
> > > through
> > > the
> > > target after the suspend, until the resume.
> > > dm_simplecmd_noflush()
> > > just
> > > keeps multipath from failing any IO that it had queueing, and
> > > it's
> > > only
> > > really necessary when we resize the device, because if we shrink
> > > the
> > > device, outstanding IO might be outside the new bounds.
> > 
> > OK, thanks for the clarification. I guess I've never fully
> > understood
> > the way queueing works in dm.
> > 
> > What about queueing in the path devices? We'll be removing
> > registration
> > keys, so IO sent by the SCSI layer may end up with RESERVATION
> > CONFLICT
> > errors. To my understanding, without the DM_NOFLUSH_FLAG the kernel
> > will freeze the queue and flush everything, as if the device was
> > closed
> > during shutdown. If DM_NOFLUSH_FLAG is set, this won't happen.
> > What's
> > preventing the SCSI layer from sending IO while we're modifying the
> > registrations?
> 
> In __dm_suspend() we block all new IOs to the dm device here:
> https://github.com/torvalds/linux/blob/fab1beda7597fac1cecc01707d55eadb6bbe773c/drivers/md/dm.c#L2955-L2966
> 
> Once we know that no new IOs are getting sent to the target, we wait
> for
> all the IOs that were send to the target to get completed by calling
> dm_wait_for_completion() here:
> 
> https://github.com/torvalds/linux/blob/fab1beda7597fac1cecc01707d55eadb6bbe773c/drivers/md/dm.c#L2973

I saw that code. I realize now that dm_wait_for_completion() will
finish all IO on underlying path devices. I wasn't 100% certain about
that in the first place.

I also saw that lock_fs() is not called if we set the noflush flag. And
I am wondering if that's what we want. If there's dirty data on the
file system and we drop our registration key, the file system will most
probably error out.

Martin

Re: [PATCH 09/15] limpathpersist: redesign failed release workaround

[Benjamin Marzinski]

On Tue, Aug 26, 2025 at 12:06:50PM +0200, Martin Wilck wrote:
> On Tue, 2025-08-26 at 10:44 +0200, Martin Wilck wrote:
> > On Mon, 2025-08-25 at 20:51 -0400, Benjamin Marzinski wrote:
> > > On Sun, Aug 24, 2025 at 05:26:50PM +0200, Martin Wilck wrote:
> > > 
> > > > 
> > > > > +	/*
> > > > > +	 * Cannot free the reservation because the path that
> > > > > is
> > > > > holding it
> > > > > +	 * is not usable. Workaround this by:
> > > > > +	 * 1. Suspending the device
> > > > > +	 * 2. Preempting the reservation to move it to a
> > > > > usable
> > > > > path
> > > > > +	 *    (this removes the registered keys on all paths
> > > > > except
> > > > > the
> > > > > +	 *    preempting one. Since the device is suspended,
> > > > > no
> > > > > IO
> > > > > can
> > > > > +	 *    go to these unregistered paths and fail).
> > > > > +	 * 3. Releasing the reservation on the path that now
> > > > > holds
> > > > > it.
> > > > > +	 * 4. Resuming the device (since it no longer matters
> > > > > that
> > > > > most of
> > > > > +	 *    that paths no longer have a registered key)
> > > > > +	 * 5. Reregistering keys on all the paths
> > > > > +	 */
> > > > > +
> > > > > +	if (!dm_simplecmd_noflush(DM_DEVICE_SUSPEND, mpp-
> > > > > >alias,
> > > > > 0))
> > > > > {
> > > > > +		condlog(0, "%s: release: failed to suspend dm
> > > > > device.",
> > > > 
> > > > Why do you use dm_simplecmd_noflush() here? Shouldn't queued IO
> > > > be
> > > > flushed from the dm device to avoid it being sent to paths that
> > > > are
> > > > going to be unregistered?
> > > > 
> > > 
> > > I'm pretty certain that DM will still flush all the IO from the
> > > target
> > > to DM core before suspending, even with dm_simplecmd_noflush() set.
> > > In
> > > request based multipath, queued IOs are never stored in the target.
> > > In
> > > bio based multipath, they are, but they will get flushed back up to
> > > DM
> > > core when suspending and queued there. No IO should happen through
> > > the
> > > target after the suspend, until the resume. dm_simplecmd_noflush()
> > > just
> > > keeps multipath from failing any IO that it had queueing, and it's
> > > only
> > > really necessary when we resize the device, because if we shrink
> > > the
> > > device, outstanding IO might be outside the new bounds.
> > 
> > OK, thanks for the clarification. I guess I've never fully understood
> > the way queueing works in dm.
> > 
> > What about queueing in the path devices? We'll be removing
> > registration
> > keys, so IO sent by the SCSI layer may end up with RESERVATION
> > CONFLICT
> > errors. To my understanding, without the DM_NOFLUSH_FLAG the kernel
> > will freeze the queue and flush everything, as if the device was
> > closed
> > during shutdown. If DM_NOFLUSH_FLAG is set, this won't happen. What's
> > preventing the SCSI layer from sending IO while we're modifying the
> > registrations?
> 
> I just re-read commit 2e93ccc1933d ("[PATCH] dm: suspend: add noflush
> pushback"), which explains that the noflush flag is mainly meant for
> handling queue_if_no_path situations, in particular being able to
> reload a queueing multipath map with new paths when all paths have
> failed. That's exactly what you wrote, queueing IO in generic dm rather
> than in the target in order to facilitate map reloads.
> 
> Here we're looking at a very different situation. RESERVATION CONFLICT
> errors won't cause queueing, because the kernel doesn't classify them
> as path errors.

True. But to be pendantic, since we're sending the PR commands directly
to the scsi devices and not through DM, multipath's queueing can't
effect them or be effected by them. DM won't ever see those ioctls or
any errors that occur from them.

> If we need to preempt a reservation held by another
> host, we'd rather not send IO to the device yet. If we have a
> reservation we want to give up, we should make sure to have all
> outstanding IO sent to the storage beforehand. In other situations like
> just registering keys without reservations being present, flushing
> won't be necessary, but it won't hurt either, AFAICS.

The only situations where we will do the suspend is when a multipath
device preempts the reservation key that it is holding, either because
it was explicitly told to (which is something that the SCSI spec
specifically calls out as valid, and is in-fact the only way to change
the reservation type of an existing reservation) or because we failed to
release a reservation, since the path holding the reservation is down.

If we do a flushing suspend, all queued IO will be failed, regardless of
the user's no_path_retry setting. We really don't want to do that unless
we have to (like when we're shrinking the device).

If we still have usable paths to the storage, that IO will get completed
before we suspend. The noflush code only matters if we don't have usable
paths. I would argue that even if we are releasing a reservation, we
want to continue queueing the IO if we end up in a queueing situation.
After we release a reservation, nobody is holding one, and all IO is
allowed. Plus, our paths still have keys registered (aside from that gap
when they are suspended) if another device grabs the reservation. So
there's no point in giving up on the queued IO when we release a
reservation, since we have every reason to believe it will be able to go
through when a path gets restored. 

> I am not sure whether it makes sense to try sending PRIN or PROUT
> commands to maps in queueing state. Depending on the nature of the
> errors that caused the paths to fail, we may or may not be able to send
> this type of commands. PRIN/PROUT must still work for devices in
> STANDBY state, but there are other situations where they wouldn't work,
> or would even hang.
> 
> I tend to think that sending PRIN or PROUT commands to queueing devices
> is an extreme corner case. Perhaps we should just refuse to do this 
> (realizing that we can't avoid a device entering queueing mode while
> we're processing PR commands, but that's an even more extreme corner
> case).

libmpathpersist runs the path checkers before it sends any commands and
doesn't send PRIN or PROUT commands to paths that are down. If there are
no usable paths, libmpathpersist will just return failure. I have no
plans to change any of this. Sure, it's unlikely that we will enter the
queueing state later, while processing the libmpathpersist command. But
the code doesn't get any simpler by doing a flushing suspend instead of
a noflush suspend, and we make sure that we won't fail IOs when
multipath is configured not to, even in this corner case. There are
plenty of places in the multipath code where we suspend (or reload a
table, where we implicitly suspend) knowing there are usable devices,
and we do noflush suspends anyways. I don't see why this is any
different.

And the window where we lose our last path while mpathpersist is running
is small but not super-small, especially in the case where we were told
to PREEMPT our key. libmpathpersist runs the path checkers pretty early.
There is still a bunch of setup and checking stuff it needs to do before
it actually gets around to suspending in preparation for a device
preempting its own key. So if the last path failed between when the path
checkers were run and when we suspend, we could have queued IO. Users
running a PREEMPT or a RELEASE command shouldn't expect it to cause
queued IO to fail if all the paths go down, and I don't see any big
benefit to offset the downside of allowing that possibility.

> 
> If we can assume that the device is not queueing, it might actually be
> better leave the NOFLUSH flag clear, making sure that the queue is 
> frozen and all IO is actually flushed before changing PR keys or
> reservations.

So, I'm assuming that you are talking about the lock_fs() call in
__dm_suspend() that gets skipped when we do a noflush suspend. This is
the only difference I can see that isn't just us opening ourselves up to
unwanted IO errors. And this admittedly will cause the fs to flush IO to
the device, which would be a good thing if we were about to lose our
ability to write to the disk. I have two arguments against caring about
this.

1. sg_persist doesn't force the filesystem to quiesce, so why should
mpathpersist? Nothing that is doing persistent reservations should rely
on that happening when they change a reservation.

2. We won't be losing our ability to write to the disk. Like I said,
there are only two cases where we try to preempt ourselves and trigger
this suspend: either we were explicitly told to preempt ourselves, or we
failed doing a release because the path holding the reservation was
down. In both cases, we keep our registered keys.

The only Persistent reservation types where a path with a registered key
can't do any IO it wants are types 1 (write exclusive) and 3 (exclusive
access), and we should probably disallow those types in libmpathpersist,
because they will never work. With those types of reservations, only the
I_T Nexus holding the reservation is allowed to do IO. This means that
only one path of a multipath device will ever be able to do IO. The
others will fail with RESEVATION CONFLICT errors. 

-Ben

> Am I getting something wrong here?
> 
> Martin

Re: [PATCH 09/15] limpathpersist: redesign failed release workaround

[Martin Wilck]

On Tue, 2025-08-26 at 17:07 -0400, Benjamin Marzinski wrote:
> On Tue, Aug 26, 2025 at 12:06:50PM +0200, Martin Wilck wrote:
> 
> 
> The only situations where we will do the suspend is when a multipath
> device preempts the reservation key that it is holding, either
> because
> it was explicitly told to (which is something that the SCSI spec
> specifically calls out as valid, and is in-fact the only way to
> change
> the reservation type of an existing reservation) or because we failed
> to
> release a reservation, since the path holding the reservation is
> down.

Right. This is what I forgot while reasoning about the noflush
semantics. Which, thanks to this discussion, I think I finally
understand correctly.

> If we do a flushing suspend, all queued IO will be failed, regardless
> of
> the user's no_path_retry setting. We really don't want to do that

I agree.

> > I tend to think that sending PRIN or PROUT commands to queueing
> > devices
> > is an extreme corner case. Perhaps we should just refuse to do this
> > (realizing that we can't avoid a device entering queueing mode
> > while
> > we're processing PR commands, but that's an even more extreme
> > corner
> > case).
> 
> libmpathpersist runs the path checkers before it sends any commands
> and
> doesn't send PRIN or PROUT commands to paths that are down. If there
> are
> no usable paths, libmpathpersist will just return failure. I have no
> plans to change any of this.

Side note: There's a realistic chance that persistent reservation
commands may succeed on paths that appear down for regular IO, e.g. in
ALUA STANDBY state. But I doubt that it makes sense to try that.

> 1. sg_persist doesn't force the filesystem to quiesce, so why should
> mpathpersist? Nothing that is doing persistent reservations should
> rely
> on that happening when they change a reservation.
> 
> 2. We won't be losing our ability to write to the disk. Like I said,
> there are only two cases where we try to preempt ourselves and
> trigger
> this suspend: either we were explicitly told to preempt ourselves, or
> we
> failed doing a release because the path holding the reservation was
> down. In both cases, we keep our registered keys.

You're right. Sorry for the confusion, and thanks for taking the time
to explain.

Martin

Re: [PATCH 10/15] libmpathpersist: fail the release if all threads fail

[Benjamin Marzinski]

On Sun, Aug 24, 2025 at 05:33:07PM +0200, Martin Wilck wrote:
> On Thu, 2025-07-10 at 14:10 -0400, Benjamin Marzinski wrote:
> > If none of the threads succeeds in issuing the release, simply return
> > failure, instead of trying the workaround.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmpathpersist/mpath_persist_int.c | 12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/libmpathpersist/mpath_persist_int.c
> > b/libmpathpersist/mpath_persist_int.c
> > index 7fb08b2e..ad5a4ee7 100644
> > --- a/libmpathpersist/mpath_persist_int.c
> > +++ b/libmpathpersist/mpath_persist_int.c
> > @@ -475,15 +475,21 @@ static int mpath_prout_rel(struct multipath
> > *mpp,int rq_servact, int rq_scope,
> >  		}
> >  	}
> >  
> > +	rc = MPATH_PR_DMMP_ERROR;
> 
> I find the relationship between "status" and "rc" a bit confusing in
> this patch. Can we use something like "bool all_failed" instead?

Sure.

-Ben

> 
> Martin
> 
> 
> >  	for (i = 0; i < count; i++){
> >  		/*  check thread status here and return the status
> > */
> >  
> > -		if (thread[i].param.status ==
> > MPATH_PR_RESERV_CONFLICT)
> > +		if (thread[i].param.status == MPATH_PR_SUCCESS)
> > +			rc = MPATH_PR_SUCCESS;
> > +		else if (thread[i].param.status ==
> > MPATH_PR_RESERV_CONFLICT)
> >  			status = MPATH_PR_RESERV_CONFLICT;
> > -		else if (status == MPATH_PR_SUCCESS
> > -				&& thread[i].param.status !=
> > MPATH_PR_RESERV_CONFLICT)
> > +		else if (status == MPATH_PR_SUCCESS)
> >  			status = thread[i].param.status;
> >  	}
> > +	if (rc != MPATH_PR_SUCCESS) {
> > +		condlog(0, "%s: all threads failed to release
> > reservation.", mpp->wwid);
> > +		return status;
> > +	}
> >  
> >  	status = mpath_prin_activepath (mpp, MPATH_PRIN_RRES_SA,
> > &resp, noisy);
> >  	if (status != MPATH_PR_SUCCESS){