Re: [PATCH] libmpathpersist: fix self-preemption when key has no active reservation

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,
> 				    &paramp, 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)