diff mbox series

[PATCHv5,4/9] blk-integrity: consider entire bio list for merging

Message ID 20240913182854.2445457-5-kbusch@meta.com
State New
Headers show
Series block integrity merging and counting | expand

Commit Message

Keith Busch Sept. 13, 2024, 6:28 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.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Martin K. Petersen <martin.petersen@oracle.com>
Signed-off-by: Keith Busch <kbusch@kernel.org>
---
 block/blk-integrity.c | 4 ----
 1 file changed, 4 deletions(-)

Comments

Christoph Hellwig Sept. 14, 2024, 7:30 a.m. UTC | #1 
On Fri, Sep 13, 2024 at 11:28:49AM -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 I explained in detail last round this is still wrong.  There is
no bio list here ever.
Keith Busch Sept. 14, 2024, 4:51 p.m. UTC | #2 
On Sat, Sep 14, 2024 at 09:30:12AM +0200, Christoph Hellwig wrote:
> On Fri, Sep 13, 2024 at 11:28:49AM -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 I explained in detail last round this is still wrong.  There is
> no bio list here ever.

Could you explain "wrong"? If we assume there is never bio list here,
then the current code is performing a useless no-op, and this patch
removes it. That's a good thing, no?
Christoph Hellwig Sept. 16, 2024, 6:44 a.m. UTC | #3 
On Sat, Sep 14, 2024 at 10:51:10AM -0600, Keith Busch wrote:
> On Sat, Sep 14, 2024 at 09:30:12AM +0200, Christoph Hellwig wrote:
> > On Fri, Sep 13, 2024 at 11:28:49AM -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 I explained in detail last round this is still wrong.  There is
> > no bio list here ever.
> 
> Could you explain "wrong"? If we assume there is never bio list here,
> then the current code is performing a useless no-op, and this patch
> removes it. That's a good thing, no?

The wrong thing primarily is the above commit log.  The code change
itself is correct, but we'd be better off killing the iteration over
the bio chain as well to make the code less confusing.
Keith Busch Sept. 16, 2024, 8:40 a.m. UTC | #4 
On Mon, Sep 16, 2024 at 08:44:13AM +0200, Christoph Hellwig wrote:
> The wrong thing primarily is the above commit log.  The code change
> itself is correct, but we'd be better off killing the iteration over
> the bio chain as well to make the code less confusing.

Okay, gotcha. I mentioned the existing code is a no op in "most cases"
but it should just be "always". Maybe a sanity check with a WARN here is
appropriate too.

BTW, sorry for mislabeling the tags. The patch numbers changed a few
times in this series, and I pasted reviews from the a different message.
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;