[02/13] target: split target_submit_cmd_map_sgls

Message ID 20210209123845.4856-3-michael.christie@oracle.com
State New
Headers show
Series
  • target: fix cmd plugging and completion
Related show

Commit Message

Mike Christie Feb. 9, 2021, 12:38 p.m.
Separate target_submit_cmd_map_sgls into the part that does:
- the initial cmd setup
- will not sleep
- and gives us access to the se_device
and the part that:
- can sleep
- handles the actual submission

This will be needed for loop in the next patches which needs to add
the cmd to the lio workqueue and can't sleep in that initial submission
path.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_transport.c | 149 ++++++++++++++++++-------
 1 file changed, 109 insertions(+), 40 deletions(-)

Comments

kernel test robot Feb. 9, 2021, 4:15 p.m. | #1
Hi Mike,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on vhost/linux-next v5.11-rc7 next-20210125]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Mike-Christie/target-move-t_task_cdb-initialization/20210209-213926
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next
config: h8300-randconfig-s031-20210209 (attached as .config)
compiler: h8300-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-215-g0fb77bb6-dirty
        # https://github.com/0day-ci/linux/commit/3382952197b63f11c166ff293f819ce6ac4d52ae
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Mike-Christie/target-move-t_task_cdb-initialization/20210209-213926
        git checkout 3382952197b63f11c166ff293f819ce6ac4d52ae
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=h8300 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
>> drivers/target/target_core_transport.c:1695:12: sparse: sparse: incorrect type in assignment (different base types) @@     expected int rc @@     got restricted sense_reason_t @@
   drivers/target/target_core_transport.c:1695:12: sparse:     expected int rc
   drivers/target/target_core_transport.c:1695:12: sparse:     got restricted sense_reason_t
   drivers/target/target_core_transport.c:1699:12: sparse: sparse: incorrect type in assignment (different base types) @@     expected int rc @@     got restricted sense_reason_t @@
   drivers/target/target_core_transport.c:1699:12: sparse:     expected int rc
   drivers/target/target_core_transport.c:1699:12: sparse:     got restricted sense_reason_t
>> drivers/target/target_core_transport.c:1736:51: sparse: sparse: incorrect type in argument 2 (different base types) @@     expected restricted sense_reason_t [usertype] @@     got int rc @@
   drivers/target/target_core_transport.c:1736:51: sparse:     expected restricted sense_reason_t [usertype]
   drivers/target/target_core_transport.c:1736:51: sparse:     got int rc

vim +1695 drivers/target/target_core_transport.c

  1678	
  1679	/**
  1680	 * target_submit - perform final initialization and submit cmd to LIO core
  1681	 * @se_cmd: command descriptor to submit
  1682	 * @cdb: pointer to SCSI CDB
  1683	 *
  1684	 * target_submit_prep must have been called on the cmd, and this must be
  1685	 * called from process context.
  1686	 */
  1687	static void target_submit(struct se_cmd *se_cmd, unsigned char *cdb)
  1688	{
  1689		struct scatterlist *sgl = se_cmd->t_data_sg;
  1690		unsigned char *buf = NULL;
  1691		int rc;
  1692	
  1693		might_sleep();
  1694	
> 1695		rc = target_cmd_init_cdb(se_cmd, cdb);
  1696		if (rc)
  1697			goto fail;
  1698	
  1699		rc = target_cmd_parse_cdb(se_cmd);
  1700		if (rc != 0)
  1701			goto fail;
  1702	
  1703		if (se_cmd->t_data_nents != 0) {
  1704			BUG_ON(!sgl);
  1705			/*
  1706			 * A work-around for tcm_loop as some userspace code via
  1707			 * scsi-generic do not memset their associated read buffers,
  1708			 * so go ahead and do that here for type non-data CDBs.  Also
  1709			 * note that this is currently guaranteed to be a single SGL
  1710			 * for this case by target core in target_setup_cmd_from_cdb()
  1711			 * -> transport_generic_cmd_sequencer().
  1712			 */
  1713			if (!(se_cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) &&
  1714			     se_cmd->data_direction == DMA_FROM_DEVICE) {
  1715				if (sgl)
  1716					buf = kmap(sg_page(sgl)) + sgl->offset;
  1717	
  1718				if (buf) {
  1719					memset(buf, 0, sgl->length);
  1720					kunmap(sg_page(sgl));
  1721				}
  1722			}
  1723	
  1724		}
  1725	
  1726		/*
  1727		 * Check if we need to delay processing because of ALUA
  1728		 * Active/NonOptimized primary access state..
  1729		 */
  1730		core_alua_check_nonop_delay(se_cmd);
  1731	
  1732		transport_handle_cdb_direct(se_cmd);
  1733		return;
  1734	
  1735	fail:
> 1736		transport_generic_request_failure(se_cmd, rc);
  1737	}
  1738	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Christoph Hellwig Feb. 10, 2021, 8:36 a.m. | #2
Can you just kill off target_submit_cmd_map_sgls entirely and just
open code the two calls in the three callers?

Patch

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 5c4adde96d5e..71b0a862608b 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1571,12 +1571,9 @@  transport_generic_map_mem_to_cmd(struct se_cmd *cmd, struct scatterlist *sgl,
 }
 
 /**
- * target_submit_cmd_map_sgls - lookup unpacked lun and submit uninitialized
- * 			 se_cmd + use pre-allocated SGL memory.
- *
+ * target_submit_prep - prep cmd for submission to lio core
  * @se_cmd: command descriptor to submit
  * @se_sess: associated se_sess for endpoint
- * @cdb: pointer to SCSI CDB
  * @sense: pointer to SCSI sense buffer
  * @unpacked_lun: unpacked LUN to reference for struct se_lun
  * @data_length: fabric expected data transfer length
@@ -1592,26 +1589,29 @@  transport_generic_map_mem_to_cmd(struct se_cmd *cmd, struct scatterlist *sgl,
  *
  * Task tags are supported if the caller has set @se_cmd->tag.
  *
- * Returns non zero to signal active I/O shutdown failure.  All other
- * setup exceptions will be returned as a SCSI CHECK_CONDITION response,
- * but still return zero here.
+ * Returns:
+ *	- less than zero to signal active I/O shutdown failure
+ *	- zero on success.
+ *	- one for all other setup exceptions. The cmd will be returned as a
+ *	  SCSI CHECK_CONDITION response in this case.
  *
- * This may only be called from process context, and also currently
- * assumes internal allocation of fabric payload buffer by target-core.
+ * This may only be called from interrupt context if the caller's
+ * queue_status and release_cmd callouts do not block.
+ *
+ * This assumes internal allocation of fabric payload buffer by target-core.
  */
-int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess,
-		unsigned char *cdb, unsigned char *sense, u64 unpacked_lun,
-		u32 data_length, int task_attr, int data_dir, int flags,
-		struct scatterlist *sgl, u32 sgl_count,
-		struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
-		struct scatterlist *sgl_prot, u32 sgl_prot_count)
+static int
+target_submit_prep(struct se_cmd *se_cmd, struct se_session *se_sess,
+		   unsigned char *sense, u64 unpacked_lun,
+		   u32 data_length, int task_attr, int data_dir, int flags,
+		   struct scatterlist *sgl, u32 sgl_count,
+		   struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
+		   struct scatterlist *sgl_prot, u32 sgl_prot_count)
 {
 	struct se_portal_group *se_tpg;
 	sense_reason_t rc;
 	int ret;
 
-	might_sleep();
-
 	se_tpg = se_sess->se_tpg;
 	BUG_ON(!se_tpg);
 	BUG_ON(se_cmd->se_tfo || se_cmd->se_sess);
@@ -1642,14 +1642,6 @@  int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 	 */
 	if (flags & TARGET_SCF_BIDI_OP)
 		se_cmd->se_cmd_flags |= SCF_BIDI;
-
-	rc = target_cmd_init_cdb(se_cmd, cdb);
-	if (rc) {
-		transport_send_check_condition_and_sense(se_cmd, rc, 0);
-		target_put_sess_cmd(se_cmd);
-		return 0;
-	}
-
 	/*
 	 * Locate se_lun pointer and attach it to struct se_cmd
 	 */
@@ -1657,13 +1649,7 @@  int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 	if (rc) {
 		transport_send_check_condition_and_sense(se_cmd, rc, 0);
 		target_put_sess_cmd(se_cmd);
-		return 0;
-	}
-
-	rc = target_cmd_parse_cdb(se_cmd);
-	if (rc != 0) {
-		transport_generic_request_failure(se_cmd, rc);
-		return 0;
+		return 1;
 	}
 
 	/*
@@ -1684,6 +1670,43 @@  int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 	if (sgl_count != 0) {
 		BUG_ON(!sgl);
 
+		rc = transport_generic_map_mem_to_cmd(se_cmd, sgl, sgl_count,
+				sgl_bidi, sgl_bidi_count);
+		if (rc != 0) {
+			transport_generic_request_failure(se_cmd, rc);
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+/**
+ * target_submit - perform final initialization and submit cmd to LIO core
+ * @se_cmd: command descriptor to submit
+ * @cdb: pointer to SCSI CDB
+ *
+ * target_submit_prep must have been called on the cmd, and this must be
+ * called from process context.
+ */
+static void target_submit(struct se_cmd *se_cmd, unsigned char *cdb)
+{
+	struct scatterlist *sgl = se_cmd->t_data_sg;
+	unsigned char *buf = NULL;
+	int rc;
+
+	might_sleep();
+
+	rc = target_cmd_init_cdb(se_cmd, cdb);
+	if (rc)
+		goto fail;
+
+	rc = target_cmd_parse_cdb(se_cmd);
+	if (rc != 0)
+		goto fail;
+
+	if (se_cmd->t_data_nents != 0) {
+		BUG_ON(!sgl);
 		/*
 		 * A work-around for tcm_loop as some userspace code via
 		 * scsi-generic do not memset their associated read buffers,
@@ -1694,8 +1717,6 @@  int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 		 */
 		if (!(se_cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) &&
 		     se_cmd->data_direction == DMA_FROM_DEVICE) {
-			unsigned char *buf = NULL;
-
 			if (sgl)
 				buf = kmap(sg_page(sgl)) + sgl->offset;
 
@@ -1705,12 +1726,6 @@  int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 			}
 		}
 
-		rc = transport_generic_map_mem_to_cmd(se_cmd, sgl, sgl_count,
-				sgl_bidi, sgl_bidi_count);
-		if (rc != 0) {
-			transport_generic_request_failure(se_cmd, rc);
-			return 0;
-		}
 	}
 
 	/*
@@ -1720,6 +1735,60 @@  int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 	core_alua_check_nonop_delay(se_cmd);
 
 	transport_handle_cdb_direct(se_cmd);
+	return;
+
+fail:
+	transport_generic_request_failure(se_cmd, rc);
+}
+
+/**
+ * target_submit_cmd_map_sgls - lookup unpacked lun and submit uninitialized
+ *					se_cmd + use pre-allocated SGL memory.
+ *
+ * @se_cmd: command descriptor to submit
+ * @se_sess: associated se_sess for endpoint
+ * @cdb: pointer to SCSI CDB
+ * @sense: pointer to SCSI sense buffer
+ * @unpacked_lun: unpacked LUN to reference for struct se_lun
+ * @data_length: fabric expected data transfer length
+ * @task_attr: SAM task attribute
+ * @data_dir: DMA data direction
+ * @flags: flags for command submission from target_sc_flags_tables
+ * @sgl: struct scatterlist memory for unidirectional mapping
+ * @sgl_count: scatterlist count for unidirectional mapping
+ * @sgl_bidi: struct scatterlist memory for bidirectional READ mapping
+ * @sgl_bidi_count: scatterlist count for bidirectional READ mapping
+ * @sgl_prot: struct scatterlist memory protection information
+ * @sgl_prot_count: scatterlist count for protection information
+ *
+ * Task tags are supported if the caller has set @se_cmd->tag.
+ *
+ * Returns non zero to signal active I/O shutdown failure.  All other
+ * setup exceptions will be returned as a SCSI CHECK_CONDITION response,
+ * but still return zero here.
+ *
+ * This may only be called from process context, and also currently
+ * assumes internal allocation of fabric payload buffer by target-core.
+ */
+int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess,
+		unsigned char *cdb, unsigned char *sense, u64 unpacked_lun,
+		u32 data_length, int task_attr, int data_dir, int flags,
+		struct scatterlist *sgl, u32 sgl_count,
+		struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
+		struct scatterlist *sgl_prot, u32 sgl_prot_count)
+{
+	int ret;
+
+	ret = target_submit_prep(se_cmd, se_sess, sense, unpacked_lun,
+				 data_length, task_attr, data_dir, flags,
+				 sgl, sgl_count, sgl_bidi, sgl_bidi_count,
+				 sgl_prot, sgl_prot_count);
+	if (ret < 0)
+		return ret;
+	else if (ret > 0)
+		return 0;
+
+	target_submit(se_cmd, cdb);
 	return 0;
 }
 EXPORT_SYMBOL(target_submit_cmd_map_sgls);