[PATCH 0/2] Fix bio merge off cached request path

[PATCH 0/2] Fix bio merge off cached request path

[Jens Axboe]

Hi,

#1 is just a cleanup, #2 fixes the real issue here that's leading to
crashes for me.

-- 
Jens Axboe

[PATCH 1/2] block: get rid of useless goto and label in blk_mq_get_new_requests()

[Jens Axboe]

Expected case is returning a request, just check for success and return
the request rather than having an error label.

Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-mq.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index ecfc47fad236..ca33cb755c5f 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2720,11 +2720,8 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
 	}
 
 	rq = __blk_mq_alloc_requests(&data);
-	if (!rq)
-		goto fail;
-	return rq;
-
-fail:
+	if (rq)
+		return rq;
 	rq_qos_cleanup(q, bio);
 	if (bio->bi_opf & REQ_NOWAIT)
 		bio_wouldblock_error(bio);
-- 
2.34.1

Re: [PATCH 1/2] block: get rid of useless goto and label in blk_mq_get_new_requests()

[Christoph Hellwig]

On Thu, Dec 02, 2021 at 12:47:40PM -0700, Jens Axboe wrote:
> Expected case is returning a request, just check for success and return
> the request rather than having an error label.

I have to say the current flow is much easier to read, but the new
version is indeed shorter.

[PATCH 2/2] block: fix double bio queue when merging in cached request path

[Jens Axboe]

When we attempt to merge off the cached request path, we return NULL
if successful. This makes the caller believe that it's should allocate
a new request, and hence we end up with the bio both merged and associated
with a new request. This, predictably, leads to all sorts of crashes.

Pass in a pointer to the bio pointer, and clear it for the merge case.
Then the caller knows that the bio is already queued, and no new requests
need to get allocated.

Fixes: 5b13bc8a3fd5 ("blk-mq: cleanup request allocation")
Signed-off-by: Jens Axboe <axboe@kernel.dk>
---
 block/blk-mq.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index ca33cb755c5f..fc4520e992b1 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -2731,7 +2731,7 @@ static struct request *blk_mq_get_new_requests(struct request_queue *q,
 }
 
 static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
-		struct blk_plug *plug, struct bio *bio, unsigned int nsegs)
+		struct blk_plug *plug, struct bio **bio, unsigned int nsegs)
 {
 	struct request *rq;
 
@@ -2741,19 +2741,21 @@ static inline struct request *blk_mq_get_cached_request(struct request_queue *q,
 	if (!rq || rq->q != q)
 		return NULL;
 
-	if (unlikely(!submit_bio_checks(bio)))
+	if (unlikely(!submit_bio_checks(*bio)))
 		return NULL;
-	if (blk_mq_attempt_bio_merge(q, bio, nsegs))
+	if (blk_mq_attempt_bio_merge(q, *bio, nsegs)) {
+		*bio = NULL;
 		return NULL;
-	if (blk_mq_get_hctx_type(bio->bi_opf) != rq->mq_hctx->type)
+	}
+	if (blk_mq_get_hctx_type((*bio)->bi_opf) != rq->mq_hctx->type)
 		return NULL;
-	if (op_is_flush(rq->cmd_flags) != op_is_flush(bio->bi_opf))
+	if (op_is_flush(rq->cmd_flags) != op_is_flush((*bio)->bi_opf))
 		return NULL;
 
-	rq->cmd_flags = bio->bi_opf;
+	rq->cmd_flags = (*bio)->bi_opf;
 	plug->cached_rq = rq_list_next(rq);
 	INIT_LIST_HEAD(&rq->queuelist);
-	rq_qos_throttle(q, bio);
+	rq_qos_throttle(q, *bio);
 	return rq;
 }
 
@@ -2789,8 +2791,10 @@ void blk_mq_submit_bio(struct bio *bio)
 	if (!bio_integrity_prep(bio))
 		return;
 
-	rq = blk_mq_get_cached_request(q, plug, bio, nr_segs);
+	rq = blk_mq_get_cached_request(q, plug, &bio, nr_segs);
 	if (!rq) {
+		if (!bio)
+			return;
 		rq = blk_mq_get_new_requests(q, plug, bio, nr_segs);
 		if (unlikely(!rq))
 			return;
-- 
2.34.1

Re: [PATCH 2/2] block: fix double bio queue when merging in cached request path

[Ming Lei]

On Thu, Dec 02, 2021 at 12:47:41PM -0700, Jens Axboe wrote:
> When we attempt to merge off the cached request path, we return NULL
> if successful. This makes the caller believe that it's should allocate
> a new request, and hence we end up with the bio both merged and associated
> with a new request. This, predictably, leads to all sorts of crashes.
> 
> Pass in a pointer to the bio pointer, and clear it for the merge case.
> Then the caller knows that the bio is already queued, and no new requests
> need to get allocated.
> 
> Fixes: 5b13bc8a3fd5 ("blk-mq: cleanup request allocation")
> Signed-off-by: Jens Axboe <axboe@kernel.dk>

Io hang in io_uring workload can't be triggered any more with this
patch:

Reviewed-by: Ming Lei <ming.lei@redhat.com>


-- 
Ming

Re: [PATCH 2/2] block: fix double bio queue when merging in cached request path

[Christoph Hellwig]

Looks fine:

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