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

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)