[PATCH 0/2] libmpathpersist: reserve and release fixes

[PATCH 0/2] libmpathpersist: reserve and release fixes

[Benjamin Marzinski]

These are two patches libmpathpersist patches. The first deals with a
possible race condition where mpath_prout_common() can fail because a
path went down after libmpathpersist checked its status but before it
handled the prout command. Previously libmpathpersist wouldn't fail
without ever trying another path in these cases. The second patch
keeps libmpathpersit from being able to remove a reservation that
is held by a different key than the releasing device. 

Benjamin Marzinski (2):
  libmpathpersist: retry commands on other paths in mpath_prout_common
  libmpathpersist: check released key against the reservation key

 libmpathpersist/mpath_persist.h     |  3 +++
 libmpathpersist/mpath_persist_int.c | 20 ++++++++++++++++----
 libmpathpersist/mpath_pr_ioctl.c    | 24 +++++++++++++-----------
 3 files changed, 32 insertions(+), 15 deletions(-)

-- 
2.48.1

[PATCH 1/2] libmpathpersist: retry commands on other paths in mpath_prout_common

[Benjamin Marzinski]

mpath_prout_common() will only try sending the prout command to one
path. If that fails, it will give up. There are a number of cases where
it is reasonable to assume that sending the command down another path
could succeed. Keep trying other available paths in these cases.

Do do this, this patch adds a new error code, MPATH_PR_RETRYABLE_ERROR.
libmpathpersist will not return this error to users. It will change it
to MPATH_PR_OTHER if it fails trying on all the paths.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist.h     |  3 +++
 libmpathpersist/mpath_persist_int.c | 13 +++++++++----
 libmpathpersist/mpath_pr_ioctl.c    | 24 +++++++++++++-----------
 3 files changed, 25 insertions(+), 15 deletions(-)

diff --git a/libmpathpersist/mpath_persist.h b/libmpathpersist/mpath_persist.h
index d94205f0..32aa7ffe 100644
--- a/libmpathpersist/mpath_persist.h
+++ b/libmpathpersist/mpath_persist.h
@@ -62,6 +62,9 @@ extern "C" {
 #define MPATH_PR_THREAD_ERROR		14  /* pthreads error (e.g. unable to create new thread) */
 #define MPATH_PR_OTHER			15  /*other error/warning has occurred(transport
 					      or driver error) */
+#define MPATH_PR_RETRYABLE_ERROR	16  /* error that might be succeed
+					       down another path. Internal
+					       only. */
 
 /* PR MASK */
 #define MPATH_F_APTPL_MASK		0x01	/* APTPL MASK*/
diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 7a6992c2..b838a014 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -92,7 +92,7 @@ static int mpath_prin_activepath (struct multipath *mpp, int rq_servact,
 			}
 		}
 	}
-	return ret;
+	return (ret == MPATH_PR_RETRYABLE_ERROR) ? MPATH_PR_OTHER : ret;
 }
 
 void *mpath_alloc_prin_response(int prin_sa)
@@ -372,7 +372,7 @@ static int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 	}
 
 	pthread_attr_destroy(&attr);
-	return (status);
+	return (status == MPATH_PR_RETRYABLE_ERROR) ? MPATH_PR_OTHER : status;
 }
 
 static int send_prout_activepath(char *dev, int rq_servact, int rq_scope,
@@ -416,6 +416,7 @@ static int mpath_prout_common(struct multipath *mpp,int rq_servact, int rq_scope
 	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){
@@ -426,12 +427,16 @@ 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);
-			return ret ;
+			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;
 }
@@ -635,7 +640,7 @@ out1:
 	free (pamp);
 out:
 	free (pr_buff);
-	return (status);
+	return (status == MPATH_PR_RETRYABLE_ERROR) ? MPATH_PR_OTHER : status;
 }
 
 int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
diff --git a/libmpathpersist/mpath_pr_ioctl.c b/libmpathpersist/mpath_pr_ioctl.c
index 093ec71b..bf3a817d 100644
--- a/libmpathpersist/mpath_pr_ioctl.c
+++ b/libmpathpersist/mpath_pr_ioctl.c
@@ -52,7 +52,7 @@ int prout_do_scsi_ioctl(char * dev, int rq_servact, int rq_scope,
 	fd = open(devname, O_RDONLY);
 	if(fd < 0){
 		condlog (1, "%s: unable to open device.", dev);
-		return MPATH_PR_FILE_ERROR;
+		return MPATH_PR_RETRYABLE_ERROR;
 	}
 
 	unsigned char cdb[MPATH_PROUT_CMDLEN] =
@@ -123,14 +123,16 @@ retry :
 		goto retry;
 	}
 
-	if (((status == MPATH_PR_SENSE_NOT_READY )&& (Sensedata.ASC == 0x04)&&
-				(Sensedata.ASCQ == 0x07))&& (retry > 0))
-	{
-		usleep(1000);
-		--retry;
-		condlog(3, "%s: retrying for sense 02/04/07."
-			" Remaining retries = %d", dev, retry);
-		goto retry;
+	if (status == MPATH_PR_SENSE_NOT_READY) {
+		if (Sensedata.ASC == 0x04 && Sensedata.ASCQ == 0x07 &&
+		    retry > 0) {
+				usleep(1000);
+				--retry;
+				condlog(3, "%s: retrying for sense 02/04/07."
+					" Remaining retries = %d", dev, retry);
+				goto retry;
+		} else
+			status = MPATH_PR_RETRYABLE_ERROR;
 	}
 
 	close(fd);
@@ -342,7 +344,7 @@ int prin_do_scsi_ioctl(char * dev, int rq_servact, struct prin_resp * resp, int
 	fd = open(devname, O_RDONLY);
 	if(fd < 0){
 		condlog(0, "%s: Unable to open device ", dev);
-		return MPATH_PR_FILE_ERROR;
+		return MPATH_PR_RETRYABLE_ERROR;
 	}
 
 	if (mpath_mx_alloc_len)
@@ -488,7 +490,7 @@ int mpath_translate_response (char * dev, struct sg_io_hdr io_hdr,
 	case DID_OK :
 		break;
 	default :
-		return MPATH_PR_OTHER;
+		return MPATH_PR_RETRYABLE_ERROR;
 	}
 	switch(io_hdr.driver_status)
 	{
-- 
2.48.1

[PATCH 2/2] libmpathpersist: check released key against the reservation key

[Benjamin Marzinski]

According to the SCSI Spec, if a persistent reservation RELEASE is
issued using the correct key for the I_T_L Nexus that it is issued on
but the reservation is held by a different key, the command should
return Success and do nothing. When libmpathpersist tried to release a
reservation that was held by a different key, it ended up using the
failback code designed to release a reservation held by a failed path to
release it anyways. This means that any node could release a scsi
persistent reservation held by another node. Fix this to follow the SCSI
Spec.

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

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index b838a014..05739db8 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -541,6 +541,13 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 		condlog (2, "%s: Path holding reservation is released.", mpp->wwid);
 		return MPATH_PR_SUCCESS;
 	}
+	if (!get_be64(mpp->reservation_key) ||
+	    memcmp(&mpp->reservation_key, resp.prin_descriptor.prin_readresv.key, 8)) {
+		condlog(2, "%s: Releasing key not holding reservation.",
+			mpp->wwid);
+		return MPATH_PR_SUCCESS;
+	}
+
 	condlog (2, "%s: Path holding reservation is not available.", mpp->wwid);
 
 	pr_buff =  mpath_alloc_prin_response(MPATH_PRIN_RFSTAT_SA);
-- 
2.48.1

Re: [PATCH 0/2] libmpathpersist: reserve and release fixes

[Martin Wilck]

On Tue, 2025-06-03 at 23:35 -0400, Benjamin Marzinski wrote:
> These are two patches libmpathpersist patches. The first deals with a
> possible race condition where mpath_prout_common() can fail because a
> path went down after libmpathpersist checked its status but before it
> handled the prout command. Previously libmpathpersist wouldn't fail
> without ever trying another path in these cases. The second patch
> keeps libmpathpersit from being able to remove a reservation that
> is held by a different key than the releasing device. 
> 
> Benjamin Marzinski (2):
>   libmpathpersist: retry commands on other paths in
> mpath_prout_common
>   libmpathpersist: check released key against the reservation key
> 
>  libmpathpersist/mpath_persist.h     |  3 +++
>  libmpathpersist/mpath_persist_int.c | 20 ++++++++++++++++----
>  libmpathpersist/mpath_pr_ioctl.c    | 24 +++++++++++++-----------
>  3 files changed, 32 insertions(+), 15 deletions(-)

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