From 6d243686c9f9752a9d6512e0d7579e09d0cefb2c Mon Sep 17 00:00:00 2001 From: vlnb Date: Fri, 10 Aug 2012 01:48:35 +0000 Subject: [PATCH] Fix possible commands hangs with lockless queuecommand() in scst_local --- scst/include/scst.h | 12 ++++++++++ scst/src/scst_lib.c | 1 + scst/src/scst_targ.c | 30 ++++++++++++++++-------- scst_local/Makefile | 2 +- scst_local/README | 2 +- scst_local/scst_local.c | 51 ++++++++++------------------------------- 6 files changed, 48 insertions(+), 50 deletions(-) diff --git a/scst/include/scst.h b/scst/include/scst.h index 8d30b43e..a7593007 100644 --- a/scst/include/scst.h +++ b/scst/include/scst.h @@ -669,6 +669,12 @@ struct scst_tgt_template { */ unsigned fake_aca:1; + /* + * True, if this target adapter can call scst_cmd_init_done() from + * several threads at the same time. + */ + unsigned multithreaded_init_done:1; + /* * Preferred SCSI LUN addressing method. */ @@ -1796,6 +1802,12 @@ struct scst_order_data { int num_free_sn_slots; /* if it's <0, then all slots are busy */ 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 + */ + spinlock_t init_done_lock; }; /* diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index e62ccf07..c11b7cbb 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -3347,6 +3347,7 @@ static void scst_init_order_data(struct scst_order_data *order_data) order_data->cur_sn_slot = &order_data->sn_slots[0]; for (i = 0; i < (int)ARRAY_SIZE(order_data->sn_slots); i++) atomic_set(&order_data->sn_slots[i], 0); + spin_lock_init(&order_data->init_done_lock); return; } diff --git a/scst/src/scst_targ.c b/scst/src/scst_targ.c index aeead5fb..9b027612 100644 --- a/scst/src/scst_targ.c +++ b/scst/src/scst_targ.c @@ -313,7 +313,7 @@ static int scst_init_cmd(struct scst_cmd *cmd, enum scst_exec_context *context) } else { unsigned long flags; spin_lock_irqsave(&scst_init_lock, flags); - TRACE_MGMT_DBG("Adding cmd %p to init cmd list)", cmd); + TRACE_MGMT_DBG("Adding cmd %p to init cmd list", cmd); list_add_tail(&cmd->cmd_list_entry, &scst_init_cmd_list); if (test_bit(SCST_CMD_ABORTED, &cmd->cmd_flags)) scst_init_poll_cnt++; @@ -343,9 +343,11 @@ static int scst_init_cmd(struct scst_cmd *cmd, enum scst_exec_context *context) * 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 iSCSI - * immediate commands with multiple connections per session seems to be an - * exception. For it, some mutex/lock shall be used for the serialization. + * 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) @@ -1188,8 +1190,10 @@ void scst_restart_cmd(struct scst_cmd *cmd, int status, cmd->state = SCST_CMD_STATE_RDY_TO_XFER; else cmd->state = SCST_CMD_STATE_TGT_PRE_EXEC; - if (cmd->set_sn_on_restart_cmd) + if (cmd->set_sn_on_restart_cmd) { + EXTRACHECKS_BUG_ON(cmd->tgtt->multithreaded_init_done); scst_cmd_set_sn(cmd); + } #ifdef CONFIG_SCST_TEST_IO_IN_SIRQ if (cmd->op_flags & SCST_TEST_IO_IN_SIRQ_ALLOWED) break; @@ -3770,8 +3774,8 @@ static int scst_finish_cmd(struct scst_cmd *cmd) * number. A command that must be executed after previously received commands * is assigned a new and higher slot number. * - * No locks, but it must be externally serialized (see comment for - * scst_cmd_init_done() in scst.h) + * No locks expected, but it must be externally serialized (see comment before + * scst_cmd_init_done() definition) * * Note: This approach in full compliance with SAM may result in the reordering * of conflicting SIMPLE READ and/or WRITE commands (commands with at least @@ -4003,8 +4007,16 @@ static int __scst_init_cmd(struct scst_cmd *cmd) */ scst_pre_parse(cmd); - if (!cmd->set_sn_on_restart_cmd) - scst_cmd_set_sn(cmd); + if (!cmd->set_sn_on_restart_cmd) { + if (cmd->tgtt->multithreaded_init_done) { + 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); + } else + scst_cmd_set_sn(cmd); + } } else if (res < 0) { TRACE_DBG("Finishing cmd %p", cmd); scst_set_cmd_error(cmd, diff --git a/scst_local/Makefile b/scst_local/Makefile index 47a6f70e..156aae5f 100644 --- a/scst_local/Makefile +++ b/scst_local/Makefile @@ -16,7 +16,7 @@ EXTRA_CFLAGS += -I$(SCST_INC_DIR) -I$(SCST_DIR) EXTRA_CFLAGS += $(call enable-Wextra,-Wextra -Wno-unused-parameter\ -Wno-missing-field-initializers) -#EXTRA_CFLAGS += -DCONFIG_SCST_LOCAL_FORCE_DIRECT_PROCESSING +#EXTRA_CFLAGS += -DCONFIG_SCST_LOCAL_DIRECT_PROCESSING EXTRA_CFLAGS += -DCONFIG_SCST_EXTRACHECKS diff --git a/scst_local/README b/scst_local/README index 1d2a4ffe..5685abba 100644 --- a/scst_local/README +++ b/scst_local/README @@ -234,7 +234,7 @@ Compilation options There are the following compilation options, that could be commented in/out in Makefile: - - CONFIG_SCST_LOCAL_FORCE_DIRECT_PROCESSING - by default, when this option + - CONFIG_SCST_LOCAL_DIRECT_PROCESSING - by default, when this option is not defined, scst_local reschedules all commands for processing in one of the SCST threads. If this option is defined, scst_local tries to not do it, if possible (sometimes queuecommand() called under diff --git a/scst_local/scst_local.c b/scst_local/scst_local.c index ac74e13d..efa4d0c3 100644 --- a/scst_local/scst_local.c +++ b/scst_local/scst_local.c @@ -922,7 +922,6 @@ static int scst_local_send_resp(struct scsi_cmnd *cmnd, * target driver and have it do its magic ... */ #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 37) -#ifdef CONFIG_SCST_LOCAL_FORCE_DIRECT_PROCESSING static int scst_local_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *SCpnt) #else @@ -931,12 +930,6 @@ static int scst_local_queuecommand_lck(struct scsi_cmnd *SCpnt, __acquires(&h->host_lock) __releases(&h->host_lock) #endif -#else -static int scst_local_queuecommand_lck(struct scsi_cmnd *SCpnt, - void (*done)(struct scsi_cmnd *)) - __acquires(&h->host_lock) - __releases(&h->host_lock) -#endif { #if (LINUX_VERSION_CODE < KERNEL_VERSION(2, 6, 25)) struct scst_local_tgt_specific *tgt_specific = NULL; @@ -1085,30 +1078,19 @@ static int scst_local_queuecommand_lck(struct scsi_cmnd *SCpnt, scst_cmd_set_tgt_priv(scst_cmd, SCpnt); #endif -/* - * Although starting from 2.6.37 queuecommand() called with no host_lock - * held, in fact without DEF_SCSI_QCMD() it doesn't work and leading - * to various problems like hangs under highload. Most likely, it is caused - * by some not reenrable block layer function(s). So, until that changed, we - * have to go ahead with extra context switch. In this regard doesn't matter - * much if we under host_lock or not (although we absolutely don't need this - * lock), so let's have simpler code with DEF_SCSI_QCMD(). - */ -#ifdef CONFIG_SCST_LOCAL_FORCE_DIRECT_PROCESSING #if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 37) - scst_cmd_init_done(scst_cmd, SCST_CONTEXT_DIRECT); +#ifdef CONFIG_SCST_LOCAL_DIRECT_PROCESSING + /* + * NOTE! At the moment in scst_estimate_context*() returning + * DIRECT contexts disabled, so this option doesn't have any + * real effect. + */ + if (spin_is_locked(SCpnt->device->host->host_lock)) + scst_cmd_init_done(scst_cmd, SCST_CONTEXT_THREAD); + else + scst_cmd_init_done(scst_cmd, scst_estimate_context()); #else - { - /* - * NOTE! At the moment in scst_estimate_context*() returning - * DIRECT contexts disabled, so this option doesn't have any - * real effect. - */ - struct Scsi_Host *h = SCpnt->device->host; - spin_unlock_irq(h->host_lock); - scst_cmd_init_done(scst_cmd, scst_estimate_context_atomic()); - spin_lock_irq(h->host_lock); - } + scst_cmd_init_done(scst_cmd, SCST_CONTEXT_THREAD); #endif #else /* @@ -1122,16 +1104,6 @@ static int scst_local_queuecommand_lck(struct scsi_cmnd *SCpnt, return 0; } -#if LINUX_VERSION_CODE >= KERNEL_VERSION(2, 6, 37) -#if !defined(CONFIG_SCST_LOCAL_FORCE_DIRECT_PROCESSING) -/* - * See comment in scst_local_queuecommand_lck() near - * CONFIG_SCST_LOCAL_FORCE_DIRECT_PROCESSING - */ -static DEF_SCSI_QCMD(scst_local_queuecommand) -#endif -#endif - static int scst_local_targ_pre_exec(struct scst_cmd *scst_cmd) { int res = SCST_PREPROCESS_STATUS_SUCCESS; @@ -1396,6 +1368,7 @@ static struct scst_tgt_template scst_local_targ_tmpl = { .sg_tablesize = 0xffff, #endif .xmit_response_atomic = 1, + .multithreaded_init_done = 1, #ifndef CONFIG_SCST_PROC .enabled_attr_not_needed = 1, .tgtt_attrs = scst_local_tgtt_attrs,