Re: [Qemu-devel] [PATCH v2 3/5] virtio-blk: add DISCARD and WRITE ZEROES features

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3528 bytes --]

On Thu, Jan 31, 2019 at 04:19:12PM +0100, Stefano Garzarella wrote:
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 542ec52536..34ee676895 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -147,6 +147,30 @@ out:
>      aio_context_release(blk_get_aio_context(s->conf.conf.blk));
>  }
>  
> +static void virtio_blk_discard_wzeroes_complete(void *opaque, int ret)
> +{
> +    VirtIOBlockReq *req = opaque;
> +    VirtIOBlock *s = req->dev;
> +    bool is_wzeroes = (virtio_ldl_p(VIRTIO_DEVICE(req->dev), &req->out.type) &

s/req->dev/s/

> +                       ~VIRTIO_BLK_T_BARRIER) == VIRTIO_BLK_T_WRITE_ZEROES;
> +
> +    aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
> +    if (ret) {
> +        if (virtio_blk_handle_rw_error(req, -ret, 0, is_wzeroes)) {

The third argument is bool, please use false instead of 0.

> +            goto out;
> +        }
> +    }
> +
> +    virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
> +    if (is_wzeroes) {
> +        block_acct_done(blk_get_stats(req->dev->blk), &req->acct);

s/req->dev->blk/s->blk/

> +static uint8_t virtio_blk_handle_dwz(VirtIOBlockReq *req, bool is_wzeroes,
> +    struct virtio_blk_discard_write_zeroes *dwz_hdr)
> +{
> +    VirtIOBlock *s = req->dev;
> +    uint64_t sector;
> +    uint32_t num_sectors, flags;
> +    uint8_t err_status;
> +    int bytes;
> +
> +    sector = virtio_ldq_p(VIRTIO_DEVICE(req->dev), &dwz_hdr->sector);

Here and throughout the rest of the function:

  VirtIODevice *vdev = VIRTIO_DEVICE(s);

s/VIRTIO_DEVICE(req->dev)/vdev/

and then to clean up the remaining instances:

s/req->dev/s/

> +    if (is_wzeroes) { /* VIRTIO_BLK_T_WRITE_ZEROES */
> +        int blk_aio_flags = 0;
> +
> +        if (s->conf.wz_may_unmap &&

The inconsistent naming is a bit messy and could confuse readers:
write_zeroes vs wzeroes vs wz

The VIRTIO spec and QEMU code uses write_zeroes, please stick to that
even though it is longer.

s/is_wzeroes/is_write_zeroes/
s/wz_map_unmap/write_zeroes_may_unmap/
s/virtio_blk_discard_wzeroes_complete/virtio_blk_discard_write_zeroes_complete/

This is a question of style and a local dwz_hdr variable does make the
code easier to read, so I'm not totally against shortening the name, but
please consistently use the long form in user-visible options, struct
field names, and function names because these things have a large scope.

> @@ -765,6 +904,22 @@ static void virtio_blk_update_config(VirtIODevice *vdev, uint8_t *config)
>      blkcfg.alignment_offset = 0;
>      blkcfg.wce = blk_enable_write_cache(s->blk);
>      virtio_stw_p(vdev, &blkcfg.num_queues, s->conf.num_queues);
> +    if (s->conf.discard_wzeroes) {
> +        virtio_stl_p(vdev, &blkcfg.max_discard_sectors,
> +                     s->conf.dwz_max_sectors);
> +        virtio_stl_p(vdev, &blkcfg.discard_sector_alignment,
> +                     blk_size >> BDRV_SECTOR_BITS);
> +        virtio_stl_p(vdev, &blkcfg.max_write_zeroes_sectors,
> +                     s->conf.dwz_max_sectors);
> +        blkcfg.write_zeroes_may_unmap = s->conf.wz_may_unmap;

Does this need to be an option since MAY_UNMAP is advisory anyway?

Another way of asking: what happens in the worst case if the guest sends
MAY_UNMAP but the QEMU block device doesn't support unmap?

Dropping this option would make the user interface simpler (no need to
worry about the flag) and the implementation too.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]