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

[Stefano Garzarella]

On Fri, Feb 01, 2019 at 12:58:31PM +0800, Stefan Hajnoczi wrote:
> 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/
> 

Thanks! I'll follow all of these suggestions.

> > +    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.
> 

Thanks! I'll change all the name.

> > @@ -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.

Make sense, I'll drop this option.

Only a question about options: I used a single option "dwz_max_sectors"
for both "max_discard_sectors" and "max_write_zeroes_sectors".
Since I'll include two options to enable/disable discard and
write_zeroes features, do you think make sense to split this
configurable option in two?

Thanks,
Stefano