Re: [PATCH 1/4] block: don't decrement nr_phys_segments for physically contigous segments

[Ming Lei <ming.lei@redhat.com>]

Hi Christoph,

On Thu, May 16, 2019 at 10:40:55AM +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 could cause overwrites of too small kmalloc
> allocations in any driver using ranged discard, or also mistrigger
> the single segment optimization in the nvme-pci driver.
> 
> 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.
> 
> Fixes: b35ba01ea697 ("nvme: support ranged discard requests")
> Fixes: 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support")

ll_merge_requests_fn() is only called from attempt_merge() in case
that ELEVATOR_BACK_MERGE is returned from blk_try_req_merge(). However,
for discard merge of both virtio_blk and nvme, ELEVATOR_DISCARD_MERGE is
always returned from blk_try_req_merge() in attempt_merge(), so looks
ll_merge_requests_fn() shouldn't be called for virtio_blk/nvme's discard
request. Just wondering if you may explain a bit how the change on
ll_merge_requests_fn() in this patch makes a difference on the above
two commits?

> Fixes: 297910571f08 ("nvme-pci: optimize mapping single segment requests using SGLs")

I guess it should be dff824b2aadb ("nvme-pci: optimize mapping of small
single segment requests").

Yes, this patch helps for this case, cause blk_rq_nr_phys_segments() may be 1
but there are two bios which share same segment.

Thanks,
Ming