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

[Stefan Hajnoczi]

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.

signature.asc
Description: PGP signature