From e35fddbb6e21a2f5bdcf051dfe5621bdfe9ac5fb Mon Sep 17 00:00:00 2001 From: Tony Battersby Date: Wed, 10 Dec 2025 14:54:26 -0500 Subject: [PATCH] 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: https://github.com/SCST-project/scst/issues/333 Signed-off-by: Tony Battersby --- scst/include/scst.h | 13 +---------- scst/src/scst_targ.c | 48 ++++++++++------------------------------- scst_local/scst_local.c | 1 - 3 files changed, 12 insertions(+), 50 deletions(-) diff --git a/scst/include/scst.h b/scst/include/scst.h index 360c21244..137ede92d 100644 --- a/scst/include/scst.h +++ b/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) diff --git a/scst/src/scst_targ.c b/scst/src/scst_targ.c index a015fc660..b0a3d550b 100644 --- a/scst/src/scst_targ.c +++ b/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)); diff --git a/scst_local/scst_local.c b/scst_local/scst_local.c index 897f3aa18..070ad4fff 100644 --- a/scst_local/scst_local.c +++ b/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,