Hi Ming,
On Tue, Aug 11, 2020 at 05:21:34PM +0800, Ming Lei wrote:
> 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support") starts
> to support multi-range discard for virtio-blk. However, the virtio-blk
> disk may report max discard segment as 1, at least that is exactly what
> qemu is doing.
>
> So far, block layer switches to normal request merge if max discard segment
> limit is 1, and multiple bios can be merged to single segment. This way may
> cause memory corruption in virtblk_setup_discard_write_zeroes().
>
> Fix the issue by handling single max discard segment in straightforward
> way.
>
> Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> Fixes: 1f23816b8eb8 ("virtio_blk: add discard and write zeroes support")
> Cc: Christoph Hellwig <hch@xxxxxx>
> Cc: Changpeng Liu <changpeng.liu@xxxxxxxxx>
> Cc: Daniel Verkamp <dverkamp@xxxxxxxxxxxx>
> Cc: Michael S. Tsirkin <mst@xxxxxxxxxx>
> Cc: Stefan Hajnoczi <stefanha@xxxxxxxxxx>
> ---
> drivers/block/virtio_blk.c | 23 +++++++++++++++--------
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 63b213e00b37..05b01903122b 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c'
> @@ -126,14 +126,21 @@ static int virtblk_setup_discard_write_zeroes(struct request *req, bool unmap)
> if (!range)
> return -ENOMEM;
We are allocating the 'range' array to contain 'segments' elements.
When queue_max_discard_segments() returns 1, should we limit 'segments'
to 1?
>
> - __rq_for_each_bio(bio, req) {
> - u64 sector = bio->bi_iter.bi_sector;
> - u32 num_sectors = bio->bi_iter.bi_size >> SECTOR_SHIFT;
> -
> - range[n].flags = cpu_to_le32(flags);
> - range[n].num_sectors = cpu_to_le32(num_sectors);
> - range[n].sector = cpu_to_le64(sector);
> - n++;
> + if (queue_max_discard_segments(req->q) == 1) {
> + range[0].flags = cpu_to_le32(flags);
> + range[0].num_sectors = cpu_to_le32(blk_rq_sectors(req));
> + range[0].sector = cpu_to_le64(blk_rq_pos(req));
> + n = 1;
^
this doesn't seem necessary since we don't use 'n' afterwards.
Thanks,
Stefano
> + } else {
> + __rq_for_each_bio(bio, req) {
> + u64 sector = bio->bi_iter.bi_sector;
> + u32 num_sectors = bio->bi_iter.bi_size >> SECTOR_SHIFT;
> +
> + range[n].flags = cpu_to_le32(flags);
> + range[n].num_sectors = cpu_to_le32(num_sectors);
> + range[n].sector = cpu_to_le64(sector);
> + n++;
> + }
> }
>
> req->special_vec.bv_page = virt_to_page(range);
> --
> 2.25.2
>
