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 <[email protected]> > --- > 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
