[PATCH 0/4] target: Add target_submit_cmd_map_mem and convert tcm_loop+tcm_vhost

[PATCH 0/4] target: Add target_submit_cmd_map_mem and convert tcm_loop+tcm_vhost

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

Hi hch & Co,

This series adds a new target_submit_cmd_map_mem() caller to accept
pre-allocated SGL memory within the core generic I/O submission path.
Patch #1 contains the core I/O changes, patch #2 + #4 includes the
conversion of tcm_loop+tcm_vhost to use this new caller -> drop their
internal open-coded equivalents using transport_generic_map_mem_to_cmd().

Patch #3 carries forward a work-around for tcm_loop w/ scsi-generic with
user-space code that does not zero out it's READ payload buffer + ends
up passing a payload filled with random data into target core's control CDB
emulation.  Since we're not using bounce buffers any more for control
CDB emulation in modern v3.x code, AFAICT tcm_loop still requires this
extra bit to function properly with some legacy user-space code.

Regardless, the main I/O path changes end up being very mechnical in
nature for existing core and fabric code, and has been running as expected
with fio small-block workloads last hours.

Please review.

Thanks Christoph!

--nab

Nicholas Bellinger (4):
  target: Add target_submit_cmd_map_mem for SGL fabric memory
    passthrough
  tcm_loop: Convert I/O path to use target_submit_cmd_map_mem
  target: Add TARGET_SCF_MAP_CLEAR_MEM work-around for tcm_loop
  tcm_vhost: Convert I/O path to use target_submit_cmd_map_mem

 drivers/target/loopback/tcm_loop.c     |   62 ++-------------
 drivers/target/target_core_transport.c |  129 ++++++++++++++++++++++++++------
 drivers/vhost/tcm_vhost.c              |   68 ++++-------------
 drivers/vhost/tcm_vhost.h              |    8 ++
 include/target/target_core_base.h      |    2 +
 include/target/target_core_fabric.h    |    3 +
 6 files changed, 141 insertions(+), 131 deletions(-)

-- 
1.7.2.5

[PATCH 1/4] target: Add target_submit_cmd_map_mem for SGL fabric memory passthrough

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch adds a new TARGET_SCF_MAP_MEM flag within __target_submit_cmd()
+ a target_submit_cmd_map_mem() wrapper to pass pre-allocated SGL memory
using existing transport_generic_map_mem_to_cmd() logic into the generic
target submit I/O codepath.

Reported-by: Christoph Hellwig <hch@lst.de>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c |  108 +++++++++++++++++++++++++-------
 include/target/target_core_base.h      |    1 +
 include/target/target_core_fabric.h    |    3 +
 3 files changed, 89 insertions(+), 23 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 221f67f..ad2097e 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1455,29 +1455,11 @@ int transport_handle_cdb_direct(
 }
 EXPORT_SYMBOL(transport_handle_cdb_direct);
 
-/**
- * target_submit_cmd - lookup unpacked lun and submit uninitialized se_cmd
- *
- * @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_addr: SAM task attribute
- * @data_dir: DMA data direction
- * @flags: flags for command submission from target_sc_flags_tables
- *
- * 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(struct se_cmd *se_cmd, struct se_session *se_sess,
+static int __target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 		unsigned char *cdb, unsigned char *sense, u32 unpacked_lun,
-		u32 data_length, int task_attr, int data_dir, int flags)
+		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 se_portal_group *se_tpg;
 	int rc;
@@ -1524,7 +1506,19 @@ int target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 		transport_generic_request_failure(se_cmd);
 		return 0;
 	}
-
+	/*
+	 * When TARGET_SCF_MAP_MEM has been set perform a SGL passthrough
+	 * mapping for pre-allocated fabric memory, instead of having target
+	 * core perform an internal SGL allocation.
+	 */
+	if (flags & TARGET_SCF_MAP_MEM) {
+		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);
+			return 0;
+		}
+	}
 	/*
 	 * Check if we need to delay processing because of ALUA
 	 * Active/NonOptimized primary access state..
@@ -1534,8 +1528,76 @@ int target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 	transport_handle_cdb_direct(se_cmd);
 	return 0;
 }
+
+/*
+ * target_submit_cmd - lookup unpacked lun and submit uninitialized se_cmd
+ *
+ * @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_addr: SAM task attribute
+ * @data_dir: DMA data direction
+ * @flags: flags for command submission from target_sc_flags_tables
+ *
+ * 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(struct se_cmd *se_cmd, struct se_session *se_sess,
+		unsigned char *cdb, unsigned char *sense, u32 unpacked_lun,
+		u32 data_length, int task_attr, int data_dir, int flags)
+{
+	return __target_submit_cmd(se_cmd, se_sess, cdb, sense, unpacked_lun,
+			data_length, task_attr, data_dir, flags,
+			NULL, 0, NULL, 0);
+}
 EXPORT_SYMBOL(target_submit_cmd);
 
+/*
+ * target_submit_cmd_map_mem - lookup unpacked lun and submit uninitialized
+ * 			       se_cmd using pre-allocated 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_addr: 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
+ *
+ * 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_mem(struct se_cmd *se_cmd, struct se_session *se_sess,
+		unsigned char *cdb, unsigned char *sense, u32 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)
+{
+	int lflags = TARGET_SCF_MAP_MEM | flags;
+
+	return __target_submit_cmd(se_cmd, se_sess, cdb, sense, unpacked_lun,
+			data_length, task_attr, data_dir, lflags,
+			sgl, sgl_count, sgl_bidi, sgl_bidi_count);
+}
+EXPORT_SYMBOL(target_submit_cmd_map_mem);
+
 static void target_complete_tmr_failure(struct work_struct *work)
 {
 	struct se_cmd *se_cmd = container_of(work, struct se_cmd, work);
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 5be8937..6309298 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -220,6 +220,7 @@ enum target_sc_flags_table {
 	TARGET_SCF_BIDI_OP		= 0x01,
 	TARGET_SCF_ACK_KREF		= 0x02,
 	TARGET_SCF_UNKNOWN_SIZE		= 0x04,
+	TARGET_SCF_MAP_MEM		= 0x08,
 };
 
 /* fabric independent task management function values */
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 85a5d7a..f315c80 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -102,6 +102,9 @@ int	transport_lookup_cmd_lun(struct se_cmd *, u32);
 int	target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *);
 int	target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
 		unsigned char *, u32, u32, int, int, int);
+int	target_submit_cmd_map_mem(struct se_cmd *, struct se_session *,
+		unsigned char *, unsigned char *, u32, u32, int, int, int,
+		struct scatterlist *, u32, struct scatterlist *, u32);
 int	target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
 		unsigned char *sense, u32 unpacked_lun,
 		void *fabric_tmr_ptr, unsigned char tm_type,
-- 
1.7.2.5

[PATCH 2/4] tcm_loop: Convert I/O path to use target_submit_cmd_map_mem

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch converts tcm_loop to use target_submit_cmd_map_mem() for
I/O submission and mapping of pre-allocated SGL memory from incoming
scsi_cmnd -> se_cmd descriptors.

This includes removing the original open-coded fabric uses of target
core callers to support transport_generic_map_mem_to_cmd() between
target_setup_cmd_from_cdb() and transport_handle_cdb_direct() logic.

Reported-by: Christoph Hellwig <hch@lst.de>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/loopback/tcm_loop.c |   62 +++++-------------------------------
 1 files changed, 8 insertions(+), 54 deletions(-)

diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 7a0da1a..e20b809 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -166,7 +166,7 @@ static void tcm_loop_submission_work(struct work_struct *work)
 	struct tcm_loop_tpg *tl_tpg;
 	struct scatterlist *sgl_bidi = NULL;
 	u32 sgl_bidi_count = 0;
-	int ret;
+	int rc;
 
 	tl_hba = *(struct tcm_loop_hba **)shost_priv(sc->device->host);
 	tl_tpg = &tl_hba->tl_hba_tpgs[sc->device->id];
@@ -187,12 +187,6 @@ static void tcm_loop_submission_work(struct work_struct *work)
 		set_host_byte(sc, DID_ERROR);
 		goto out_done;
 	}
-
-	transport_init_se_cmd(se_cmd, tl_tpg->tl_se_tpg.se_tpg_tfo,
-			tl_nexus->se_sess,
-			scsi_bufflen(sc), sc->sc_data_direction,
-			tcm_loop_sam_attr(sc), &tl_cmd->tl_sense_buf[0]);
-
 	if (scsi_bidi_cmnd(sc)) {
 		struct scsi_data_buffer *sdb = scsi_in(sc);
 
@@ -201,56 +195,16 @@ static void tcm_loop_submission_work(struct work_struct *work)
 		se_cmd->se_cmd_flags |= SCF_BIDI;
 
 	}
-
-	if (transport_lookup_cmd_lun(se_cmd, tl_cmd->sc->device->lun) < 0) {
-		kmem_cache_free(tcm_loop_cmd_cache, tl_cmd);
+	rc = target_submit_cmd_map_mem(se_cmd, tl_nexus->se_sess, sc->cmnd,
+			&tl_cmd->tl_sense_buf[0], tl_cmd->sc->device->lun,
+			scsi_bufflen(sc), tcm_loop_sam_attr(sc),
+			sc->sc_data_direction, 0,
+			scsi_sglist(sc), scsi_sg_count(sc),
+			sgl_bidi, sgl_bidi_count);
+	if (rc < 0) {
 		set_host_byte(sc, DID_NO_CONNECT);
 		goto out_done;
 	}
-
-	/*
-	 * Because some userspace code via scsi-generic do not memset their
-	 * associated read buffers, go ahead and do that here for type
-	 * non-data CDBs.  Also note that this is currently guaranteed to be a
-	 * single SGL for this case by target core in
-	 * target_setup_cmd_from_cdb() -> transport_generic_cmd_sequencer().
-	 */
-	if (!(se_cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) &&
-	    se_cmd->data_direction == DMA_FROM_DEVICE) {
-		struct scatterlist *sg = scsi_sglist(sc);
-		unsigned char *buf = kmap(sg_page(sg)) + sg->offset;
-
-		if (buf != NULL) {
-			memset(buf, 0, sg->length);
-			kunmap(sg_page(sg));
-		}
-	}
-
-	ret = target_setup_cmd_from_cdb(se_cmd, sc->cmnd);
-	if (ret == -ENOMEM) {
-		transport_send_check_condition_and_sense(se_cmd,
-				TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
-		transport_generic_free_cmd(se_cmd, 0);
-		return;
-	} else if (ret < 0) {
-		if (se_cmd->se_cmd_flags & SCF_SCSI_RESERVATION_CONFLICT)
-			tcm_loop_queue_status(se_cmd);
-		else
-			transport_send_check_condition_and_sense(se_cmd,
-					se_cmd->scsi_sense_reason, 0);
-		transport_generic_free_cmd(se_cmd, 0);
-		return;
-	}
-
-	ret = transport_generic_map_mem_to_cmd(se_cmd, scsi_sglist(sc),
-			scsi_sg_count(sc), sgl_bidi, sgl_bidi_count);
-	if (ret) {
-		transport_send_check_condition_and_sense(se_cmd,
-					se_cmd->scsi_sense_reason, 0);
-		transport_generic_free_cmd(se_cmd, 0);
-		return;
-	}
-	transport_handle_cdb_direct(se_cmd);
 	return;
 
 out_done:
-- 
1.7.2.5

[PATCH 3/4] target: Add TARGET_SCF_MAP_CLEAR_MEM work-around for tcm_loop

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch carries forward a work-around from tcm_loop to target
core code to explicitly clear control CDB READ paylods in order to
avoid bugs in scsi-generic user-space code for INQUIRY that do not
explicitly zero CDB payload memory.

Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/loopback/tcm_loop.c     |    2 +-
 drivers/target/target_core_transport.c |   21 +++++++++++++++++++++
 include/target/target_core_base.h      |    1 +
 3 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index e20b809..911381f 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -198,7 +198,7 @@ static void tcm_loop_submission_work(struct work_struct *work)
 	rc = target_submit_cmd_map_mem(se_cmd, tl_nexus->se_sess, sc->cmnd,
 			&tl_cmd->tl_sense_buf[0], tl_cmd->sc->device->lun,
 			scsi_bufflen(sc), tcm_loop_sam_attr(sc),
-			sc->sc_data_direction, 0,
+			sc->sc_data_direction, TARGET_SCF_MAP_CLEAR_MEM,
 			scsi_sglist(sc), scsi_sg_count(sc),
 			sgl_bidi, sgl_bidi_count);
 	if (rc < 0) {
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index ad2097e..665ace5 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1512,6 +1512,27 @@ static int __target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess
 	 * core perform an internal SGL allocation.
 	 */
 	if (flags & TARGET_SCF_MAP_MEM) {
+		/*
+		 * A work-around for tcm_loop as some userspace code via
+		 * scsi-generic do not memset their associated read buffers,
+		 * so go ahead and do that here for type non-data CDBs.  Also
+		 * note that this is currently guaranteed to be a single SGL
+		 * for this case by target core in target_setup_cmd_from_cdb()
+		 * -> transport_generic_cmd_sequencer().
+		 */
+		if (flags & TARGET_SCF_MAP_CLEAR_MEM &&
+		   !(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;
+
+			if (buf) {
+				memset(buf, 0, sgl->length);
+				kunmap(sg_page(sgl));
+			}
+		}
 		rc = transport_generic_map_mem_to_cmd(se_cmd, sgl, sgl_count,
 				sgl_bidi, sgl_bidi_count);
 		if (rc != 0) {
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 6309298..f660ad2 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -221,6 +221,7 @@ enum target_sc_flags_table {
 	TARGET_SCF_ACK_KREF		= 0x02,
 	TARGET_SCF_UNKNOWN_SIZE		= 0x04,
 	TARGET_SCF_MAP_MEM		= 0x08,
+	TARGET_SCF_MAP_CLEAR_MEM	= 0x10,
 };
 
 /* fabric independent task management function values */
-- 
1.7.2.5

[PATCH 4/4] tcm_vhost: Convert I/O path to use target_submit_cmd_map_mem

[Nicholas A. Bellinger]

From: Nicholas Bellinger <nab@linux-iscsi.org>

This patch converts tcm_vhost to use target_submit_cmd_map_mem() for
I/O submission and mapping of pre-allocated SGL memory from incoming
virtio-scsi SGL memory -> se_cmd descriptors.

This includes removing the original open-coded fabric uses of target
core callers to support transport_generic_map_mem_to_cmd() between
target_setup_cmd_from_cdb() and transport_handle_cdb_direct() logic.

It also includes adding a handful of new tcm_vhost_cmnd member +
assignments in vhost_scsi_allocate_cmd() used from cmwq process
context I/O submission within tcm_vhost_submission_work()

Reported-by: Christoph Hellwig <hch@lst.de>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Stefan Hajnoczi <stefanha@gmail.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/vhost/tcm_vhost.c |   68 +++++++++-----------------------------------
 drivers/vhost/tcm_vhost.h |    8 +++++
 2 files changed, 22 insertions(+), 54 deletions(-)

diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
index 89dc99b..2ca5f02 100644
--- a/drivers/vhost/tcm_vhost.c
+++ b/drivers/vhost/tcm_vhost.c
@@ -415,10 +415,7 @@ static struct tcm_vhost_cmd *vhost_scsi_allocate_cmd(
 {
 	struct tcm_vhost_cmd *tv_cmd;
 	struct tcm_vhost_nexus *tv_nexus;
-	struct se_portal_group *se_tpg = &tv_tpg->se_tpg;
 	struct se_session *se_sess;
-	struct se_cmd *se_cmd;
-	int sam_task_attr;
 
 	tv_nexus = tv_tpg->tpg_nexus;
 	if (!tv_nexus) {
@@ -434,23 +431,11 @@ static struct tcm_vhost_cmd *vhost_scsi_allocate_cmd(
 	}
 	INIT_LIST_HEAD(&tv_cmd->tvc_completion_list);
 	tv_cmd->tvc_tag = v_req->tag;
+	tv_cmd->tvc_task_attr = v_req->task_attr;
+	tv_cmd->tvc_exp_data_len = exp_data_len;
+	tv_cmd->tvc_data_direction = data_direction;
+	tv_cmd->tvc_nexus = tv_nexus;
 
-	se_cmd = &tv_cmd->tvc_se_cmd;
-	/*
-	 * Locate the SAM Task Attr from virtio_scsi_cmd_req
-	 */
-	sam_task_attr = v_req->task_attr;
-	/*
-	 * Initialize struct se_cmd descriptor from TCM infrastructure
-	 */
-	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess, exp_data_len,
-				data_direction, sam_task_attr,
-				&tv_cmd->tvc_sense_buf[0]);
-
-#if 0	/* FIXME: vhost_scsi_allocate_cmd() BIDI operation */
-	if (bidi)
-		se_cmd->se_cmd_flags |= SCF_BIDI;
-#endif
 	return tv_cmd;
 }
 
@@ -549,37 +534,10 @@ static void tcm_vhost_submission_work(struct work_struct *work)
 {
 	struct tcm_vhost_cmd *tv_cmd =
 		container_of(work, struct tcm_vhost_cmd, work);
+	struct tcm_vhost_nexus *tv_nexus;
 	struct se_cmd *se_cmd = &tv_cmd->tvc_se_cmd;
 	struct scatterlist *sg_ptr, *sg_bidi_ptr = NULL;
 	int rc, sg_no_bidi = 0;
-	/*
-	 * Locate the struct se_lun pointer based on v_req->lun, and
-	 * attach it to struct se_cmd
-	 */
-	rc = transport_lookup_cmd_lun(&tv_cmd->tvc_se_cmd, tv_cmd->tvc_lun);
-	if (rc < 0) {
-		pr_err("Failed to look up lun: %d\n", tv_cmd->tvc_lun);
-		transport_send_check_condition_and_sense(&tv_cmd->tvc_se_cmd,
-			tv_cmd->tvc_se_cmd.scsi_sense_reason, 0);
-		transport_generic_free_cmd(se_cmd, 0);
-		return;
-	}
-
-	rc = target_setup_cmd_from_cdb(se_cmd, tv_cmd->tvc_cdb);
-	if (rc == -ENOMEM) {
-		transport_send_check_condition_and_sense(se_cmd,
-				TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
-		transport_generic_free_cmd(se_cmd, 0);
-		return;
-	} else if (rc < 0) {
-		if (se_cmd->se_cmd_flags & SCF_SCSI_RESERVATION_CONFLICT)
-			tcm_vhost_queue_status(se_cmd);
-		else
-			transport_send_check_condition_and_sense(se_cmd,
-					se_cmd->scsi_sense_reason, 0);
-		transport_generic_free_cmd(se_cmd, 0);
-		return;
-	}
 
 	if (tv_cmd->tvc_sgl_count) {
 		sg_ptr = tv_cmd->tvc_sgl;
@@ -597,17 +555,19 @@ static void tcm_vhost_submission_work(struct work_struct *work)
 	} else {
 		sg_ptr = NULL;
 	}
-
-	rc = transport_generic_map_mem_to_cmd(se_cmd, sg_ptr,
-				tv_cmd->tvc_sgl_count, sg_bidi_ptr,
-				sg_no_bidi);
+	tv_nexus = tv_cmd->tvc_nexus;
+
+	rc = target_submit_cmd_map_mem(se_cmd, tv_nexus->tvn_se_sess,
+			tv_cmd->tvc_cdb, &tv_cmd->tvc_sense_buf[0],
+			tv_cmd->tvc_lun, tv_cmd->tvc_exp_data_len,
+			tv_cmd->tvc_task_attr, tv_cmd->tvc_data_direction,
+			0, sg_ptr, tv_cmd->tvc_sgl_count,
+			sg_bidi_ptr, sg_no_bidi);
 	if (rc < 0) {
 		transport_send_check_condition_and_sense(se_cmd,
-				se_cmd->scsi_sense_reason, 0);
+				TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
 		transport_generic_free_cmd(se_cmd, 0);
-		return;
 	}
-	transport_handle_cdb_direct(se_cmd);
 }
 
 static void vhost_scsi_handle_vq(struct vhost_scsi *vs)
diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
index d9e9355..7e87c63 100644
--- a/drivers/vhost/tcm_vhost.h
+++ b/drivers/vhost/tcm_vhost.h
@@ -5,6 +5,12 @@
 struct tcm_vhost_cmd {
 	/* Descriptor from vhost_get_vq_desc() for virt_queue segment */
 	int tvc_vq_desc;
+	/* virtio-scsi initiator task attribute */
+	int tvc_task_attr;
+	/* virtio-scsi initiator data direction */
+	enum dma_data_direction tvc_data_direction;
+	/* Expected data transfer length from virtio-scsi header */
+	u32 tvc_exp_data_len;
 	/* The Tag from include/linux/virtio_scsi.h:struct virtio_scsi_cmd_req */
 	u64 tvc_tag;
 	/* The number of scatterlists associated with this cmd */
@@ -17,6 +23,8 @@ struct tcm_vhost_cmd {
 	struct virtio_scsi_cmd_resp __user *tvc_resp;
 	/* Pointer to vhost_scsi for our device */
 	struct vhost_scsi *tvc_vhost;
+	/* Pointer to vhost nexus memory */
+	struct tcm_vhost_nexus *tvc_nexus;
 	/* The TCM I/O descriptor that is accessed via container_of() */
 	struct se_cmd tvc_se_cmd;
 	/* work item used for cmwq dispatch to tcm_vhost_submission_work() */
-- 
1.7.2.5

Re: [PATCH 4/4] tcm_vhost: Convert I/O path to use target_submit_cmd_map_mem

[Michael S. Tsirkin]

On Tue, Oct 02, 2012 at 07:15:47AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch converts tcm_vhost to use target_submit_cmd_map_mem() for
> I/O submission and mapping of pre-allocated SGL memory from incoming
> virtio-scsi SGL memory -> se_cmd descriptors.
> 
> This includes removing the original open-coded fabric uses of target
> core callers to support transport_generic_map_mem_to_cmd() between
> target_setup_cmd_from_cdb() and transport_handle_cdb_direct() logic.
> 
> It also includes adding a handful of new tcm_vhost_cmnd member +
> assignments in vhost_scsi_allocate_cmd() used from cmwq process
> context I/O submission within tcm_vhost_submission_work()
> 
> Reported-by: Christoph Hellwig <hch@lst.de>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Cc: Stefan Hajnoczi <stefanha@gmail.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
> ---
>  drivers/vhost/tcm_vhost.c |   68 +++++++++-----------------------------------
>  drivers/vhost/tcm_vhost.h |    8 +++++
>  2 files changed, 22 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> index 89dc99b..2ca5f02 100644
> --- a/drivers/vhost/tcm_vhost.c
> +++ b/drivers/vhost/tcm_vhost.c
> @@ -415,10 +415,7 @@ static struct tcm_vhost_cmd *vhost_scsi_allocate_cmd(
>  {
>  	struct tcm_vhost_cmd *tv_cmd;
>  	struct tcm_vhost_nexus *tv_nexus;
> -	struct se_portal_group *se_tpg = &tv_tpg->se_tpg;
>  	struct se_session *se_sess;
> -	struct se_cmd *se_cmd;
> -	int sam_task_attr;
>  
>  	tv_nexus = tv_tpg->tpg_nexus;
>  	if (!tv_nexus) {
> @@ -434,23 +431,11 @@ static struct tcm_vhost_cmd *vhost_scsi_allocate_cmd(
>  	}
>  	INIT_LIST_HEAD(&tv_cmd->tvc_completion_list);
>  	tv_cmd->tvc_tag = v_req->tag;
> +	tv_cmd->tvc_task_attr = v_req->task_attr;
> +	tv_cmd->tvc_exp_data_len = exp_data_len;
> +	tv_cmd->tvc_data_direction = data_direction;
> +	tv_cmd->tvc_nexus = tv_nexus;
>  
> -	se_cmd = &tv_cmd->tvc_se_cmd;
> -	/*
> -	 * Locate the SAM Task Attr from virtio_scsi_cmd_req
> -	 */
> -	sam_task_attr = v_req->task_attr;
> -	/*
> -	 * Initialize struct se_cmd descriptor from TCM infrastructure
> -	 */
> -	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess, exp_data_len,
> -				data_direction, sam_task_attr,
> -				&tv_cmd->tvc_sense_buf[0]);
> -
> -#if 0	/* FIXME: vhost_scsi_allocate_cmd() BIDI operation */
> -	if (bidi)
> -		se_cmd->se_cmd_flags |= SCF_BIDI;
> -#endif
>  	return tv_cmd;
>  }
>  
> @@ -549,37 +534,10 @@ static void tcm_vhost_submission_work(struct work_struct *work)
>  {
>  	struct tcm_vhost_cmd *tv_cmd =
>  		container_of(work, struct tcm_vhost_cmd, work);
> +	struct tcm_vhost_nexus *tv_nexus;
>  	struct se_cmd *se_cmd = &tv_cmd->tvc_se_cmd;
>  	struct scatterlist *sg_ptr, *sg_bidi_ptr = NULL;
>  	int rc, sg_no_bidi = 0;
> -	/*
> -	 * Locate the struct se_lun pointer based on v_req->lun, and
> -	 * attach it to struct se_cmd
> -	 */
> -	rc = transport_lookup_cmd_lun(&tv_cmd->tvc_se_cmd, tv_cmd->tvc_lun);
> -	if (rc < 0) {
> -		pr_err("Failed to look up lun: %d\n", tv_cmd->tvc_lun);
> -		transport_send_check_condition_and_sense(&tv_cmd->tvc_se_cmd,
> -			tv_cmd->tvc_se_cmd.scsi_sense_reason, 0);
> -		transport_generic_free_cmd(se_cmd, 0);
> -		return;
> -	}
> -

IIUC tv_cmd can come from userspace so calling pr_error here
was a bug, it's good that it's fixed now.
And looking at target_submit_cmd_map_mem, it looks that
at least lun lookup error no longer triggers pr_error, right?
If yes it's good.

> -	rc = target_setup_cmd_from_cdb(se_cmd, tv_cmd->tvc_cdb);
> -	if (rc == -ENOMEM) {
> -		transport_send_check_condition_and_sense(se_cmd,
> -				TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
> -		transport_generic_free_cmd(se_cmd, 0);
> -		return;
> -	} else if (rc < 0) {
> -		if (se_cmd->se_cmd_flags & SCF_SCSI_RESERVATION_CONFLICT)
> -			tcm_vhost_queue_status(se_cmd);
> -		else
> -			transport_send_check_condition_and_sense(se_cmd,
> -					se_cmd->scsi_sense_reason, 0);
> -		transport_generic_free_cmd(se_cmd, 0);
> -		return;
> -	}
>  
>  	if (tv_cmd->tvc_sgl_count) {
>  		sg_ptr = tv_cmd->tvc_sgl;
> @@ -597,17 +555,19 @@ static void tcm_vhost_submission_work(struct work_struct *work)
>  	} else {
>  		sg_ptr = NULL;
>  	}
> -
> -	rc = transport_generic_map_mem_to_cmd(se_cmd, sg_ptr,
> -				tv_cmd->tvc_sgl_count, sg_bidi_ptr,
> -				sg_no_bidi);
> +	tv_nexus = tv_cmd->tvc_nexus;
> +
> +	rc = target_submit_cmd_map_mem(se_cmd, tv_nexus->tvn_se_sess,
> +			tv_cmd->tvc_cdb, &tv_cmd->tvc_sense_buf[0],
> +			tv_cmd->tvc_lun, tv_cmd->tvc_exp_data_len,
> +			tv_cmd->tvc_task_attr, tv_cmd->tvc_data_direction,
> +			0, sg_ptr, tv_cmd->tvc_sgl_count,
> +			sg_bidi_ptr, sg_no_bidi);
>  	if (rc < 0) {
>  		transport_send_check_condition_and_sense(se_cmd,
> -				se_cmd->scsi_sense_reason, 0);
> +				TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
>  		transport_generic_free_cmd(se_cmd, 0);
> -		return;
>  	}
> -	transport_handle_cdb_direct(se_cmd);
>  }
>  
>  static void vhost_scsi_handle_vq(struct vhost_scsi *vs)
> diff --git a/drivers/vhost/tcm_vhost.h b/drivers/vhost/tcm_vhost.h
> index d9e9355..7e87c63 100644
> --- a/drivers/vhost/tcm_vhost.h
> +++ b/drivers/vhost/tcm_vhost.h
> @@ -5,6 +5,12 @@
>  struct tcm_vhost_cmd {
>  	/* Descriptor from vhost_get_vq_desc() for virt_queue segment */
>  	int tvc_vq_desc;
> +	/* virtio-scsi initiator task attribute */
> +	int tvc_task_attr;
> +	/* virtio-scsi initiator data direction */
> +	enum dma_data_direction tvc_data_direction;
> +	/* Expected data transfer length from virtio-scsi header */
> +	u32 tvc_exp_data_len;
>  	/* The Tag from include/linux/virtio_scsi.h:struct virtio_scsi_cmd_req */
>  	u64 tvc_tag;
>  	/* The number of scatterlists associated with this cmd */
> @@ -17,6 +23,8 @@ struct tcm_vhost_cmd {
>  	struct virtio_scsi_cmd_resp __user *tvc_resp;
>  	/* Pointer to vhost_scsi for our device */
>  	struct vhost_scsi *tvc_vhost;
> +	/* Pointer to vhost nexus memory */
> +	struct tcm_vhost_nexus *tvc_nexus;
>  	/* The TCM I/O descriptor that is accessed via container_of() */
>  	struct se_cmd tvc_se_cmd;
>  	/* work item used for cmwq dispatch to tcm_vhost_submission_work() */
> -- 
> 1.7.2.5

Re: [PATCH 0/4] target: Add target_submit_cmd_map_mem and convert tcm_loop+tcm_vhost

[Michael S. Tsirkin]

On Tue, Oct 02, 2012 at 07:15:43AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> Hi hch & Co,
> 
> This series adds a new target_submit_cmd_map_mem() caller to accept
> pre-allocated SGL memory within the core generic I/O submission path.
> Patch #1 contains the core I/O changes, patch #2 + #4 includes the
> conversion of tcm_loop+tcm_vhost to use this new caller -> drop their
> internal open-coded equivalents using transport_generic_map_mem_to_cmd().
> 
> Patch #3 carries forward a work-around for tcm_loop w/ scsi-generic with
> user-space code that does not zero out it's READ payload buffer + ends
> up passing a payload filled with random data into target core's control CDB
> emulation.  Since we're not using bounce buffers any more for control
> CDB emulation in modern v3.x code, AFAICT tcm_loop still requires this
> extra bit to function properly with some legacy user-space code.
> 
> Regardless, the main I/O path changes end up being very mechnical in
> nature for existing core and fabric code, and has been running as expected
> with fio small-block workloads last hours.
> 
> Please review.
> 
> Thanks Christoph!
> 
> --nab

For the vhost part:

ACK series.

> Nicholas Bellinger (4):
>   target: Add target_submit_cmd_map_mem for SGL fabric memory
>     passthrough
>   tcm_loop: Convert I/O path to use target_submit_cmd_map_mem
>   target: Add TARGET_SCF_MAP_CLEAR_MEM work-around for tcm_loop
>   tcm_vhost: Convert I/O path to use target_submit_cmd_map_mem
> 
>  drivers/target/loopback/tcm_loop.c     |   62 ++-------------
>  drivers/target/target_core_transport.c |  129 ++++++++++++++++++++++++++------
>  drivers/vhost/tcm_vhost.c              |   68 ++++-------------
>  drivers/vhost/tcm_vhost.h              |    8 ++
>  include/target/target_core_base.h      |    2 +
>  include/target/target_core_fabric.h    |    3 +
>  6 files changed, 141 insertions(+), 131 deletions(-)
> 
> -- 
> 1.7.2.5

Re: [PATCH 1/4] target: Add target_submit_cmd_map_mem for SGL fabric memory passthrough

[Christoph Hellwig]

On Tue, Oct 02, 2012 at 07:15:44AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch adds a new TARGET_SCF_MAP_MEM flag within __target_submit_cmd()
> + a target_submit_cmd_map_mem() wrapper to pass pre-allocated SGL memory
> using existing transport_generic_map_mem_to_cmd() logic into the generic
> target submit I/O codepath.

Instead of requiring a flag we could simply check for the sglist_count
to be non-zero instead of requiring multiple pieces of information to
be in sync.

I'd also be tempted to remove the wrappers and just expose what is
__target_submit_cmd now to the drivers, but that can be done
independently as a cleanup.

Re: [PATCH 2/4] tcm_loop: Convert I/O path to use target_submit_cmd_map_mem

[Christoph Hellwig]

On Tue, Oct 02, 2012 at 07:15:45AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch converts tcm_loop to use target_submit_cmd_map_mem() for
> I/O submission and mapping of pre-allocated SGL memory from incoming
> scsi_cmnd -> se_cmd descriptors.
> 
> This includes removing the original open-coded fabric uses of target
> core callers to support transport_generic_map_mem_to_cmd() between
> target_setup_cmd_from_cdb() and transport_handle_cdb_direct() logic.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/4] target: Add TARGET_SCF_MAP_CLEAR_MEM work-around for tcm_loop

[Christoph Hellwig]

On Tue, Oct 02, 2012 at 07:15:46AM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch carries forward a work-around from tcm_loop to target
> core code to explicitly clear control CDB READ paylods in order to
> avoid bugs in scsi-generic user-space code for INQUIRY that do not
> explicitly zero CDB payload memory.

I never understood why we'd limit this to loop.  As far as I can see
we can get the same issue using sg-passthrough ontop of vhost, and for
the other drivers we'd simply rely on the drivers zeroing the pages.

I'd be much happier if we'd just unconditionally do this for the control
plane commands, as the performance over head for those should be
negligible.

A good place would be to do it as part of transport_kmap_data_sg, after
adding a data direction flag to it so that we'll only do it for the from
device direction.

Re: [PATCH 0/4] target: Add target_submit_cmd_map_mem and convert tcm_loop+tcm_vhost

[Christoph Hellwig]

This should also grow a patch 5 to unexport
transport_generic_map_mem_to_cmd and mark it static.

Re: [PATCH 1/4] target: Add target_submit_cmd_map_mem for SGL fabric memory passthrough

[Nicholas A. Bellinger]

On Tue, 2012-10-02 at 11:17 -0400, Christoph Hellwig wrote:
> On Tue, Oct 02, 2012 at 07:15:44AM +0000, Nicholas A. Bellinger wrote:
> > From: Nicholas Bellinger <nab@linux-iscsi.org>
> > 
> > This patch adds a new TARGET_SCF_MAP_MEM flag within __target_submit_cmd()
> > + a target_submit_cmd_map_mem() wrapper to pass pre-allocated SGL memory
> > using existing transport_generic_map_mem_to_cmd() logic into the generic
> > target submit I/O codepath.
> 
> Instead of requiring a flag we could simply check for the sglist_count
> to be non-zero instead of requiring multiple pieces of information to
> be in sync.
> 

Mmmm, fair point here.  In that case I'd rather rename
__target_submit_cmd() to target_submit_cmd_mapsgl(), and update the
target_submit_cmd() wrapper accordingly.

> I'd also be tempted to remove the wrappers and just expose what is
> __target_submit_cmd now to the drivers, but that can be done
> independently as a cleanup.
> 

<nod>, dropping the extra target_submit_cmd_map_mem bit + flag now.

Thanks hch!

--nab