[PATCH v6 0/4] scsi: target: fix LUN_RESET hang on aborted iSCSI WRITEs

[PATCH v6 0/4] scsi: target: fix LUN_RESET hang on aborted iSCSI WRITEs

[Laurence Oberman]

These four patches fix a LUN_RESET hang on iSCSI targets, and two
pre-existing bugs that fixing it exposes.

I had LLM help in generating and reviewing these and suggestions to 
change them, as well as multiple sashiko reviews.
While the initial 2 patches were fully tested at a customer to fix the
hangs, they needed changes posted now here to be accepted.

A WRITE command that is still in TRANSPORT_WRITE_PENDING has never been
passed to the backend, so when a TMR aborts it the fabric is the one that
has to complete it. Patch 4 completes it at the one site where nothing
does today. Patches 1 to 3 make that safe, and all three fix problems that are
reachable without patch 4.

 - Patch 1 stops compare_and_write_post() re-enabling interrupts in a
   caller that had them disabled. target_complete_cmd() may be called
   from interrupt context, and for an aborted command it reaches that
   callback, which used spin_lock_irq()/spin_unlock_irq().

 - Patch 2 stops COMPARE AND WRITE releasing dev->caw_sem when it never
   took it. The callback is installed in sbc_parse_cdb() and the semaphore
   is taken much later in sbc_compare_and_write(), so a command aborted in
   between releases a semaphore it does not hold, which lets two COMPARE
   AND WRITE commands run against a device at once.

 - Patch 3 makes target_abort_work be queued at most once per command, at
   both sites in target_core_transport.c that queue it. Without it a
   second call re-queues an se_cmd->work that is already queued.

 - Patch 4 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.

Changes since v5, all from Bart Van Assche's review and the Sashiko
review of v5:
 - New patch 1 converts compare_and_write_post() to
   spin_lock_irqsave(). Pre-existing, but patch 2 modifies that function
   and leaving spin_lock_irq() a few lines above a new
   spin_lock_irqsave() would be confusing. compare_and_write_callback()
   has the same pattern on its success path, which is only reached from
   target_complete_ok_work() in process context, so it is left alone.
 - New patch 2 for caw_sem, as Bart proposed: track whether the semaphore
   is held rather than inferring it. This also covers the case the two
   options I had offered did not -- a release of a semaphore that was
   never taken -- and it makes the one side effect of running the
   completion callback twice idempotent, so no separate guard for the
   callback is needed. COMPARE AND WRITE's is the only
   transport_complete_callback in the tree.
 - Patch 3 now covers transport_generic_request_failure() as well. v5
   guarded only target_cmd_interrupted() and left the file's other
   target_abort_work queueing site untouched, so the claim did not
   actually hold. The Sashiko review of v5 caught it.
 - Patch 3 keeps a lockless test of CMD_T_ABORTED and CMD_T_STOP before
   taking t_state_lock, since it runs for every completion.
 - Patch 3 has Cc: stable, since patch 4 depends on it.
 - __target_claim_abort() and sbc_caw_sem_release() carry __must_hold()
   and __must_not_hold(). drivers/target is not opted in to context
   analysis (no CONTEXT_ANALYSIS := y in its Makefile), so these are
   documentation for now rather than checked; turning the opt-in on for
   the whole subsystem is not something this series should carry.
 - Patch 4 no longer calls iscsit_stop_dataout_timer(), which contradicted
   its own comment. The timer is armed only in
   iscsit_set_unsolicited_dataout() and iscsit_send_r2t(), and this branch
   is reached before either, so there was nothing to stop.
 - Patch 4 explains why the other two early returns for aborted commands
   in iscsit_execute_cmd() cannot hang: both TMR paths iterate
   dev->queues[].state_list, a command only reaches that list via
   transport_generic_new_cmd(), and for those two returns
   iscsit_execute_cmd() has run before it. Only the immediate-data path
   reaches the list first, which is why it is the one that strands a TMR.
 - Patch 3's subject is "Queue the abort of a command at most once" rather
   than "Complete ... at most once", which is what it guarantees.

v5: https://lore.kernel.org/linux-scsi/20261026192835.1431767-1-loberman@redhat.com/
v4: https://lore.kernel.org/linux-scsi/20261026150722.823941-1-loberman@redhat.com/
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/

Patches 1 to 3 carry no Fixes: tag. For patches 1 and 2 the behaviour is
as old as COMPARE AND WRITE itself; for patch 3, the commit
that made ABORT and LUN RESET synchronous added the same abort-queueing
pattern to both functions, but the double queueing is not what it set out
to do. All are marked for stable regardless, since patch 4 depends on patch 3
and patch 3's reasoning depends on patch 2. Please say which
commits you would like named and I will add them.

How this was found:

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 4,
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. That is the failure patch 3 now prevents outright.

What is and is not tested:

Patch 4's change has run 300 LUN_RESET cycles with no hang, in two
independent runs on a 5.14-based distro kernel, one instrumented and one
with the tracing removed. The same reproducer previously failed at 95 and
196 cycles. In those kernels the double queueing was prevented by an
iSCSI-private claim that earlier versions of this series carried, rather
than by patch 2, so the hang fix is validated but patches 1 and 2 are not.
Neither of their bugs was observed in the field; both were found by
inspection while reviewing this series. I would rather say that plainly
than imply coverage they do not have.

The reporter's environment is a production cluster and 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 and I will report the result.

Two things found while reviewing v6 locally are not addressed here.

Completing the command in iscsit_execute_cmd() means
iscsit_get_immediate_data() can read cmd->sense_reason and
cmd->unsolicited_data after target_abort_work has freed the command. The
caller defect is pre-existing: iscsit_handle_immediate_data() sets
ICF_GOT_LAST_DATAOUT when all of a WRITE's data arrives as immediate data,
and the branch directly above the one patch 4 changes then calls
target_execute_cmd(), which queues the same target_abort_work for an
aborted command and returns 0 to the same caller, which goes on to
dereference it. What patch 4 changes is which commands reach that: today
an all-immediate-data abort, after the patch also one whose length exceeds
FirstBurstLength.

Given how you wanted the caw_sem bug handled, I would rather ask than
guess: should the caller be fixed first in this series, or as a follow-up?
The two shapes I can see are holding a reference across
iscsit_sequence_cmd(), or giving iscsit_execute_cmd() a return value that
means "command consumed, do not touch it". The second changes the contract
for every caller of iscsit_sequence_cmd(), so it is not something I want
to decide on my own.

Second: a COMPARE AND WRITE that fails target_check_reservation() reaches
compare_and_write_callback() with success == false, its SG lists
allocated, and neither CMD_T_ABORTED nor CMD_T_STOP set, which fires the
WARN_ON() there. That is pre-existing and none of these patches change
whether it is reached. Working out whether the WARN_ON or the path is
wrong is a separate question and I did not want to guess at it here.

Laurence Oberman (4):
  scsi: target: Do not re-enable interrupts in compare_and_write_post()
  scsi: target: Only release caw_sem if this command took it
  scsi: target: Queue the abort of a command at most once
  scsi: target: iscsi: Complete an aborted WRITE stuck before the first
    R2T

 drivers/target/iscsi/iscsi_target_erl1.c | 12 +++-
 drivers/target/target_core_sbc.c         | 54 +++++++++++------
 drivers/target/target_core_transport.c   | 77 +++++++++++++++++++++---
 include/target/target_core_base.h        |  2 +
 4 files changed, 117 insertions(+), 28 deletions(-)

-- 
2.43.0

[PATCH v6 1/4] scsi: target: Do not re-enable interrupts in compare_and_write_post()

[Laurence Oberman]

target_complete_cmd() may be called by a backend driver from interrupt
context, as the comment above target_complete_cmd_with_sense() says. For a
command that has been aborted it calls target_cmd_interrupted(), which
invokes cmd->transport_complete_callback(). Once the WRITE phase of a
COMPARE AND WRITE has been set up that callback is
compare_and_write_post(), which takes t_state_lock with spin_lock_irq()
and drops it with spin_unlock_irq(), unconditionally enabling interrupts
in a caller that had them disabled.

Use spin_lock_irqsave() / spin_unlock_irqrestore() instead.

compare_and_write_callback() has the same pattern, but its
spin_lock_irq() is on the path taken after a successful compare, which is
reached only from target_complete_ok_work() in process context; the
success == false path returns long before it. It is left alone.

Signed-off-by: Laurence Oberman <loberman@redhat.com>
---

Notes:
    Pre-existing, and not needed by the rest of the series, but
    compare_and_write_post() is modified by patch 2 and the mismatch between
    its spin_lock_irq() and the spin_lock_irqsave() a few lines below would be
    confusing to leave in place.
    
    No Fixes: tag -- the pattern predates target_cmd_interrupted().

 drivers/target/target_core_sbc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 21f5cb8..8c52e1e 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -352,15 +352,16 @@ static sense_reason_t compare_and_write_post(struct se_cmd *cmd, bool success,
 {
 	struct se_device *dev = cmd->se_dev;
 	sense_reason_t ret = TCM_NO_SENSE;
+	unsigned long flags;
 
-	spin_lock_irq(&cmd->t_state_lock);
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
 	if (success) {
 		*post_ret = 1;
 
 		if (cmd->scsi_status == SAM_STAT_CHECK_CONDITION)
 			ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 	}
-	spin_unlock_irq(&cmd->t_state_lock);
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 
 	/*
 	 * Unlock ->caw_sem originally obtained during sbc_compare_and_write()
-- 
2.43.0

[PATCH v6 2/4] scsi: target: Only release caw_sem if this command took it

[Laurence Oberman]

sbc_parse_cdb() installs compare_and_write_callback() as the command's
transport_complete_callback when it parses a COMPARE AND WRITE. The
semaphore that callback releases is not taken until much later, in
sbc_compare_and_write(), which runs when the command is executed. A
command that is aborted or fails between those two points reaches the
callback without holding dev->caw_sem, and releases it anyway.

compare_and_write_callback() tries to cover that case by returning early
when cmd->t_data_sg or cmd->t_bidi_data_sg is NULL, on the assumption that
a command with no buffers cannot have taken the semaphore. That assumption
does not hold: transport_generic_new_cmd() allocates both lists, and a
command aborted while it sits in TRANSPORT_WRITE_PENDING has the buffers
and not the semaphore. up() on a semaphore that was never taken raises its
count, which later lets two COMPARE AND WRITE commands run against the
same device at once.

The same callback can also run more than once for one command --
target_cmd_interrupted() and transport_generic_request_failure() both
invoke it -- which releases the semaphore a second time.

Track whether the semaphore is held in SCF_CAW_SEM_HELD, set once
down_interruptible() has succeeded in sbc_compare_and_write(). Release it
through sbc_caw_sem_release(), which tests and clears the flag under
t_state_lock and only calls up() if the flag was set, so neither a command
that never took the semaphore nor a second invocation of the callback can
release it. The early return for missing buffers becomes a goto out, since
the release is now decided by the flag rather than inferred from the
buffers.

Cc: stable@vger.kernel.org
Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Laurence Oberman <loberman@redhat.com>
---

Notes:
    No Fixes: tag: the callback has been installed at parse time and the
    semaphore taken at execute time since COMPARE AND WRITE was added, so I
    am not confident which commit to name.  Please say which you would like
    and I will add it.  Cc: stable is on the patch because the consequence --
    a semaphore released without being taken -- is worth backporting
    regardless of which commit is cited.
    
    Not observed in the field; found while reviewing the double completion
    that patch 2 fixes.

 drivers/target/target_core_sbc.c  | 49 +++++++++++++++++++++----------
 include/target/target_core_base.h |  1 +
 2 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/drivers/target/target_core_sbc.c b/drivers/target/target_core_sbc.c
index 8c52e1e..5193453 100644
--- a/drivers/target/target_core_sbc.c
+++ b/drivers/target/target_core_sbc.c
@@ -347,10 +347,33 @@ sbc_execute_rw(struct se_cmd *cmd)
 			       cmd->data_direction);
 }
 
+/*
+ * Release dev->caw_sem, but only if this command is the one holding it.
+ * sbc_compare_and_write() takes the semaphore when the command is executed,
+ * which is long after sbc_parse_cdb() installs the completion callback, so a
+ * command that is aborted or fails before then reaches the callback without
+ * holding it.  The test and clear are done under t_state_lock so that a second
+ * invocation of the callback cannot release the semaphore twice.
+ */
+static void sbc_caw_sem_release(struct se_cmd *cmd)
+	__must_not_hold(&cmd->t_state_lock)
+{
+	struct se_device *dev = cmd->se_dev;
+	unsigned long flags;
+	bool held;
+
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	held = cmd->se_cmd_flags & SCF_CAW_SEM_HELD;
+	cmd->se_cmd_flags &= ~SCF_CAW_SEM_HELD;
+	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
+	if (held)
+		up(&dev->caw_sem);
+}
+
 static sense_reason_t compare_and_write_post(struct se_cmd *cmd, bool success,
 					     int *post_ret)
 {
-	struct se_device *dev = cmd->se_dev;
 	sense_reason_t ret = TCM_NO_SENSE;
 	unsigned long flags;
 
@@ -363,11 +386,7 @@ static sense_reason_t compare_and_write_post(struct se_cmd *cmd, bool success,
 	}
 	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 
-	/*
-	 * Unlock ->caw_sem originally obtained during sbc_compare_and_write()
-	 * before the original READ I/O submission.
-	 */
-	up(&dev->caw_sem);
+	sbc_caw_sem_release(cmd);
 
 	return ret;
 }
@@ -450,11 +469,12 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
 
 	if (!success) {
 		/*
-		 * Handle early failure in transport_generic_request_failure(),
-		 * which will not have taken ->caw_sem yet..
+		 * Handle early failure in transport_generic_request_failure().
+		 * Whether ->caw_sem was taken is tracked in SCF_CAW_SEM_HELD, so
+		 * this is only about there being no buffers to compare.
 		 */
 		if (!cmd->t_data_sg || !cmd->t_bidi_data_sg)
-			return TCM_NO_SENSE;
+			goto out;
 
 		/*
 		 * The command has been stopped or aborted so
@@ -556,11 +576,7 @@ static sense_reason_t compare_and_write_callback(struct se_cmd *cmd, bool succes
 	return ret;
 
 out:
-	/*
-	 * In the MISCOMPARE or failure case, unlock ->caw_sem obtained in
-	 * sbc_compare_and_write() before the original READ I/O submission.
-	 */
-	up(&dev->caw_sem);
+	sbc_caw_sem_release(cmd);
 	sg_free_table(&write_tbl);
 	return ret;
 }
@@ -581,6 +597,9 @@ sbc_compare_and_write(struct se_cmd *cmd)
 		cmd->transport_complete_callback = NULL;
 		return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 	}
+	spin_lock_irq(&cmd->t_state_lock);
+	cmd->se_cmd_flags |= SCF_CAW_SEM_HELD;
+	spin_unlock_irq(&cmd->t_state_lock);
 	/*
 	 * Reset cmd->data_length to individual block_size in order to not
 	 * confuse backend drivers that depend on this value matching the
@@ -592,7 +611,7 @@ sbc_compare_and_write(struct se_cmd *cmd)
 			      DMA_FROM_DEVICE);
 	if (ret) {
 		cmd->transport_complete_callback = NULL;
-		up(&dev->caw_sem);
+		sbc_caw_sem_release(cmd);
 		return ret;
 	}
 	/*
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 002b0fc..bd846ab 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -168,6 +168,7 @@ enum se_cmd_flags_table {
 	SCF_TREAT_READ_AS_NORMAL		= (1 << 18),
 	SCF_TASK_ORDERED_SYNC			= (1 << 19),
 	SCF_ATOMIC				= (1 << 20),
+	SCF_CAW_SEM_HELD			= (1 << 21),
 };
 
 /*
-- 
2.43.0

[PATCH v6 3/4] scsi: target: Queue the abort of a command at most once

[Laurence Oberman]

target_cmd_interrupted() and transport_generic_request_failure() both
queue target_abort_work for a command that has CMD_T_ABORTED set, and
neither records that it has done so: CMD_T_COMPLETE is not set and t_state
is not changed, so nothing stops a second site reaching the same
conclusion for the same command. The second call re-initialises and
re-queues cmd->work while the first is still queued or running.

More than one site can reach that conclusion. A WRITE still in
TRANSPORT_WRITE_PENDING has never been passed to the backend, so the
fabric has to complete it when a TMR aborts it, and for iSCSI both the
DataOut paths and iscsit_release_commands_from_conn() during connection
teardown will do so. The two queueing sites can also reach each other: if
CMD_T_ABORTED is set after target_cmd_interrupted() has returned false in
target_execute_cmd() and execution then fails,
transport_generic_request_failure() queues the work, and a later
target_execute_cmd() -- from iscsit_task_reassign_complete_write(), for
example -- queues it a second time.

Add a CMD_T_ABORT_QUEUED flag, tested and set under t_state_lock by
__target_claim_abort(), and use it at both sites. In
target_cmd_interrupted() the CMD_T_ABORTED test moves under the same lock,
which is the lock __target_check_io_state() sets CMD_T_ABORTED under, so
there is no window between the test and the claim. Second and later calls
are a no-op by design and target_cmd_interrupted() still returns true, so
callers do not proceed with normal completion. A fabric that completes an
aborted command exactly once never reaches the claim and is unaffected.

target_cmd_interrupted() runs for every completion, so it keeps a lockless
test of the two flags and only takes t_state_lock when one of them is set.
A flag set immediately after that test is handled exactly as it was when
the whole function was lockless.

The CMD_T_STOP branch is deliberately left unguarded. complete_all() is
idempotent and t_transport_stop_comp is initialised once in
__target_init_cmd(), so a repeated call there is harmless. A guard shared
between the two branches would also be wrong: a first call taking the
CMD_T_STOP branch would then suppress the queueing of target_abort_work if
CMD_T_ABORTED were set afterwards, which is the hang the next patch is
about.

transport_complete_callback() can still run more than once for a command,
across the two branches here and from
transport_generic_request_failure(). The only callback in the tree is
COMPARE AND WRITE's, whose side effect -- releasing dev->caw_sem -- is
made one-shot by the previous patch, so a repeated call now has nothing
left to repeat.

Cc: stable@vger.kernel.org
Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Laurence Oberman <loberman@redhat.com>
---

Notes:
    Prerequisite for patch 3, which relies on a second completion of an
    aborted command being a no-op.  All three patches need to travel together
    to stable.
    
    No Fixes: tag: 2c9fa49e100f added the same abort-queueing pattern to both
    functions, so it is arguably the right one, but the double queueing is
    not what that commit set out to do.  Happy to add it if you would like.

 drivers/target/target_core_transport.c | 77 ++++++++++++++++++++++----
 include/target/target_core_base.h      |  1 +
 2 files changed, 68 insertions(+), 10 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 927861c..9ef8e93 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -882,24 +882,72 @@ static void target_abort_work(struct work_struct *work)
 	target_handle_abort(cmd);
 }
 
+/*
+ * Claim the queueing of target_abort_work for an aborted command.  Returns
+ * true for the one caller that must queue it; every later caller gets false.
+ *
+ * Caller must hold cmd->t_state_lock, which is the lock
+ * __target_check_io_state() sets CMD_T_ABORTED under, so that the flag cannot
+ * be set between a caller's CMD_T_ABORTED test and this claim.
+ */
+static bool __target_claim_abort(struct se_cmd *cmd)
+	__must_hold(&cmd->t_state_lock)
+{
+	lockdep_assert_held(&cmd->t_state_lock);
+
+	if (cmd->transport_state & CMD_T_ABORT_QUEUED)
+		return false;
+
+	cmd->transport_state |= CMD_T_ABORT_QUEUED;
+	return true;
+}
+
+/*
+ * Returns true if the command has been aborted or stopped, in which case the
+ * caller must not carry on with normal completion.  target_abort_work is
+ * queued at most once per command; a later call for an already-claimed abort
+ * is a no-op and still returns true.
+ */
 static bool target_cmd_interrupted(struct se_cmd *cmd)
 {
+	bool queue_abort = false, stop = false;
+	unsigned long flags;
 	int post_ret;
 
+	/*
+	 * This runs for every completion, so stay out of t_state_lock unless
+	 * one of the two flags is set.  A flag set just after this test is
+	 * handled the same way it was before the lock was introduced here.
+	 */
+	if (!(cmd->transport_state & (CMD_T_ABORTED | CMD_T_STOP)))
+		return false;
+
+	spin_lock_irqsave(&cmd->t_state_lock, flags);
 	if (cmd->transport_state & CMD_T_ABORTED) {
-		if (cmd->transport_complete_callback)
-			cmd->transport_complete_callback(cmd, false, &post_ret);
+		queue_abort = __target_claim_abort(cmd);
+		if (!queue_abort) {
+			spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+			return 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);
-		return true;
-	} else if (cmd->transport_state & CMD_T_STOP) {
-		if (cmd->transport_complete_callback)
-			cmd->transport_complete_callback(cmd, false, &post_ret);
+	} else {
 		complete_all(&cmd->t_transport_stop_comp);
-		return true;
 	}
 
-	return false;
+	return true;
 }
 
 static void target_complete(struct se_cmd *cmd, int success)
@@ -2084,6 +2132,7 @@ EXPORT_SYMBOL(target_submit_tmr);
 void transport_generic_request_failure(struct se_cmd *cmd,
 		sense_reason_t sense_reason)
 {
+	unsigned long flags;
 	int ret = 0, post_ret;
 
 	pr_debug("-----[ Storage Engine Exception; sense_reason %d\n",
@@ -2099,8 +2148,16 @@ void transport_generic_request_failure(struct se_cmd *cmd,
 		cmd->transport_complete_callback(cmd, false, &post_ret);
 
 	if (cmd->transport_state & CMD_T_ABORTED) {
-		INIT_WORK(&cmd->work, target_abort_work);
-		queue_work(target_completion_wq, &cmd->work);
+		bool queue_abort;
+
+		spin_lock_irqsave(&cmd->t_state_lock, flags);
+		queue_abort = __target_claim_abort(cmd);
+		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
+
+		if (queue_abort) {
+			INIT_WORK(&cmd->work, target_abort_work);
+			queue_work(target_completion_wq, &cmd->work);
+		}
 		return;
 	}
 
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index bd846ab..0341844 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -538,6 +538,7 @@ struct se_cmd {
 #define CMD_T_STOP		(1 << 5)
 #define CMD_T_TAS		(1 << 10)
 #define CMD_T_FABRIC_STOP	(1 << 11)
+#define CMD_T_ABORT_QUEUED	(1 << 12)
 	spinlock_t		t_state_lock;
 	struct kref		cmd_kref;
 	struct completion	t_transport_stop_comp;
-- 
2.43.0

[PATCH v6 4/4] 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 this site with nothing to complete the command.
Before it, transport_check_aborted_status() sent the delayed TASK_ABORTED
status here; 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. It became a hang once ABORT and LUN
RESET handling was made synchronous, because the abort path then waits in
target_put_cmd_and_wait() for a fabric completion that no longer comes.

iscsit_execute_cmd() returns early for an aborted command in two other
places, and neither can hang. Both TMR paths -- core_tmr_abort_task() and
core_tmr_drain_state_list() -- iterate dev->queues[].state_list, and a
command only reaches that list via target_add_to_state_list() in
transport_generic_new_cmd(). For a command with no immediate data,
iscsit_process_scsi_cmd() runs iscsit_sequence_cmd(), and therefore
iscsit_execute_cmd(), before transport_generic_new_cmd(); a command with
cmd->sense_reason set returns before it as well. In both cases no TMR can
find the command, so nothing waits on it. Only the immediate-data path
calls transport_generic_new_cmd() first, which is why this is the one
early return that strands a TMR.

Complete the command here 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.
If the connection drops before target_abort_work has run,
iscsit_release_commands_from_conn() will complete the same command again,
which the claim added in the previous patch makes a no-op.

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: 2c9fa49e100f ("scsi: target/core: Make ABORT and LUN RESET handling synchronous")
Cc: stable@vger.kernel.org
Signed-off-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/target/iscsi/iscsi_target_erl1.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/target/iscsi/iscsi_target_erl1.c b/drivers/target/iscsi/iscsi_target_erl1.c
index d8ca06e..95163b1 100644
--- a/drivers/target/iscsi/iscsi_target_erl1.c
+++ b/drivers/target/iscsi/iscsi_target_erl1.c
@@ -12,6 +12,7 @@
 #include <linux/slab.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>
 
@@ -927,8 +928,17 @@ 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) {
+					target_complete_cmd(&cmd->se_cmd,
+							    SAM_STAT_TASK_ABORTED);
 					return 0;
+				}
 
 				iscsit_set_dataout_sequence_values(cmd);
 				conn->conn_transport->iscsit_get_dataout(conn, cmd, false);
-- 
2.43.0