On Tue, May 21, 2019 at 09:01:40AM +0200, Christoph Hellwig wrote:
> Currently ll_merge_requests_fn, unlike all other merge functions,
> reduces nr_phys_segments by one if the last segment of the previous,
> and the first segment of the next segement are contigous. While this
> seems like a nice solution to avoid building smaller than possible
> requests it causes a mismatch between the segments actually present
> in the request and those iterated over by the bvec iterators, including
> __rq_for_each_bio. This can for example mistrigger the single segment
> optimization in the nvme-pci driver, and might lead to mismatching
> nr_phys_segments number when recalculating the number of request
> when inserting a cloned request.
>
> We could possibly work around this by making the bvec iterators take
> the front and back segment size into account, but that would require
> moving them from the bio to the bio_iter and spreading this mess
> over all users of bvecs. Or we could simply remove this optimization
> under the assumption that most users already build good enough bvecs,
> and that the bio merge patch never cared about this optimization
> either. The latter is what this patch does.
>
> dff824b2aadb ("nvme-pci: optimize mapping of small single segment requests").
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reviewed-by: Hannes Reinecke <hare@xxxxxxxx>
> ---
> block/blk-merge.c | 23 +----------------------
> 1 file changed, 1 insertion(+), 22 deletions(-)
>
> diff --git a/block/blk-merge.c b/block/blk-merge.c
> index 21e87a714a73..80a5a0facb87 100644
> --- a/block/blk-merge.c
> +++ b/block/blk-merge.c
> @@ -358,7 +358,6 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
> unsigned front_seg_size;
> struct bio *fbio, *bbio;
> struct bvec_iter iter;
> - bool new_bio = false;
>
> if (!bio)
> return 0;
> @@ -379,31 +378,12 @@ static unsigned int __blk_recalc_rq_segments(struct request_queue *q,
> nr_phys_segs = 0;
> for_each_bio(bio) {
> bio_for_each_bvec(bv, bio, iter) {
> - if (new_bio) {
> - if (seg_size + bv.bv_len
> - > queue_max_segment_size(q))
> - goto new_segment;
> - if (!biovec_phys_mergeable(q, &bvprv, &bv))
> - goto new_segment;
> -
> - seg_size += bv.bv_len;
> -
> - if (nr_phys_segs == 1 && seg_size >
> - front_seg_size)
> - front_seg_size = seg_size;
> -
> - continue;
> - }
> -new_segment:
> bvec_split_segs(q, &bv, &nr_phys_segs, &seg_size,
> &front_seg_size, NULL, UINT_MAX);
> - new_bio = false;
> }
> bbio = bio;
> - if (likely(bio->bi_iter.bi_size)) {
> + if (likely(bio->bi_iter.bi_size))
> bvprv = bv;
> - new_bio = true;
> - }
> }
>
> fbio->bi_seg_front_size = front_seg_size;
> @@ -725,7 +705,6 @@ static int ll_merge_requests_fn(struct request_queue *q, struct request *req,
> req->bio->bi_seg_front_size = seg_size;
> if (next->nr_phys_segments == 1)
> next->biotail->bi_seg_back_size = seg_size;
> - total_phys_segments--;
> }
>
> if (total_phys_segments > queue_max_segments(q))
> --
> 2.20.1
>
Reviewed-by: Ming Lei <ming.lei@xxxxxxxxxx>
Thanks,
Ming
