diff mbox series

[PATCHv4,04/10] blk-integrity: consider entire bio list for merging

Message ID 20240911201240.3982856-5-kbusch@meta.com (mailing list archive)
State Superseded
Headers show
Series block integrity merging and counting | expand

Commit Message

Keith Busch Sept. 11, 2024, 8:12 p.m. UTC
From: Keith Busch <kbusch@kernel.org>

If a bio is merged to a request, the entire bio list is merged, so don't
temporarily detach it from its list when counting segments. In most
cases, bi_next will already be NULL, so detaching is usually a no-op.
But if the bio does have a list, the current code is miscounting the
segments for the resulting merge.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-integrity.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Christoph Hellwig Sept. 12, 2024, 7:42 a.m. UTC | #1
On Wed, Sep 11, 2024 at 01:12:34PM -0700, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
> 
> If a bio is merged to a request, the entire bio list is merged, so don't
> temporarily detach it from its list when counting segments. In most
> cases, bi_next will already be NULL, so detaching is usually a no-op.
> But if the bio does have a list, the current code is miscounting the
> segments for the resulting merge.

As far as I can tell we can never get here with bi_next set.  Rationale:

blk_integrity_merge_bio has two callers: ll_new_hw_segment and
blk_rq_merge_ok.

ll_new_hw_segment is called from ll_back_merge_fn and ll_front_merge_fn.

ll_back_merge_fn is called from blk_rq_append_bio and
bio_attempt_back_merge.

blk_rq_append_bio is always used for a single bio and in fact used
to build bio chains.

bio_attempt_back_merge is called from blk_attempt_bio_merge,
blk_mq_sched_try_merge and blk_zone_write_plug_init_request.

blk_attempt_bio_merge is called from blk_attempt_plug_merge and
blk_bio_list_merge.

blk_attempt_plug_merge is called from blk_mq_attempt_bio_merge,
which always operates on a single bio.

blk_bio_list_merge is called from blk_mq_sched_bio_merge,
kyber_bio_merge where the latter is just an indirect call from
the former, and blk_mq_sched_bio_merge is called from
blk_mq_attempt_bio_merge which was considered above.

blk_mq_sched_try_merge is called from bfq_bio_merge and dd_bio_merge,
both of which are implementation of the ->bio_merge elevator_mq_ops
method called from blk_mq_sched_bio_merge, which was considered above.

blk_zone_write_plug_init_request is called from blk_mq_submit_bio
and always operates on a single bio.

ll_front_merge_fn is called from bio_attempt_front_merge.

bio_attempt_front_merge is called from blk_attempt_bio_merge and
blk_mq_sched_try_merge, both of which were considered above.

blk_rq_merge_ok is called from blk_attempt_bio_merge,
blk_zone_write_plug_init_request and elv_bio_merge_ok.

blk_attempt_bio_merge and blk_zone_write_plug_init_request were
already considered above.

elv_bio_merge_ok is called from bfq_request_merge and dd_request_merge
and elv_merge.  The first two are implementations of the ->request_merge
elevator_mq_ops method called from elv_merge, which is called from
blk_mq_sched_try_merge, which was considered above.

Also it feels like the call in blk_rq_merge_ok is superflous from this.
diff mbox series

Patch

diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index afd101555d3cb..84065691aaed0 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -134,7 +134,6 @@  bool blk_integrity_merge_bio(struct request_queue *q, struct request *req,
 			     struct bio *bio)
 {
 	int nr_integrity_segs;
-	struct bio *next = bio->bi_next;
 
 	if (blk_integrity_rq(req) == 0 && bio_integrity(bio) == NULL)
 		return true;
@@ -145,10 +144,7 @@  bool blk_integrity_merge_bio(struct request_queue *q, struct request *req,
 	if (bio_integrity(req->bio)->bip_flags != bio_integrity(bio)->bip_flags)
 		return false;
 
-	bio->bi_next = NULL;
 	nr_integrity_segs = blk_rq_count_integrity_sg(q, bio);
-	bio->bi_next = next;
-
 	if (req->nr_integrity_segments + nr_integrity_segs >
 	    q->limits.max_integrity_segments)
 		return false;