On Thu, Jul 16, 2026 at 10:41:27AM +0200, Martin Wilck wrote:
> Thanks for your patch!
>
> On Sat, 2026-07-11 at 11:31 +0800, Cykang wrote:
> > 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]>
>
> Your patch would cause PREEMPT to do nothing at all when it doesn't own
> the reservation (more precisely, when sa_key doesn't match the mpp's
> key). That seems wrong to me. You are right that in this case the keys
> shouldn't be re-registered. But we must take care that the mpp's key is
> unregistered on all paths. AFAICS, this is the effect of the command
>
> status = mpath_prout_common(mpp, rq_servact, rq_scope,
> rq_type,
> ¶mp, noisy, &pp, NULL);
>
With Cykang's patch, this call happens in the
do_mpath_persistent_reserve_out() fallthrough, so it should still work.
And if we are unregistering, then there is no need to suspend while we
are doing the work, so I don't see a need for calling preempt_self() in
this case. The one thing we should do is call
update_prflag(mpp, 0);
on success. Multipathd should still detect that it was preempted if we
don't, but we might as well explicitly tell it.
-Ben
> in preempt_self(), if and only if this command was successful and the
> key of the mpp doesn't hold the current reservation. (According to SPC-
> 6, in this case the storage server should "remove the registrations for
> all I_T nexuses specified by the SERVICE ACTION RESERVATION KEY
> field").
>
> So IMO what we should do is simply resume and return in preempt_self()
> after the above command if we don't hold the reservation, which we
> should check using check_holding_reservation().
>
> I guess this requires a new PREE_WORK_ enum value.
>
> I'll leave the final verdict to Ben, who knows the logic of our PR code
> better than me.
>
> Regards
> Martin
>
>
> > ---
> > libmpathpersist/mpath_persist_int.c | 13 +++++++------
> > 1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/libmpathpersist/mpath_persist_int.c
> > b/libmpathpersist/mpath_persist_int.c
> > index 3091c5c2..2ee7fe68 100644
> > --- a/libmpathpersist/mpath_persist_int.c
> > +++ b/libmpathpersist/mpath_persist_int.c
> > @@ -960,14 +960,15 @@ int do_mpath_persistent_reserve_out(vector
> > curmp, vector pathvec, int fd,
> > ret = preempt_all(mpp, rq_servact,
> > rq_scope,
> > 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;
> > }
> > }
> > - /* 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: {
>
> --
> 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)
