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

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);

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)