[PATCH 0/4] libmultipath: Yet Another PR patchset

[PATCH 0/4] libmultipath: Yet Another PR patchset

[Benjamin Marzinski]

This patchset handles 4 separate issues:
1. The self-preemption workaround for changing keys using REGISTER AND
   IGNORE, while the path holding the reservation was down was broken
   by a recent commit.
2. mpathpersist was failing a RESERVE command issued on a multipath
   device that held a reservation, when the holding path was down.
   Issuing an identical reserve on a device that holds a reservation
   is supposed to succeed, and multipathd has enough information to
   know the device is actually holding the reservation. So if
   multipathd says that the device is holding the reservation, it
   should not fail, even if the path holding the reservation is down.
3. mpathpersist was not clearing the reservation when the key holding
   it was unregistered, if the path holding the reservation was down.
4. When unregistering a key that holds a reservation, depending on the
   order that the paths got unregistered, there was a window where
   IO going to the device could fail. This should not happen because
   the reservation should be removed and the key should be unregistered
   atomically.

Benjamin Marzinski (4):
  libmpathpersist: Fix REGISTER AND IGNORE while holding a reservation
  libmpathpersist: Handle RESERVE with reservation held by failed path
  libmpathpersist: use check_holding_reservation in mpath_prout_rel
  libmpathpersist: Fix unregistering while holding the reservation

 libmpathpersist/mpath_persist_int.c | 199 +++++++++++++++++-----------
 1 file changed, 125 insertions(+), 74 deletions(-)

-- 
2.50.1

[PATCH 1/4] libmpathpersist: Fix REGISTER AND IGNORE while holding a reservation

[Benjamin Marzinski]

If a device that is holding a reservation changes its registered key,
but the path holding the reservation is unavailable, libmpathpersist
must preempt the old key to update the reservation. If the key is
changed using REGISTER AND IGNORE, set_ignored_key() determines the old
key to preempt. Unfortunately, commit 165427dda broke it, by comparing
the wrong key against the actual reservation key. Then commit cf0eea85
broke it more, by using mpp->reservation_key after it had been updated,
so it was no longer the old key. Fix this by correctly comparing the old
key against the actual reservation key.

Fixes: 165427dda ("libmpathpersist: Add safety check for preempting on key change")
Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 libmpathpersist/mpath_persist_int.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index ab3d70cd..476c3433 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -791,16 +791,17 @@ static int reservation_key_matches(struct multipath *mpp, uint8_t *key, int nois
  * 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)
+static void set_ignored_key(struct multipath *mpp, uint8_t *curr_key,
+			    uint8_t *key)
 {
 	memset(key, 0, 8);
-	if (!get_be64(mpp->reservation_key))
+	if (memcmp(curr_key, key, 8) == 0)
 		return;
 	if (get_prhold(mpp->alias) == PR_UNSET)
 		return;
-	if (reservation_key_matches(mpp, key, 0) == YNU_NO)
+	if (reservation_key_matches(mpp, curr_key, 0) == YNU_NO)
 		return;
-	memcpy(key, &mpp->reservation_key, 8);
+	memcpy(key, curr_key, 8);
 }
 
 int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
@@ -824,6 +825,7 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	select_reservation_key(conf, mpp);
 	put_multipath_config(conf);
 
+	memcpy(&oldkey, &mpp->reservation_key, 8);
 	unregistering = (memcmp(&zerokey, paramp->sa_key, 8) == 0);
 	if (mpp->prkey_source == PRKEY_SOURCE_FILE &&
 	    (rq_servact == MPATH_PROUT_REG_IGN_SA ||
@@ -832,7 +834,6 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	       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)) {
@@ -895,7 +896,7 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	put_multipath_config(conf);
 
 	if (rq_servact == MPATH_PROUT_REG_IGN_SA)
-		set_ignored_key(mpp, paramp->key);
+		set_ignored_key(mpp, (uint8_t *)&oldkey, paramp->key);
 
 	switch(rq_servact)
 	{
-- 
2.50.1

[PATCH 2/4] libmpathpersist: Handle RESERVE with reservation held by failed path

[Benjamin Marzinski]

Issuing a RESERVE on a device that already holds the reservation should
succeed, as long as the type is the same. But if the path that holds the
reservation is unavailable, mpathpersist fails, since it gets a
reservation conflict on all available paths. To deal with this, if the
multipath device has failed paths, and the key holding the reservation
matches the multipath device's key, and multipathd says that it is
holding the reservation, assume the reservation is held by a failed path
and claim the RESERVE succeeded, even though none of the actual scsi
commands did

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

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 476c3433..33c9e57a 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -200,7 +200,7 @@ static void *mpath_prout_pthread_fn(void *p)
 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 noisy, struct path **pptr, bool *failed_paths)
 {
 	int i, j, ret;
 	struct pathgroup *pgp = NULL;
@@ -213,6 +213,8 @@ mpath_prout_common(struct multipath *mpp, int rq_servact, int rq_scope,
 			if (!((pp->state == PATH_UP) || (pp->state == PATH_GHOST))) {
 				condlog(1, "%s: %s path not up. Skip",
 					mpp->wwid, pp->dev);
+				if (failed_paths)
+					*failed_paths = true;
 				continue;
 			}
 
@@ -247,6 +249,8 @@ mpath_prout_common(struct multipath *mpp, int rq_servact, int rq_scope,
 			}
 			if (ret != MPATH_PR_RETRYABLE_ERROR)
 				return ret;
+			if (failed_paths)
+				*failed_paths = true;
 		}
 	}
 	if (found)
@@ -333,7 +337,7 @@ void preempt_missing_path(struct multipath *mpp, uint8_t *key, uint8_t *sa_key,
 	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);
+				    rq_type, &paramp, noisy, NULL, NULL);
 	if (status != MPATH_PR_SUCCESS)
 		condlog(0, "%s: register: pr preempt command failed.", mpp->wwid);
 }
@@ -586,7 +590,7 @@ static int do_preempt_self(struct multipath *mpp, struct be64 sa_key,
 	memcpy(paramp.key, &mpp->reservation_key, 8);
 	memcpy(paramp.sa_key, &sa_key, 8);
 	status = mpath_prout_common(mpp, rq_servact, rq_scope, rq_type,
-				    &paramp, noisy, &pp);
+				    &paramp, noisy, &pp, NULL);
 	if (status != MPATH_PR_SUCCESS) {
 		condlog(0, "%s: self preempt command failed.", mpp->wwid);
 		goto fail_resume;
@@ -769,20 +773,25 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 	return preempt_self(mpp, MPATH_PROUT_PREE_SA, rq_scope, rq_type, noisy, true);
 }
 
-static int reservation_key_matches(struct multipath *mpp, uint8_t *key, int noisy)
+static int reservation_key_matches(struct multipath *mpp, uint8_t *key,
+				   unsigned int *type)
 {
 	struct prin_resp resp = {{{.prgeneration = 0}}};
 	int status;
 
-	status = mpath_prin_activepath(mpp, MPATH_PRIN_RRES_SA, &resp, noisy);
+	status = mpath_prin_activepath(mpp, MPATH_PRIN_RRES_SA, &resp, 0);
 	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)
+	if (memcmp(key, resp.prin_descriptor.prin_readresv.key, 8) == 0) {
+		if (type)
+			*type = resp.prin_descriptor.prin_readresv.scope_type &
+				MPATH_PR_TYPE_MASK;
 		return YNU_YES;
+	}
 	return YNU_NO;
 }
 
@@ -799,11 +808,20 @@ static void set_ignored_key(struct multipath *mpp, uint8_t *curr_key,
 		return;
 	if (get_prhold(mpp->alias) == PR_UNSET)
 		return;
-	if (reservation_key_matches(mpp, curr_key, 0) == YNU_NO)
+	if (reservation_key_matches(mpp, curr_key, NULL) == YNU_NO)
 		return;
 	memcpy(key, curr_key, 8);
 }
 
+static bool check_holding_reservation(struct multipath *mpp, unsigned int *type)
+{
+	if (get_be64(mpp->reservation_key) &&
+	    get_prhold(mpp->alias) == PR_SET &&
+	    reservation_key_matches(mpp, (uint8_t *)&mpp->reservation_key, type) == YNU_YES)
+		return true;
+	return false;
+}
+
 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)
@@ -815,6 +833,7 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	struct config *conf;
 	bool unregistering, preempting_reservation = false;
 	bool updated_prkey = false;
+	bool failed_paths = false;
 
 	ret = mpath_get_map(curmp, fd, &mpp);
 	if (ret != MPATH_PR_SUCCESS)
@@ -906,7 +925,7 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 		break;
 	case MPATH_PROUT_PREE_SA:
 	case MPATH_PROUT_PREE_AB_SA:
-		if (reservation_key_matches(mpp, paramp->sa_key, noisy) == YNU_YES) {
+		if (reservation_key_matches(mpp, paramp->sa_key, NULL) == YNU_YES) {
 			preempting_reservation = true;
 			if (memcmp(paramp->sa_key, &zerokey, 8) == 0) {
 				/* all registrants case */
@@ -923,10 +942,18 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 		}
 		/* fallthrough */
 	case MPATH_PROUT_RES_SA:
-	case MPATH_PROUT_CLEAR_SA:
+	case MPATH_PROUT_CLEAR_SA: {
+		unsigned int res_type;
 		ret = mpath_prout_common(mpp, rq_servact, rq_scope, rq_type,
-					 paramp, noisy, NULL);
+					 paramp, noisy, NULL, &failed_paths);
+		if (rq_servact == MPATH_PROUT_RES_SA &&
+		    ret != MPATH_PR_SUCCESS && failed_paths &&
+		    check_holding_reservation(mpp, &res_type) &&
+		    res_type == rq_type)
+			/* The reserve failed, but multipathd says we hold it */
+			ret = MPATH_PR_SUCCESS;
 		break;
+	}
 	case MPATH_PROUT_REL_SA:
 		ret = mpath_prout_rel(mpp, rq_servact, rq_scope, rq_type, paramp, noisy);
 		break;
-- 
2.50.1

Re: [PATCH 2/4] libmpathpersist: Handle RESERVE with reservation held by failed path

[Martin Wilck]

On Tue, 2025-09-23 at 18:12 -0400, Benjamin Marzinski wrote:
> Issuing a RESERVE on a device that already holds the reservation
> should
> succeed, as long as the type is the same. But if the path that holds
> the
> reservation is unavailable, mpathpersist fails, since it gets a
> reservation conflict on all available paths. To deal with this, if
> the
> multipath device has failed paths, and the key holding the
> reservation
> matches the multipath device's key, and multipathd says that it is
> holding the reservation, assume the reservation is held by a failed
> path
> and claim the RESERVE succeeded, even though none of the actual scsi
> commands did

Can we really trust multipathd? What if the PR had been preemtped by
another host?

> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmpathpersist/mpath_persist_int.c | 47 +++++++++++++++++++++++----
> --
>  1 file changed, 37 insertions(+), 10 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist_int.c
> b/libmpathpersist/mpath_persist_int.c
> index 476c3433..33c9e57a 100644
> --- a/libmpathpersist/mpath_persist_int.c
> +++ b/libmpathpersist/mpath_persist_int.c
> @@ -200,7 +200,7 @@ static void *mpath_prout_pthread_fn(void *p)
>  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 noisy, struct path **pptr, bool
> *failed_paths)
>  {
>  	int i, j, ret;
>  	struct pathgroup *pgp = NULL;
> @@ -213,6 +213,8 @@ mpath_prout_common(struct multipath *mpp, int
> rq_servact, int rq_scope,
>  			if (!((pp->state == PATH_UP) || (pp->state
> == PATH_GHOST))) {
>  				condlog(1, "%s: %s path not up.
> Skip",
>  					mpp->wwid, pp->dev);
> +				if (failed_paths)
> +					*failed_paths = true;
>  				continue;
>  			}
>  
> @@ -247,6 +249,8 @@ mpath_prout_common(struct multipath *mpp, int
> rq_servact, int rq_scope,
>  			}
>  			if (ret != MPATH_PR_RETRYABLE_ERROR)
>  				return ret;
> +			if (failed_paths)
> +				*failed_paths = true;
>  		}
>  	}
>  	if (found)
> @@ -333,7 +337,7 @@ void preempt_missing_path(struct multipath *mpp,
> uint8_t *key, uint8_t *sa_key,
>  	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);
> +				    rq_type, &paramp, noisy, NULL,
> NULL);
>  	if (status != MPATH_PR_SUCCESS)
>  		condlog(0, "%s: register: pr preempt command
> failed.", mpp->wwid);
>  }
> @@ -586,7 +590,7 @@ static int do_preempt_self(struct multipath *mpp,
> struct be64 sa_key,
>  	memcpy(paramp.key, &mpp->reservation_key, 8);
>  	memcpy(paramp.sa_key, &sa_key, 8);
>  	status = mpath_prout_common(mpp, rq_servact, rq_scope,
> rq_type,
> -				    &paramp, noisy, &pp);
> +				    &paramp, noisy, &pp, NULL);
>  	if (status != MPATH_PR_SUCCESS) {
>  		condlog(0, "%s: self preempt command failed.", mpp-
> >wwid);
>  		goto fail_resume;
> @@ -769,20 +773,25 @@ static int mpath_prout_rel(struct multipath
> *mpp,int rq_servact, int rq_scope,
>  	return preempt_self(mpp, MPATH_PROUT_PREE_SA, rq_scope,
> rq_type, noisy, true);
>  }
>  
> -static int reservation_key_matches(struct multipath *mpp, uint8_t
> *key, int noisy)
> +static int reservation_key_matches(struct multipath *mpp, uint8_t
> *key,
> +				   unsigned int *type)
>  {
>  	struct prin_resp resp = {{{.prgeneration = 0}}};
>  	int status;
>  
> -	status = mpath_prin_activepath(mpp, MPATH_PRIN_RRES_SA,
> &resp, noisy);
> +	status = mpath_prin_activepath(mpp, MPATH_PRIN_RRES_SA,
> &resp, 0);
>  	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)
> +	if (memcmp(key, resp.prin_descriptor.prin_readresv.key, 8)
> == 0) {
> +		if (type)
> +			*type =
> resp.prin_descriptor.prin_readresv.scope_type &
> +				MPATH_PR_TYPE_MASK;
>  		return YNU_YES;
> +	}
>  	return YNU_NO;
>  }
>  
> @@ -799,11 +808,20 @@ static void set_ignored_key(struct multipath
> *mpp, uint8_t *curr_key,
>  		return;
>  	if (get_prhold(mpp->alias) == PR_UNSET)
>  		return;
> -	if (reservation_key_matches(mpp, curr_key, 0) == YNU_NO)
> +	if (reservation_key_matches(mpp, curr_key, NULL) == YNU_NO)
>  		return;
>  	memcpy(key, curr_key, 8);
>  }
>  
> +static bool check_holding_reservation(struct multipath *mpp,
> unsigned int *type)
> +{
> +	if (get_be64(mpp->reservation_key) &&
> +	    get_prhold(mpp->alias) == PR_SET &&
> +	    reservation_key_matches(mpp, (uint8_t *)&mpp-
> >reservation_key, type) == YNU_YES)
> +		return true;
> +	return false;
> +}
> +
>  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)
> @@ -815,6 +833,7 @@ int do_mpath_persistent_reserve_out(vector curmp,
> vector pathvec, int fd,
>  	struct config *conf;
>  	bool unregistering, preempting_reservation = false;
>  	bool updated_prkey = false;
> +	bool failed_paths = false;
>  
>  	ret = mpath_get_map(curmp, fd, &mpp);
>  	if (ret != MPATH_PR_SUCCESS)
> @@ -906,7 +925,7 @@ int do_mpath_persistent_reserve_out(vector curmp,
> vector pathvec, int fd,
>  		break;
>  	case MPATH_PROUT_PREE_SA:
>  	case MPATH_PROUT_PREE_AB_SA:
> -		if (reservation_key_matches(mpp, paramp->sa_key,
> noisy) == YNU_YES) {
> +		if (reservation_key_matches(mpp, paramp->sa_key,
> NULL) == YNU_YES) {
>  			preempting_reservation = true;
>  			if (memcmp(paramp->sa_key, &zerokey, 8) ==
> 0) {
>  				/* all registrants case */
> @@ -923,10 +942,18 @@ int do_mpath_persistent_reserve_out(vector
> curmp, vector pathvec, int fd,
>  		}
>  		/* fallthrough */
>  	case MPATH_PROUT_RES_SA:
> -	case MPATH_PROUT_CLEAR_SA:
> +	case MPATH_PROUT_CLEAR_SA: {
> +		unsigned int res_type;
>  		ret = mpath_prout_common(mpp, rq_servact, rq_scope,
> rq_type,
> -					 paramp, noisy, NULL);
> +					 paramp, noisy, NULL,
> &failed_paths);
> +		if (rq_servact == MPATH_PROUT_RES_SA &&
> +		    ret != MPATH_PR_SUCCESS && failed_paths &&
> +		    check_holding_reservation(mpp, &res_type) &&
> +		    res_type == rq_type)
> +			/* The reserve failed, but multipathd says
> we hold it */
> +			ret = MPATH_PR_SUCCESS;

I'd prefer splitting up the case statement here, as the code for
MPATH_PROUT_RES_SA is now pretty different from the other cases.

Martin

Re: [PATCH 2/4] libmpathpersist: Handle RESERVE with reservation held by failed path

[Benjamin Marzinski]

On Wed, Sep 24, 2025 at 08:02:41PM +0200, Martin Wilck wrote:
> On Tue, 2025-09-23 at 18:12 -0400, Benjamin Marzinski wrote:
> > Issuing a RESERVE on a device that already holds the reservation
> > should
> > succeed, as long as the type is the same. But if the path that holds
> > the
> > reservation is unavailable, mpathpersist fails, since it gets a
> > reservation conflict on all available paths. To deal with this, if
> > the
> > multipath device has failed paths, and the key holding the
> > reservation
> > matches the multipath device's key, and multipathd says that it is
> > holding the reservation, assume the reservation is held by a failed
> > path
> > and claim the RESERVE succeeded, even though none of the actual scsi
> > commands did
> 
> Can we really trust multipathd? What if the PR had been preemtped by
> another host?

We also check that key that multipath is using for this device is that
key that is currently holding the reservation (by issuing a
READ RESERVATION command).

So we know that the key that we are using to grab the reservation is the
key for this multipath device. The scsi device also says that this is
the key that is currently holding the reservation. And multipathd says
that the device should be holding the reservation. The multipathd check
is just a safety check to deal with the remote possiblity that another
node is also using the same key.

-Ben
 
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmpathpersist/mpath_persist_int.c | 47 +++++++++++++++++++++++----
> > --
> >  1 file changed, 37 insertions(+), 10 deletions(-)
> > 
> > diff --git a/libmpathpersist/mpath_persist_int.c
> > b/libmpathpersist/mpath_persist_int.c
> > index 476c3433..33c9e57a 100644
> > --- a/libmpathpersist/mpath_persist_int.c
> > +++ b/libmpathpersist/mpath_persist_int.c
> > @@ -200,7 +200,7 @@ static void *mpath_prout_pthread_fn(void *p)
> >  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 noisy, struct path **pptr, bool
> > *failed_paths)
> >  {
> >  	int i, j, ret;
> >  	struct pathgroup *pgp = NULL;
> > @@ -213,6 +213,8 @@ mpath_prout_common(struct multipath *mpp, int
> > rq_servact, int rq_scope,
> >  			if (!((pp->state == PATH_UP) || (pp->state
> > == PATH_GHOST))) {
> >  				condlog(1, "%s: %s path not up.
> > Skip",
> >  					mpp->wwid, pp->dev);
> > +				if (failed_paths)
> > +					*failed_paths = true;
> >  				continue;
> >  			}
> >  
> > @@ -247,6 +249,8 @@ mpath_prout_common(struct multipath *mpp, int
> > rq_servact, int rq_scope,
> >  			}
> >  			if (ret != MPATH_PR_RETRYABLE_ERROR)
> >  				return ret;
> > +			if (failed_paths)
> > +				*failed_paths = true;
> >  		}
> >  	}
> >  	if (found)
> > @@ -333,7 +337,7 @@ void preempt_missing_path(struct multipath *mpp,
> > uint8_t *key, uint8_t *sa_key,
> >  	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);
> > +				    rq_type, &paramp, noisy, NULL,
> > NULL);
> >  	if (status != MPATH_PR_SUCCESS)
> >  		condlog(0, "%s: register: pr preempt command
> > failed.", mpp->wwid);
> >  }
> > @@ -586,7 +590,7 @@ static int do_preempt_self(struct multipath *mpp,
> > struct be64 sa_key,
> >  	memcpy(paramp.key, &mpp->reservation_key, 8);
> >  	memcpy(paramp.sa_key, &sa_key, 8);
> >  	status = mpath_prout_common(mpp, rq_servact, rq_scope,
> > rq_type,
> > -				    &paramp, noisy, &pp);
> > +				    &paramp, noisy, &pp, NULL);
> >  	if (status != MPATH_PR_SUCCESS) {
> >  		condlog(0, "%s: self preempt command failed.", mpp-
> > >wwid);
> >  		goto fail_resume;
> > @@ -769,20 +773,25 @@ static int mpath_prout_rel(struct multipath
> > *mpp,int rq_servact, int rq_scope,
> >  	return preempt_self(mpp, MPATH_PROUT_PREE_SA, rq_scope,
> > rq_type, noisy, true);
> >  }
> >  
> > -static int reservation_key_matches(struct multipath *mpp, uint8_t
> > *key, int noisy)
> > +static int reservation_key_matches(struct multipath *mpp, uint8_t
> > *key,
> > +				   unsigned int *type)
> >  {
> >  	struct prin_resp resp = {{{.prgeneration = 0}}};
> >  	int status;
> >  
> > -	status = mpath_prin_activepath(mpp, MPATH_PRIN_RRES_SA,
> > &resp, noisy);
> > +	status = mpath_prin_activepath(mpp, MPATH_PRIN_RRES_SA,
> > &resp, 0);
> >  	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)
> > +	if (memcmp(key, resp.prin_descriptor.prin_readresv.key, 8)
> > == 0) {
> > +		if (type)
> > +			*type =
> > resp.prin_descriptor.prin_readresv.scope_type &
> > +				MPATH_PR_TYPE_MASK;
> >  		return YNU_YES;
> > +	}
> >  	return YNU_NO;
> >  }
> >  
> > @@ -799,11 +808,20 @@ static void set_ignored_key(struct multipath
> > *mpp, uint8_t *curr_key,
> >  		return;
> >  	if (get_prhold(mpp->alias) == PR_UNSET)
> >  		return;
> > -	if (reservation_key_matches(mpp, curr_key, 0) == YNU_NO)
> > +	if (reservation_key_matches(mpp, curr_key, NULL) == YNU_NO)
> >  		return;
> >  	memcpy(key, curr_key, 8);
> >  }
> >  
> > +static bool check_holding_reservation(struct multipath *mpp,
> > unsigned int *type)
> > +{
> > +	if (get_be64(mpp->reservation_key) &&
> > +	    get_prhold(mpp->alias) == PR_SET &&
> > +	    reservation_key_matches(mpp, (uint8_t *)&mpp-
> > >reservation_key, type) == YNU_YES)
> > +		return true;
> > +	return false;
> > +}
> > +
> >  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)
> > @@ -815,6 +833,7 @@ int do_mpath_persistent_reserve_out(vector curmp,
> > vector pathvec, int fd,
> >  	struct config *conf;
> >  	bool unregistering, preempting_reservation = false;
> >  	bool updated_prkey = false;
> > +	bool failed_paths = false;
> >  
> >  	ret = mpath_get_map(curmp, fd, &mpp);
> >  	if (ret != MPATH_PR_SUCCESS)
> > @@ -906,7 +925,7 @@ int do_mpath_persistent_reserve_out(vector curmp,
> > vector pathvec, int fd,
> >  		break;
> >  	case MPATH_PROUT_PREE_SA:
> >  	case MPATH_PROUT_PREE_AB_SA:
> > -		if (reservation_key_matches(mpp, paramp->sa_key,
> > noisy) == YNU_YES) {
> > +		if (reservation_key_matches(mpp, paramp->sa_key,
> > NULL) == YNU_YES) {
> >  			preempting_reservation = true;
> >  			if (memcmp(paramp->sa_key, &zerokey, 8) ==
> > 0) {
> >  				/* all registrants case */
> > @@ -923,10 +942,18 @@ int do_mpath_persistent_reserve_out(vector
> > curmp, vector pathvec, int fd,
> >  		}
> >  		/* fallthrough */
> >  	case MPATH_PROUT_RES_SA:
> > -	case MPATH_PROUT_CLEAR_SA:
> > +	case MPATH_PROUT_CLEAR_SA: {
> > +		unsigned int res_type;
> >  		ret = mpath_prout_common(mpp, rq_servact, rq_scope,
> > rq_type,
> > -					 paramp, noisy, NULL);
> > +					 paramp, noisy, NULL,
> > &failed_paths);
> > +		if (rq_servact == MPATH_PROUT_RES_SA &&
> > +		    ret != MPATH_PR_SUCCESS && failed_paths &&
> > +		    check_holding_reservation(mpp, &res_type) &&
> > +		    res_type == rq_type)
> > +			/* The reserve failed, but multipathd says
> > we hold it */
> > +			ret = MPATH_PR_SUCCESS;
> 
> I'd prefer splitting up the case statement here, as the code for
> MPATH_PROUT_RES_SA is now pretty different from the other cases.
> 
> Martin

[PATCH 3/4] libmpathpersist: use check_holding_reservation in mpath_prout_rel

[Benjamin Marzinski]

Instead of open-coding mostly the same work, just make mpath_prout_rel()
call check_holding_reservation().

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

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 33c9e57a..9c71011f 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -639,12 +639,42 @@ static int preempt_all(struct multipath *mpp, int rq_servact, int rq_scope,
 			       noisy, false);
 }
 
+static int reservation_key_matches(struct multipath *mpp, uint8_t *key,
+				   unsigned int *type)
+{
+	struct prin_resp resp = {{{.prgeneration = 0}}};
+	int status;
+
+	status = mpath_prin_activepath(mpp, MPATH_PRIN_RRES_SA, &resp, 0);
+	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) {
+		if (type)
+			*type = resp.prin_descriptor.prin_readresv.scope_type &
+				MPATH_PR_TYPE_MASK;
+		return YNU_YES;
+	}
+	return YNU_NO;
+}
+
+static bool check_holding_reservation(struct multipath *mpp, unsigned int *type)
+{
+	if (get_be64(mpp->reservation_key) &&
+	    get_prhold(mpp->alias) == PR_SET &&
+	    reservation_key_matches(mpp, (uint8_t *)&mpp->reservation_key, type) == YNU_YES)
+		return true;
+	return false;
+}
+
 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)
 {
 	int i, j;
-	int num = 0;
 	struct pathgroup *pgp = NULL;
 	struct path *pp = NULL;
 	int active_pathcount = 0;
@@ -652,9 +682,8 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 	int rc;
 	int count = 0;
 	int status = MPATH_PR_SUCCESS;
-	struct prin_resp resp = {{{.prgeneration = 0}}};
 	bool all_threads_failed;
-	unsigned int scope_type;
+	unsigned int res_type;
 
 	if (!mpp)
 		return MPATH_PR_DMMP_ERROR;
@@ -733,27 +762,13 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 		return 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 MPATH_PR_OTHER;
-	}
-
-	num = resp.prin_descriptor.prin_readresv.additional_length / 8;
-	if (num == 0){
-		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)) {
+	if (!check_holding_reservation(mpp, &res_type)) {
 		condlog(2, "%s: Releasing key not holding reservation.", mpp->wwid);
 		return MPATH_PR_SUCCESS;
 	}
-
-	scope_type = resp.prin_descriptor.prin_readresv.scope_type;
-	if ((scope_type & MPATH_PR_TYPE_MASK) != rq_type) {
+	if (res_type != rq_type) {
 		condlog(2, "%s: --prout_type %u doesn't match reservation %u",
-			mpp->wwid, rq_type, scope_type & MPATH_PR_TYPE_MASK);
+			mpp->wwid, rq_type, res_type);
 		return MPATH_PR_RESERV_CONFLICT;
 	}
 
@@ -773,28 +788,6 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 	return preempt_self(mpp, MPATH_PROUT_PREE_SA, rq_scope, rq_type, noisy, true);
 }
 
-static int reservation_key_matches(struct multipath *mpp, uint8_t *key,
-				   unsigned int *type)
-{
-	struct prin_resp resp = {{{.prgeneration = 0}}};
-	int status;
-
-	status = mpath_prin_activepath(mpp, MPATH_PRIN_RRES_SA, &resp, 0);
-	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) {
-		if (type)
-			*type = resp.prin_descriptor.prin_readresv.scope_type &
-				MPATH_PR_TYPE_MASK;
-		return YNU_YES;
-	}
-	return YNU_NO;
-}
-
 /*
  * for MPATH_PROUT_REG_IGN_SA, we use the ignored paramp->key to store the
  * currently registered key for use in preempt_missing_path(), but only if
@@ -813,15 +806,6 @@ static void set_ignored_key(struct multipath *mpp, uint8_t *curr_key,
 	memcpy(key, curr_key, 8);
 }
 
-static bool check_holding_reservation(struct multipath *mpp, unsigned int *type)
-{
-	if (get_be64(mpp->reservation_key) &&
-	    get_prhold(mpp->alias) == PR_SET &&
-	    reservation_key_matches(mpp, (uint8_t *)&mpp->reservation_key, type) == YNU_YES)
-		return true;
-	return false;
-}
-
 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)
-- 
2.50.1

Re: [PATCH 3/4] libmpathpersist: use check_holding_reservation in mpath_prout_rel

[Martin Wilck]

On Tue, 2025-09-23 at 18:12 -0400, Benjamin Marzinski wrote:
> Instead of open-coding mostly the same work, just make
> mpath_prout_rel()
> call check_holding_reservation().
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmpathpersist/mpath_persist_int.c | 86 ++++++++++++---------------
> --
>  1 file changed, 35 insertions(+), 51 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist_int.c
> b/libmpathpersist/mpath_persist_int.c
> index 33c9e57a..9c71011f 100644
> --- a/libmpathpersist/mpath_persist_int.c
> +++ b/libmpathpersist/mpath_persist_int.c
> @@ -639,12 +639,42 @@ static int preempt_all(struct multipath *mpp,
> int rq_servact, int rq_scope,
>  			       noisy, false);
>  }
>  
> +static int reservation_key_matches(struct multipath *mpp, uint8_t
> *key,
> +				   unsigned int *type)
> +{
> +	struct prin_resp resp = {{{.prgeneration = 0}}};
> +	int status;
> +
> +	status = mpath_prin_activepath(mpp, MPATH_PRIN_RRES_SA,
> &resp, 0);
> +	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) {
> +		if (type)
> +			*type =
> resp.prin_descriptor.prin_readresv.scope_type &
> +				MPATH_PR_TYPE_MASK;
> +		return YNU_YES;
> +	}
> +	return YNU_NO;
> +}
> +
> +static bool check_holding_reservation(struct multipath *mpp,
> unsigned int *type)
> +{
> +	if (get_be64(mpp->reservation_key) &&
> +	    get_prhold(mpp->alias) == PR_SET &&
> +	    reservation_key_matches(mpp, (uint8_t *)&mpp-
> >reservation_key, type) == YNU_YES)
> +		return true;
> +	return false;
> +}
> +
>  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)
>  {
>  	int i, j;
> -	int num = 0;
>  	struct pathgroup *pgp = NULL;
>  	struct path *pp = NULL;
>  	int active_pathcount = 0;
> @@ -652,9 +682,8 @@ static int mpath_prout_rel(struct multipath
> *mpp,int rq_servact, int rq_scope,
>  	int rc;
>  	int count = 0;
>  	int status = MPATH_PR_SUCCESS;
> -	struct prin_resp resp = {{{.prgeneration = 0}}};
>  	bool all_threads_failed;
> -	unsigned int scope_type;
> +	unsigned int res_type;
>  
>  	if (!mpp)
>  		return MPATH_PR_DMMP_ERROR;
> @@ -733,27 +762,13 @@ static int mpath_prout_rel(struct multipath
> *mpp,int rq_servact, int rq_scope,
>  		return 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 MPATH_PR_OTHER;
> -	}
> -
> -	num = resp.prin_descriptor.prin_readresv.additional_length /
> 8;
> -	if (num == 0){
> -		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)) {
> +	if (!check_holding_reservation(mpp, &res_type)) {

The old code didn't call check_prhold() while
check_holding_reservation() does.

Is it possible that get_prhold() would return a "false negative" and
thus we'd wrongly skip releasing the reservation?

Martin

Re: [PATCH 3/4] libmpathpersist: use check_holding_reservation in mpath_prout_rel

[Benjamin Marzinski]

On Wed, Sep 24, 2025 at 08:37:36PM +0200, Martin Wilck wrote:
> On Tue, 2025-09-23 at 18:12 -0400, Benjamin Marzinski wrote:
> > Instead of open-coding mostly the same work, just make
> > mpath_prout_rel()
> > call check_holding_reservation().
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmpathpersist/mpath_persist_int.c | 86 ++++++++++++---------------
> > --
> >  1 file changed, 35 insertions(+), 51 deletions(-)
> > 
> > diff --git a/libmpathpersist/mpath_persist_int.c
> > b/libmpathpersist/mpath_persist_int.c
> > index 33c9e57a..9c71011f 100644
> > --- a/libmpathpersist/mpath_persist_int.c
> > +++ b/libmpathpersist/mpath_persist_int.c
> > @@ -639,12 +639,42 @@ static int preempt_all(struct multipath *mpp,
> > int rq_servact, int rq_scope,
> >  			       noisy, false);
> >  }
> >  
> > +static int reservation_key_matches(struct multipath *mpp, uint8_t
> > *key,
> > +				   unsigned int *type)
> > +{
> > +	struct prin_resp resp = {{{.prgeneration = 0}}};
> > +	int status;
> > +
> > +	status = mpath_prin_activepath(mpp, MPATH_PRIN_RRES_SA,
> > &resp, 0);
> > +	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) {
> > +		if (type)
> > +			*type =
> > resp.prin_descriptor.prin_readresv.scope_type &
> > +				MPATH_PR_TYPE_MASK;
> > +		return YNU_YES;
> > +	}
> > +	return YNU_NO;
> > +}
> > +
> > +static bool check_holding_reservation(struct multipath *mpp,
> > unsigned int *type)
> > +{
> > +	if (get_be64(mpp->reservation_key) &&
> > +	    get_prhold(mpp->alias) == PR_SET &&
> > +	    reservation_key_matches(mpp, (uint8_t *)&mpp-
> > >reservation_key, type) == YNU_YES)
> > +		return true;
> > +	return false;
> > +}
> > +
> >  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)
> >  {
> >  	int i, j;
> > -	int num = 0;
> >  	struct pathgroup *pgp = NULL;
> >  	struct path *pp = NULL;
> >  	int active_pathcount = 0;
> > @@ -652,9 +682,8 @@ static int mpath_prout_rel(struct multipath
> > *mpp,int rq_servact, int rq_scope,
> >  	int rc;
> >  	int count = 0;
> >  	int status = MPATH_PR_SUCCESS;
> > -	struct prin_resp resp = {{{.prgeneration = 0}}};
> >  	bool all_threads_failed;
> > -	unsigned int scope_type;
> > +	unsigned int res_type;
> >  
> >  	if (!mpp)
> >  		return MPATH_PR_DMMP_ERROR;
> > @@ -733,27 +762,13 @@ static int mpath_prout_rel(struct multipath
> > *mpp,int rq_servact, int rq_scope,
> >  		return 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 MPATH_PR_OTHER;
> > -	}
> > -
> > -	num = resp.prin_descriptor.prin_readresv.additional_length /
> > 8;
> > -	if (num == 0){
> > -		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)) {
> > +	if (!check_holding_reservation(mpp, &res_type)) {
> 
> The old code didn't call check_prhold() while
> check_holding_reservation() does.
> 
> Is it possible that get_prhold() would return a "false negative" and
> thus we'd wrongly skip releasing the reservation?

At the point where we check this, we've already done the RELEASE and
gotten a success, but the reservation is still held. This means that
either an unavailable path on this device is holding it, or another
device that is using the same key is holding it. These checks are to
determine whether or not we PREEMPT the key to force-clear the
reservation. What we are adding here is a requirement that multipathd
says we are holding the reservation. 

I'm not sure how we would get a false negative. When multipathd first
starts tracking a device (when it starts up, creates a new device, or is
reconfigured) it assumes that there aren't multiple devices holding the
same key. So if it sees that its configured key is holding the
reservation, it assumes that this device is holding the reservation. It
will only unset the holding state if it notices that the devices key is
not registered, or it is told to drop the reservation. Once it is
started the only way it will set the holding state on a device is when
libmpathpersist tells it to, but that should always happen when
libmpathpersist grabs a reservation.

Now, this code is admittedly complex, and there are lots of corner cases
that I might not have thought through which could produce a false
negative, and it is always possible that libmpathpersist fails to
communicate with multipathd to set prhold.  Also, I don't think we really
need to worry much about another device holding a duplicate key when we
are explicitly asked to release the reservation (that means whoever is
issuing this command also thinks we own the device).

So I'm fine with backing this change out. I was on the fence about it
myself. If I do, check_holding_reservation() and
reservation_key_matches() will still need to be moved earlier in the
code for the next patch. That patch also makes changes to
check_holding_reservation(). Do you prefer moving the functions in a
separate patch from editting them. Or are you fine with doing both in
the same patch, as long as I call it out.

-Ben
 
> Martin

Re: [PATCH 3/4] libmpathpersist: use check_holding_reservation in mpath_prout_rel

[Martin Wilck]

On Thu, 2025-09-25 at 15:38 -0400, Benjamin Marzinski wrote:
> On Wed, Sep 24, 2025 at 08:37:36PM +0200, Martin Wilck wrote:
> > On Tue, 2025-09-23 at 18:12 -0400, Benjamin Marzinski wrote:
> > > -
> > > -	num =
> > > resp.prin_descriptor.prin_readresv.additional_length /
> > > 8;
> > > -	if (num == 0){
> > > -		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)) {
> > > +	if (!check_holding_reservation(mpp, &res_type)) {
> > 
> > The old code didn't call check_prhold() while
> > check_holding_reservation() does.
> > 
> > Is it possible that get_prhold() would return a "false negative"
> > and
> > thus we'd wrongly skip releasing the reservation?
> 
> At the point where we check this, we've already done the RELEASE and
> gotten a success, but the reservation is still held. This means that
> either an unavailable path on this device is holding it, or another
> device that is using the same key is holding it. These checks are to
> determine whether or not we PREEMPT the key to force-clear the
> reservation. What we are adding here is a requirement that multipathd
> says we are holding the reservation. 
> 
> I'm not sure how we would get a false negative. When multipathd first
> starts tracking a device (when it starts up, creates a new device, or
> is
> reconfigured) it assumes that there aren't multiple devices holding
> the
> same key. So if it sees that its configured key is holding the
> reservation, it assumes that this device is holding the reservation.
> It
> will only unset the holding state if it notices that the devices key
> is
> not registered, or it is told to drop the reservation. Once it is
> started the only way it will set the holding state on a device is
> when
> libmpathpersist tells it to, but that should always happen when
> libmpathpersist grabs a reservation.
> 
> Now, this code is admittedly complex, and there are lots of corner
> cases
> that I might not have thought through which could produce a false
> negative, and it is always possible that libmpathpersist fails to
> communicate with multipathd to set prhold.  Also, I don't think we
> really
> need to worry much about another device holding a duplicate key when
> we
> are explicitly asked to release the reservation (that means whoever
> is
> issuing this command also thinks we own the device).

> So I'm fine with backing this change out.

That was not my intention. I just notived the change andwanted to get 
confirmation from you that this was well thought-out. Which you have
given in your response.

>  I was on the fence about it
> myself. If I do, check_holding_reservation() and
> reservation_key_matches() will still need to be moved earlier in the
> code for the next patch. That patch also makes changes to
> check_holding_reservation(). Do you prefer moving the functions in a
> separate patch from editting them. Or are you fine with doing both in
> the same patch, as long as I call it out.

You can leave it as-is.

Regards
Martin

[PATCH 4/4] libmpathpersist: Fix unregistering while holding the reservation

[Benjamin Marzinski]

There were two problems with how libmpathpersist handled unregistering
a key while holding the reseravation (which should also release the
reservation).
1. If the path holding the reservation is not unregistered first, there
   will be unregistered paths, while a reservation is still held, which
   would cause IO to those paths to fail, when it shouldn't.
2. If the path that holds the reservation is down, libmpathpersist was
   not clearing the resrvation, since the there were no registered keys
   it could use for the PREEMPT command workaround

To fix these, libmpathpersist now releases the reservation first when
trying to unregister a key that is holding the reservation.
mpath_prout_rel() has a new option so that if it needs to self preempt
to clear the reservation, it won't re-register the paths when called
as part of unregistering a key. Also, instead of checking if the device
is currently holding a reservation using mpp->reservation_key in
check_holding_reservation() (which will already be set to 0 when called
as part of unregistering a key), pass in the key to check.

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

diff --git a/libmpathpersist/mpath_persist_int.c b/libmpathpersist/mpath_persist_int.c
index 9c71011f..f130f5f5 100644
--- a/libmpathpersist/mpath_persist_int.c
+++ b/libmpathpersist/mpath_persist_int.c
@@ -564,18 +564,23 @@ static int mpath_prout_reg(struct multipath *mpp,int rq_servact, int rq_scope,
 	return status;
 }
 
+enum preempt_work {
+	PREE_WORK_NONE,
+	PREE_WORK_REL,
+	PREE_WORK_REL_UNREG,
+};
 /*
  * Called to make a multipath device preempt its own reservation (and
- * optionally release the reservation). Doing this causes the reservation
- * keys to be removed from all the device paths except that path used to issue
- * the preempt, so they need to be restored. To avoid the chance that IO
- * goes to these paths when they don't have a registered key, the device
- * is suspended before issuing the preemption, and the keys are reregistered
- * before resuming it.
+ * optional extra work). Doing this causes the reservation keys to be removed
+ * from all the device paths except that path used to issue the preempt, so
+ * they may need to be restored. To avoid the chance that IO goes to these
+ * paths when they don't have a registered key and a reservation exists, the
+ * device is suspended before issuing the preemption, and the keys are
+ * reregistered (or the reservation is released) before resuming it.
  */
 static int do_preempt_self(struct multipath *mpp, struct be64 sa_key,
 			   int rq_servact, int rq_scope, unsigned int rq_type,
-			   int noisy, bool do_release)
+			   int noisy, enum preempt_work work)
 {
 	int status, rel_status = MPATH_PR_SUCCESS;
 	struct path *pp = NULL;
@@ -596,7 +601,7 @@ static int do_preempt_self(struct multipath *mpp, struct be64 sa_key,
 		goto fail_resume;
 	}
 
-	if (do_release) {
+	if (work != PREE_WORK_NONE) {
 		memset(&paramp, 0, sizeof(paramp));
 		memcpy(paramp.key, &mpp->reservation_key, 8);
 		rel_status = prout_do_scsi_ioctl(pp->dev, MPATH_PROUT_REL_SA,
@@ -604,15 +609,26 @@ static int do_preempt_self(struct multipath *mpp, struct be64 sa_key,
 		if (rel_status != MPATH_PR_SUCCESS)
 			condlog(0, "%s: release on alternate path failed.",
 				mpp->wwid);
+		else if (work == PREE_WORK_REL_UNREG) {
+			/* unregister the last path */
+			rel_status = prout_do_scsi_ioctl(pp->dev,
+							 MPATH_PROUT_REG_IGN_SA,
+							 rq_scope, rq_type,
+							 &paramp, noisy);
+			if (rel_status != MPATH_PR_SUCCESS)
+				condlog(0, "%s: final self preempt unregister failed,",
+					mpp->wwid);
+		}
+	}
+	if (work != PREE_WORK_REL_UNREG) {
+		memset(&paramp, 0, sizeof(paramp));
+		memcpy(paramp.sa_key, &mpp->reservation_key, 8);
+		status = mpath_prout_reg(mpp, MPATH_PROUT_REG_IGN_SA, rq_scope,
+					 rq_type, &paramp, noisy);
+		if (status != MPATH_PR_SUCCESS)
+			condlog(0, "%s: self preempt failed to reregister paths.",
+				mpp->wwid);
 	}
-
-	memset(&paramp, 0, sizeof(paramp));
-	memcpy(paramp.sa_key, &mpp->reservation_key, 8);
-	status = mpath_prout_reg(mpp, MPATH_PROUT_REG_IGN_SA, rq_scope,
-				 rq_type, &paramp, noisy);
-	if (status != MPATH_PR_SUCCESS)
-		condlog(0, "%s: self preempt failed to reregister paths.",
-			mpp->wwid);
 
 fail_resume:
 	if (!dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias, udev_flags)) {
@@ -625,10 +641,10 @@ fail_resume:
 }
 
 static int preempt_self(struct multipath *mpp, int rq_servact, int rq_scope,
-			unsigned int rq_type, int noisy, bool do_release)
+			unsigned int rq_type, int noisy, enum preempt_work work)
 {
 	return do_preempt_self(mpp, mpp->reservation_key, rq_servact, rq_scope,
-			       rq_type, noisy, do_release);
+			       rq_type, noisy, work);
 }
 
 static int preempt_all(struct multipath *mpp, int rq_servact, int rq_scope,
@@ -636,7 +652,7 @@ static int preempt_all(struct multipath *mpp, int rq_servact, int rq_scope,
 {
 	struct be64 zerokey = {0};
 	return do_preempt_self(mpp, zerokey, rq_servact, rq_scope, rq_type,
-			       noisy, false);
+			       noisy, PREE_WORK_NONE);
 }
 
 static int reservation_key_matches(struct multipath *mpp, uint8_t *key,
@@ -661,18 +677,21 @@ static int reservation_key_matches(struct multipath *mpp, uint8_t *key,
 	return YNU_NO;
 }
 
-static bool check_holding_reservation(struct multipath *mpp, unsigned int *type)
+static bool check_holding_reservation(struct multipath *mpp, uint8_t *curr_key,
+				      unsigned int *type)
 {
-	if (get_be64(mpp->reservation_key) &&
+	uint64_t zerokey = 0;
+	if (memcmp(curr_key, &zerokey, 8) != 0 &&
 	    get_prhold(mpp->alias) == PR_SET &&
-	    reservation_key_matches(mpp, (uint8_t *)&mpp->reservation_key, type) == YNU_YES)
+	    reservation_key_matches(mpp, curr_key, type) == YNU_YES)
 		return true;
 	return false;
 }
 
-static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
+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)
+			   struct prout_param_descriptor *paramp, int noisy,
+			   bool *unregistered)
 {
 	int i, j;
 	struct pathgroup *pgp = NULL;
@@ -685,6 +704,8 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 	bool all_threads_failed;
 	unsigned int res_type;
 
+	if (unregistered)
+		*unregistered = false;
 	if (!mpp)
 		return MPATH_PR_DMMP_ERROR;
 
@@ -762,7 +783,8 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 		return status;
 	}
 
-	if (!check_holding_reservation(mpp, &res_type)) {
+	if (!check_holding_reservation(mpp, (uint8_t *)&mpp->reservation_key,
+				       &res_type)) {
 		condlog(2, "%s: Releasing key not holding reservation.", mpp->wwid);
 		return MPATH_PR_SUCCESS;
 	}
@@ -785,7 +807,10 @@ static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int rq_scope,
 	 * 4. Reregistering keys on all the paths
 	 * 5. Resuming the device
 	 */
-	return preempt_self(mpp, MPATH_PROUT_PREE_SA, rq_scope, rq_type, noisy, true);
+	if (unregistered)
+		*unregistered = true;
+	return preempt_self(mpp, MPATH_PROUT_PREE_SA, rq_scope, rq_type, noisy,
+			    unregistered ? PREE_WORK_REL_UNREG : PREE_WORK_REL);
 }
 
 /*
@@ -828,7 +853,7 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	select_reservation_key(conf, mpp);
 	put_multipath_config(conf);
 
-	memcpy(&oldkey, &mpp->reservation_key, 8);
+	oldkey = mpp->reservation_key;
 	unregistering = (memcmp(&zerokey, paramp->sa_key, 8) == 0);
 	if (mpp->prkey_source == PRKEY_SOURCE_FILE &&
 	    (rq_servact == MPATH_PROUT_REG_IGN_SA ||
@@ -905,7 +930,21 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 	{
 	case MPATH_PROUT_REG_SA:
 	case MPATH_PROUT_REG_IGN_SA:
-		ret= mpath_prout_reg(mpp, rq_servact, rq_scope, rq_type, paramp, noisy);
+		if (unregistering && check_holding_reservation(mpp, (uint8_t *)&oldkey, &rq_type)) {
+			bool unregistered = false;
+			struct be64 newkey = mpp->reservation_key;
+			/* temporarily restore reservation key */
+			mpp->reservation_key = oldkey;
+			ret = mpath_prout_rel(mpp, MPATH_PROUT_REL_SA, rq_scope,
+					      rq_type, paramp, noisy,
+					      &unregistered);
+			mpp->reservation_key = newkey;
+			if (ret == MPATH_PR_SUCCESS && !unregistered)
+				ret = mpath_prout_reg(mpp, rq_servact, rq_scope,
+						      rq_type, paramp, noisy);
+		} else
+			ret = mpath_prout_reg(mpp, rq_servact, rq_scope,
+					      rq_type, paramp, noisy);
 		break;
 	case MPATH_PROUT_PREE_SA:
 	case MPATH_PROUT_PREE_AB_SA:
@@ -921,7 +960,7 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 		/* if we are preempting ourself */
 		if (memcmp(paramp->sa_key, paramp->key, 8) == 0) {
 			ret = preempt_self(mpp, rq_servact, rq_scope, rq_type,
-					   noisy, false);
+					   noisy, PREE_WORK_NONE);
 			break;
 		}
 		/* fallthrough */
@@ -932,14 +971,14 @@ int do_mpath_persistent_reserve_out(vector curmp, vector pathvec, int fd,
 					 paramp, noisy, NULL, &failed_paths);
 		if (rq_servact == MPATH_PROUT_RES_SA &&
 		    ret != MPATH_PR_SUCCESS && failed_paths &&
-		    check_holding_reservation(mpp, &res_type) &&
+		    check_holding_reservation(mpp, paramp->key, &res_type) &&
 		    res_type == rq_type)
 			/* The reserve failed, but multipathd says we hold it */
 			ret = MPATH_PR_SUCCESS;
 		break;
 	}
 	case MPATH_PROUT_REL_SA:
-		ret = mpath_prout_rel(mpp, rq_servact, rq_scope, rq_type, paramp, noisy);
+		ret = mpath_prout_rel(mpp, rq_servact, rq_scope, rq_type, paramp, noisy, NULL);
 		break;
 	default:
 		return MPATH_PR_OTHER;
-- 
2.50.1

Re: [PATCH 4/4] libmpathpersist: Fix unregistering while holding the reservation

[Martin Wilck]

On Tue, 2025-09-23 at 18:12 -0400, Benjamin Marzinski wrote:
> There were two problems with how libmpathpersist handled
> unregistering
> a key while holding the reseravation (which should also release the
> reservation).
> 1. If the path holding the reservation is not unregistered first,
> there
>    will be unregistered paths, while a reservation is still held,
> which
>    would cause IO to those paths to fail, when it shouldn't.
> 2. If the path that holds the reservation is down, libmpathpersist
> was
>    not clearing the resrvation, since the there were no registered
> keys
>    it could use for the PREEMPT command workaround
> 
> To fix these, libmpathpersist now releases the reservation first when
> trying to unregister a key that is holding the reservation.
> mpath_prout_rel() has a new option so that if it needs to self
> preempt
> to clear the reservation, it won't re-register the paths when called
> as part of unregistering a key. Also, instead of checking if the
> device
> is currently holding a reservation using mpp->reservation_key in
> check_holding_reservation() (which will already be set to 0 when
> called
> as part of unregistering a key), pass in the key to check.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  libmpathpersist/mpath_persist_int.c | 101 +++++++++++++++++++-------
> --
>  1 file changed, 70 insertions(+), 31 deletions(-)
> 
> diff --git a/libmpathpersist/mpath_persist_int.c
> b/libmpathpersist/mpath_persist_int.c
> index 9c71011f..f130f5f5 100644
> --- a/libmpathpersist/mpath_persist_int.c
> +++ b/libmpathpersist/mpath_persist_int.c
> @@ -564,18 +564,23 @@ static int mpath_prout_reg(struct multipath
> *mpp,int rq_servact, int rq_scope,
>  	return status;
>  }
>  
> +enum preempt_work {
> +	PREE_WORK_NONE,
> +	PREE_WORK_REL,
> +	PREE_WORK_REL_UNREG,
> +};
>  /*
>   * Called to make a multipath device preempt its own reservation
> (and
> - * optionally release the reservation). Doing this causes the
> reservation
> - * keys to be removed from all the device paths except that path
> used to issue
> - * the preempt, so they need to be restored. To avoid the chance
> that IO
> - * goes to these paths when they don't have a registered key, the
> device
> - * is suspended before issuing the preemption, and the keys are
> reregistered
> - * before resuming it.
> + * optional extra work). Doing this causes the reservation keys to
> be removed
> + * from all the device paths except that path used to issue the
> preempt, so
> + * they may need to be restored. To avoid the chance that IO goes to
> these
> + * paths when they don't have a registered key and a reservation
> exists, the
> + * device is suspended before issuing the preemption, and the keys
> are
> + * reregistered (or the reservation is released) before resuming it.
>   */
>  static int do_preempt_self(struct multipath *mpp, struct be64
> sa_key,
>  			   int rq_servact, int rq_scope, unsigned
> int rq_type,
> -			   int noisy, bool do_release)
> +			   int noisy, enum preempt_work work)
>  {
>  	int status, rel_status = MPATH_PR_SUCCESS;
>  	struct path *pp = NULL;
> @@ -596,7 +601,7 @@ static int do_preempt_self(struct multipath *mpp,
> struct be64 sa_key,
>  		goto fail_resume;
>  	}
>  
> -	if (do_release) {
> +	if (work != PREE_WORK_NONE) {
>  		memset(&paramp, 0, sizeof(paramp));
>  		memcpy(paramp.key, &mpp->reservation_key, 8);
>  		rel_status = prout_do_scsi_ioctl(pp->dev,
> MPATH_PROUT_REL_SA,
> @@ -604,15 +609,26 @@ static int do_preempt_self(struct multipath
> *mpp, struct be64 sa_key,
>  		if (rel_status != MPATH_PR_SUCCESS)
>  			condlog(0, "%s: release on alternate path
> failed.",
>  				mpp->wwid);
> +		else if (work == PREE_WORK_REL_UNREG) {
> +			/* unregister the last path */
> +			rel_status = prout_do_scsi_ioctl(pp->dev,
> +							
> MPATH_PROUT_REG_IGN_SA,
> +							 rq_scope,
> rq_type,
> +							 &paramp,
> noisy);
> +			if (rel_status != MPATH_PR_SUCCESS)
> +				condlog(0, "%s: final self preempt
> unregister failed,",
> +					mpp->wwid);
> +		}
> +	}
> +	if (work != PREE_WORK_REL_UNREG) {
> +		memset(&paramp, 0, sizeof(paramp));
> +		memcpy(paramp.sa_key, &mpp->reservation_key, 8);
> +		status = mpath_prout_reg(mpp,
> MPATH_PROUT_REG_IGN_SA, rq_scope,
> +					 rq_type, &paramp, noisy);
> +		if (status != MPATH_PR_SUCCESS)
> +			condlog(0, "%s: self preempt failed to
> reregister paths.",
> +				mpp->wwid);
>  	}
> -
> -	memset(&paramp, 0, sizeof(paramp));
> -	memcpy(paramp.sa_key, &mpp->reservation_key, 8);
> -	status = mpath_prout_reg(mpp, MPATH_PROUT_REG_IGN_SA,
> rq_scope,
> -				 rq_type, &paramp, noisy);
> -	if (status != MPATH_PR_SUCCESS)
> -		condlog(0, "%s: self preempt failed to reregister
> paths.",
> -			mpp->wwid);
>  
>  fail_resume:
>  	if (!dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias,
> udev_flags)) {
> @@ -625,10 +641,10 @@ fail_resume:
>  }
>  
>  static int preempt_self(struct multipath *mpp, int rq_servact, int
> rq_scope,
> -			unsigned int rq_type, int noisy, bool
> do_release)
> +			unsigned int rq_type, int noisy, enum
> preempt_work work)
>  {
>  	return do_preempt_self(mpp, mpp->reservation_key,
> rq_servact, rq_scope,
> -			       rq_type, noisy, do_release);
> +			       rq_type, noisy, work);
>  }
>  
>  static int preempt_all(struct multipath *mpp, int rq_servact, int
> rq_scope,
> @@ -636,7 +652,7 @@ static int preempt_all(struct multipath *mpp, int
> rq_servact, int rq_scope,
>  {
>  	struct be64 zerokey = {0};
>  	return do_preempt_self(mpp, zerokey, rq_servact, rq_scope,
> rq_type,
> -			       noisy, false);
> +			       noisy, PREE_WORK_NONE);
>  }
>  
>  static int reservation_key_matches(struct multipath *mpp, uint8_t
> *key,
> @@ -661,18 +677,21 @@ static int reservation_key_matches(struct
> multipath *mpp, uint8_t *key,
>  	return YNU_NO;
>  }
>  
> -static bool check_holding_reservation(struct multipath *mpp,
> unsigned int *type)
> +static bool check_holding_reservation(struct multipath *mpp, uint8_t
> *curr_key,
> +				      unsigned int *type)
>  {
> -	if (get_be64(mpp->reservation_key) &&
> +	uint64_t zerokey = 0;
> +	if (memcmp(curr_key, &zerokey, 8) != 0 &&
>  	    get_prhold(mpp->alias) == PR_SET &&
> -	    reservation_key_matches(mpp, (uint8_t *)&mpp-
> >reservation_key, type) == YNU_YES)
> +	    reservation_key_matches(mpp, curr_key, type) == YNU_YES)
>  		return true;
>  	return false;
>  }
>  
> -static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int
> rq_scope,
> +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)
> +			   struct prout_param_descriptor *paramp,
> int noisy,
> +			   bool *unregistered)
>  {
>  	int i, j;
>  	struct pathgroup *pgp = NULL;
> @@ -685,6 +704,8 @@ static int mpath_prout_rel(struct multipath
> *mpp,int rq_servact, int rq_scope,
>  	bool all_threads_failed;
>  	unsigned int res_type;
>  
> +	if (unregistered)
> +		*unregistered = false;
>  	if (!mpp)
>  		return MPATH_PR_DMMP_ERROR;
>  
> @@ -762,7 +783,8 @@ static int mpath_prout_rel(struct multipath
> *mpp,int rq_servact, int rq_scope,
>  		return status;
>  	}
>  
> -	if (!check_holding_reservation(mpp, &res_type)) {
> +	if (!check_holding_reservation(mpp, (uint8_t *)&mpp-
> >reservation_key,
> +				       &res_type)) {
>  		condlog(2, "%s: Releasing key not holding
> reservation.", mpp->wwid);
>  		return MPATH_PR_SUCCESS;
>  	}
> @@ -785,7 +807,10 @@ static int mpath_prout_rel(struct multipath
> *mpp,int rq_servact, int rq_scope,
>  	 * 4. Reregistering keys on all the paths
>  	 * 5. Resuming the device
>  	 */
> -	return preempt_self(mpp, MPATH_PROUT_PREE_SA, rq_scope,
> rq_type, noisy, true);
> +	if (unregistered)
> +		*unregistered = true;
> +	return preempt_self(mpp, MPATH_PROUT_PREE_SA, rq_scope,
> rq_type, noisy,
> +			    unregistered ? PREE_WORK_REL_UNREG :
> PREE_WORK_REL);

This is hard to understand and possibly error-prone in the future.

You use the pointer "unregistered" as a boolean, which looks like a
typo (missing "*") in the first place. It turns out that it's
intentional, and that (unregistered != NULL) indicates that this code
was called from the MPATH_PROUT_REG_IGN_SA case in
do_mpath_persistent_reserve_out() (as opposed to the MPATH_PROUT_REL_SA
case).

Could you write this in a somewhat more obvious way, please; perhaps by
passing a preempt_work parameter to mpath_prout_rel()?


>  }
>  
>  /*
> @@ -828,7 +853,7 @@ int do_mpath_persistent_reserve_out(vector curmp,
> vector pathvec, int fd,
>  	select_reservation_key(conf, mpp);
>  	put_multipath_config(conf);
>  
> -	memcpy(&oldkey, &mpp->reservation_key, 8);
> +	oldkey = mpp->reservation_key;
>  	unregistering = (memcmp(&zerokey, paramp->sa_key, 8) == 0);
>  	if (mpp->prkey_source == PRKEY_SOURCE_FILE &&
>  	    (rq_servact == MPATH_PROUT_REG_IGN_SA ||
> @@ -905,7 +930,21 @@ int do_mpath_persistent_reserve_out(vector
> curmp, vector pathvec, int fd,
>  	{
>  	case MPATH_PROUT_REG_SA:
>  	case MPATH_PROUT_REG_IGN_SA:
> -		ret= mpath_prout_reg(mpp, rq_servact, rq_scope,
> rq_type, paramp, noisy);
> +		if (unregistering && check_holding_reservation(mpp,
> (uint8_t *)&oldkey, &rq_type)) {
> +			bool unregistered = false;
> +			struct be64 newkey = mpp->reservation_key;
> +			/* temporarily restore reservation key */
> +			mpp->reservation_key = oldkey;
> +			ret = mpath_prout_rel(mpp,
> MPATH_PROUT_REL_SA, rq_scope,
> +					      rq_type, paramp,
> noisy,
> +					      &unregistered);
> +			mpp->reservation_key = newkey;
> +			if (ret == MPATH_PR_SUCCESS &&
> !unregistered)
> +				ret = mpath_prout_reg(mpp,
> rq_servact, rq_scope,
> +						      rq_type,
> paramp, noisy);

I comment stating that mpp->reservation_key is zero here and that
mpath_prout_reg() actually unregisters the key would help readers of
this code.

Regards
Martin

> +		} else
> +			ret = mpath_prout_reg(mpp, rq_servact,
> rq_scope,
> +					      rq_type, paramp,
> noisy);
>  		break;
>  	case MPATH_PROUT_PREE_SA:
>  	case MPATH_PROUT_PREE_AB_SA:
> @@ -921,7 +960,7 @@ int do_mpath_persistent_reserve_out(vector curmp,
> vector pathvec, int fd,
>  		/* if we are preempting ourself */
>  		if (memcmp(paramp->sa_key, paramp->key, 8) == 0) {
>  			ret = preempt_self(mpp, rq_servact,
> rq_scope, rq_type,
> -					   noisy, false);
> +					   noisy, PREE_WORK_NONE);
>  			break;
>  		}
>  		/* fallthrough */
> @@ -932,14 +971,14 @@ int do_mpath_persistent_reserve_out(vector
> curmp, vector pathvec, int fd,
>  					 paramp, noisy, NULL,
> &failed_paths);
>  		if (rq_servact == MPATH_PROUT_RES_SA &&
>  		    ret != MPATH_PR_SUCCESS && failed_paths &&
> -		    check_holding_reservation(mpp, &res_type) &&
> +		    check_holding_reservation(mpp, paramp->key,
> &res_type) &&
>  		    res_type == rq_type)
>  			/* The reserve failed, but multipathd says
> we hold it */
>  			ret = MPATH_PR_SUCCESS;
>  		break;
>  	}
>  	case MPATH_PROUT_REL_SA:
> -		ret = mpath_prout_rel(mpp, rq_servact, rq_scope,
> rq_type, paramp, noisy);
> +		ret = mpath_prout_rel(mpp, rq_servact, rq_scope,
> rq_type, paramp, noisy, NULL);
>  		break;
>  	default:
>  		return MPATH_PR_OTHER;

Re: [PATCH 4/4] libmpathpersist: Fix unregistering while holding the reservation

[Benjamin Marzinski]

On Wed, Sep 24, 2025 at 09:12:26PM +0200, Martin Wilck wrote:
> On Tue, 2025-09-23 at 18:12 -0400, Benjamin Marzinski wrote:
> > There were two problems with how libmpathpersist handled
> > unregistering
> > a key while holding the reseravation (which should also release the
> > reservation).
> > 1. If the path holding the reservation is not unregistered first,
> > there
> >    will be unregistered paths, while a reservation is still held,
> > which
> >    would cause IO to those paths to fail, when it shouldn't.
> > 2. If the path that holds the reservation is down, libmpathpersist
> > was
> >    not clearing the resrvation, since the there were no registered
> > keys
> >    it could use for the PREEMPT command workaround
> > 
> > To fix these, libmpathpersist now releases the reservation first when
> > trying to unregister a key that is holding the reservation.
> > mpath_prout_rel() has a new option so that if it needs to self
> > preempt
> > to clear the reservation, it won't re-register the paths when called
> > as part of unregistering a key. Also, instead of checking if the
> > device
> > is currently holding a reservation using mpp->reservation_key in
> > check_holding_reservation() (which will already be set to 0 when
> > called
> > as part of unregistering a key), pass in the key to check.
> > 
> > Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> > ---
> >  libmpathpersist/mpath_persist_int.c | 101 +++++++++++++++++++-------
> > --
> >  1 file changed, 70 insertions(+), 31 deletions(-)
> > 
> > diff --git a/libmpathpersist/mpath_persist_int.c
> > b/libmpathpersist/mpath_persist_int.c
> > index 9c71011f..f130f5f5 100644
> > --- a/libmpathpersist/mpath_persist_int.c
> > +++ b/libmpathpersist/mpath_persist_int.c
> > @@ -564,18 +564,23 @@ static int mpath_prout_reg(struct multipath
> > *mpp,int rq_servact, int rq_scope,
> >  	return status;
> >  }
> >  
> > +enum preempt_work {
> > +	PREE_WORK_NONE,
> > +	PREE_WORK_REL,
> > +	PREE_WORK_REL_UNREG,
> > +};
> >  /*
> >   * Called to make a multipath device preempt its own reservation
> > (and
> > - * optionally release the reservation). Doing this causes the
> > reservation
> > - * keys to be removed from all the device paths except that path
> > used to issue
> > - * the preempt, so they need to be restored. To avoid the chance
> > that IO
> > - * goes to these paths when they don't have a registered key, the
> > device
> > - * is suspended before issuing the preemption, and the keys are
> > reregistered
> > - * before resuming it.
> > + * optional extra work). Doing this causes the reservation keys to
> > be removed
> > + * from all the device paths except that path used to issue the
> > preempt, so
> > + * they may need to be restored. To avoid the chance that IO goes to
> > these
> > + * paths when they don't have a registered key and a reservation
> > exists, the
> > + * device is suspended before issuing the preemption, and the keys
> > are
> > + * reregistered (or the reservation is released) before resuming it.
> >   */
> >  static int do_preempt_self(struct multipath *mpp, struct be64
> > sa_key,
> >  			   int rq_servact, int rq_scope, unsigned
> > int rq_type,
> > -			   int noisy, bool do_release)
> > +			   int noisy, enum preempt_work work)
> >  {
> >  	int status, rel_status = MPATH_PR_SUCCESS;
> >  	struct path *pp = NULL;
> > @@ -596,7 +601,7 @@ static int do_preempt_self(struct multipath *mpp,
> > struct be64 sa_key,
> >  		goto fail_resume;
> >  	}
> >  
> > -	if (do_release) {
> > +	if (work != PREE_WORK_NONE) {
> >  		memset(&paramp, 0, sizeof(paramp));
> >  		memcpy(paramp.key, &mpp->reservation_key, 8);
> >  		rel_status = prout_do_scsi_ioctl(pp->dev,
> > MPATH_PROUT_REL_SA,
> > @@ -604,15 +609,26 @@ static int do_preempt_self(struct multipath
> > *mpp, struct be64 sa_key,
> >  		if (rel_status != MPATH_PR_SUCCESS)
> >  			condlog(0, "%s: release on alternate path
> > failed.",
> >  				mpp->wwid);
> > +		else if (work == PREE_WORK_REL_UNREG) {
> > +			/* unregister the last path */
> > +			rel_status = prout_do_scsi_ioctl(pp->dev,
> > +							
> > MPATH_PROUT_REG_IGN_SA,
> > +							 rq_scope,
> > rq_type,
> > +							 &paramp,
> > noisy);
> > +			if (rel_status != MPATH_PR_SUCCESS)
> > +				condlog(0, "%s: final self preempt
> > unregister failed,",
> > +					mpp->wwid);
> > +		}
> > +	}
> > +	if (work != PREE_WORK_REL_UNREG) {
> > +		memset(&paramp, 0, sizeof(paramp));
> > +		memcpy(paramp.sa_key, &mpp->reservation_key, 8);
> > +		status = mpath_prout_reg(mpp,
> > MPATH_PROUT_REG_IGN_SA, rq_scope,
> > +					 rq_type, &paramp, noisy);
> > +		if (status != MPATH_PR_SUCCESS)
> > +			condlog(0, "%s: self preempt failed to
> > reregister paths.",
> > +				mpp->wwid);
> >  	}
> > -
> > -	memset(&paramp, 0, sizeof(paramp));
> > -	memcpy(paramp.sa_key, &mpp->reservation_key, 8);
> > -	status = mpath_prout_reg(mpp, MPATH_PROUT_REG_IGN_SA,
> > rq_scope,
> > -				 rq_type, &paramp, noisy);
> > -	if (status != MPATH_PR_SUCCESS)
> > -		condlog(0, "%s: self preempt failed to reregister
> > paths.",
> > -			mpp->wwid);
> >  
> >  fail_resume:
> >  	if (!dm_simplecmd_noflush(DM_DEVICE_RESUME, mpp->alias,
> > udev_flags)) {
> > @@ -625,10 +641,10 @@ fail_resume:
> >  }
> >  
> >  static int preempt_self(struct multipath *mpp, int rq_servact, int
> > rq_scope,
> > -			unsigned int rq_type, int noisy, bool
> > do_release)
> > +			unsigned int rq_type, int noisy, enum
> > preempt_work work)
> >  {
> >  	return do_preempt_self(mpp, mpp->reservation_key,
> > rq_servact, rq_scope,
> > -			       rq_type, noisy, do_release);
> > +			       rq_type, noisy, work);
> >  }
> >  
> >  static int preempt_all(struct multipath *mpp, int rq_servact, int
> > rq_scope,
> > @@ -636,7 +652,7 @@ static int preempt_all(struct multipath *mpp, int
> > rq_servact, int rq_scope,
> >  {
> >  	struct be64 zerokey = {0};
> >  	return do_preempt_self(mpp, zerokey, rq_servact, rq_scope,
> > rq_type,
> > -			       noisy, false);
> > +			       noisy, PREE_WORK_NONE);
> >  }
> >  
> >  static int reservation_key_matches(struct multipath *mpp, uint8_t
> > *key,
> > @@ -661,18 +677,21 @@ static int reservation_key_matches(struct
> > multipath *mpp, uint8_t *key,
> >  	return YNU_NO;
> >  }
> >  
> > -static bool check_holding_reservation(struct multipath *mpp,
> > unsigned int *type)
> > +static bool check_holding_reservation(struct multipath *mpp, uint8_t
> > *curr_key,
> > +				      unsigned int *type)
> >  {
> > -	if (get_be64(mpp->reservation_key) &&
> > +	uint64_t zerokey = 0;
> > +	if (memcmp(curr_key, &zerokey, 8) != 0 &&
> >  	    get_prhold(mpp->alias) == PR_SET &&
> > -	    reservation_key_matches(mpp, (uint8_t *)&mpp-
> > >reservation_key, type) == YNU_YES)
> > +	    reservation_key_matches(mpp, curr_key, type) == YNU_YES)
> >  		return true;
> >  	return false;
> >  }
> >  
> > -static int mpath_prout_rel(struct multipath *mpp,int rq_servact, int
> > rq_scope,
> > +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)
> > +			   struct prout_param_descriptor *paramp,
> > int noisy,
> > +			   bool *unregistered)
> >  {
> >  	int i, j;
> >  	struct pathgroup *pgp = NULL;
> > @@ -685,6 +704,8 @@ static int mpath_prout_rel(struct multipath
> > *mpp,int rq_servact, int rq_scope,
> >  	bool all_threads_failed;
> >  	unsigned int res_type;
> >  
> > +	if (unregistered)
> > +		*unregistered = false;
> >  	if (!mpp)
> >  		return MPATH_PR_DMMP_ERROR;
> >  
> > @@ -762,7 +783,8 @@ static int mpath_prout_rel(struct multipath
> > *mpp,int rq_servact, int rq_scope,
> >  		return status;
> >  	}
> >  
> > -	if (!check_holding_reservation(mpp, &res_type)) {
> > +	if (!check_holding_reservation(mpp, (uint8_t *)&mpp-
> > >reservation_key,
> > +				       &res_type)) {
> >  		condlog(2, "%s: Releasing key not holding
> > reservation.", mpp->wwid);
> >  		return MPATH_PR_SUCCESS;
> >  	}
> > @@ -785,7 +807,10 @@ static int mpath_prout_rel(struct multipath
> > *mpp,int rq_servact, int rq_scope,
> >  	 * 4. Reregistering keys on all the paths
> >  	 * 5. Resuming the device
> >  	 */
> > -	return preempt_self(mpp, MPATH_PROUT_PREE_SA, rq_scope,
> > rq_type, noisy, true);
> > +	if (unregistered)
> > +		*unregistered = true;
> > +	return preempt_self(mpp, MPATH_PROUT_PREE_SA, rq_scope,
> > rq_type, noisy,
> > +			    unregistered ? PREE_WORK_REL_UNREG :
> > PREE_WORK_REL);
> 
> This is hard to understand and possibly error-prone in the future.
> 
> You use the pointer "unregistered" as a boolean, which looks like a
> typo (missing "*") in the first place. It turns out that it's
> intentional, and that (unregistered != NULL) indicates that this code
> was called from the MPATH_PROUT_REG_IGN_SA case in
> do_mpath_persistent_reserve_out() (as opposed to the MPATH_PROUT_REL_SA
> case).
> 
> Could you write this in a somewhat more obvious way, please; perhaps by
> passing a preempt_work parameter to mpath_prout_rel()?
> 

Sure.

> 
> >  }
> >  
> >  /*
> > @@ -828,7 +853,7 @@ int do_mpath_persistent_reserve_out(vector curmp,
> > vector pathvec, int fd,
> >  	select_reservation_key(conf, mpp);
> >  	put_multipath_config(conf);
> >  
> > -	memcpy(&oldkey, &mpp->reservation_key, 8);
> > +	oldkey = mpp->reservation_key;
> >  	unregistering = (memcmp(&zerokey, paramp->sa_key, 8) == 0);
> >  	if (mpp->prkey_source == PRKEY_SOURCE_FILE &&
> >  	    (rq_servact == MPATH_PROUT_REG_IGN_SA ||
> > @@ -905,7 +930,21 @@ int do_mpath_persistent_reserve_out(vector
> > curmp, vector pathvec, int fd,
> >  	{
> >  	case MPATH_PROUT_REG_SA:
> >  	case MPATH_PROUT_REG_IGN_SA:
> > -		ret= mpath_prout_reg(mpp, rq_servact, rq_scope,
> > rq_type, paramp, noisy);
> > +		if (unregistering && check_holding_reservation(mpp,
> > (uint8_t *)&oldkey, &rq_type)) {
> > +			bool unregistered = false;
> > +			struct be64 newkey = mpp->reservation_key;
> > +			/* temporarily restore reservation key */
> > +			mpp->reservation_key = oldkey;
> > +			ret = mpath_prout_rel(mpp,
> > MPATH_PROUT_REL_SA, rq_scope,
> > +					      rq_type, paramp,
> > noisy,
> > +					      &unregistered);
> > +			mpp->reservation_key = newkey;
> > +			if (ret == MPATH_PR_SUCCESS &&
> > !unregistered)
> > +				ret = mpath_prout_reg(mpp,
> > rq_servact, rq_scope,
> > +						      rq_type,
> > paramp, noisy);
> 
> I comment stating that mpp->reservation_key is zero here and that
> mpath_prout_reg() actually unregisters the key would help readers of
> this code.

Sure.

-Ben
 
> Regards
> Martin
> 
> > +		} else
> > +			ret = mpath_prout_reg(mpp, rq_servact,
> > rq_scope,
> > +					      rq_type, paramp,
> > noisy);
> >  		break;
> >  	case MPATH_PROUT_PREE_SA:
> >  	case MPATH_PROUT_PREE_AB_SA:
> > @@ -921,7 +960,7 @@ int do_mpath_persistent_reserve_out(vector curmp,
> > vector pathvec, int fd,
> >  		/* if we are preempting ourself */
> >  		if (memcmp(paramp->sa_key, paramp->key, 8) == 0) {
> >  			ret = preempt_self(mpp, rq_servact,
> > rq_scope, rq_type,
> > -					   noisy, false);
> > +					   noisy, PREE_WORK_NONE);
> >  			break;
> >  		}
> >  		/* fallthrough */
> > @@ -932,14 +971,14 @@ int do_mpath_persistent_reserve_out(vector
> > curmp, vector pathvec, int fd,
> >  					 paramp, noisy, NULL,
> > &failed_paths);
> >  		if (rq_servact == MPATH_PROUT_RES_SA &&
> >  		    ret != MPATH_PR_SUCCESS && failed_paths &&
> > -		    check_holding_reservation(mpp, &res_type) &&
> > +		    check_holding_reservation(mpp, paramp->key,
> > &res_type) &&
> >  		    res_type == rq_type)
> >  			/* The reserve failed, but multipathd says
> > we hold it */
> >  			ret = MPATH_PR_SUCCESS;
> >  		break;
> >  	}
> >  	case MPATH_PROUT_REL_SA:
> > -		ret = mpath_prout_rel(mpp, rq_servact, rq_scope,
> > rq_type, paramp, noisy);
> > +		ret = mpath_prout_rel(mpp, rq_servact, rq_scope,
> > rq_type, paramp, noisy, NULL);
> >  		break;
> >  	default:
> >  		return MPATH_PR_OTHER;

Re: [PATCH 0/4] libmultipath: Yet Another PR patchset

[Martin Wilck]

On Tue, 2025-09-23 at 18:12 -0400, Benjamin Marzinski wrote:
> This patchset handles 4 separate issues:
> 1. The self-preemption workaround for changing keys using REGISTER
> AND
>    IGNORE, while the path holding the reservation was down was broken
>    by a recent commit.
> 2. mpathpersist was failing a RESERVE command issued on a multipath
>    device that held a reservation, when the holding path was down.
>    Issuing an identical reserve on a device that holds a reservation
>    is supposed to succeed, and multipathd has enough information to
>    know the device is actually holding the reservation. So if
>    multipathd says that the device is holding the reservation, it
>    should not fail, even if the path holding the reservation is down.
> 3. mpathpersist was not clearing the reservation when the key holding
>    it was unregistered, if the path holding the reservation was down.
> 4. When unregistering a key that holds a reservation, depending on
> the
>    order that the paths got unregistered, there was a window where
>    IO going to the device could fail. This should not happen because
>    the reservation should be removed and the key should be
> unregistered
>    atomically.
> 
> Benjamin Marzinski (4):
>   libmpathpersist: Fix REGISTER AND IGNORE while holding a
> reservation
>   libmpathpersist: Handle RESERVE with reservation held by failed
> path
>   libmpathpersist: use check_holding_reservation in mpath_prout_rel
>   libmpathpersist: Fix unregistering while holding the reservation
> 
>  libmpathpersist/mpath_persist_int.c | 199 +++++++++++++++++---------
> --
>  1 file changed, 125 insertions(+), 74 deletions(-)

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

Re: [PATCH 0/4] libmultipath: Yet Another PR patchset

[Martin Wilck]

On Tue, 2025-10-07 at 22:23 +0200, Martin Wilck wrote:
> On Tue, 2025-09-23 at 18:12 -0400, Benjamin Marzinski wrote:
> > This patchset handles 4 separate issues:
> > 1. The self-preemption workaround for changing keys using REGISTER
> > AND
> >    IGNORE, while the path holding the reservation was down was
> > broken
> >    by a recent commit.
> > 2. mpathpersist was failing a RESERVE command issued on a multipath
> >    device that held a reservation, when the holding path was down.
> >    Issuing an identical reserve on a device that holds a
> > reservation
> >    is supposed to succeed, and multipathd has enough information to
> >    know the device is actually holding the reservation. So if
> >    multipathd says that the device is holding the reservation, it
> >    should not fail, even if the path holding the reservation is
> > down.
> > 3. mpathpersist was not clearing the reservation when the key
> > holding
> >    it was unregistered, if the path holding the reservation was
> > down.
> > 4. When unregistering a key that holds a reservation, depending on
> > the
> >    order that the paths got unregistered, there was a window where
> >    IO going to the device could fail. This should not happen
> > because
> >    the reservation should be removed and the key should be
> > unregistered
> >    atomically.
> > 
> > Benjamin Marzinski (4):
> >   libmpathpersist: Fix REGISTER AND IGNORE while holding a
> > reservation
> >   libmpathpersist: Handle RESERVE with reservation held by failed
> > path
> >   libmpathpersist: use check_holding_reservation in mpath_prout_rel
> >   libmpathpersist: Fix unregistering while holding the reservation
> > 
> >  libmpathpersist/mpath_persist_int.c | 199 +++++++++++++++++-------
> > --
> > --
> >  1 file changed, 125 insertions(+), 74 deletions(-)
> 
> For the series: 
> Reviewed-by: Martin Wilck <mwilck@suse.com>

Nope. I meant to reply to the V2. Sorry.