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

[Laurence Oberman <loberman@redhat.com>]

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