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 <[email protected]> > --- > 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, ¶mp, noisy, NULL); > + rq_type, ¶mp, 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, > - ¶mp, noisy, &pp); > + ¶mp, 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
