All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 00/25 V5] target: fix cmd plugging and submission
@ 2021-02-17 20:27 Mike Christie
  2021-02-17 20:27 ` [PATCH 01/25] target: move t_task_cdb initialization Mike Christie
                   ` (24 more replies)
  0 siblings, 25 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:27 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel

The following patches were made over Martin's 5.12 branches
to handle conflicts with the in_interrupt changes.

The patches fix the following issues:

1. target_core_iblock plugs and unplugs the queue for every
command. To handle this issue and handle an issue that
vhost-scsi and loop were avoiding by adding their own workqueue,
I added a new submission workqueue to LIO. Drivers can pass cmds
to it, and we can then submit batches of cmds.

2. vhost-scsi and loop on the submission side were doing a work
per cmd but because we can block in the block layer on resources
like tags we can end up creating lots of threads that will fight
each other. In this patchset I just use a cmd list per device to
avoid abusing the workueue layer and to better batch the cmds
to the lower layers.

The combined patchset fixes a major perf issue we've been hitting
with vhost-scsi where IOPs were stuck at 230K when running:

    fio --filename=/dev/sda  --direct=1 --rw=randrw --bs=4k
    --ioengine=libaio --iodepth=128  --numjobs=8 --time_based
    --group_reporting --runtime=60

The patches in this set get me to 350K when using devices that
have native IOPs of around 400-500K. 

3. Fix target_submit* error handling. While handling Christoph's
comment to kill target_submit_cmd_map_sgls I hit several bugs that
are now also fixed up.

V5:
- Add WARN if driver has used simple API and target_stop_session
- Added fix for simple API users (usb, sbp, and xen) for early failure
(invalid LUN, write on a RO dev, etc) handling.

V4:
- Fixed the target_submit error handling.
- Dropped get_cdb callback.
- Fixed kernel robot errors for incorrect return values and unused
variables.
- Used flush instead of cancel to fix bug in tmr code.
- Fixed race in tcmu.
- Made completion affinity handling a configfs setting
- Dropped patch that added the per device work lists. It really helped
a lot for higher perf initiators and tcm loop but only gave around a 5%
boost to other drivers. So I dropped it for now to see if there is
something more generic we can do.

V3:
- Fix rc type in target_submit so its a sense_reason_t
- Add BUG_ON if caller uses target_queue_cmd_submit but hasn't
implemented get_cdb.
- Drop unused variables in loop.
- Fix race in tcmu plug check
- Add comment about how plug check works in iblock
- Do a flush when handling TMRs instead of cancel

V2:
- Fix up container_of use coding style
- Handle offlist review comment from Laurence where with the
original code and my patches we can hit a bug where the cmd
times out, LIO starts up the TMR code, but it misses the cmd
because it's on the workqueue.
- Made the work per device work instead of session to handle
the previous issue and so if one dev hits some issue it sleeps on,
it won't block other devices.





^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 01/25] target: move t_task_cdb initialization
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
@ 2021-02-17 20:27 ` Mike Christie
  2021-02-17 20:27 ` [PATCH 02/25] target: drop kref_get_unless_zero in target_get_sess_cmd Mike Christie
                   ` (23 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:27 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie, Himanshu Madhani

The next patch splits target_submit_cmd_map_sgls so the initialization
and submission part can be called at different times. If the init part
fails we can reference the t_task_cdb early in some of the logging
and tracing code. This moves it to transport_init_se_cmd so we don't
hit NULL pointer crashes.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Tested-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/target/target_core_transport.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index d47bfd8b0f87..5c4adde96d5e 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1389,6 +1389,7 @@ void transport_init_se_cmd(
 	INIT_WORK(&cmd->work, NULL);
 	kref_init(&cmd->cmd_kref);
 
+	cmd->t_task_cdb = &cmd->__t_task_cdb[0];
 	cmd->se_tfo = tfo;
 	cmd->se_sess = se_sess;
 	cmd->data_length = data_length;
@@ -1430,7 +1431,6 @@ target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
 {
 	sense_reason_t ret;
 
-	cmd->t_task_cdb = &cmd->__t_task_cdb[0];
 	/*
 	 * Ensure that the received CDB is less than the max (252 + 8) bytes
 	 * for VARIABLE_LENGTH_CMD
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 02/25] target: drop kref_get_unless_zero in target_get_sess_cmd
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
  2021-02-17 20:27 ` [PATCH 01/25] target: move t_task_cdb initialization Mike Christie
@ 2021-02-17 20:27 ` Mike Christie
  2021-02-17 20:27 ` [PATCH 03/25] target: rename transport_init_se_cmd Mike Christie
                   ` (22 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:27 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie, Himanshu Madhani

The kref_get_unless_zero use in target_get_sess_cmd was added
in:

commit 1b4c59b7a1d0 ("target: fix potential race window in
target_sess_cmd_list_waiting()")'

but it does not seem to do anything.

I think the original patch might have thought we could have added the
cmd to the sess_wait_list and then target_wait_for_sess_cmds could
do a put before target_get_sess_cmd did it's get. That wouldn't
happen because we do the get first then grab the sess lock and put
it on the list.

It's also not needed now, because the sess_cmd_list does not exist
anymore and we instead wait on the session cmd_count.

The other problem with the patch is that several
target_submit_cmd_map_sgls/ target_submit_cmd callers do not handle the
error case properly if it were to ever happen. These drivers think
they have their normal refcount on the cmd and in many cases do a
transport_generic_free_cmd plus target_put_sess_cmd so they would
have fired off the refcount WARN/BUGs.

So this patch just changes the kref_get_unless_zero to kref_get.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Tested-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/target/target_core_transport.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 5c4adde96d5e..b5427e26187b 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2775,9 +2775,7 @@ int target_get_sess_cmd(struct se_cmd *se_cmd, bool ack_kref)
 	 * invocations before se_cmd descriptor release.
 	 */
 	if (ack_kref) {
-		if (!kref_get_unless_zero(&se_cmd->cmd_kref))
-			return -EINVAL;
-
+		kref_get(&se_cmd->cmd_kref);
 		se_cmd->se_cmd_flags |= SCF_ACK_KREF;
 	}
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 03/25] target: rename transport_init_se_cmd
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
  2021-02-17 20:27 ` [PATCH 01/25] target: move t_task_cdb initialization Mike Christie
  2021-02-17 20:27 ` [PATCH 02/25] target: drop kref_get_unless_zero in target_get_sess_cmd Mike Christie
@ 2021-02-17 20:27 ` Mike Christie
  2021-02-17 20:27 ` [PATCH 04/25] target: break up target_submit_cmd_map_sgls Mike Christie
                   ` (21 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:27 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie, Himanshu Madhani

Rename transport_init_se_cmd to __target_init_cmd to reflect that
it's more of an internal function that drivers should normally not
use (usb seems to use it wrong and iscsi is that weird guy), and
because we are going to add a new init function in the next patches.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
---
 drivers/target/iscsi/iscsi_target.c    | 16 ++++++++--------
 drivers/target/target_core_transport.c | 16 ++++++++--------
 drivers/target/target_core_xcopy.c     |  8 ++++----
 drivers/usb/gadget/function/f_tcm.c    | 20 ++++++++++----------
 include/target/target_core_fabric.h    |  2 +-
 5 files changed, 31 insertions(+), 31 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 518fac4864cf..f2107705f2ea 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1154,10 +1154,10 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 	/*
 	 * Initialize struct se_cmd descriptor from target_core_mod infrastructure
 	 */
-	transport_init_se_cmd(&cmd->se_cmd, &iscsi_ops,
-			conn->sess->se_sess, be32_to_cpu(hdr->data_length),
-			cmd->data_direction, sam_task_attr,
-			cmd->sense_buffer + 2, scsilun_to_int(&hdr->lun));
+	__target_init_cmd(&cmd->se_cmd, &iscsi_ops,
+			 conn->sess->se_sess, be32_to_cpu(hdr->data_length),
+			 cmd->data_direction, sam_task_attr,
+			 cmd->sense_buffer + 2, scsilun_to_int(&hdr->lun));
 
 	pr_debug("Got SCSI Command, ITT: 0x%08x, CmdSN: 0x%08x,"
 		" ExpXferLen: %u, Length: %u, CID: %hu\n", hdr->itt,
@@ -2013,10 +2013,10 @@ iscsit_handle_task_mgt_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 					     buf);
 	}
 
-	transport_init_se_cmd(&cmd->se_cmd, &iscsi_ops,
-			      conn->sess->se_sess, 0, DMA_NONE,
-			      TCM_SIMPLE_TAG, cmd->sense_buffer + 2,
-			      scsilun_to_int(&hdr->lun));
+	__target_init_cmd(&cmd->se_cmd, &iscsi_ops,
+			  conn->sess->se_sess, 0, DMA_NONE,
+			  TCM_SIMPLE_TAG, cmd->sense_buffer + 2,
+			  scsilun_to_int(&hdr->lun));
 
 	target_get_sess_cmd(&cmd->se_cmd, true);
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index b5427e26187b..013f4a5e8972 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1297,7 +1297,7 @@ target_check_max_data_sg_nents(struct se_cmd *cmd, struct se_device *dev,
  * Compare the data buffer size from the CDB with the data buffer limit from the transport
  * header. Set @cmd->residual_count and SCF_OVERFLOW_BIT or SCF_UNDERFLOW_BIT if necessary.
  *
- * Note: target drivers set @cmd->data_length by calling transport_init_se_cmd().
+ * Note: target drivers set @cmd->data_length by calling __target_init_cmd().
  *
  * Return: TCM_NO_SENSE
  */
@@ -1369,7 +1369,7 @@ target_cmd_size_check(struct se_cmd *cmd, unsigned int size)
  *
  * Preserves the value of @cmd->tag.
  */
-void transport_init_se_cmd(
+void __target_init_cmd(
 	struct se_cmd *cmd,
 	const struct target_core_fabric_ops *tfo,
 	struct se_session *se_sess,
@@ -1403,7 +1403,7 @@ void transport_init_se_cmd(
 
 	cmd->state_active = false;
 }
-EXPORT_SYMBOL(transport_init_se_cmd);
+EXPORT_SYMBOL(__target_init_cmd);
 
 static sense_reason_t
 transport_check_alloc_task_attr(struct se_cmd *cmd)
@@ -1623,9 +1623,9 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 	 * exceptions are handled by sending exception status via
 	 * target_core_fabric_ops->queue_status() callback
 	 */
-	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
-				data_length, data_dir, task_attr, sense,
-				unpacked_lun);
+	__target_init_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
+			  data_length, data_dir, task_attr, sense,
+			  unpacked_lun);
 
 	if (flags & TARGET_SCF_UNKNOWN_SIZE)
 		se_cmd->unknown_data_length = 1;
@@ -1797,8 +1797,8 @@ int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
 	se_tpg = se_sess->se_tpg;
 	BUG_ON(!se_tpg);
 
-	transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
-			      0, DMA_NONE, TCM_SIMPLE_TAG, sense, unpacked_lun);
+	__target_init_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
+			  0, DMA_NONE, TCM_SIMPLE_TAG, sense, unpacked_lun);
 	/*
 	 * FIXME: Currently expect caller to handle se_cmd->se_tmr_req
 	 * allocation failure.
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index 66d6f1d06f21..e86cc6135587 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -615,8 +615,8 @@ static int target_xcopy_read_source(
 	pr_debug("XCOPY: Built READ_16: LBA: %llu Sectors: %u Length: %u\n",
 		(unsigned long long)src_lba, src_sectors, length);
 
-	transport_init_se_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, length,
-			      DMA_FROM_DEVICE, 0, &xpt_cmd.sense_buffer[0], 0);
+	__target_init_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, length,
+			  DMA_FROM_DEVICE, 0, &xpt_cmd.sense_buffer[0], 0);
 
 	rc = target_xcopy_setup_pt_cmd(&xpt_cmd, xop, src_dev, &cdb[0],
 				remote_port);
@@ -660,8 +660,8 @@ static int target_xcopy_write_destination(
 	pr_debug("XCOPY: Built WRITE_16: LBA: %llu Sectors: %u Length: %u\n",
 		(unsigned long long)dst_lba, dst_sectors, length);
 
-	transport_init_se_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, length,
-			      DMA_TO_DEVICE, 0, &xpt_cmd.sense_buffer[0], 0);
+	__target_init_cmd(se_cmd, &xcopy_pt_tfo, &xcopy_pt_sess, length,
+			  DMA_TO_DEVICE, 0, &xpt_cmd.sense_buffer[0], 0);
 
 	rc = target_xcopy_setup_pt_cmd(&xpt_cmd, xop, dst_dev, &cdb[0],
 				remote_port);
diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index 410fa89eae8f..dcce6e2605f5 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1050,11 +1050,11 @@ static void usbg_cmd_work(struct work_struct *work)
 	tv_nexus = tpg->tpg_nexus;
 	dir = get_cmd_dir(cmd->cmd_buf);
 	if (dir < 0) {
-		transport_init_se_cmd(se_cmd,
-				tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
-				tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
-				cmd->prio_attr, cmd->sense_iu.sense,
-				cmd->unpacked_lun);
+		__target_init_cmd(se_cmd,
+				  tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
+				  tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
+				  cmd->prio_attr, cmd->sense_iu.sense,
+				  cmd->unpacked_lun);
 		goto out;
 	}
 
@@ -1181,11 +1181,11 @@ static void bot_cmd_work(struct work_struct *work)
 	tv_nexus = tpg->tpg_nexus;
 	dir = get_cmd_dir(cmd->cmd_buf);
 	if (dir < 0) {
-		transport_init_se_cmd(se_cmd,
-				tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
-				tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
-				cmd->prio_attr, cmd->sense_iu.sense,
-				cmd->unpacked_lun);
+		__target_init_cmd(se_cmd,
+				  tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
+				  tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
+				  cmd->prio_attr, cmd->sense_iu.sense,
+				  cmd->unpacked_lun);
 		goto out;
 	}
 
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index d60a3eb7517a..4975c4d2a933 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -148,7 +148,7 @@ void	transport_deregister_session_configfs(struct se_session *);
 void	transport_deregister_session(struct se_session *);
 
 
-void	transport_init_se_cmd(struct se_cmd *,
+void	__target_init_cmd(struct se_cmd *,
 		const struct target_core_fabric_ops *,
 		struct se_session *, u32, int, int, unsigned char *, u64);
 sense_reason_t transport_lookup_cmd_lun(struct se_cmd *);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 04/25] target: break up target_submit_cmd_map_sgls
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (2 preceding siblings ...)
  2021-02-17 20:27 ` [PATCH 03/25] target: rename transport_init_se_cmd Mike Christie
@ 2021-02-17 20:27 ` Mike Christie
  2021-02-17 20:27 ` [PATCH 05/25] srpt: Convert to new submission API Mike Christie
                   ` (20 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:27 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie, Bart Van Assche, Juergen Gross, Hannes Reinecke,
	Nilesh Javali, Michael Cyr, Chris Boot, Felipe Balbi,
	Himanshu Madhani

This breaks up target_submit_cmd_map_sgls into 3 helpers:

- target_init_cmd: Do the basic general setup and get a refcount to
the session to make sure the caller can execute the cmd.

- target_submit_prep: Do the mapping, cdb processing and get a ref
to the lun.

- target_submit: Pass the cmd to LIO core for execution.

The above functions must be used by drivers that either:
1. rely on lio for session shutdown synchronization by calling
target_stop_session.
2. need to map sgls

When the next patches are applied then simple drivers that
do not need the extra functionality above can use
target_submit_cmd and not worry about failures being returned
and how to handle them, since many drivers were getting this
wrong and would have hit refcount bugs.

Also, by breaking target_submit_cmd_map_sgls up into these 3
helper functions, we can allow the later patches to do the init/prep
from interrupt context and then do the submission from a workqueue.

Cc: Bart Van Assche <bvanassche@acm.org>
Cc: Juergen Gross <jgross@suse.com>
Cc: Hannes Reinecke <hare@suse.de>
Cc: Nilesh Javali <njavali@marvell.com>
Cc: Michael Cyr <mikecyr@linux.ibm.com>
Cc: Chris Boot <bootc@bootc.net>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Tested-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/target/target_core_transport.c | 201 +++++++++++++++++--------
 include/target/target_core_fabric.h    |   8 +
 2 files changed, 148 insertions(+), 61 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 013f4a5e8972..8b2b805316dc 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1571,46 +1571,31 @@ 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.
- *
- * @se_cmd: command descriptor to submit
+ * target_init_cmd - initialize se_cmd
+ * @se_cmd: command descriptor to init
  * @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.
+ * Returns:
+ *	- less than zero to signal active I/O shutdown failure.
+ *	- zero on success.
  *
- * This may only be called from process context, and also currently
- * assumes internal allocation of fabric payload buffer by target-core.
+ * If the fabric driver calls target_stop_session, then it must check the
+ * return code and handle failures. This will never fail for other drivers,
+ * and the return code can be ignored.
  */
-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 target_init_cmd(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 se_portal_group *se_tpg;
-	sense_reason_t rc;
-	int ret;
-
-	might_sleep();
 
 	se_tpg = se_sess->se_tpg;
 	BUG_ON(!se_tpg);
@@ -1618,53 +1603,69 @@ int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess
 
 	if (flags & TARGET_SCF_USE_CPUID)
 		se_cmd->se_cmd_flags |= SCF_USE_CPUID;
+	/*
+	 * Signal bidirectional data payloads to target-core
+	 */
+	if (flags & TARGET_SCF_BIDI_OP)
+		se_cmd->se_cmd_flags |= SCF_BIDI;
+
+	if (flags & TARGET_SCF_UNKNOWN_SIZE)
+		se_cmd->unknown_data_length = 1;
 	/*
 	 * Initialize se_cmd for target operation.  From this point
 	 * exceptions are handled by sending exception status via
 	 * target_core_fabric_ops->queue_status() callback
 	 */
-	__target_init_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
-			  data_length, data_dir, task_attr, sense,
-			  unpacked_lun);
+	__target_init_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess, data_length,
+			  data_dir, task_attr, sense, unpacked_lun);
 
-	if (flags & TARGET_SCF_UNKNOWN_SIZE)
-		se_cmd->unknown_data_length = 1;
 	/*
 	 * Obtain struct se_cmd->cmd_kref reference. A second kref_get here is
 	 * necessary for fabrics using TARGET_SCF_ACK_KREF that expect a second
 	 * kref_put() to happen during fabric packet acknowledgement.
 	 */
-	ret = target_get_sess_cmd(se_cmd, flags & TARGET_SCF_ACK_KREF);
-	if (ret)
-		return ret;
-	/*
-	 * Signal bidirectional data payloads to target-core
-	 */
-	if (flags & TARGET_SCF_BIDI_OP)
-		se_cmd->se_cmd_flags |= SCF_BIDI;
+	return target_get_sess_cmd(se_cmd, flags & TARGET_SCF_ACK_KREF);
+}
+EXPORT_SYMBOL_GPL(target_init_cmd);
+
+/**
+ * target_submit_prep - prepare cmd for submission
+ * @se_cmd: command descriptor to prep
+ * @cdb: pointer to SCSI CDB
+ * @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
+ *
+ * Returns:
+ *	- less than zero to signal failure.
+ *	- zero on success.
+ * If failure is returned, lio will the callers queue_status to complete
+ * the cmd.
+ */
+int target_submit_prep(struct se_cmd *se_cmd, unsigned char *cdb,
+		       struct scatterlist *sgl, u32 sgl_count,
+		       struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
+		       struct scatterlist *sgl_prot, u32 sgl_prot_count)
+{
+	sense_reason_t rc;
 
 	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;
-	}
+	if (rc)
+		goto send_cc_direct;
 
 	/*
 	 * Locate se_lun pointer and attach it to struct se_cmd
 	 */
 	rc = transport_lookup_cmd_lun(se_cmd);
-	if (rc) {
-		transport_send_check_condition_and_sense(se_cmd, rc, 0);
-		target_put_sess_cmd(se_cmd);
-		return 0;
-	}
+	if (rc)
+		goto send_cc_direct;
 
 	rc = target_cmd_parse_cdb(se_cmd);
-	if (rc != 0) {
-		transport_generic_request_failure(se_cmd, rc);
-		return 0;
-	}
+	if (rc != 0)
+		goto generic_fail;
 
 	/*
 	 * Save pointers for SGLs containing protection information,
@@ -1684,6 +1685,41 @@ 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)
+			goto generic_fail;
+	}
+
+	return 0;
+
+send_cc_direct:
+	transport_send_check_condition_and_sense(se_cmd, rc, 0);
+	target_put_sess_cmd(se_cmd);
+	return -EIO;
+
+generic_fail:
+	transport_generic_request_failure(se_cmd, rc);
+	return -EIO;
+}
+EXPORT_SYMBOL_GPL(target_submit_prep);
+
+/**
+ * target_submit - perform final initialization and submit cmd to LIO core
+ * @se_cmd: command descriptor to submit
+ *
+ * target_submit_prep must have been called on the cmd, and this must be
+ * called from process context.
+ */
+void target_submit(struct se_cmd *se_cmd)
+{
+	struct scatterlist *sgl = se_cmd->t_data_sg;
+	unsigned char *buf = NULL;
+
+	might_sleep();
+
+	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 +1730,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 +1739,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 +1748,57 @@ 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);
+}
+EXPORT_SYMBOL_GPL(target_submit);
+
+/**
+ * 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 rc;
+
+	rc = target_init_cmd(se_cmd, se_sess, sense, unpacked_lun,
+			     data_length, task_attr, data_dir, flags);
+	if (rc < 0)
+		return rc;
+
+	if (target_submit_prep(se_cmd, cdb, sgl, sgl_count, sgl_bidi,
+			       sgl_bidi_count, sgl_prot, sgl_prot_count))
+		return 0;
+
+	target_submit(se_cmd);
 	return 0;
 }
 EXPORT_SYMBOL(target_submit_cmd_map_sgls);
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 4975c4d2a933..4b5f6687393a 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -151,6 +151,14 @@ void	transport_deregister_session(struct se_session *);
 void	__target_init_cmd(struct se_cmd *,
 		const struct target_core_fabric_ops *,
 		struct se_session *, u32, int, int, unsigned char *, u64);
+int	target_init_cmd(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);
+int	target_submit_prep(struct se_cmd *se_cmd, unsigned char *cdb,
+		struct scatterlist *sgl, u32 sgl_count,
+		struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
+		struct scatterlist *sgl_prot, u32 sgl_prot_count);
+void	target_submit(struct se_cmd *se_cmd);
 sense_reason_t transport_lookup_cmd_lun(struct se_cmd *);
 sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
 sense_reason_t target_cmd_parse_cdb(struct se_cmd *);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 05/25] srpt: Convert to new submission API
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (3 preceding siblings ...)
  2021-02-17 20:27 ` [PATCH 04/25] target: break up target_submit_cmd_map_sgls Mike Christie
@ 2021-02-17 20:27 ` Mike Christie
  2021-02-19  0:22   ` Bart Van Assche
  2021-02-17 20:27 ` [PATCH 06/25] ibmvscsi_tgt: " Mike Christie
                   ` (19 subsequent siblings)
  24 siblings, 1 reply; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:27 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie, Bart Van Assche

target_submit_cmd_map_sgls is being removed, so convert srpt to
the new submission API.

srpt uses target_stop_session to sync session shutdown with lio core,
so we use target_init_cmd/target_submit_prep/target_submit, because
target_init_cmd will detect the target_stop_session call and return
an error.

Cc: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 6be60aa5ffe2..87741e0b4bca 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1528,16 +1528,19 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
 		goto busy;
 	}
 
-	rc = target_submit_cmd_map_sgls(cmd, ch->sess, srp_cmd->cdb,
-			       &send_ioctx->sense_data[0],
-			       scsilun_to_int(&srp_cmd->lun), data_len,
-			       TCM_SIMPLE_TAG, dir, TARGET_SCF_ACK_KREF,
-			       sg, sg_cnt, NULL, 0, NULL, 0);
+	rc = target_init_cmd(cmd, ch->sess, &send_ioctx->sense_data[0],
+			     scsilun_to_int(&srp_cmd->lun), data_len,
+			     TCM_SIMPLE_TAG, dir, TARGET_SCF_ACK_KREF);
 	if (rc != 0) {
 		pr_debug("target_submit_cmd() returned %d for tag %#llx\n", rc,
 			 srp_cmd->tag);
 		goto busy;
 	}
+
+	if (target_submit_prep(cmd, srp_cmd->cdb, sg, sg_cnt, NULL, 0, NULL, 0))
+		return;
+
+	target_submit(cmd);
 	return;
 
 busy:
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 06/25] ibmvscsi_tgt: Convert to new submission API
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (4 preceding siblings ...)
  2021-02-17 20:27 ` [PATCH 05/25] srpt: Convert to new submission API Mike Christie
@ 2021-02-17 20:27 ` Mike Christie
  2021-02-17 20:27 ` [PATCH 07/25] qla2xxx: " Mike Christie
                   ` (18 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:27 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie, Michael Cyr

target_submit_cmd is now only for simple drivers that do their
own sync during shutdown and do not use target_stop_session. It
will never return a failure, so we can remove that code from
the driver.

Cc: Michael Cyr <mikecyr@linux.ibm.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
index cc3908c2d2f9..cfc54532402c 100644
--- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
+++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
@@ -2670,7 +2670,6 @@ static void ibmvscsis_parse_cmd(struct scsi_info *vscsi,
 	u64 data_len = 0;
 	enum dma_data_direction dir;
 	int attr = 0;
-	int rc = 0;
 
 	nexus = vscsi->tport.ibmv_nexus;
 	/*
@@ -2725,17 +2724,9 @@ static void ibmvscsis_parse_cmd(struct scsi_info *vscsi,
 
 	srp->lun.scsi_lun[0] &= 0x3f;
 
-	rc = target_submit_cmd(&cmd->se_cmd, nexus->se_sess, srp->cdb,
-			       cmd->sense_buf, scsilun_to_int(&srp->lun),
-			       data_len, attr, dir, 0);
-	if (rc) {
-		dev_err(&vscsi->dev, "target_submit_cmd failed, rc %d\n", rc);
-		spin_lock_bh(&vscsi->intr_lock);
-		list_del(&cmd->list);
-		ibmvscsis_free_cmd_resources(vscsi, cmd);
-		spin_unlock_bh(&vscsi->intr_lock);
-		goto fail;
-	}
+	target_submit_cmd(&cmd->se_cmd, nexus->se_sess, srp->cdb,
+			  cmd->sense_buf, scsilun_to_int(&srp->lun),
+			  data_len, attr, dir, 0);
 	return;
 
 fail:
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 07/25] qla2xxx: Convert to new submission API
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (5 preceding siblings ...)
  2021-02-17 20:27 ` [PATCH 06/25] ibmvscsi_tgt: " Mike Christie
@ 2021-02-17 20:27 ` Mike Christie
  2021-02-17 20:27 ` [PATCH 08/25] tcm_loop: " Mike Christie
                   ` (17 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:27 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie, Nilesh Javali, Himanshu Madhani

target_submit_cmd is now only for simple drivers that do their
own sync during shutdown and do not use target_stop_session.

tcm_qla2xxx uses target_stop_session to sync session shutdown with lio
core, so we use target_init_cmd/target_submit_prep/target_submit, because
target_init_cmd will detect the target_stop_session call and return
an error.

Cc: Nilesh Javali <njavali@marvell.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Tested-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/scsi/qla2xxx/tcm_qla2xxx.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index b55fc768a2a7..56394d901791 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -451,7 +451,7 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha, struct qla_tgt_cmd *cmd,
 	struct se_portal_group *se_tpg;
 	struct tcm_qla2xxx_tpg *tpg;
 #endif
-	int target_flags = TARGET_SCF_ACK_KREF;
+	int rc, target_flags = TARGET_SCF_ACK_KREF;
 	unsigned long flags;
 
 	if (bidi)
@@ -486,9 +486,17 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha, struct qla_tgt_cmd *cmd,
 	list_add_tail(&cmd->sess_cmd_list, &sess->sess_cmd_list);
 	spin_unlock_irqrestore(&sess->sess_cmd_lock, flags);
 
-	return target_submit_cmd(se_cmd, se_sess, cdb, &cmd->sense_buffer[0],
-				 cmd->unpacked_lun, data_length, fcp_task_attr,
-				 data_dir, target_flags);
+	rc = target_init_cmd(se_cmd, se_sess, &cmd->sense_buffer[0],
+			     cmd->unpacked_lun, data_length, fcp_task_attr,
+			     data_dir, target_flags);
+	if (rc)
+		return rc;
+
+	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0))
+		return 0;
+
+	target_submit(se_cmd);
+	return 0;
 }
 
 static void tcm_qla2xxx_handle_data_work(struct work_struct *work)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 08/25] tcm_loop: Convert to new submission API
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (6 preceding siblings ...)
  2021-02-17 20:27 ` [PATCH 07/25] qla2xxx: " Mike Christie
@ 2021-02-17 20:27 ` Mike Christie
  2021-02-17 20:27 ` [PATCH 09/25] sbp_target: " Mike Christie
                   ` (16 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:27 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

target_submit_cmd_map_sgls is being removed, so convert loop to
the new submission API.

Even though loop does its own shutdown sync, this has loop use
target_init_cmd/target_submit_prep/target_submit since it
needed to map sgls and in the next patches it will use the
API to use LIO's workqueue.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/target/loopback/tcm_loop.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index badba437e5f9..461f4125fcab 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -113,7 +113,6 @@ 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, transfer_length;
-	int rc;
 
 	tl_hba = *(struct tcm_loop_hba **)shost_priv(sc->device->host);
 	tl_tpg = &tl_hba->tl_hba_tpgs[sc->device->id];
@@ -151,17 +150,16 @@ static void tcm_loop_submission_work(struct work_struct *work)
 	}
 
 	se_cmd->tag = tl_cmd->sc_cmd_tag;
-	rc = target_submit_cmd_map_sgls(se_cmd, tl_nexus->se_sess, sc->cmnd,
-			&tl_cmd->tl_sense_buf[0], tl_cmd->sc->device->lun,
-			transfer_length, TCM_SIMPLE_TAG,
-			sc->sc_data_direction, 0,
-			scsi_sglist(sc), scsi_sg_count(sc),
-			sgl_bidi, sgl_bidi_count,
-			scsi_prot_sglist(sc), scsi_prot_sg_count(sc));
-	if (rc < 0) {
-		set_host_byte(sc, DID_NO_CONNECT);
-		goto out_done;
-	}
+	target_init_cmd(se_cmd, tl_nexus->se_sess, &tl_cmd->tl_sense_buf[0],
+			tl_cmd->sc->device->lun, transfer_length,
+			TCM_SIMPLE_TAG, sc->sc_data_direction, 0);
+
+	if (target_submit_prep(se_cmd, sc->cmnd, scsi_sglist(sc),
+			       scsi_sg_count(sc), sgl_bidi, sgl_bidi_count,
+			       scsi_prot_sglist(sc), scsi_prot_sg_count(sc)))
+		return;
+
+	target_submit(se_cmd);
 	return;
 
 out_done:
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 09/25] sbp_target: Convert to new submission API
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (7 preceding siblings ...)
  2021-02-17 20:27 ` [PATCH 08/25] tcm_loop: " Mike Christie
@ 2021-02-17 20:27 ` Mike Christie
  2021-02-17 20:27 ` [PATCH 10/25] usb gadget: " Mike Christie
                   ` (15 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:27 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie, Chris Boot

target_submit_cmd is now only for simple drivers that do their
own sync during shutdown and do not use target_stop_session. It
will never return a failure, so we can remove that code from
the driver.

Cc: Chris Boot <bootc@bootc.net>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/target/sbp/sbp_target.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index e4a9b9fe3dfb..f467c50aee08 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -1218,11 +1218,9 @@ static void sbp_handle_command(struct sbp_target_request *req)
 
 	/* only used for printk until we do TMRs */
 	req->se_cmd.tag = req->orb_pointer;
-	if (target_submit_cmd(&req->se_cmd, sess->se_sess, req->cmd_buf,
-			      req->sense_buf, unpacked_lun, data_length,
-			      TCM_SIMPLE_TAG, data_dir, TARGET_SCF_ACK_KREF))
-		goto err;
-
+	target_submit_cmd(&req->se_cmd, sess->se_sess, req->cmd_buf,
+			  req->sense_buf, unpacked_lun, data_length,
+			  TCM_SIMPLE_TAG, data_dir, TARGET_SCF_ACK_KREF);
 	return;
 
 err:
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 10/25] usb gadget: Convert to new submission API
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (8 preceding siblings ...)
  2021-02-17 20:27 ` [PATCH 09/25] sbp_target: " Mike Christie
@ 2021-02-17 20:27 ` Mike Christie
  2021-02-17 20:27 ` [PATCH 11/25] vhost-scsi: " Mike Christie
                   ` (14 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:27 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie, Felipe Balbi

target_submit_cmd is now only for simple drivers that do their
own sync during shutdown and do not use target_stop_session. It
will never return a failure, so we can remove that code from
the driver.

Note:
Before these patches target_submit_cmd would never return an
error for usb since it does not use target_stop_session. If
it did then we would have hit a refcount error here:

    transport_send_check_condition_and_sense(se_cmd,
                             TCM_UNSUPPORTED_SCSI_OPCODE, 1);
    transport_generic_free_cmd(&cmd->se_cmd, 0);

transport_send_check_condition_and_sense calls queue_status
and the driver can sometimes do transport_generic_free_cmd from
there via uasp_status_data_cmpl. In that case, the above
transport_generic_free_cmd would then hit a refcount error.

So that other use of the above error path in the driver is also
probably wrong, but someone with the hardware needs to fix that.

Cc: Felipe Balbi <balbi@kernel.org>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/usb/gadget/function/f_tcm.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/usb/gadget/function/f_tcm.c b/drivers/usb/gadget/function/f_tcm.c
index dcce6e2605f5..7acb507946e6 100644
--- a/drivers/usb/gadget/function/f_tcm.c
+++ b/drivers/usb/gadget/function/f_tcm.c
@@ -1058,11 +1058,9 @@ static void usbg_cmd_work(struct work_struct *work)
 		goto out;
 	}
 
-	if (target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess, cmd->cmd_buf,
-			      cmd->sense_iu.sense, cmd->unpacked_lun, 0,
-			      cmd->prio_attr, dir, flags) < 0)
-		goto out;
-
+	target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess, cmd->cmd_buf,
+			  cmd->sense_iu.sense, cmd->unpacked_lun, 0,
+			  cmd->prio_attr, dir, flags);
 	return;
 
 out:
@@ -1189,11 +1187,9 @@ static void bot_cmd_work(struct work_struct *work)
 		goto out;
 	}
 
-	if (target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
-			cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
-			cmd->data_len, cmd->prio_attr, dir, 0) < 0)
-		goto out;
-
+	target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
+			  cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
+			  cmd->data_len, cmd->prio_attr, dir, 0);
 	return;
 
 out:
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 11/25] vhost-scsi: Convert to new submission API
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (9 preceding siblings ...)
  2021-02-17 20:27 ` [PATCH 10/25] usb gadget: " Mike Christie
@ 2021-02-17 20:27 ` Mike Christie
  2021-02-17 20:27 ` [PATCH 12/25] xen-scsiback: " Mike Christie
                   ` (13 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:27 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

target_submit_cmd_map_sgls is being removed, so convert vhost-scsi
to the new submission API. This has it use
target_init_cmd/target_submit_prep/target_submit because we need to
have lio core map sgls which is now done in target_submit_prep,
and in the next patches we will do the target_submit step from
the lio workqueue.

Note: vhost-scsi never calls target_stop_session so
target_submit_cmd_map_sgls never failed (in the new API
target_init_cmd handles target_stop_session being called when cmds
are being submitted). If it were to have used target_stop_session
and got an error, we would have hit a refcount bug like xen and usb,
because it does:

if (rc < 0) {
	transport_send_check_condition_and_sense(se_cmd,
			TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
	transport_generic_free_cmd(se_cmd, 0);
}

transport_send_check_condition_and_sense calls queue_status which
does transport_generic_free_cmd, and then we do an extra
transport_generic_free_cmd call above which would have dropped
the refcount to -1 and the refcount code would spit out errors.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/vhost/scsi.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 4ce9f00ae10e..76508d408bb3 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -789,7 +789,6 @@ static void vhost_scsi_submission_work(struct work_struct *work)
 	struct vhost_scsi_nexus *tv_nexus;
 	struct se_cmd *se_cmd = &cmd->tvc_se_cmd;
 	struct scatterlist *sg_ptr, *sg_prot_ptr = NULL;
-	int rc;
 
 	/* FIXME: BIDI operation */
 	if (cmd->tvc_sgl_count) {
@@ -805,18 +804,17 @@ static void vhost_scsi_submission_work(struct work_struct *work)
 	tv_nexus = cmd->tvc_nexus;
 
 	se_cmd->tag = 0;
-	rc = target_submit_cmd_map_sgls(se_cmd, tv_nexus->tvn_se_sess,
-			cmd->tvc_cdb, &cmd->tvc_sense_buf[0],
+	target_init_cmd(se_cmd, tv_nexus->tvn_se_sess, &cmd->tvc_sense_buf[0],
 			cmd->tvc_lun, cmd->tvc_exp_data_len,
 			vhost_scsi_to_tcm_attr(cmd->tvc_task_attr),
-			cmd->tvc_data_direction, TARGET_SCF_ACK_KREF,
-			sg_ptr, cmd->tvc_sgl_count, NULL, 0, sg_prot_ptr,
-			cmd->tvc_prot_sgl_count);
-	if (rc < 0) {
-		transport_send_check_condition_and_sense(se_cmd,
-				TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
-		transport_generic_free_cmd(se_cmd, 0);
-	}
+			cmd->tvc_data_direction, TARGET_SCF_ACK_KREF);
+
+	if (target_submit_prep(se_cmd, cmd->tvc_cdb, sg_ptr,
+			       cmd->tvc_sgl_count, NULL, 0, sg_prot_ptr,
+			       cmd->tvc_prot_sgl_count))
+		return;
+
+	target_submit(se_cmd);
 }
 
 static void
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 12/25] xen-scsiback: Convert to new submission API
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (10 preceding siblings ...)
  2021-02-17 20:27 ` [PATCH 11/25] vhost-scsi: " Mike Christie
@ 2021-02-17 20:27 ` Mike Christie
  2021-02-17 20:27 ` [PATCH 13/25] tcm_fc: " Mike Christie
                   ` (12 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:27 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

target_submit_cmd_map_sgls is being removed, so convert xen
to the new submission API. This has it use
target_init_cmd/target_submit_prep/target_submit because we need to
have lio core map sgls which is now done in target_submit_prep.
target_init_cmd will never fail for xen because it does it's
own sync during session shutdown, so we can remove that code.

Note: xen never calls target_stop_session so
target_submit_cmd_map_sgls never failed (in the new API
target_init_cmd handles target_stop_session being called when cmds
are being submitted). If it were to have used target_stop_session
and got an error, we would have hit a refcount bug like xen and usb,
because it does:

    if (rc < 0) {
            transport_send_check_condition_and_sense(se_cmd,
                            TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
            transport_generic_free_cmd(se_cmd, 0);
    }

transport_send_check_condition_and_sense calls queue_status which
calls scsiback_cmd_done->target_put_sess_cmd. We do an extra
transport_generic_free_cmd call above which would have dropped
the refcount to -1 and the refcount code would spit out errors.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/xen/xen-scsiback.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 862162dca33c..7bf9a6bede6d 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -360,21 +360,18 @@ static void scsiback_cmd_exec(struct vscsibk_pend *pending_req)
 {
 	struct se_cmd *se_cmd = &pending_req->se_cmd;
 	struct se_session *sess = pending_req->v2p->tpg->tpg_nexus->tvn_se_sess;
-	int rc;
 
 	scsiback_get(pending_req->info);
 	se_cmd->tag = pending_req->rqid;
-	rc = target_submit_cmd_map_sgls(se_cmd, sess, pending_req->cmnd,
-			pending_req->sense_buffer, pending_req->v2p->lun,
-			pending_req->data_len, 0,
-			pending_req->sc_data_direction, TARGET_SCF_ACK_KREF,
-			pending_req->sgl, pending_req->n_sg,
-			NULL, 0, NULL, 0);
-	if (rc < 0) {
-		transport_send_check_condition_and_sense(se_cmd,
-				TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE, 0);
-		transport_generic_free_cmd(se_cmd, 0);
-	}
+	target_init_cmd(se_cmd, sess, pending_req->sense_buffer,
+			pending_req->v2p->lun, pending_req->data_len, 0,
+			pending_req->sc_data_direction, TARGET_SCF_ACK_KREF);
+
+	if (target_submit_prep(se_cmd, pending_req->cmnd, pending_req->sgl,
+			       pending_req->n_sg, NULL, 0, NULL, 0))
+		return;
+
+	target_submit(se_cmd);
 }
 
 static int scsiback_gnttab_data_map_batch(struct gnttab_map_grant_ref *map,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 13/25] tcm_fc: Convert to new submission API
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (11 preceding siblings ...)
  2021-02-17 20:27 ` [PATCH 12/25] xen-scsiback: " Mike Christie
@ 2021-02-17 20:27 ` Mike Christie
  2021-02-17 20:28 ` [PATCH 14/25] target: remove target_submit_cmd_map_sgls Mike Christie
                   ` (11 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:27 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

target_submit_cmd is now only for simple drivers that do their
own sync during shutdown and do not use target_stop_session,

tcm_fc uses target_stop_session to sync session shutdown with lio
core, so we use target_init_cmd/target_submit_prep/target_submit,
because target_init_cmd will now detect the target_stop_session call
and return an error.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/target/tcm_fc/tfc_cmd.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index 768f250680d9..1376501ee3d0 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -543,16 +543,22 @@ static void ft_send_work(struct work_struct *work)
 
 	fc_seq_set_resp(cmd->seq, ft_recv_seq, cmd);
 	cmd->se_cmd.tag = fc_seq_exch(cmd->seq)->rxid;
+
 	/*
 	 * Use a single se_cmd->cmd_kref as we expect to release se_cmd
 	 * directly from ft_check_stop_free callback in response path.
 	 */
-	if (target_submit_cmd(&cmd->se_cmd, cmd->sess->se_sess, fcp->fc_cdb,
-			      &cmd->ft_sense_buffer[0], scsilun_to_int(&fcp->fc_lun),
-			      ntohl(fcp->fc_dl), task_attr, data_dir,
-			      TARGET_SCF_ACK_KREF))
+	if (target_init_cmd(&cmd->se_cmd, cmd->sess->se_sess,
+			    &cmd->ft_sense_buffer[0],
+			    scsilun_to_int(&fcp->fc_lun), ntohl(fcp->fc_dl),
+			    task_attr, data_dir, TARGET_SCF_ACK_KREF))
 		goto err;
 
+	if (target_submit_prep(&cmd->se_cmd, fcp->fc_cdb, NULL, 0, NULL, 0,
+			       NULL, 0))
+		return;
+
+	target_submit(&cmd->se_cmd);
 	pr_debug("r_ctl %x target_submit_cmd %p\n", fh->fh_r_ctl, cmd);
 	return;
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 14/25] target: remove target_submit_cmd_map_sgls
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (12 preceding siblings ...)
  2021-02-17 20:27 ` [PATCH 13/25] tcm_fc: " Mike Christie
@ 2021-02-17 20:28 ` Mike Christie
  2021-02-19 19:01   ` Bodo Stroesser
  2021-02-17 20:28 ` [PATCH 15/25] target: add gfp_t arg to target_cmd_init_cdb Mike Christie
                   ` (10 subsequent siblings)
  24 siblings, 1 reply; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:28 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

Convert target_submit_cmd to do its own calls and then remove
target_submit_cmd_map_sgls since no one uses it.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Tested-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/target/target_core_transport.c | 76 ++++++--------------------
 include/target/target_core_fabric.h    |  6 +-
 2 files changed, 18 insertions(+), 64 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 8b2b805316dc..1f35cce6e92b 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1752,8 +1752,7 @@ void target_submit(struct se_cmd *se_cmd)
 EXPORT_SYMBOL_GPL(target_submit);
 
 /**
- * target_submit_cmd_map_sgls - lookup unpacked lun and submit uninitialized
- *					se_cmd + use pre-allocated SGL memory.
+ * target_submit_cmd - lookup unpacked lun and submit uninitialized se_cmd
  *
  * @se_cmd: command descriptor to submit
  * @se_sess: associated se_sess for endpoint
@@ -1764,76 +1763,35 @@ EXPORT_SYMBOL_GPL(target_submit);
  * @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.
+ *
+ * It also assumes interal target core SGL memory allocation.
+ *
+ * This function must only be used by drivers that do their own
+ * sync during shutdown and does not use target_stop_session. If there
+ * is a failure this function will call into the fabric driver's
+ * queue_status with a CHECK_CONDITION.
  */
-int target_submit_cmd_map_sgls(struct se_cmd *se_cmd, struct se_session *se_sess,
+void target_submit_cmd(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)
+		u32 data_length, int task_attr, int data_dir, int flags)
 {
 	int rc;
 
-	rc = target_init_cmd(se_cmd, se_sess, sense, unpacked_lun,
-			     data_length, task_attr, data_dir, flags);
-	if (rc < 0)
-		return rc;
+	rc = target_init_cmd(se_cmd, se_sess, sense, unpacked_lun, data_length,
+			     task_attr, data_dir, flags);
+	WARN(rc, "Invalid target_submit_cmd use. Driver must not use target_stop_session or call target_init_cmd directly.\n");
+	if (rc)
+		return;
 
-	if (target_submit_prep(se_cmd, cdb, sgl, sgl_count, sgl_bidi,
-			       sgl_bidi_count, sgl_prot, sgl_prot_count))
-		return 0;
+	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0))
+		return;
 
 	target_submit(se_cmd);
-	return 0;
-}
-EXPORT_SYMBOL(target_submit_cmd_map_sgls);
-
-/**
- * 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_attr: SAM task attribute
- * @data_dir: DMA data direction
- * @flags: flags for command submission from target_sc_flags_tables
- *
- * 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.
- *
- * It also assumes interal target core SGL memory allocation.
- */
-int target_submit_cmd(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)
-{
-	return target_submit_cmd_map_sgls(se_cmd, se_sess, cdb, sense,
-			unpacked_lun, data_length, task_attr, data_dir,
-			flags, NULL, 0, NULL, 0, NULL, 0);
 }
 EXPORT_SYMBOL(target_submit_cmd);
 
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 4b5f6687393a..86b0d4a7df92 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -162,11 +162,7 @@ void	target_submit(struct se_cmd *se_cmd);
 sense_reason_t transport_lookup_cmd_lun(struct se_cmd *);
 sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
 sense_reason_t target_cmd_parse_cdb(struct se_cmd *);
-int	target_submit_cmd_map_sgls(struct se_cmd *, struct se_session *,
-		unsigned char *, unsigned char *, u64, u32, int, int, int,
-		struct scatterlist *, u32, struct scatterlist *, u32,
-		struct scatterlist *, u32);
-int	target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
+void	target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
 		unsigned char *, u64, u32, int, int, int);
 int	target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
 		unsigned char *sense, u64 unpacked_lun,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 15/25] target: add gfp_t arg to target_cmd_init_cdb
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (13 preceding siblings ...)
  2021-02-17 20:28 ` [PATCH 14/25] target: remove target_submit_cmd_map_sgls Mike Christie
@ 2021-02-17 20:28 ` Mike Christie
  2021-02-19 18:53   ` Bodo Stroesser
  2021-02-19 19:32   ` Bodo Stroesser
  2021-02-17 20:28 ` [PATCH 16/25] target: add workqueue based cmd submission Mike Christie
                   ` (9 subsequent siblings)
  24 siblings, 2 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:28 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

tcm_loop could be used like a normal block device, so we can't use
GFP_KERNEL. This adds a gfp_t arg to target_cmd_init_cdb and covnerts
the users. For every driver but loop I kept GFP_KERNEL. For loop and
xcopy I switched to GFP_NOIO.

This will also be useful in the later patches where loop needs to
do target_submit_prep from interrupt context to get a ref to the
se_device, and so it will need to use GFP_ATOMIC.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c  |  3 ++-
 drivers/scsi/qla2xxx/tcm_qla2xxx.c     |  3 ++-
 drivers/target/iscsi/iscsi_target.c    |  3 ++-
 drivers/target/loopback/tcm_loop.c     |  3 ++-
 drivers/target/target_core_transport.c | 14 ++++++++------
 drivers/target/target_core_xcopy.c     |  2 +-
 drivers/target/tcm_fc/tfc_cmd.c        |  2 +-
 drivers/vhost/scsi.c                   |  2 +-
 drivers/xen/xen-scsiback.c             |  2 +-
 include/target/target_core_fabric.h    |  5 +++--
 10 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 87741e0b4bca..51c386a215f5 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1537,7 +1537,8 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
 		goto busy;
 	}
 
-	if (target_submit_prep(cmd, srp_cmd->cdb, sg, sg_cnt, NULL, 0, NULL, 0))
+	if (target_submit_prep(cmd, srp_cmd->cdb, sg, sg_cnt, NULL, 0, NULL, 0,
+			       GFP_KERNEL))
 		return;
 
 	target_submit(cmd);
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 56394d901791..12a2265eb2de 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -492,7 +492,8 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha, struct qla_tgt_cmd *cmd,
 	if (rc)
 		return rc;
 
-	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0))
+	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0,
+			       GFP_KERNEL))
 		return 0;
 
 	target_submit(se_cmd);
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index f2107705f2ea..566adfde1661 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1166,7 +1166,8 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 
 	target_get_sess_cmd(&cmd->se_cmd, true);
 
-	cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb);
+	cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb,
+						GFP_KERNEL);
 	if (cmd->sense_reason) {
 		if (cmd->sense_reason == TCM_OUT_OF_RESOURCES) {
 			return iscsit_add_reject_cmd(cmd,
diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 461f4125fcab..677e4b8f0642 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -156,7 +156,8 @@ static void tcm_loop_submission_work(struct work_struct *work)
 
 	if (target_submit_prep(se_cmd, sc->cmnd, scsi_sglist(sc),
 			       scsi_sg_count(sc), sgl_bidi, sgl_bidi_count,
-			       scsi_prot_sglist(sc), scsi_prot_sg_count(sc)))
+			       scsi_prot_sglist(sc), scsi_prot_sg_count(sc),
+			       GFP_NOIO))
 		return;
 
 	target_submit(se_cmd);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 1f35cce6e92b..6c88ca832da6 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1427,7 +1427,7 @@ transport_check_alloc_task_attr(struct se_cmd *cmd)
 }
 
 sense_reason_t
-target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
+target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb, gfp_t gfp)
 {
 	sense_reason_t ret;
 
@@ -1448,8 +1448,7 @@ target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
 	 * setup the pointer from __t_task_cdb to t_task_cdb.
 	 */
 	if (scsi_command_size(cdb) > sizeof(cmd->__t_task_cdb)) {
-		cmd->t_task_cdb = kzalloc(scsi_command_size(cdb),
-						GFP_KERNEL);
+		cmd->t_task_cdb = kzalloc(scsi_command_size(cdb), gfp);
 		if (!cmd->t_task_cdb) {
 			pr_err("Unable to allocate cmd->t_task_cdb"
 				" %u > sizeof(cmd->__t_task_cdb): %lu ops\n",
@@ -1638,6 +1637,7 @@ EXPORT_SYMBOL_GPL(target_init_cmd);
  * @sgl_bidi_count: scatterlist count for bidirectional READ mapping
  * @sgl_prot: struct scatterlist memory protection information
  * @sgl_prot_count: scatterlist count for protection information
+ * @gfp_t: gfp allocation type
  *
  * Returns:
  *	- less than zero to signal failure.
@@ -1648,11 +1648,12 @@ EXPORT_SYMBOL_GPL(target_init_cmd);
 int target_submit_prep(struct se_cmd *se_cmd, unsigned char *cdb,
 		       struct scatterlist *sgl, u32 sgl_count,
 		       struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
-		       struct scatterlist *sgl_prot, u32 sgl_prot_count)
+		       struct scatterlist *sgl_prot, u32 sgl_prot_count,
+		       gfp_t gfp)
 {
 	sense_reason_t rc;
 
-	rc = target_cmd_init_cdb(se_cmd, cdb);
+	rc = target_cmd_init_cdb(se_cmd, cdb, gfp);
 	if (rc)
 		goto send_cc_direct;
 
@@ -1788,7 +1789,8 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 	if (rc)
 		return;
 
-	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0))
+	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0,
+			       GFP_KERNEL))
 		return;
 
 	target_submit(se_cmd);
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index e86cc6135587..d31ed071cb08 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -554,7 +554,7 @@ static int target_xcopy_setup_pt_cmd(
 	}
 	cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
 
-	if (target_cmd_init_cdb(cmd, cdb))
+	if (target_cmd_init_cdb(cmd, cdb, GFP_KERNEL))
 		return -EINVAL;
 
 	cmd->tag = 0;
diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index 1376501ee3d0..410b723f9d79 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -555,7 +555,7 @@ static void ft_send_work(struct work_struct *work)
 		goto err;
 
 	if (target_submit_prep(&cmd->se_cmd, fcp->fc_cdb, NULL, 0, NULL, 0,
-			       NULL, 0))
+			       NULL, 0, GFP_KERNEL))
 		return;
 
 	target_submit(&cmd->se_cmd);
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 76508d408bb3..93f5631b469c 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -811,7 +811,7 @@ static void vhost_scsi_submission_work(struct work_struct *work)
 
 	if (target_submit_prep(se_cmd, cmd->tvc_cdb, sg_ptr,
 			       cmd->tvc_sgl_count, NULL, 0, sg_prot_ptr,
-			       cmd->tvc_prot_sgl_count))
+			       cmd->tvc_prot_sgl_count, GFP_KERNEL))
 		return;
 
 	target_submit(se_cmd);
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 7bf9a6bede6d..eb3d8e35cbcd 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -368,7 +368,7 @@ static void scsiback_cmd_exec(struct vscsibk_pend *pending_req)
 			pending_req->sc_data_direction, TARGET_SCF_ACK_KREF);
 
 	if (target_submit_prep(se_cmd, pending_req->cmnd, pending_req->sgl,
-			       pending_req->n_sg, NULL, 0, NULL, 0))
+			       pending_req->n_sg, NULL, 0, NULL, 0, GFP_KERNEL))
 		return;
 
 	target_submit(se_cmd);
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 86b0d4a7df92..0543ab107723 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -157,10 +157,11 @@ int	target_init_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 int	target_submit_prep(struct se_cmd *se_cmd, unsigned char *cdb,
 		struct scatterlist *sgl, u32 sgl_count,
 		struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
-		struct scatterlist *sgl_prot, u32 sgl_prot_count);
+		struct scatterlist *sgl_prot, u32 sgl_prot_count, gfp_t gfp);
 void	target_submit(struct se_cmd *se_cmd);
 sense_reason_t transport_lookup_cmd_lun(struct se_cmd *);
-sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
+sense_reason_t target_cmd_init_cdb(struct se_cmd *se_cmd, unsigned char *cdb,
+				   gfp_t gfp);
 sense_reason_t target_cmd_parse_cdb(struct se_cmd *);
 void	target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
 		unsigned char *, u64, u32, int, int, int);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 16/25] target: add workqueue based cmd submission
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (14 preceding siblings ...)
  2021-02-17 20:28 ` [PATCH 15/25] target: add gfp_t arg to target_cmd_init_cdb Mike Christie
@ 2021-02-17 20:28 ` Mike Christie
  2021-02-19 19:10   ` Bodo Stroesser
  2021-02-17 20:28 ` [PATCH 17/25] vhost scsi: use lio wq cmd submission helper Mike Christie
                   ` (8 subsequent siblings)
  24 siblings, 1 reply; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:28 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

loop and vhost-scsi do their target cmd submission from driver
workqueues. This allows them to avoid an issue where the backend may
block waiting for resources like tags/requests, mem/locks, etc
and that ends up blocking their entire submission path and for the
case of vhost-scsi both the submission and completion path.

This patch adds a helper drivers can use to submit from a lio
workqueue. This code will then be extended in the next patches to
fix the plugging of backend devices.

Note: I'm only converting vhost/loop initially, but the workqueue
based submission will work for other drivers and have similar
benefits where the main target loops will not end up blocking one
some backend resource. I'll port others when I have more time to
test as I think we might want to make it configurable for some
drivers.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Tested-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/target/target_core_device.c    | 10 ++++--
 drivers/target/target_core_internal.h  |  1 +
 drivers/target/target_core_transport.c | 42 +++++++++++++++++++++++++-
 include/target/target_core_base.h      |  8 ++++-
 include/target/target_core_fabric.h    |  2 ++
 5 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c
index 7787c527aad3..74d3a4896588 100644
--- a/drivers/target/target_core_device.c
+++ b/drivers/target/target_core_device.c
@@ -735,8 +735,14 @@ struct se_device *target_alloc_device(struct se_hba *hba, const char *name)
 
 	dev->queue_cnt = nr_cpu_ids;
 	for (i = 0; i < dev->queue_cnt; i++) {
-		INIT_LIST_HEAD(&dev->queues[i].state_list);
-		spin_lock_init(&dev->queues[i].lock);
+		struct se_device_queue *q;
+
+		q = &dev->queues[i];
+		INIT_LIST_HEAD(&q->state_list);
+		spin_lock_init(&q->lock);
+
+		init_llist_head(&q->sq.cmd_list);
+		INIT_WORK(&q->sq.work, target_queued_submit_work);
 	}
 
 	dev->se_hba = hba;
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index e7b3c6e5d574..56f841fd7f04 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -153,6 +153,7 @@ void	target_qf_do_work(struct work_struct *work);
 bool	target_check_wce(struct se_device *dev);
 bool	target_check_fua(struct se_device *dev);
 void	__target_execute_cmd(struct se_cmd *, bool);
+void	target_queued_submit_work(struct work_struct *work);
 
 /* target_core_stat.c */
 void	target_stat_setup_dev_default_groups(struct se_device *);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 6c88ca832da6..dd63f81bd702 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -41,6 +41,7 @@
 #include <trace/events/target.h>
 
 static struct workqueue_struct *target_completion_wq;
+static struct workqueue_struct *target_submission_wq;
 static struct kmem_cache *se_sess_cache;
 struct kmem_cache *se_ua_cache;
 struct kmem_cache *t10_pr_reg_cache;
@@ -129,8 +130,15 @@ int init_se_kmem_caches(void)
 	if (!target_completion_wq)
 		goto out_free_lba_map_mem_cache;
 
+	target_submission_wq = alloc_workqueue("target_submission",
+					       WQ_MEM_RECLAIM, 0);
+	if (!target_submission_wq)
+		goto out_free_completion_wq;
+
 	return 0;
 
+out_free_completion_wq:
+	destroy_workqueue(target_completion_wq);
 out_free_lba_map_mem_cache:
 	kmem_cache_destroy(t10_alua_lba_map_mem_cache);
 out_free_lba_map_cache:
@@ -153,6 +161,7 @@ int init_se_kmem_caches(void)
 
 void release_se_kmem_caches(void)
 {
+	destroy_workqueue(target_submission_wq);
 	destroy_workqueue(target_completion_wq);
 	kmem_cache_destroy(se_sess_cache);
 	kmem_cache_destroy(se_ua_cache);
@@ -1380,7 +1389,6 @@ void __target_init_cmd(
 {
 	INIT_LIST_HEAD(&cmd->se_delayed_node);
 	INIT_LIST_HEAD(&cmd->se_qf_node);
-	INIT_LIST_HEAD(&cmd->se_cmd_list);
 	INIT_LIST_HEAD(&cmd->state_list);
 	init_completion(&cmd->t_transport_stop_comp);
 	cmd->free_compl = NULL;
@@ -1797,6 +1805,38 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 }
 EXPORT_SYMBOL(target_submit_cmd);
 
+void target_queued_submit_work(struct work_struct *work)
+{
+	struct se_cmd_queue *sq = container_of(work, struct se_cmd_queue, work);
+	struct se_cmd *se_cmd, *next_cmd;
+	struct llist_node *cmd_list;
+
+	cmd_list = llist_del_all(&sq->cmd_list);
+	if (!cmd_list)
+		/* Previous call took what we were queued to submit */
+		return;
+
+	cmd_list = llist_reverse_order(cmd_list);
+	llist_for_each_entry_safe(se_cmd, next_cmd, cmd_list, se_cmd_list)
+		target_submit(se_cmd);
+}
+
+/**
+ * target_queue_submission - queue the cmd to run on the LIO workqueue
+ * @se_cmd: command descriptor to submit
+ */
+void target_queue_submission(struct se_cmd *se_cmd)
+{
+	struct se_device *se_dev = se_cmd->se_dev;
+	int cpu = se_cmd->cpuid;
+	struct se_cmd_queue *sq;
+
+	sq = &se_dev->queues[cpu].sq;
+	llist_add(&se_cmd->se_cmd_list, &sq->cmd_list);
+	queue_work_on(cpu, target_submission_wq, &sq->work);
+}
+EXPORT_SYMBOL_GPL(target_queue_submission);
+
 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 63dd12124139..815de4c97230 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -487,7 +487,7 @@ struct se_cmd {
 	/* Only used for internal passthrough and legacy TCM fabric modules */
 	struct se_session	*se_sess;
 	struct se_tmr_req	*se_tmr_req;
-	struct list_head	se_cmd_list;
+	struct llist_node	se_cmd_list;
 	struct completion	*free_compl;
 	struct completion	*abrt_compl;
 	const struct target_core_fabric_ops *se_tfo;
@@ -764,9 +764,15 @@ struct se_dev_stat_grps {
 	struct config_group scsi_lu_group;
 };
 
+struct se_cmd_queue {
+	struct llist_head	cmd_list;
+	struct work_struct	work;
+};
+
 struct se_device_queue {
 	struct list_head	state_list;
 	spinlock_t		lock;
+	struct se_cmd_queue	sq;
 };
 
 struct se_device {
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 0543ab107723..3c5ade7a04a6 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -165,6 +165,8 @@ sense_reason_t target_cmd_init_cdb(struct se_cmd *se_cmd, unsigned char *cdb,
 sense_reason_t target_cmd_parse_cdb(struct se_cmd *);
 void	target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
 		unsigned char *, u64, u32, int, int, int);
+void	target_queue_submission(struct se_cmd *se_cmd);
+
 int	target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
 		unsigned char *sense, u64 unpacked_lun,
 		void *fabric_tmr_ptr, unsigned char tm_type,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 17/25] vhost scsi: use lio wq cmd submission helper
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (15 preceding siblings ...)
  2021-02-17 20:28 ` [PATCH 16/25] target: add workqueue based cmd submission Mike Christie
@ 2021-02-17 20:28 ` Mike Christie
  2021-02-17 20:28 ` [PATCH 18/25] tcm loop: use blk cmd allocator for se_cmds Mike Christie
                   ` (7 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:28 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

Convert vhost-scsi to use the lio wq cmd submission helper.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/vhost/scsi.c | 36 ++++++------------------------------
 1 file changed, 6 insertions(+), 30 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 93f5631b469c..f3448e542965 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -85,7 +85,7 @@ struct vhost_scsi_cmd {
 	/* The number of scatterlists associated with this cmd */
 	u32 tvc_sgl_count;
 	u32 tvc_prot_sgl_count;
-	/* Saved unpacked SCSI LUN for vhost_scsi_submission_work() */
+	/* Saved unpacked SCSI LUN for vhost_scsi_target_queue_cmd() */
 	u32 tvc_lun;
 	/* Pointer to the SGL formatted memory from virtio-scsi */
 	struct scatterlist *tvc_sgl;
@@ -101,8 +101,6 @@ struct vhost_scsi_cmd {
 	struct vhost_scsi_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 vhost_scsi_submission_work() */
-	struct work_struct work;
 	/* Copy of the incoming SCSI command descriptor block (CDB) */
 	unsigned char tvc_cdb[VHOST_SCSI_MAX_CDB_SIZE];
 	/* Sense buffer that will be mapped into outgoing status */
@@ -240,8 +238,6 @@ struct vhost_scsi_ctx {
 	struct iov_iter out_iter;
 };
 
-static struct workqueue_struct *vhost_scsi_workqueue;
-
 /* Global spinlock to protect vhost_scsi TPG list for vhost IOCTL access */
 static DEFINE_MUTEX(vhost_scsi_mutex);
 static LIST_HEAD(vhost_scsi_list);
@@ -782,12 +778,10 @@ static int vhost_scsi_to_tcm_attr(int attr)
 	return TCM_SIMPLE_TAG;
 }
 
-static void vhost_scsi_submission_work(struct work_struct *work)
+static void vhost_scsi_target_queue_cmd(struct vhost_scsi_cmd *cmd)
 {
-	struct vhost_scsi_cmd *cmd =
-		container_of(work, struct vhost_scsi_cmd, work);
-	struct vhost_scsi_nexus *tv_nexus;
 	struct se_cmd *se_cmd = &cmd->tvc_se_cmd;
+	struct vhost_scsi_nexus *tv_nexus;
 	struct scatterlist *sg_ptr, *sg_prot_ptr = NULL;
 
 	/* FIXME: BIDI operation */
@@ -814,7 +808,7 @@ static void vhost_scsi_submission_work(struct work_struct *work)
 			       cmd->tvc_prot_sgl_count, GFP_KERNEL))
 		return;
 
-	target_submit(se_cmd);
+	target_queue_submission(se_cmd);
 }
 
 static void
@@ -1130,14 +1124,7 @@ vhost_scsi_handle_vq(struct vhost_scsi *vs, struct vhost_virtqueue *vq)
 		 * vhost_scsi_queue_data_in() and vhost_scsi_queue_status()
 		 */
 		cmd->tvc_vq_desc = vc.head;
-		/*
-		 * Dispatch cmd descriptor for cmwq execution in process
-		 * context provided by vhost_scsi_workqueue.  This also ensures
-		 * cmd is executed on the same kworker CPU as this vhost
-		 * thread to gain positive L2 cache locality effects.
-		 */
-		INIT_WORK(&cmd->work, vhost_scsi_submission_work);
-		queue_work(vhost_scsi_workqueue, &cmd->work);
+		vhost_scsi_target_queue_cmd(cmd);
 		ret = 0;
 err:
 		/*
@@ -2487,17 +2474,9 @@ static int __init vhost_scsi_init(void)
 		" on "UTS_RELEASE"\n", VHOST_SCSI_VERSION, utsname()->sysname,
 		utsname()->machine);
 
-	/*
-	 * Use our own dedicated workqueue for submitting I/O into
-	 * target core to avoid contention within system_wq.
-	 */
-	vhost_scsi_workqueue = alloc_workqueue("vhost_scsi", 0, 0);
-	if (!vhost_scsi_workqueue)
-		goto out;
-
 	ret = vhost_scsi_register();
 	if (ret < 0)
-		goto out_destroy_workqueue;
+		goto out;
 
 	ret = target_register_template(&vhost_scsi_ops);
 	if (ret < 0)
@@ -2507,8 +2486,6 @@ static int __init vhost_scsi_init(void)
 
 out_vhost_scsi_deregister:
 	vhost_scsi_deregister();
-out_destroy_workqueue:
-	destroy_workqueue(vhost_scsi_workqueue);
 out:
 	return ret;
 };
@@ -2517,7 +2494,6 @@ static void vhost_scsi_exit(void)
 {
 	target_unregister_template(&vhost_scsi_ops);
 	vhost_scsi_deregister();
-	destroy_workqueue(vhost_scsi_workqueue);
 };
 
 MODULE_DESCRIPTION("VHOST_SCSI series fabric driver");
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 18/25] tcm loop: use blk cmd allocator for se_cmds
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (16 preceding siblings ...)
  2021-02-17 20:28 ` [PATCH 17/25] vhost scsi: use lio wq cmd submission helper Mike Christie
@ 2021-02-17 20:28 ` Mike Christie
  2021-02-17 20:28 ` [PATCH 19/25] tcm loop: use lio wq cmd submission helper Mike Christie
                   ` (6 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:28 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

This just has tcm loop use the block layer cmd allocator for se_cmds
instead of using the tcm_loop_cmd_cache. In future patches when we
can use the host tags for internal requests like TMFs we can completely
kill the tcm_loop_cmd_cache.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/target/loopback/tcm_loop.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 677e4b8f0642..fb877aec6321 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -67,8 +67,12 @@ static void tcm_loop_release_cmd(struct se_cmd *se_cmd)
 {
 	struct tcm_loop_cmd *tl_cmd = container_of(se_cmd,
 				struct tcm_loop_cmd, tl_se_cmd);
+	struct scsi_cmnd *sc = tl_cmd->sc;
 
-	kmem_cache_free(tcm_loop_cmd_cache, tl_cmd);
+	if (se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
+		kmem_cache_free(tcm_loop_cmd_cache, tl_cmd);
+	else
+		sc->scsi_done(sc);
 }
 
 static int tcm_loop_show_info(struct seq_file *m, struct Scsi_Host *host)
@@ -164,7 +168,6 @@ static void tcm_loop_submission_work(struct work_struct *work)
 	return;
 
 out_done:
-	kmem_cache_free(tcm_loop_cmd_cache, tl_cmd);
 	sc->scsi_done(sc);
 }
 
@@ -174,20 +177,14 @@ static void tcm_loop_submission_work(struct work_struct *work)
  */
 static int tcm_loop_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
 {
-	struct tcm_loop_cmd *tl_cmd;
+	struct tcm_loop_cmd *tl_cmd = scsi_cmd_priv(sc);
 
 	pr_debug("%s() %d:%d:%d:%llu got CDB: 0x%02x scsi_buf_len: %u\n",
 		 __func__, sc->device->host->host_no, sc->device->id,
 		 sc->device->channel, sc->device->lun, sc->cmnd[0],
 		 scsi_bufflen(sc));
 
-	tl_cmd = kmem_cache_zalloc(tcm_loop_cmd_cache, GFP_ATOMIC);
-	if (!tl_cmd) {
-		set_host_byte(sc, DID_ERROR);
-		sc->scsi_done(sc);
-		return 0;
-	}
-
+	memset(tl_cmd, 0, sizeof(*tl_cmd));
 	tl_cmd->sc = sc;
 	tl_cmd->sc_cmd_tag = sc->request->tag;
 	INIT_WORK(&tl_cmd->work, tcm_loop_submission_work);
@@ -319,6 +316,7 @@ static struct scsi_host_template tcm_loop_driver_template = {
 	.dma_boundary		= PAGE_SIZE - 1,
 	.module			= THIS_MODULE,
 	.track_queue_depth	= 1,
+	.cmd_size		= sizeof(struct tcm_loop_cmd),
 };
 
 static int tcm_loop_driver_probe(struct device *dev)
@@ -579,7 +577,6 @@ static int tcm_loop_queue_data_or_status(const char *func,
 	if ((se_cmd->se_cmd_flags & SCF_OVERFLOW_BIT) ||
 	    (se_cmd->se_cmd_flags & SCF_UNDERFLOW_BIT))
 		scsi_set_resid(sc, se_cmd->residual_count);
-	sc->scsi_done(sc);
 	return 0;
 }
 
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 19/25] tcm loop: use lio wq cmd submission helper
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (17 preceding siblings ...)
  2021-02-17 20:28 ` [PATCH 18/25] tcm loop: use blk cmd allocator for se_cmds Mike Christie
@ 2021-02-17 20:28 ` Mike Christie
  2021-02-19 19:38   ` Bodo Stroesser
  2021-02-17 20:28 ` [PATCH 20/25] target: cleanup cmd flag bits Mike Christie
                   ` (5 subsequent siblings)
  24 siblings, 1 reply; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:28 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

Convert loop to use the lio wq cmd submission helper.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Tested-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/target/loopback/tcm_loop.c | 22 ++++++----------------
 drivers/target/loopback/tcm_loop.h |  1 -
 2 files changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index fb877aec6321..2687fd7d45db 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -39,7 +39,6 @@
 
 #define to_tcm_loop_hba(hba)	container_of(hba, struct tcm_loop_hba, dev)
 
-static struct workqueue_struct *tcm_loop_workqueue;
 static struct kmem_cache *tcm_loop_cmd_cache;
 
 static int tcm_loop_hba_no_cnt;
@@ -106,10 +105,8 @@ static struct device_driver tcm_loop_driverfs = {
  */
 static struct device *tcm_loop_primary;
 
-static void tcm_loop_submission_work(struct work_struct *work)
+static void tcm_loop_target_queue_cmd(struct tcm_loop_cmd *tl_cmd)
 {
-	struct tcm_loop_cmd *tl_cmd =
-		container_of(work, struct tcm_loop_cmd, work);
 	struct se_cmd *se_cmd = &tl_cmd->tl_se_cmd;
 	struct scsi_cmnd *sc = tl_cmd->sc;
 	struct tcm_loop_nexus *tl_nexus;
@@ -161,10 +158,10 @@ static void tcm_loop_submission_work(struct work_struct *work)
 	if (target_submit_prep(se_cmd, sc->cmnd, scsi_sglist(sc),
 			       scsi_sg_count(sc), sgl_bidi, sgl_bidi_count,
 			       scsi_prot_sglist(sc), scsi_prot_sg_count(sc),
-			       GFP_NOIO))
+			       GFP_ATOMIC))
 		return;
 
-	target_submit(se_cmd);
+	target_queue_submission(se_cmd);
 	return;
 
 out_done:
@@ -187,8 +184,8 @@ static int tcm_loop_queuecommand(struct Scsi_Host *sh, struct scsi_cmnd *sc)
 	memset(tl_cmd, 0, sizeof(*tl_cmd));
 	tl_cmd->sc = sc;
 	tl_cmd->sc_cmd_tag = sc->request->tag;
-	INIT_WORK(&tl_cmd->work, tcm_loop_submission_work);
-	queue_work(tcm_loop_workqueue, &tl_cmd->work);
+
+	tcm_loop_target_queue_cmd(tl_cmd);
 	return 0;
 }
 
@@ -1160,17 +1157,13 @@ static int __init tcm_loop_fabric_init(void)
 {
 	int ret = -ENOMEM;
 
-	tcm_loop_workqueue = alloc_workqueue("tcm_loop", 0, 0);
-	if (!tcm_loop_workqueue)
-		goto out;
-
 	tcm_loop_cmd_cache = kmem_cache_create("tcm_loop_cmd_cache",
 				sizeof(struct tcm_loop_cmd),
 				__alignof__(struct tcm_loop_cmd),
 				0, NULL);
 	if (!tcm_loop_cmd_cache) {
 		pr_debug("kmem_cache_create() for tcm_loop_cmd_cache failed\n");
-		goto out_destroy_workqueue;
+		goto out;
 	}
 
 	ret = tcm_loop_alloc_core_bus();
@@ -1187,8 +1180,6 @@ static int __init tcm_loop_fabric_init(void)
 	tcm_loop_release_core_bus();
 out_destroy_cache:
 	kmem_cache_destroy(tcm_loop_cmd_cache);
-out_destroy_workqueue:
-	destroy_workqueue(tcm_loop_workqueue);
 out:
 	return ret;
 }
@@ -1198,7 +1189,6 @@ static void __exit tcm_loop_fabric_exit(void)
 	target_unregister_template(&loop_ops);
 	tcm_loop_release_core_bus();
 	kmem_cache_destroy(tcm_loop_cmd_cache);
-	destroy_workqueue(tcm_loop_workqueue);
 }
 
 MODULE_DESCRIPTION("TCM loopback virtual Linux/SCSI fabric module");
diff --git a/drivers/target/loopback/tcm_loop.h b/drivers/target/loopback/tcm_loop.h
index d3110909a213..437663b3905c 100644
--- a/drivers/target/loopback/tcm_loop.h
+++ b/drivers/target/loopback/tcm_loop.h
@@ -16,7 +16,6 @@ struct tcm_loop_cmd {
 	struct scsi_cmnd *sc;
 	/* The TCM I/O descriptor that is accessed via container_of() */
 	struct se_cmd tl_se_cmd;
-	struct work_struct work;
 	struct completion tmr_done;
 	/* Sense buffer that will be mapped into outgoing status */
 	unsigned char tl_sense_buf[TRANSPORT_SENSE_BUFFER];
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 20/25] target: cleanup cmd flag bits
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (18 preceding siblings ...)
  2021-02-17 20:28 ` [PATCH 19/25] tcm loop: use lio wq cmd submission helper Mike Christie
@ 2021-02-17 20:28 ` Mike Christie
  2021-02-17 20:28 ` [PATCH 21/25] target: fix backend plugging Mike Christie
                   ` (4 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:28 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie, Chaitanya Kulkarni, Himanshu Madhani

We have a couple holes in the cmd flags definitions. This cleans
up the definitions to fix that and make it easier to read.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
---
 include/target/target_core_base.h | 38 +++++++++++++++----------------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 815de4c97230..5e6703ca102d 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -127,25 +127,25 @@ enum transport_state_table {
 
 /* Used for struct se_cmd->se_cmd_flags */
 enum se_cmd_flags_table {
-	SCF_SUPPORTED_SAM_OPCODE	= 0x00000001,
-	SCF_TRANSPORT_TASK_SENSE	= 0x00000002,
-	SCF_EMULATED_TASK_SENSE		= 0x00000004,
-	SCF_SCSI_DATA_CDB		= 0x00000008,
-	SCF_SCSI_TMR_CDB		= 0x00000010,
-	SCF_FUA				= 0x00000080,
-	SCF_SE_LUN_CMD			= 0x00000100,
-	SCF_BIDI			= 0x00000400,
-	SCF_SENT_CHECK_CONDITION	= 0x00000800,
-	SCF_OVERFLOW_BIT		= 0x00001000,
-	SCF_UNDERFLOW_BIT		= 0x00002000,
-	SCF_ALUA_NON_OPTIMIZED		= 0x00008000,
-	SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC = 0x00020000,
-	SCF_COMPARE_AND_WRITE		= 0x00080000,
-	SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC = 0x00200000,
-	SCF_ACK_KREF			= 0x00400000,
-	SCF_USE_CPUID			= 0x00800000,
-	SCF_TASK_ATTR_SET		= 0x01000000,
-	SCF_TREAT_READ_AS_NORMAL	= 0x02000000,
+	SCF_SUPPORTED_SAM_OPCODE		= (1 << 0),
+	SCF_TRANSPORT_TASK_SENSE		= (1 << 1),
+	SCF_EMULATED_TASK_SENSE			= (1 << 2),
+	SCF_SCSI_DATA_CDB			= (1 << 3),
+	SCF_SCSI_TMR_CDB			= (1 << 4),
+	SCF_FUA					= (1 << 5),
+	SCF_SE_LUN_CMD				= (1 << 6),
+	SCF_BIDI				= (1 << 7),
+	SCF_SENT_CHECK_CONDITION		= (1 << 8),
+	SCF_OVERFLOW_BIT			= (1 << 9),
+	SCF_UNDERFLOW_BIT			= (1 << 10),
+	SCF_ALUA_NON_OPTIMIZED			= (1 << 11),
+	SCF_PASSTHROUGH_SG_TO_MEM_NOALLOC	= (1 << 12),
+	SCF_COMPARE_AND_WRITE			= (1 << 13),
+	SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC	= (1 << 14),
+	SCF_ACK_KREF				= (1 << 15),
+	SCF_USE_CPUID				= (1 << 16),
+	SCF_TASK_ATTR_SET			= (1 << 17),
+	SCF_TREAT_READ_AS_NORMAL		= (1 << 18),
 };
 
 /*
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 21/25] target: fix backend plugging
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (19 preceding siblings ...)
  2021-02-17 20:28 ` [PATCH 20/25] target: cleanup cmd flag bits Mike Christie
@ 2021-02-17 20:28 ` Mike Christie
  2021-02-19 19:40   ` Bodo Stroesser
  2021-02-17 20:28 ` [PATCH 22/25] target iblock: add backend plug/unplug callouts Mike Christie
                   ` (3 subsequent siblings)
  24 siblings, 1 reply; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:28 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

target_core_iblock is plugging and unplugging on every command and this
is causing perf issues for drivers that prefer batched cmds. With the
last patches we can now take multiple cmds from a fabric driver queue
and then pass them down the backend drivers in a batch. This patch adds
this support by adding 2 callouts to the backend for plugging and
unplugging the device. The next 2 patches add support for iblock and
tcmu device plugging.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_transport.c | 43 +++++++++++++++++++++++++-
 include/target/target_core_backend.h   |  2 ++
 include/target/target_core_base.h      |  4 +++
 3 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index dd63f81bd702..18cb00a1ee2f 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1805,10 +1805,42 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 }
 EXPORT_SYMBOL(target_submit_cmd);
 
+
+static struct se_dev_plug *target_plug_device(struct se_device *se_dev)
+{
+	struct se_dev_plug *se_plug;
+
+	if (!se_dev->transport->plug_device)
+		return NULL;
+
+	se_plug = se_dev->transport->plug_device(se_dev);
+	if (!se_plug)
+		return NULL;
+
+	se_plug->se_dev = se_dev;
+	/*
+	 * We have a ref to the lun at this point, but the cmds could
+	 * complete before we unplug, so grab a ref to the se_device so we
+	 * can call back into the backend.
+	 */
+	config_group_get(&se_dev->dev_group);
+	return se_plug;
+}
+
+static void target_unplug_device(struct se_dev_plug *se_plug)
+{
+	struct se_device *se_dev = se_plug->se_dev;
+
+	se_dev->transport->unplug_device(se_plug);
+	config_group_put(&se_dev->dev_group);
+}
+
 void target_queued_submit_work(struct work_struct *work)
 {
 	struct se_cmd_queue *sq = container_of(work, struct se_cmd_queue, work);
 	struct se_cmd *se_cmd, *next_cmd;
+	struct se_dev_plug *se_plug = NULL;
+	struct se_device *se_dev = NULL;
 	struct llist_node *cmd_list;
 
 	cmd_list = llist_del_all(&sq->cmd_list);
@@ -1817,8 +1849,17 @@ void target_queued_submit_work(struct work_struct *work)
 		return;
 
 	cmd_list = llist_reverse_order(cmd_list);
-	llist_for_each_entry_safe(se_cmd, next_cmd, cmd_list, se_cmd_list)
+	llist_for_each_entry_safe(se_cmd, next_cmd, cmd_list, se_cmd_list) {
+		if (!se_dev) {
+			se_dev = se_cmd->se_dev;
+			se_plug = target_plug_device(se_dev);
+		}
+
 		target_submit(se_cmd);
+	}
+
+	if (se_plug)
+		target_unplug_device(se_plug);
 }
 
 /**
diff --git a/include/target/target_core_backend.h b/include/target/target_core_backend.h
index 6336780d83a7..aa5f83e55501 100644
--- a/include/target/target_core_backend.h
+++ b/include/target/target_core_backend.h
@@ -34,6 +34,8 @@ struct target_backend_ops {
 	int (*configure_device)(struct se_device *);
 	void (*destroy_device)(struct se_device *);
 	void (*free_device)(struct se_device *device);
+	struct se_dev_plug *(*plug_device)(struct se_device *se_dev);
+	void (*unplug_device)(struct se_dev_plug *se_plug);
 
 	ssize_t (*set_configfs_dev_params)(struct se_device *,
 					   const char *, ssize_t);
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 5e6703ca102d..b8e0a3250bd0 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -769,6 +769,10 @@ struct se_cmd_queue {
 	struct work_struct	work;
 };
 
+struct se_dev_plug {
+	struct se_device	*se_dev;
+};
+
 struct se_device_queue {
 	struct list_head	state_list;
 	spinlock_t		lock;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 22/25] target iblock: add backend plug/unplug callouts
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (20 preceding siblings ...)
  2021-02-17 20:28 ` [PATCH 21/25] target: fix backend plugging Mike Christie
@ 2021-02-17 20:28 ` Mike Christie
  2021-02-17 20:28 ` [PATCH 23/25] target_core_user: " Mike Christie
                   ` (2 subsequent siblings)
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:28 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

This patch adds plug/unplug callouts for iblock. For initiator drivers
like iscsi which wants to pass multiple cmds to its xmit thread instead
of one cmd at a time, this increases IOPs by around 10% with vhost-scsi
(combined with the last patches we can see a total 40-50% increase). For
driver combos like tcm_loop and faster drivers like the iser initiator, we
can still see IOPs increase by 20-30% when tcm_loop's nr_hw_queues setting
is also increased.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_iblock.c | 44 ++++++++++++++++++++++++++++-
 drivers/target/target_core_iblock.h | 10 +++++++
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/target/target_core_iblock.c b/drivers/target/target_core_iblock.c
index 8ed93fd205c7..33c88eca090f 100644
--- a/drivers/target/target_core_iblock.c
+++ b/drivers/target/target_core_iblock.c
@@ -61,9 +61,18 @@ static struct se_device *iblock_alloc_device(struct se_hba *hba, const char *nam
 		return NULL;
 	}
 
+	ib_dev->ibd_plug = kcalloc(nr_cpu_ids, sizeof(*ib_dev->ibd_plug),
+				   GFP_KERNEL);
+	if (!ib_dev->ibd_plug)
+		goto free_dev;
+
 	pr_debug( "IBLOCK: Allocated ib_dev for %s\n", name);
 
 	return &ib_dev->dev;
+
+free_dev:
+	kfree(ib_dev);
+	return NULL;
 }
 
 static int iblock_configure_device(struct se_device *dev)
@@ -171,6 +180,7 @@ static void iblock_dev_call_rcu(struct rcu_head *p)
 	struct se_device *dev = container_of(p, struct se_device, rcu_head);
 	struct iblock_dev *ib_dev = IBLOCK_DEV(dev);
 
+	kfree(ib_dev->ibd_plug);
 	kfree(ib_dev);
 }
 
@@ -188,6 +198,33 @@ static void iblock_destroy_device(struct se_device *dev)
 	bioset_exit(&ib_dev->ibd_bio_set);
 }
 
+static struct se_dev_plug *iblock_plug_device(struct se_device *se_dev)
+{
+	struct iblock_dev *ib_dev = IBLOCK_DEV(se_dev);
+	struct iblock_dev_plug *ib_dev_plug;
+
+	/*
+	 * Each se_device has a per cpu work this can be run from. Wwe
+	 * shouldn't have multiple threads on the same cpu calling this
+	 * at the same time.
+	 */
+	ib_dev_plug = &ib_dev->ibd_plug[smp_processor_id()];
+	if (test_and_set_bit(IBD_PLUGF_PLUGGED, &ib_dev_plug->flags))
+		return NULL;
+
+	blk_start_plug(&ib_dev_plug->blk_plug);
+	return &ib_dev_plug->se_plug;
+}
+
+static void iblock_unplug_device(struct se_dev_plug *se_plug)
+{
+	struct iblock_dev_plug *ib_dev_plug = container_of(se_plug,
+					struct iblock_dev_plug, se_plug);
+
+	blk_finish_plug(&ib_dev_plug->blk_plug);
+	clear_bit(IBD_PLUGF_PLUGGED, &ib_dev_plug->flags);
+}
+
 static unsigned long long iblock_emulate_read_cap_with_block_size(
 	struct se_device *dev,
 	struct block_device *bd,
@@ -337,7 +374,10 @@ static void iblock_submit_bios(struct bio_list *list)
 {
 	struct blk_plug plug;
 	struct bio *bio;
-
+	/*
+	 * The block layer handles nested plugs, so just plug/unplug to handle
+	 * fabric drivers that didn't support batching and multi bio cmds.
+	 */
 	blk_start_plug(&plug);
 	while ((bio = bio_list_pop(list)))
 		submit_bio(bio);
@@ -870,6 +910,8 @@ static const struct target_backend_ops iblock_ops = {
 	.configure_device	= iblock_configure_device,
 	.destroy_device		= iblock_destroy_device,
 	.free_device		= iblock_free_device,
+	.plug_device		= iblock_plug_device,
+	.unplug_device		= iblock_unplug_device,
 	.parse_cdb		= iblock_parse_cdb,
 	.set_configfs_dev_params = iblock_set_configfs_dev_params,
 	.show_configfs_dev_params = iblock_show_configfs_dev_params,
diff --git a/drivers/target/target_core_iblock.h b/drivers/target/target_core_iblock.h
index cefc641145b3..8c55375d2f75 100644
--- a/drivers/target/target_core_iblock.h
+++ b/drivers/target/target_core_iblock.h
@@ -4,6 +4,7 @@
 
 #include <linux/atomic.h>
 #include <linux/refcount.h>
+#include <linux/blkdev.h>
 #include <target/target_core_base.h>
 
 #define IBLOCK_VERSION		"4.0"
@@ -17,6 +18,14 @@ struct iblock_req {
 
 #define IBDF_HAS_UDEV_PATH		0x01
 
+#define IBD_PLUGF_PLUGGED		0x01
+
+struct iblock_dev_plug {
+	struct se_dev_plug se_plug;
+	struct blk_plug blk_plug;
+	unsigned long flags;
+};
+
 struct iblock_dev {
 	struct se_device dev;
 	unsigned char ibd_udev_path[SE_UDEV_PATH_LEN];
@@ -24,6 +33,7 @@ struct iblock_dev {
 	struct bio_set	ibd_bio_set;
 	struct block_device *ibd_bd;
 	bool ibd_readonly;
+	struct iblock_dev_plug *ibd_plug;
 } ____cacheline_aligned;
 
 #endif /* TARGET_CORE_IBLOCK_H */
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 23/25] target_core_user: add backend plug/unplug callouts
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (21 preceding siblings ...)
  2021-02-17 20:28 ` [PATCH 22/25] target iblock: add backend plug/unplug callouts Mike Christie
@ 2021-02-17 20:28 ` Mike Christie
  2021-02-19 19:45   ` Bodo Stroesser
  2021-02-17 20:28 ` [PATCH 24/25] target: flush submission work during TMR processing Mike Christie
  2021-02-17 20:28 ` [PATCH 25/25] target: make completion affinity configurable Mike Christie
  24 siblings, 1 reply; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:28 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

This patch adds plug/unplug callouts for tcmu, so we can avoid the
number of times we switch to userspace. Using this driver with tcm
loop is a common config, and dependng on the nr_hw_queues
(nr_hw_queues=1 performs much better) and fio jobs (lower num jobs
around 4) this patch can increase IOPs by only around 5-10% because
we hit other issues like the big per tcmu device mutex.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/target/target_core_user.c | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_user.c b/drivers/target/target_core_user.c
index a5991df23581..a433bda56b89 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -111,6 +111,7 @@ struct tcmu_dev {
 	struct kref kref;
 
 	struct se_device se_dev;
+	struct se_dev_plug se_plug;
 
 	char *name;
 	struct se_hba *hba;
@@ -119,6 +120,7 @@ struct tcmu_dev {
 #define TCMU_DEV_BIT_BROKEN 1
 #define TCMU_DEV_BIT_BLOCKED 2
 #define TCMU_DEV_BIT_TMR_NOTIFY 3
+#define TCM_DEV_BIT_PLUGGED 4
 	unsigned long flags;
 
 	struct uio_info uio_info;
@@ -959,6 +961,25 @@ static uint32_t ring_insert_padding(struct tcmu_dev *udev, size_t cmd_size)
 	return cmd_head;
 }
 
+static void tcmu_unplug_device(struct se_dev_plug *se_plug)
+{
+	struct se_device *se_dev = se_plug->se_dev;
+	struct tcmu_dev *udev = TCMU_DEV(se_dev);
+
+	clear_bit(TCM_DEV_BIT_PLUGGED, &udev->flags);
+	uio_event_notify(&udev->uio_info);
+}
+
+static struct se_dev_plug *tcmu_plug_device(struct se_device *se_dev)
+{
+	struct tcmu_dev *udev = TCMU_DEV(se_dev);
+
+	if (!test_and_set_bit(TCM_DEV_BIT_PLUGGED, &udev->flags))
+		return &udev->se_plug;
+
+	return NULL;
+}
+
 /**
  * queue_cmd_ring - queue cmd to ring or internally
  * @tcmu_cmd: cmd to queue
@@ -1086,8 +1107,8 @@ static int queue_cmd_ring(struct tcmu_cmd *tcmu_cmd, sense_reason_t *scsi_err)
 
 	list_add_tail(&tcmu_cmd->queue_entry, &udev->inflight_queue);
 
-	/* TODO: only if FLUSH and FUA? */
-	uio_event_notify(&udev->uio_info);
+	if (!test_bit(TCM_DEV_BIT_PLUGGED, &udev->flags))
+		uio_event_notify(&udev->uio_info);
 
 	return 0;
 
@@ -2840,6 +2861,8 @@ static struct target_backend_ops tcmu_ops = {
 	.configure_device	= tcmu_configure_device,
 	.destroy_device		= tcmu_destroy_device,
 	.free_device		= tcmu_free_device,
+	.unplug_device		= tcmu_unplug_device,
+	.plug_device		= tcmu_plug_device,
 	.parse_cdb		= tcmu_parse_cdb,
 	.tmr_notify		= tcmu_tmr_notify,
 	.set_configfs_dev_params = tcmu_set_configfs_dev_params,
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 24/25] target: flush submission work during TMR processing
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (22 preceding siblings ...)
  2021-02-17 20:28 ` [PATCH 23/25] target_core_user: " Mike Christie
@ 2021-02-17 20:28 ` Mike Christie
  2021-02-19 19:46   ` Bodo Stroesser
  2021-02-17 20:28 ` [PATCH 25/25] target: make completion affinity configurable Mike Christie
  24 siblings, 1 reply; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:28 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

If a cmd is on the submission workqueue then the TMR code will
miss it, and end up returning task not found or success for
lun resets. The fabric driver might then tell the initiator that
the running cmds have been handled when they are about to run.

This adds a flush when we are processing TMRs to make sure queued
cmds do not run after returning the TMR response.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Tested-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/target/target_core_tmr.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index 7347285471fa..e7fcbc09f9db 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -124,6 +124,8 @@ void core_tmr_abort_task(
 	int i;
 
 	for (i = 0; i < dev->queue_cnt; i++) {
+		flush_work(&dev->queues[i].sq.work);
+
 		spin_lock_irqsave(&dev->queues[i].lock, flags);
 		list_for_each_entry_safe(se_cmd, next, &dev->queues[i].state_list,
 					 state_list) {
@@ -302,6 +304,8 @@ static void core_tmr_drain_state_list(
 	 * in the Control Mode Page.
 	 */
 	for (i = 0; i < dev->queue_cnt; i++) {
+		flush_work(&dev->queues[i].sq.work);
+
 		spin_lock_irqsave(&dev->queues[i].lock, flags);
 		list_for_each_entry_safe(cmd, next, &dev->queues[i].state_list,
 					 state_list) {
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* [PATCH 25/25] target: make completion affinity configurable
  2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
                   ` (23 preceding siblings ...)
  2021-02-17 20:28 ` [PATCH 24/25] target: flush submission work during TMR processing Mike Christie
@ 2021-02-17 20:28 ` Mike Christie
  24 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-17 20:28 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie, Himanshu Madhani

It may not always be best to complete the IO on same CPU as it was
submitted on. This allows userspace to config it.

It's useful for vhost-scsi which has the single thread for submissions
and completions. Forcing the completion on the submission cpu is not
always the best thing to do since the thread could be running
on a different CPU now, and it conflicts with what the user has setup
in the lower levels with settings like the block layer rq_affinity
or for network block devices what the user has setup on their nic.

The new setting is in
/sys/kernel/config/target/$fabric/$target/param/cmd_completion_affinity

Writing 0 gives the current default behavior of completing on
the submission CPU. 1 completes the cmd on the CPU the lower layers
sent it to us from.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>
---
 drivers/target/target_core_fabric_configfs.c | 49 ++++++++++++++++++++
 drivers/target/target_core_internal.h        |  1 +
 drivers/target/target_core_transport.c       |  9 +++-
 include/target/target_core_base.h            |  7 +++
 4 files changed, 64 insertions(+), 2 deletions(-)

diff --git a/drivers/target/target_core_fabric_configfs.c b/drivers/target/target_core_fabric_configfs.c
index ee85602213f7..7ab27580758a 100644
--- a/drivers/target/target_core_fabric_configfs.c
+++ b/drivers/target/target_core_fabric_configfs.c
@@ -892,6 +892,7 @@ static void target_fabric_release_wwn(struct config_item *item)
 	struct target_fabric_configfs *tf = wwn->wwn_tf;
 
 	configfs_remove_default_groups(&wwn->fabric_stat_group);
+	configfs_remove_default_groups(&wwn->param_group);
 	tf->tf_ops->fabric_drop_wwn(wwn);
 }
 
@@ -918,6 +919,49 @@ TF_CIT_SETUP(wwn_fabric_stats, NULL, NULL, NULL);
 
 /* End of tfc_wwn_fabric_stats_cit */
 
+static ssize_t
+target_fabric_wwn_cmd_completion_affinity_show(struct config_item *item,
+					       char *page)
+{
+	struct se_wwn *wwn = container_of(to_config_group(item), struct se_wwn,
+					  param_group);
+	return sprintf(page, "%u\n", wwn->cmd_compl_affinity);
+}
+
+static ssize_t
+target_fabric_wwn_cmd_completion_affinity_store(struct config_item *item,
+						const char *page, size_t count)
+{
+	struct se_wwn *wwn = container_of(to_config_group(item), struct se_wwn,
+					  param_group);
+	u8 compl_val;
+
+	if (kstrtou8(page, 0, &compl_val))
+		return -EINVAL;
+
+	switch (compl_val) {
+	case SE_COMPL_AFFINITY_CPUID:
+	case SE_COMPL_AFFINITY_CURR_CPU:
+		wwn->cmd_compl_affinity = compl_val;
+		break;
+	default:
+		pr_err("Command completion value must be between %d and %d\n",
+		       SE_COMPL_AFFINITY_CPUID, SE_COMPL_AFFINITY_CURR_CPU);
+		return -EINVAL;
+	}
+
+	wwn->cmd_compl_affinity = compl_val;
+	return count;
+}
+CONFIGFS_ATTR(target_fabric_wwn_, cmd_completion_affinity);
+
+static struct configfs_attribute *target_fabric_wwn_param_attrs[] = {
+	&target_fabric_wwn_attr_cmd_completion_affinity,
+	NULL,
+};
+
+TF_CIT_SETUP(wwn_param, NULL, NULL, target_fabric_wwn_param_attrs);
+
 /* Start of tfc_wwn_cit */
 
 static struct config_group *target_fabric_make_wwn(
@@ -945,6 +989,10 @@ static struct config_group *target_fabric_make_wwn(
 			&tf->tf_wwn_fabric_stats_cit);
 	configfs_add_default_group(&wwn->fabric_stat_group, &wwn->wwn_group);
 
+	config_group_init_type_name(&wwn->param_group, "param",
+			&tf->tf_wwn_param_cit);
+	configfs_add_default_group(&wwn->param_group, &wwn->wwn_group);
+
 	if (tf->tf_ops->add_wwn_groups)
 		tf->tf_ops->add_wwn_groups(wwn);
 	return &wwn->wwn_group;
@@ -974,6 +1022,7 @@ int target_fabric_setup_cits(struct target_fabric_configfs *tf)
 	target_fabric_setup_discovery_cit(tf);
 	target_fabric_setup_wwn_cit(tf);
 	target_fabric_setup_wwn_fabric_stats_cit(tf);
+	target_fabric_setup_wwn_param_cit(tf);
 	target_fabric_setup_tpg_cit(tf);
 	target_fabric_setup_tpg_base_cit(tf);
 	target_fabric_setup_tpg_port_cit(tf);
diff --git a/drivers/target/target_core_internal.h b/drivers/target/target_core_internal.h
index 56f841fd7f04..a343bcfa2180 100644
--- a/drivers/target/target_core_internal.h
+++ b/drivers/target/target_core_internal.h
@@ -34,6 +34,7 @@ struct target_fabric_configfs {
 	struct config_item_type tf_discovery_cit;
 	struct config_item_type	tf_wwn_cit;
 	struct config_item_type tf_wwn_fabric_stats_cit;
+	struct config_item_type tf_wwn_param_cit;
 	struct config_item_type tf_tpg_cit;
 	struct config_item_type tf_tpg_base_cit;
 	struct config_item_type tf_tpg_lun_cit;
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 18cb00a1ee2f..16d1fce86d7f 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -857,7 +857,8 @@ static bool target_cmd_interrupted(struct se_cmd *cmd)
 /* May be called from interrupt context so must not sleep. */
 void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
 {
-	int success;
+	struct se_wwn *wwn = cmd->se_sess->se_tpg->se_tpg_wwn;
+	int success, cpu = WORK_CPU_UNBOUND;
 	unsigned long flags;
 
 	if (target_cmd_interrupted(cmd))
@@ -884,7 +885,11 @@ void target_complete_cmd(struct se_cmd *cmd, u8 scsi_status)
 
 	INIT_WORK(&cmd->work, success ? target_complete_ok_work :
 		  target_complete_failure_work);
-	queue_work_on(cmd->cpuid, target_completion_wq, &cmd->work);
+
+	if (wwn->cmd_compl_affinity == SE_COMPL_AFFINITY_CPUID)
+		cpu = cmd->cpuid;
+
+	queue_work_on(cpu, target_completion_wq, &cmd->work);
 }
 EXPORT_SYMBOL(target_complete_cmd);
 
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index b8e0a3250bd0..4ed385537301 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -943,11 +943,18 @@ static inline struct se_portal_group *param_to_tpg(struct config_item *item)
 			tpg_param_group);
 }
 
+enum {
+	SE_COMPL_AFFINITY_CPUID,	/* Use se_cmd's cpuid for completion */
+	SE_COMPL_AFFINITY_CURR_CPU,	/* Complete on current CPU */
+};
+
 struct se_wwn {
 	struct target_fabric_configfs *wwn_tf;
 	void			*priv;
 	struct config_group	wwn_group;
 	struct config_group	fabric_stat_group;
+	struct config_group	param_group;
+	u8			cmd_compl_affinity;
 };
 
 static inline void atomic_inc_mb(atomic_t *v)
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* Re: [PATCH 05/25] srpt: Convert to new submission API
  2021-02-17 20:27 ` [PATCH 05/25] srpt: Convert to new submission API Mike Christie
@ 2021-02-19  0:22   ` Bart Van Assche
  0 siblings, 0 replies; 39+ messages in thread
From: Bart Van Assche @ 2021-02-19  0:22 UTC (permalink / raw)
  To: Mike Christie, bostroesser, mst, stefanha, Chaitanya.Kulkarni,
	hch, loberman, martin.petersen, linux-scsi, target-devel

On 2/17/21 12:27 PM, Mike Christie wrote:
> target_submit_cmd_map_sgls is being removed, so convert srpt to
> the new submission API.
> 
> srpt uses target_stop_session to sync session shutdown with lio core,
> so we use target_init_cmd/target_submit_prep/target_submit, because
> target_init_cmd will detect the target_stop_session call and return
> an error.


Reviewed-by: Bart Van Assche <bvanassche@acm.org>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 15/25] target: add gfp_t arg to target_cmd_init_cdb
  2021-02-17 20:28 ` [PATCH 15/25] target: add gfp_t arg to target_cmd_init_cdb Mike Christie
@ 2021-02-19 18:53   ` Bodo Stroesser
  2021-02-19 19:32   ` Bodo Stroesser
  1 sibling, 0 replies; 39+ messages in thread
From: Bodo Stroesser @ 2021-02-19 18:53 UTC (permalink / raw)
  To: Mike Christie, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel

On 17.02.21 21:28, Mike Christie wrote:
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1427,7 +1427,7 @@ transport_check_alloc_task_attr(struct se_cmd *cmd)
>   }
>   
>   sense_reason_t
> -target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
> +target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb, gfp_t gfp)
>   {
>   	sense_reason_t ret;
>   
> @@ -1448,8 +1448,7 @@ target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
>   	 * setup the pointer from __t_task_cdb to t_task_cdb.
>   	 */
>   	if (scsi_command_size(cdb) > sizeof(cmd->__t_task_cdb)) {
> -		cmd->t_task_cdb = kzalloc(scsi_command_size(cdb),
> -						GFP_KERNEL);
> +		cmd->t_task_cdb = kzalloc(scsi_command_size(cdb), gfp);
>   		if (!cmd->t_task_cdb) {
>   			pr_err("Unable to allocate cmd->t_task_cdb"
>   				" %u > sizeof(cmd->__t_task_cdb): %lu ops\n",
> @@ -1638,6 +1637,7 @@ EXPORT_SYMBOL_GPL(target_init_cmd);
>    * @sgl_bidi_count: scatterlist count for bidirectional READ mapping
>    * @sgl_prot: struct scatterlist memory protection information
>    * @sgl_prot_count: scatterlist count for protection information
> + * @gfp_t: gfp allocation type


Just a nit: shoulodn't it be "@gfp: ..."?

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 14/25] target: remove target_submit_cmd_map_sgls
  2021-02-17 20:28 ` [PATCH 14/25] target: remove target_submit_cmd_map_sgls Mike Christie
@ 2021-02-19 19:01   ` Bodo Stroesser
  0 siblings, 0 replies; 39+ messages in thread
From: Bodo Stroesser @ 2021-02-19 19:01 UTC (permalink / raw)
  To: Mike Christie, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel

On 17.02.21 21:28, Mike Christie wrote:
> Convert target_submit_cmd to do its own calls and then remove
> target_submit_cmd_map_sgls since no one uses it.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> Tested-by: Laurence Oberman <loberman@redhat.com>
> ---
>   drivers/target/target_core_transport.c | 76 ++++++--------------------
>   include/target/target_core_fabric.h    |  6 +-
>   2 files changed, 18 insertions(+), 64 deletions(-)
> 

Looks good.

Reviewed-by: Bodo Stroesser <bostroesser@gmail.com>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 16/25] target: add workqueue based cmd submission
  2021-02-17 20:28 ` [PATCH 16/25] target: add workqueue based cmd submission Mike Christie
@ 2021-02-19 19:10   ` Bodo Stroesser
  0 siblings, 0 replies; 39+ messages in thread
From: Bodo Stroesser @ 2021-02-19 19:10 UTC (permalink / raw)
  To: Mike Christie, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel

On 17.02.21 21:28, Mike Christie wrote:
> loop and vhost-scsi do their target cmd submission from driver
> workqueues. This allows them to avoid an issue where the backend may
> block waiting for resources like tags/requests, mem/locks, etc
> and that ends up blocking their entire submission path and for the
> case of vhost-scsi both the submission and completion path.
> 
> This patch adds a helper drivers can use to submit from a lio
> workqueue. This code will then be extended in the next patches to
> fix the plugging of backend devices.
> 
> Note: I'm only converting vhost/loop initially, but the workqueue
> based submission will work for other drivers and have similar
> benefits where the main target loops will not end up blocking one
> some backend resource. I'll port others when I have more time to
> test as I think we might want to make it configurable for some
> drivers.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> Tested-by: Laurence Oberman <loberman@redhat.com>
> ---
>   drivers/target/target_core_device.c    | 10 ++++--
>   drivers/target/target_core_internal.h  |  1 +
>   drivers/target/target_core_transport.c | 42 +++++++++++++++++++++++++-
>   include/target/target_core_base.h      |  8 ++++-
>   include/target/target_core_fabric.h    |  2 ++
>   5 files changed, 59 insertions(+), 4 deletions(-)
> 


Looks good.

Reviewed-by: Bodo Stroesser <bostroesser@gmail.com>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 15/25] target: add gfp_t arg to target_cmd_init_cdb
  2021-02-17 20:28 ` [PATCH 15/25] target: add gfp_t arg to target_cmd_init_cdb Mike Christie
  2021-02-19 18:53   ` Bodo Stroesser
@ 2021-02-19 19:32   ` Bodo Stroesser
  2021-02-19 19:41     ` michael.christie
  1 sibling, 1 reply; 39+ messages in thread
From: Bodo Stroesser @ 2021-02-19 19:32 UTC (permalink / raw)
  To: Mike Christie, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel

On 17.02.21 21:28, Mike Christie wrote:
> tcm_loop could be used like a normal block device, so we can't use
> GFP_KERNEL. This adds a gfp_t arg to target_cmd_init_cdb and covnerts
> the users. For every driver but loop I kept GFP_KERNEL. For loop and
> xcopy I switched to GFP_NOIO.


In the patch below for xcopy GFP_KERNEL is inserted.

> 
> This will also be useful in the later patches where loop needs to
> do target_submit_prep from interrupt context to get a ref to the
> se_device, and so it will need to use GFP_ATOMIC.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Tested-by: Laurence Oberman <loberman@redhat.com>
> ---
>   drivers/infiniband/ulp/srpt/ib_srpt.c  |  3 ++-
>   drivers/scsi/qla2xxx/tcm_qla2xxx.c     |  3 ++-
>   drivers/target/iscsi/iscsi_target.c    |  3 ++-
>   drivers/target/loopback/tcm_loop.c     |  3 ++-
>   drivers/target/target_core_transport.c | 14 ++++++++------
>   drivers/target/target_core_xcopy.c     |  2 +-
>   drivers/target/tcm_fc/tfc_cmd.c        |  2 +-
>   drivers/vhost/scsi.c                   |  2 +-
>   drivers/xen/xen-scsiback.c             |  2 +-
>   include/target/target_core_fabric.h    |  5 +++--
>   10 files changed, 23 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 87741e0b4bca..51c386a215f5 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -1537,7 +1537,8 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
>   		goto busy;
>   	}
>   
> -	if (target_submit_prep(cmd, srp_cmd->cdb, sg, sg_cnt, NULL, 0, NULL, 0))
> +	if (target_submit_prep(cmd, srp_cmd->cdb, sg, sg_cnt, NULL, 0, NULL, 0,
> +			       GFP_KERNEL))
>   		return;
>   
>   	target_submit(cmd);
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> index 56394d901791..12a2265eb2de 100644
> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
> @@ -492,7 +492,8 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha, struct qla_tgt_cmd *cmd,
>   	if (rc)
>   		return rc;
>   
> -	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0))
> +	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0,
> +			       GFP_KERNEL))
>   		return 0;
>   
>   	target_submit(se_cmd);
> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
> index f2107705f2ea..566adfde1661 100644
> --- a/drivers/target/iscsi/iscsi_target.c
> +++ b/drivers/target/iscsi/iscsi_target.c
> @@ -1166,7 +1166,8 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
>   
>   	target_get_sess_cmd(&cmd->se_cmd, true);
>   
> -	cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb);
> +	cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb,
> +						GFP_KERNEL);
>   	if (cmd->sense_reason) {
>   		if (cmd->sense_reason == TCM_OUT_OF_RESOURCES) {
>   			return iscsit_add_reject_cmd(cmd,
> diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
> index 461f4125fcab..677e4b8f0642 100644
> --- a/drivers/target/loopback/tcm_loop.c
> +++ b/drivers/target/loopback/tcm_loop.c
> @@ -156,7 +156,8 @@ static void tcm_loop_submission_work(struct work_struct *work)
>   
>   	if (target_submit_prep(se_cmd, sc->cmnd, scsi_sglist(sc),
>   			       scsi_sg_count(sc), sgl_bidi, sgl_bidi_count,
> -			       scsi_prot_sglist(sc), scsi_prot_sg_count(sc)))
> +			       scsi_prot_sglist(sc), scsi_prot_sg_count(sc),
> +			       GFP_NOIO))
>   		return;
>   
>   	target_submit(se_cmd);
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 1f35cce6e92b..6c88ca832da6 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -1427,7 +1427,7 @@ transport_check_alloc_task_attr(struct se_cmd *cmd)
>   }
>   
>   sense_reason_t
> -target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
> +target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb, gfp_t gfp)
>   {
>   	sense_reason_t ret;
>   
> @@ -1448,8 +1448,7 @@ target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
>   	 * setup the pointer from __t_task_cdb to t_task_cdb.
>   	 */
>   	if (scsi_command_size(cdb) > sizeof(cmd->__t_task_cdb)) {
> -		cmd->t_task_cdb = kzalloc(scsi_command_size(cdb),
> -						GFP_KERNEL);
> +		cmd->t_task_cdb = kzalloc(scsi_command_size(cdb), gfp);
>   		if (!cmd->t_task_cdb) {
>   			pr_err("Unable to allocate cmd->t_task_cdb"
>   				" %u > sizeof(cmd->__t_task_cdb): %lu ops\n",
> @@ -1638,6 +1637,7 @@ EXPORT_SYMBOL_GPL(target_init_cmd);
>    * @sgl_bidi_count: scatterlist count for bidirectional READ mapping
>    * @sgl_prot: struct scatterlist memory protection information
>    * @sgl_prot_count: scatterlist count for protection information
> + * @gfp_t: gfp allocation type
>    *
>    * Returns:
>    *	- less than zero to signal failure.
> @@ -1648,11 +1648,12 @@ EXPORT_SYMBOL_GPL(target_init_cmd);
>   int target_submit_prep(struct se_cmd *se_cmd, unsigned char *cdb,
>   		       struct scatterlist *sgl, u32 sgl_count,
>   		       struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
> -		       struct scatterlist *sgl_prot, u32 sgl_prot_count)
> +		       struct scatterlist *sgl_prot, u32 sgl_prot_count,
> +		       gfp_t gfp)
>   {
>   	sense_reason_t rc;
>   
> -	rc = target_cmd_init_cdb(se_cmd, cdb);
> +	rc = target_cmd_init_cdb(se_cmd, cdb, gfp);
>   	if (rc)
>   		goto send_cc_direct;
>   
> @@ -1788,7 +1789,8 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
>   	if (rc)
>   		return;
>   
> -	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0))
> +	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0,
> +			       GFP_KERNEL))
>   		return;
>   
>   	target_submit(se_cmd);
> diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
> index e86cc6135587..d31ed071cb08 100644
> --- a/drivers/target/target_core_xcopy.c
> +++ b/drivers/target/target_core_xcopy.c
> @@ -554,7 +554,7 @@ static int target_xcopy_setup_pt_cmd(
>   	}
>   	cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
>   
> -	if (target_cmd_init_cdb(cmd, cdb))
> +	if (target_cmd_init_cdb(cmd, cdb, GFP_KERNEL))
>   		return -EINVAL;
>   
>   	cmd->tag = 0;
> diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
> index 1376501ee3d0..410b723f9d79 100644
> --- a/drivers/target/tcm_fc/tfc_cmd.c
> +++ b/drivers/target/tcm_fc/tfc_cmd.c
> @@ -555,7 +555,7 @@ static void ft_send_work(struct work_struct *work)
>   		goto err;
>   
>   	if (target_submit_prep(&cmd->se_cmd, fcp->fc_cdb, NULL, 0, NULL, 0,
> -			       NULL, 0))
> +			       NULL, 0, GFP_KERNEL))
>   		return;
>   
>   	target_submit(&cmd->se_cmd);
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 76508d408bb3..93f5631b469c 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -811,7 +811,7 @@ static void vhost_scsi_submission_work(struct work_struct *work)
>   
>   	if (target_submit_prep(se_cmd, cmd->tvc_cdb, sg_ptr,
>   			       cmd->tvc_sgl_count, NULL, 0, sg_prot_ptr,
> -			       cmd->tvc_prot_sgl_count))
> +			       cmd->tvc_prot_sgl_count, GFP_KERNEL))
>   		return;
>   
>   	target_submit(se_cmd);
> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
> index 7bf9a6bede6d..eb3d8e35cbcd 100644
> --- a/drivers/xen/xen-scsiback.c
> +++ b/drivers/xen/xen-scsiback.c
> @@ -368,7 +368,7 @@ static void scsiback_cmd_exec(struct vscsibk_pend *pending_req)
>   			pending_req->sc_data_direction, TARGET_SCF_ACK_KREF);
>   
>   	if (target_submit_prep(se_cmd, pending_req->cmnd, pending_req->sgl,
> -			       pending_req->n_sg, NULL, 0, NULL, 0))
> +			       pending_req->n_sg, NULL, 0, NULL, 0, GFP_KERNEL))
>   		return;
>   
>   	target_submit(se_cmd);
> diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
> index 86b0d4a7df92..0543ab107723 100644
> --- a/include/target/target_core_fabric.h
> +++ b/include/target/target_core_fabric.h
> @@ -157,10 +157,11 @@ int	target_init_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
>   int	target_submit_prep(struct se_cmd *se_cmd, unsigned char *cdb,
>   		struct scatterlist *sgl, u32 sgl_count,
>   		struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
> -		struct scatterlist *sgl_prot, u32 sgl_prot_count);
> +		struct scatterlist *sgl_prot, u32 sgl_prot_count, gfp_t gfp);
>   void	target_submit(struct se_cmd *se_cmd);
>   sense_reason_t transport_lookup_cmd_lun(struct se_cmd *);
> -sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
> +sense_reason_t target_cmd_init_cdb(struct se_cmd *se_cmd, unsigned char *cdb,
> +				   gfp_t gfp);
>   sense_reason_t target_cmd_parse_cdb(struct se_cmd *);
>   void	target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
>   		unsigned char *, u64, u32, int, int, int);
> 

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 19/25] tcm loop: use lio wq cmd submission helper
  2021-02-17 20:28 ` [PATCH 19/25] tcm loop: use lio wq cmd submission helper Mike Christie
@ 2021-02-19 19:38   ` Bodo Stroesser
  0 siblings, 0 replies; 39+ messages in thread
From: Bodo Stroesser @ 2021-02-19 19:38 UTC (permalink / raw)
  To: Mike Christie, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel

On 17.02.21 21:28, Mike Christie wrote:
> Convert loop to use the lio wq cmd submission helper.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> Tested-by: Laurence Oberman <loberman@redhat.com>
> ---
>   drivers/target/loopback/tcm_loop.c | 22 ++++++----------------
>   drivers/target/loopback/tcm_loop.h |  1 -
>   2 files changed, 6 insertions(+), 17 deletions(-)


Looks good.

Reviewed-by: Bodo Stroesser <bostroesser@gmail.com>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 21/25] target: fix backend plugging
  2021-02-17 20:28 ` [PATCH 21/25] target: fix backend plugging Mike Christie
@ 2021-02-19 19:40   ` Bodo Stroesser
  0 siblings, 0 replies; 39+ messages in thread
From: Bodo Stroesser @ 2021-02-19 19:40 UTC (permalink / raw)
  To: Mike Christie, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel

On 17.02.21 21:28, Mike Christie wrote:
> target_core_iblock is plugging and unplugging on every command and this
> is causing perf issues for drivers that prefer batched cmds. With the
> last patches we can now take multiple cmds from a fabric driver queue
> and then pass them down the backend drivers in a batch. This patch adds
> this support by adding 2 callouts to the backend for plugging and
> unplugging the device. The next 2 patches add support for iblock and
> tcmu device plugging.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/target/target_core_transport.c | 43 +++++++++++++++++++++++++-
>   include/target/target_core_backend.h   |  2 ++
>   include/target/target_core_base.h      |  4 +++
>   3 files changed, 48 insertions(+), 1 deletion(-)
> 

Looks good.

Reviewed-by: Bodo Stroesser <bostroesser@gmail.com>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 15/25] target: add gfp_t arg to target_cmd_init_cdb
  2021-02-19 19:32   ` Bodo Stroesser
@ 2021-02-19 19:41     ` michael.christie
  0 siblings, 0 replies; 39+ messages in thread
From: michael.christie @ 2021-02-19 19:41 UTC (permalink / raw)
  To: Bodo Stroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel

On 2/19/21 1:32 PM, Bodo Stroesser wrote:
> On 17.02.21 21:28, Mike Christie wrote:
>> tcm_loop could be used like a normal block device, so we can't use
>> GFP_KERNEL. This adds a gfp_t arg to target_cmd_init_cdb and covnerts
>> the users. For every driver but loop I kept GFP_KERNEL. For loop and
>> xcopy I switched to GFP_NOIO.
> 
> 
> In the patch below for xcopy GFP_KERNEL is inserted.

The code is correct. I'll fix the commit message. I thought it needed
to be NOIO when I first made the patch. I later realized we would never
hit an issue since it's outside the main IO path that would get executed if
WRITEs were done to satisfy the GFP_KERNEL alloc in the xcopy code.


> 
>>
>> This will also be useful in the later patches where loop needs to
>> do target_submit_prep from interrupt context to get a ref to the
>> se_device, and so it will need to use GFP_ATOMIC.
>>
>> Signed-off-by: Mike Christie <michael.christie@oracle.com>
>> Reviewed-by: Christoph Hellwig <hch@lst.de>
>> Tested-by: Laurence Oberman <loberman@redhat.com>
>> ---
>>   drivers/infiniband/ulp/srpt/ib_srpt.c  |  3 ++-
>>   drivers/scsi/qla2xxx/tcm_qla2xxx.c     |  3 ++-
>>   drivers/target/iscsi/iscsi_target.c    |  3 ++-
>>   drivers/target/loopback/tcm_loop.c     |  3 ++-
>>   drivers/target/target_core_transport.c | 14 ++++++++------
>>   drivers/target/target_core_xcopy.c     |  2 +-
>>   drivers/target/tcm_fc/tfc_cmd.c        |  2 +-
>>   drivers/vhost/scsi.c                   |  2 +-
>>   drivers/xen/xen-scsiback.c             |  2 +-
>>   include/target/target_core_fabric.h    |  5 +++--
>>   10 files changed, 23 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
>> index 87741e0b4bca..51c386a215f5 100644
>> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
>> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
>> @@ -1537,7 +1537,8 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
>>           goto busy;
>>       }
>>   -    if (target_submit_prep(cmd, srp_cmd->cdb, sg, sg_cnt, NULL, 0, NULL, 0))
>> +    if (target_submit_prep(cmd, srp_cmd->cdb, sg, sg_cnt, NULL, 0, NULL, 0,
>> +                   GFP_KERNEL))
>>           return;
>>         target_submit(cmd);
>> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
>> index 56394d901791..12a2265eb2de 100644
>> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
>> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
>> @@ -492,7 +492,8 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha, struct qla_tgt_cmd *cmd,
>>       if (rc)
>>           return rc;
>>   -    if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0))
>> +    if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0,
>> +                   GFP_KERNEL))
>>           return 0;
>>         target_submit(se_cmd);
>> diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
>> index f2107705f2ea..566adfde1661 100644
>> --- a/drivers/target/iscsi/iscsi_target.c
>> +++ b/drivers/target/iscsi/iscsi_target.c
>> @@ -1166,7 +1166,8 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
>>         target_get_sess_cmd(&cmd->se_cmd, true);
>>   -    cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb);
>> +    cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb,
>> +                        GFP_KERNEL);
>>       if (cmd->sense_reason) {
>>           if (cmd->sense_reason == TCM_OUT_OF_RESOURCES) {
>>               return iscsit_add_reject_cmd(cmd,
>> diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
>> index 461f4125fcab..677e4b8f0642 100644
>> --- a/drivers/target/loopback/tcm_loop.c
>> +++ b/drivers/target/loopback/tcm_loop.c
>> @@ -156,7 +156,8 @@ static void tcm_loop_submission_work(struct work_struct *work)
>>         if (target_submit_prep(se_cmd, sc->cmnd, scsi_sglist(sc),
>>                      scsi_sg_count(sc), sgl_bidi, sgl_bidi_count,
>> -                   scsi_prot_sglist(sc), scsi_prot_sg_count(sc)))
>> +                   scsi_prot_sglist(sc), scsi_prot_sg_count(sc),
>> +                   GFP_NOIO))
>>           return;
>>         target_submit(se_cmd);
>> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
>> index 1f35cce6e92b..6c88ca832da6 100644
>> --- a/drivers/target/target_core_transport.c
>> +++ b/drivers/target/target_core_transport.c
>> @@ -1427,7 +1427,7 @@ transport_check_alloc_task_attr(struct se_cmd *cmd)
>>   }
>>     sense_reason_t
>> -target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
>> +target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb, gfp_t gfp)
>>   {
>>       sense_reason_t ret;
>>   @@ -1448,8 +1448,7 @@ target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
>>        * setup the pointer from __t_task_cdb to t_task_cdb.
>>        */
>>       if (scsi_command_size(cdb) > sizeof(cmd->__t_task_cdb)) {
>> -        cmd->t_task_cdb = kzalloc(scsi_command_size(cdb),
>> -                        GFP_KERNEL);
>> +        cmd->t_task_cdb = kzalloc(scsi_command_size(cdb), gfp);
>>           if (!cmd->t_task_cdb) {
>>               pr_err("Unable to allocate cmd->t_task_cdb"
>>                   " %u > sizeof(cmd->__t_task_cdb): %lu ops\n",
>> @@ -1638,6 +1637,7 @@ EXPORT_SYMBOL_GPL(target_init_cmd);
>>    * @sgl_bidi_count: scatterlist count for bidirectional READ mapping
>>    * @sgl_prot: struct scatterlist memory protection information
>>    * @sgl_prot_count: scatterlist count for protection information
>> + * @gfp_t: gfp allocation type
>>    *
>>    * Returns:
>>    *    - less than zero to signal failure.
>> @@ -1648,11 +1648,12 @@ EXPORT_SYMBOL_GPL(target_init_cmd);
>>   int target_submit_prep(struct se_cmd *se_cmd, unsigned char *cdb,
>>                  struct scatterlist *sgl, u32 sgl_count,
>>                  struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
>> -               struct scatterlist *sgl_prot, u32 sgl_prot_count)
>> +               struct scatterlist *sgl_prot, u32 sgl_prot_count,
>> +               gfp_t gfp)
>>   {
>>       sense_reason_t rc;
>>   -    rc = target_cmd_init_cdb(se_cmd, cdb);
>> +    rc = target_cmd_init_cdb(se_cmd, cdb, gfp);
>>       if (rc)
>>           goto send_cc_direct;
>>   @@ -1788,7 +1789,8 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
>>       if (rc)
>>           return;
>>   -    if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0))
>> +    if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0,
>> +                   GFP_KERNEL))
>>           return;
>>         target_submit(se_cmd);
>> diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
>> index e86cc6135587..d31ed071cb08 100644
>> --- a/drivers/target/target_core_xcopy.c
>> +++ b/drivers/target/target_core_xcopy.c
>> @@ -554,7 +554,7 @@ static int target_xcopy_setup_pt_cmd(
>>       }
>>       cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
>>   -    if (target_cmd_init_cdb(cmd, cdb))
>> +    if (target_cmd_init_cdb(cmd, cdb, GFP_KERNEL))
>>           return -EINVAL;
>>         cmd->tag = 0;
>> diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
>> index 1376501ee3d0..410b723f9d79 100644
>> --- a/drivers/target/tcm_fc/tfc_cmd.c
>> +++ b/drivers/target/tcm_fc/tfc_cmd.c
>> @@ -555,7 +555,7 @@ static void ft_send_work(struct work_struct *work)
>>           goto err;
>>         if (target_submit_prep(&cmd->se_cmd, fcp->fc_cdb, NULL, 0, NULL, 0,
>> -                   NULL, 0))
>> +                   NULL, 0, GFP_KERNEL))
>>           return;
>>         target_submit(&cmd->se_cmd);
>> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
>> index 76508d408bb3..93f5631b469c 100644
>> --- a/drivers/vhost/scsi.c
>> +++ b/drivers/vhost/scsi.c
>> @@ -811,7 +811,7 @@ static void vhost_scsi_submission_work(struct work_struct *work)
>>         if (target_submit_prep(se_cmd, cmd->tvc_cdb, sg_ptr,
>>                      cmd->tvc_sgl_count, NULL, 0, sg_prot_ptr,
>> -                   cmd->tvc_prot_sgl_count))
>> +                   cmd->tvc_prot_sgl_count, GFP_KERNEL))
>>           return;
>>         target_submit(se_cmd);
>> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
>> index 7bf9a6bede6d..eb3d8e35cbcd 100644
>> --- a/drivers/xen/xen-scsiback.c
>> +++ b/drivers/xen/xen-scsiback.c
>> @@ -368,7 +368,7 @@ static void scsiback_cmd_exec(struct vscsibk_pend *pending_req)
>>               pending_req->sc_data_direction, TARGET_SCF_ACK_KREF);
>>         if (target_submit_prep(se_cmd, pending_req->cmnd, pending_req->sgl,
>> -                   pending_req->n_sg, NULL, 0, NULL, 0))
>> +                   pending_req->n_sg, NULL, 0, NULL, 0, GFP_KERNEL))
>>           return;
>>         target_submit(se_cmd);
>> diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
>> index 86b0d4a7df92..0543ab107723 100644
>> --- a/include/target/target_core_fabric.h
>> +++ b/include/target/target_core_fabric.h
>> @@ -157,10 +157,11 @@ int    target_init_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
>>   int    target_submit_prep(struct se_cmd *se_cmd, unsigned char *cdb,
>>           struct scatterlist *sgl, u32 sgl_count,
>>           struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
>> -        struct scatterlist *sgl_prot, u32 sgl_prot_count);
>> +        struct scatterlist *sgl_prot, u32 sgl_prot_count, gfp_t gfp);
>>   void    target_submit(struct se_cmd *se_cmd);
>>   sense_reason_t transport_lookup_cmd_lun(struct se_cmd *);
>> -sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
>> +sense_reason_t target_cmd_init_cdb(struct se_cmd *se_cmd, unsigned char *cdb,
>> +                   gfp_t gfp);
>>   sense_reason_t target_cmd_parse_cdb(struct se_cmd *);
>>   void    target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
>>           unsigned char *, u64, u32, int, int, int);
>>


^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 23/25] target_core_user: add backend plug/unplug callouts
  2021-02-17 20:28 ` [PATCH 23/25] target_core_user: " Mike Christie
@ 2021-02-19 19:45   ` Bodo Stroesser
  0 siblings, 0 replies; 39+ messages in thread
From: Bodo Stroesser @ 2021-02-19 19:45 UTC (permalink / raw)
  To: Mike Christie, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel

On 17.02.21 21:28, Mike Christie wrote:
> This patch adds plug/unplug callouts for tcmu, so we can avoid the
> number of times we switch to userspace. Using this driver with tcm
> loop is a common config, and dependng on the nr_hw_queues
> (nr_hw_queues=1 performs much better) and fio jobs (lower num jobs
> around 4) this patch can increase IOPs by only around 5-10% because
> we hit other issues like the big per tcmu device mutex.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> ---
>   drivers/target/target_core_user.c | 27 +++++++++++++++++++++++++--
>   1 file changed, 25 insertions(+), 2 deletions(-)
> 


Looks good.

Reviewed-by: Bodo Stroesser <bostroesser@gmail.com>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* Re: [PATCH 24/25] target: flush submission work during TMR processing
  2021-02-17 20:28 ` [PATCH 24/25] target: flush submission work during TMR processing Mike Christie
@ 2021-02-19 19:46   ` Bodo Stroesser
  0 siblings, 0 replies; 39+ messages in thread
From: Bodo Stroesser @ 2021-02-19 19:46 UTC (permalink / raw)
  To: Mike Christie, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel

On 17.02.21 21:28, Mike Christie wrote:
> If a cmd is on the submission workqueue then the TMR code will
> miss it, and end up returning task not found or success for
> lun resets. The fabric driver might then tell the initiator that
> the running cmds have been handled when they are about to run.
> 
> This adds a flush when we are processing TMRs to make sure queued
> cmds do not run after returning the TMR response.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>
> Tested-by: Laurence Oberman <loberman@redhat.com>
> ---
>   drivers/target/target_core_tmr.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
> index 7347285471fa..e7fcbc09f9db 100644
> --- a/drivers/target/target_core_tmr.c
> +++ b/drivers/target/target_core_tmr.c
> @@ -124,6 +124,8 @@ void core_tmr_abort_task(
>   	int i;
>   
>   	for (i = 0; i < dev->queue_cnt; i++) {
> +		flush_work(&dev->queues[i].sq.work);
> +
>   		spin_lock_irqsave(&dev->queues[i].lock, flags);
>   		list_for_each_entry_safe(se_cmd, next, &dev->queues[i].state_list,
>   					 state_list) {
> @@ -302,6 +304,8 @@ static void core_tmr_drain_state_list(
>   	 * in the Control Mode Page.
>   	 */
>   	for (i = 0; i < dev->queue_cnt; i++) {
> +		flush_work(&dev->queues[i].sq.work);
> +
>   		spin_lock_irqsave(&dev->queues[i].lock, flags);
>   		list_for_each_entry_safe(cmd, next, &dev->queues[i].state_list,
>   					 state_list) {
> 

Looks good.

Reviewed-by: Bodo Stroesser <bostroesser@gmail.com>

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 15/25] target: add gfp_t arg to target_cmd_init_cdb
  2021-02-27 16:59 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
@ 2021-02-27 16:59 ` Mike Christie
  0 siblings, 0 replies; 39+ messages in thread
From: Mike Christie @ 2021-02-27 16:59 UTC (permalink / raw)
  To: bostroesser, mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

tcm_loop could be used like a normal block device, so we can't use
GFP_KERNEL and should use GFP_NOIO. This adds a gfp_t arg to
target_cmd_init_cdb and covnerts the users. For every driver but loop
I kept GFP_KERNEL.

This will also be useful in the later patches where loop needs to
do target_submit_prep from interrupt context to get a ref to the
se_device, and so it will need to use GFP_ATOMIC.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Laurence Oberman <loberman@redhat.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c  |  3 ++-
 drivers/scsi/qla2xxx/tcm_qla2xxx.c     |  3 ++-
 drivers/target/iscsi/iscsi_target.c    |  3 ++-
 drivers/target/loopback/tcm_loop.c     |  3 ++-
 drivers/target/target_core_transport.c | 14 ++++++++------
 drivers/target/target_core_xcopy.c     |  2 +-
 drivers/target/tcm_fc/tfc_cmd.c        |  2 +-
 drivers/vhost/scsi.c                   |  2 +-
 drivers/xen/xen-scsiback.c             |  2 +-
 include/target/target_core_fabric.h    |  5 +++--
 10 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 87741e0b4bca..51c386a215f5 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1537,7 +1537,8 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
 		goto busy;
 	}
 
-	if (target_submit_prep(cmd, srp_cmd->cdb, sg, sg_cnt, NULL, 0, NULL, 0))
+	if (target_submit_prep(cmd, srp_cmd->cdb, sg, sg_cnt, NULL, 0, NULL, 0,
+			       GFP_KERNEL))
 		return;
 
 	target_submit(cmd);
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 56394d901791..12a2265eb2de 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -492,7 +492,8 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha, struct qla_tgt_cmd *cmd,
 	if (rc)
 		return rc;
 
-	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0))
+	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0,
+			       GFP_KERNEL))
 		return 0;
 
 	target_submit(se_cmd);
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index f2107705f2ea..566adfde1661 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1166,7 +1166,8 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 
 	target_get_sess_cmd(&cmd->se_cmd, true);
 
-	cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb);
+	cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb,
+						GFP_KERNEL);
 	if (cmd->sense_reason) {
 		if (cmd->sense_reason == TCM_OUT_OF_RESOURCES) {
 			return iscsit_add_reject_cmd(cmd,
diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 461f4125fcab..677e4b8f0642 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -156,7 +156,8 @@ static void tcm_loop_submission_work(struct work_struct *work)
 
 	if (target_submit_prep(se_cmd, sc->cmnd, scsi_sglist(sc),
 			       scsi_sg_count(sc), sgl_bidi, sgl_bidi_count,
-			       scsi_prot_sglist(sc), scsi_prot_sg_count(sc)))
+			       scsi_prot_sglist(sc), scsi_prot_sg_count(sc),
+			       GFP_NOIO))
 		return;
 
 	target_submit(se_cmd);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 1f35cce6e92b..6c88ca832da6 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1427,7 +1427,7 @@ transport_check_alloc_task_attr(struct se_cmd *cmd)
 }
 
 sense_reason_t
-target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
+target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb, gfp_t gfp)
 {
 	sense_reason_t ret;
 
@@ -1448,8 +1448,7 @@ target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
 	 * setup the pointer from __t_task_cdb to t_task_cdb.
 	 */
 	if (scsi_command_size(cdb) > sizeof(cmd->__t_task_cdb)) {
-		cmd->t_task_cdb = kzalloc(scsi_command_size(cdb),
-						GFP_KERNEL);
+		cmd->t_task_cdb = kzalloc(scsi_command_size(cdb), gfp);
 		if (!cmd->t_task_cdb) {
 			pr_err("Unable to allocate cmd->t_task_cdb"
 				" %u > sizeof(cmd->__t_task_cdb): %lu ops\n",
@@ -1638,6 +1637,7 @@ EXPORT_SYMBOL_GPL(target_init_cmd);
  * @sgl_bidi_count: scatterlist count for bidirectional READ mapping
  * @sgl_prot: struct scatterlist memory protection information
  * @sgl_prot_count: scatterlist count for protection information
+ * @gfp: gfp allocation type
  *
  * Returns:
  *	- less than zero to signal failure.
@@ -1648,11 +1648,12 @@ EXPORT_SYMBOL_GPL(target_init_cmd);
 int target_submit_prep(struct se_cmd *se_cmd, unsigned char *cdb,
 		       struct scatterlist *sgl, u32 sgl_count,
 		       struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
-		       struct scatterlist *sgl_prot, u32 sgl_prot_count)
+		       struct scatterlist *sgl_prot, u32 sgl_prot_count,
+		       gfp_t gfp)
 {
 	sense_reason_t rc;
 
-	rc = target_cmd_init_cdb(se_cmd, cdb);
+	rc = target_cmd_init_cdb(se_cmd, cdb, gfp);
 	if (rc)
 		goto send_cc_direct;
 
@@ -1788,7 +1789,8 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 	if (rc)
 		return;
 
-	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0))
+	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0,
+			       GFP_KERNEL))
 		return;
 
 	target_submit(se_cmd);
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index e86cc6135587..d31ed071cb08 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -554,7 +554,7 @@ static int target_xcopy_setup_pt_cmd(
 	}
 	cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
 
-	if (target_cmd_init_cdb(cmd, cdb))
+	if (target_cmd_init_cdb(cmd, cdb, GFP_KERNEL))
 		return -EINVAL;
 
 	cmd->tag = 0;
diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index 1376501ee3d0..410b723f9d79 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -555,7 +555,7 @@ static void ft_send_work(struct work_struct *work)
 		goto err;
 
 	if (target_submit_prep(&cmd->se_cmd, fcp->fc_cdb, NULL, 0, NULL, 0,
-			       NULL, 0))
+			       NULL, 0, GFP_KERNEL))
 		return;
 
 	target_submit(&cmd->se_cmd);
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 76508d408bb3..93f5631b469c 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -811,7 +811,7 @@ static void vhost_scsi_submission_work(struct work_struct *work)
 
 	if (target_submit_prep(se_cmd, cmd->tvc_cdb, sg_ptr,
 			       cmd->tvc_sgl_count, NULL, 0, sg_prot_ptr,
-			       cmd->tvc_prot_sgl_count))
+			       cmd->tvc_prot_sgl_count, GFP_KERNEL))
 		return;
 
 	target_submit(se_cmd);
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 7bf9a6bede6d..eb3d8e35cbcd 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -368,7 +368,7 @@ static void scsiback_cmd_exec(struct vscsibk_pend *pending_req)
 			pending_req->sc_data_direction, TARGET_SCF_ACK_KREF);
 
 	if (target_submit_prep(se_cmd, pending_req->cmnd, pending_req->sgl,
-			       pending_req->n_sg, NULL, 0, NULL, 0))
+			       pending_req->n_sg, NULL, 0, NULL, 0, GFP_KERNEL))
 		return;
 
 	target_submit(se_cmd);
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 86b0d4a7df92..0543ab107723 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -157,10 +157,11 @@ int	target_init_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 int	target_submit_prep(struct se_cmd *se_cmd, unsigned char *cdb,
 		struct scatterlist *sgl, u32 sgl_count,
 		struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
-		struct scatterlist *sgl_prot, u32 sgl_prot_count);
+		struct scatterlist *sgl_prot, u32 sgl_prot_count, gfp_t gfp);
 void	target_submit(struct se_cmd *se_cmd);
 sense_reason_t transport_lookup_cmd_lun(struct se_cmd *);
-sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
+sense_reason_t target_cmd_init_cdb(struct se_cmd *se_cmd, unsigned char *cdb,
+				   gfp_t gfp);
 sense_reason_t target_cmd_parse_cdb(struct se_cmd *);
 void	target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
 		unsigned char *, u64, u32, int, int, int);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

* Re: [PATCH 15/25] target: add gfp_t arg to target_cmd_init_cdb
  2021-02-12  7:26 ` [PATCH 15/25] target: add gfp_t arg to target_cmd_init_cdb Mike Christie
@ 2021-02-16  8:40   ` Christoph Hellwig
  0 siblings, 0 replies; 39+ messages in thread
From: Christoph Hellwig @ 2021-02-16  8:40 UTC (permalink / raw)
  To: Mike Christie
  Cc: mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel

On Fri, Feb 12, 2021 at 01:26:32AM -0600, Mike Christie wrote:
> tcm_loop could be used like a normal block device, so we can't use
> GFP_KERNEL. This adds a gfp_t arg to target_cmd_init_cdb and covnerts
> the users. For every driver but loop I kept GFP_KERNEL. For loop and
> xcopy I switched to GFP_NOIO.
> 
> This will also be useful in the later patches where loop needs to
> do target_submit_prep from interrupt context to get a ref to the
> se_device, and so it will need to use GFP_ATOMIC.
> 
> Signed-off-by: Mike Christie <michael.christie@oracle.com>

Looks good,

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

^ permalink raw reply	[flat|nested] 39+ messages in thread

* [PATCH 15/25] target: add gfp_t arg to target_cmd_init_cdb
  2021-02-12  7:26 PATCH 00/25 V4] target: fix cmd plugging and submission Mike Christie
@ 2021-02-12  7:26 ` Mike Christie
  2021-02-16  8:40   ` Christoph Hellwig
  0 siblings, 1 reply; 39+ messages in thread
From: Mike Christie @ 2021-02-12  7:26 UTC (permalink / raw)
  To: mst, stefanha, Chaitanya.Kulkarni, hch, loberman,
	martin.petersen, linux-scsi, target-devel
  Cc: Mike Christie

tcm_loop could be used like a normal block device, so we can't use
GFP_KERNEL. This adds a gfp_t arg to target_cmd_init_cdb and covnerts
the users. For every driver but loop I kept GFP_KERNEL. For loop and
xcopy I switched to GFP_NOIO.

This will also be useful in the later patches where loop needs to
do target_submit_prep from interrupt context to get a ref to the
se_device, and so it will need to use GFP_ATOMIC.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c  |  3 ++-
 drivers/scsi/qla2xxx/tcm_qla2xxx.c     |  3 ++-
 drivers/target/iscsi/iscsi_target.c    |  3 ++-
 drivers/target/loopback/tcm_loop.c     |  3 ++-
 drivers/target/target_core_transport.c | 13 +++++++------
 drivers/target/target_core_xcopy.c     |  2 +-
 drivers/target/tcm_fc/tfc_cmd.c        |  2 +-
 drivers/vhost/scsi.c                   |  2 +-
 drivers/xen/xen-scsiback.c             |  2 +-
 include/target/target_core_fabric.h    |  5 +++--
 10 files changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 87741e0b4bca..51c386a215f5 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1537,7 +1537,8 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
 		goto busy;
 	}
 
-	if (target_submit_prep(cmd, srp_cmd->cdb, sg, sg_cnt, NULL, 0, NULL, 0))
+	if (target_submit_prep(cmd, srp_cmd->cdb, sg, sg_cnt, NULL, 0, NULL, 0,
+			       GFP_KERNEL))
 		return;
 
 	target_submit(cmd);
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 56394d901791..12a2265eb2de 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -492,7 +492,8 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha, struct qla_tgt_cmd *cmd,
 	if (rc)
 		return rc;
 
-	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0))
+	if (target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0,
+			       GFP_KERNEL))
 		return 0;
 
 	target_submit(se_cmd);
diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index f2107705f2ea..566adfde1661 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1166,7 +1166,8 @@ int iscsit_setup_scsi_cmd(struct iscsi_conn *conn, struct iscsi_cmd *cmd,
 
 	target_get_sess_cmd(&cmd->se_cmd, true);
 
-	cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb);
+	cmd->sense_reason = target_cmd_init_cdb(&cmd->se_cmd, hdr->cdb,
+						GFP_KERNEL);
 	if (cmd->sense_reason) {
 		if (cmd->sense_reason == TCM_OUT_OF_RESOURCES) {
 			return iscsit_add_reject_cmd(cmd,
diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 461f4125fcab..677e4b8f0642 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -156,7 +156,8 @@ static void tcm_loop_submission_work(struct work_struct *work)
 
 	if (target_submit_prep(se_cmd, sc->cmnd, scsi_sglist(sc),
 			       scsi_sg_count(sc), sgl_bidi, sgl_bidi_count,
-			       scsi_prot_sglist(sc), scsi_prot_sg_count(sc)))
+			       scsi_prot_sglist(sc), scsi_prot_sg_count(sc),
+			       GFP_NOIO))
 		return;
 
 	target_submit(se_cmd);
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index ad30a99a5cb2..4723cc6abf61 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1427,7 +1427,7 @@ transport_check_alloc_task_attr(struct se_cmd *cmd)
 }
 
 sense_reason_t
-target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
+target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb, gfp_t gfp)
 {
 	sense_reason_t ret;
 
@@ -1448,8 +1448,7 @@ target_cmd_init_cdb(struct se_cmd *cmd, unsigned char *cdb)
 	 * setup the pointer from __t_task_cdb to t_task_cdb.
 	 */
 	if (scsi_command_size(cdb) > sizeof(cmd->__t_task_cdb)) {
-		cmd->t_task_cdb = kzalloc(scsi_command_size(cdb),
-						GFP_KERNEL);
+		cmd->t_task_cdb = kzalloc(scsi_command_size(cdb), gfp);
 		if (!cmd->t_task_cdb) {
 			pr_err("Unable to allocate cmd->t_task_cdb"
 				" %u > sizeof(cmd->__t_task_cdb): %lu ops\n",
@@ -1638,6 +1637,7 @@ EXPORT_SYMBOL_GPL(target_init_cmd);
  * @sgl_bidi_count: scatterlist count for bidirectional READ mapping
  * @sgl_prot: struct scatterlist memory protection information
  * @sgl_prot_count: scatterlist count for protection information
+ * @gfp_t: gfp allocation type
  *
  * Returns:
  *	- less than zero to signal failure.
@@ -1648,11 +1648,12 @@ EXPORT_SYMBOL_GPL(target_init_cmd);
 int target_submit_prep(struct se_cmd *se_cmd, unsigned char *cdb,
 		       struct scatterlist *sgl, u32 sgl_count,
 		       struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
-		       struct scatterlist *sgl_prot, u32 sgl_prot_count)
+		       struct scatterlist *sgl_prot, u32 sgl_prot_count,
+		       gfp_t gfp)
 {
 	sense_reason_t rc;
 
-	rc = target_cmd_init_cdb(se_cmd, cdb);
+	rc = target_cmd_init_cdb(se_cmd, cdb, gfp);
 	if (rc)
 		goto send_cc_direct;
 
@@ -1782,7 +1783,7 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 {
 	target_init_cmd(se_cmd, se_sess, sense, unpacked_lun, data_length,
 			task_attr, data_dir, flags);
-	target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0);
+	target_submit_prep(se_cmd, cdb, NULL, 0, NULL, 0, NULL, 0, GFP_KERNEL);
 	target_submit(se_cmd);
 }
 EXPORT_SYMBOL(target_submit_cmd);
diff --git a/drivers/target/target_core_xcopy.c b/drivers/target/target_core_xcopy.c
index e86cc6135587..d31ed071cb08 100644
--- a/drivers/target/target_core_xcopy.c
+++ b/drivers/target/target_core_xcopy.c
@@ -554,7 +554,7 @@ static int target_xcopy_setup_pt_cmd(
 	}
 	cmd->se_cmd_flags |= SCF_SE_LUN_CMD;
 
-	if (target_cmd_init_cdb(cmd, cdb))
+	if (target_cmd_init_cdb(cmd, cdb, GFP_KERNEL))
 		return -EINVAL;
 
 	cmd->tag = 0;
diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index 1376501ee3d0..410b723f9d79 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -555,7 +555,7 @@ static void ft_send_work(struct work_struct *work)
 		goto err;
 
 	if (target_submit_prep(&cmd->se_cmd, fcp->fc_cdb, NULL, 0, NULL, 0,
-			       NULL, 0))
+			       NULL, 0, GFP_KERNEL))
 		return;
 
 	target_submit(&cmd->se_cmd);
diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 76508d408bb3..93f5631b469c 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -811,7 +811,7 @@ static void vhost_scsi_submission_work(struct work_struct *work)
 
 	if (target_submit_prep(se_cmd, cmd->tvc_cdb, sg_ptr,
 			       cmd->tvc_sgl_count, NULL, 0, sg_prot_ptr,
-			       cmd->tvc_prot_sgl_count))
+			       cmd->tvc_prot_sgl_count, GFP_KERNEL))
 		return;
 
 	target_submit(se_cmd);
diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c
index 7bf9a6bede6d..eb3d8e35cbcd 100644
--- a/drivers/xen/xen-scsiback.c
+++ b/drivers/xen/xen-scsiback.c
@@ -368,7 +368,7 @@ static void scsiback_cmd_exec(struct vscsibk_pend *pending_req)
 			pending_req->sc_data_direction, TARGET_SCF_ACK_KREF);
 
 	if (target_submit_prep(se_cmd, pending_req->cmnd, pending_req->sgl,
-			       pending_req->n_sg, NULL, 0, NULL, 0))
+			       pending_req->n_sg, NULL, 0, NULL, 0, GFP_KERNEL))
 		return;
 
 	target_submit(se_cmd);
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 86b0d4a7df92..0543ab107723 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -157,10 +157,11 @@ int	target_init_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
 int	target_submit_prep(struct se_cmd *se_cmd, unsigned char *cdb,
 		struct scatterlist *sgl, u32 sgl_count,
 		struct scatterlist *sgl_bidi, u32 sgl_bidi_count,
-		struct scatterlist *sgl_prot, u32 sgl_prot_count);
+		struct scatterlist *sgl_prot, u32 sgl_prot_count, gfp_t gfp);
 void	target_submit(struct se_cmd *se_cmd);
 sense_reason_t transport_lookup_cmd_lun(struct se_cmd *);
-sense_reason_t target_cmd_init_cdb(struct se_cmd *, unsigned char *);
+sense_reason_t target_cmd_init_cdb(struct se_cmd *se_cmd, unsigned char *cdb,
+				   gfp_t gfp);
 sense_reason_t target_cmd_parse_cdb(struct se_cmd *);
 void	target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
 		unsigned char *, u64, u32, int, int, int);
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 39+ messages in thread

end of thread, other threads:[~2021-02-27 17:11 UTC | newest]

Thread overview: 39+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-17 20:27 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
2021-02-17 20:27 ` [PATCH 01/25] target: move t_task_cdb initialization Mike Christie
2021-02-17 20:27 ` [PATCH 02/25] target: drop kref_get_unless_zero in target_get_sess_cmd Mike Christie
2021-02-17 20:27 ` [PATCH 03/25] target: rename transport_init_se_cmd Mike Christie
2021-02-17 20:27 ` [PATCH 04/25] target: break up target_submit_cmd_map_sgls Mike Christie
2021-02-17 20:27 ` [PATCH 05/25] srpt: Convert to new submission API Mike Christie
2021-02-19  0:22   ` Bart Van Assche
2021-02-17 20:27 ` [PATCH 06/25] ibmvscsi_tgt: " Mike Christie
2021-02-17 20:27 ` [PATCH 07/25] qla2xxx: " Mike Christie
2021-02-17 20:27 ` [PATCH 08/25] tcm_loop: " Mike Christie
2021-02-17 20:27 ` [PATCH 09/25] sbp_target: " Mike Christie
2021-02-17 20:27 ` [PATCH 10/25] usb gadget: " Mike Christie
2021-02-17 20:27 ` [PATCH 11/25] vhost-scsi: " Mike Christie
2021-02-17 20:27 ` [PATCH 12/25] xen-scsiback: " Mike Christie
2021-02-17 20:27 ` [PATCH 13/25] tcm_fc: " Mike Christie
2021-02-17 20:28 ` [PATCH 14/25] target: remove target_submit_cmd_map_sgls Mike Christie
2021-02-19 19:01   ` Bodo Stroesser
2021-02-17 20:28 ` [PATCH 15/25] target: add gfp_t arg to target_cmd_init_cdb Mike Christie
2021-02-19 18:53   ` Bodo Stroesser
2021-02-19 19:32   ` Bodo Stroesser
2021-02-19 19:41     ` michael.christie
2021-02-17 20:28 ` [PATCH 16/25] target: add workqueue based cmd submission Mike Christie
2021-02-19 19:10   ` Bodo Stroesser
2021-02-17 20:28 ` [PATCH 17/25] vhost scsi: use lio wq cmd submission helper Mike Christie
2021-02-17 20:28 ` [PATCH 18/25] tcm loop: use blk cmd allocator for se_cmds Mike Christie
2021-02-17 20:28 ` [PATCH 19/25] tcm loop: use lio wq cmd submission helper Mike Christie
2021-02-19 19:38   ` Bodo Stroesser
2021-02-17 20:28 ` [PATCH 20/25] target: cleanup cmd flag bits Mike Christie
2021-02-17 20:28 ` [PATCH 21/25] target: fix backend plugging Mike Christie
2021-02-19 19:40   ` Bodo Stroesser
2021-02-17 20:28 ` [PATCH 22/25] target iblock: add backend plug/unplug callouts Mike Christie
2021-02-17 20:28 ` [PATCH 23/25] target_core_user: " Mike Christie
2021-02-19 19:45   ` Bodo Stroesser
2021-02-17 20:28 ` [PATCH 24/25] target: flush submission work during TMR processing Mike Christie
2021-02-19 19:46   ` Bodo Stroesser
2021-02-17 20:28 ` [PATCH 25/25] target: make completion affinity configurable Mike Christie
  -- strict thread matches above, loose matches on Subject: below --
2021-02-27 16:59 [PATCH 00/25 V5] target: fix cmd plugging and submission Mike Christie
2021-02-27 16:59 ` [PATCH 15/25] target: add gfp_t arg to target_cmd_init_cdb Mike Christie
2021-02-12  7:26 PATCH 00/25 V4] target: fix cmd plugging and submission Mike Christie
2021-02-12  7:26 ` [PATCH 15/25] target: add gfp_t arg to target_cmd_init_cdb Mike Christie
2021-02-16  8:40   ` Christoph Hellwig

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.