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

[Benjamin Marzinski <bmarzins@redhat.com>]

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