From dd52454052061959d25623500e48032f5ee3b206 Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Wed, 21 Aug 2019 21:30:39 +0000 Subject: [PATCH 1/5] scst: Introduce struct scst_icmd_priv This patch does not change any functionality but improves source code readability. git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@8511 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/scst_copy_mgr.c | 3 ++- scst/src/scst_lib.c | 10 ++++------ scst/src/scst_priv.h | 5 +++++ 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/scst/src/scst_copy_mgr.c b/scst/src/scst_copy_mgr.c index eda4dfd6a..562c5e309 100644 --- a/scst/src/scst_copy_mgr.c +++ b/scst/src/scst_copy_mgr.c @@ -3726,8 +3726,9 @@ static int scst_cm_release(struct scst_tgt *tgt) static int scst_cm_xmit_response(struct scst_cmd *cmd) { + struct scst_icmd_priv *icmd_priv = cmd->tgt_i_priv; + scst_i_finish_fn_t f = icmd_priv->finish_fn; int res = SCST_TGT_RES_SUCCESS; - scst_i_finish_fn_t f = (void *) *((unsigned long long **)cmd->tgt_i_priv); TRACE_ENTRY(); diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 31c8cb443..9e5617d83 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -6944,8 +6944,9 @@ out_done: int scst_finish_internal_cmd(struct scst_cmd *cmd) { - int res; + struct scst_icmd_priv *icmd_priv = cmd->tgt_i_priv; unsigned long flags; + int res; TRACE_ENTRY(); @@ -6971,11 +6972,8 @@ int scst_finish_internal_cmd(struct scst_cmd *cmd) if (cmd->cdb[0] == REQUEST_SENSE) scst_complete_request_sense(cmd); - else { - scst_i_finish_fn_t f = (void *) *((unsigned long long **)cmd->tgt_i_priv); - - f(cmd); - } + else + icmd_priv->finish_fn(cmd); __scst_cmd_put(cmd); diff --git a/scst/src/scst_priv.h b/scst/src/scst_priv.h index 20756def1..4b30dde37 100644 --- a/scst/src/scst_priv.h +++ b/scst/src/scst_priv.h @@ -157,6 +157,11 @@ static inline void scst_set_cmd_state(struct scst_cmd *cmd, typedef void (*scst_i_finish_fn_t) (struct scst_cmd *cmd); +/* Private data associated with an internal command. */ +struct scst_icmd_priv { + scst_i_finish_fn_t finish_fn; +}; + extern struct mutex scst_mutex2; extern int scst_threads; From f16c5d1b2e23e079958b103a5fb17475e7bbdd6b Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Wed, 21 Aug 2019 21:31:31 +0000 Subject: [PATCH 2/5] scst: Rework scst_prepare_request_sense() This patch makes the implementation of scst_prepare_request_sense() consistent with the other code that submits internal commands. git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@8512 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/scst_lib.c | 35 +++++++++++++++++++++++++---------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 9e5617d83..3b776141a 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -5843,16 +5843,31 @@ static void scst_prelim_finish_internal_cmd(struct scst_cmd *cmd) return; } +struct scst_request_sense_priv { + scst_i_finish_fn_t finish_fn; + struct scst_cmd *orig_cmd; +}; + +static void scst_complete_request_sense(struct scst_cmd *cmd); + int scst_prepare_request_sense(struct scst_cmd *orig_cmd) { - int res = 0; static const uint8_t request_sense[6] = { REQUEST_SENSE, 0, 0, 0, SCST_SENSE_BUFFERSIZE, 0 }; + struct scst_request_sense_priv *priv; struct scst_cmd *rs_cmd; + int res = -ENOMEM; TRACE_ENTRY(); + priv = kzalloc(sizeof(*priv), GFP_KERNEL); + if (!priv) + goto out; + + priv->finish_fn = scst_complete_request_sense; + priv->orig_cmd = orig_cmd; + if (orig_cmd->sense != NULL) { TRACE_MEM("Releasing sense %p (orig_cmd %p)", orig_cmd->sense, orig_cmd); @@ -5864,9 +5879,9 @@ int scst_prepare_request_sense(struct scst_cmd *orig_cmd) request_sense, sizeof(request_sense), SCST_CMD_QUEUE_HEAD_OF_QUEUE); if (rs_cmd == NULL) - goto out_error; + goto free_priv; - rs_cmd->tgt_i_priv = orig_cmd; + rs_cmd->tgt_i_priv = priv; rs_cmd->cdb[1] |= scst_get_cmd_dev_d_sense(orig_cmd); rs_cmd->expected_data_direction = SCST_DATA_READ; @@ -5884,19 +5899,22 @@ out: TRACE_EXIT_RES(res); return res; -out_error: - res = -1; +free_priv: + kfree(priv); goto out; } static void scst_complete_request_sense(struct scst_cmd *req_cmd) { - struct scst_cmd *orig_cmd = req_cmd->tgt_i_priv; + struct scst_request_sense_priv *priv = req_cmd->tgt_i_priv; + struct scst_cmd *orig_cmd = priv->orig_cmd; uint8_t *buf; int len; TRACE_ENTRY(); + kfree(priv); + sBUG_ON(orig_cmd == NULL); len = scst_get_buf_full(req_cmd, &buf); @@ -6970,10 +6988,7 @@ int scst_finish_internal_cmd(struct scst_cmd *cmd) scst_finish_cmd_mgmt(cmd); } - if (cmd->cdb[0] == REQUEST_SENSE) - scst_complete_request_sense(cmd); - else - icmd_priv->finish_fn(cmd); + icmd_priv->finish_fn(cmd); __scst_cmd_put(cmd); From e9c16240df8037cae9d1f77cde6ca549ce9888ea Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Wed, 21 Aug 2019 21:32:05 +0000 Subject: [PATCH 3/5] scst: Improve several source code comments git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@8513 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/scst_lib.c | 22 +++++-- scst/src/scst_targ.c | 147 +++++++++++++++++++++++++++---------------- 2 files changed, 109 insertions(+), 60 deletions(-) diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 3b776141a..8f7273609 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -11679,6 +11679,13 @@ static int get_cdb_info_write_same32(struct scst_cmd *cmd, return get_cdb_info_write_same(cmd, sdbops, cmd->cdb[10] & 1 /*NDOB*/); } +/** + * scst_set_cmd_from_cdb_info() - Parse the SCSI CDB. + * @cmd: SCSI command to parse. + * @ptr: Pointer to the function that will be used to parse @cmd. + * + * Return: 0 on success or >0 if the SCSI command is invalid. + */ static int scst_set_cmd_from_cdb_info(struct scst_cmd *cmd, const struct scst_sdbops *ptr) { @@ -11968,15 +11975,18 @@ static int get_cdb_info_mo(struct scst_cmd *cmd, return get_cdb_info_len_4(cmd, sdbops); } -/* - * scst_get_cdb_info() - fill various info about the command's CDB +/** + * scst_get_cdb_info() - Parse the SCSI CDB according to the SCSI device type. + * @cmd: SCSI command to parse. * * Description: - * Fills various info about the command's CDB in the corresponding fields - * in the command. + * Fill in the following fields in @cmd: cdb_len, cmd_naca, + * cmd_linked, op_name, data_direction, op_flags, lba_off, lba_len, + * len_off, len_len, data_direction, lba, data_len, ... + * Sets SCST_LBA_NOT_VALID if the CDB has not been recognized. * - * Returns: 0 on success, <0 if command is unknown, >0 if command - * is invalid. + * Return: 0 on success, < 0 if the CDB is not recognized, > 0 if the CDB has + * been recognized but is invalid. */ int scst_get_cdb_info(struct scst_cmd *cmd) { diff --git a/scst/src/scst_targ.c b/scst/src/scst_targ.c index aa5c563b0..c9ddf3aa8 100644 --- a/scst/src/scst_targ.c +++ b/scst/src/scst_targ.c @@ -582,23 +582,23 @@ static void __scst_rx_cmd(struct scst_cmd *cmd, struct scst_session *sess, } /** - * scst_rx_cmd_prealloced() - notify SCST that new command received - * @cmd: new cmd to initialized - * @sess: SCST session - * @lun: LUN for the command - * @lun_len: length of the LUN in bytes - * @cdb: CDB of the command - * @cdb_len: length of the CDB in bytes - * @atomic: true, if current context is atomic + * scst_rx_cmd_prealloced() - Notify SCST that a new command has been received. + * @cmd: New cmd to be initialized. + * @sess: SCST session. + * @lun: LUN information from the SCSI transport header. + * @lun_len: Length of the LUN in bytes. + * @cdb: CDB of the command. + * @cdb_len: Length of the CDB in bytes. + * @atomic: True if called from an atomic context. * - * Description: - * Initializes new prealloced SCST command. Returns 0 on success or - * negative error code otherwise. + * Initializes a new preallocated SCST command. * * Must not be called in parallel with scst_unregister_session() for the * same session. * * Cmd supposed to be zeroed! + * + * Return: 0 upon success or a negative error code otherwise. */ int scst_rx_cmd_prealloced(struct scst_cmd *cmd, struct scst_session *sess, const uint8_t *lun, int lun_len, const uint8_t *cdb, @@ -632,20 +632,21 @@ out: EXPORT_SYMBOL(scst_rx_cmd_prealloced); /** - * scst_rx_cmd() - create new command - * @sess: SCST session - * @lun: LUN for the command - * @lun_len: length of the LUN in bytes - * @cdb: CDB of the command - * @cdb_len: length of the CDB in bytes - * @atomic: true, if current context is atomic + * scst_rx_cmd() - Allocate a new SCSI command. + * @sess: SCST session. + * @lun: LUN for the command. + * @lun_len: Length of the LUN in bytes. + * @cdb: CDB of the commandl + * @cdb_len: Length of the CDB in bytes. + * @atomic: True if called from atomic context. * - * Description: - * Creates new SCST command. Returns new command on success or - * NULL otherwise. + * Allocates a new SCST command. * * Must not be called in parallel with scst_unregister_session() for the * same session. + * + * Return: pointer to newly allocated SCST command upon success or NULL upon + * failure. */ struct scst_cmd *scst_rx_cmd(struct scst_session *sess, const uint8_t *lun, int lun_len, const uint8_t *cdb, @@ -680,8 +681,14 @@ out: } EXPORT_SYMBOL(scst_rx_cmd); -/* - * No locks, but might be on IRQ. Returns: +/** + * scst_init_cmd() - ... + * @cmd: SCSI command to initialize. + * @context: Context in which to ... + * + * No locks, but might be on IRQ. + * + * Return: * - < 0 if the caller must not perform any further processing of @cmd; * - >= 0 if the caller must continue processing @cmd. */ @@ -766,29 +773,29 @@ out_redirect: } /** - * scst_cmd_init_done() - the command's initialization done - * @cmd: SCST command - * @pref_context: preferred command execution context + * scst_cmd_init_done() - Tells SCST to start processing a SCSI command. + * @cmd: SCST command. + * @pref_context: Preferred command execution context. * * Description: - * Notifies SCST that the driver finished its part of the command - * initialization, and the command is ready for execution. - * The second argument sets preferred command execution context. - * See SCST_CONTEXT_* constants for details. + * Notifies SCST that the target driver finished its part of the command + * 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 not 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. + * 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) @@ -921,6 +928,16 @@ out: } EXPORT_SYMBOL(scst_cmd_init_done); +/** + * scst_pre_parse() - Parse the SCSI CDB. + * @cmd: SCSI command to parse the CDB of. + * + * Parse the SCSI CDB and call scst_set_cmd_abnormal_done_state() if the CDB + * has been recognized and is invalid. + * + * Return: 0 on success, < 0 if the CDB is not recognized, > 0 if the CDB has + * been recognized but is invalid. + */ int scst_pre_parse(struct scst_cmd *cmd) { int res; @@ -4979,12 +4996,19 @@ struct scst_tgt_dev *scst_lookup_tgt_dev(struct scst_session *sess, u64 lun) return NULL; } -/* - * Returns 0 on success, > 0 when we need to wait for unblock, - * < 0 if there is no device (lun) or device type handler. +/** + * scst_translate_lun() - Translate @cmd->lun into a tgt_dev pointer. + * @cmd: SCSI command for which to translate the LUN number. * - * No locks, but might be on IRQ, protection is done by the - * suspended activity. + * Initialize the following @cmd members: cpu_cmd_counter, cmd_threads, + * tgt_dev, cur_order_data, dev and devt. + * + * The caller must not hold any locks. May be called from IRQ context. The data + * structures used in this function are either protected by an RCU read lock or + * only change while activity is suspended. + * + * Return: 0 on success, > 0 if further processing of @cmd must wait for command + * processing to resume or < 0 if LUN translation failed. */ static int scst_translate_lun(struct scst_cmd *cmd) { @@ -5132,11 +5156,14 @@ out_bypass_aca: return; } -/* +/** + * __scst_init_cmd() - Translate the LUN, parse the CDB and set the sequence nr. + * @cmd: SCSI command to initialize. + * * No locks, but might be on IRQ. * - * Returns 0 on success, > 0 when we need to wait for unblock, - * < 0 if there is no device (lun) or device type handler. + * Return: 0 on success, > 0 if further processing of @cmd must wait for command + * processing to resume or < 0 if LUN translation failed. */ static int __scst_init_cmd(struct scst_cmd *cmd) { @@ -5901,7 +5928,10 @@ out: return res; } -/* No locks */ +/* + * This function is called for aborted commands before a response is sent. The + * caller must not hold any locks. + */ void scst_done_cmd_mgmt(struct scst_cmd *cmd) { struct scst_mgmt_cmd_stub *mstb, *t; @@ -6057,7 +6087,10 @@ void scst_async_mcmd_completed(struct scst_mgmt_cmd *mcmd, int status) } EXPORT_SYMBOL_GPL(scst_async_mcmd_completed); -/* No locks */ +/* + * This function is called for aborted commands after a response has been + * sent. The caller must not hold any locks. + */ void scst_finish_cmd_mgmt(struct scst_cmd *cmd) { struct scst_mgmt_cmd_stub *mstb, *t; @@ -8251,7 +8284,7 @@ out_free: * appropriate error code otherwise * * Description: - * Registers new session. Returns new session on success or NULL otherwise. + * Registers a new session. * * Note: A session creation and initialization is a complex task, * which requires sleeping state, so it can't be fully done @@ -8271,6 +8304,8 @@ out_free: * failed session will be returned in xmit_response() with BUSY status. * In case of failure the driver shall call scst_unregister_session() * inside result_fn(), it will NOT be called automatically. + * + * Return: Session pointer upon success or NULL upon failure. */ struct scst_session *scst_register_session(struct scst_tgt *tgt, int atomic, const char *initiator_name, void *tgt_priv, void *result_fn_data, @@ -8301,12 +8336,14 @@ EXPORT_SYMBOL_GPL(scst_register_session); * appropriate error code otherwise. * * Description: - * Registers new MQ session. Returns new session on success or NULL otherwise. + * Registers a new MQ session. * * MQ session is a session, which is part of a set of sessions from the same * initiator, for instance, one session per CPU. The only difference with * non-MQ sessions is that reservations are not supported on MQ-sessions * (because there is no way to group sessions together from reservations POV) + * + * Return: Session pointer upon success or NULL upon failure. */ struct scst_session *scst_register_session_mq(struct scst_tgt *tgt, int atomic, const char *initiator_name, void *tgt_priv, void *result_fn_data, @@ -8327,7 +8364,9 @@ EXPORT_SYMBOL_GPL(scst_register_session_mq); * @tgt_priv: pointer to target driver's private data * * Description: - * Registers new session. Returns new session on success or NULL otherwise. + * Registers a new session. + * + * Return: Session pointer upon success or NULL upon failure. */ struct scst_session *scst_register_session_non_gpl(struct scst_tgt *tgt, const char *initiator_name, void *tgt_priv) From 456359e1298a568dfa45a641e398a9b6ce4449ba Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Wed, 21 Aug 2019 21:33:16 +0000 Subject: [PATCH 4/5] scst_copy_mgr: Reduce code duplication This patch does not change the behavior of the copy manager. git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@8514 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/scst_copy_mgr.c | 46 +++++++--------------------------------- 1 file changed, 8 insertions(+), 38 deletions(-) diff --git a/scst/src/scst_copy_mgr.c b/scst/src/scst_copy_mgr.c index 562c5e309..4d36ac26d 100644 --- a/scst/src/scst_copy_mgr.c +++ b/scst/src/scst_copy_mgr.c @@ -2551,7 +2551,7 @@ out: static int scst_cm_dev_register(struct scst_device *dev, uint64_t lun) { - int res, i; + int res; struct scst_acg_dev *acg_dev; bool add_lun; @@ -2561,27 +2561,12 @@ static int scst_cm_dev_register(struct scst_device *dev, uint64_t lun) TRACE_DBG("dev %s, LUN %ld", dev->virt_name, (unsigned long)lun); - rcu_read_lock(); - for (i = 0; i < SESS_TGT_DEV_LIST_HASH_SIZE; i++) { - struct scst_tgt_dev *tgt_dev; - struct list_head *head = &scst_cm_sess->sess_tgt_dev_list[i]; - - list_for_each_entry_rcu(tgt_dev, head, - sess_tgt_dev_list_entry) { - if (tgt_dev->dev == dev) { - rcu_read_unlock(); - /* - * It's OK, because the copy manager could - * auto register some devices - */ - TRACE_DBG("Copy Manager already registered " - "device %s", dev->virt_name); - res = 0; - goto out; - } - } + if (scst_cm_get_lun(dev) != SCST_MAX_LUN) { + TRACE_DBG("Copy Manager already registered device %s", + dev->virt_name); + res = 0; + goto out; } - rcu_read_unlock(); if (lun == SCST_MAX_LUN) { add_lun = true; @@ -2630,9 +2615,8 @@ out_err: /* scst_mutex supposed to be held and activities suspended */ static void scst_cm_dev_unregister(struct scst_device *dev, bool del_lun) { - int i; struct scst_cm_desig *des, *t; - u32 lun = SCST_MAX_LUN; + u32 lun; TRACE_ENTRY(); @@ -2651,21 +2635,7 @@ static void scst_cm_dev_unregister(struct scst_device *dev, bool del_lun) if (!del_lun) goto out; - rcu_read_lock(); - for (i = 0; i < SESS_TGT_DEV_LIST_HASH_SIZE; i++) { - struct scst_tgt_dev *tgt_dev; - struct list_head *head = &scst_cm_sess->sess_tgt_dev_list[i]; - - list_for_each_entry_rcu(tgt_dev, head, - sess_tgt_dev_list_entry) { - if (tgt_dev->dev == dev) { - lun = tgt_dev->lun; - break; - } - } - } - rcu_read_unlock(); - + lun = scst_cm_get_lun(dev); if (lun != SCST_MAX_LUN) scst_acg_del_lun(scst_cm_tgt->default_acg, lun, false); From 4aa7817dbb7495c6787a94694781738986e7ec3a Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Wed, 21 Aug 2019 21:34:04 +0000 Subject: [PATCH 5/5] scst_copy_mgr: Avoid that device deletion sporadically causes a hang Avoid that deleting a device concurrently with scst_cm_init_inq_finish() causes command processing to hang. Reported-by: valera Fixes: 0bb6de9471bd ("scst_vdisk: Avoid that LUN refresh triggers a general protection fault" / r7101) git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@8515 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/scst_copy_mgr.c | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/scst/src/scst_copy_mgr.c b/scst/src/scst_copy_mgr.c index 4d36ac26d..bbeec43b7 100644 --- a/scst/src/scst_copy_mgr.c +++ b/scst/src/scst_copy_mgr.c @@ -2341,17 +2341,24 @@ static void scst_cm_init_inq_finish(struct scst_cmd *cmd) /* cmd->dev can be NULL here! */ rc = scst_cm_err_check_retry(cmd, cmd->start_time, scst_cm_inq_retry_fn); - if (rc == SCST_CM_STATUS_RETRY || !cmd->dev || !cmd->tgt_dev) + if (rc == SCST_CM_STATUS_RETRY) + goto out; + + /* + * Since scst_cm_inq_retry_fn() uses cmd->tgt_i_priv, only free that + * pointer once it is clear that that function won't be called. + */ + kfree(priv); + priv = NULL; + cmd->tgt_i_priv = NULL; + + if (WARN_ON_ONCE(!dev)) goto out; spin_lock_bh(&dev->dev_lock); scst_unblock_dev(dev); spin_unlock_bh(&dev->dev_lock); - kfree(priv); - priv = NULL; - cmd->tgt_i_priv = NULL; - if (rc != SCST_CM_STATUS_CMD_SUCCEEDED) { PRINT_CRIT_ERROR("Unable to perform initial INQUIRY for device " "%s. Copy manager for this device will be disabled", @@ -2449,13 +2456,16 @@ out: static int scst_cm_send_init_inquiry(struct scst_device *dev, unsigned int unpacked_lun, struct scst_cm_init_inq_priv *priv) { - int res; + int res = -EINVAL; static const uint8_t inq_cdb[6] = { INQUIRY, 1, 0x83, 0x10, 0, 0 }; __be64 lun; struct scst_cmd *cmd; TRACE_ENTRY(); + if (WARN_ON_ONCE(!dev)) + goto out; + if (priv == NULL) { priv = kzalloc(sizeof(*priv), GFP_KERNEL); if (priv == NULL) {