[PATCH 00/14] Additional fixes and cleanups

[PATCH 00/14] Additional fixes and cleanups

[Benjamin Marzinski]

This patchset builds on top of my previous libmpathpersist patchset,
("Improve mpathpersist's unavailable path handling") and add various
fixes and cleanups to multipath's persistent reservation handling.

The issues handled by this patchset are:
- Handling removal of keys from paths that were down when the multipath
  device was unregistered.
- Changing how conflicts on key registration are handled. Instead of
  the current rollback method (which was broken anyways),
  libmpathpersist now retrys with REGISTER AND IGNORE, as long as the
  REGISTER command completed successfully down some of the paths.
- Changing when the reservation key is set to fix corner cases on
  failure and registration while paths are coming up.
- Retrying on conflicts in mpath_prout_common to fix corner cases when
  a path is coming up while a doing a reserve, preempt or clear.
- Allowing registrations to succeed when there are retryable errors, 
  if the paths are actually down.
- Fixing the reservation key validation code

I realize that a number of these fixes are dealing with races between
libmpathperist and multipathd, which points to shortcomings with the
current design of libmpathpersist. The code could likely be simpler and 
more robust if limpathpersist was a thin wrapper around commands to
multipathd, which handled issuing the actual persistent reservation
commands. However, we have customers that need persistent reservations
working well on currently released versions of the mutipath-tools, and
redesigning how the libmpathpersist library works is a much larger task,
and even harder to backport. I'd like to know if people think that would
be a worthwhile task for later, however.

Benjamin Marzinski (14):
  limpathpersist: remove update_map_pr code for NULL pp
  libmpathpersist: move update_map_pr to multipathd
  multipathd: clean up update_map_pr and mpath_pr_event_handle
  libmpathpersist: clean up duplicate function declarations
  multipathd: wrap setting and unsetting prflag
  multipathd: unregister PR key when path is restored if necessary
  libmpathpersist: Fix-up reservation_key checking
  libmpathpersist: change how reservation conflicts are handled
  libmpathpersist: Clear prkey in multipathd before unregistering
  libmpathpersist: only clear the key if we are using the prkeys file
  libmpathpersist: Restore old reservation key on failure
  libmpatpersist: update reservation key before checking paths
  libmpathpersist: retry on conflicts in mpath_prout_common
  libmpathpersist: Don't always fail registrations for retryable errors

 libmpathpersist/libmpathpersist.version |   1 -
 libmpathpersist/mpath_persist_int.c     | 363 +++++++++++++-----------
 libmpathpersist/mpath_persist_int.h     |   2 -
 libmpathpersist/mpath_pr_ioctl.c        |  12 +-
 libmultipath/structs.h                  |   2 +
 mpathpersist/main.c                     |   2 -
 multipathd/cli_handlers.c               |  11 +-
 multipathd/main.c                       | 132 +++++++--
 multipathd/main.h                       |   2 +
 9 files changed, 311 insertions(+), 216 deletions(-)

-- 
2.48.1

[PATCH 01/14] limpathpersist: remove update_map_pr code for NULL pp

[Benjamin Marzinski]

Since update_map_pr is always called with pp set now, remove the code
to handle being called with NULL pp.

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

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 91f0d018..afd19bef 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -833,18 +833,8 @@ int update_map_pr(struct multipath *mpp, struct path *pp)
 		condlog(0,"%s : failed to alloc resp in update_map_pr", mpp->alias);
 		return MPATH_PR_OTHER;
 	}
-	if (!pp && count_active_paths(mpp) == 0) {
-		condlog(2, "%s: No available paths to check pr status",
-			mpp->alias);
-		goto out;
-	}
-	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);
 
+	ret = prin_do_scsi_ioctl(pp->dev, MPATH_PRIN_RKEY_SA, resp, noisy);
 	if (ret != MPATH_PR_SUCCESS )
 	{
 		if (ret == MPATH_PR_ILLEGAL_REQ)
-- 
2.48.1

[PATCH 02/14] libmpathpersist: move update_map_pr to multipathd

[Benjamin Marzinski]

multipathd is now the only program that calls update_map_pr(), so move
it there, and make it static. There are no other code changes.

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

diff --git a/libmpathpersist/libmpathpersist.version b/libmpathpersist/libmpathpersist.version
index b31237db..7f47ea99 100644
--- a/libmpathpersist/libmpathpersist.version
+++ b/libmpathpersist/libmpathpersist.version
@@ -40,5 +40,4 @@ __LIBMPATHPERSIST_INT_2.0.0 {
 	mpath_alloc_prin_response;
 	prin_do_scsi_ioctl;
 	prout_do_scsi_ioctl;
-	update_map_pr;
 };
diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index afd19bef..c6b2e788 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -806,78 +806,3 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	}
 	return ret;
 }
-
-int update_map_pr(struct multipath *mpp, struct path *pp)
-{
-	int noisy=0;
-	struct prin_resp *resp;
-	unsigned int i;
-	int ret = MPATH_PR_OTHER, isFound;
-	bool was_set = (mpp->prflag == PR_SET);
-
-	/* If pr is explicitly unset, it must be manually set */
-	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 = PR_UNSET;
-		condlog(was_set ? 2 : 4, "%s: reservation_key not set in multipath.conf", mpp->alias);
-		return MPATH_PR_SKIP;
-	}
-
-	resp = mpath_alloc_prin_response(MPATH_PRIN_RKEY_SA);
-	if (!resp)
-	{
-		condlog(0,"%s : failed to alloc resp in update_map_pr", mpp->alias);
-		return MPATH_PR_OTHER;
-	}
-
-	ret = prin_do_scsi_ioctl(pp->dev, MPATH_PRIN_RKEY_SA, resp, noisy);
-	if (ret != MPATH_PR_SUCCESS )
-	{
-		if (ret == MPATH_PR_ILLEGAL_REQ)
-			mpp->prflag = PR_UNSET;
-		condlog(0,"%s : pr in read keys service action failed Error=%d", mpp->alias, ret);
-		goto out;
-	}
-	mpp->prflag = PR_UNSET;
-
-	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);
-		goto out;
-	}
-
-	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++ )
-	{
-		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(3, "%s: reservation key found in pr in readkeys response", mpp->alias);
-			isFound =1;
-		}
-	}
-
-	if (isFound)
-	{
-		mpp->prflag = PR_SET;
-		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);
-	return ret;
-}
diff --git a/libmpathpersist/mpath_persist_int.h b/libmpathpersist/mpath_persist_int.h
index 7b32c1e2..ed7b46f7 100644
--- a/libmpathpersist/mpath_persist_int.h
+++ b/libmpathpersist/mpath_persist_int.h
@@ -20,6 +20,5 @@ 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, struct path *pp);
 
 #endif /* MPATH_PERSIST_INT_H_INCLUDED */
diff --git a/multipathd/main.c b/multipathd/main.c
index d22425f6..fd0c0dba 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -4248,6 +4248,81 @@ static void check_prhold(struct multipath *mpp, struct path *pp)
 		mpp->prhold = PR_SET;
 }
 
+static int update_map_pr(struct multipath *mpp, struct path *pp)
+{
+	int noisy=0;
+	struct prin_resp *resp;
+	unsigned int i;
+	int ret = MPATH_PR_OTHER, isFound;
+	bool was_set = (mpp->prflag == PR_SET);
+
+	/* If pr is explicitly unset, it must be manually set */
+	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 = PR_UNSET;
+		condlog(was_set ? 2 : 4, "%s: reservation_key not set in multipath.conf", mpp->alias);
+		return MPATH_PR_SKIP;
+	}
+
+	resp = mpath_alloc_prin_response(MPATH_PRIN_RKEY_SA);
+	if (!resp)
+	{
+		condlog(0,"%s : failed to alloc resp in update_map_pr", mpp->alias);
+		return MPATH_PR_OTHER;
+	}
+
+	ret = prin_do_scsi_ioctl(pp->dev, MPATH_PRIN_RKEY_SA, resp, noisy);
+	if (ret != MPATH_PR_SUCCESS )
+	{
+		if (ret == MPATH_PR_ILLEGAL_REQ)
+			mpp->prflag = PR_UNSET;
+		condlog(0,"%s : pr in read keys service action failed Error=%d", mpp->alias, ret);
+		goto out;
+	}
+	mpp->prflag = PR_UNSET;
+
+	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);
+		goto out;
+	}
+
+	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++ )
+	{
+		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(3, "%s: reservation key found in pr in readkeys response", mpp->alias);
+			isFound =1;
+		}
+	}
+
+	if (isFound)
+	{
+		mpp->prflag = PR_SET;
+		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);
+	return ret;
+}
+
 static void mpath_pr_event_handle(struct path *pp)
 {
 	struct multipath *mpp = pp->mpp;
-- 
2.48.1

[PATCH 03/14] multipathd: clean up update_map_pr and mpath_pr_event_handle

[Benjamin Marzinski]

Store the READ KEYS response and the prout_param_descriptor on the stack
to avoid having to fail these functions for allocation reasons. Don't
explicitly check for additional_length == 0, since the for-loop already
handles that.  Also cleanup formatting issues,remove redundant messages,
and reduce the log level of others.

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

diff --git a/multipathd/main.c b/multipathd/main.c
index fd0c0dba..0e95ca3a 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -4251,7 +4251,7 @@ static void check_prhold(struct multipath *mpp, struct path *pp)
 static int update_map_pr(struct multipath *mpp, struct path *pp)
 {
 	int noisy=0;
-	struct prin_resp *resp;
+	struct prin_resp resp;
 	unsigned int i;
 	int ret = MPATH_PR_OTHER, isFound;
 	bool was_set = (mpp->prflag == PR_SET);
@@ -4260,57 +4260,42 @@ static int update_map_pr(struct multipath *mpp, struct path *pp)
 	if (mpp->prflag == PR_UNSET)
 		return MPATH_PR_SKIP;
 
-	if (!get_be64(mpp->reservation_key))
-	{
+	if (!get_be64(mpp->reservation_key)) {
 		/* Nothing to do. Assuming pr mgmt feature is disabled*/
 		mpp->prflag = PR_UNSET;
 		condlog(was_set ? 2 : 4, "%s: reservation_key not set in multipath.conf", mpp->alias);
 		return MPATH_PR_SKIP;
 	}
 
-	resp = mpath_alloc_prin_response(MPATH_PRIN_RKEY_SA);
-	if (!resp)
-	{
-		condlog(0,"%s : failed to alloc resp in update_map_pr", mpp->alias);
-		return MPATH_PR_OTHER;
-	}
+	memset(&resp, 0, sizeof(resp));
 
-	ret = prin_do_scsi_ioctl(pp->dev, MPATH_PRIN_RKEY_SA, resp, noisy);
-	if (ret != MPATH_PR_SUCCESS )
-	{
+	ret = prin_do_scsi_ioctl(pp->dev, MPATH_PRIN_RKEY_SA, &resp, noisy);
+	if (ret != MPATH_PR_SUCCESS) {
 		if (ret == MPATH_PR_ILLEGAL_REQ)
 			mpp->prflag = PR_UNSET;
 		condlog(0,"%s : pr in read keys service action failed Error=%d", mpp->alias, ret);
-		goto out;
+		return ret;
 	}
 	mpp->prflag = PR_UNSET;
 
-	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);
-		goto out;
-	}
-
-	condlog(3, "%s: Multipath reservation_key: 0x%" PRIx64 " ", mpp->alias,
+	condlog(4, "%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++ )
-	{
-		if (libmp_verbosity >= 3) {
-			condlog(3, "%s: PR IN READKEYS[%d] reservation key:",
+	isFound = 0;
+	for (i = 0; i < resp.prin_descriptor.prin_readkeys.additional_length / 8; i++) {
+		uint8_t *keyp = &resp.prin_descriptor.prin_readkeys.key_list[i * 8];
+
+		if (libmp_verbosity >= 4) {
+			condlog(4, "%s: PR IN READKEYS[%d] reservation key:",
 				mpp->alias, i);
-			dumpHex((char *)&resp->prin_descriptor.prin_readkeys.key_list[i * 8], 8, 1);
+			dumpHex((char *)keyp, 8, 1);
 		}
 
-		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;
-		}
+		if (!memcmp(&mpp->reservation_key, keyp, 8))
+			isFound = 1;
 	}
 
-	if (isFound)
-	{
+	if (isFound) {
 		mpp->prflag = PR_SET;
 		condlog(was_set ? 3 : 2, "%s: key found. prflag set.",
 			mpp->alias);
@@ -4318,16 +4303,14 @@ static int update_map_pr(struct multipath *mpp, struct path *pp)
 		condlog(was_set ? 1 : 3, "%s: key not found. prflag unset.",
 			mpp->alias);
 
-out:
-	free(resp);
-	return ret;
+	return MPATH_PR_SUCCESS;
 }
 
 static void mpath_pr_event_handle(struct path *pp)
 {
 	struct multipath *mpp = pp->mpp;
 	int ret;
-	struct prout_param_descriptor *param;
+	struct prout_param_descriptor param;
 
 	if (pp->bus != SYSFS_BUS_SCSI) {
 		mpp->prflag = PR_UNSET;
@@ -4345,21 +4328,17 @@ static void mpath_pr_event_handle(struct path *pp)
 	if (mpp->prflag != PR_SET)
 		return;
 
-	param = (struct prout_param_descriptor *)calloc(1, sizeof(struct prout_param_descriptor));
-	if (!param)
-		return;
+	memset(&param, 0, sizeof(param));
 
-	param->sa_flags = mpp->sa_flags;
-	memcpy(param->sa_key, &mpp->reservation_key, 8);
-	param->num_transportid = 0;
+	param.sa_flags = mpp->sa_flags;
+	memcpy(param.sa_key, &mpp->reservation_key, 8);
+	param.num_transportid = 0;
 
 	condlog(3, "device %s:%s", pp->dev, pp->mpp->wwid);
 
-	ret = prout_do_scsi_ioctl(pp->dev, MPATH_PROUT_REG_IGN_SA, 0, 0, param, 0);
+	ret = prout_do_scsi_ioctl(pp->dev, MPATH_PROUT_REG_IGN_SA, 0, 0, &param, 0);
 	if (ret != MPATH_PR_SUCCESS )
 	{
 		condlog(0,"%s: Reservation registration failed. Error: %d", pp->dev, ret);
 	}
-
-	free(param);
 }
-- 
2.48.1

[PATCH 04/14] libmpathpersist: clean up duplicate function declarations

[Benjamin Marzinski]

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.h |  1 -
 libmpathpersist/mpath_pr_ioctl.c    | 10 +++-------
 mpathpersist/main.c                 |  2 --
 3 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.h b/libmpathpersist/mpath_persist_int.h
index ed7b46f7..c09b2ba6 100644
--- a/libmpathpersist/mpath_persist_int.h
+++ b/libmpathpersist/mpath_persist_int.h
@@ -6,7 +6,6 @@
  * but aren't part of the public libmpathpersist API.
  */
 
-void * mpath_alloc_prin_response(int prin_sa);
 int do_mpath_persistent_reserve_in(vector curmp, vector pathvec,
 				   int fd, int rq_servact,
 				   struct prin_resp *resp, int noisy);
diff --git a/libmpathpersist/mpath_pr_ioctl.c b/libmpathpersist/mpath_pr_ioctl.c
index 7e1d2896..dfdbbb65 100644
--- a/libmpathpersist/mpath_pr_ioctl.c
+++ b/libmpathpersist/mpath_pr_ioctl.c
@@ -14,19 +14,15 @@
 #include "mpath_pr_ioctl.h"
 #include "mpath_persist.h"
 #include "unaligned.h"
-
 #include "debug.h"
 #include "structs.h" /* FILE_NAME_SIZE */
+#include "mpath_persist_int.h"
 
 #define TIMEOUT 2000
 #define MAXRETRY 5
 
-int prin_do_scsi_ioctl(char * dev, int rq_servact, struct prin_resp *resp, int noisy);
-int mpath_translate_response (char * dev, struct sg_io_hdr io_hdr,
-			      SenseData_t *Sensedata);
-void dumpHex(const char* str, int len, int no_ascii);
-int prout_do_scsi_ioctl( char * dev, int rq_servact, int rq_scope,
-		unsigned int rq_type, struct prout_param_descriptor *paramp, int noisy);
+int mpath_translate_response(char *dev, struct sg_io_hdr io_hdr,
+			     SenseData_t *Sensedata);
 uint32_t  format_transportids(struct prout_param_descriptor *paramp);
 void convert_be32_to_cpu(uint32_t *num);
 void convert_be16_to_cpu(uint16_t *num);
diff --git a/mpathpersist/main.c b/mpathpersist/main.c
index efb46b95..97fd5a43 100644
--- a/mpathpersist/main.c
+++ b/mpathpersist/main.c
@@ -38,8 +38,6 @@ void mpath_print_buf_readcap(struct prin_resp *pr_buff);
 void mpath_print_buf_readfullstat(struct prin_resp *pr_buff);
 void mpath_print_buf_readresv(struct prin_resp *pr_buff);
 void mpath_print_buf_readkeys(struct prin_resp *pr_buff);
-void dumpHex(const char* str, int len, int no_ascii);
-void * mpath_alloc_prin_response(int prin_sa);
 void mpath_print_transport_id(struct prin_fulldescr *fdesc);
 int construct_transportid(const char * inp, struct transportid transid[], int num_transportids);
 
-- 
2.48.1

[PATCH 05/14] multipathd: wrap setting and unsetting prflag

[Benjamin Marzinski]

When prflag is unset, prhold and sa_flags should also be unset. A future
patch will add another variable to be set when prflag is set. Wrap all
these actions in set_pr() and unset_pr().

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 multipathd/cli_handlers.c | 11 +++++------
 multipathd/main.c         | 34 ++++++++++++++++++++--------------
 multipathd/main.h         |  2 ++
 3 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/multipathd/cli_handlers.c b/multipathd/cli_handlers.c
index 34910c9b..c1051c85 100644
--- a/multipathd/cli_handlers.c
+++ b/multipathd/cli_handlers.c
@@ -1290,7 +1290,7 @@ cli_setprstatus(void * v, struct strbuf *reply, void * data)
 		return -ENODEV;
 
 	if (mpp->prflag != PR_SET) {
-		mpp->prflag = PR_SET;
+		set_pr(mpp);
 		condlog(2, "%s: prflag set", param);
 	}
 
@@ -1311,12 +1311,11 @@ cli_unsetprstatus(void * v, struct strbuf *reply, void * data)
 		return -ENODEV;
 
 	if (mpp->prflag != PR_UNSET) {
-		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);
+		if (mpp->prhold != PR_UNSET)
+			condlog(2, "%s: prhold unset (by clearing prflag)",
+				param);
+		unset_pr(mpp);
 	}
 
 	return 0;
diff --git a/multipathd/main.c b/multipathd/main.c
index 0e95ca3a..c1ed1488 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -4227,10 +4227,6 @@ 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;
 
@@ -4248,6 +4244,18 @@ static void check_prhold(struct multipath *mpp, struct path *pp)
 		mpp->prhold = PR_SET;
 }
 
+void set_pr(struct multipath *mpp)
+{
+	mpp->prflag = PR_SET;
+}
+
+void unset_pr(struct multipath *mpp)
+{
+	mpp->prflag = PR_UNSET;
+	mpp->prhold = PR_UNSET;
+	mpp->sa_flags = 0;
+}
+
 static int update_map_pr(struct multipath *mpp, struct path *pp)
 {
 	int noisy=0;
@@ -4262,7 +4270,7 @@ static int update_map_pr(struct multipath *mpp, struct path *pp)
 
 	if (!get_be64(mpp->reservation_key)) {
 		/* Nothing to do. Assuming pr mgmt feature is disabled*/
-		mpp->prflag = PR_UNSET;
+		unset_pr(mpp);
 		condlog(was_set ? 2 : 4, "%s: reservation_key not set in multipath.conf", mpp->alias);
 		return MPATH_PR_SKIP;
 	}
@@ -4272,11 +4280,10 @@ static int update_map_pr(struct multipath *mpp, struct path *pp)
 	ret = prin_do_scsi_ioctl(pp->dev, MPATH_PRIN_RKEY_SA, &resp, noisy);
 	if (ret != MPATH_PR_SUCCESS) {
 		if (ret == MPATH_PR_ILLEGAL_REQ)
-			mpp->prflag = PR_UNSET;
+			unset_pr(mpp);
 		condlog(0,"%s : pr in read keys service action failed Error=%d", mpp->alias, ret);
 		return ret;
 	}
-	mpp->prflag = PR_UNSET;
 
 	condlog(4, "%s: Multipath reservation_key: 0x%" PRIx64 " ", mpp->alias,
 		get_be64(mpp->reservation_key));
@@ -4296,12 +4303,14 @@ static int update_map_pr(struct multipath *mpp, struct path *pp)
 	}
 
 	if (isFound) {
-		mpp->prflag = PR_SET;
+		set_pr(mpp);
 		condlog(was_set ? 3 : 2, "%s: key found. prflag set.",
 			mpp->alias);
-	} else
+	} else {
+		unset_pr(mpp);
 		condlog(was_set ? 1 : 3, "%s: key not found. prflag unset.",
 			mpp->alias);
+	}
 
 	return MPATH_PR_SUCCESS;
 }
@@ -4313,15 +4322,12 @@ static void mpath_pr_event_handle(struct path *pp)
 	struct prout_param_descriptor param;
 
 	if (pp->bus != SYSFS_BUS_SCSI) {
-		mpp->prflag = PR_UNSET;
+		unset_pr(mpp);
 		return;
 	}
 
-	if (update_map_pr(mpp, pp) != MPATH_PR_SUCCESS) {
-		if (mpp->prflag == PR_UNSET)
-			mpp->prhold = PR_UNSET;
+	if (update_map_pr(mpp, pp) != MPATH_PR_SUCCESS)
 		return;
-	}
 
 	check_prhold(mpp, pp);
 
diff --git a/multipathd/main.h b/multipathd/main.h
index c94362e4..29b57e3d 100644
--- a/multipathd/main.h
+++ b/multipathd/main.h
@@ -52,4 +52,6 @@ bool check_path_wwid_change(struct path *pp);
 int finish_path_init(struct path *pp, struct vectors * vecs);
 int resize_map(struct multipath *mpp, unsigned long long size,
 	       struct vectors *vecs);
+void set_pr(struct multipath *mpp);
+void unset_pr(struct multipath *mpp);
 #endif /* MAIN_H_INCLUDED */
-- 
2.48.1

[PATCH 06/14] multipathd: unregister PR key when path is restored if necessary

[Benjamin Marzinski]

It is possible that a path was unavailable and either the registered PR
key was removed or the registered PR key was changed and then that new
key was preempted. In both of these situations, this path will still
have a registered key (just not one that matches mpp->reservation_key)
but it should not have one. If the path becomes usable again in this
state, it may allow the multipath device to access storage that it
shouldn't be allowed to access.

To deal with this, track if a multipath device ever had a registered PR
key. If so, and the device no longer has a registered key, explicitly
clear the key when paths get restored.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmultipath/structs.h |  2 ++
 multipathd/main.c      | 46 ++++++++++++++++++++++++++++++++----------
 2 files changed, 37 insertions(+), 11 deletions(-)

diff --git a/libmultipath/structs.h b/libmultipath/structs.h
index 97093675..a206a7d1 100644
--- a/libmultipath/structs.h
+++ b/libmultipath/structs.h
@@ -524,6 +524,8 @@ struct multipath {
 	int prflag;
 	int prhold;
 	int all_tg_pt;
+	bool ever_registered_pr;
+
 	struct gen_multipath generic_mp;
 	bool fpin_must_reload;
 };
diff --git a/multipathd/main.c b/multipathd/main.c
index c1ed1488..cb00c1f8 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2638,7 +2638,8 @@ update_path_state (struct vectors * vecs, struct path * pp)
 
 		/* newstate == PATH_UP || newstate == PATH_GHOST */
 
-		if (pp->mpp->prflag != PR_UNSET) {
+		if (pp->mpp->prflag != PR_UNSET ||
+		    pp->mpp->ever_registered_pr) {
 			int prflag = pp->mpp->prflag;
 			/*
 			 * Check Persistent Reservation.
@@ -4246,6 +4247,7 @@ static void check_prhold(struct multipath *mpp, struct path *pp)
 
 void set_pr(struct multipath *mpp)
 {
+	mpp->ever_registered_pr = true;
 	mpp->prflag = PR_SET;
 }
 
@@ -4256,23 +4258,28 @@ void unset_pr(struct multipath *mpp)
 	mpp->sa_flags = 0;
 }
 
+/*
+ * Returns MPATH_PR_SUCCESS unless if fails to read the PR keys. If
+ * MPATH_PR_SUCCESS is returned, mpp->prflag will be either PR_SET or
+ * PR_UNSET.
+ */
 static int update_map_pr(struct multipath *mpp, struct path *pp)
 {
 	int noisy=0;
 	struct prin_resp resp;
 	unsigned int i;
-	int ret = MPATH_PR_OTHER, isFound;
+	int ret, isFound;
 	bool was_set = (mpp->prflag == PR_SET);
 
 	/* If pr is explicitly unset, it must be manually set */
 	if (mpp->prflag == PR_UNSET)
-		return MPATH_PR_SKIP;
+		return MPATH_PR_SUCCESS;
 
 	if (!get_be64(mpp->reservation_key)) {
 		/* Nothing to do. Assuming pr mgmt feature is disabled*/
 		unset_pr(mpp);
 		condlog(was_set ? 2 : 4, "%s: reservation_key not set in multipath.conf", mpp->alias);
-		return MPATH_PR_SKIP;
+		return MPATH_PR_SUCCESS;
 	}
 
 	memset(&resp, 0, sizeof(resp));
@@ -4320,6 +4327,7 @@ static void mpath_pr_event_handle(struct path *pp)
 	struct multipath *mpp = pp->mpp;
 	int ret;
 	struct prout_param_descriptor param;
+	bool clear_reg = false;
 
 	if (pp->bus != SYSFS_BUS_SCSI) {
 		unset_pr(mpp);
@@ -4331,20 +4339,36 @@ static void mpath_pr_event_handle(struct path *pp)
 
 	check_prhold(mpp, pp);
 
-	if (mpp->prflag != PR_SET)
-		return;
+	if (mpp->prflag != PR_SET) {
+		if (!mpp->ever_registered_pr)
+			return;
+		/*
+		 * This path may have been unusable and either the
+		 * registration was cleared or the registered
+		 * key was switched and then that new key was preempted.
+		 * In either case, this path should not have a registration
+		 * but it might still have one, just with a different
+		 * key than mpp->reservation_key is currently set to.
+		 * clear it to be sure.
+		 */
+		clear_reg = true;
+	}
 
 	memset(&param, 0, sizeof(param));
 
-	param.sa_flags = mpp->sa_flags;
-	memcpy(param.sa_key, &mpp->reservation_key, 8);
-	param.num_transportid = 0;
+	if (!clear_reg) {
+		param.sa_flags = mpp->sa_flags;
+		memcpy(param.sa_key, &mpp->reservation_key, 8);
+		param.num_transportid = 0;
+	}
 
-	condlog(3, "device %s:%s", pp->dev, pp->mpp->wwid);
+	condlog(3, "%s registration for device %s:%s",
+		clear_reg ? "Clearing" : "Setting", pp->dev, pp->mpp->wwid);
 
 	ret = prout_do_scsi_ioctl(pp->dev, MPATH_PROUT_REG_IGN_SA, 0, 0, &param, 0);
 	if (ret != MPATH_PR_SUCCESS )
 	{
-		condlog(0,"%s: Reservation registration failed. Error: %d", pp->dev, ret);
+		condlog(0, "%s: %s reservation registration failed. Error: %d",
+			clear_reg ? "Clearing" : "Setting", pp->dev, ret);
 	}
 }
-- 
2.48.1

[PATCH 07/14] libmpathpersist: Fix-up reservation_key checking

[Benjamin Marzinski]

The reservation key checking in do_mpath_persistent_reserve_out() was
slightly wrong. It allowed invalid keys for preempting. It now correctly
checks the reservation key for the preempt commands.

It also was a little overly strict in some places. Formerly, it only
allowed registering from any key to the configured key or registering
from the configured key to any key (as long as you use the prkeys file).
Now it also allows unregistering from any key and registering an
unregistered device to any key (as long as you use the prkeys file).

Also, clarify the code by replacing prkey with a bool tracking if you
are registering or unregistering.

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

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index c6b2e788..71933fe5 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -709,9 +709,9 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 {
 	struct multipath *mpp;
 	int ret;
-	uint64_t prkey;
+	uint64_t zerokey = 0;
 	struct config *conf;
-	bool preempting_reservation = false;
+	bool unregistering, preempting_reservation = false;
 
 	ret = mpath_get_map(curmp, pathvec, fd, &mpp);
 	if (ret != MPATH_PR_SUCCESS)
@@ -727,11 +727,12 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	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 &&
+	unregistering = (memcmp(&zerokey, paramp->sa_key, 8) == 0);
+	if (mpp->prkey_source == PRKEY_SOURCE_FILE && !unregistering &&
 	    (rq_servact == MPATH_PROUT_REG_IGN_SA ||
 	     (rq_servact == MPATH_PROUT_REG_SA &&
 	      (!get_be64(mpp->reservation_key) ||
+	       memcmp(paramp->key, &zerokey, 8) == 0 ||
 	       memcmp(paramp->key, &mpp->reservation_key, 8) == 0)))) {
 		memcpy(&mpp->reservation_key, paramp->sa_key, 8);
 		if (update_prkey_flags(mpp->alias, get_be64(mpp->reservation_key),
@@ -742,19 +743,38 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 		}
 	}
 
-	if (!get_be64(mpp->reservation_key) &&
-	    (prkey || rq_servact != MPATH_PROUT_REG_IGN_SA)) {
-		condlog(0, "%s: no configured reservation key", mpp->alias);
-		return MPATH_PR_SYNTAX_ERROR;
+	/*
+	 * If you are registering a non-zero key, mpp->reservation_key
+	 * must be set and must equal paramp->sa_key.
+	 * If you're not registering a key, mpp->reservation_key must be
+	 * set, and must equal paramp->key
+	 */
+	if ((rq_servact == MPATH_PROUT_REG_IGN_SA ||
+	     rq_servact == MPATH_PROUT_REG_SA)) {
+		if (!unregistering && !get_be64(mpp->reservation_key)) {
+			condlog(0, "%s: no configured reservation key",
+				mpp->alias);
+			return MPATH_PR_SYNTAX_ERROR;
+		}
+		if (!unregistering &&
+		    memcmp(paramp->sa_key, &mpp->reservation_key, 8)) {
+			condlog(0, "%s: configured reservation key doesn't match: 0x%" PRIx64,
+				mpp->alias, get_be64(mpp->reservation_key));
+			return MPATH_PR_SYNTAX_ERROR;
+		}
+	} else {
+		if (!get_be64(mpp->reservation_key)) {
+			condlog(0, "%s: no configured reservation key",
+				mpp->alias);
+			return MPATH_PR_SYNTAX_ERROR;
+		}
+		if (memcmp(paramp->key, &mpp->reservation_key, 8)) {
+			condlog(0, "%s: configured reservation key doesn't match: 0x%" PRIx64,
+				mpp->alias, get_be64(mpp->reservation_key));
+			return MPATH_PR_SYNTAX_ERROR;
+		}
 	}
 
-	if (memcmp(paramp->key, &mpp->reservation_key, 8) &&
-	    memcmp(paramp->sa_key, &mpp->reservation_key, 8) &&
-	    (prkey || rq_servact != MPATH_PROUT_REG_IGN_SA)) {
-		condlog(0, "%s: configured reservation key doesn't match: 0x%" PRIx64,
-			mpp->alias, get_be64(mpp->reservation_key));
-		return MPATH_PR_SYNTAX_ERROR;
-	}
 
 	switch(rq_servact)
 	{
@@ -785,7 +805,7 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	switch (rq_servact) {
 	case MPATH_PROUT_REG_SA:
 	case MPATH_PROUT_REG_IGN_SA:
-		if (prkey == 0) {
+		if (unregistering) {
 			update_prflag(mpp->alias, 0);
 			update_prkey(mpp->alias, 0);
 		} else
-- 
2.48.1

[PATCH 08/14] libmpathpersist: change how reservation conflicts are handled

[Benjamin Marzinski]

If registering a key on a path fails with a reservation conflict in
mpath_prout_reg(), libmpathpersist currently tries to roll back the
registration. This code doesn't always make much sense. First, it
updates the configured key, but doesn't fix it if it does a rollback.
Second, it always rolls the key back to 0x0, unregistering paths that
may have been previously registered. These rollback only happen on the
paths where the registration succeeded, meaning that they were in the
expected state when the command was run. The paths where the command
failed, that were in an unexpected state, remain in that state.

The code no longer attempts to rollback registrations that failed
with a reservation conflict. Instead, it checks that at least one
path was in the expected state and was successfully registered. If
so, then it assumes that the registration command was a resonable one
and retries it on the paths that failed with a reservation conflict.
But instead of using MPATH_PROUT_REG_SA, it uses MPATH_PROUT_REG_IGN_SA
so that it will ignore the current key. This will keep it from
failing with a reservation conflict because the path doesn't have the
expected key registered on it. If path reservations failed for reasons
other than a reservation conflict, the command still returns failure.

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

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 71933fe5..ad98001d 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -344,13 +344,13 @@ static int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 	int i, j, k;
 	struct pathgroup *pgp = NULL;
 	struct path *pp = NULL;
-	int rollback = 0;
+	bool can_retry = false;
+	bool need_retry = false;
 	int active_pathcount=0;
 	int rc;
 	int count=0;
 	int status = MPATH_PR_SUCCESS;
 	int all_tg_pt;
-	uint64_t sa_key = 0;
 
 	if (!mpp)
 		return MPATH_PR_DMMP_ERROR;
@@ -439,43 +439,62 @@ static int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 				condlog (0, "%s: Thread[%d] failed to join thread %d", mpp->wwid, i, rc);
 			}
 		}
-		if (!rollback && (thread[i].param.status == MPATH_PR_RESERV_CONFLICT)){
-			rollback = 1;
-			sa_key = get_unaligned_be64(&paramp->sa_key[0]);
-			status = MPATH_PR_RESERV_CONFLICT ;
-		}
-		if (!rollback && (status == MPATH_PR_SUCCESS)){
+		/*
+		 * We only retry if there is at least one registration that
+		 * returned a reservation conflict (which we need to retry)
+		 * and at least one registration the return success, so we
+		 * know that the command worked on some of the paths. If
+		 * the registation fails on all paths, then it wasn't a
+		 * valid request, so there's no need to retry.
+		 */
+		if (thread[i].param.status == MPATH_PR_RESERV_CONFLICT)
+			need_retry = true;
+		else if (thread[i].param.status == MPATH_PR_SUCCESS)
+			can_retry = true;
+		else if (status == MPATH_PR_SUCCESS)
 			status = thread[i].param.status;
-		}
 	}
-	if (rollback && ((rq_servact == MPATH_PROUT_REG_SA) && sa_key != 0 )){
-		condlog (3, "%s: ERROR: initiating pr out rollback", mpp->wwid);
-		memcpy(&paramp->key, &paramp->sa_key, 8);
-		memset(&paramp->sa_key, 0, 8);
-		for( i=0 ; i < count ; i++){
-			if(thread[i].param.status == MPATH_PR_SUCCESS) {
-				rc = pthread_create(&thread[i].id, &attr, mpath_prout_pthread_fn,
-						(void *)(&thread[i].param));
-				if (rc){
-					condlog (0, "%s: failed to create thread for rollback. %d",  mpp->wwid, rc);
-					thread[i].param.status = MPATH_PR_THREAD_ERROR;
-				}
-			} else
+	if (need_retry && can_retry && rq_servact == MPATH_PROUT_REG_SA &&
+	    status == MPATH_PR_SUCCESS) {
+		condlog(3, "%s: ERROR: initiating pr out retry", mpp->wwid);
+		for (i = 0; i < count; i++) {
+			if (thread[i].param.status != MPATH_PR_RESERV_CONFLICT) {
 				thread[i].param.status = MPATH_PR_SKIP;
+				continue;
+			}
+			/*
+			 * retry using MPATH_PROUT_REG_IGN_SA to avoid
+			 * conflicts. We already know that some paths
+			 * succeeded using MPATH_PROUT_REG_SA.
+			 */
+			thread[i].param.rq_servact = MPATH_PROUT_REG_IGN_SA;
+			rc = pthread_create(&thread[i].id, &attr,
+					    mpath_prout_pthread_fn,
+					    (void *)(&thread[i].param));
+			if (rc) {
+				condlog(0, "%s: failed to create thread for retry. %d",
+					mpp->wwid, rc);
+				thread[i].param.status = MPATH_PR_THREAD_ERROR;
+			}
 		}
-		for(i=0; i < count ; i++){
+		for (i = 0; i < count; i++) {
 			if (thread[i].param.status != MPATH_PR_SKIP &&
 			    thread[i].param.status != MPATH_PR_THREAD_ERROR) {
 				rc = pthread_join(thread[i].id, NULL);
-				if (rc){
-					condlog (3, "%s: failed to join thread while rolling back %d",
-						 mpp->wwid, i);
+				if (rc) {
+					condlog(3, "%s: failed to join thread while retrying %d",
+						mpp->wwid, i);
 				}
+				if (status == MPATH_PR_SUCCESS)
+					status = thread[i].param.status;
 			}
 		}
+		need_retry = false;
 	}
 
 	pthread_attr_destroy(&attr);
+	if (need_retry)
+		status = MPATH_PR_RESERV_CONFLICT;
 	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;
-- 
2.48.1

[PATCH 09/14] libmpathpersist: Clear prkey in multipathd before unregistering

[Benjamin Marzinski]

When you register or switch keys in libmpathpersist, it updates
mpp->reservation_key in multipathd before doing the registration. This
means that any paths that come online while you are doing the
registration get the new key registered. libmpathpersist didn't do
this when unregistering a key. This could cause the same problem.  A
path that got restored while unregistering the device could end up
getting the old key registered on it. Fix this by unsetting the key
before doing the unregister, instead of afterwards.

There is still a race condition associated with updating
mpp->reservation_key before doing the registration (but not on
unregistration). This will be dealt with by a future patch.

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

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index ad98001d..dfdadab6 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -747,7 +747,7 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 		set_ignored_key(mpp, paramp->key);
 
 	unregistering = (memcmp(&zerokey, paramp->sa_key, 8) == 0);
-	if (mpp->prkey_source == PRKEY_SOURCE_FILE && !unregistering &&
+	if (mpp->prkey_source == PRKEY_SOURCE_FILE &&
 	    (rq_servact == MPATH_PROUT_REG_IGN_SA ||
 	     (rq_servact == MPATH_PROUT_REG_SA &&
 	      (!get_be64(mpp->reservation_key) ||
@@ -824,10 +824,9 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	switch (rq_servact) {
 	case MPATH_PROUT_REG_SA:
 	case MPATH_PROUT_REG_IGN_SA:
-		if (unregistering) {
+		if (unregistering)
 			update_prflag(mpp->alias, 0);
-			update_prkey(mpp->alias, 0);
-		} else
+		else
 			update_prflag(mpp->alias, 1);
 		break;
 	case MPATH_PROUT_CLEAR_SA:
-- 
2.48.1

[PATCH 10/14] libmpathpersist: only clear the key if we are using the prkeys file

[Benjamin Marzinski]

Otherwise this request will create a useless prkeys file.

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

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index dfdadab6..f901b955 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -831,7 +831,8 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 		break;
 	case MPATH_PROUT_CLEAR_SA:
 		update_prflag(mpp->alias, 0);
-		update_prkey(mpp->alias, 0);
+		if (mpp->prkey_source == PRKEY_SOURCE_FILE)
+			update_prkey(mpp->alias, 0);
 		break;
 	case MPATH_PROUT_RES_SA:
 	case MPATH_PROUT_REL_SA:
-- 
2.48.1

[PATCH 11/14] libmpathpersist: Restore old reservation key on failure

[Benjamin Marzinski]

If we updated the key and then failed, restore the old key.

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

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index f901b955..ca3dab5c 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -729,8 +729,10 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	struct multipath *mpp;
 	int ret;
 	uint64_t zerokey = 0;
+	struct be64 oldkey = {0};
 	struct config *conf;
 	bool unregistering, preempting_reservation = false;
+	bool updated_prkey = false;
 
 	ret = mpath_get_map(curmp, pathvec, fd, &mpp);
 	if (ret != MPATH_PR_SUCCESS)
@@ -753,6 +755,8 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	      (!get_be64(mpp->reservation_key) ||
 	       memcmp(paramp->key, &zerokey, 8) == 0 ||
 	       memcmp(paramp->key, &mpp->reservation_key, 8) == 0)))) {
+		updated_prkey = true;
+		memcpy(&oldkey, &mpp->reservation_key, 8);
 		memcpy(&mpp->reservation_key, paramp->sa_key, 8);
 		if (update_prkey_flags(mpp->alias, get_be64(mpp->reservation_key),
 				       paramp->sa_flags)) {
@@ -818,8 +822,12 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 		return MPATH_PR_OTHER;
 	}
 
-	if (ret != MPATH_PR_SUCCESS)
+	if (ret != MPATH_PR_SUCCESS) {
+		if (updated_prkey)
+			update_prkey_flags(mpp->alias, get_be64(oldkey),
+					   mpp->sa_flags);
 		return ret;
+	}
 
 	switch (rq_servact) {
 	case MPATH_PROUT_REG_SA:
-- 
2.48.1

[PATCH 12/14] libmpatpersist: update reservation key before checking paths

[Benjamin Marzinski]

There is a race condition when changing reservation keys where a failed
path could come back online after libmpathpersist checks the paths, but
before it updates the reservation key. In this case, the path would come
up and get reregistered with the old key by multipathd, and
libmpathpersist would not update its key, because the path was down
when it checked.

To fix this, check the paths after updating the key, so any path that
comes up after getting checked will use the updated key.

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

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index ca3dab5c..06747391 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -111,39 +111,18 @@ void *mpath_alloc_prin_response(int prin_sa)
 	return ptr;
 }
 
-static int get_mpvec(vector curmp, vector pathvec, char *refwwid)
+static int get_path_info(struct multipath *mpp, vector pathvec)
 {
-	int i;
-	struct multipath *mpp;
-
-	vector_foreach_slot (curmp, mpp, i){
-		/*
-		 * discard out of scope maps
-		 */
-		if (!mpp->alias) {
-			condlog(0, "%s: map with empty alias!", __func__);
-			continue;
-		}
-
-		if (mpp->pg != NULL)
-			/* Already seen this one */
-			continue;
-
-		if (refwwid && strncmp (mpp->alias, refwwid, WWID_SIZE - 1))
-			continue;
-
-		if (update_multipath_table(mpp, pathvec, DI_CHECKER) != DMP_OK ||
-		    update_mpp_paths(mpp, pathvec)) {
-			condlog(1, "error parsing map %s", mpp->wwid);
-			remove_map(mpp, pathvec, curmp);
-			i--;
-		} else
-			extract_hwe_from_path(mpp);
+	if (update_multipath_table(mpp, pathvec, DI_CHECKER) != DMP_OK ||
+	    update_mpp_paths(mpp, pathvec)) {
+		condlog(0, "error parsing map %s", mpp->wwid);
+		return MPATH_PR_DMMP_ERROR;
 	}
+	extract_hwe_from_path(mpp);
 	return MPATH_PR_SUCCESS ;
 }
 
-static int mpath_get_map(vector curmp, vector pathvec, int fd, struct multipath **pmpp)
+static int mpath_get_map(vector curmp, int fd, struct multipath **pmpp)
 {
 	int rc;
 	struct stat info;
@@ -175,12 +154,6 @@ static int mpath_get_map(vector curmp, vector pathvec, int fd, struct multipath
 
 	condlog(4, "alias = %s", alias);
 
-	/* get info of all paths from the dm device     */
-	if (get_mpvec(curmp, pathvec, alias)) {
-		condlog(0, "%s: failed to get device info.", alias);
-		return MPATH_PR_DMMP_ERROR;
-	}
-
 	mpp = find_mp_by_alias(curmp, alias);
 
 	if (!mpp) {
@@ -201,7 +174,11 @@ int do_mpath_persistent_reserve_in(vector curmp, vector pathvec,
 	struct multipath *mpp;
 	int ret;
 
-	ret = mpath_get_map(curmp, pathvec, fd, &mpp);
+	ret = mpath_get_map(curmp, fd, &mpp);
+	if (ret != MPATH_PR_SUCCESS)
+		return ret;
+
+	ret = get_path_info(mpp, pathvec);
 	if (ret != MPATH_PR_SUCCESS)
 		return ret;
 
@@ -734,20 +711,15 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	bool unregistering, preempting_reservation = false;
 	bool updated_prkey = false;
 
-	ret = mpath_get_map(curmp, pathvec, fd, &mpp);
+	ret = mpath_get_map(curmp, fd, &mpp);
 	if (ret != MPATH_PR_SUCCESS)
 		return ret;
 
 	conf = get_multipath_config();
 	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);
 
-	if (rq_servact == MPATH_PROUT_REG_IGN_SA)
-		set_ignored_key(mpp, paramp->key);
-
 	unregistering = (memcmp(&zerokey, paramp->sa_key, 8) == 0);
 	if (mpp->prkey_source == PRKEY_SOURCE_FILE &&
 	    (rq_servact == MPATH_PROUT_REG_IGN_SA ||
@@ -798,6 +770,21 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 		}
 	}
 
+	ret = get_path_info(mpp, pathvec);
+	if (ret != MPATH_PR_SUCCESS) {
+		if (updated_prkey)
+			update_prkey_flags(mpp->alias, get_be64(oldkey),
+					   mpp->sa_flags);
+		return ret;
+	}
+
+	conf = get_multipath_config();
+	select_all_tg_pt(conf, mpp);
+	select_skip_kpartx(conf, mpp);
+	put_multipath_config(conf);
+
+	if (rq_servact == MPATH_PROUT_REG_IGN_SA)
+		set_ignored_key(mpp, paramp->key);
 
 	switch(rq_servact)
 	{
-- 
2.48.1

[PATCH 13/14] libmpathpersist: retry on conflicts in mpath_prout_common

[Benjamin Marzinski]

mpath_prout_common() just needs to execute a prout command down one
path. If it uses a path that was down when the key was changed and has
since been restored, but multipathd hasn't noticed yet, that path will
still be using the old key. This was causing mpath_prout_common() to
fail with MPATH_PR_RESERV_CONFLICT, even if there were other paths that
would work.

Now, if prout command fails with MPATH_PR_RESERV_CONFLICT,
mpath_prout_common() checks if pp->dmstate is PSTATE_FAILED. If it is,
mpath_prout_common() assumes that multipathd has not yet noticed that
the path is back online and it might still have and old key, so it
doesn't immediately return. If it can't successfully send the command
down another path, it will still return MPATH_PR_RESERV_CONFLICT.

Also, make sure prout_do_scsi_ioctl() always returns a MPATH_PR_*
type error.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c | 14 +++++++++++++-
 libmpathpersist/mpath_pr_ioctl.c    |  2 +-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 06747391..d3c1a789 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -207,6 +207,7 @@ static int mpath_prout_common(struct multipath *mpp,int rq_servact, int rq_scope
 	struct pathgroup *pgp = NULL;
 	struct path *pp = NULL;
 	bool found = false;
+	bool conflict = false;
 
 	vector_foreach_slot (mpp->pg, pgp, j){
 		vector_foreach_slot (pgp->paths, pp, i){
@@ -222,12 +223,23 @@ static int mpath_prout_common(struct multipath *mpp,int rq_servact, int rq_scope
 						  rq_type, paramp, noisy);
 			if (ret == MPATH_PR_SUCCESS && pptr)
 				*pptr = pp;
+			/*
+			 * If this path is considered down by the kernel,
+			 * it may have just come back up, and multipathd
+			 * may not have had time to update the key. Allow
+			 * reservation conflicts.
+			 */
+			if (ret == MPATH_PR_RESERV_CONFLICT &&
+			    pp->dmstate == PSTATE_FAILED) {
+				conflict = true;
+				continue;
+			}
 			if (ret != MPATH_PR_RETRYABLE_ERROR)
 				return ret;
 		}
 	}
 	if (found)
-		return MPATH_PR_OTHER;
+		return conflict ? MPATH_PR_RESERV_CONFLICT : MPATH_PR_OTHER;
 	condlog (0, "%s: no path available", mpp->wwid);
 	return MPATH_PR_DMMP_ERROR;
 }
diff --git a/libmpathpersist/mpath_pr_ioctl.c b/libmpathpersist/mpath_pr_ioctl.c
index dfdbbb65..6eaec7cd 100644
--- a/libmpathpersist/mpath_pr_ioctl.c
+++ b/libmpathpersist/mpath_pr_ioctl.c
@@ -103,7 +103,7 @@ retry :
 	{
 		condlog(0, "%s: ioctl failed %d", dev, ret);
 		close(fd);
-		return ret;
+		return MPATH_PR_OTHER;
 	}
 
 	condlog(4, "%s: Duration=%u (ms)", dev, io_hdr.duration);
-- 
2.48.1

[PATCH 14/14] libmpathpersist: Don't always fail registrations for retryable errors

[Benjamin Marzinski]

When libmpathpersist registers a key, it's possible that a path fails
between when it checks the path's status, and when it tries to do the
registrations on the path. In this case, the registration will fail with
a retryable error. If the registration was allowed to succeed,
multipathd would update the now failed path's key when it came back
online, and everything would work correctly. However it is possible for
a registration to fail with a retryable error on a path that is still
usable.

Libmpathpersist needs to avoid the case where it does not update the
key of a usable path. Otherwise the path might be able to write to
storage it shouldn't be allowed to. Or it could fail writing to storage
that it should be allowed to write to. So if a registration would
succeed except for retryable errors, libmpathpersist now rechecks all
those paths to see if they are still usable. If they are, then it fails
the registration as before. If they are not, then the registration
succeeds.

Also, rename can_retry to had_success, since it is used for checking
more than retries now.

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

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index d3c1a789..d5e441ef 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -325,6 +325,48 @@ void preempt_missing_path(struct multipath *mpp, uint8_t *key, uint8_t *sa_key,
 			mpp->wwid);
 }
 
+/*
+ * If libmpathpersist fails at updating the key on a path with a retryable
+ * error, it has probably failed. But there is a chance that the path is
+ * still usable. To make sure a path isn't active without a key, when it
+ * should have one, or with a key, when it shouldn't have one, check if
+ * the path is still usable. If it is, we must fail the registration.
+ */
+static int check_failed_paths(struct multipath *mpp,
+			      struct threadinfo *thread, int count)
+{
+	int i, j, k;
+	int ret;
+	struct pathgroup *pgp;
+	struct path *pp;
+	struct config *conf;
+
+	for (i = 0; i < count; i++) {
+		if (thread[i].param.status != MPATH_PR_RETRYABLE_ERROR)
+			continue;
+		vector_foreach_slot (mpp->pg, pgp, j) {
+			vector_foreach_slot (pgp->paths, pp, k) {
+				if (strncmp(pp->dev, thread[i].param.dev,
+					    FILE_NAME_SIZE) == 0)
+					goto match;
+			}
+		}
+		/* no match. This shouldn't ever happen. */
+		condlog(0, "%s: Error: can't find path %s", mpp->wwid,
+			thread[i].param.dev);
+		continue;
+match:
+		conf = get_multipath_config();
+		ret = pathinfo(pp, conf, DI_CHECKER);
+		put_multipath_config(conf);
+		/* If pathinfo fails, or if the path is active, return error */
+		if (ret != PATHINFO_OK || pp->state == PATH_UP ||
+		    pp->state == PATH_GHOST)
+			return MPATH_PR_OTHER;
+	}
+	return MPATH_PR_SUCCESS;
+}
+
 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)
@@ -333,8 +375,9 @@ static int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 	int i, j, k;
 	struct pathgroup *pgp = NULL;
 	struct path *pp = NULL;
-	bool can_retry = false;
+	bool had_success = false;
 	bool need_retry = false;
+	bool retryable_error = false;
 	int active_pathcount=0;
 	int rc;
 	int count=0;
@@ -438,16 +481,20 @@ static int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 		 */
 		if (thread[i].param.status == MPATH_PR_RESERV_CONFLICT)
 			need_retry = true;
+		else if (thread[i].param.status == MPATH_PR_RETRYABLE_ERROR)
+			retryable_error = true;
 		else if (thread[i].param.status == MPATH_PR_SUCCESS)
-			can_retry = true;
+			had_success = true;
 		else if (status == MPATH_PR_SUCCESS)
 			status = thread[i].param.status;
 	}
-	if (need_retry && can_retry && rq_servact == MPATH_PROUT_REG_SA &&
+	if (need_retry && had_success && rq_servact == MPATH_PROUT_REG_SA &&
 	    status == MPATH_PR_SUCCESS) {
 		condlog(3, "%s: ERROR: initiating pr out retry", mpp->wwid);
+		retryable_error = false;
 		for (i = 0; i < count; i++) {
-			if (thread[i].param.status != MPATH_PR_RESERV_CONFLICT) {
+			/* retry retryable errors and conflicts */
+			if (thread[i].param.status != MPATH_PR_RESERV_CONFLICT && thread[i].param.status != MPATH_PR_RETRYABLE_ERROR) {
 				thread[i].param.status = MPATH_PR_SKIP;
 				continue;
 			}
@@ -474,7 +521,9 @@ static int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 					condlog(3, "%s: failed to join thread while retrying %d",
 						mpp->wwid, i);
 				}
-				if (status == MPATH_PR_SUCCESS)
+				if (thread[i].param.status == MPATH_PR_RETRYABLE_ERROR)
+					retryable_error = true;
+				else if (status == MPATH_PR_SUCCESS)
 					status = thread[i].param.status;
 			}
 		}
@@ -483,10 +532,17 @@ static int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 
 	pthread_attr_destroy(&attr);
 	if (need_retry)
-		status = MPATH_PR_RESERV_CONFLICT;
+		return MPATH_PR_RESERV_CONFLICT;
+	if (status != MPATH_PR_SUCCESS)
+		return status;
+	/* If you had retryable errors on all paths, fail the registration */
+	if (!had_success)
+		return MPATH_PR_OTHER;
+	if (retryable_error)
+		status = check_failed_paths(mpp, thread, count);
 	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;
+	return status;
 }
 
 static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
-- 
2.48.1

Re: [PATCH 10/14] libmpathpersist: only clear the key if we are using the prkeys file

[Martin Wilck]

On Fri, 2025-07-25 at 23:58 -0400, Benjamin Marzinski wrote:
> Otherwise this request will create a useless prkeys file.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmpathpersist/mpath_persist_int.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/libmpathpersist/mpath_persist_int.c
> b/libmpathpersist/mpath_persist_int.c
> index dfdadab6..f901b955 100644
> --- a/libmpathpersist/mpath_persist_int.c
> +++ b/libmpathpersist/mpath_persist_int.c
> @@ -831,7 +831,8 @@ int do_mpath_persistent_reserve_out(vector curmp,
> vector pathvec, int fd,
>  		break;
>  	case MPATH_PROUT_CLEAR_SA:
>  		update_prflag(mpp->alias, 0);
> -		update_prkey(mpp->alias, 0);
> +		if (mpp->prkey_source == PRKEY_SOURCE_FILE)
> +			update_prkey(mpp->alias, 0);

Should this condition be checked in update_prkey_flags() directly?

Martin

Re: [PATCH 12/14] libmpatpersist: update reservation key before checking paths

[Martin Wilck]

On Fri, 2025-07-25 at 23:58 -0400, Benjamin Marzinski wrote:
> There is a race condition when changing reservation keys where a
> failed
> path could come back online after libmpathpersist checks the paths,
> but
> before it updates the reservation key. In this case, the path would
> come
> up and get reregistered with the old key by multipathd, and
> libmpathpersist would not update its key, because the path was down
> when it checked.
> 
> To fix this, check the paths after updating the key, so any path that
> comes up after getting checked will use the updated key.

In do_mpath_persistent_reserve_out(), you call update_prkey_flags()
before checking for MPATH_PR_SYNTAX_ERROR. Perhaps you should check the
key parameters first?

Martin

Re: [PATCH 00/14] Additional fixes and cleanups

[Martin Wilck]

On Fri, 2025-07-25 at 23:58 -0400, Benjamin Marzinski wrote:
> This patchset builds on top of my previous libmpathpersist patchset,
> ("Improve mpathpersist's unavailable path handling") and add various
> fixes and cleanups to multipath's persistent reservation handling.
> 
> The issues handled by this patchset are:
> - Handling removal of keys from paths that were down when the
> multipath
>   device was unregistered.
> - Changing how conflicts on key registration are handled. Instead of
>   the current rollback method (which was broken anyways),
>   libmpathpersist now retrys with REGISTER AND IGNORE, as long as the
>   REGISTER command completed successfully down some of the paths.
> - Changing when the reservation key is set to fix corner cases on
>   failure and registration while paths are coming up.
> - Retrying on conflicts in mpath_prout_common to fix corner cases
> when
>   a path is coming up while a doing a reserve, preempt or clear.
> - Allowing registrations to succeed when there are retryable errors, 
>   if the paths are actually down.
> - Fixing the reservation key validation code
> 
> I realize that a number of these fixes are dealing with races between
> libmpathperist and multipathd, which points to shortcomings with the
> current design of libmpathpersist. The code could likely be simpler
> and 
> more robust if limpathpersist was a thin wrapper around commands to
> multipathd, which handled issuing the actual persistent reservation
> commands. However, we have customers that need persistent
> reservations
> working well on currently released versions of the mutipath-tools,
> and
> redesigning how the libmpathpersist library works is a much larger
> task,
> and even harder to backport. I'd like to know if people think that
> would
> be a worthwhile task for later, however.

Difficult to say. It seems to me that you have eliminated most obvious
races. But yes, it would be cleaner to have a single instance
(multipathd) deal with PRs.

It is awkward that libmpathpersist contains code that sends commands to
multipathd, while multipathd itself links with libmpathpersist. This
begs for a separation of libraries.

Also, I think there are still too many instances of condlog(0, ...) in
libmpathpersist. But then, there are too many in our code in general,
so we can clean this up later.


> 
> Benjamin Marzinski (14):
>   limpathpersist: remove update_map_pr code for NULL pp
>   libmpathpersist: move update_map_pr to multipathd
>   multipathd: clean up update_map_pr and mpath_pr_event_handle
>   libmpathpersist: clean up duplicate function declarations
>   multipathd: wrap setting and unsetting prflag
>   multipathd: unregister PR key when path is restored if necessary
>   libmpathpersist: Fix-up reservation_key checking
>   libmpathpersist: change how reservation conflicts are handled
>   libmpathpersist: Clear prkey in multipathd before unregistering
>   libmpathpersist: only clear the key if we are using the prkeys file
>   libmpathpersist: Restore old reservation key on failure
>   libmpatpersist: update reservation key before checking paths
>   libmpathpersist: retry on conflicts in mpath_prout_common
>   libmpathpersist: Don't always fail registrations for retryable
> errors
> 
>  libmpathpersist/libmpathpersist.version |   1 -
>  libmpathpersist/mpath_persist_int.c     | 363 +++++++++++++---------
> --
>  libmpathpersist/mpath_persist_int.h     |   2 -
>  libmpathpersist/mpath_pr_ioctl.c        |  12 +-
>  libmultipath/structs.h                  |   2 +
>  mpathpersist/main.c                     |   2 -
>  multipathd/cli_handlers.c               |  11 +-
>  multipathd/main.c                       | 132 +++++++--
>  multipathd/main.h                       |   2 +
>  9 files changed, 311 insertions(+), 216 deletions(-)

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

As for the previous series, I have applied out clang-format formatting
and fixed a few typos, and pushed the result to my tip branch [1].

Regards
Martin

Re: [PATCH 10/14] libmpathpersist: only clear the key if we are using the prkeys file

[Benjamin Marzinski]

On Mon, Aug 25, 2025 at 06:21:59PM +0200, Martin Wilck wrote:
> On Fri, 2025-07-25 at 23:58 -0400, Benjamin Marzinski wrote:
> > Otherwise this request will create a useless prkeys file.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmpathpersist/mpath_persist_int.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/libmpathpersist/mpath_persist_int.c
> > b/libmpathpersist/mpath_persist_int.c
> > index dfdadab6..f901b955 100644
> > --- a/libmpathpersist/mpath_persist_int.c
> > +++ b/libmpathpersist/mpath_persist_int.c
> > @@ -831,7 +831,8 @@ int do_mpath_persistent_reserve_out(vector curmp,
> > vector pathvec, int fd,
> >  		break;
> >  	case MPATH_PROUT_CLEAR_SA:
> >  		update_prflag(mpp->alias, 0);
> > -		update_prkey(mpp->alias, 0);
> > +		if (mpp->prkey_source == PRKEY_SOURCE_FILE)
> > +			update_prkey(mpp->alias, 0);
> 
> Should this condition be checked in update_prkey_flags() directly?

We could, but it would just end up being a pointless extra check most of
the time. Aside from the CLEAR command, we only set the prkey when we
are registering a key or reverting the prkey if that registration
failed. When we first set it during register, we need to check
prkey_source early, since there is a bunch of things we can't do if we
aren't using the prkeys file. And we don't want to revert it unless we
updated it and have an old value to revert to.  In both cases we already
had to do the necessary check before calling update_prkey(). The CLEAR
command is the only one where we don't need to check if we are using the
prkeys file earlier. So, I'd prefer to leave the check outside of
update_prkey_flags() here as well, to avoid the unnecessary extra work.

-Ben

> 
> Martin

Re: [PATCH 12/14] libmpatpersist: update reservation key before checking paths

[Benjamin Marzinski]

On Mon, Aug 25, 2025 at 06:36:18PM +0200, Martin Wilck wrote:
> On Fri, 2025-07-25 at 23:58 -0400, Benjamin Marzinski wrote:
> > There is a race condition when changing reservation keys where a
> > failed
> > path could come back online after libmpathpersist checks the paths,
> > but
> > before it updates the reservation key. In this case, the path would
> > come
> > up and get reregistered with the old key by multipathd, and
> > libmpathpersist would not update its key, because the path was down
> > when it checked.
> > 
> > To fix this, check the paths after updating the key, so any path that
> > comes up after getting checked will use the updated key.
> 
> In do_mpath_persistent_reserve_out(), you call update_prkey_flags()
> before checking for MPATH_PR_SYNTAX_ERROR. Perhaps you should check the
> key parameters first?

We could, but the way the code currently is, if you called
update_prkey_flags() earlier, you can't fail those MPATH_PR_SYNTAX_ERROR
checks.

You can only call update_prkey_flags() if rq_servact is
MPATH_PROUT_REG_IGN_SA or MPATH_PROUT_REG_SA, so we only need to look at
the True branch of the outermost if statement in those
MPATH_PR_SYNTAX_ERROR checks. We also cannot fail those checks if
unregistering is true, so we only care about the case were
paramp->sa_key is non-zero. And when we call update_prkey_flags(), we
also copy the value of paramp->sa_key to mpp->reservation_key, so it too
must be non-zero. This means that can't fail the checks since they
check if mpp->reservation_key is zero or not equal to paramp->sa_key.

But it doesn't hury anything to move the update_prkey_flags() call to
after those checks either. So I'm fine with either solving this by
adding an explanation of why we don't need to worry about failing these
checks if we updated the key to the comments above the checks, or by
just moving the update_prkey_flags() call to after them. Do you have a
preference?

-Ben

 
> Martin

Re: [PATCH 10/14] libmpathpersist: only clear the key if we are using the prkeys file

[Martin Wilck]

On Thu, 2025-08-28 at 23:21 -0400, Benjamin Marzinski wrote:
> On Mon, Aug 25, 2025 at 06:21:59PM +0200, Martin Wilck wrote:
> > On Fri, 2025-07-25 at 23:58 -0400, Benjamin Marzinski wrote:
> > > Otherwise this request will create a useless prkeys file.
> > > 
> > > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > > ---
> > >  libmpathpersist/mpath_persist_int.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/libmpathpersist/mpath_persist_int.c
> > > b/libmpathpersist/mpath_persist_int.c
> > > index dfdadab6..f901b955 100644
> > > --- a/libmpathpersist/mpath_persist_int.c
> > > +++ b/libmpathpersist/mpath_persist_int.c
> > > @@ -831,7 +831,8 @@ int do_mpath_persistent_reserve_out(vector
> > > curmp,
> > > vector pathvec, int fd,
> > >  		break;
> > >  	case MPATH_PROUT_CLEAR_SA:
> > >  		update_prflag(mpp->alias, 0);
> > > -		update_prkey(mpp->alias, 0);
> > > +		if (mpp->prkey_source == PRKEY_SOURCE_FILE)
> > > +			update_prkey(mpp->alias, 0);
> > 
> > Should this condition be checked in update_prkey_flags() directly?
> 
> We could, but it would just end up being a pointless extra check most
> of
> the time. Aside from the CLEAR command, we only set the prkey when we
> are registering a key or reverting the prkey if that registration
> failed. When we first set it during register, we need to check
> prkey_source early, since there is a bunch of things we can't do if
> we
> aren't using the prkeys file. And we don't want to revert it unless
> we
> updated it and have an old value to revert to.  In both cases we
> already
> had to do the necessary check before calling update_prkey(). The
> CLEAR
> command is the only one where we don't need to check if we are using
> the
> prkeys file earlier. So, I'd prefer to leave the check outside of
> update_prkey_flags() here as well, to avoid the unnecessary extra
> work.

Well, the unnecessary work is just a single instruction in this case.
I think multipathd has more pressing efficiency issues.
Perhaps we could improve our layering.

But it isn't important. I'm fine with leaving it as it currently is.

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

Martin

Re: [PATCH 12/14] libmpatpersist: update reservation key before checking paths

[Martin Wilck]

On Thu, 2025-08-28 at 23:37 -0400, Benjamin Marzinski wrote:
> On Mon, Aug 25, 2025 at 06:36:18PM +0200, Martin Wilck wrote:
> > On Fri, 2025-07-25 at 23:58 -0400, Benjamin Marzinski wrote:
> > > There is a race condition when changing reservation keys where a
> > > failed
> > > path could come back online after libmpathpersist checks the
> > > paths,
> > > but
> > > before it updates the reservation key. In this case, the path
> > > would
> > > come
> > > up and get reregistered with the old key by multipathd, and
> > > libmpathpersist would not update its key, because the path was
> > > down
> > > when it checked.
> > > 
> > > To fix this, check the paths after updating the key, so any path
> > > that
> > > comes up after getting checked will use the updated key.
> > 
> > In do_mpath_persistent_reserve_out(), you call update_prkey_flags()
> > before checking for MPATH_PR_SYNTAX_ERROR. Perhaps you should check
> > the
> > key parameters first?
> 
> We could, but the way the code currently is, if you called
> update_prkey_flags() earlier, you can't fail those
> MPATH_PR_SYNTAX_ERROR
> checks.
> 
> You can only call update_prkey_flags() if rq_servact is
> MPATH_PROUT_REG_IGN_SA or MPATH_PROUT_REG_SA, so we only need to look
> at
> the True branch of the outermost if statement in those
> MPATH_PR_SYNTAX_ERROR checks. We also cannot fail those checks if
> unregistering is true, so we only care about the case were
> paramp->sa_key is non-zero. And when we call update_prkey_flags(), we
> also copy the value of paramp->sa_key to mpp->reservation_key, so it
> too
> must be non-zero. This means that can't fail the checks since they
> check if mpp->reservation_key is zero or not equal to paramp->sa_key.
> 
> But it doesn't hury anything to move the update_prkey_flags() call to
> after those checks either. So I'm fine with either solving this by
> adding an explanation of why we don't need to worry about failing
> these
> checks if we updated the key to the comments above the checks, or by
> just moving the update_prkey_flags() call to after them. Do you have
> a
> preference?

I gather that the conditional would become more complicated when we
move the code. A comment with an explanation will do the job.

Martin