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

[Laurence Oberman <loberman@redhat.com>]

On Mon, 2026-10-26 at 08:24 -0700, Bart Van Assche wrote:
> 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.

Hi Bart



You are right, I apologize. A guard that only fires on a
second completion is a no-op for any fabric that completes once, so
there is nothing for iSER, FC or SRP to regress. I will do it in
target_cmd_interrupted() instead.

That makes v5 two patches: the core guard, and the iscsit_execute_cmd()
completion that is the actual hang fix, with no completion_claimed
member and no helpers. v4 patches 2 and 3 are then unnecessary, since a
second target_complete_cmd() becomes harmless wherever it comes from,
and it also picks up the ICF_OOO_CMDSN path you mention, which I could
not reach from the fabric side at all.

One design question before I write it. target_cmd_interrupted() has two
branches:

	if (cmd->transport_state & CMD_T_ABORTED) {
		...
		INIT_WORK(&cmd->work, target_abort_work);
		queue_work(target_completion_wq, &cmd->work);
		return true;
	} else if (cmd->transport_state & CMD_T_STOP) {
		...
		complete_all(&cmd->t_transport_stop_comp);
		return true;
	}

If the guard is one-shot across both, a second call that would have
taken the CMD_T_STOP branch is suppressed too, and I am not sure that
is safe --
a suppressed complete_all() could leave a waiter in
transport_wait_for_tasks() stuck. Should the guard cover only the
CMD_T_ABORTED branch, or do you see a reason it is safe across both?

 > I think that's the commit this patch fixes, or at least it should be
 > mentioned too.

Agreed. transport_check_aborted_status() sent the status but nothing
waited on the reference until 2c9fa49e100f, so that is what turned this
into a hang. v5 will carry:

  Fixes: 2c9fa49e100f ("scsi: target/core: Make ABORT and LUN RESET
handling synchronous")

and explain aaa00cc93c1d in the body as what left the site with nothing
to complete the command. Both are in v5.0 so the stable range is
unchanged.

On the annotations: thank you, I had not come across them. They are in
include/linux/compiler-context-analysis.h and I can see how
drivers/nvme/host uses them. They apply to completion_claimed and the
two
helpers, which the core approach removes, so there may be nothing left
to
annotate -- but if any of that survives in v5 it will carry
__must_hold()/__must_not_hold()/__guarded_by() rather than comments,
and
spin_lock_irq() rather than spin_lock_irqsave(), which is right for the
process-context callers.

Worth saying plainly: the core guard will be new code with no field
evidence behind it. The 300 reset cycles validated completing the
command in iscsit_execute_cmd(); the claim was only ever there to stop
a double completion that an earlier version of mine had caused. Moving
that guard into target core does not change the hang fix, but it does
mean the guard itself arrives untested, and I would rather say so than
imply otherwise.

The reporter is covered on a distro backport in the meantime, so there
is no schedule pressure on this from my side.

I will post v5 once you have had a chance to answer the CMD_T_STOP
question.

Thanks for the thorough review.

Laurence