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