| Message ID | 20240913182854.2445457-5-kbusch@meta.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | block integrity merging and counting | expand |
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.
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?
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.
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 --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;