All of lore.kernel.org
 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 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.