[03/19] target: Avoid that aborting a command sporadically hangs

Commit Message

Bart Van Assche May 11, 2017, 12:23 a.m. UTC

On Fri, 2017-05-05 at 10:53 +0200, Christoph Hellwig wrote:
> On Thu, May 04, 2017 at 03:50:46PM -0700, Bart Van Assche wrote:
> > For several target drivers (e.g. ib_srpt and ib_isert) sleeping inside
> > transport_generic_free_cmd() causes RDMA completion processing to stall.
> > Hence only sleep inside this function if the (iSCSI) target driver
> > requires this.
> 
> This looks reasonable to me:
> 
> Reviewed-by: Christoph Hellwig <[e-mail]>
> 
> But as a further step can we try to move the waiting behavior entirely
> into the caller that actually cares,
> 
> e.g. move the conditional target_wait_free_cmd before the call
> to transport_generic_free_cmd in transport_generic_free_cmd,
> and move this second wait_for_completion after the
> transport_generic_free_cmd call based on an indicator (return value
> or se_cmd flag)?

Hello Christoph,

I have started working on eliminating the waiting code from
transport_generic_free_cmd(). I'm still working on a patch for the iSCSI
target driver. What I came up so far for the loop and Xen scsiback drivers
is attached to this e-mail.

Bart.

Patch

From 4c961dc45bb5d35cdefa8f0f94ebd8453a9d6cc4 Mon Sep 17 00:00:00 2001
From: Bart Van Assche <[e-mail]>
Date: Wed, 10 May 2017 15:01:38 -0700
Subject: [PATCH 5/5] target/tcm_loop: Make TMF processing slightly faster

Target drivers must guarantee that struct se_cmd and struct se_tmr_req
exist as long as target_tmr_work() is in progress. This is why the
tcm_loop driver today passes 1 as second argument to
transport_generic_free_cmd() from inside the TMF code. Instead of
making the TMF code wait, make the TMF code obtain two references
(SCF_ACK_KREF) and drop one reference from inside the .check_stop_free()
callback.

Signed-off-by: Bart Van Assche <[e-mail]>
Cc: Christoph Hellwig <[e-mail]>
Cc: Hannes Reinecke <[e-mail]>
Cc: David Disseldorp <[e-mail]>
---
 drivers/target/loopback/tcm_loop.c | 27 +++++++++++----------------
 1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/target/loopback/tcm_loop.c b/drivers/target/loopback/tcm_loop.c
index 48d751c53104..136d70285f9a 100644
--- a/drivers/target/loopback/tcm_loop.c
+++ b/drivers/target/loopback/tcm_loop.c
@@ -51,27 +51,18 @@  static int tcm_loop_queue_status(struct se_cmd *se_cmd);
  */
 static int tcm_loop_check_stop_free(struct se_cmd *se_cmd)
 {
-	/*
-	 * Do not release struct se_cmd's containing a valid TMR
-	 * pointer.  These will be released directly in tcm_loop_device_reset()
-	 * with transport_generic_free_cmd().
-	 */
-	if (se_cmd->se_cmd_flags & SCF_SCSI_TMR_CDB)
-		return 0;
-	/*
-	 * Release the struct se_cmd, which will make a callback to release
-	 * struct tcm_loop_cmd * in tcm_loop_deallocate_core_cmd()
-	 */
-	transport_generic_free_cmd(se_cmd, 0);
-	return 1;
+	return transport_generic_free_cmd(se_cmd, 0);
 }
 
 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 se_tmr_req *se_tmr = se_cmd->se_tmr_req;
+	struct tcm_loop_tmr *tl_tmr = se_tmr ? se_tmr->fabric_tmr_ptr : NULL;
 
 	kmem_cache_free(tcm_loop_cmd_cache, tl_cmd);
+	kfree(tl_tmr);
 }
 
 static int tcm_loop_show_info(struct seq_file *m, struct Scsi_Host *host)
@@ -251,19 +242,23 @@  static int tcm_loop_issue_tmr(struct tcm_loop_tpg *tl_tpg,
 
 	rc = target_submit_tmr(se_cmd, se_sess, NULL, 0 /* unpacked_lun */,
 			       tl_tmr, tmr, GFP_KERNEL, TCM_SIMPLE_TAG,
-			       0 /*flags*/);
+			       TARGET_SCF_ACK_KREF);
 	if (rc < 0)
 		goto release;
 	wait_for_completion(&tl_tmr->done);
 	ret = se_cmd->se_tmr_req->response;
+	target_put_sess_cmd(se_cmd);
+
+out:
+	return ret;
 
 release:
 	if (se_cmd)
-		transport_generic_free_cmd(se_cmd, 1);
+		transport_generic_free_cmd(se_cmd, 0);
 	else
 		kmem_cache_free(tcm_loop_cmd_cache, tl_cmd);
 	kfree(tl_tmr);
-	return ret;
+	goto out;
 }
 
 static int tcm_loop_abort_task(struct scsi_cmnd *sc)
-- 
2.12.2