Re: [PATCH v4 0/3] scsi: target: iscsi: fix stalled and doubly completed aborted WRITEs

[Bart Van Assche]

On 10/26/26 8:07 AM, Laurence Oberman wrote:
> On moving the one-shot guard into target core:
>
> This will be explored later on but is not in scope for this series; it
> changes the path every fabric completes through and I have no way to test
> easily in iSER, FC or SRP right now.

I don't think that argument holds. Fabric drivers that complete an
aborted command exactly once never trigger a one-shot guard in
target_cmd_interrupted(), so their behavior wouldn't change. The guard
can be a new transport_state bit that is tested and set under
t_state_lock, and the CMD_T_ABORTED test can be moved under that lock
too. Today target_cmd_interrupted() reads transport_state without
holding any lock. That would:
- make the iSCSI-private completion_claimed member and both new helpers
  unnecessary;
- close the race that patch 3 only narrows (CMD_T_ABORTED being set
  between the iSCSI test and target_cmd_interrupted());
- also cover the sites this series doesn't touch, e.g. the deferred
  ICF_OOO_CMDSN path where iscsit_execute_ooo_cmdsns() ends up calling
  target_execute_cmd() for an aborted command without taking the claim.

Please consider doing this instead of adding iSCSI-specific
bookkeeping. If the iSCSI-only approach is kept for stable, please
explain in the cover letter why the core approach isn't suitable.

Patch 1/3:

> Fixes: aaa00cc93c1d ("scsi: target/core: Fix TAS handling for aborted 
commands")

transport_check_aborted_status() only sent the TASK ABORTED status. It
didn't drop the reference that the abort path now waits on. Nothing
waited in target_put_cmd_and_wait() until commit 2c9fa49e100f
("scsi: target/core: Make ABORT and LUN RESET handling synchronous").
I think that's the commit this patch fixes, or at least it should be
mentioned too. Both commits went into v5.0, so stable coverage doesn't
change.

> +bool iscsit_claim_completion_locked(struct iscsit_cmd *cmd)
> +{
> +	lockdep_assert_held(&cmd->se_cmd.t_state_lock);

Please also add __must_hold(&cmd->se_cmd.t_state_lock) to the
declaration so that Clang's context analysis can check this at compile
time.

> +/* Complete an aborted command at most once.  Must not hold 
t_state_lock. */
> +void iscsit_complete_aborted_cmd(struct iscsit_cmd *cmd)

Please change that comment into a __must_not_hold() annotation.

> +	spin_lock_irqsave(&se_cmd->t_state_lock, flags);

All callers run in process context with interrupts enabled, and
iscsit_release_commands_from_conn() uses spin_lock_irq() on the same
lock. spin_lock_irq() is enough here.

> +	bool			completion_claimed;

Please add __guarded_by(&se_cmd.t_state_lock) instead of just stating
in a comment which lock protects this member.

Patch 2/3: looks fine to me, assuming the series stays as it is.

Patch 3/3: as explained above, this patch narrows the race but doesn't
close it, and the ooo_cmdsn branch right above the new code still
defers to a path that doesn't take the claim. I'd rather see this
solved in target_cmd_interrupted() than merge a partial fix that will
be replaced later anyway.

Thanks,

Bart.