[PATCH 0/9] target: Optimizations + cleanups for v3.11

[PATCH 0/9] target: Optimizations + cleanups for v3.11

[Nicholas A. Bellinger]

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

Hi folks,

This series contains a number of target optimizations added during
recent prototype scsi-mq performance profiling in order to avoid
unnecessary se_cmd->t_state_lock acquire/release in fast path I/O
submission and completion code.

All of the target optimizations are independent of driver code, so
all fabrics should benefit from these improvements.

Also included are two vhost/scsi specific patches, one in the same
vein to avoid unnecessary se_cmd->t_state_lock access, and the second
to convert vhost/scsi to proper cmd_kref TARGET_SCF_ACK_KREF usage.

Please have a look.

Thanks,

--nab

Nicholas Bellinger (9):
  target: Add transport_cmd_check_stop write_pending bit
  target: Drop unnecessary CMD_T_DEV_ACTIVE check from
    transport_lun_remove_cmd
  target: Remove legacy t_fe_count + avoid t_state_lock access in
    transport_put_cmd
  target: Avoid extra t_state_lock access in __target_execute_cmd
  target: Drop unnecessary t_state_lock access for
    SCF_SUPPORTED_SAM_OPCODE assignment
  iscsi-target: Avoid unnecessary t_state_lock during unsolicited
    data-out check
  target: Drop legacy se_cmd->check_release bit
  vhost/scsi: Drop unnecessary wait_for_tasks=true usage with
    transport_generic_free_cmd
  vhost/scsi: Convert to se_cmd->cmd_kref TARGET_SCF_ACK_KREF usage

 drivers/target/iscsi/iscsi_target.c    |    6 ---
 drivers/target/target_core_tmr.c       |   12 +----
 drivers/target/target_core_transport.c |   72 ++++++++-----------------------
 drivers/vhost/scsi.c                   |   33 +++++++++------
 include/target/target_core_base.h      |    3 -
 5 files changed, 41 insertions(+), 85 deletions(-)

-- 
1.7.2.5

[PATCH 1/9] target: Add transport_cmd_check_stop write_pending bit

[Nicholas A. Bellinger]

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

This patch adds a new transport_cmd_check_stop() parameter for signaling
when TRANSPORT_WRITE_PENDING needs to be set.

This allows transport_generic_new_cmd() to avoid the extra lock acquire/release
of ->t_state_lock in the fast path for DMA_TO_DEVICE operations ahead of
transport_cmd_check_stop() + se_tfo->write_pending().

Cc: Christoph Hellwig <hch@lst.de>
Cc: Roland Dreier <roland@kernel.org>
Cc: Kent Overstreet <koverstreet@google.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Moussa Ba <moussaba@micron.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c |   19 +++++++++----------
 1 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 4a79336..15afa83 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -445,11 +445,15 @@ static void target_remove_from_state_list(struct se_cmd *cmd)
 	spin_unlock_irqrestore(&dev->execute_task_lock, flags);
 }
 
-static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists)
+static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists,
+				    bool write_pending)
 {
 	unsigned long flags;
 
 	spin_lock_irqsave(&cmd->t_state_lock, flags);
+	if (write_pending)
+		cmd->t_state = TRANSPORT_WRITE_PENDING;
+
 	/*
 	 * Determine if IOCTL context caller in requesting the stopping of this
 	 * command for LUN shutdown purposes.
@@ -514,7 +518,7 @@ static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists)
 
 static int transport_cmd_check_stop_to_fabric(struct se_cmd *cmd)
 {
-	return transport_cmd_check_stop(cmd, true);
+	return transport_cmd_check_stop(cmd, true, false);
 }
 
 static void transport_lun_remove_cmd(struct se_cmd *cmd)
@@ -2116,12 +2120,7 @@ transport_generic_new_cmd(struct se_cmd *cmd)
 		target_execute_cmd(cmd);
 		return 0;
 	}
-
-	spin_lock_irq(&cmd->t_state_lock);
-	cmd->t_state = TRANSPORT_WRITE_PENDING;
-	spin_unlock_irq(&cmd->t_state_lock);
-
-	transport_cmd_check_stop(cmd, false);
+	transport_cmd_check_stop(cmd, false, true);
 
 	ret = cmd->se_tfo->write_pending(cmd);
 	if (ret == -EAGAIN || ret == -ENOMEM)
@@ -2325,7 +2324,7 @@ static int transport_lun_wait_for_tasks(struct se_cmd *cmd, struct se_lun *lun)
 		pr_debug("ConfigFS ITT[0x%08x] - CMD_T_STOP, skipping\n",
 			 cmd->se_tfo->get_task_tag(cmd));
 		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-		transport_cmd_check_stop(cmd, false);
+		transport_cmd_check_stop(cmd, false, false);
 		return -EPERM;
 	}
 	cmd->transport_state |= CMD_T_LUN_FE_STOP;
@@ -2433,7 +2432,7 @@ check_cond:
 
 			spin_unlock_irqrestore(&cmd->t_state_lock,
 					cmd_flags);
-			transport_cmd_check_stop(cmd, false);
+			transport_cmd_check_stop(cmd, false, false);
 			complete(&cmd->transport_lun_fe_stop_comp);
 			spin_lock_irqsave(&lun->lun_cmd_lock, lun_flags);
 			continue;
-- 
1.7.2.5

[PATCH 2/9] target: Drop unnecessary CMD_T_DEV_ACTIVE check from transport_lun_remove_cmd

[Nicholas A. Bellinger]

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

This patch drops an unnecessary acquire/release of se_cmd->t_state_lock within
transport_lun_remove_cmd() when checking CMD_T_DEV_ACTIVE for invoking
target_remove_from_state_list().

For all fast path completion cases, transport_lun_remove_cmd() is always
called ahead of transport_cmd_check_stop(), and since transport_cmd_check_stop()
is calling target_remove_from_state_list() when remove_from_lists=true,
the t_state_lock usage in transport_lun_remove_cmd() can safely be removed.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Roland Dreier <roland@kernel.org>
Cc: Kent Overstreet <koverstreet@google.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Moussa Ba <moussaba@micron.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 15afa83..d062878 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -529,13 +529,6 @@ static void transport_lun_remove_cmd(struct se_cmd *cmd)
 	if (!lun)
 		return;
 
-	spin_lock_irqsave(&cmd->t_state_lock, flags);
-	if (cmd->transport_state & CMD_T_DEV_ACTIVE) {
-		cmd->transport_state &= ~CMD_T_DEV_ACTIVE;
-		target_remove_from_state_list(cmd);
-	}
-	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-
 	spin_lock_irqsave(&lun->lun_cmd_lock, flags);
 	if (!list_empty(&cmd->se_lun_node))
 		list_del_init(&cmd->se_lun_node);
-- 
1.7.2.5

[PATCH 3/9] target: Remove legacy t_fe_count + avoid t_state_lock access in transport_put_cmd

[Nicholas A. Bellinger]

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

This patch removes legacy se_cmd->t_fe_count usage in order to avoid
se_cmd->t_state_lock access within transport_put_cmd() during normal
fast path se_cmd descriptor release.

Also drop the left-over parameter usage within core_tmr_handle_tas_abort()

Cc: Christoph Hellwig <hch@lst.de>
Cc: Roland Dreier <roland@kernel.org>
Cc: Kent Overstreet <koverstreet@google.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Moussa Ba <moussaba@micron.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_tmr.c       |   12 ++----------
 drivers/target/target_core_transport.c |   19 -------------------
 include/target/target_core_base.h      |    1 -
 3 files changed, 2 insertions(+), 30 deletions(-)

diff --git a/drivers/target/target_core_tmr.c b/drivers/target/target_core_tmr.c
index d0b4dd9..0d7cacb 100644
--- a/drivers/target/target_core_tmr.c
+++ b/drivers/target/target_core_tmr.c
@@ -85,13 +85,8 @@ void core_tmr_release_req(
 static void core_tmr_handle_tas_abort(
 	struct se_node_acl *tmr_nacl,
 	struct se_cmd *cmd,
-	int tas,
-	int fe_count)
+	int tas)
 {
-	if (!fe_count) {
-		transport_cmd_finish_abort(cmd, 1);
-		return;
-	}
 	/*
 	 * TASK ABORTED status (TAS) bit support
 	*/
@@ -253,7 +248,6 @@ static void core_tmr_drain_state_list(
 	LIST_HEAD(drain_task_list);
 	struct se_cmd *cmd, *next;
 	unsigned long flags;
-	int fe_count;
 
 	/*
 	 * Complete outstanding commands with TASK_ABORTED SAM status.
@@ -329,12 +323,10 @@ static void core_tmr_drain_state_list(
 		spin_lock_irqsave(&cmd->t_state_lock, flags);
 		target_stop_cmd(cmd, &flags);
 
-		fe_count = atomic_read(&cmd->t_fe_count);
-
 		cmd->transport_state |= CMD_T_ABORTED;
 		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 
-		core_tmr_handle_tas_abort(tmr_nacl, cmd, tas, fe_count);
+		core_tmr_handle_tas_abort(tmr_nacl, cmd, tas);
 	}
 }
 
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index d062878..daa7aa8 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1967,24 +1967,8 @@ static void transport_release_cmd(struct se_cmd *cmd)
  */
 static void transport_put_cmd(struct se_cmd *cmd)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&cmd->t_state_lock, flags);
-	if (atomic_read(&cmd->t_fe_count) &&
-	    !atomic_dec_and_test(&cmd->t_fe_count)) {
-		spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-		return;
-	}
-
-	if (cmd->transport_state & CMD_T_DEV_ACTIVE) {
-		cmd->transport_state &= ~CMD_T_DEV_ACTIVE;
-		target_remove_from_state_list(cmd);
-	}
-	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
-
 	transport_free_pages(cmd);
 	transport_release_cmd(cmd);
-	return;
 }
 
 void *transport_kmap_data_sg(struct se_cmd *cmd)
@@ -2100,9 +2084,6 @@ transport_generic_new_cmd(struct se_cmd *cmd)
 		if (ret < 0)
 			return TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
 	}
-
-	atomic_inc(&cmd->t_fe_count);
-
 	/*
 	 * If this command is not a write we can execute it right here,
 	 * for write buffers we need to notify the fabric driver first
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index e773dfa..3b337b9 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -458,7 +458,6 @@ struct se_cmd {
 	unsigned char		*t_task_cdb;
 	unsigned char		__t_task_cdb[TCM_MAX_COMMAND_SIZE];
 	unsigned long long	t_task_lba;
-	atomic_t		t_fe_count;
 	unsigned int		transport_state;
 #define CMD_T_ABORTED		(1 << 0)
 #define CMD_T_ACTIVE		(1 << 1)
-- 
1.7.2.5

[PATCH 4/9] target: Avoid extra t_state_lock access in __target_execute_cmd

[Nicholas A. Bellinger]

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

This patch makes target_execute_cmd() set CMD_T_BUSY|CMD_T_SENT while
holding se_cmd->t_state_lock, in order to avoid the extra aquire/release
in __target_execute_cmd().

It also clears these bits in case of a target_handle_task_attr()
failure.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Roland Dreier <roland@kernel.org>
Cc: Kent Overstreet <koverstreet@google.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Moussa Ba <moussaba@micron.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c |   16 +++++++++-------
 1 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index daa7aa8..f9e69f5 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1579,10 +1579,6 @@ static void __target_execute_cmd(struct se_cmd *cmd)
 {
 	sense_reason_t ret;
 
-	spin_lock_irq(&cmd->t_state_lock);
-	cmd->transport_state |= (CMD_T_BUSY|CMD_T_SENT);
-	spin_unlock_irq(&cmd->t_state_lock);
-
 	if (cmd->execute_cmd) {
 		ret = cmd->execute_cmd(cmd);
 		if (ret) {
@@ -1689,11 +1685,17 @@ void target_execute_cmd(struct se_cmd *cmd)
 	}
 
 	cmd->t_state = TRANSPORT_PROCESSING;
-	cmd->transport_state |= CMD_T_ACTIVE;
+	cmd->transport_state |= CMD_T_ACTIVE|CMD_T_BUSY|CMD_T_SENT;
 	spin_unlock_irq(&cmd->t_state_lock);
 
-	if (!target_handle_task_attr(cmd))
-		__target_execute_cmd(cmd);
+	if (target_handle_task_attr(cmd)) {
+		spin_lock_irq(&cmd->t_state_lock);
+		cmd->transport_state &= ~CMD_T_BUSY|CMD_T_SENT;
+		spin_unlock_irq(&cmd->t_state_lock);
+		return;
+	}
+
+	__target_execute_cmd(cmd);
 }
 EXPORT_SYMBOL(target_execute_cmd);
 
-- 
1.7.2.5

[PATCH 5/9] target: Drop unnecessary t_state_lock access for SCF_SUPPORTED_SAM_OPCODE assignment

[Nicholas A. Bellinger]

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

This patch drops the se_cmd->t_state_lock access around SCF_SUPPORTED_SAM_OPCODE
assignment within target_setup_cmd_from_cdb().

Original v4.0 target code required this as fabrics would be checking for
this values in different process contexts for setup and I/O submission.

Given that modern v4.1 target code performs setup and I/O submission
from the same process context, this t_state_lock access is no longer
required.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Roland Dreier <roland@kernel.org>
Cc: Kent Overstreet <koverstreet@google.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Moussa Ba <moussaba@micron.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index f9e69f5..77c0b29 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1088,7 +1088,6 @@ sense_reason_t
 target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb)
 {
 	struct se_device *dev = cmd->se_dev;
-	unsigned long flags;
 	sense_reason_t ret;
 
 	/*
@@ -1148,9 +1147,7 @@ target_setup_cmd_from_cdb(struct se_cmd *cmd, unsigned char *cdb)
 	if (ret)
 		return ret;
 
-	spin_lock_irqsave(&cmd->t_state_lock, flags);
 	cmd->se_cmd_flags |= SCF_SUPPORTED_SAM_OPCODE;
-	spin_unlock_irqrestore(&cmd->t_state_lock, flags);
 
 	spin_lock(&cmd->se_lun->lun_sep_lock);
 	if (cmd->se_lun->lun_sep)
-- 
1.7.2.5

[PATCH 6/9] iscsi-target: Avoid unnecessary t_state_lock during unsolicited data-out check

[Nicholas A. Bellinger]

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

In modern iscsi-target code, the setup and I/O submission is done within a
single process context, so there is no need to acquire se_cmd->t_state_lock while
checking SCF_SUPPORTED_SAM_OPCODE for determining when unsolicited data-out
should be dumped.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Roland Dreier <roland@kernel.org>
Cc: Kent Overstreet <koverstreet@google.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Moussa Ba <moussaba@micron.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/iscsi/iscsi_target.c |    6 ------
 1 files changed, 0 insertions(+), 6 deletions(-)

diff --git a/drivers/target/iscsi/iscsi_target.c b/drivers/target/iscsi/iscsi_target.c
index 262ef1f..24783ee 100644
--- a/drivers/target/iscsi/iscsi_target.c
+++ b/drivers/target/iscsi/iscsi_target.c
@@ -1277,7 +1277,6 @@ iscsit_check_dataout_hdr(struct iscsi_conn *conn, unsigned char *buf,
 	struct iscsi_data *hdr = (struct iscsi_data *)buf;
 	struct iscsi_cmd *cmd = NULL;
 	struct se_cmd *se_cmd;
-	unsigned long flags;
 	u32 payload_length = ntoh24(hdr->dlength);
 	int rc;
 
@@ -1356,14 +1355,9 @@ iscsit_check_dataout_hdr(struct iscsi_conn *conn, unsigned char *buf,
 		 */
 
 		/* Something's amiss if we're not in WRITE_PENDING state... */
-		spin_lock_irqsave(&se_cmd->t_state_lock, flags);
 		WARN_ON(se_cmd->t_state != TRANSPORT_WRITE_PENDING);
-		spin_unlock_irqrestore(&se_cmd->t_state_lock, flags);
-
-		spin_lock_irqsave(&se_cmd->t_state_lock, flags);
 		if (!(se_cmd->se_cmd_flags & SCF_SUPPORTED_SAM_OPCODE))
 			dump_unsolicited_data = 1;
-		spin_unlock_irqrestore(&se_cmd->t_state_lock, flags);
 
 		if (dump_unsolicited_data) {
 			/*
-- 
1.7.2.5

[PATCH 7/9] target: Drop legacy se_cmd->check_release bit

[Nicholas A. Bellinger]

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

Now with iscsi-target using modern se_cmd->cmd_kref accounting in
v3.10 code, it's safe to go ahead and drop the legacy release
codepath + se_cmd->check_release bit in transport_release_cmd()

Cc: Christoph Hellwig <hch@lst.de>
Cc: Roland Dreier <roland@kernel.org>
Cc: Kent Overstreet <koverstreet@google.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Moussa Ba <moussaba@micron.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/target/target_core_transport.c |    8 +-------
 include/target/target_core_base.h      |    2 --
 2 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 77c0b29..abb7e40 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1951,11 +1951,7 @@ static void transport_release_cmd(struct se_cmd *cmd)
 	 * If this cmd has been setup with target_get_sess_cmd(), drop
 	 * the kref and call ->release_cmd() in kref callback.
 	 */
-	 if (cmd->check_release != 0) {
-		target_put_sess_cmd(cmd->se_sess, cmd);
-		return;
-	}
-	cmd->se_tfo->release_cmd(cmd);
+	target_put_sess_cmd(cmd->se_sess, cmd);
 }
 
 /**
@@ -2171,8 +2167,6 @@ int target_get_sess_cmd(struct se_session *se_sess, struct se_cmd *se_cmd,
 		goto out;
 	}
 	list_add_tail(&se_cmd->se_cmd_list, &se_sess->sess_cmd_list);
-	se_cmd->check_release = 1;
-
 out:
 	spin_unlock_irqrestore(&se_sess->sess_cmd_lock, flags);
 	return ret;
diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h
index 3b337b9..a5c97db 100644
--- a/include/target/target_core_base.h
+++ b/include/target/target_core_base.h
@@ -424,8 +424,6 @@ struct se_cmd {
 	int			sam_task_attr;
 	/* Transport protocol dependent state, see transport_state_table */
 	enum transport_state_table t_state;
-	/* Used to signal cmd->se_tfo->check_release_cmd() usage per cmd */
-	unsigned		check_release:1;
 	unsigned		cmd_wait_set:1;
 	unsigned		unknown_data_length:1;
 	/* See se_cmd_flags_table */
-- 
1.7.2.5

[PATCH 8/9] vhost/scsi: Drop unnecessary wait_for_tasks=true usage with transport_generic_free_cmd

[Nicholas A. Bellinger]

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

This patch changes vhost_scsi_free_cmd() to call transport_generic_free_cmd()
with wait_for_tasks=false in order to avoid the extra se_cmd->t_state_lock
access for the wait_for_tasks=true case.

This is unnecessary because vhost_scsi_free_cmd() is only ever called by
vhost_scsi_complete_cmd_work() after TCM completion handoff, and by
vhost_scsi_handle_vq() exception code before TCM submission handoff, so
there is never a case where se_cmd is still active from TCM's perspective
when transport_generic_free_cmd() is called.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Roland Dreier <roland@kernel.org>
Cc: Kent Overstreet <koverstreet@google.com>
Cc: Asias He <asias@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Moussa Ba <moussaba@micron.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/vhost/scsi.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index 7014202..aacf71e 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -557,7 +557,7 @@ static void vhost_scsi_free_cmd(struct tcm_vhost_cmd *tv_cmd)
 	struct se_cmd *se_cmd = &tv_cmd->tvc_se_cmd;
 
 	/* TODO locking against target/backend threads? */
-	transport_generic_free_cmd(se_cmd, 1);
+	transport_generic_free_cmd(se_cmd, 0);
 
 	if (tv_cmd->tvc_sgl_count) {
 		u32 i;
-- 
1.7.2.5

[PATCH 9/9] vhost/scsi: Convert to se_cmd->cmd_kref TARGET_SCF_ACK_KREF usage

[Nicholas A. Bellinger]

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

This patch coverts vhost/scsi to se_cmd->cmd_kref TARGET_SCF_ACK_KREF
usage, instead of assuming that vhost_scsi_free_cmd() is always called
before TCM processing is completed in the response fast path.

This includes adding vhost_scsi_check_stop_free() -> target_put_sess_cmd()
to perform the second se_cmd->cmd_kref put, and moving vhost_scsi_free_cmd()
resource release into tcm_vhost_release_cmd() that is invoked once the last
se_cmd->cmd_kref put occurs.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Roland Dreier <roland@kernel.org>
Cc: Kent Overstreet <koverstreet@google.com>
Cc: Asias He <asias@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Or Gerlitz <ogerlitz@mellanox.com>
Cc: Moussa Ba <moussaba@micron.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
 drivers/vhost/scsi.c |   31 +++++++++++++++++++------------
 1 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
index aacf71e..1e5e820 100644
--- a/drivers/vhost/scsi.c
+++ b/drivers/vhost/scsi.c
@@ -446,7 +446,19 @@ static u32 tcm_vhost_tpg_get_inst_index(struct se_portal_group *se_tpg)
 
 static void tcm_vhost_release_cmd(struct se_cmd *se_cmd)
 {
-	return;
+	struct tcm_vhost_cmd *tv_cmd = container_of(se_cmd,
+				struct tcm_vhost_cmd, tvc_se_cmd);
+
+	if (tv_cmd->tvc_sgl_count) {
+		u32 i;
+		for (i = 0; i < tv_cmd->tvc_sgl_count; i++)
+			put_page(sg_page(&tv_cmd->tvc_sgl[i]));
+
+		kfree(tv_cmd->tvc_sgl);
+        }
+
+	tcm_vhost_put_inflight(tv_cmd->inflight);
+	kfree(tv_cmd);
 }
 
 static int tcm_vhost_shutdown_session(struct se_session *se_sess)
@@ -559,17 +571,11 @@ static void vhost_scsi_free_cmd(struct tcm_vhost_cmd *tv_cmd)
 	/* TODO locking against target/backend threads? */
 	transport_generic_free_cmd(se_cmd, 0);
 
-	if (tv_cmd->tvc_sgl_count) {
-		u32 i;
-		for (i = 0; i < tv_cmd->tvc_sgl_count; i++)
-			put_page(sg_page(&tv_cmd->tvc_sgl[i]));
-
-		kfree(tv_cmd->tvc_sgl);
-	}
-
-	tcm_vhost_put_inflight(tv_cmd->inflight);
+}
 
-	kfree(tv_cmd);
+static int vhost_scsi_check_stop_free(struct se_cmd *se_cmd)
+{
+	return target_put_sess_cmd(se_cmd->se_sess, se_cmd);
 }
 
 static void tcm_vhost_do_evt_work(struct vhost_scsi *vs,
@@ -847,7 +853,7 @@ static void tcm_vhost_submission_work(struct work_struct *work)
 			tv_cmd->tvc_cdb, &tv_cmd->tvc_sense_buf[0],
 			tv_cmd->tvc_lun, tv_cmd->tvc_exp_data_len,
 			tv_cmd->tvc_task_attr, tv_cmd->tvc_data_direction,
-			0, sg_ptr, tv_cmd->tvc_sgl_count,
+			TARGET_SCF_ACK_KREF, sg_ptr, tv_cmd->tvc_sgl_count,
 			sg_bidi_ptr, sg_no_bidi);
 	if (rc < 0) {
 		transport_send_check_condition_and_sense(se_cmd,
@@ -2008,6 +2014,7 @@ static struct target_core_fabric_ops tcm_vhost_ops = {
 	.tpg_release_fabric_acl		= tcm_vhost_release_fabric_acl,
 	.tpg_get_inst_index		= tcm_vhost_tpg_get_inst_index,
 	.release_cmd			= tcm_vhost_release_cmd,
+	.check_stop_free		= vhost_scsi_check_stop_free,
 	.shutdown_session		= tcm_vhost_shutdown_session,
 	.close_session			= tcm_vhost_close_session,
 	.sess_get_index			= tcm_vhost_sess_get_index,
-- 
1.7.2.5

Re: [PATCH 1/9] target: Add transport_cmd_check_stop write_pending bit

[Jörn Engel]

On Fri, 7 June 2013 21:34:16 +0000, Nicholas A. Bellinger wrote:
>  
> -static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists)
> +static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists,
> +				    bool write_pending)
...

> -	return transport_cmd_check_stop(cmd, true);
> +	return transport_cmd_check_stop(cmd, true, false);

At this point I would prefer to pass in a flags.
transport_cmd_check_stop(cmd, SC_REMOVE_FROM_LISTS) seems more
readable to me.

The rest of the patchset I rather like.  Well done.

Jörn

--
When you close your hand, you own nothing. When you open it up, you
own the whole world.
-- Li Mu Bai in Tiger & Dragon
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/9] target: Add transport_cmd_check_stop write_pending bit

[Christoph Hellwig]

On Sat, Jun 08, 2013 at 12:00:36AM -0400, Jörn Engel wrote:
> > -static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists)
> > +static int transport_cmd_check_stop(struct se_cmd *cmd, bool remove_from_lists,
> > +				    bool write_pending)
> ...
> 
> > -	return transport_cmd_check_stop(cmd, true);
> > +	return transport_cmd_check_stop(cmd, true, false);
> 
> At this point I would prefer to pass in a flags.
> transport_cmd_check_stop(cmd, SC_REMOVE_FROM_LISTS) seems more
> readable to me.

The whole area needs a major cleanup.  The list removal is mostly unrelated
to the rest of the function, so it really should be split out.

Tje all to target_remove_from_state_list actually happens unconditionaly,
just that in the CMD_T_LUN_STOP case it is done after clearing CMD_T_ACTIVE.

I can't see a reason for that delay, so unless proven otherwise the call
to it should be lifted from transport_cmd_check_stop to
transport_cmd_check_stop_to_fabric.  The same probably applies to the clearing
of cmd->se_lun, and the call to ->check_stop_free can already be done in
the caller just based on the return value from transport_cmd_check_stop.

Then we can replace the irqsave locking with just _irq locking because 
static int transport_cmd_check_stop should never be called with irqs
disabled and finally add a variant that has the lock already taken
instead of adding the new flag.

Longer term down the road we should get rid of the _irq locking in the target
core entirely.  As we moved all major work to workqueues a while ago nothing
should be nessecary although a small audit is needed first.

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/9] target: Avoid extra t_state_lock access in __target_execute_cmd

[Christoph Hellwig]

On Fri, Jun 07, 2013 at 09:34:19PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch makes target_execute_cmd() set CMD_T_BUSY|CMD_T_SENT while
> holding se_cmd->t_state_lock, in order to avoid the extra aquire/release
> in __target_execute_cmd().

Can you also do an audit that we actually still need all these flags?
I'm pretty sure there's a fair amount of duplication between all the t_state
and transport_state flags that could be rationalized.

Re: [PATCH 8/9] vhost/scsi: Drop unnecessary wait_for_tasks=true usage with transport_generic_free_cmd

[Asias He]

On Fri, Jun 07, 2013 at 09:34:23PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch changes vhost_scsi_free_cmd() to call transport_generic_free_cmd()
> with wait_for_tasks=false in order to avoid the extra se_cmd->t_state_lock
> access for the wait_for_tasks=true case.
> 
> This is unnecessary because vhost_scsi_free_cmd() is only ever called by
> vhost_scsi_complete_cmd_work() after TCM completion handoff, and by
> vhost_scsi_handle_vq() exception code before TCM submission handoff, so
> there is never a case where se_cmd is still active from TCM's perspective
> when transport_generic_free_cmd() is called.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Roland Dreier <roland@kernel.org>
> Cc: Kent Overstreet <koverstreet@google.com>
> Cc: Asias He <asias@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Or Gerlitz <ogerlitz@mellanox.com>
> Cc: Moussa Ba <moussaba@micron.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>

Acked-by: Asias He <asias@redhat.com>

> ---
>  drivers/vhost/scsi.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index 7014202..aacf71e 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -557,7 +557,7 @@ static void vhost_scsi_free_cmd(struct tcm_vhost_cmd *tv_cmd)
>  	struct se_cmd *se_cmd = &tv_cmd->tvc_se_cmd;
>  
>  	/* TODO locking against target/backend threads? */
> -	transport_generic_free_cmd(se_cmd, 1);
> +	transport_generic_free_cmd(se_cmd, 0);
>  
>  	if (tv_cmd->tvc_sgl_count) {
>  		u32 i;
> -- 
> 1.7.2.5
> 

-- 
Asias

Re: [PATCH 9/9] vhost/scsi: Convert to se_cmd->cmd_kref TARGET_SCF_ACK_KREF usage

[Asias He]

On Fri, Jun 07, 2013 at 09:34:24PM +0000, Nicholas A. Bellinger wrote:
> From: Nicholas Bellinger <nab@linux-iscsi.org>
> 
> This patch coverts vhost/scsi to se_cmd->cmd_kref TARGET_SCF_ACK_KREF
> usage, instead of assuming that vhost_scsi_free_cmd() is always called
> before TCM processing is completed in the response fast path.
> 
> This includes adding vhost_scsi_check_stop_free() -> target_put_sess_cmd()
> to perform the second se_cmd->cmd_kref put, and moving vhost_scsi_free_cmd()
> resource release into tcm_vhost_release_cmd() that is invoked once the last
> se_cmd->cmd_kref put occurs.
> 
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Roland Dreier <roland@kernel.org>
> Cc: Kent Overstreet <koverstreet@google.com>
> Cc: Asias He <asias@redhat.com>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Or Gerlitz <ogerlitz@mellanox.com>
> Cc: Moussa Ba <moussaba@micron.com>
> Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>

Acked-by: Asias He <asias@redhat.com>

> ---
>  drivers/vhost/scsi.c |   31 +++++++++++++++++++------------
>  1 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/vhost/scsi.c b/drivers/vhost/scsi.c
> index aacf71e..1e5e820 100644
> --- a/drivers/vhost/scsi.c
> +++ b/drivers/vhost/scsi.c
> @@ -446,7 +446,19 @@ static u32 tcm_vhost_tpg_get_inst_index(struct se_portal_group *se_tpg)
>  
>  static void tcm_vhost_release_cmd(struct se_cmd *se_cmd)
>  {
> -	return;
> +	struct tcm_vhost_cmd *tv_cmd = container_of(se_cmd,
> +				struct tcm_vhost_cmd, tvc_se_cmd);
> +
> +	if (tv_cmd->tvc_sgl_count) {
> +		u32 i;
> +		for (i = 0; i < tv_cmd->tvc_sgl_count; i++)
> +			put_page(sg_page(&tv_cmd->tvc_sgl[i]));
> +
> +		kfree(tv_cmd->tvc_sgl);
> +        }
> +
> +	tcm_vhost_put_inflight(tv_cmd->inflight);
> +	kfree(tv_cmd);
>  }
>  
>  static int tcm_vhost_shutdown_session(struct se_session *se_sess)
> @@ -559,17 +571,11 @@ static void vhost_scsi_free_cmd(struct tcm_vhost_cmd *tv_cmd)
>  	/* TODO locking against target/backend threads? */
>  	transport_generic_free_cmd(se_cmd, 0);
>  
> -	if (tv_cmd->tvc_sgl_count) {
> -		u32 i;
> -		for (i = 0; i < tv_cmd->tvc_sgl_count; i++)
> -			put_page(sg_page(&tv_cmd->tvc_sgl[i]));
> -
> -		kfree(tv_cmd->tvc_sgl);
> -	}
> -
> -	tcm_vhost_put_inflight(tv_cmd->inflight);
> +}
>  
> -	kfree(tv_cmd);
> +static int vhost_scsi_check_stop_free(struct se_cmd *se_cmd)
> +{
> +	return target_put_sess_cmd(se_cmd->se_sess, se_cmd);
>  }
>  
>  static void tcm_vhost_do_evt_work(struct vhost_scsi *vs,
> @@ -847,7 +853,7 @@ static void tcm_vhost_submission_work(struct work_struct *work)
>  			tv_cmd->tvc_cdb, &tv_cmd->tvc_sense_buf[0],
>  			tv_cmd->tvc_lun, tv_cmd->tvc_exp_data_len,
>  			tv_cmd->tvc_task_attr, tv_cmd->tvc_data_direction,
> -			0, sg_ptr, tv_cmd->tvc_sgl_count,
> +			TARGET_SCF_ACK_KREF, sg_ptr, tv_cmd->tvc_sgl_count,
>  			sg_bidi_ptr, sg_no_bidi);
>  	if (rc < 0) {
>  		transport_send_check_condition_and_sense(se_cmd,
> @@ -2008,6 +2014,7 @@ static struct target_core_fabric_ops tcm_vhost_ops = {
>  	.tpg_release_fabric_acl		= tcm_vhost_release_fabric_acl,
>  	.tpg_get_inst_index		= tcm_vhost_tpg_get_inst_index,
>  	.release_cmd			= tcm_vhost_release_cmd,
> +	.check_stop_free		= vhost_scsi_check_stop_free,
>  	.shutdown_session		= tcm_vhost_shutdown_session,
>  	.close_session			= tcm_vhost_close_session,
>  	.sess_get_index			= tcm_vhost_sess_get_index,
> -- 
> 1.7.2.5
> 

-- 
Asias