linux-block.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] Avoid that scsi-mq queue processing stalls
@ 2017-04-03 23:22 Bart Van Assche
  2017-04-03 23:22 ` [PATCH v2 1/5] blk-mq: Export blk_mq_sched_restart_hctx() Bart Van Assche
                   ` (4 more replies)
  0 siblings, 5 replies; 17+ messages in thread
From: Bart Van Assche @ 2017-04-03 23:22 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, Martin K . Petersen, James Bottomley, Bart Van Assche

Hello Jens,

The five patches in this patch series fix the queue lockup I reported
last week on the linux-block mailing list. Please consider these patches
for kernel v4.11.

Thanks,

Bart.

Changes compared to v1 of this patch:
- Reworked scsi_restart_queues() such that it no longer takes the SCSI
  host lock.
- Added two patches - one for exporting blk_mq_sched_restart_hctx() and
  another one to make iterating with RCU over blk_mq_tag_set.tag_list safe.

Bart Van Assche (5):
  block: Export blk_mq_sched_restart_hctx()
  block: Make it safe to use RCU to iterate over blk_mq_tag_set.tag_list
  blk-mq: Introduce blk_mq_ops.restart_hctx
  scsi: Add scsi_restart_queues()
  scsi: Ensure that scsi_run_queue() runs all hardware queues

 block/blk-mq-sched.c    | 22 ++++++++--------------
 block/blk-mq-sched.h    | 14 --------------
 block/blk-mq.c          |  6 ++++++
 drivers/scsi/scsi_lib.c | 20 +++++++++++++++++---
 include/linux/blk-mq.h  |  8 ++++++++
 include/linux/blkdev.h  |  1 -
 6 files changed, 39 insertions(+), 32 deletions(-)

-- 
2.12.0

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

* [PATCH v2 1/5] blk-mq: Export blk_mq_sched_restart_hctx()
  2017-04-03 23:22 [PATCH v2 0/5] Avoid that scsi-mq queue processing stalls Bart Van Assche
@ 2017-04-03 23:22 ` Bart Van Assche
  2017-04-04  6:40   ` Christoph Hellwig
  2017-04-03 23:22 ` [PATCH v2 2/5] blk-mq: Make it safe to use RCU to iterate over blk_mq_tag_set.tag_list Bart Van Assche
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Bart Van Assche @ 2017-04-03 23:22 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, Martin K . Petersen, James Bottomley,
	Bart Van Assche, Christoph Hellwig, Hannes Reinecke

Since a later patch will add a call to this function from the
SCSI core, export this function. Move the BLK_MQ_S_SCHED_RESTART
bit test from blk_mq_sched_restart_hctx() into the callers of
this function. This leads to some code duplication but that will
be addressed in another patch in this series.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 block/blk-mq-sched.c   | 17 +++++++++--------
 include/linux/blk-mq.h |  1 +
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 09af8ff18719..414ed4b3d266 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -317,14 +317,13 @@ static bool blk_mq_sched_bypass_insert(struct blk_mq_hw_ctx *hctx,
 	return true;
 }
 
-static void blk_mq_sched_restart_hctx(struct blk_mq_hw_ctx *hctx)
+void blk_mq_sched_restart_hctx(struct blk_mq_hw_ctx *hctx)
 {
-	if (test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state)) {
-		clear_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state);
-		if (blk_mq_hctx_has_pending(hctx))
-			blk_mq_run_hw_queue(hctx, true);
-	}
+	clear_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state);
+	if (blk_mq_hctx_has_pending(hctx))
+		blk_mq_run_hw_queue(hctx, true);
 }
+EXPORT_SYMBOL(blk_mq_sched_restart_hctx);
 
 void blk_mq_sched_restart_queues(struct blk_mq_hw_ctx *hctx)
 {
@@ -334,9 +333,11 @@ void blk_mq_sched_restart_queues(struct blk_mq_hw_ctx *hctx)
 	if (test_bit(QUEUE_FLAG_RESTART, &q->queue_flags)) {
 		if (test_and_clear_bit(QUEUE_FLAG_RESTART, &q->queue_flags)) {
 			queue_for_each_hw_ctx(q, hctx, i)
-				blk_mq_sched_restart_hctx(hctx);
+				if (test_bit(BLK_MQ_S_SCHED_RESTART,
+					     &hctx->state))
+					blk_mq_sched_restart_hctx(hctx);
 		}
-	} else {
+	} else if (test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state)) {
 		blk_mq_sched_restart_hctx(hctx);
 	}
 }
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index ea2e9dcd3aef..f62f3ce2dc65 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -237,6 +237,7 @@ void blk_mq_stop_hw_queues(struct request_queue *q);
 void blk_mq_start_hw_queues(struct request_queue *q);
 void blk_mq_start_stopped_hw_queue(struct blk_mq_hw_ctx *hctx, bool async);
 void blk_mq_start_stopped_hw_queues(struct request_queue *q, bool async);
+void blk_mq_sched_restart_hctx(struct blk_mq_hw_ctx *hctx);
 void blk_mq_run_hw_queues(struct request_queue *q, bool async);
 void blk_mq_delay_queue(struct blk_mq_hw_ctx *hctx, unsigned long msecs);
 void blk_mq_tagset_busy_iter(struct blk_mq_tag_set *tagset,
-- 
2.12.0

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

* [PATCH v2 2/5] blk-mq: Make it safe to use RCU to iterate over blk_mq_tag_set.tag_list
  2017-04-03 23:22 [PATCH v2 0/5] Avoid that scsi-mq queue processing stalls Bart Van Assche
  2017-04-03 23:22 ` [PATCH v2 1/5] blk-mq: Export blk_mq_sched_restart_hctx() Bart Van Assche
@ 2017-04-03 23:22 ` Bart Van Assche
  2017-04-04  6:40   ` Christoph Hellwig
  2017-04-03 23:22 ` [PATCH v2 3/5] blk-mq: Introduce blk_mq_ops.restart_hctx Bart Van Assche
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Bart Van Assche @ 2017-04-03 23:22 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, Martin K . Petersen, James Bottomley,
	Bart Van Assche, Christoph Hellwig, Hannes Reinecke

A later patch in this series will namely use RCU to iterate over
this list.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 block/blk-mq.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 061fc2cc88d3..8fb983e6e2e4 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2093,6 +2093,8 @@ static void blk_mq_update_tag_set_depth(struct blk_mq_tag_set *set, bool shared)
 {
 	struct request_queue *q;
 
+	lockdep_assert_held(&set->tag_list_lock);
+
 	list_for_each_entry(q, &set->tag_list, tag_set_list) {
 		blk_mq_freeze_queue(q);
 		queue_set_hctx_shared(q, shared);
@@ -2113,6 +2115,8 @@ static void blk_mq_del_queue_tag_set(struct request_queue *q)
 		blk_mq_update_tag_set_depth(set, false);
 	}
 	mutex_unlock(&set->tag_list_lock);
+
+	synchronize_rcu();
 }
 
 static void blk_mq_add_queue_tag_set(struct blk_mq_tag_set *set,
@@ -2618,6 +2622,8 @@ void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues)
 {
 	struct request_queue *q;
 
+	lockdep_assert_held(&set->tag_list_lock);
+
 	if (nr_hw_queues > nr_cpu_ids)
 		nr_hw_queues = nr_cpu_ids;
 	if (nr_hw_queues < 1 || nr_hw_queues == set->nr_hw_queues)
-- 
2.12.0

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

* [PATCH v2 3/5] blk-mq: Introduce blk_mq_ops.restart_hctx
  2017-04-03 23:22 [PATCH v2 0/5] Avoid that scsi-mq queue processing stalls Bart Van Assche
  2017-04-03 23:22 ` [PATCH v2 1/5] blk-mq: Export blk_mq_sched_restart_hctx() Bart Van Assche
  2017-04-03 23:22 ` [PATCH v2 2/5] blk-mq: Make it safe to use RCU to iterate over blk_mq_tag_set.tag_list Bart Van Assche
@ 2017-04-03 23:22 ` Bart Van Assche
  2017-04-04  6:42   ` Christoph Hellwig
  2017-04-05 20:41   ` Omar Sandoval
  2017-04-03 23:22 ` [PATCH v2 4/5] scsi: Add scsi_restart_hctx() Bart Van Assche
  2017-04-03 23:22 ` [PATCH v2 5/5] scsi: Ensure that scsi_run_queue() runs all hardware queues Bart Van Assche
  4 siblings, 2 replies; 17+ messages in thread
From: Bart Van Assche @ 2017-04-03 23:22 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, Martin K . Petersen, James Bottomley,
	Bart Van Assche, Christoph Hellwig, Hannes Reinecke

If a tag set is shared among multiple hardware queues, leave
it to the block driver to rerun hardware queues. Hence remove
QUEUE_FLAG_RESTART and introduce blk_mq_ops.restart_hctx.
Remove blk_mq_sched_mark_restart_queue() because this
function has no callers.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 block/blk-mq-sched.c   | 15 ++++-----------
 block/blk-mq-sched.h   | 14 --------------
 include/linux/blk-mq.h |  7 +++++++
 include/linux/blkdev.h |  1 -
 4 files changed, 11 insertions(+), 26 deletions(-)

diff --git a/block/blk-mq-sched.c b/block/blk-mq-sched.c
index 414ed4b3d266..f0c691a3ef9e 100644
--- a/block/blk-mq-sched.c
+++ b/block/blk-mq-sched.c
@@ -328,18 +328,11 @@ EXPORT_SYMBOL(blk_mq_sched_restart_hctx);
 void blk_mq_sched_restart_queues(struct blk_mq_hw_ctx *hctx)
 {
 	struct request_queue *q = hctx->queue;
-	unsigned int i;
-
-	if (test_bit(QUEUE_FLAG_RESTART, &q->queue_flags)) {
-		if (test_and_clear_bit(QUEUE_FLAG_RESTART, &q->queue_flags)) {
-			queue_for_each_hw_ctx(q, hctx, i)
-				if (test_bit(BLK_MQ_S_SCHED_RESTART,
-					     &hctx->state))
-					blk_mq_sched_restart_hctx(hctx);
-		}
-	} else if (test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state)) {
+
+	if (q->mq_ops->restart_hctx)
+		q->mq_ops->restart_hctx(q, hctx);
+	else if (test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state))
 		blk_mq_sched_restart_hctx(hctx);
-	}
 }
 
 /*
diff --git a/block/blk-mq-sched.h b/block/blk-mq-sched.h
index a75b16b123f7..fe62b1eccf4c 100644
--- a/block/blk-mq-sched.h
+++ b/block/blk-mq-sched.h
@@ -131,20 +131,6 @@ static inline void blk_mq_sched_mark_restart_hctx(struct blk_mq_hw_ctx *hctx)
 		set_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state);
 }
 
-/*
- * Mark a hardware queue and the request queue it belongs to as needing a
- * restart.
- */
-static inline void blk_mq_sched_mark_restart_queue(struct blk_mq_hw_ctx *hctx)
-{
-	struct request_queue *q = hctx->queue;
-
-	if (!test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state))
-		set_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state);
-	if (!test_bit(QUEUE_FLAG_RESTART, &q->queue_flags))
-		set_bit(QUEUE_FLAG_RESTART, &q->queue_flags);
-}
-
 static inline bool blk_mq_sched_needs_restart(struct blk_mq_hw_ctx *hctx)
 {
 	return test_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state);
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index f62f3ce2dc65..ebeea36ff9cd 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -86,6 +86,7 @@ struct blk_mq_queue_data {
 };
 
 typedef int (queue_rq_fn)(struct blk_mq_hw_ctx *, const struct blk_mq_queue_data *);
+typedef void (restart_fn)(struct request_queue *q, struct blk_mq_hw_ctx *hctx);
 typedef enum blk_eh_timer_return (timeout_fn)(struct request *, bool);
 typedef int (init_hctx_fn)(struct blk_mq_hw_ctx *, void *, unsigned int);
 typedef void (exit_hctx_fn)(struct blk_mq_hw_ctx *, unsigned int);
@@ -109,6 +110,12 @@ struct blk_mq_ops {
 	queue_rq_fn		*queue_rq;
 
 	/*
+	 * Called upon request completion to rerun all hctxs that share a tag
+	 * set with the hctx for which a request finished.
+	 */
+	restart_fn		*restart_hctx;
+
+	/*
 	 * Called on request timeout
 	 */
 	timeout_fn		*timeout;
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index a2dc6b390d48..a80543ec8be7 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -615,7 +615,6 @@ struct request_queue {
 #define QUEUE_FLAG_FLUSH_NQ    25	/* flush not queueuable */
 #define QUEUE_FLAG_DAX         26	/* device supports DAX */
 #define QUEUE_FLAG_STATS       27	/* track rq completion times */
-#define QUEUE_FLAG_RESTART     28	/* queue needs restart at completion */
 #define QUEUE_FLAG_POLL_STATS  29	/* collecting stats for hybrid polling */
 #define QUEUE_FLAG_REGISTERED  30	/* queue has been registered to a disk */
 
-- 
2.12.0

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

* [PATCH v2 4/5] scsi: Add scsi_restart_hctx()
  2017-04-03 23:22 [PATCH v2 0/5] Avoid that scsi-mq queue processing stalls Bart Van Assche
                   ` (2 preceding siblings ...)
  2017-04-03 23:22 ` [PATCH v2 3/5] blk-mq: Introduce blk_mq_ops.restart_hctx Bart Van Assche
@ 2017-04-03 23:22 ` Bart Van Assche
  2017-04-04  6:42   ` Christoph Hellwig
  2017-04-03 23:22 ` [PATCH v2 5/5] scsi: Ensure that scsi_run_queue() runs all hardware queues Bart Van Assche
  4 siblings, 1 reply; 17+ messages in thread
From: Bart Van Assche @ 2017-04-03 23:22 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, Martin K . Petersen, James Bottomley,
	Bart Van Assche, Christoph Hellwig, Hannes Reinecke

This patch avoids that if multiple SCSI devices are associated with
a SCSI host that a queue can get stuck if scsi_queue_rq() returns
"busy".

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/scsi_lib.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index c1519660824b..0e240aebc150 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -555,6 +555,21 @@ void scsi_run_host_queues(struct Scsi_Host *shost)
 		scsi_run_queue(sdev->request_queue);
 }
 
+static void scsi_restart_hctx(struct request_queue *q,
+			      struct blk_mq_hw_ctx *hctx)
+{
+	struct blk_mq_tags *tags = hctx->tags;
+	struct blk_mq_tag_set *set = q->tag_set;
+	int i;
+
+	rcu_read_lock();
+	list_for_each_entry_rcu(q, &set->tag_list, tag_set_list)
+		queue_for_each_hw_ctx(q, hctx, i)
+			if (hctx->tags == tags)
+				blk_mq_sched_restart_hctx(hctx);
+	rcu_read_unlock();
+}
+
 static void scsi_uninit_cmd(struct scsi_cmnd *cmd)
 {
 	if (!blk_rq_is_passthrough(cmd->request)) {
@@ -2156,6 +2171,7 @@ struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
 
 static const struct blk_mq_ops scsi_mq_ops = {
 	.queue_rq	= scsi_queue_rq,
+	.restart_hctx	= scsi_restart_hctx,
 	.complete	= scsi_softirq_done,
 	.timeout	= scsi_timeout,
 	.init_request	= scsi_init_request,
-- 
2.12.0

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

* [PATCH v2 5/5] scsi: Ensure that scsi_run_queue() runs all hardware queues
  2017-04-03 23:22 [PATCH v2 0/5] Avoid that scsi-mq queue processing stalls Bart Van Assche
                   ` (3 preceding siblings ...)
  2017-04-03 23:22 ` [PATCH v2 4/5] scsi: Add scsi_restart_hctx() Bart Van Assche
@ 2017-04-03 23:22 ` Bart Van Assche
  4 siblings, 0 replies; 17+ messages in thread
From: Bart Van Assche @ 2017-04-03 23:22 UTC (permalink / raw)
  To: Jens Axboe
  Cc: linux-block, Martin K . Petersen, James Bottomley,
	Bart Van Assche, Christoph Hellwig

commit 52d7f1b5c2f3 ("blk-mq: Avoid that requeueing starts stopped
queues") removed the blk_mq_stop_hw_queue() call from scsi_queue_rq()
for the BLK_MQ_RQ_QUEUE_BUSY case. blk_mq_start_stopped_hw_queues()
only runs queues that had been stopped. Hence change the
blk_mq_start_stopped_hw_queues() call in scsi_run_queue() into
blk_mq_run_hw_queues(). Remove the blk_mq_start_stopped_hw_queues()
call from scsi_end_request() because __blk_mq_finish_request()
already runs all hardware queues if needed.

Fixes: commit 52d7f1b5c2f3 ("blk-mq: Avoid that requeueing starts stopped queues")
Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Cc: Martin K. Petersen <martin.petersen@oracle.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
Cc: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/scsi_lib.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 0e240aebc150..4459b18f62a1 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -496,7 +496,7 @@ static void scsi_run_queue(struct request_queue *q)
 		scsi_starved_list_run(sdev->host);
 
 	if (q->mq_ops)
-		blk_mq_start_stopped_hw_queues(q, false);
+		blk_mq_run_hw_queues(q, false);
 	else
 		blk_run_queue(q);
 }
@@ -681,8 +681,6 @@ static bool scsi_end_request(struct request *req, int error,
 		if (scsi_target(sdev)->single_lun ||
 		    !list_empty(&sdev->host->starved_list))
 			kblockd_schedule_work(&sdev->requeue_work);
-		else
-			blk_mq_start_stopped_hw_queues(q, true);
 	} else {
 		unsigned long flags;
 
-- 
2.12.0

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

* Re: [PATCH v2 1/5] blk-mq: Export blk_mq_sched_restart_hctx()
  2017-04-03 23:22 ` [PATCH v2 1/5] blk-mq: Export blk_mq_sched_restart_hctx() Bart Van Assche
@ 2017-04-04  6:40   ` Christoph Hellwig
  0 siblings, 0 replies; 17+ messages in thread
From: Christoph Hellwig @ 2017-04-04  6:40 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Jens Axboe, linux-block, Martin K . Petersen, James Bottomley,
	Christoph Hellwig, Hannes Reinecke

> +void blk_mq_sched_restart_hctx(struct blk_mq_hw_ctx *hctx)
>  {
> +	clear_bit(BLK_MQ_S_SCHED_RESTART, &hctx->state);
> +	if (blk_mq_hctx_has_pending(hctx))
> +		blk_mq_run_hw_queue(hctx, true);
>  }
> +EXPORT_SYMBOL(blk_mq_sched_restart_hctx);

_GPL export like the other _hctx functions, please.

Otherwise looks fine:

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

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

* Re: [PATCH v2 2/5] blk-mq: Make it safe to use RCU to iterate over blk_mq_tag_set.tag_list
  2017-04-03 23:22 ` [PATCH v2 2/5] blk-mq: Make it safe to use RCU to iterate over blk_mq_tag_set.tag_list Bart Van Assche
@ 2017-04-04  6:40   ` Christoph Hellwig
  2017-04-04 15:55     ` Bart Van Assche
  0 siblings, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2017-04-04  6:40 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Jens Axboe, linux-block, Martin K . Petersen, James Bottomley,
	Christoph Hellwig, Hannes Reinecke

On Mon, Apr 03, 2017 at 04:22:25PM -0700, Bart Van Assche wrote:
> A later patch in this series will namely use RCU to iterate over
> this list.

It also adds a couple lockdep_assert_held calls, which might be worth
mentionining.

Otherwise looks good:

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

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

* Re: [PATCH v2 3/5] blk-mq: Introduce blk_mq_ops.restart_hctx
  2017-04-03 23:22 ` [PATCH v2 3/5] blk-mq: Introduce blk_mq_ops.restart_hctx Bart Van Assche
@ 2017-04-04  6:42   ` Christoph Hellwig
  2017-04-05 20:41   ` Omar Sandoval
  1 sibling, 0 replies; 17+ messages in thread
From: Christoph Hellwig @ 2017-04-04  6:42 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Jens Axboe, linux-block, Martin K . Petersen, James Bottomley,
	Christoph Hellwig, Hannes Reinecke

On Mon, Apr 03, 2017 at 04:22:26PM -0700, Bart Van Assche wrote:
> If a tag set is shared among multiple hardware queues, leave
> it to the block driver to rerun hardware queues. Hence remove
> QUEUE_FLAG_RESTART and introduce blk_mq_ops.restart_hctx.
> Remove blk_mq_sched_mark_restart_queue() because this
> function has no callers.

This looks fine, but I think it needs to also actually implement at
least a dummy restart_hctx method for SCSI and NVMe to keep the
existing functionality.

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

* Re: [PATCH v2 4/5] scsi: Add scsi_restart_hctx()
  2017-04-03 23:22 ` [PATCH v2 4/5] scsi: Add scsi_restart_hctx() Bart Van Assche
@ 2017-04-04  6:42   ` Christoph Hellwig
  2017-04-04 15:56     ` Bart Van Assche
  0 siblings, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2017-04-04  6:42 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Jens Axboe, linux-block, Martin K . Petersen, James Bottomley,
	Christoph Hellwig, Hannes Reinecke

> +static void scsi_restart_hctx(struct request_queue *q,
> +			      struct blk_mq_hw_ctx *hctx)
> +{
> +	struct blk_mq_tags *tags = hctx->tags;
> +	struct blk_mq_tag_set *set = q->tag_set;
> +	int i;
> +
> +	rcu_read_lock();
> +	list_for_each_entry_rcu(q, &set->tag_list, tag_set_list)
> +		queue_for_each_hw_ctx(q, hctx, i)
> +			if (hctx->tags == tags)
> +				blk_mq_sched_restart_hctx(hctx);
> +	rcu_read_unlock();
> +}

This looks like generic block layer code, why is it in SCSI?

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

* Re: [PATCH v2 2/5] blk-mq: Make it safe to use RCU to iterate over blk_mq_tag_set.tag_list
  2017-04-04  6:40   ` Christoph Hellwig
@ 2017-04-04 15:55     ` Bart Van Assche
  0 siblings, 0 replies; 17+ messages in thread
From: Bart Van Assche @ 2017-04-04 15:55 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jens Axboe, linux-block, Martin K . Petersen, James Bottomley,
	Hannes Reinecke

On 04/03/2017 11:40 PM, Christoph Hellwig wrote:=0A=
> On Mon, Apr 03, 2017 at 04:22:25PM -0700, Bart Van Assche wrote:=0A=
>> A later patch in this series will namely use RCU to iterate over=0A=
>> this list.=0A=
> =0A=
> It also adds a couple lockdep_assert_held calls, which might be worth=0A=
> mentioning.=0A=
> =0A=
> Otherwise looks good:=0A=
> =0A=
> Reviewed-by: Christoph Hellwig <hch@lst.de>=0A=
=0A=
Thanks for the review. I will update the patch description.=0A=
=0A=
Bart.=0A=
=0A=

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

* Re: [PATCH v2 4/5] scsi: Add scsi_restart_hctx()
  2017-04-04  6:42   ` Christoph Hellwig
@ 2017-04-04 15:56     ` Bart Van Assche
  2017-04-05  6:20       ` Christoph Hellwig
  0 siblings, 1 reply; 17+ messages in thread
From: Bart Van Assche @ 2017-04-04 15:56 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jens Axboe, linux-block, Martin K . Petersen, James Bottomley,
	Hannes Reinecke

On 04/03/2017 11:42 PM, Christoph Hellwig wrote:=0A=
>> +static void scsi_restart_hctx(struct request_queue *q,=0A=
>> +			      struct blk_mq_hw_ctx *hctx)=0A=
>> +{=0A=
>> +	struct blk_mq_tags *tags =3D hctx->tags;=0A=
>> +	struct blk_mq_tag_set *set =3D q->tag_set;=0A=
>> +	int i;=0A=
>> +=0A=
>> +	rcu_read_lock();=0A=
>> +	list_for_each_entry_rcu(q, &set->tag_list, tag_set_list)=0A=
>> +		queue_for_each_hw_ctx(q, hctx, i)=0A=
>> +			if (hctx->tags =3D=3D tags)=0A=
>> +				blk_mq_sched_restart_hctx(hctx);=0A=
>> +	rcu_read_unlock();=0A=
>> +}=0A=
> =0A=
> This looks like generic block layer code, why is it in SCSI?=0A=
=0A=
Hello Christoph,=0A=
=0A=
That's an excellent question. I assume that you are fine with moving=0A=
this code to the block layer?=0A=
=0A=
Bart.=0A=
=0A=

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

* Re: [PATCH v2 4/5] scsi: Add scsi_restart_hctx()
  2017-04-04 15:56     ` Bart Van Assche
@ 2017-04-05  6:20       ` Christoph Hellwig
  2017-04-05 17:36         ` Bart Van Assche
  0 siblings, 1 reply; 17+ messages in thread
From: Christoph Hellwig @ 2017-04-05  6:20 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Christoph Hellwig, Jens Axboe, linux-block, Martin K . Petersen,
	James Bottomley, Hannes Reinecke

On Tue, Apr 04, 2017 at 03:56:34PM +0000, Bart Van Assche wrote:
> > This looks like generic block layer code, why is it in SCSI?
> 
> Hello Christoph,
> 
> That's an excellent question. I assume that you are fine with moving
> this code to the block layer?

Yes.  In fact I wonder if we need the blk_mq_ops method at all now
that you're using RCU locking and don't need the scsi host_lock.

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

* Re: [PATCH v2 4/5] scsi: Add scsi_restart_hctx()
  2017-04-05  6:20       ` Christoph Hellwig
@ 2017-04-05 17:36         ` Bart Van Assche
  0 siblings, 0 replies; 17+ messages in thread
From: Bart Van Assche @ 2017-04-05 17:36 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jens Axboe, linux-block, Martin K . Petersen, James Bottomley,
	Hannes Reinecke

On 04/04/2017 11:20 PM, Christoph Hellwig wrote:
> On Tue, Apr 04, 2017 at 03:56:34PM +0000, Bart Van Assche wrote:
>>> This looks like generic block layer code, why is it in SCSI?
>>
>> That's an excellent question. I assume that you are fine with moving
>> this code to the block layer?
> 
> Yes.  In fact I wonder if we need the blk_mq_ops method at all now
> that you're using RCU locking and don't need the scsi host_lock.

Hello Christoph,

My plan is to remove the new blk_mq_ops method again before I post v3 of
this patch series.

Bart.

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

* Re: [PATCH v2 3/5] blk-mq: Introduce blk_mq_ops.restart_hctx
  2017-04-03 23:22 ` [PATCH v2 3/5] blk-mq: Introduce blk_mq_ops.restart_hctx Bart Van Assche
  2017-04-04  6:42   ` Christoph Hellwig
@ 2017-04-05 20:41   ` Omar Sandoval
  2017-04-05 20:51     ` Bart Van Assche
  1 sibling, 1 reply; 17+ messages in thread
From: Omar Sandoval @ 2017-04-05 20:41 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Jens Axboe, linux-block, Martin K . Petersen, James Bottomley,
	Christoph Hellwig, Hannes Reinecke

On Mon, Apr 03, 2017 at 04:22:26PM -0700, Bart Van Assche wrote:
> If a tag set is shared among multiple hardware queues, leave
> it to the block driver to rerun hardware queues. Hence remove
> QUEUE_FLAG_RESTART and introduce blk_mq_ops.restart_hctx.
> Remove blk_mq_sched_mark_restart_queue() because this
> function has no callers.

Hi, Bart,

Kyber uses blk_mq_sched_mark_restart_queue() and the QUEUE_FLAG_RESTART
bit. If it's not too much trouble, it'd make things easier for me if you
left it in place. If it's a pain, it's fine if you get rid of it, I can
reintroduce it in my series.

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

* Re: [PATCH v2 3/5] blk-mq: Introduce blk_mq_ops.restart_hctx
  2017-04-05 20:41   ` Omar Sandoval
@ 2017-04-05 20:51     ` Bart Van Assche
  2017-04-05 20:54       ` Omar Sandoval
  0 siblings, 1 reply; 17+ messages in thread
From: Bart Van Assche @ 2017-04-05 20:51 UTC (permalink / raw)
  To: osandov; +Cc: hch, James.Bottomley, linux-block, hare, martin.petersen, axboe

On Wed, 2017-04-05 at 13:41 -0700, Omar Sandoval wrote:
> On Mon, Apr 03, 2017 at 04:22:26PM -0700, Bart Van Assche wrote:
> > If a tag set is shared among multiple hardware queues, leave
> > it to the block driver to rerun hardware queues. Hence remove
> > QUEUE_FLAG_RESTART and introduce blk_mq_ops.restart_hctx.
> > Remove blk_mq_sched_mark_restart_queue() because this
> > function has no callers.
>=20
> Kyber uses blk_mq_sched_mark_restart_queue() and the QUEUE_FLAG_RESTART
> bit. If it's not too much trouble, it'd make things easier for me if you
> left it in place. If it's a pain, it's fine if you get rid of it, I can
> reintroduce it in my series.

Hello Omar,

Would it be OK for you to reintroduce blk_mq_sched_mark_restart_queue()?
Since that function does not yet have any users I can't test any changes
I make to that function ...

Thanks,

Bart.=

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

* Re: [PATCH v2 3/5] blk-mq: Introduce blk_mq_ops.restart_hctx
  2017-04-05 20:51     ` Bart Van Assche
@ 2017-04-05 20:54       ` Omar Sandoval
  0 siblings, 0 replies; 17+ messages in thread
From: Omar Sandoval @ 2017-04-05 20:54 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: hch, James.Bottomley, linux-block, hare, martin.petersen, axboe

On Wed, Apr 05, 2017 at 08:51:51PM +0000, Bart Van Assche wrote:
> On Wed, 2017-04-05 at 13:41 -0700, Omar Sandoval wrote:
> > On Mon, Apr 03, 2017 at 04:22:26PM -0700, Bart Van Assche wrote:
> > > If a tag set is shared among multiple hardware queues, leave
> > > it to the block driver to rerun hardware queues. Hence remove
> > > QUEUE_FLAG_RESTART and introduce blk_mq_ops.restart_hctx.
> > > Remove blk_mq_sched_mark_restart_queue() because this
> > > function has no callers.
> > 
> > Kyber uses blk_mq_sched_mark_restart_queue() and the QUEUE_FLAG_RESTART
> > bit. If it's not too much trouble, it'd make things easier for me if you
> > left it in place. If it's a pain, it's fine if you get rid of it, I can
> > reintroduce it in my series.
> 
> Hello Omar,
> 
> Would it be OK for you to reintroduce blk_mq_sched_mark_restart_queue()?
> Since that function does not yet have any users I can't test any changes
> I make to that function ...
> 
> Thanks,
> 
> Bart.

Yeah, that's fine.

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

end of thread, other threads:[~2017-04-05 20:54 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-03 23:22 [PATCH v2 0/5] Avoid that scsi-mq queue processing stalls Bart Van Assche
2017-04-03 23:22 ` [PATCH v2 1/5] blk-mq: Export blk_mq_sched_restart_hctx() Bart Van Assche
2017-04-04  6:40   ` Christoph Hellwig
2017-04-03 23:22 ` [PATCH v2 2/5] blk-mq: Make it safe to use RCU to iterate over blk_mq_tag_set.tag_list Bart Van Assche
2017-04-04  6:40   ` Christoph Hellwig
2017-04-04 15:55     ` Bart Van Assche
2017-04-03 23:22 ` [PATCH v2 3/5] blk-mq: Introduce blk_mq_ops.restart_hctx Bart Van Assche
2017-04-04  6:42   ` Christoph Hellwig
2017-04-05 20:41   ` Omar Sandoval
2017-04-05 20:51     ` Bart Van Assche
2017-04-05 20:54       ` Omar Sandoval
2017-04-03 23:22 ` [PATCH v2 4/5] scsi: Add scsi_restart_hctx() Bart Van Assche
2017-04-04  6:42   ` Christoph Hellwig
2017-04-04 15:56     ` Bart Van Assche
2017-04-05  6:20       ` Christoph Hellwig
2017-04-05 17:36         ` Bart Van Assche
2017-04-03 23:22 ` [PATCH v2 5/5] scsi: Ensure that scsi_run_queue() runs all hardware queues Bart Van Assche

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).