On Fri, Nov 12, 2021 at 08:47:01AM -0700, Jens Axboe wrote:

On 11/12/21 5:47 AM, Ming Lei wrote:

quoted

On Fri, Nov 12, 2021 at 09:44:41AM +0100, Christoph Hellwig wrote:

quoted

On Fri, Nov 12, 2021 at 04:37:19PM +0800, Ming Lei wrote:

quoted

quoted

can only be used for reads, and no fua can be set if the preallocating
I/O didn't use fua, etc.

What are the pitfalls of just chanigng cmd_flags?

Then we need to check cmd_flags carefully, such as hctx->type has to
be same, flush & passthrough flags has to be same, that said all
->cmd_flags used for allocating rqs have to be same with the following
bio->bi_opf.

In usual cases, I guess all IOs submitted from same plug batch should be
same type. If not, we can switch to change cmd_flags.

Jens: is this a limit fitting into your use cases?

I guess as a quick fix this rejecting different flags is probably the
best we can do for now, but I suspect we'll want to eventually relax
them.

rw mixed workload will be affected, so I think we need to switch to
change cmd_flags, how about the following patch?

From 9ab77b7adee768272944c20b7cffc8abdb85a35b Mon Sep 17 00:00:00 2001
From: Ming Lei <redacted>
Date: Fri, 12 Nov 2021 08:14:38 +0800
Subject: [PATCH] blk-mq: fix filesystem I/O request allocation

submit_bio_checks() may update bio->bi_opf, so we have to initialize
blk_mq_alloc_data.cmd_flags with bio->bi_opf after submit_bio_checks()
returns when allocating new request.

In case of using cached request, fallback to allocate new request if
cached rq isn't compatible with the incoming bio, otherwise change
rq->cmd_flags with incoming bio->bi_opf.

Fixes: 900e080752025f00 ("block: move queue enter logic into blk_mq_submit_bio()")
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Ming Lei <redacted>
---
 block/blk-mq.c | 39 ++++++++++++++++++++++++++++++---------
 block/blk-mq.h | 26 +++++++++++++++-----------
 2 files changed, 45 insertions(+), 20 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index f511db395c7f..3ab34c4f20da 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c

@@ -2521,12 +2521,8 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
 	};
 	struct request *rq;
 
-	if (unlikely(bio_queue_enter(bio)))
-		return NULL;
-	if (unlikely(!submit_bio_checks(bio)))
-		goto put_exit;
 	if (blk_mq_attempt_bio_merge(q, bio, nsegs, same_queue_rq))
-		goto put_exit;
+		return NULL;
 
 	rq_qos_throttle(q, bio);
 

@@ -2543,19 +2539,32 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
 	rq_qos_cleanup(q, bio);
 	if (bio->bi_opf & REQ_NOWAIT)
 		bio_wouldblock_error(bio);
-put_exit:
-	blk_queue_exit(q);
+
 	return NULL;
 }
 
+static inline bool blk_mq_can_use_cached_rq(struct request *rq,
+		struct bio *bio)
+{
+	if (blk_mq_get_hctx_type(bio->bi_opf) != rq->mq_hctx->type)
+		return false;
+
+	if (op_is_flush(rq->cmd_flags) != op_is_flush(bio->bi_opf))
+		return false;
+
+	return true;

I think we should just check if hctx is the same, that should be enough.
We don't need to match the type, just disallow if hw queue has changed.

But bio doesn't have hw queue. Figuring out exact hw queue seems
necessary and needs more cpu cycles than getting hctx type.


Thanks,
Ming