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

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

[Laurence Oberman]

These three patches apply on top of commit d5869dae5080 ("scsi: target: iscsi: Fix hang for aborted WRITE_PENDING commands").

A WRITE command that is still in TRANSPORT_WRITE_PENDING has never been
passed to the backend, so when a TMR aborts it the iSCSI fabric is the one
that has to complete it. That commit fixed the case where the final
DataOut PDU for an already-issued R2T arrives after the abort. Two more
paths can leave such a command with nothing to complete it, or let it be
completed twice:

 - Patch 1 is the hang this work started from. A WRITE that arrives with
   immediate data, aborted after that data has been received but before
   the first R2T is built, is never completed by anything: no R2T has been
   sent so the initiator cannot send more data, and no DataOut timer is
   armed so the ERL=0 timeout cannot fire either. core_tmr_lun_reset()
   then blocks for as long as the connection stays up.

 - Patches 2 and 3 fix two existing sites that complete an aborted
   WRITE_PENDING command without recording that they have done so, and
   which connection teardown can therefore complete a second time.

Patch 1 also adds a one-shot completion claim. target_complete_cmd() on a
command that already has CMD_T_ABORTED set goes into
target_cmd_interrupted(), which queues target_abort_work and returns
without setting CMD_T_COMPLETE and without changing t_state, so target
core records nothing that would stop a second completion. The command
stays in TRANSPORT_WRITE_PENDING and on conn_cmd_list until
target_abort_work reaches iscsit_aborted_task(), and
iscsit_release_commands_from_conn() completes anything it finds in
TRANSPORT_WRITE_PENDING. The claim is what makes exactly one site
complete the command.

Changes since v3:
 - Dropped the patch that completed an aborted WRITE in the
   DATAOUT_SEND_R2T branch of iscsit_check_dataout_payload(). The review
   of v3 pointed out that the adjacent DATAOUT_SEND_TO_TRANSPORT branch
   deliberately checks ICF_OOO_CMDSN and defers, and the dropped patch did
   not: it could complete and free a command still reachable by raw
   pointer from sess_ooo_cmdsn_list, which iscsit_execute_ooo_cmdsns()
   dereferences later. Simply mirroring the neighbour is not enough
   either, because a command with ICF_NON_IMMEDIATE_UNSOLICITED_DATA set
   falls through iscsit_execute_cmd() without being completed when the
   CmdSN gap is filled. That window needs more thought than this series
   should carry, and it was never observed in the field, so it will come
   back on its own.
 - Patches 1 to 3 are unchanged from v3 patches 1, 3 and 4 respectively.

v3: https://lore.kernel.org/linux-scsi/20261026124344.745124-1-loberman@redhat.com/
v2: https://lore.kernel.org/linux-scsi/20261016175351.126373-1-loberman@redhat.com/
v1: https://lore.kernel.org/linux-scsi/20261014143932.39453-1-loberman@redhat.com/

Two other findings against v3 are pre-existing rather than introduced
here, and I am reporting them separately rather than widening this series
again:

iscsit_get_immediate_data() reads cmd->sense_reason and
cmd->unsolicited_data after iscsit_sequence_cmd() returns, holding no
reference, and target_handle_abort() drops the final reference for an
aborted command. Patch 1 adds a path where that can happen, but so does
the branch directly above it: iscsit_handle_immediate_data() sets
ICF_GOT_LAST_DATAOUT when all of the command's data arrives as immediate
data, and iscsit_execute_cmd() then calls target_execute_cmd() from the
same caller with the same abort states. Patch 1 adds a third path with a
property two existing paths already have.

In patch 3, CMD_T_ABORTED can be set between the check and
target_cmd_interrupted() inside target_execute_cmd(), leaving the
completion unclaimed. That is also what happens today for every aborted
command on that path; the patch narrows the window rather than closing it.

A correction to something I said during the v2 review, in case it is
relied on: I wrote that if the outstanding_r2ts read in the dropped patch
was stale low, "the later completion in __iscsit_check_dataout_hdr()
becomes a no-op". That was wrong -- before patch 2 of this series that
site called target_complete_cmd() directly and never consulted the claim.

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.

How this was found and tested:

The hang was reported against an Oracle RAC cluster on two-session iSCSI
LIO targets, where one node's LUN_RESET aborts WRITEs belonging to the
other node's session. It was not reproducible in a lab, so the diagnosis
came from instrumented kernels in the reporter's environment, logging
every site that completes or waits on an aborted command.

Across two hosts every stall had the identical signature given in patch 1,
and the stall only ever ended when the initiator gave up and dropped the
connection. 217 of 222 LUN_RESET drains completed in under a second; the
rest took 46 to 54 seconds, which is where the hung-task panics came from.
One 53.7 second stall recovered on its own and the host ran for a further
19 hours.

An earlier attempt at this fix completed the command from
core_tmr_drain_state_list() instead. That is not safe: at that point the
initiator may still be sending DataOut for the command, and the result was
a double completion -- re-queued se_cmd->work, list_del corruption and a
refcount underflow. Mentioning it in case the same idea comes up in
review.

What is and is not tested:

Patch 1, backported to a 5.14-based distro kernel, has run 300 LUN_RESET
cycles with no hang, in two independent runs: one instrumented and one
with the tracing removed. The same reproducer previously failed at 95 and
196 cycles. The instrumented kernels also carried the dropped
DATAOUT_SEND_R2T patch, and recorded that its site fired zero times in
either run, so removing it does not change anything those runs exercised.

Patches 2 and 3 are closed by inspection. Patch 2's site did run during
those tests, but by way of the unclaimed target_complete_cmd() it
replaces; when the claim succeeds the two are the same two calls in the
same order, so the tested behaviour is preserved. Patch 3 leaves the
non-aborted path through target_execute_cmd() untouched.

The reporter's environment is a production cluster and is the only place
this has ever reproduced, so I cannot offer an upstream -rc run from
there. A build of this series is going to them for a further 300-cycle run
and I will report back. If that is a blocker for patches 2 and 3, I would
rather you took patch 1 on its own than held the series.

Laurence Oberman (3):
  scsi: target: iscsi: Complete an aborted WRITE stuck before the first
    R2T
  scsi: target: iscsi: Claim the completion in
    __iscsit_check_dataout_hdr()
  scsi: target: iscsi: Claim the completion on the DataOut submit path

 drivers/target/iscsi/iscsi_target.c      | 28 +++++++++++----
 drivers/target/iscsi/iscsi_target_erl1.c | 10 +++++-
 drivers/target/iscsi/iscsi_target_util.c | 46 ++++++++++++++++++++++++
 drivers/target/iscsi/iscsi_target_util.h |  2 ++
 include/target/iscsi/iscsi_target_core.h |  6 ++++
 5 files changed, 85 insertions(+), 7 deletions(-)

-- 
2.43.0

[PATCH v4 1/3] scsi: target: iscsi: Complete an aborted WRITE stuck before the first R2T

[Laurence Oberman]

A WRITE that arrives with immediate data, and whose data length exceeds
FirstBurstLength, is left in TRANSPORT_WRITE_PENDING by
iscsit_execute_cmd() until the first R2T is built. If a LUN_RESET or
ABORT_TASK from another session sets CMD_T_ABORTED in that window,
iscsit_execute_cmd() returns without sending the R2T.

Nothing then completes the command. No R2T has been sent, so the
initiator cannot legally send more data; no DataOut timer was armed, so
the ERL=0 timeout cannot fire either. The command stays in
TRANSPORT_WRITE_PENDING with CMD_T_ABORTED set, and
core_tmr_drain_state_list() blocks in target_put_cmd_and_wait() on it for
as long as the connection stays up:

  INFO: task kworker/5:2 blocked for more than 46 seconds.
   target_put_cmd_and_wait+0x6d/0xb0 [target_core_mod]
   core_tmr_drain_state_list+0x2dc/0x380 [target_core_mod]
   core_tmr_lun_reset+0x93/0x1a0 [target_core_mod]

Commit aaa00cc93c1d ("scsi: target/core: Fix TAS handling for
aborted commands") is what left it that way. Before it, this site
called transport_check_aborted_status(), which sent the delayed
TASK_ABORTED status for the command; that commit replaced the call
with a bare CMD_T_ABORTED test and a return, on the basis that the
target core would send TASK_ABORTED from the ABORT TMF context
instead. That holds only for commands the core can finish on its
own. A WRITE still in TRANSPORT_WRITE_PENDING has never been passed
to the backend, so the core's abort path is itself waiting in
target_put_cmd_and_wait() for the fabric to complete it, and since
that commit the fabric no longer does.

Complete it in iscsit_execute_cmd() instead. No DataOut path can race
with this: without an R2T the initiator cannot send further data for the
command, and the branch is only reached when no unsolicited DataOut is
expected either.

That introduces a second site which can complete a command that
iscsit_release_commands_from_conn() would also complete during connection
teardown, because t_state is still TRANSPORT_WRITE_PENDING there.
target_complete_cmd() on a command that already has CMD_T_ABORTED set
goes straight into target_cmd_interrupted(), which queues
target_abort_work and returns without setting CMD_T_COMPLETE and without
changing t_state, so nothing in target core records that a completion has
happened. A second target_complete_cmd() re-initialises and re-queues
se_cmd->work while it is still queued and drops a command reference that
was never taken. Add a one-shot claim, taken under se_cmd->t_state_lock,
so that only one of the two sites completes the command.

This was reported against an Oracle RAC cluster on two-session iSCSI LIO
targets, where one node's LUN_RESET aborts WRITEs belonging to the other
node's session. It was not reproducible in a lab, so the diagnosis came
from instrumented kernels in the reporter's environment. Across two
hosts, every stalled command had the same signature: DMA_TO_DEVICE,
TRANSPORT_WRITE_PENDING, CMD_T_ABORTED | CMD_T_TAS, outstanding_r2ts == 0,
write_data_done == first_burst_len, and an empty cmd_r2t_list. 217 of 222
LUN_RESET drains completed in under a second; the rest took 46 to 54
seconds, which is where the hung-task panics came from. One 53.7 second
stall recovered on its own and the host ran for a further 19 hours. The
stall only ever ended when the initiator gave up and dropped the
connection, which let iscsit_release_commands_from_conn() complete the
command.

Fixes: aaa00cc93c1d ("scsi: target/core: Fix TAS handling for aborted commands")
Cc: stable@vger.kernel.org
Signed-off-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/target/iscsi/iscsi_target.c      | 12 +++++--
 drivers/target/iscsi/iscsi_target_erl1.c | 10 +++++-
 drivers/target/iscsi/iscsi_target_util.c | 46 ++++++++++++++++++++++++
 drivers/target/iscsi/iscsi_target_util.h |  2 ++
 include/target/iscsi/iscsi_target_core.h |  6 ++++
 5 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 124ff26..aab9237 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -4198,10 +4198,18 @@ static void iscsit_release_commands_from_conn(struct iscsit_conn *conn)
 		if (cmd->se_cmd.t_state == TRANSPORT_WRITE_PENDING) {
 			/*
 			 * We never submitted the cmd to LIO core, so we have
-			 * to tell LIO to perform the completion process.
+			 * to tell LIO to perform the completion process --
+			 * unless iscsit_execute_cmd() already did.  t_state
+			 * stays TRANSPORT_WRITE_PENDING across that
+			 * completion, so the claim is the only thing that can
+			 * tell us.
 			 */
+			bool claimed = iscsit_claim_completion_locked(cmd);
+
 			spin_unlock_irq(&se_cmd->t_state_lock);
-			target_complete_cmd(&cmd->se_cmd, SAM_STAT_TASK_ABORTED);
+			if (claimed)
+				target_complete_cmd(&cmd->se_cmd,
+						    SAM_STAT_TASK_ABORTED);
 			continue;
 		}
 		spin_unlock_irq(&se_cmd->t_state_lock);
diff --git a/drivers/target/iscsi/iscsi_target_erl1.c b/drivers/target/iscsi/iscsi_target_erl1.c
index d8ca06e..cf72c79 100644
--- a/drivers/target/iscsi/iscsi_target_erl1.c
+++ b/drivers/target/iscsi/iscsi_target_erl1.c
@@ -927,8 +927,16 @@ int iscsit_execute_cmd(struct iscsit_cmd *cmd, int ooo)
 
 			if (!(cmd->cmd_flags &
 					ICF_NON_IMMEDIATE_UNSOLICITED_DATA)) {
-				if (cmd->se_cmd.transport_state & CMD_T_ABORTED)
+				/*
+				 * No R2T has been sent for this command yet,
+				 * so returning here leaves the initiator with
+				 * nothing to send and no DataOut timer armed.
+				 * Complete it, or nothing ever will.
+				 */
+				if (cmd->se_cmd.transport_state & CMD_T_ABORTED) {
+					iscsit_complete_aborted_cmd(cmd);
 					return 0;
+				}
 
 				iscsit_set_dataout_sequence_values(cmd);
 				conn->conn_transport->iscsit_get_dataout(conn, cmd, false);
diff --git a/drivers/target/iscsi/iscsi_target_util.c b/drivers/target/iscsi/iscsi_target_util.c
index c1888c4..31248cb 100644
--- a/drivers/target/iscsi/iscsi_target_util.c
+++ b/drivers/target/iscsi/iscsi_target_util.c
@@ -14,6 +14,7 @@
 #include <scsi/scsi_tcq.h>
 #include <scsi/iscsi_proto.h>
 #include <target/target_core_base.h>
+#include <target/target_core_backend.h>
 #include <target/target_core_fabric.h>
 #include <target/iscsi/iscsi_transport.h>
 
@@ -186,6 +187,51 @@ struct iscsit_cmd *iscsit_allocate_cmd(struct iscsit_conn *conn, int state)
 }
 EXPORT_SYMBOL(iscsit_allocate_cmd);
 
+/*
+ * A WRITE command still in TRANSPORT_WRITE_PENDING has never been passed to
+ * the backend, so when a TMR aborts it the fabric has to complete it.  More
+ * than one site can reach that conclusion for the same command, and
+ * target_complete_cmd() on a command that already has CMD_T_ABORTED set
+ * neither sets CMD_T_COMPLETE nor changes t_state, so target core keeps no
+ * state that would stop a second completion.  Serialise them here: whichever
+ * site claims the completion performs it and every other site leaves the
+ * command alone.
+ *
+ * Caller must hold cmd->se_cmd.t_state_lock.
+ */
+bool iscsit_claim_completion_locked(struct iscsit_cmd *cmd)
+{
+	lockdep_assert_held(&cmd->se_cmd.t_state_lock);
+
+	if (cmd->completion_claimed)
+		return false;
+
+	cmd->completion_claimed = true;
+	return true;
+}
+
+/* Complete an aborted command at most once.  Must not hold t_state_lock. */
+void iscsit_complete_aborted_cmd(struct iscsit_cmd *cmd)
+{
+	struct se_cmd *se_cmd = &cmd->se_cmd;
+	unsigned long flags;
+	bool claimed;
+
+	spin_lock_irqsave(&se_cmd->t_state_lock, flags);
+	claimed = iscsit_claim_completion_locked(cmd);
+	spin_unlock_irqrestore(&se_cmd->t_state_lock, flags);
+
+	if (!claimed)
+		return;
+
+	/*
+	 * No-op unless a DataOut timer was armed for this command; callers
+	 * that complete before the first R2T have none.
+	 */
+	iscsit_stop_dataout_timer(cmd);
+	target_complete_cmd(se_cmd, SAM_STAT_TASK_ABORTED);
+}
+
 struct iscsi_seq *iscsit_get_seq_holder_for_datain(
 	struct iscsit_cmd *cmd,
 	u32 seq_send_order)
diff --git a/drivers/target/iscsi/iscsi_target_util.h b/drivers/target/iscsi/iscsi_target_util.h
index 7ae48a8..8f3fe74 100644
--- a/drivers/target/iscsi/iscsi_target_util.h
+++ b/drivers/target/iscsi/iscsi_target_util.h
@@ -18,6 +18,8 @@ extern struct iscsi_r2t *iscsit_get_r2t_from_list(struct iscsit_cmd *);
 extern void iscsit_free_r2t(struct iscsi_r2t *, struct iscsit_cmd *);
 extern void iscsit_free_r2ts_from_list(struct iscsit_cmd *);
 extern struct iscsit_cmd *iscsit_allocate_cmd(struct iscsit_conn *, int);
+extern bool iscsit_claim_completion_locked(struct iscsit_cmd *cmd);
+extern void iscsit_complete_aborted_cmd(struct iscsit_cmd *cmd);
 extern struct iscsi_seq *iscsit_get_seq_holder_for_datain(struct iscsit_cmd *, u32);
 extern struct iscsi_seq *iscsit_get_seq_holder_for_r2t(struct iscsit_cmd *);
 extern struct iscsi_r2t *iscsit_get_holder_for_r2tsn(struct iscsit_cmd *, u32);
diff --git a/include/target/iscsi/iscsi_target_core.h b/include/target/iscsi/iscsi_target_core.h
index 51ca80a..5eb3cf2 100644
--- a/include/target/iscsi/iscsi_target_core.h
+++ b/include/target/iscsi/iscsi_target_core.h
@@ -496,6 +496,12 @@ struct iscsit_cmd {
 	u32			first_data_sg_off;
 	u32			kmapped_nents;
 	sense_reason_t		sense_reason;
+	/*
+	 * Set once one site has claimed the completion of this command.
+	 * The claim is taken before target_complete_cmd() is called.
+	 * Protected by se_cmd.t_state_lock.
+	 */
+	bool			completion_claimed;
 }  ____cacheline_aligned;
 
 struct iscsi_tmr_req {
-- 
2.43.0

[PATCH v4 2/3] scsi: target: iscsi: Claim the completion in __iscsit_check_dataout_hdr()

[Laurence Oberman]

Commit d5869dae5080 ("scsi: target: iscsi: Fix hang for aborted
WRITE_PENDING commands") made __iscsit_check_dataout_hdr() complete an
aborted WRITE once the final DataOut PDU for the last outstanding R2T has
been received. That call records nothing to say the command has been
completed.

target_complete_cmd() on a command that already has CMD_T_ABORTED set goes
to target_cmd_interrupted(), which queues target_abort_work and returns
without setting CMD_T_COMPLETE and without changing t_state. The command
therefore stays in TRANSPORT_WRITE_PENDING and on conn_cmd_list until
target_abort_work reaches iscsit_aborted_task(). If the connection drops in
that window, iscsit_release_commands_from_conn() finds the command still in
TRANSPORT_WRITE_PENDING and completes it a second time, which
re-initialises and re-queues an se_cmd->work that is already queued.

Route this site through iscsit_complete_aborted_cmd() so that the
completion is claimed and connection teardown leaves the command alone. The
helper makes the same iscsit_stop_dataout_timer() and target_complete_cmd()
calls when it takes the claim, so the completing path is unchanged.

Fixes: d5869dae5080 ("scsi: target: iscsi: Fix hang for aborted WRITE_PENDING commands")
Cc: stable@vger.kernel.org
Signed-off-by: Laurence Oberman <loberman@redhat.com>
---

Notes:
    This depends on patch 1 of this series, which adds
    iscsit_complete_aborted_cmd().  d5869dae5080 has been backported to 5.10.y,
    5.15.y, 6.1.y, 6.6.y, 6.12.y, 6.18.y and 7.2.y, none of which carry the
    helper, so a backport to those branches needs patch 1 as well, or the claim
    open-coded.  Happy to send a standalone version for stable if you would
    rather not carry patch 1 there.

 drivers/target/iscsi/iscsi_target.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index aab9237..1db6051 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1533,10 +1533,8 @@ __iscsit_check_dataout_hdr(struct iscsit_conn *conn, void *buf,
 		 */
 		if (se_cmd->transport_state & CMD_T_ABORTED) {
 			if (hdr->flags & ISCSI_FLAG_CMD_FINAL &&
-			    --cmd->outstanding_r2ts < 1) {
-				iscsit_stop_dataout_timer(cmd);
-				target_complete_cmd(se_cmd, SAM_STAT_TASK_ABORTED);
-			}
+			    --cmd->outstanding_r2ts < 1)
+				iscsit_complete_aborted_cmd(cmd);
 
 			return iscsit_dump_data_payload(conn, payload_length, 1);
 		}
-- 
2.43.0

[PATCH v4 3/3] scsi: target: iscsi: Claim the completion on the DataOut submit path

[Laurence Oberman]

When the final DataOut PDU for a WRITE has been received,
iscsit_check_dataout_payload() submits the command with
target_execute_cmd(). For a command that has already been aborted,
target_execute_cmd() calls target_cmd_interrupted() before it touches
t_state, so it queues target_abort_work and returns with the command still
in TRANSPORT_WRITE_PENDING and still on conn_cmd_list, recording nothing to
say that a completion has been started.

A connection drop before target_abort_work reaches iscsit_aborted_task()
then lets iscsit_release_commands_from_conn() complete the command a second
time.

Complete aborted commands through iscsit_complete_aborted_cmd() rather than
handing them to target_execute_cmd(). Both reach target_cmd_interrupted()
and queue target_abort_work; the work target_execute_cmd() does after that
test is not reached for an aborted command either way. The dataout timer
has already been stopped a few lines above, so the helper's call to
iscsit_stop_dataout_timer() is a no-op here.

Fixes: ea87981a0ee8 ("scsi: target: iscsi: Handle abort for WRITE_PENDING cmds")
Signed-off-by: Laurence Oberman <loberman@redhat.com>
---

Notes:
    Not observed in the field; closed by inspection after the review of v2
    pointed out that this site has the same shape as the one in
    __iscsit_check_dataout_hdr().  I have deliberately left Cc: stable off,
    since the window predates d5869dae5080 and has never been seen -- I would
    rather it sat in mainline for a cycle first.
    
    The review of v3 noted that CMD_T_ABORTED can still be set between this
    check and target_cmd_interrupted() inside target_execute_cmd(), leaving the
    completion unclaimed.  That is true, and it is the pre-existing behaviour
    for every aborted command on this path today; this patch narrows the window
    rather than closing it.  Closing it properly needs the one-shot guard to
    live in target_cmd_interrupted() under t_state_lock, which is the same lock
    __target_check_io_state() sets CMD_T_ABORTED under.  See the cover letter.
    
    Happy to drop this patch if you would prefer the series to stop at patch 2.

 drivers/target/iscsi/iscsi_target.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 1db6051..9341565 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1683,6 +1683,16 @@ iscsit_check_dataout_payload(struct iscsit_cmd *cmd, struct iscsi_data *hdr,
 		iscsit_stop_dataout_timer(cmd);
 		if (ooo_cmdsn)
 			return 0;
+		/*
+		 * target_execute_cmd() tests CMD_T_ABORTED before it touches
+		 * t_state, so for an aborted command it would queue
+		 * target_abort_work and return with the command still in
+		 * TRANSPORT_WRITE_PENDING and the completion unclaimed.
+		 */
+		if (cmd->se_cmd.transport_state & CMD_T_ABORTED) {
+			iscsit_complete_aborted_cmd(cmd);
+			return 0;
+		}
 		target_execute_cmd(&cmd->se_cmd);
 		return 0;
 	} else /* DATAOUT_CANNOT_RECOVER */
-- 
2.43.0

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.

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

[Laurence Oberman]

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

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

[Bart Van Assche]

On 10/26/26 9:12 AM, Laurence Oberman wrote:
 > 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?

Please limit the guard to the CMD_T_ABORTED branch. The only things in
target_cmd_interrupted() that must not run twice are in that branch:
INIT_WORK() + queue_work() on cmd->work, and the
transport_complete_callback() call. For COMPARE AND WRITE the latter
releases dev->caw_sem in compare_and_write_callback(), so calling it
twice would release that semaphore twice.

The CMD_T_STOP branch doesn't need a guard. complete_all() is
idempotent and t_transport_stop_comp is initialized only once, in
__target_init_cmd(). Suppressing a repeated complete_all() wouldn't
strand a waiter either, since the completion stays done. However, a
guard shared by both branches would be wrong for another reason: if
the first call takes the CMD_T_STOP branch and CMD_T_ABORTED is set
afterwards, a shared guard would suppress queueing target_abort_work
and we would be back to the target_put_cmd_and_wait() hang.

Something like this (untested):

	static bool target_cmd_interrupted(struct se_cmd *cmd)
	{
		bool queue_abort = false, stop = false;
		unsigned long flags;
		int post_ret;

		spin_lock_irqsave(&cmd->t_state_lock, flags);
		if (cmd->transport_state & CMD_T_ABORTED) {
			if (cmd->transport_state & CMD_T_ABORT_QUEUED) {
				spin_unlock_irqrestore(&cmd->t_state_lock,
						       flags);
				return true;
			}
			cmd->transport_state |= CMD_T_ABORT_QUEUED;
			queue_abort = true;
		} else if (cmd->transport_state & CMD_T_STOP) {
			stop = true;
		}
		spin_unlock_irqrestore(&cmd->t_state_lock, flags);

		if (!queue_abort && !stop)
			return false;
		if (cmd->transport_complete_callback)
			cmd->transport_complete_callback(cmd, false,
							 &post_ret);
		if (queue_abort) {
			INIT_WORK(&cmd->work, target_abort_work);
			queue_work(target_completion_wq, &cmd->work);
		} else {
			complete_all(&cmd->t_transport_stop_comp);
		}
		return true;
	}

Notes:
- Please use spin_lock_irqsave(): target_complete_cmd() may be called by
   backend drivers from interrupt context.
- Call the callback and queue_work() after unlocking t_state_lock,
   like the current code does.
- A repeated call in the aborted case must still return true so that
   callers don't proceed with normal completion.
- Please describe in the patch description that the second call is a
   no-op by design and that fabrics that complete once are unaffected.

Thanks,

Bart.

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

[Laurence Oberman]

On Mon, 2026-10-26 at 09:47 -0700, Bart Van Assche wrote:
> On 10/26/26 9:12 AM, Laurence Oberman wrote:
>  > 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?
> 
> Please limit the guard to the CMD_T_ABORTED branch. The only things
> in
> target_cmd_interrupted() that must not run twice are in that branch:
> INIT_WORK() + queue_work() on cmd->work, and the
> transport_complete_callback() call. For COMPARE AND WRITE the latter
> releases dev->caw_sem in compare_and_write_callback(), so calling it
> twice would release that semaphore twice.
> 
> The CMD_T_STOP branch doesn't need a guard. complete_all() is
> idempotent and t_transport_stop_comp is initialized only once, in
> __target_init_cmd(). Suppressing a repeated complete_all() wouldn't
> strand a waiter either, since the completion stays done. However, a
> guard shared by both branches would be wrong for another reason: if
> the first call takes the CMD_T_STOP branch and CMD_T_ABORTED is set
> afterwards, a shared guard would suppress queueing target_abort_work
> and we would be back to the target_put_cmd_and_wait() hang.
> 
> Something like this (untested):
> 
> 	static bool target_cmd_interrupted(struct se_cmd *cmd)
> 	{
> 		bool queue_abort = false, stop = false;
> 		unsigned long flags;
> 		int post_ret;
> 
> 		spin_lock_irqsave(&cmd->t_state_lock, flags);
> 		if (cmd->transport_state & CMD_T_ABORTED) {
> 			if (cmd->transport_state &
> CMD_T_ABORT_QUEUED) {
> 				spin_unlock_irqrestore(&cmd-
> >t_state_lock,
> 						       flags);
> 				return true;
> 			}
> 			cmd->transport_state |= CMD_T_ABORT_QUEUED;
> 			queue_abort = true;
> 		} else if (cmd->transport_state & CMD_T_STOP) {
> 			stop = true;
> 		}
> 		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
> 
> 		if (!queue_abort && !stop)
> 			return false;
> 		if (cmd->transport_complete_callback)
> 			cmd->transport_complete_callback(cmd, false,
> 							 &post_ret);
> 		if (queue_abort) {
> 			INIT_WORK(&cmd->work, target_abort_work);
> 			queue_work(target_completion_wq, &cmd-
> >work);
> 		} else {
> 			complete_all(&cmd->t_transport_stop_comp);
> 		}
> 		return true;
> 	}
> 
> Notes:
> - Please use spin_lock_irqsave(): target_complete_cmd() may be called
> by
>    backend drivers from interrupt context.
> - Call the callback and queue_work() after unlocking t_state_lock,
>    like the current code does.
> - A repeated call in the aborted case must still return true so that
>    callers don't proceed with normal completion.
> - Please describe in the patch description that the second call is a
>    no-op by design and that fabrics that complete once are
> unaffected.
> 
> Thanks,
> 
> Bart.

Hello Bart, 
You always take the time to help me and I appreciate it very much.
I am following up shortly with a V5 that I think addresses your
concerns and also limits this to only two patches. 
I am back-porting to RHEL9 and will get it tested the same way there
too but I expect it will be fine.

Thank you
Laurence