On Tue, 2026-07-21 at 15:24 -0400, Benjamin Marzinski wrote: > From: Cykang <[email protected]> > > According to the SCSI-3 Persistent Reservations specification, when a > self-preemption is performed with a key that does not currently hold > an active reservation, the operation should be treated as an > unregister > and should not fail. > > The current code in do_mpath_persistent_reserve_out() does not check > whether the preempting key has a reservation before invoking the > self-preemption path. It unconditionally calls preempt_self() and > re-registers the key on the local paths, even when the key has no > reservation. This violates the SCSI-3 PR spec and can lead to > incorrect reservation states. > > This patch moves the self-preemption check inside the conditional > block so that it only executes when the key matches, but does not > bypass the general preemption path for keys without a reservation. > With this change, the code will fall through to the common PR_OUT > handling (e.g., MPATH_PROUT_RES_SA or CLEAR_SA) when the key is not > reserved, allowing the storage target to properly process the > operation as an unregister. > > Signed-off-by: Cykang <[email protected]> > Signed-off-by: Benjamin Marzinski <[email protected]>
Reviewed-by: Martin Wilck <[email protected]> One minor remark below. > --- > libmpathpersist/mpath_persist_int.c | 27 ++++++++++++++++++++------- > 1 file changed, 20 insertions(+), 7 deletions(-) > > diff --git a/libmpathpersist/mpath_persist_int.c > b/libmpathpersist/mpath_persist_int.c > index 3091c5c2..97fc71a7 100644 > --- a/libmpathpersist/mpath_persist_int.c > +++ b/libmpathpersist/mpath_persist_int.c > @@ -841,6 +841,7 @@ int do_mpath_persistent_reserve_out(vector curmp, > vector pathvec, int fd, > bool unregistering, preempting_reservation = false; > bool updated_prkey = false; > bool failed_paths = false; > + bool self_preempt_unreg = false; > > ret = mpath_get_map(curmp, fd, &mpp); > if (ret != MPATH_PR_SUCCESS) > @@ -953,7 +954,10 @@ 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, > NULL) == YNU_YES) { > + ret = reservation_key_matches(mpp, paramp->sa_key, > NULL); > + if (ret == YNU_UNDEF) > + return MPATH_PR_OTHER; > + if (ret == YNU_YES) { > preempting_reservation = true; > if (memcmp(paramp->sa_key, &zerokey, 8) == > 0) { > /* all registrants case */ > @@ -961,13 +965,20 @@ int do_mpath_persistent_reserve_out(vector > curmp, vector pathvec, int fd, > rq_type, noisy); > break; > } > + /* 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, > PREE_WORK_NONE); > + break; > + } > + } else if (memcmp(paramp->sa_key, paramp->key, 8) == > 0) { > + /* > + * We are self-preempting, but we don't hold > the > + * reservation. This will unregister the > device > + */ > + self_preempt_unreg = true; If you don't mind, I'll add a comment here that this will fall though further down. It's not immediately obvious IMO. Regards Martin > } > - /* 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, PREE_WORK_NONE); > - break; > - } > + > /* fallthrough */ > case MPATH_PROUT_RES_SA: > case MPATH_PROUT_CLEAR_SA: { > @@ -1018,6 +1029,8 @@ int do_mpath_persistent_reserve_out(vector > curmp, vector pathvec, int fd, > case MPATH_PROUT_PREE_AB_SA: > if (preempting_reservation) > update_prhold(mpp->alias, true); > + else if (self_preempt_unreg) > + update_prflag(mpp, 0); > } > return ret; > } -- Dr. Martin Wilck <[email protected]> SUSE Software Solutions Germany GmbH, Frankenstr. 146, 90461 Nürnberg, Germany Geschäftsführer: Jochen Jaser, Andrew McDonald (HRB 36809,AG Nürnberg)
