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