less special casing for flush requests v2

less special casing for flush requests v2

[Christoph Hellwig]

Hi all,

inspired by Barts quest for less reording during requeues, this series
aims to limit the scope of requeues.  This is mostly done by simply
sending down more than we currently do down the normal submission path
with the help of Bart's patch to allow requests that are in the flush
state machine to still be seen by the I/O schedulers.

Changes since v1:
 - rebased
 - fix a bug in the blk state machine insertation order

Diffstat:
 block/blk-flush.c      |  110 +++++++++++++++++++++++++++----------------------
 block/blk-mq-debugfs.c |    1 
 block/blk-mq.c         |   54 ++++++++----------------
 block/blk-mq.h         |    5 --
 block/blk.h            |    2 
 include/linux/blk-mq.h |   31 +++++--------
 include/linux/blkdev.h |    1 
 7 files changed, 94 insertions(+), 110 deletions(-)

[PATCH 1/7] blk-mq: factor out a blk_rq_init_flush helper

[Christoph Hellwig]

Factor out a helper from blk_insert_flush that initializes the flush
machine related fields in struct request, and don't bother with the
full memset as there's just a few fields to initialize, and all but
one already have explicit initializers.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
---
 block/blk-flush.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 04698ed9bcd4a9..ed37d272f787eb 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -376,6 +376,15 @@ static enum rq_end_io_ret mq_flush_data_end_io(struct request *rq,
 	return RQ_END_IO_NONE;
 }
 
+static void blk_rq_init_flush(struct request *rq)
+{
+	rq->flush.seq = 0;
+	INIT_LIST_HEAD(&rq->flush.list);
+	rq->rq_flags |= RQF_FLUSH_SEQ;
+	rq->flush.saved_end_io = rq->end_io; /* Usually NULL */
+	rq->end_io = mq_flush_data_end_io;
+}
+
 /**
  * blk_insert_flush - insert a new PREFLUSH/FUA request
  * @rq: request to insert
@@ -437,13 +446,7 @@ void blk_insert_flush(struct request *rq)
 	 * @rq should go through flush machinery.  Mark it part of flush
 	 * sequence and submit for further processing.
 	 */
-	memset(&rq->flush, 0, sizeof(rq->flush));
-	INIT_LIST_HEAD(&rq->flush.list);
-	rq->rq_flags |= RQF_FLUSH_SEQ;
-	rq->flush.saved_end_io = rq->end_io; /* Usually NULL */
-
-	rq->end_io = mq_flush_data_end_io;
-
+	blk_rq_init_flush(rq);
 	spin_lock_irq(&fq->mq_flush_lock);
 	blk_flush_complete_seq(rq, fq, REQ_FSEQ_ACTIONS & ~policy, 0);
 	spin_unlock_irq(&fq->mq_flush_lock);
-- 
2.39.2

[PATCH 2/7] blk-mq: reflow blk_insert_flush

[Christoph Hellwig]

Use a switch statement to decide on the disposition of a flush request
instead of multiple if statements, out of which one does checks that are
more complex than required.  Also warn on a malformed request early
on instead of doing a BUG_ON later.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
---
 block/blk-flush.c | 53 +++++++++++++++++++++++------------------------
 1 file changed, 26 insertions(+), 27 deletions(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index ed37d272f787eb..d8144f1f6fb12f 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -402,6 +402,9 @@ void blk_insert_flush(struct request *rq)
 	struct blk_flush_queue *fq = blk_get_flush_queue(q, rq->mq_ctx);
 	struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
 
+	/* FLUSH/FUA request must never be merged */
+	WARN_ON_ONCE(rq->bio != rq->biotail);
+
 	/*
 	 * @policy now records what operations need to be done.  Adjust
 	 * REQ_PREFLUSH and FUA for the driver.
@@ -417,39 +420,35 @@ void blk_insert_flush(struct request *rq)
 	 */
 	rq->cmd_flags |= REQ_SYNC;
 
-	/*
-	 * An empty flush handed down from a stacking driver may
-	 * translate into nothing if the underlying device does not
-	 * advertise a write-back cache.  In this case, simply
-	 * complete the request.
-	 */
-	if (!policy) {
+	switch (policy) {
+	case 0:
+		/*
+		 * An empty flush handed down from a stacking driver may
+		 * translate into nothing if the underlying device does not
+		 * advertise a write-back cache.  In this case, simply
+		 * complete the request.
+		 */
 		blk_mq_end_request(rq, 0);
 		return;
-	}
-
-	BUG_ON(rq->bio != rq->biotail); /*assumes zero or single bio rq */
-
-	/*
-	 * If there's data but flush is not necessary, the request can be
-	 * processed directly without going through flush machinery.  Queue
-	 * for normal execution.
-	 */
-	if ((policy & REQ_FSEQ_DATA) &&
-	    !(policy & (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH))) {
+	case REQ_FSEQ_DATA:
+		/*
+		 * If there's data, but no flush is necessary, the request can
+		 * be processed directly without going through flush machinery.
+		 * Queue for normal execution.
+		 */
 		blk_mq_request_bypass_insert(rq, 0);
 		blk_mq_run_hw_queue(hctx, false);
 		return;
+	default:
+		/*
+		 * Mark the request as part of a flush sequence and submit it
+		 * for further processing to the flush state machine.
+		 */
+		blk_rq_init_flush(rq);
+		spin_lock_irq(&fq->mq_flush_lock);
+		blk_flush_complete_seq(rq, fq, REQ_FSEQ_ACTIONS & ~policy, 0);
+		spin_unlock_irq(&fq->mq_flush_lock);
 	}
-
-	/*
-	 * @rq should go through flush machinery.  Mark it part of flush
-	 * sequence and submit for further processing.
-	 */
-	blk_rq_init_flush(rq);
-	spin_lock_irq(&fq->mq_flush_lock);
-	blk_flush_complete_seq(rq, fq, REQ_FSEQ_ACTIONS & ~policy, 0);
-	spin_unlock_irq(&fq->mq_flush_lock);
 }
 
 /**
-- 
2.39.2

[PATCH 3/7] blk-mq: defer to the normal submission path for non-flush flush commands

[Christoph Hellwig]

If blk_insert_flush decides that a command does not need to use the
flush state machine, return false and let blk_mq_submit_bio handle
it the normal way (including using an I/O scheduler) instead of doing
a bypass insert.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
---
 block/blk-flush.c | 22 ++++++++--------------
 block/blk-mq.c    |  8 ++++----
 block/blk-mq.h    |  4 ----
 block/blk.h       |  2 +-
 4 files changed, 13 insertions(+), 23 deletions(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index d8144f1f6fb12f..6fb9cf2d38184b 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -385,22 +385,17 @@ static void blk_rq_init_flush(struct request *rq)
 	rq->end_io = mq_flush_data_end_io;
 }
 
-/**
- * blk_insert_flush - insert a new PREFLUSH/FUA request
- * @rq: request to insert
- *
- * To be called from __elv_add_request() for %ELEVATOR_INSERT_FLUSH insertions.
- * or __blk_mq_run_hw_queue() to dispatch request.
- * @rq is being submitted.  Analyze what needs to be done and put it on the
- * right queue.
+/*
+ * Insert a PREFLUSH/FUA request into the flush state machine.
+ * Returns true if the request has been consumed by the flush state machine,
+ * or false if the caller should continue to process it.
  */
-void blk_insert_flush(struct request *rq)
+bool blk_insert_flush(struct request *rq)
 {
 	struct request_queue *q = rq->q;
 	unsigned long fflags = q->queue_flags;	/* may change, cache */
 	unsigned int policy = blk_flush_policy(fflags, rq);
 	struct blk_flush_queue *fq = blk_get_flush_queue(q, rq->mq_ctx);
-	struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
 
 	/* FLUSH/FUA request must never be merged */
 	WARN_ON_ONCE(rq->bio != rq->biotail);
@@ -429,16 +424,14 @@ void blk_insert_flush(struct request *rq)
 		 * complete the request.
 		 */
 		blk_mq_end_request(rq, 0);
-		return;
+		return true;
 	case REQ_FSEQ_DATA:
 		/*
 		 * If there's data, but no flush is necessary, the request can
 		 * be processed directly without going through flush machinery.
 		 * Queue for normal execution.
 		 */
-		blk_mq_request_bypass_insert(rq, 0);
-		blk_mq_run_hw_queue(hctx, false);
-		return;
+		return false;
 	default:
 		/*
 		 * Mark the request as part of a flush sequence and submit it
@@ -448,6 +441,7 @@ void blk_insert_flush(struct request *rq)
 		spin_lock_irq(&fq->mq_flush_lock);
 		blk_flush_complete_seq(rq, fq, REQ_FSEQ_ACTIONS & ~policy, 0);
 		spin_unlock_irq(&fq->mq_flush_lock);
+		return true;
 	}
 }
 
diff --git a/block/blk-mq.c b/block/blk-mq.c
index e021740154feae..c0b394096b6b6b 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -45,6 +45,8 @@
 static DEFINE_PER_CPU(struct llist_head, blk_cpu_done);
 
 static void blk_mq_insert_request(struct request *rq, blk_insert_t flags);
+static void blk_mq_request_bypass_insert(struct request *rq,
+		blk_insert_t flags);
 static void blk_mq_try_issue_list_directly(struct blk_mq_hw_ctx *hctx,
 		struct list_head *list);
 
@@ -2430,7 +2432,7 @@ static void blk_mq_run_work_fn(struct work_struct *work)
  * Should only be used carefully, when the caller knows we want to
  * bypass a potential IO scheduler on the target device.
  */
-void blk_mq_request_bypass_insert(struct request *rq, blk_insert_t flags)
+static void blk_mq_request_bypass_insert(struct request *rq, blk_insert_t flags)
 {
 	struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
 
@@ -2977,10 +2979,8 @@ void blk_mq_submit_bio(struct bio *bio)
 		return;
 	}
 
-	if (op_is_flush(bio->bi_opf)) {
-		blk_insert_flush(rq);
+	if (op_is_flush(bio->bi_opf) && blk_insert_flush(rq))
 		return;
-	}
 
 	if (plug) {
 		blk_add_rq_to_plug(plug, rq);
diff --git a/block/blk-mq.h b/block/blk-mq.h
index d15981db34b958..ec7d2fb0b3c8ef 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -64,10 +64,6 @@ struct blk_mq_tags *blk_mq_alloc_map_and_rqs(struct blk_mq_tag_set *set,
 void blk_mq_free_map_and_rqs(struct blk_mq_tag_set *set,
 			     struct blk_mq_tags *tags,
 			     unsigned int hctx_idx);
-/*
- * Internal helpers for request insertion into sw queues
- */
-void blk_mq_request_bypass_insert(struct request *rq, blk_insert_t flags);
 
 /*
  * CPU -> queue mappings
diff --git a/block/blk.h b/block/blk.h
index 45547bcf111938..9f171b8f1e3402 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -269,7 +269,7 @@ bool blk_bio_list_merge(struct request_queue *q, struct list_head *list,
  */
 #define ELV_ON_HASH(rq) ((rq)->rq_flags & RQF_HASHED)
 
-void blk_insert_flush(struct request *rq);
+bool blk_insert_flush(struct request *rq);
 
 int elevator_switch(struct request_queue *q, struct elevator_type *new_e);
 void elevator_disable(struct request_queue *q);
-- 
2.39.2

[PATCH 4/7] blk-mq: use the I/O scheduler for writes from the flush state machine

[Christoph Hellwig]

From: Bart Van Assche <bvanassche@acm.org>

Send write requests issued by the flush state machine through the normal
I/O submission path including the I/O scheduler (if present) so that I/O
scheduler policies are applied to writes with the FUA flag set.

Separate the I/O scheduler members from the flush members in struct
request since now a request may pass through both an I/O scheduler
and the flush machinery.

Note that the actual flush requests, which have no bio attached to the
request still bypass the I/O schedulers.

Signed-off-by: Bart Van Assche <bvanassche@acm.org>
[hch: rebased]
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
---
 block/blk-mq.c         |  4 ++--
 include/linux/blk-mq.h | 27 +++++++++++----------------
 2 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index c0b394096b6b6b..aac67bc3d3680c 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -458,7 +458,7 @@ static struct request *__blk_mq_alloc_requests(struct blk_mq_alloc_data *data)
 		 * Flush/passthrough requests are special and go directly to the
 		 * dispatch list.
 		 */
-		if (!op_is_flush(data->cmd_flags) &&
+		if ((data->cmd_flags & REQ_OP_MASK) != REQ_OP_FLUSH &&
 		    !blk_op_is_passthrough(data->cmd_flags)) {
 			struct elevator_mq_ops *ops = &q->elevator->type->ops;
 
@@ -2497,7 +2497,7 @@ static void blk_mq_insert_request(struct request *rq, blk_insert_t flags)
 		 * dispatch it given we prioritize requests in hctx->dispatch.
 		 */
 		blk_mq_request_bypass_insert(rq, flags);
-	} else if (rq->rq_flags & RQF_FLUSH_SEQ) {
+	} else if (req_op(rq) == REQ_OP_FLUSH) {
 		/*
 		 * Firstly normal IO request is inserted to scheduler queue or
 		 * sw queue, meantime we add flush request to dispatch queue(
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 49d14b1acfa5df..935201c8974371 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -169,25 +169,20 @@ struct request {
 		void *completion_data;
 	};
 
-
 	/*
 	 * Three pointers are available for the IO schedulers, if they need
-	 * more they have to dynamically allocate it.  Flush requests are
-	 * never put on the IO scheduler. So let the flush fields share
-	 * space with the elevator data.
+	 * more they have to dynamically allocate it.
 	 */
-	union {
-		struct {
-			struct io_cq		*icq;
-			void			*priv[2];
-		} elv;
-
-		struct {
-			unsigned int		seq;
-			struct list_head	list;
-			rq_end_io_fn		*saved_end_io;
-		} flush;
-	};
+	struct {
+		struct io_cq		*icq;
+		void			*priv[2];
+	} elv;
+
+	struct {
+		unsigned int		seq;
+		struct list_head	list;
+		rq_end_io_fn		*saved_end_io;
+	} flush;
 
 	union {
 		struct __call_single_data csd;
-- 
2.39.2

[PATCH 5/7] blk-mq: defer to the normal submission path for post-flush requests

[Christoph Hellwig]

Requests with the FUA bit on hardware without FUA support need a post
flush before returning to the caller, but they can still be sent using
the normal I/O path after initializing the flush-related fields and
end I/O handler.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 block/blk-flush.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 6fb9cf2d38184b..7121f9ad0762f8 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -432,6 +432,17 @@ bool blk_insert_flush(struct request *rq)
 		 * Queue for normal execution.
 		 */
 		return false;
+	case REQ_FSEQ_DATA | REQ_FSEQ_POSTFLUSH:
+		/*
+		 * Initialize the flush fields and completion handler to trigger
+		 * the post flush, and then just pass the command on.
+		 */
+		blk_rq_init_flush(rq);
+		rq->flush.seq |= REQ_FSEQ_POSTFLUSH;
+		spin_lock_irq(&fq->mq_flush_lock);
+		list_move_tail(&rq->flush.list, &fq->flush_data_in_flight);
+		spin_unlock_irq(&fq->mq_flush_lock);
+		return false;
 	default:
 		/*
 		 * Mark the request as part of a flush sequence and submit it
-- 
2.39.2

[PATCH 6/7] blk-mq: do not do head insertions post-pre-flush commands

[Christoph Hellwig]

blk_flush_complete_seq currently queues requests that write data after
a pre-flush from the flush state machine at the head of the queue.
This doesn't really make sense, as the original request bypassed all
queue lists by directly diverting to blk_insert_flush from
blk_mq_submit_bio.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
---
 block/blk-flush.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index 7121f9ad0762f8..f407a59503173d 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -188,7 +188,7 @@ static void blk_flush_complete_seq(struct request *rq,
 
 	case REQ_FSEQ_DATA:
 		list_move_tail(&rq->flush.list, &fq->flush_data_in_flight);
-		blk_mq_add_to_requeue_list(rq, BLK_MQ_INSERT_AT_HEAD);
+		blk_mq_add_to_requeue_list(rq, 0);
 		blk_mq_kick_requeue_list(q);
 		break;
 
-- 
2.39.2

[PATCH 7/7] blk-mq: don't use the requeue list to queue flush commands

[Christoph Hellwig]

Currently both requeues of commands that were already sent to the
driver and flush commands submitted from the flush state machine
share the same requeue_list struct request_queue, despite requeues
doing head insertations and flushes not.  Switch to using two
separate lists instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
---
 block/blk-flush.c      |  9 +++++++--
 block/blk-mq-debugfs.c |  1 -
 block/blk-mq.c         | 42 +++++++++++++-----------------------------
 block/blk-mq.h         |  1 -
 include/linux/blk-mq.h |  4 +---
 include/linux/blkdev.h |  1 +
 6 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/block/blk-flush.c b/block/blk-flush.c
index f407a59503173d..dba392cf22bec6 100644
--- a/block/blk-flush.c
+++ b/block/blk-flush.c
@@ -188,7 +188,9 @@ static void blk_flush_complete_seq(struct request *rq,
 
 	case REQ_FSEQ_DATA:
 		list_move_tail(&rq->flush.list, &fq->flush_data_in_flight);
-		blk_mq_add_to_requeue_list(rq, 0);
+		spin_lock(&q->requeue_lock);
+		list_add_tail(&rq->queuelist, &q->flush_list);
+		spin_unlock(&q->requeue_lock);
 		blk_mq_kick_requeue_list(q);
 		break;
 
@@ -346,7 +348,10 @@ static void blk_kick_flush(struct request_queue *q, struct blk_flush_queue *fq,
 	smp_wmb();
 	req_ref_set(flush_rq, 1);
 
-	blk_mq_add_to_requeue_list(flush_rq, 0);
+	spin_lock(&q->requeue_lock);
+	list_add_tail(&flush_rq->queuelist, &q->flush_list);
+	spin_unlock(&q->requeue_lock);
+
 	blk_mq_kick_requeue_list(q);
 }
 
diff --git a/block/blk-mq-debugfs.c b/block/blk-mq-debugfs.c
index 22e39b9a77ecf2..68165a50951b68 100644
--- a/block/blk-mq-debugfs.c
+++ b/block/blk-mq-debugfs.c
@@ -244,7 +244,6 @@ static const char *const cmd_flag_name[] = {
 #define RQF_NAME(name) [ilog2((__force u32)RQF_##name)] = #name
 static const char *const rqf_name[] = {
 	RQF_NAME(STARTED),
-	RQF_NAME(SOFTBARRIER),
 	RQF_NAME(FLUSH_SEQ),
 	RQF_NAME(MIXED_MERGE),
 	RQF_NAME(MQ_INFLIGHT),
diff --git a/block/blk-mq.c b/block/blk-mq.c
index aac67bc3d3680c..551e7760f45e20 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1416,13 +1416,16 @@ static void __blk_mq_requeue_request(struct request *rq)
 void blk_mq_requeue_request(struct request *rq, bool kick_requeue_list)
 {
 	struct request_queue *q = rq->q;
+	unsigned long flags;
 
 	__blk_mq_requeue_request(rq);
 
 	/* this request will be re-inserted to io scheduler queue */
 	blk_mq_sched_requeue_request(rq);
 
-	blk_mq_add_to_requeue_list(rq, BLK_MQ_INSERT_AT_HEAD);
+	spin_lock_irqsave(&q->requeue_lock, flags);
+	list_add_tail(&rq->queuelist, &q->requeue_list);
+	spin_unlock_irqrestore(&q->requeue_lock, flags);
 
 	if (kick_requeue_list)
 		blk_mq_kick_requeue_list(q);
@@ -1434,13 +1437,16 @@ static void blk_mq_requeue_work(struct work_struct *work)
 	struct request_queue *q =
 		container_of(work, struct request_queue, requeue_work.work);
 	LIST_HEAD(rq_list);
-	struct request *rq, *next;
+	LIST_HEAD(flush_list);
+	struct request *rq;
 
 	spin_lock_irq(&q->requeue_lock);
 	list_splice_init(&q->requeue_list, &rq_list);
+	list_splice_init(&q->flush_list, &flush_list);
 	spin_unlock_irq(&q->requeue_lock);
 
-	list_for_each_entry_safe(rq, next, &rq_list, queuelist) {
+	while (!list_empty(&rq_list)) {
+		rq = list_entry(rq_list.next, struct request, queuelist);
 		/*
 		 * If RQF_DONTPREP ist set, the request has been started by the
 		 * driver already and might have driver-specific data allocated
@@ -1448,18 +1454,16 @@ static void blk_mq_requeue_work(struct work_struct *work)
 		 * block layer merges for the request.
 		 */
 		if (rq->rq_flags & RQF_DONTPREP) {
-			rq->rq_flags &= ~RQF_SOFTBARRIER;
 			list_del_init(&rq->queuelist);
 			blk_mq_request_bypass_insert(rq, 0);
-		} else if (rq->rq_flags & RQF_SOFTBARRIER) {
-			rq->rq_flags &= ~RQF_SOFTBARRIER;
+		} else {
 			list_del_init(&rq->queuelist);
 			blk_mq_insert_request(rq, BLK_MQ_INSERT_AT_HEAD);
 		}
 	}
 
-	while (!list_empty(&rq_list)) {
-		rq = list_entry(rq_list.next, struct request, queuelist);
+	while (!list_empty(&flush_list)) {
+		rq = list_entry(flush_list.next, struct request, queuelist);
 		list_del_init(&rq->queuelist);
 		blk_mq_insert_request(rq, 0);
 	}
@@ -1467,27 +1471,6 @@ static void blk_mq_requeue_work(struct work_struct *work)
 	blk_mq_run_hw_queues(q, false);
 }
 
-void blk_mq_add_to_requeue_list(struct request *rq, blk_insert_t insert_flags)
-{
-	struct request_queue *q = rq->q;
-	unsigned long flags;
-
-	/*
-	 * We abuse this flag that is otherwise used by the I/O scheduler to
-	 * request head insertion from the workqueue.
-	 */
-	BUG_ON(rq->rq_flags & RQF_SOFTBARRIER);
-
-	spin_lock_irqsave(&q->requeue_lock, flags);
-	if (insert_flags & BLK_MQ_INSERT_AT_HEAD) {
-		rq->rq_flags |= RQF_SOFTBARRIER;
-		list_add(&rq->queuelist, &q->requeue_list);
-	} else {
-		list_add_tail(&rq->queuelist, &q->requeue_list);
-	}
-	spin_unlock_irqrestore(&q->requeue_lock, flags);
-}
-
 void blk_mq_kick_requeue_list(struct request_queue *q)
 {
 	kblockd_mod_delayed_work_on(WORK_CPU_UNBOUND, &q->requeue_work, 0);
@@ -4239,6 +4222,7 @@ int blk_mq_init_allocated_queue(struct blk_mq_tag_set *set,
 	blk_mq_update_poll_flag(q);
 
 	INIT_DELAYED_WORK(&q->requeue_work, blk_mq_requeue_work);
+	INIT_LIST_HEAD(&q->flush_list);
 	INIT_LIST_HEAD(&q->requeue_list);
 	spin_lock_init(&q->requeue_lock);
 
diff --git a/block/blk-mq.h b/block/blk-mq.h
index ec7d2fb0b3c8ef..8c642e9f32f102 100644
--- a/block/blk-mq.h
+++ b/block/blk-mq.h
@@ -47,7 +47,6 @@ int blk_mq_update_nr_requests(struct request_queue *q, unsigned int nr);
 void blk_mq_wake_waiters(struct request_queue *q);
 bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *,
 			     unsigned int);
-void blk_mq_add_to_requeue_list(struct request *rq, blk_insert_t insert_flags);
 void blk_mq_flush_busy_ctxs(struct blk_mq_hw_ctx *hctx, struct list_head *list);
 struct request *blk_mq_dequeue_from_ctx(struct blk_mq_hw_ctx *hctx,
 					struct blk_mq_ctx *start);
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 935201c8974371..d778cb6b211233 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -28,8 +28,6 @@ typedef __u32 __bitwise req_flags_t;
 
 /* drive already may have started this one */
 #define RQF_STARTED		((__force req_flags_t)(1 << 1))
-/* may not be passed by ioscheduler */
-#define RQF_SOFTBARRIER		((__force req_flags_t)(1 << 3))
 /* request for flush sequence */
 #define RQF_FLUSH_SEQ		((__force req_flags_t)(1 << 4))
 /* merge of different types, fail separately */
@@ -65,7 +63,7 @@ typedef __u32 __bitwise req_flags_t;
 
 /* flags that prevent us from merging requests: */
 #define RQF_NOMERGE_FLAGS \
-	(RQF_STARTED | RQF_SOFTBARRIER | RQF_FLUSH_SEQ | RQF_SPECIAL_PAYLOAD)
+	(RQF_STARTED | RQF_FLUSH_SEQ | RQF_SPECIAL_PAYLOAD)
 
 enum mq_rq_state {
 	MQ_RQ_IDLE		= 0,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 3952c52d6cd1b0..fe99948688dfda 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -487,6 +487,7 @@ struct request_queue {
 	 * for flush operations
 	 */
 	struct blk_flush_queue	*fq;
+	struct list_head	flush_list;
 
 	struct list_head	requeue_list;
 	spinlock_t		requeue_lock;
-- 
2.39.2

Re: [PATCH 1/7] blk-mq: factor out a blk_rq_init_flush helper

[Bart Van Assche]

On 5/18/23 21:40, Christoph Hellwig wrote:
> Factor out a helper from blk_insert_flush that initializes the flush
> machine related fields in struct request, and don't bother with the
> full memset as there's just a few fields to initialize, and all but
> one already have explicit initializers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>   block/blk-flush.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/block/blk-flush.c b/block/blk-flush.c
> index 04698ed9bcd4a9..ed37d272f787eb 100644
> --- a/block/blk-flush.c
> +++ b/block/blk-flush.c
> @@ -376,6 +376,15 @@ static enum rq_end_io_ret mq_flush_data_end_io(struct request *rq,
>   	return RQ_END_IO_NONE;
>   }
>   
> +static void blk_rq_init_flush(struct request *rq)
> +{
> +	rq->flush.seq = 0;
> +	INIT_LIST_HEAD(&rq->flush.list);
> +	rq->rq_flags |= RQF_FLUSH_SEQ;
> +	rq->flush.saved_end_io = rq->end_io; /* Usually NULL */
> +	rq->end_io = mq_flush_data_end_io;
> +}
> +
>   /**
>    * blk_insert_flush - insert a new PREFLUSH/FUA request
>    * @rq: request to insert
> @@ -437,13 +446,7 @@ void blk_insert_flush(struct request *rq)
>   	 * @rq should go through flush machinery.  Mark it part of flush
>   	 * sequence and submit for further processing.
>   	 */
> -	memset(&rq->flush, 0, sizeof(rq->flush));
> -	INIT_LIST_HEAD(&rq->flush.list);
> -	rq->rq_flags |= RQF_FLUSH_SEQ;
> -	rq->flush.saved_end_io = rq->end_io; /* Usually NULL */
> -
> -	rq->end_io = mq_flush_data_end_io;
> -
> +	blk_rq_init_flush(rq);
>   	spin_lock_irq(&fq->mq_flush_lock);
>   	blk_flush_complete_seq(rq, fq, REQ_FSEQ_ACTIONS & ~policy, 0);
>   	spin_unlock_irq(&fq->mq_flush_lock);

Although I'm not sure it's useful to keep the "/* Usually NULL */" comment:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 2/7] blk-mq: reflow blk_insert_flush

[Bart Van Assche]

On 5/18/23 21:40, Christoph Hellwig wrote:
> diff --git a/block/blk-flush.c b/block/blk-flush.c
> index ed37d272f787eb..d8144f1f6fb12f 100644
> --- a/block/blk-flush.c
> +++ b/block/blk-flush.c
> @@ -402,6 +402,9 @@ void blk_insert_flush(struct request *rq)
>   	struct blk_flush_queue *fq = blk_get_flush_queue(q, rq->mq_ctx);
>   	struct blk_mq_hw_ctx *hctx = rq->mq_hctx;
>   
> +	/* FLUSH/FUA request must never be merged */
> +	WARN_ON_ONCE(rq->bio != rq->biotail);

request -> requests?

> -	/*
> -	 * If there's data but flush is not necessary, the request can be
> -	 * processed directly without going through flush machinery.  Queue
> -	 * for normal execution.
> -	 */
> -	if ((policy & REQ_FSEQ_DATA) &&
> -	    !(policy & (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH))) {
> +	case REQ_FSEQ_DATA:
> +		/*
> +		 * If there's data, but no flush is necessary, the request can
> +		 * be processed directly without going through flush machinery.
> +		 * Queue for normal execution.
> +		 */

there's -> there is. "there's" is a contraction of "there has". See also
https://dictionary.cambridge.org/dictionary/english/there-s.

Otherwise this patch looks good to me. Hence:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 5/7] blk-mq: defer to the normal submission path for post-flush requests

[Bart Van Assche]

On 5/18/23 21:40, Christoph Hellwig wrote:
> Requests with the FUA bit on hardware without FUA support need a post
> flush before returning to the caller, but they can still be sent using
> the normal I/O path after initializing the flush-related fields and
> end I/O handler.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 4/7] blk-mq: use the I/O scheduler for writes from the flush state machine

[Bart Van Assche]

On 5/18/23 21:40, Christoph Hellwig wrote:
>   	/*
>   	 * Three pointers are available for the IO schedulers, if they need
> -	 * more they have to dynamically allocate it.  Flush requests are
> -	 * never put on the IO scheduler. So let the flush fields share
> -	 * space with the elevator data.
> +	 * more they have to dynamically allocate it.
>   	 */

(commenting on my own patch) How about adding the words "private data" in the
above comment to improve clarity? That will change the above comment into:

	/*
	 * Three pointers are available for IO schedulers. If they need
	 * more private data they have to allocate it dynamically.
	 */

Re: [PATCH 7/7] blk-mq: don't use the requeue list to queue flush commands

[Bart Van Assche]

On 5/18/23 21:40, Christoph Hellwig wrote:
> Currently both requeues of commands that were already sent to the
> driver and flush commands submitted from the flush state machine
> share the same requeue_list struct request_queue, despite requeues
> doing head insertations and flushes not.  Switch to using two
> separate lists instead.

insertations -> insertions. See also https://www.google.com/search?q=insertation.

> @@ -1434,13 +1437,16 @@ static void blk_mq_requeue_work(struct work_struct *work)
>   	struct request_queue *q =
>   		container_of(work, struct request_queue, requeue_work.work);
>   	LIST_HEAD(rq_list);
> -	struct request *rq, *next;
> +	LIST_HEAD(flush_list);
> +	struct request *rq;
>   
>   	spin_lock_irq(&q->requeue_lock);
>   	list_splice_init(&q->requeue_list, &rq_list);
> +	list_splice_init(&q->flush_list, &flush_list);
>   	spin_unlock_irq(&q->requeue_lock);

"rq_list" stands for "request_list". That name is now confusing since this patch
add a second request list (flush_list).

Otherwise this patch looks good to me.

Thanks,

Bart.

Re: less special casing for flush requests v2

[Jens Axboe]

On Fri, 19 May 2023 06:40:43 +0200, Christoph Hellwig wrote:
> inspired by Barts quest for less reording during requeues, this series
> aims to limit the scope of requeues.  This is mostly done by simply
> sending down more than we currently do down the normal submission path
> with the help of Bart's patch to allow requests that are in the flush
> state machine to still be seen by the I/O schedulers.
> 
> Changes since v1:
>  - rebased
>  - fix a bug in the blk state machine insertation order
> 
> [...]

Applied, thanks!

[1/7] blk-mq: factor out a blk_rq_init_flush helper
      commit: 0b573692f19501dfe2aeaf37b272ec07f60c70b9
[2/7] blk-mq: reflow blk_insert_flush
      commit: c1075e548ce6e6b5c7b71f2b05d344164ebc52bb
[3/7] blk-mq: defer to the normal submission path for non-flush flush commands
      commit: 360f264834e34d08530c2fb9b67e3ffa65318761
[4/7] blk-mq: use the I/O scheduler for writes from the flush state machine
      commit: be4c427809b0a746aff54dbb8ef663f0184291d0
[5/7] blk-mq: defer to the normal submission path for post-flush requests
      commit: 615939a2ae734e3e68c816d6749d1f5f79c62ab7
[6/7] blk-mq: do not do head insertions post-pre-flush commands
      commit: 1e82fadfc6b96ca79f69d0bcf938d31032bb43d2
[7/7] blk-mq: don't use the requeue list to queue flush commands
      commit: 9a67aa52a42b31ad44220cc218df3b75a5cd5d05

Best regards,
-- 
Jens Axboe

Re: [PATCH 7/7] blk-mq: don't use the requeue list to queue flush commands

[Christoph Hellwig]

On Fri, May 19, 2023 at 12:55:45PM -0700, Bart Van Assche wrote:
>>   	LIST_HEAD(rq_list);
>> -	struct request *rq, *next;
>> +	LIST_HEAD(flush_list);
>> +	struct request *rq;
>>     	spin_lock_irq(&q->requeue_lock);
>>   	list_splice_init(&q->requeue_list, &rq_list);
>> +	list_splice_init(&q->flush_list, &flush_list);
>>   	spin_unlock_irq(&q->requeue_lock);
>
> "rq_list" stands for "request_list". That name is now confusing since this patch
> add a second request list (flush_list).

It is.  But I think you were planning on doing a bigger rework in this
area anyway?

Re: [PATCH 7/7] blk-mq: don't use the requeue list to queue flush commands

[Bart Van Assche]

On 5/19/23 21:56, Christoph Hellwig wrote:
> On Fri, May 19, 2023 at 12:55:45PM -0700, Bart Van Assche wrote:
>>>    	LIST_HEAD(rq_list);
>>> -	struct request *rq, *next;
>>> +	LIST_HEAD(flush_list);
>>> +	struct request *rq;
>>>      	spin_lock_irq(&q->requeue_lock);
>>>    	list_splice_init(&q->requeue_list, &rq_list);
>>> +	list_splice_init(&q->flush_list, &flush_list);
>>>    	spin_unlock_irq(&q->requeue_lock);
>>
>> "rq_list" stands for "request_list". That name is now confusing since this patch
>> add a second request list (flush_list).
> 
> It is.  But I think you were planning on doing a bigger rework in this
> area anyway?

Yes, that's right. I plan to remove both rq_list and flush_list from 
this function.

Bart.

Re: [PATCH 4/7] blk-mq: use the I/O scheduler for writes from the flush state machine

[Christoph Hellwig]

I turns out this causes some kind of hang I haven't been able to
debug yet in blktests' hotplug test.   Can you drop this and the
subsequent patches for now?

Re: [PATCH 4/7] blk-mq: use the I/O scheduler for writes from the flush state machine

[Bart Van Assche]

On 5/23/23 22:53, Christoph Hellwig wrote:
> I turns out this causes some kind of hang I haven't been able to
> debug yet in blktests' hotplug test.   Can you drop this and the
> subsequent patches for now?

Hi Christoph,

I haven't seen this hang in my tests. If you can tell me how to run 
blktests I can help with root-causing this issue. This is how I run 
blktests:

modprobe brd
echo "TEST_DEVS=(/dev/ram0)" > config
export use_siw=1
./check -q

Thanks,

Bart.

Re: [PATCH 4/7] blk-mq: use the I/O scheduler for writes from the flush state machine

[Christoph Hellwig]

On Wed, May 24, 2023 at 11:07:41AM -0700, Bart Van Assche wrote:
> On 5/23/23 22:53, Christoph Hellwig wrote:
>> I turns out this causes some kind of hang I haven't been able to
>> debug yet in blktests' hotplug test.   Can you drop this and the
>> subsequent patches for now?
>
> Hi Christoph,
>
> I haven't seen this hang in my tests. If you can tell me how to run 
> blktests I can help with root-causing this issue. This is how I run 
> blktests:

This is a simple ./check run with this config, and most importantly
modular scsi_debug (which is not my usual config, othewise I would
have noticed it earlier):

----- snip -----
TEST_DEVS=(/dev/vdb)
nvme_trtype=tcp
----- snip -----

It hangs in block/001 when probing scsi_debug:

[  242.790601] INFO: task modprobe:3702 blocked for more than 120 seconds.
[  242.791572]       Not tainted 6.4.0-rc2+ #1179
[  242.792201] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
message.
[  242.793387] task:modprobe        state:D stack:0     pid:3702  ppid:3686
flags:0x00004002
[  242.794724] Call Trace:
[  242.795121]  <TASK>
[  242.795465]  __schedule+0x307/0x840
[  242.796053]  ? call_usermodehelper_exec+0xee/0x180
[  242.796812]  schedule+0x57/0xa0
[  242.797316]  async_synchronize_full+0xa0/0x130
[  242.798029]  ? destroy_sched_domains_rcu+0x20/0x20
[  242.798803]  do_init_module+0x19f/0x200
[  242.799657]  __do_sys_finit_module+0x9e/0xf0
[  242.800324]  do_syscall_64+0x34/0x80
[  242.800879]  entry_SYSCALL_64_after_hwframe+0x63/0xcd

Re: [PATCH 4/7] blk-mq: use the I/O scheduler for writes from the flush state machine

[Bart Van Assche]

On 5/23/23 22:53, Christoph Hellwig wrote:
> I turns out this causes some kind of hang I haven't been able to
> debug yet in blktests' hotplug test.   Can you drop this and the
> subsequent patches for now?

Hi Christoph,

Are you perhaps referring to test block/027? It passes in my test-VM
with Jens' for-next branch and the following blktests config file:

TEST_DEVS=(/dev/vdb)

It seems to me that the entire patch series is still present on Jens'
for-next branch (commit 05ab4411bd69b809aa62c774866ce0b9be072418)?

Thanks,

Bart.

Re: [PATCH 4/7] blk-mq: use the I/O scheduler for writes from the flush state machine

[Christoph Hellwig]

On Sun, May 28, 2023 at 01:50:50PM -0700, Bart Van Assche wrote:
> On 5/23/23 22:53, Christoph Hellwig wrote:
>> I turns out this causes some kind of hang I haven't been able to
>> debug yet in blktests' hotplug test.   Can you drop this and the
>> subsequent patches for now?
>
> Hi Christoph,
>
> Are you perhaps referring to test block/027? It passes in my test-VM
> with Jens' for-next branch and the following blktests config file:

No, I mean block/001

Re: [PATCH 4/7] blk-mq: use the I/O scheduler for writes from the flush state machine

[Bart Van Assche]

On 5/30/23 07:55, Christoph Hellwig wrote:
> No, I mean block/001

Test block/001 also passes on my test setup. That setup is as follows:
* Kernel commit 0cac5b67c168 ("Merge branch 'for-6.5/block' into
   for-next").
* Multiple debugging options enabled in the kernel config (DEBUG_LIST,
   PROVE_LOCKING, KASAN, UBSAN, ...).

I ran this test twice: once with /dev/vdb as test device and once with 
/dev/ram0 as test device.

Bart.

Re: [PATCH 4/7] blk-mq: use the I/O scheduler for writes from the flush state machine

[Christoph Hellwig]

On Wed, May 31, 2023 at 06:34:49AM -0700, Bart Van Assche wrote:
> On 5/30/23 07:55, Christoph Hellwig wrote:
>> No, I mean block/001
>
> Test block/001 also passes on my test setup. That setup is as follows:
> * Kernel commit 0cac5b67c168 ("Merge branch 'for-6.5/block' into
>   for-next").
> * Multiple debugging options enabled in the kernel config (DEBUG_LIST,
>   PROVE_LOCKING, KASAN, UBSAN, ...).
>
> I ran this test twice: once with /dev/vdb as test device and once with 
> /dev/ram0 as test device.

Hi Bart,

I did a quick re-run on the latest for-6.5/block tree and can't reproduce
the hang.  Let me see if it is still there with my old branch or if
something else made it go away.