Re: [PATCH 01/12] scsi_transport_srp: Introduce srp_wait_for_queuecommand()

[Bart Van Assche <bart.vanassche@sandisk.com>]

On 04/30/15 19:25, Christoph Hellwig wrote:
> On Thu, Apr 30, 2015 at 12:58:50PM +0200, Bart Van Assche wrote:
>> The only callers in upstream code of scsi_target_block() and
>> scsi_target_unblock() I am aware of are the FC, iSCSI and SRP transport
>> layers and the drivers that use these transport layers. As far as I can see
>> both functions are always called from thread context and without holding any
>> spinlocks. A possible alternative to what I proposed in my previous e-mail
>> could be to provide a new function that waits for active queuecommand()
>> calls and that has to be called explicitly.
> 
> A separate helper sounds fine for now, although I suspect we'll
> eventually migrate the call to it into scsi_target_block().

(+Jens)

How about the patch below (lightly tested so far) ?

[PATCH] Introduce scsi_wait_for_queuecommand()

Move the functionality for waiting until active queuecommand()
calls have finished from the SRP transport layer into the SCSI
core. Add support for blk-mq in scsi_wait_for_queuecommand().
Introduce a request_fn_active counter in struct blk_mq_hw_ctx
to make it possible to implement this functionality for blk-mq.
---
 block/blk-mq.c                    | 19 ++++++++--------
 drivers/scsi/scsi_lib.c           | 48 +++++++++++++++++++++++++++++++++++++++
 drivers/scsi/scsi_transport_srp.c | 24 +-------------------
 include/linux/blk-mq.h            |  1 +
 include/scsi/scsi_device.h        |  1 +
 5 files changed, 60 insertions(+), 33 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index ade8a2d..d0063e4 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -786,12 +786,11 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
 	 * If we have previous entries on our dispatch list, grab them
 	 * and stuff them at the front for more fair dispatch.
 	 */
-	if (!list_empty_careful(&hctx->dispatch)) {
-		spin_lock(&hctx->lock);
-		if (!list_empty(&hctx->dispatch))
-			list_splice_init(&hctx->dispatch, &rq_list);
-		spin_unlock(&hctx->lock);
-	}
+	spin_lock(&hctx->lock);
+	hctx->request_fn_active++;
+	if (!list_empty(&hctx->dispatch))
+		list_splice_init(&hctx->dispatch, &rq_list);
+	spin_unlock(&hctx->lock);
 
 	/*
 	 * Start off with dptr being NULL, so we start the first request
@@ -851,11 +850,11 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx)
 	 * Any items that need requeuing? Stuff them into hctx->dispatch,
 	 * that is where we will continue on next queue run.
 	 */
-	if (!list_empty(&rq_list)) {
-		spin_lock(&hctx->lock);
+	spin_lock(&hctx->lock);
+	if (!list_empty(&rq_list))
 		list_splice(&rq_list, &hctx->dispatch);
-		spin_unlock(&hctx->lock);
-	}
+	hctx->request_fn_active--;
+	spin_unlock(&hctx->lock);
 }
 
 /*
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index b1a2631..1466ef2 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -3073,6 +3073,54 @@ scsi_target_unblock(struct device *dev, enum scsi_device_state new_state)
 EXPORT_SYMBOL_GPL(scsi_target_unblock);
 
 /**
+ * scsi_request_fn_active() - number of ongoing scsi_request_fn() calls.
+ * @shost: SCSI host.
+ */
+static int scsi_request_fn_active(struct Scsi_Host *shost)
+{
+	struct scsi_device *sdev;
+	struct request_queue *q;
+	struct blk_mq_hw_ctx *hctx;
+	unsigned int i;
+	int request_fn_active = 0;
+
+	shost_for_each_device(sdev, shost) {
+		q = sdev->request_queue;
+
+		if (q->mq_ops) {
+			queue_for_each_hw_ctx(q, hctx, i) {
+				spin_lock(&hctx->lock);
+				request_fn_active += q->request_fn_active;
+				spin_unlock(&hctx->lock);
+			}
+		} else {
+			spin_lock_irq(q->queue_lock);
+			request_fn_active += q->request_fn_active;
+			spin_unlock_irq(q->queue_lock);
+		}
+	}
+
+	return request_fn_active;
+}
+
+/**
+ * scsi_wait_for_queuecommand() - wait until queuecommand() calls have finished
+ * @shost: SCSI host.
+ *
+ * Although functions like scsi_target_block() and scsi_target_unblock(shost,
+ * SDEV_TRANSPORT_OFFLINE) prevent new calls to the queuecommand() callback
+ * function these functions do not wait until ongoing queucommand() calls have
+ * finished. Hence this function that waits until any ongoing queucommand()
+ * calls have finished.
+ */
+void scsi_wait_for_queuecommand(struct Scsi_Host *shost)
+{
+	while (scsi_request_fn_active(shost))
+		msleep(20);
+}
+EXPORT_SYMBOL(scsi_wait_for_queuecommand);
+
+/**
  * scsi_kmap_atomic_sg - find and atomically map an sg-elemnt
  * @sgl:	scatter-gather list
  * @sg_count:	number of segments in sg
diff --git a/drivers/scsi/scsi_transport_srp.c b/drivers/scsi/scsi_transport_srp.c
index ae45bd9..aaf4581 100644
--- a/drivers/scsi/scsi_transport_srp.c
+++ b/drivers/scsi/scsi_transport_srp.c
@@ -504,27 +504,6 @@ void srp_start_tl_fail_timers(struct srp_rport *rport)
 EXPORT_SYMBOL(srp_start_tl_fail_timers);
 
 /**
- * scsi_request_fn_active() - number of kernel threads inside scsi_request_fn()
- * @shost: SCSI host for which to count the number of scsi_request_fn() callers.
- */
-static int scsi_request_fn_active(struct Scsi_Host *shost)
-{
-	struct scsi_device *sdev;
-	struct request_queue *q;
-	int request_fn_active = 0;
-
-	shost_for_each_device(sdev, shost) {
-		q = sdev->request_queue;
-
-		spin_lock_irq(q->queue_lock);
-		request_fn_active += q->request_fn_active;
-		spin_unlock_irq(q->queue_lock);
-	}
-
-	return request_fn_active;
-}
-
-/**
  * srp_reconnect_rport() - reconnect to an SRP target port
  * @rport: SRP target port.
  *
@@ -559,8 +538,7 @@ int srp_reconnect_rport(struct srp_rport *rport)
 	if (res)
 		goto out;
 	scsi_target_block(&shost->shost_gendev);
-	while (scsi_request_fn_active(shost))
-		msleep(20);
+	scsi_wait_for_queuecommand(shost);
 	res = rport->state != SRP_RPORT_LOST ? i->f->reconnect(rport) : -ENODEV;
 	pr_debug("%s (state %d): transport.reconnect() returned %d\n",
 		 dev_name(&shost->shost_gendev), rport->state, res);
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 2056a99..732b746 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -22,6 +22,7 @@ struct blk_mq_hw_ctx {
 	struct {
 		spinlock_t		lock;
 		struct list_head	dispatch;
+		int			request_fn_active;
 	} ____cacheline_aligned_in_smp;
 
 	unsigned long		state;		/* BLK_MQ_S_* flags */
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index a4c9336..5540d02 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -412,6 +412,7 @@ extern void scsi_scan_target(struct device *parent, unsigned int channel,
 extern void scsi_target_reap(struct scsi_target *);
 extern void scsi_target_block(struct device *);
 extern void scsi_target_unblock(struct device *, enum scsi_device_state);
+extern void scsi_wait_for_queuecommand(struct Scsi_Host *shost);
 extern void scsi_remove_target(struct device *);
 extern void int_to_scsilun(u64, struct scsi_lun *);
 extern u64 scsilun_to_int(struct scsi_lun *);
-- 
2.1.4