diff mbox series

[RFC,1/2] virtio-blk: add DISCARD and WRITE ZEROES features

Message ID 20190124172323.230296-2-sgarzare@redhat.com (mailing list archive)
State New, archived
Headers show
Series virtio-blk: add DISCARD and WRITE ZEROES features | expand

Commit Message

Stefano Garzarella Jan. 24, 2019, 5:23 p.m. UTC
This patch adds the support of DISCARD and WRITE ZEROES commands,
that have been introduced in the virtio-blk protocol to have
better performance when using SSD backend.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 hw/block/virtio-blk.c | 79 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)

Comments

Dr. David Alan Gilbert Jan. 24, 2019, 5:55 p.m. UTC | #1
* Stefano Garzarella (sgarzare@redhat.com) wrote:
> This patch adds the support of DISCARD and WRITE ZEROES commands,
> that have been introduced in the virtio-blk protocol to have
> better performance when using SSD backend.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>

Hi,
  Do you need to make those features machine-type dependent
so that a VM started on a nice new qemu with this feature doesn't
get confused when live migrated back to an older version?

Dave

> ---
>  hw/block/virtio-blk.c | 79 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 79 insertions(+)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index f208c6ddb9..8850957751 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -145,6 +145,25 @@ 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;
> +
> +    aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
> +    if (ret) {
> +        if (virtio_blk_handle_rw_error(req, -ret, 0)) {
> +            goto out;
> +        }
> +    }
> +
> +    virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
> +    virtio_blk_free_request(req);
> +
> +out:
> +    aio_context_release(blk_get_aio_context(s->conf.conf.blk));
> +}
> +
>  #ifdef __linux__
>  
>  typedef struct {
> @@ -584,6 +603,56 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>          virtio_blk_free_request(req);
>          break;
>      }
> +    /*
> +     * VIRTIO_BLK_T_DISCARD and VIRTIO_BLK_T_WRITE_ZEROES are defined with
> +     * VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch statement,
> +     * so we must mask it for these requests, then we will check the type.
> +     */
> +    case VIRTIO_BLK_T_DISCARD & ~VIRTIO_BLK_T_OUT:
> +    case VIRTIO_BLK_T_WRITE_ZEROES & ~VIRTIO_BLK_T_OUT:
> +    {
> +        struct virtio_blk_discard_write_zeroes dwz_hdr;
> +        uint64_t sector;
> +        int bytes;
> +
> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &dwz_hdr,
> +                                sizeof(dwz_hdr)) != sizeof(dwz_hdr))) {
> +            virtio_error(vdev, "virtio-blk discard/wzeroes header too short");
> +            return -1;
> +        }
> +
> +        sector = virtio_ldq_p(VIRTIO_DEVICE(req->dev), &dwz_hdr.sector);
> +        bytes = virtio_ldl_p(VIRTIO_DEVICE(req->dev),
> +                             &dwz_hdr.num_sectors) << BDRV_SECTOR_BITS;
> +
> +        if (!virtio_blk_sect_range_ok(req->dev, sector, bytes)) {
> +            virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> +            virtio_blk_free_request(req);
> +            return 0;
> +        }
> +
> +        if ((type & ~(VIRTIO_BLK_T_BARRIER)) == VIRTIO_BLK_T_DISCARD) {
> +            blk_aio_pdiscard(req->dev->blk, sector << BDRV_SECTOR_BITS, bytes,
> +                             virtio_blk_discard_wzeroes_complete, req);
> +        } else if ((type & ~(VIRTIO_BLK_T_BARRIER)) ==
> +                   VIRTIO_BLK_T_WRITE_ZEROES) {
> +            int flags = 0;
> +
> +            if (virtio_ldl_p(VIRTIO_DEVICE(req->dev), &dwz_hdr.flags) &
> +                VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) {
> +                flags |= BDRV_REQ_MAY_UNMAP;
> +            }
> +
> +            blk_aio_pwrite_zeroes(req->dev->blk, sector << BDRV_SECTOR_BITS,
> +                                  bytes, flags,
> +                                  virtio_blk_discard_wzeroes_complete, req);
> +        } else { /* Unsupported if VIRTIO_BLK_T_OUT is not set */
> +            virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> +            virtio_blk_free_request(req);
> +        }
> +
> +        break;
> +    }
>      default:
>          virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
>          virtio_blk_free_request(req);
> @@ -763,6 +832,14 @@ 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);
> +    virtio_stl_p(vdev, &blkcfg.max_discard_sectors, BDRV_REQUEST_MAX_SECTORS);
> +    virtio_stl_p(vdev, &blkcfg.max_discard_seg, 1);
> +    virtio_stl_p(vdev, &blkcfg.discard_sector_alignment,
> +                 blk_size >> BDRV_SECTOR_BITS);
> +    virtio_stl_p(vdev, &blkcfg.max_write_zeroes_sectors,
> +                 BDRV_REQUEST_MAX_SECTORS);
> +    virtio_stl_p(vdev, &blkcfg.max_write_zeroes_seg, 1);
> +    blkcfg.write_zeroes_may_unmap = 1;
>      memcpy(config, &blkcfg, sizeof(struct virtio_blk_config));
>  }
>  
> @@ -787,6 +864,8 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
>      virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
>      virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
>      virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
> +    virtio_add_feature(&features, VIRTIO_BLK_F_DISCARD);
> +    virtio_add_feature(&features, VIRTIO_BLK_F_WRITE_ZEROES);
>      if (virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
>          if (s->conf.scsi) {
>              error_setg(errp, "Please set scsi=off for virtio-blk devices in order to use virtio 1.0");
> -- 
> 2.20.1
> 
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Stefano Garzarella Jan. 24, 2019, 6:31 p.m. UTC | #2
On Thu, Jan 24, 2019 at 6:55 PM Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
>
> * Stefano Garzarella (sgarzare@redhat.com) wrote:
> > This patch adds the support of DISCARD and WRITE ZEROES commands,
> > that have been introduced in the virtio-blk protocol to have
> > better performance when using SSD backend.
> >
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
>
> Hi,
>   Do you need to make those features machine-type dependent
> so that a VM started on a nice new qemu with this feature doesn't
> get confused when live migrated back to an older version?
>

Hi Dave,
oh, thanks! I think the answer is absolutely yes :)
I'll fix it!

Another doubt that I have now is that I need to flush the
MultiReqBuffer before submitting DISCARD or WRITE ZEROES commands.

Thanks,
Stefano
Stefan Hajnoczi Jan. 25, 2019, 2:58 p.m. UTC | #3
On Thu, Jan 24, 2019 at 06:23:22PM +0100, Stefano Garzarella wrote:
> @@ -584,6 +603,56 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>          virtio_blk_free_request(req);
>          break;
>      }
> +    /*
> +     * VIRTIO_BLK_T_DISCARD and VIRTIO_BLK_T_WRITE_ZEROES are defined with
> +     * VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch statement,
> +     * so we must mask it for these requests, then we will check the type.
> +     */
> +    case VIRTIO_BLK_T_DISCARD & ~VIRTIO_BLK_T_OUT:
> +    case VIRTIO_BLK_T_WRITE_ZEROES & ~VIRTIO_BLK_T_OUT:
> +    {
> +        struct virtio_blk_discard_write_zeroes dwz_hdr;
> +        uint64_t sector;
> +        int bytes;
> +
> +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &dwz_hdr,
> +                                sizeof(dwz_hdr)) != sizeof(dwz_hdr))) {

"The data used for discard or write zeroes command is described by one
or more virtio_blk_discard_write_zeroes structs."

This needs to be a loop so that multiple dwz structs can be processed up
to the length of virtio_blk_req->data[].

> +            virtio_error(vdev, "virtio-blk discard/wzeroes header too short");
> +            return -1;
> +        }
> +
> +        sector = virtio_ldq_p(VIRTIO_DEVICE(req->dev), &dwz_hdr.sector);
> +        bytes = virtio_ldl_p(VIRTIO_DEVICE(req->dev),
> +                             &dwz_hdr.num_sectors) << BDRV_SECTOR_BITS;

Please handle num_sectors << BDRV_SECTOR_BITS overflow so that we don't
need to worry about what happens with an overflowed value later on.

> +
> +        if (!virtio_blk_sect_range_ok(req->dev, sector, bytes)) {
> +            virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> +            virtio_blk_free_request(req);
> +            return 0;
> +        }
> +
> +        if ((type & ~(VIRTIO_BLK_T_BARRIER)) == VIRTIO_BLK_T_DISCARD) {

Missing device requirement: "the device MUST set the status byte to VIRTIO_BLK_S_UNSUPP for discard commands if the unmap flag is set."

> +            blk_aio_pdiscard(req->dev->blk, sector << BDRV_SECTOR_BITS, bytes,
> +                             virtio_blk_discard_wzeroes_complete, req);
> +        } else if ((type & ~(VIRTIO_BLK_T_BARRIER)) ==
> +                   VIRTIO_BLK_T_WRITE_ZEROES) {
> +            int flags = 0;
> +
> +            if (virtio_ldl_p(VIRTIO_DEVICE(req->dev), &dwz_hdr.flags) &
> +                VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) {
> +                flags |= BDRV_REQ_MAY_UNMAP;
> +            }
> +
> +            blk_aio_pwrite_zeroes(req->dev->blk, sector << BDRV_SECTOR_BITS,
> +                                  bytes, flags,
> +                                  virtio_blk_discard_wzeroes_complete, req);

Please add block_acct_start(), this is treated as a write.

> +        } else { /* Unsupported if VIRTIO_BLK_T_OUT is not set */
> +            virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> +            virtio_blk_free_request(req);
> +        }
> +
> +        break;
> +    }
>      default:
>          virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
>          virtio_blk_free_request(req);
> @@ -763,6 +832,14 @@ 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);
> +    virtio_stl_p(vdev, &blkcfg.max_discard_sectors, BDRV_REQUEST_MAX_SECTORS);
> +    virtio_stl_p(vdev, &blkcfg.max_discard_seg, 1);
> +    virtio_stl_p(vdev, &blkcfg.discard_sector_alignment,
> +                 blk_size >> BDRV_SECTOR_BITS);
> +    virtio_stl_p(vdev, &blkcfg.max_write_zeroes_sectors,
> +                 BDRV_REQUEST_MAX_SECTORS);
> +    virtio_stl_p(vdev, &blkcfg.max_write_zeroes_seg, 1);
> +    blkcfg.write_zeroes_may_unmap = 1;

Please check hw/scsi/scsi-disk.c for an example of how to initialize
these limits.  It should be possible to set some of them via s->conf so
I'm surprised so many fields are hardcoded in this patch.
Stefano Garzarella Jan. 25, 2019, 4:18 p.m. UTC | #4
On Fri, Jan 25, 2019 at 02:58:56PM +0000, Stefan Hajnoczi wrote:
> On Thu, Jan 24, 2019 at 06:23:22PM +0100, Stefano Garzarella wrote:
> > @@ -584,6 +603,56 @@ static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
> >          virtio_blk_free_request(req);
> >          break;
> >      }
> > +    /*
> > +     * VIRTIO_BLK_T_DISCARD and VIRTIO_BLK_T_WRITE_ZEROES are defined with
> > +     * VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch statement,
> > +     * so we must mask it for these requests, then we will check the type.
> > +     */
> > +    case VIRTIO_BLK_T_DISCARD & ~VIRTIO_BLK_T_OUT:
> > +    case VIRTIO_BLK_T_WRITE_ZEROES & ~VIRTIO_BLK_T_OUT:
> > +    {
> > +        struct virtio_blk_discard_write_zeroes dwz_hdr;
> > +        uint64_t sector;
> > +        int bytes;
> > +
> > +        if (unlikely(iov_to_buf(out_iov, out_num, 0, &dwz_hdr,
> > +                                sizeof(dwz_hdr)) != sizeof(dwz_hdr))) {
> 
> "The data used for discard or write zeroes command is described by one
> or more virtio_blk_discard_write_zeroes structs."
> 
> This needs to be a loop so that multiple dwz structs can be processed up
> to the length of virtio_blk_req->data[].
> 

Since I set the "max_discard_seg" and "max_write_zeroes_seg" to 1, I
avoided the loop, but as you suggested, making their configurable, I will
add the loop. Thanks!

> > +            virtio_error(vdev, "virtio-blk discard/wzeroes header too short");
> > +            return -1;
> > +        }
> > +
> > +        sector = virtio_ldq_p(VIRTIO_DEVICE(req->dev), &dwz_hdr.sector);
> > +        bytes = virtio_ldl_p(VIRTIO_DEVICE(req->dev),
> > +                             &dwz_hdr.num_sectors) << BDRV_SECTOR_BITS;
> 
> Please handle num_sectors << BDRV_SECTOR_BITS overflow so that we don't
> need to worry about what happens with an overflowed value later on.
> 

Should I also check if num_sectors is less than "max_discard_sectors" or
"max_write_zeroes_sectors"?

> > +
> > +        if (!virtio_blk_sect_range_ok(req->dev, sector, bytes)) {
> > +            virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
> > +            virtio_blk_free_request(req);
> > +            return 0;
> > +        }
> > +
> > +        if ((type & ~(VIRTIO_BLK_T_BARRIER)) == VIRTIO_BLK_T_DISCARD) {
> 
> Missing device requirement: "the device MUST set the status byte to VIRTIO_BLK_S_UNSUPP for discard commands if the unmap flag is set."
> 

Ooh, thanks! I'll fix it!

> > +            blk_aio_pdiscard(req->dev->blk, sector << BDRV_SECTOR_BITS, bytes,
> > +                             virtio_blk_discard_wzeroes_complete, req);
> > +        } else if ((type & ~(VIRTIO_BLK_T_BARRIER)) ==
> > +                   VIRTIO_BLK_T_WRITE_ZEROES) {
> > +            int flags = 0;
> > +
> > +            if (virtio_ldl_p(VIRTIO_DEVICE(req->dev), &dwz_hdr.flags) &
> > +                VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) {
> > +                flags |= BDRV_REQ_MAY_UNMAP;
> > +            }
> > +
> > +            blk_aio_pwrite_zeroes(req->dev->blk, sector << BDRV_SECTOR_BITS,
> > +                                  bytes, flags,
> > +                                  virtio_blk_discard_wzeroes_complete, req);
> 
> Please add block_acct_start(), this is treated as a write.
> 

Sure!

> > +        } else { /* Unsupported if VIRTIO_BLK_T_OUT is not set */
> > +            virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> > +            virtio_blk_free_request(req);
> > +        }
> > +
> > +        break;
> > +    }
> >      default:
> >          virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> >          virtio_blk_free_request(req);
> > @@ -763,6 +832,14 @@ 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);
> > +    virtio_stl_p(vdev, &blkcfg.max_discard_sectors, BDRV_REQUEST_MAX_SECTORS);
> > +    virtio_stl_p(vdev, &blkcfg.max_discard_seg, 1);
> > +    virtio_stl_p(vdev, &blkcfg.discard_sector_alignment,
> > +                 blk_size >> BDRV_SECTOR_BITS);
> > +    virtio_stl_p(vdev, &blkcfg.max_write_zeroes_sectors,
> > +                 BDRV_REQUEST_MAX_SECTORS);
> > +    virtio_stl_p(vdev, &blkcfg.max_write_zeroes_seg, 1);
> > +    blkcfg.write_zeroes_may_unmap = 1;
> 
> Please check hw/scsi/scsi-disk.c for an example of how to initialize
> these limits.  It should be possible to set some of them via s->conf so
> I'm surprised so many fields are hardcoded in this patch.

Sorry for that! I'll make some of them configurable!

Last question:
Do you think make sense flush the MultiReqBuffer before call
blk_aio_pdiscard() or blk_aio_pwrite_zeroes()?

Thanks,
Stefano
Stefan Hajnoczi Jan. 27, 2019, 12:51 p.m. UTC | #5
On Fri, Jan 25, 2019 at 05:18:13PM +0100, Stefano Garzarella wrote:
> On Fri, Jan 25, 2019 at 02:58:56PM +0000, Stefan Hajnoczi wrote:
> > On Thu, Jan 24, 2019 at 06:23:22PM +0100, Stefano Garzarella wrote:
> > > +            virtio_error(vdev, "virtio-blk discard/wzeroes header too short");
> > > +            return -1;
> > > +        }
> > > +
> > > +        sector = virtio_ldq_p(VIRTIO_DEVICE(req->dev), &dwz_hdr.sector);
> > > +        bytes = virtio_ldl_p(VIRTIO_DEVICE(req->dev),
> > > +                             &dwz_hdr.num_sectors) << BDRV_SECTOR_BITS;
> > 
> > Please handle num_sectors << BDRV_SECTOR_BITS overflow so that we don't
> > need to worry about what happens with an overflowed value later on.
> > 
> 
> Should I also check if num_sectors is less than "max_discard_sectors" or
> "max_write_zeroes_sectors"?

Yes, anything that virtio_blk_sect_range_ok() doesn't check needs to be
checked manually in this patch.  I can't see a reason for allowing
requests through that exceed the limit.

> Do you think make sense flush the MultiReqBuffer before call
> blk_aio_pdiscard() or blk_aio_pwrite_zeroes()?

I don't think that is necessary since virtio-blk doesn't guarantee
ordering (the legacy barrier feature isn't supported by QEMU).

Even the MultiReqBuffer is flushed, -drive aio=threads (the default
setting) could end up submitting discard/write zeroes before "earlier"
requests - it depends on host kernel thread scheduler decisions.

Stefan
Stefano Garzarella Jan. 28, 2019, 8:28 a.m. UTC | #6
On Sun, Jan 27, 2019 at 12:51:44PM +0000, Stefan Hajnoczi wrote:
> On Fri, Jan 25, 2019 at 05:18:13PM +0100, Stefano Garzarella wrote:
> > On Fri, Jan 25, 2019 at 02:58:56PM +0000, Stefan Hajnoczi wrote:
> > > On Thu, Jan 24, 2019 at 06:23:22PM +0100, Stefano Garzarella wrote:
> > > > +            virtio_error(vdev, "virtio-blk discard/wzeroes header too short");
> > > > +            return -1;
> > > > +        }
> > > > +
> > > > +        sector = virtio_ldq_p(VIRTIO_DEVICE(req->dev), &dwz_hdr.sector);
> > > > +        bytes = virtio_ldl_p(VIRTIO_DEVICE(req->dev),
> > > > +                             &dwz_hdr.num_sectors) << BDRV_SECTOR_BITS;
> > > 
> > > Please handle num_sectors << BDRV_SECTOR_BITS overflow so that we don't
> > > need to worry about what happens with an overflowed value later on.
> > > 
> > 
> > Should I also check if num_sectors is less than "max_discard_sectors" or
> > "max_write_zeroes_sectors"?
> 
> Yes, anything that virtio_blk_sect_range_ok() doesn't check needs to be
> checked manually in this patch.  I can't see a reason for allowing
> requests through that exceed the limit.
> 
> > Do you think make sense flush the MultiReqBuffer before call
> > blk_aio_pdiscard() or blk_aio_pwrite_zeroes()?
> 
> I don't think that is necessary since virtio-blk doesn't guarantee
> ordering (the legacy barrier feature isn't supported by QEMU).
> 
> Even the MultiReqBuffer is flushed, -drive aio=threads (the default
> setting) could end up submitting discard/write zeroes before "earlier"
> requests - it depends on host kernel thread scheduler decisions.
> 

Thanks for the details!

Stefano
diff mbox series

Patch

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f208c6ddb9..8850957751 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -145,6 +145,25 @@  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;
+
+    aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
+    if (ret) {
+        if (virtio_blk_handle_rw_error(req, -ret, 0)) {
+            goto out;
+        }
+    }
+
+    virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
+    virtio_blk_free_request(req);
+
+out:
+    aio_context_release(blk_get_aio_context(s->conf.conf.blk));
+}
+
 #ifdef __linux__
 
 typedef struct {
@@ -584,6 +603,56 @@  static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
         virtio_blk_free_request(req);
         break;
     }
+    /*
+     * VIRTIO_BLK_T_DISCARD and VIRTIO_BLK_T_WRITE_ZEROES are defined with
+     * VIRTIO_BLK_T_OUT flag set. We masked this flag in the switch statement,
+     * so we must mask it for these requests, then we will check the type.
+     */
+    case VIRTIO_BLK_T_DISCARD & ~VIRTIO_BLK_T_OUT:
+    case VIRTIO_BLK_T_WRITE_ZEROES & ~VIRTIO_BLK_T_OUT:
+    {
+        struct virtio_blk_discard_write_zeroes dwz_hdr;
+        uint64_t sector;
+        int bytes;
+
+        if (unlikely(iov_to_buf(out_iov, out_num, 0, &dwz_hdr,
+                                sizeof(dwz_hdr)) != sizeof(dwz_hdr))) {
+            virtio_error(vdev, "virtio-blk discard/wzeroes header too short");
+            return -1;
+        }
+
+        sector = virtio_ldq_p(VIRTIO_DEVICE(req->dev), &dwz_hdr.sector);
+        bytes = virtio_ldl_p(VIRTIO_DEVICE(req->dev),
+                             &dwz_hdr.num_sectors) << BDRV_SECTOR_BITS;
+
+        if (!virtio_blk_sect_range_ok(req->dev, sector, bytes)) {
+            virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
+            virtio_blk_free_request(req);
+            return 0;
+        }
+
+        if ((type & ~(VIRTIO_BLK_T_BARRIER)) == VIRTIO_BLK_T_DISCARD) {
+            blk_aio_pdiscard(req->dev->blk, sector << BDRV_SECTOR_BITS, bytes,
+                             virtio_blk_discard_wzeroes_complete, req);
+        } else if ((type & ~(VIRTIO_BLK_T_BARRIER)) ==
+                   VIRTIO_BLK_T_WRITE_ZEROES) {
+            int flags = 0;
+
+            if (virtio_ldl_p(VIRTIO_DEVICE(req->dev), &dwz_hdr.flags) &
+                VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) {
+                flags |= BDRV_REQ_MAY_UNMAP;
+            }
+
+            blk_aio_pwrite_zeroes(req->dev->blk, sector << BDRV_SECTOR_BITS,
+                                  bytes, flags,
+                                  virtio_blk_discard_wzeroes_complete, req);
+        } else { /* Unsupported if VIRTIO_BLK_T_OUT is not set */
+            virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
+            virtio_blk_free_request(req);
+        }
+
+        break;
+    }
     default:
         virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
         virtio_blk_free_request(req);
@@ -763,6 +832,14 @@  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);
+    virtio_stl_p(vdev, &blkcfg.max_discard_sectors, BDRV_REQUEST_MAX_SECTORS);
+    virtio_stl_p(vdev, &blkcfg.max_discard_seg, 1);
+    virtio_stl_p(vdev, &blkcfg.discard_sector_alignment,
+                 blk_size >> BDRV_SECTOR_BITS);
+    virtio_stl_p(vdev, &blkcfg.max_write_zeroes_sectors,
+                 BDRV_REQUEST_MAX_SECTORS);
+    virtio_stl_p(vdev, &blkcfg.max_write_zeroes_seg, 1);
+    blkcfg.write_zeroes_may_unmap = 1;
     memcpy(config, &blkcfg, sizeof(struct virtio_blk_config));
 }
 
@@ -787,6 +864,8 @@  static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
     virtio_add_feature(&features, VIRTIO_BLK_F_GEOMETRY);
     virtio_add_feature(&features, VIRTIO_BLK_F_TOPOLOGY);
     virtio_add_feature(&features, VIRTIO_BLK_F_BLK_SIZE);
+    virtio_add_feature(&features, VIRTIO_BLK_F_DISCARD);
+    virtio_add_feature(&features, VIRTIO_BLK_F_WRITE_ZEROES);
     if (virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
         if (s->conf.scsi) {
             error_setg(errp, "Please set scsi=off for virtio-blk devices in order to use virtio 1.0");