On Thu, 2026-07-23 at 16:12 +0200, Martin Wilck wrote: > 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.
Forget this remark, it's fine as-is. Martin
