scst_cmd_set_sn: remove lockless fast path

The lockless fast path for scst_cmd_set_sn() can cause commands to
lockup in state EXEC_CHECK_SN when there are multiple scst_tgts
accessing the same scst_device, for example two initiators
connected to the two ports of a dual-port QLogic FC HBA in target mode
both reading from the same shared disk.  The multithreaded_init_done
value is too low-level for this; it does not take the higher-level
configuration into account.

- Remove the lockless fast path.
- Remove multithreaded_init_done, which enabled/disabled the lockless
  fast path.
- Push the locking down into scst_cmd_set_sn(), which will now apply
  regardless of set_sn_on_restart_cmd, which matters for mixed-driver
  (e.g. iSCSI+qla2xxx) target-mode setups.
- Remove a bunch of comments explaining the rules for the lockless fast
  path.

Fixes: #333
Signed-off-by: Tony Battersby <tonyb@cybernetics.com>

Author: abattersby

scst/include/scst.h

@@ -755,12 +755,6 @@ struct scst_tgt_template {
 	/* True, if this target doesn't need "enabled" attribute */
 	unsigned enabled_attr_not_needed:1;
 
-	/*
-	 * True, if this target adapter can call scst_cmd_init_done() from
-	 * several threads at the same time.
-	 */
-	unsigned multithreaded_init_done:1;
-
 	/*
 	 * True, if this target driver supports T10-PI (DIF), i.e. sending and
 	 * receiving DIF PI tags. If false, SCST will not allow to add
@@ -2033,10 +2027,7 @@ struct scst_order_data {
 	atomic_t *cur_sn_slot;
 	atomic_t sn_slots[15];
 
-	/*
-	 * Used to serialized scst_cmd_init_done() if the corresponding
-	 * session's target template has multithreaded_init_done set
-	 */
+	/* Used to serialize scst_cmd_init_done(). */
 	spinlock_t init_done_lock;
 };
 
@@ -3590,8 +3581,6 @@ void scst_cmd_init_done(struct scst_cmd *cmd, enum scst_exec_context pref_contex
  * SCST done the command's preprocessing preprocessing_done() function
  * should be called. The second argument sets preferred command execution
  * context. See SCST_CONTEXT_* constants for details.
- *
- * See comment for scst_cmd_init_done() for the serialization requirements.
  */
 static inline void scst_cmd_init_stage1_done(struct scst_cmd *cmd,
 					     enum scst_exec_context pref_context, int set_sn)

scst/src/scst_targ.c

@@ -783,20 +783,6 @@ static int scst_init_cmd(struct scst_cmd *cmd, enum scst_exec_context *context)
  *    initialization and also that the command is ready for execution. The
  *    second argument sets the preferred command execution context. See also
  *    SCST_CONTEXT_* constants for more information.
- *
- *    !!IMPORTANT!!
- *
- *    If cmd->set_sn_on_restart_cmd has not been set, this function, as well
- *    as scst_cmd_init_stage1_done() and scst_restart_cmd(), must not be
- *    called simultaneously for the same session (more precisely, for the same
- *    session/LUN, i.e. tgt_dev), i.e. they must be somehow externally
- *    serialized. This is needed to have lock free fast path in
- *    scst_cmd_set_sn(). For majority of targets those functions are naturally
- *    serialized by the single source of commands. Only some, like iSCSI
- *    immediate commands with multiple connections per session or scst_local,
- *    are exceptions. For it, some mutex/lock must be used for the
- *    serialization. Or, alternatively, multithreaded_init_done can be set in
- *    the target's template.
  */
 void scst_cmd_init_done(struct scst_cmd *cmd, enum scst_exec_context pref_context)
 {
@@ -1618,9 +1604,6 @@ static int scst_preprocessing_done(struct scst_cmd *cmd)
  *
  *    The second argument sets completion status
  *    (see SCST_PREPROCESS_STATUS_* constants for details)
- *
- *    See also comment for scst_cmd_init_done() for the serialization
- *    requirements.
  */
 void scst_restart_cmd(struct scst_cmd *cmd, int status, enum scst_exec_context pref_context)
 {
@@ -1652,10 +1635,8 @@ void scst_restart_cmd(struct scst_cmd *cmd, int status, enum scst_exec_context p
 		} else {
 			scst_set_cmd_state(cmd, SCST_CMD_STATE_TGT_PRE_EXEC);
 		}
-		if (cmd->set_sn_on_restart_cmd) {
-			EXTRACHECKS_BUG_ON(cmd->tgtt->multithreaded_init_done);
+		if (cmd->set_sn_on_restart_cmd)
 			scst_cmd_set_sn(cmd);
-		}
 #ifdef CONFIG_SCST_TEST_IO_IN_SIRQ
 		if (cmd->op_flags & SCST_TEST_IO_IN_SIRQ_ALLOWED)
 			break;
@@ -3848,7 +3829,7 @@ static void scst_cmd_set_sn(struct scst_cmd *cmd)
 
 	EXTRACHECKS_BUG_ON(cmd->sn_set || cmd->hq_cmd_inced);
 
-	/* Optimized for lockless fast path of sequence of SIMPLE commands */
+	/* Optimized for fast path of sequence of SIMPLE commands */
 
 	scst_check_debug_sn(cmd);
 
@@ -3870,6 +3851,8 @@ static void scst_cmd_set_sn(struct scst_cmd *cmd)
 		}
 	}
 
+	spin_lock_irqsave(&order_data->init_done_lock, flags);
+
 again:
 	switch (cmd->queue_type) {
 	case SCST_CMD_QUEUE_SIMPLE:
@@ -3927,7 +3910,7 @@ static void scst_cmd_set_sn(struct scst_cmd *cmd)
 		} else {
 			order_data->prev_cmd_ordered = 1;
 
-			spin_lock_irqsave(&order_data->sn_lock, flags);
+			spin_lock(&order_data->sn_lock); /* irqs already off */
 
 			/*
 			 * If no commands are going to reach
@@ -3948,7 +3931,7 @@ static void scst_cmd_set_sn(struct scst_cmd *cmd)
 					scst_inc_expected_sn_idle(order_data);
 				}
 			}
-			spin_unlock_irqrestore(&order_data->sn_lock, flags);
+			spin_unlock(&order_data->sn_lock);
 		}
 
 		cmd->sn = order_data->curr_sn;
@@ -3957,9 +3940,9 @@ static void scst_cmd_set_sn(struct scst_cmd *cmd)
 
 	case SCST_CMD_QUEUE_HEAD_OF_QUEUE:
 		TRACE_SN("HQ cmd %p (op %s)", cmd, scst_get_opcode_name(cmd));
-		spin_lock_irqsave(&order_data->sn_lock, flags);
+		spin_lock(&order_data->sn_lock); /* irqs already off */
 		order_data->hq_cmd_count++;
-		spin_unlock_irqrestore(&order_data->sn_lock, flags);
+		spin_unlock(&order_data->sn_lock);
 		cmd->hq_cmd_inced = 1;
 		goto out;
 
@@ -3977,6 +3960,7 @@ static void scst_cmd_set_sn(struct scst_cmd *cmd)
 		 order_data->cur_sn_slot - order_data->sn_slots);
 
 out:
+	spin_unlock_irqrestore(&order_data->init_done_lock, flags);
 	TRACE_EXIT();
 }
 
@@ -4226,18 +4210,8 @@ static int __scst_init_cmd(struct scst_cmd *cmd)
 		if (cmd->completed)
 			goto out;
 
-		if (!cmd->set_sn_on_restart_cmd) {
-			if (!cmd->tgtt->multithreaded_init_done) {
-				scst_cmd_set_sn(cmd);
-			} else {
-				struct scst_order_data *order_data = cmd->cur_order_data;
-				unsigned long flags;
-
-				spin_lock_irqsave(&order_data->init_done_lock, flags);
-				scst_cmd_set_sn(cmd);
-				spin_unlock_irqrestore(&order_data->init_done_lock, flags);
-			}
-		}
+		if (!cmd->set_sn_on_restart_cmd)
+			scst_cmd_set_sn(cmd);
 	} else if (res < 0) {
 		TRACE_DBG("Finishing cmd %p", cmd);
 		scst_set_cmd_error(cmd, SCST_LOAD_SENSE(scst_sense_lun_not_supported));

scst_local/scst_local.c

@@ -1352,7 +1352,6 @@ static struct scst_tgt_template scst_local_targ_tmpl = {
 	.name			= "scst_local",
 	.sg_tablesize		= 0xffff,
 	.xmit_response_atomic	= 1,
-	.multithreaded_init_done = 1,
 	.enabled_attr_not_needed = 1,
 	.tgtt_attrs		= scst_local_tgtt_attrs,
 	.tgt_attrs		= scst_local_tgt_attrs,