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

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

[Stefano Garzarella]

This series adds the support of DISCARD and WRITE ZEROES commands
and extends the virtio-blk-test to test WRITE_ZEROES command when
the feature is enabled.

v2:
- added patch 1 to use virtio_blk_handle_rw_error() with discard operation
- added patch 2 to make those new features machine-type dependent (thanks David)
- fixed patch 3 (previously patch 1/2) adding more checks, block_acct_start()
                for WRITE_ZEROES requests, and configurable parameters to
                initialize the limits (max_sectors, wzeroes_may_unmap).
                (thanks Stefan)
                I moved in a new function the code to handle a single segment,
                in order to simplify the support of multiple segments in the
                future.
- added patch 4 to change the assert on data_size following the discussion with
                Thomas, Changpeng, Michael, and Stefan (thanks all)
- fixed patch 5 (previously patch 2/2) using local dwz_hdr variable instead of
                dynamic allocation (thanks Thomas)

This series requires the new virtio headers from linux v5.0-rc1
already imported by Paolo:

Based-on: <20190104082731.24967-1-pbonzini@redhat.com>

Thanks,
Stefano

Stefano Garzarella (5):
  virtio-blk: add acct_failed param to virtio_blk_handle_rw_error()
  virtio-blk: add "discard-wzeroes" boolean property
  virtio-blk: add DISCARD and WRITE ZEROES features
  tests/virtio-blk: change assert on data_size in virtio_blk_request()
  tests/virtio-blk: add test for WRITE_ZEROES command

 hw/block/virtio-blk.c          | 185 ++++++++++++++++++++++++++++++++-
 hw/core/machine.c              |   1 +
 include/hw/virtio/virtio-blk.h |   3 +
 tests/virtio-blk-test.c        |  75 ++++++++++++-
 4 files changed, 259 insertions(+), 5 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v2 1/5] virtio-blk: add acct_failed param to virtio_blk_handle_rw_error()

[Stefano Garzarella]

We add acct_failed param in order to use virtio_blk_handle_rw_error()
also when is not required to call block_acct_failed(). (eg. a discard
operation is failed)

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

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f208c6ddb9..8a6754d9a2 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -61,7 +61,7 @@ static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
 }
 
 static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
-    bool is_read)
+    bool is_read, bool acct_failed)
 {
     BlockErrorAction action = blk_get_error_action(req->dev->blk,
                                                    is_read, error);
@@ -75,7 +75,9 @@ static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
         s->rq = req;
     } else if (action == BLOCK_ERROR_ACTION_REPORT) {
         virtio_blk_req_complete(req, VIRTIO_BLK_S_IOERR);
-        block_acct_failed(blk_get_stats(s->blk), &req->acct);
+        if (acct_failed) {
+            block_acct_failed(blk_get_stats(s->blk), &req->acct);
+        }
         virtio_blk_free_request(req);
     }
 
@@ -113,7 +115,7 @@ static void virtio_blk_rw_complete(void *opaque, int ret)
              * the memory until the request is completed (which will
              * happen on the other side of the migration).
              */
-            if (virtio_blk_handle_rw_error(req, -ret, is_read)) {
+            if (virtio_blk_handle_rw_error(req, -ret, is_read, true)) {
                 continue;
             }
         }
@@ -132,7 +134,7 @@ static void virtio_blk_flush_complete(void *opaque, int ret)
 
     aio_context_acquire(blk_get_aio_context(s->conf.conf.blk));
     if (ret) {
-        if (virtio_blk_handle_rw_error(req, -ret, 0)) {
+        if (virtio_blk_handle_rw_error(req, -ret, 0, true)) {
             goto out;
         }
     }
-- 
2.20.1

[Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Stefano Garzarella]

In order to avoid migration issues, we enable DISCARD and
WRITE ZEROES features only for machine type >= 4.0

Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 hw/block/virtio-blk.c          | 2 ++
 hw/core/machine.c              | 1 +
 include/hw/virtio/virtio-blk.h | 1 +
 3 files changed, 4 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 8a6754d9a2..542ec52536 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = {
     DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
     DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
                      IOThread *),
+    DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0,
+                     true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 2629515363..ce98857af0 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -30,6 +30,7 @@ GlobalProperty hw_compat_3_1[] = {
     { "memory-backend-memfd", "x-use-canonical-path-for-ramblock-id", "true" },
     { "tpm-crb", "ppi", "false" },
     { "tpm-tis", "ppi", "false" },
+    { "virtio-blk-device", "discard-wzeroes", "false" },
 };
 const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1);
 
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 5117431d96..c336afb4cd 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -40,6 +40,7 @@ struct VirtIOBlkConf
     uint32_t request_merging;
     uint16_t num_queues;
     uint16_t queue_size;
+    uint32_t discard_wzeroes;
 };
 
 struct VirtIOBlockDataPlane;
-- 
2.20.1

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

[Stefano Garzarella]

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.

We support only one segment per request since multiple segments
are not widely used and there are no userspace APIs that allow
applications to submit multiple segments in a single call.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 hw/block/virtio-blk.c          | 173 +++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-blk.h |   2 +
 2 files changed, 175 insertions(+)

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) &
+                       ~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)) {
+            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);
+    }
+    virtio_blk_free_request(req);
+
+out:
+    aio_context_release(blk_get_aio_context(s->conf.conf.blk));
+}
+
 #ifdef __linux__
 
 typedef struct {
@@ -480,6 +504,82 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
     return true;
 }
 
+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);
+    num_sectors = virtio_ldl_p(VIRTIO_DEVICE(req->dev), &dwz_hdr->num_sectors);
+    flags = virtio_ldl_p(VIRTIO_DEVICE(req->dev), &dwz_hdr->flags);
+
+    /*
+     * dwz_max_sectors is at most BDRV_REQUEST_MAX_SECTORS, this check
+     * make us sure that "num_sectors << BDRV_SECTOR_BITS" can fit in
+     * the integer variable.
+     */
+    if (unlikely(num_sectors > s->conf.dwz_max_sectors)) {
+        err_status = VIRTIO_BLK_S_IOERR;
+        goto err;
+    }
+
+    bytes = num_sectors << BDRV_SECTOR_BITS;
+
+    if (unlikely(!virtio_blk_sect_range_ok(req->dev, sector, bytes))) {
+        err_status = VIRTIO_BLK_S_IOERR;
+        goto err;
+    }
+
+    /*
+     * The device MUST set the status byte to VIRTIO_BLK_S_UNSUPP for discard
+     * and write zeroes commands if any unknown flag is set.
+     */
+    if (unlikely(flags & ~VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP)) {
+        err_status = VIRTIO_BLK_S_UNSUPP;
+        goto err;
+    }
+
+    if (is_wzeroes) { /* VIRTIO_BLK_T_WRITE_ZEROES */
+        int blk_aio_flags = 0;
+
+        if (s->conf.wz_may_unmap &&
+            flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) {
+            blk_aio_flags |= BDRV_REQ_MAY_UNMAP;
+        }
+
+        block_acct_start(blk_get_stats(req->dev->blk), &req->acct, bytes,
+                         BLOCK_ACCT_WRITE);
+
+        blk_aio_pwrite_zeroes(req->dev->blk, sector << BDRV_SECTOR_BITS,
+                              bytes, blk_aio_flags,
+                              virtio_blk_discard_wzeroes_complete, req);
+    } else { /* VIRTIO_BLK_T_DISCARD */
+        /*
+         * The device MUST set the status byte to VIRTIO_BLK_S_UNSUPP for
+         * discard commands if the unmap flag is set.
+         */
+        if (unlikely(flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP)) {
+            err_status = VIRTIO_BLK_S_UNSUPP;
+            goto err;
+        }
+
+        blk_aio_pdiscard(req->dev->blk, sector << BDRV_SECTOR_BITS, bytes,
+                         virtio_blk_discard_wzeroes_complete, req);
+    }
+
+    return VIRTIO_BLK_S_OK;
+
+err:
+    if (is_wzeroes) {
+        block_acct_invalid(blk_get_stats(req->dev->blk), BLOCK_ACCT_WRITE);
+    }
+    return err_status;
+}
+
 static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 {
     uint32_t type;
@@ -586,6 +686,45 @@ 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 if it is set.
+     */
+    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;
+        size_t out_len = iov_size(out_iov, out_num);
+        bool is_wzeroes = (type & ~VIRTIO_BLK_T_BARRIER) ==
+                          VIRTIO_BLK_T_WRITE_ZEROES;
+        uint8_t err_status;
+
+        /*
+         * Unsupported if VIRTIO_BLK_T_OUT is not set or the request contains
+         * more than one segment.
+         */
+        if (unlikely(!(type & VIRTIO_BLK_T_OUT) ||
+                     out_len > sizeof(dwz_hdr))) {
+            virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
+            virtio_blk_free_request(req);
+            return 0;
+        }
+
+        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;
+        }
+
+        err_status = virtio_blk_handle_dwz(req, is_wzeroes, &dwz_hdr);
+        if (err_status != VIRTIO_BLK_S_OK) {
+            virtio_blk_req_complete(req, err_status);
+            virtio_blk_free_request(req);
+        }
+
+        break;
+    }
     default:
         virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
         virtio_blk_free_request(req);
@@ -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;
+        /*
+         * We support only one segment per request since multiple segments
+         * are not widely used and there are no userspace APIs that allow
+         * applications to submit multiple segments in a single call.
+         */
+        virtio_stl_p(vdev, &blkcfg.max_discard_seg, 1);
+        virtio_stl_p(vdev, &blkcfg.max_write_zeroes_seg, 1);
+    }
     memcpy(config, &blkcfg, sizeof(struct virtio_blk_config));
 }
 
@@ -811,6 +966,10 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
     if (s->conf.num_queues > 1) {
         virtio_add_feature(&features, VIRTIO_BLK_F_MQ);
     }
+    if (s->conf.discard_wzeroes) {
+        virtio_add_feature(&features, VIRTIO_BLK_F_DISCARD);
+        virtio_add_feature(&features, VIRTIO_BLK_F_WRITE_ZEROES);
+    }
 
     return features;
 }
@@ -956,6 +1115,16 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (conf->discard_wzeroes) {
+        if (!conf->dwz_max_sectors ||
+            conf->dwz_max_sectors > BDRV_REQUEST_MAX_SECTORS) {
+            error_setg(errp, "invalid dwz-max-sectors property (%" PRIu32 "), "
+                       "must be between 1 and %lu",
+                       conf->dwz_max_sectors, BDRV_REQUEST_MAX_SECTORS);
+            return;
+        }
+    }
+
     virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
                 sizeof(struct virtio_blk_config));
 
@@ -1028,6 +1197,10 @@ static Property virtio_blk_properties[] = {
                      IOThread *),
     DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0,
                      true),
+    DEFINE_PROP_UINT32("discard-wzeroes-max-sectors", VirtIOBlock,
+                       conf.dwz_max_sectors, BDRV_REQUEST_MAX_SECTORS),
+    DEFINE_PROP_BIT("wzeroes-may-unmap", VirtIOBlock, conf.wz_may_unmap, 0,
+                    true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index c336afb4cd..4e9d4434ff 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -41,6 +41,8 @@ struct VirtIOBlkConf
     uint16_t num_queues;
     uint16_t queue_size;
     uint32_t discard_wzeroes;
+    uint32_t dwz_max_sectors;
+    uint32_t wz_may_unmap;
 };
 
 struct VirtIOBlockDataPlane;
-- 
2.20.1

[Qemu-devel] [PATCH v2 4/5] tests/virtio-blk: change assert on data_size in virtio_blk_request()

[Stefano Garzarella]

The size of data in the virtio_blk_request must be a multiple
of 512 bytes for IN and OUT requests, or a multiple of the size
of struct virtio_blk_discard_write_zeroes for DISCARD and
WRITE_ZEROES requests.

Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 tests/virtio-blk-test.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 04c608764b..0739498da7 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -144,7 +144,20 @@ static uint64_t virtio_blk_request(QGuestAllocator *alloc, QVirtioDevice *d,
     uint64_t addr;
     uint8_t status = 0xFF;
 
-    g_assert_cmpuint(data_size % 512, ==, 0);
+    switch (req->type) {
+    case VIRTIO_BLK_T_IN:
+    case VIRTIO_BLK_T_OUT:
+        g_assert_cmpuint(data_size % 512, ==, 0);
+        break;
+    case VIRTIO_BLK_T_DISCARD:
+    case VIRTIO_BLK_T_WRITE_ZEROES:
+        g_assert_cmpuint(data_size %
+                         sizeof(struct virtio_blk_discard_write_zeroes), ==, 0);
+        break;
+    default:
+        g_assert_cmpuint(data_size, ==, 0);
+    }
+
     addr = guest_alloc(alloc, sizeof(*req) + data_size);
 
     virtio_blk_fix_request(d, req);
-- 
2.20.1

[Qemu-devel] [PATCH v2 5/5] tests/virtio-blk: add test for WRITE_ZEROES command

[Stefano Garzarella]

If the WRITE_ZEROES feature is enabled, we check this command
in the test_basic().

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

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 0739498da7..35bd92dbfc 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -244,6 +244,66 @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
 
     guest_free(alloc, req_addr);
 
+    if (features & (1u << VIRTIO_BLK_F_WRITE_ZEROES)) {
+        struct virtio_blk_discard_write_zeroes dwz_hdr;
+        void *expected;
+
+        /*
+         * WRITE_ZEROES request on the same sector of previous test where
+         * we wrote "TEST".
+         */
+        req.type = VIRTIO_BLK_T_WRITE_ZEROES;
+        req.data = (char *) &dwz_hdr;
+        dwz_hdr.sector = 0;
+        dwz_hdr.num_sectors = 1;
+        dwz_hdr.flags = 0;
+
+        req_addr = virtio_blk_request(alloc, dev, &req, sizeof(dwz_hdr));
+
+        free_head = qvirtqueue_add(vq, req_addr, 16, false, true);
+        qvirtqueue_add(vq, req_addr + 16, sizeof(dwz_hdr), false, true);
+        qvirtqueue_add(vq, req_addr + 16 + sizeof(dwz_hdr), 1, true, false);
+
+        qvirtqueue_kick(dev, vq, free_head);
+
+        qvirtio_wait_used_elem(dev, vq, free_head, NULL,
+                               QVIRTIO_BLK_TIMEOUT_US);
+        status = readb(req_addr + 16 + sizeof(dwz_hdr));
+        g_assert_cmpint(status, ==, 0);
+
+        guest_free(alloc, req_addr);
+
+        /* Read request to check if the sector contains all zeroes */
+        req.type = VIRTIO_BLK_T_IN;
+        req.ioprio = 1;
+        req.sector = 0;
+        req.data = g_malloc0(512);
+
+        req_addr = virtio_blk_request(alloc, dev, &req, 512);
+
+        g_free(req.data);
+
+        free_head = qvirtqueue_add(vq, req_addr, 16, false, true);
+        qvirtqueue_add(vq, req_addr + 16, 512, true, true);
+        qvirtqueue_add(vq, req_addr + 528, 1, true, false);
+
+        qvirtqueue_kick(dev, vq, free_head);
+
+        qvirtio_wait_used_elem(dev, vq, free_head, NULL,
+                               QVIRTIO_BLK_TIMEOUT_US);
+        status = readb(req_addr + 528);
+        g_assert_cmpint(status, ==, 0);
+
+        data = g_malloc(512);
+        expected = g_malloc0(512);
+        memread(req_addr + 16, data, 512);
+        g_assert_cmpmem(data, 512, expected, 512);
+        g_free(expected);
+        g_free(data);
+
+        guest_free(alloc, req_addr);
+    }
+
     if (features & (1u << VIRTIO_F_ANY_LAYOUT)) {
         /* Write and read with 2 descriptor layout */
         /* Write request */
-- 
2.20.1

Re: [Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Dr. David Alan Gilbert]

* Stefano Garzarella (sgarzare@redhat.com) wrote:
> In order to avoid migration issues, we enable DISCARD and
> WRITE ZEROES features only for machine type >= 4.0
> 
> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  hw/block/virtio-blk.c          | 2 ++
>  hw/core/machine.c              | 1 +
>  include/hw/virtio/virtio-blk.h | 1 +
>  3 files changed, 4 insertions(+)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 8a6754d9a2..542ec52536 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = {
>      DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
>      DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
>                       IOThread *),
> +    DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0,
> +                     true),

I think that's OK, but do you really want a DEFINE_PROP_BOOL and
a bool discard_wzeroes?
I think DEFINE_PROP_BIT is mostly used for a flag word where each
property is one more bit in the field.

Dave

>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 2629515363..ce98857af0 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -30,6 +30,7 @@ GlobalProperty hw_compat_3_1[] = {
>      { "memory-backend-memfd", "x-use-canonical-path-for-ramblock-id", "true" },
>      { "tpm-crb", "ppi", "false" },
>      { "tpm-tis", "ppi", "false" },
> +    { "virtio-blk-device", "discard-wzeroes", "false" },
>  };
>  const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1);
>  
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 5117431d96..c336afb4cd 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -40,6 +40,7 @@ struct VirtIOBlkConf
>      uint32_t request_merging;
>      uint16_t num_queues;
>      uint16_t queue_size;
> +    uint32_t discard_wzeroes;
>  };
>  
>  struct VirtIOBlockDataPlane;
> -- 
> 2.20.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Stefano Garzarella]

On Thu, Jan 31, 2019 at 03:40:38PM +0000, Dr. David Alan Gilbert wrote:
> * Stefano Garzarella (sgarzare@redhat.com) wrote:
> > In order to avoid migration issues, we enable DISCARD and
> > WRITE ZEROES features only for machine type >= 4.0
> > 
> > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> >  hw/block/virtio-blk.c          | 2 ++
> >  hw/core/machine.c              | 1 +
> >  include/hw/virtio/virtio-blk.h | 1 +
> >  3 files changed, 4 insertions(+)
> > 
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 8a6754d9a2..542ec52536 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = {
> >      DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
> >      DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
> >                       IOThread *),
> > +    DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0,
> > +                     true),
> 
> I think that's OK, but do you really want a DEFINE_PROP_BOOL and
> a bool discard_wzeroes?
> I think DEFINE_PROP_BIT is mostly used for a flag word where each
> property is one more bit in the field.
> 
> Dave
> 

Hi Dave,
I was in doubt if to use DEFINE_PROP_BIT or DEFINE_PROP_BOOL, but
looking in the virtio-blk.c, I found that also other boolean like
"config-wce", "scsi", and "request-merging" was defined with
DEFINE_PROP_BIT, so I followed this trand.

But I agree with you, DEFINE_PROP_BOOL should be better, so I will change it!

Thanks,
Stefano

Re: [Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Dr. David Alan Gilbert]

* Stefano Garzarella (sgarzare@redhat.com) wrote:
> On Thu, Jan 31, 2019 at 03:40:38PM +0000, Dr. David Alan Gilbert wrote:
> > * Stefano Garzarella (sgarzare@redhat.com) wrote:
> > > In order to avoid migration issues, we enable DISCARD and
> > > WRITE ZEROES features only for machine type >= 4.0
> > > 
> > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > ---
> > >  hw/block/virtio-blk.c          | 2 ++
> > >  hw/core/machine.c              | 1 +
> > >  include/hw/virtio/virtio-blk.h | 1 +
> > >  3 files changed, 4 insertions(+)
> > > 
> > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > index 8a6754d9a2..542ec52536 100644
> > > --- a/hw/block/virtio-blk.c
> > > +++ b/hw/block/virtio-blk.c
> > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = {
> > >      DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
> > >      DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
> > >                       IOThread *),
> > > +    DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0,
> > > +                     true),
> > 
> > I think that's OK, but do you really want a DEFINE_PROP_BOOL and
> > a bool discard_wzeroes?
> > I think DEFINE_PROP_BIT is mostly used for a flag word where each
> > property is one more bit in the field.
> > 
> > Dave
> > 
> 
> Hi Dave,
> I was in doubt if to use DEFINE_PROP_BIT or DEFINE_PROP_BOOL, but
> looking in the virtio-blk.c, I found that also other boolean like
> "config-wce", "scsi", and "request-merging" was defined with
> DEFINE_PROP_BIT, so I followed this trand.

Oh odd, I don't see why it's done like that - unless that's visible to
the guest somehow directly.  The _BIT version is more commonly used
when you have a flag word that's got multiple bits defined in it.

> But I agree with you, DEFINE_PROP_BOOL should be better, so I will change it!

Thanks!

Dave

> Thanks,
> Stefano
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

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

[Michael S. Tsirkin]

On Thu, Jan 31, 2019 at 04:19:12PM +0100, Stefano Garzarella 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.
> 
> We support only one segment per request since multiple segments
> are not widely used and there are no userspace APIs that allow
> applications to submit multiple segments in a single call.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>


This does not seem to match the spec clarifications
that Stefan Hajnoczi has posted on
the virtio TC.

I think this can go any of the following ways:

- we agree that a request should have at most one segment
	no padding allowed
- we agree that a request should have at most one segment
	we require padding to 512 bytes
- we agree that a request should have at most one segment
	we also support padding to 512 bytes
- we agree that a request should have at most one segment
	we also support arbitrary padding
- we agree that a request can have any # of segments

I would also need your feedback on whether all this
is a material change to 1.1 public review according to the oasis definition:

	"Material Change" is any change to the content of a Work Product that
	that would require a compliant application or implementation to be
	modified or rewritten in order to remain compliant or which adds new
	features or otherwise expands the scope of the work product.


> ---
>  hw/block/virtio-blk.c          | 173 +++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-blk.h |   2 +
>  2 files changed, 175 insertions(+)
> 
> 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) &
> +                       ~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)) {
> +            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);
> +    }
> +    virtio_blk_free_request(req);
> +
> +out:
> +    aio_context_release(blk_get_aio_context(s->conf.conf.blk));
> +}
> +
>  #ifdef __linux__
>  
>  typedef struct {
> @@ -480,6 +504,82 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
>      return true;
>  }
>  
> +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);
> +    num_sectors = virtio_ldl_p(VIRTIO_DEVICE(req->dev), &dwz_hdr->num_sectors);
> +    flags = virtio_ldl_p(VIRTIO_DEVICE(req->dev), &dwz_hdr->flags);
> +
> +    /*
> +     * dwz_max_sectors is at most BDRV_REQUEST_MAX_SECTORS, this check
> +     * make us sure that "num_sectors << BDRV_SECTOR_BITS" can fit in
> +     * the integer variable.
> +     */
> +    if (unlikely(num_sectors > s->conf.dwz_max_sectors)) {
> +        err_status = VIRTIO_BLK_S_IOERR;
> +        goto err;
> +    }
> +
> +    bytes = num_sectors << BDRV_SECTOR_BITS;
> +
> +    if (unlikely(!virtio_blk_sect_range_ok(req->dev, sector, bytes))) {
> +        err_status = VIRTIO_BLK_S_IOERR;
> +        goto err;
> +    }
> +
> +    /*
> +     * The device MUST set the status byte to VIRTIO_BLK_S_UNSUPP for discard
> +     * and write zeroes commands if any unknown flag is set.
> +     */
> +    if (unlikely(flags & ~VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP)) {
> +        err_status = VIRTIO_BLK_S_UNSUPP;
> +        goto err;
> +    }
> +
> +    if (is_wzeroes) { /* VIRTIO_BLK_T_WRITE_ZEROES */
> +        int blk_aio_flags = 0;
> +
> +        if (s->conf.wz_may_unmap &&
> +            flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) {
> +            blk_aio_flags |= BDRV_REQ_MAY_UNMAP;
> +        }
> +
> +        block_acct_start(blk_get_stats(req->dev->blk), &req->acct, bytes,
> +                         BLOCK_ACCT_WRITE);
> +
> +        blk_aio_pwrite_zeroes(req->dev->blk, sector << BDRV_SECTOR_BITS,
> +                              bytes, blk_aio_flags,
> +                              virtio_blk_discard_wzeroes_complete, req);
> +    } else { /* VIRTIO_BLK_T_DISCARD */
> +        /*
> +         * The device MUST set the status byte to VIRTIO_BLK_S_UNSUPP for
> +         * discard commands if the unmap flag is set.
> +         */
> +        if (unlikely(flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP)) {
> +            err_status = VIRTIO_BLK_S_UNSUPP;
> +            goto err;
> +        }
> +
> +        blk_aio_pdiscard(req->dev->blk, sector << BDRV_SECTOR_BITS, bytes,
> +                         virtio_blk_discard_wzeroes_complete, req);
> +    }
> +
> +    return VIRTIO_BLK_S_OK;
> +
> +err:
> +    if (is_wzeroes) {
> +        block_acct_invalid(blk_get_stats(req->dev->blk), BLOCK_ACCT_WRITE);
> +    }
> +    return err_status;
> +}
> +
>  static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
>  {
>      uint32_t type;
> @@ -586,6 +686,45 @@ 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 if it is set.
> +     */
> +    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;
> +        size_t out_len = iov_size(out_iov, out_num);
> +        bool is_wzeroes = (type & ~VIRTIO_BLK_T_BARRIER) ==
> +                          VIRTIO_BLK_T_WRITE_ZEROES;
> +        uint8_t err_status;
> +
> +        /*
> +         * Unsupported if VIRTIO_BLK_T_OUT is not set or the request contains
> +         * more than one segment.
> +         */
> +        if (unlikely(!(type & VIRTIO_BLK_T_OUT) ||
> +                     out_len > sizeof(dwz_hdr))) {
> +            virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
> +            virtio_blk_free_request(req);
> +            return 0;
> +        }
> +
> +        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;
> +        }
> +
> +        err_status = virtio_blk_handle_dwz(req, is_wzeroes, &dwz_hdr);
> +        if (err_status != VIRTIO_BLK_S_OK) {
> +            virtio_blk_req_complete(req, err_status);
> +            virtio_blk_free_request(req);
> +        }
> +
> +        break;
> +    }
>      default:
>          virtio_blk_req_complete(req, VIRTIO_BLK_S_UNSUPP);
>          virtio_blk_free_request(req);
> @@ -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;
> +        /*
> +         * We support only one segment per request since multiple segments
> +         * are not widely used and there are no userspace APIs that allow
> +         * applications to submit multiple segments in a single call.
> +         */
> +        virtio_stl_p(vdev, &blkcfg.max_discard_seg, 1);
> +        virtio_stl_p(vdev, &blkcfg.max_write_zeroes_seg, 1);
> +    }
>      memcpy(config, &blkcfg, sizeof(struct virtio_blk_config));
>  }
>  
> @@ -811,6 +966,10 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
>      if (s->conf.num_queues > 1) {
>          virtio_add_feature(&features, VIRTIO_BLK_F_MQ);
>      }
> +    if (s->conf.discard_wzeroes) {
> +        virtio_add_feature(&features, VIRTIO_BLK_F_DISCARD);
> +        virtio_add_feature(&features, VIRTIO_BLK_F_WRITE_ZEROES);
> +    }
>  
>      return features;
>  }
> @@ -956,6 +1115,16 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>          return;
>      }
>  
> +    if (conf->discard_wzeroes) {
> +        if (!conf->dwz_max_sectors ||
> +            conf->dwz_max_sectors > BDRV_REQUEST_MAX_SECTORS) {
> +            error_setg(errp, "invalid dwz-max-sectors property (%" PRIu32 "), "
> +                       "must be between 1 and %lu",
> +                       conf->dwz_max_sectors, BDRV_REQUEST_MAX_SECTORS);
> +            return;
> +        }
> +    }
> +
>      virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
>                  sizeof(struct virtio_blk_config));
>  
> @@ -1028,6 +1197,10 @@ static Property virtio_blk_properties[] = {
>                       IOThread *),
>      DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0,
>                       true),
> +    DEFINE_PROP_UINT32("discard-wzeroes-max-sectors", VirtIOBlock,
> +                       conf.dwz_max_sectors, BDRV_REQUEST_MAX_SECTORS),
> +    DEFINE_PROP_BIT("wzeroes-may-unmap", VirtIOBlock, conf.wz_may_unmap, 0,
> +                    true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index c336afb4cd..4e9d4434ff 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -41,6 +41,8 @@ struct VirtIOBlkConf
>      uint16_t num_queues;
>      uint16_t queue_size;
>      uint32_t discard_wzeroes;
> +    uint32_t dwz_max_sectors;
> +    uint32_t wz_may_unmap;
>  };
>  
>  struct VirtIOBlockDataPlane;
> -- 
> 2.20.1

Re: [Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Michael S. Tsirkin]

On Thu, Jan 31, 2019 at 04:50:46PM +0100, Stefano Garzarella wrote:
> On Thu, Jan 31, 2019 at 03:40:38PM +0000, Dr. David Alan Gilbert wrote:
> > * Stefano Garzarella (sgarzare@redhat.com) wrote:
> > > In order to avoid migration issues, we enable DISCARD and
> > > WRITE ZEROES features only for machine type >= 4.0
> > > 
> > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > ---
> > >  hw/block/virtio-blk.c          | 2 ++
> > >  hw/core/machine.c              | 1 +
> > >  include/hw/virtio/virtio-blk.h | 1 +
> > >  3 files changed, 4 insertions(+)
> > > 
> > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > index 8a6754d9a2..542ec52536 100644
> > > --- a/hw/block/virtio-blk.c
> > > +++ b/hw/block/virtio-blk.c
> > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = {
> > >      DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
> > >      DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
> > >                       IOThread *),
> > > +    DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0,
> > > +                     true),
> > 
> > I think that's OK, but do you really want a DEFINE_PROP_BOOL and
> > a bool discard_wzeroes?
> > I think DEFINE_PROP_BIT is mostly used for a flag word where each
> > property is one more bit in the field.
> > 
> > Dave
> > 
> 
> Hi Dave,
> I was in doubt if to use DEFINE_PROP_BIT or DEFINE_PROP_BOOL, but
> looking in the virtio-blk.c, I found that also other boolean like
> "config-wce", "scsi", and "request-merging" was defined with
> DEFINE_PROP_BIT, so I followed this trand.
> 
> But I agree with you, DEFINE_PROP_BOOL should be better, so I will change it!
> 
> Thanks,
> Stefano

I wonder why doesn't virtio-blk set bits directly in host_features?
For example this is how virtio-net does it.
This would remove the need for virtio_add_feature calls.

-- 
MST

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

[Stefano Garzarella]

On Thu, Jan 31, 2019 at 11:04:10AM -0500, Michael S. Tsirkin wrote:
> On Thu, Jan 31, 2019 at 04:19:12PM +0100, Stefano Garzarella 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.
> > 
> > We support only one segment per request since multiple segments
> > are not widely used and there are no userspace APIs that allow
> > applications to submit multiple segments in a single call.
> > 
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> 
> 
> This does not seem to match the spec clarifications
> that Stefan Hajnoczi has posted on
> the virtio TC.
> 
> I think this can go any of the following ways:
> 
> - we agree that a request should have at most one segment
> 	no padding allowed
> - we agree that a request should have at most one segment
> 	we require padding to 512 bytes
> - we agree that a request should have at most one segment
> 	we also support padding to 512 bytes
> - we agree that a request should have at most one segment
> 	we also support arbitrary padding
> - we agree that a request can have any # of segments

Hi Michael,
reading the latest patch [1] sent by Stefan, I supposed that the padding
is not allowed, but if we need to support it, I'll fix the implementation.

About the number of segments, I followed the description of
max_discard_sectors [2] and the implementation provided by SPDK's
virtio-blk driver and vhost-user-blk device backend. They also only
support one segment per request, properly setting "max_discard_seg" and
"max_write_zeroes_seg", so if I understood correctly, the specification
leave to the device the freedom to support one or more segments.

> 
> I would also need your feedback on whether all this
> is a material change to 1.1 public review according to the oasis definition:
> 
> 	"Material Change" is any change to the content of a Work Product that
> 	that would require a compliant application or implementation to be
> 	modified or rewritten in order to remain compliant or which adds new
> 	features or otherwise expands the scope of the work product.
> 

IMHO and if I understood correctly, maybe only the second way (require
padding to 512 bytes) should be a "Material Change" because we need to
modify all the implementations (Linux driver, SPDK, vhost).

The other ways should be already supported, because all the
implementations set the status right behind the last byte (regardless of
padding).


#1: https://lists.oasis-open.org/archives/virtio-dev/201901/msg00135.html
#2: https://github.com/oasis-tcs/virtio-spec/blob/master/content.tex#L3876


Thanks,
Stefano

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

[Michael S. Tsirkin]

On Thu, Jan 31, 2019 at 06:01:34PM +0100, Stefano Garzarella wrote:
> On Thu, Jan 31, 2019 at 11:04:10AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Jan 31, 2019 at 04:19:12PM +0100, Stefano Garzarella 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.
> > > 
> > > We support only one segment per request since multiple segments
> > > are not widely used and there are no userspace APIs that allow
> > > applications to submit multiple segments in a single call.
> > > 
> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > 
> > 
> > This does not seem to match the spec clarifications
> > that Stefan Hajnoczi has posted on
> > the virtio TC.
> > 
> > I think this can go any of the following ways:
> > 
> > - we agree that a request should have at most one segment
> > 	no padding allowed
> > - we agree that a request should have at most one segment
> > 	we require padding to 512 bytes
> > - we agree that a request should have at most one segment
> > 	we also support padding to 512 bytes
> > - we agree that a request should have at most one segment
> > 	we also support arbitrary padding
> > - we agree that a request can have any # of segments
> 
> Hi Michael,
> reading the latest patch [1] sent by Stefan, I supposed that the padding
> is not allowed, but if we need to support it, I'll fix the implementation.
> 
> About the number of segments, I followed the description of
> max_discard_sectors [2] and the implementation provided by SPDK's
> virtio-blk driver and vhost-user-blk device backend. They also only
> support one segment per request, properly setting "max_discard_seg" and
> "max_write_zeroes_seg", so if I understood correctly, the specification
> leave to the device the freedom to support one or more segments.


Oh I missed the fact that you set the config space values to 1.
I confused it with # of sectors.


Now it looks right to me, so pls ignore my comments.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>



> > 
> > I would also need your feedback on whether all this
> > is a material change to 1.1 public review according to the oasis definition:
> > 
> > 	"Material Change" is any change to the content of a Work Product that
> > 	that would require a compliant application or implementation to be
> > 	modified or rewritten in order to remain compliant or which adds new
> > 	features or otherwise expands the scope of the work product.
> > 
> 
> IMHO and if I understood correctly, maybe only the second way (require
> padding to 512 bytes) should be a "Material Change" because we need to
> modify all the implementations (Linux driver, SPDK, vhost).
> 
> The other ways should be already supported, because all the
> implementations set the status right behind the last byte (regardless of
> padding).
> 
> 
> #1: https://lists.oasis-open.org/archives/virtio-dev/201901/msg00135.html
> #2: https://github.com/oasis-tcs/virtio-spec/blob/master/content.tex#L3876
> 
> 
> Thanks,
> Stefano

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

[Michael S. Tsirkin]

On Thu, Jan 31, 2019 at 04:19:09PM +0100, Stefano Garzarella wrote:
> This series adds the support of DISCARD and WRITE ZEROES commands
> and extends the virtio-blk-test to test WRITE_ZEROES command when
> the feature is enabled.
> 
> v2:
> - added patch 1 to use virtio_blk_handle_rw_error() with discard operation
> - added patch 2 to make those new features machine-type dependent (thanks David)
> - fixed patch 3 (previously patch 1/2) adding more checks, block_acct_start()
>                 for WRITE_ZEROES requests, and configurable parameters to
>                 initialize the limits (max_sectors, wzeroes_may_unmap).
>                 (thanks Stefan)
>                 I moved in a new function the code to handle a single segment,
>                 in order to simplify the support of multiple segments in the
>                 future.
> - added patch 4 to change the assert on data_size following the discussion with
>                 Thomas, Changpeng, Michael, and Stefan (thanks all)
> - fixed patch 5 (previously patch 2/2) using local dwz_hdr variable instead of
>                 dynamic allocation (thanks Thomas)
> 
> This series requires the new virtio headers from linux v5.0-rc1
> already imported by Paolo:
> 
> Based-on: <20190104082731.24967-1-pbonzini@redhat.com>
> 
> Thanks,
> Stefano

series:

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>


merge through block tree I guess?


> Stefano Garzarella (5):
>   virtio-blk: add acct_failed param to virtio_blk_handle_rw_error()
>   virtio-blk: add "discard-wzeroes" boolean property
>   virtio-blk: add DISCARD and WRITE ZEROES features
>   tests/virtio-blk: change assert on data_size in virtio_blk_request()
>   tests/virtio-blk: add test for WRITE_ZEROES command
> 
>  hw/block/virtio-blk.c          | 185 ++++++++++++++++++++++++++++++++-
>  hw/core/machine.c              |   1 +
>  include/hw/virtio/virtio-blk.h |   3 +
>  tests/virtio-blk-test.c        |  75 ++++++++++++-
>  4 files changed, 259 insertions(+), 5 deletions(-)
> 
> -- 
> 2.20.1

Re: [Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Stefano Garzarella]

On Thu, Jan 31, 2019 at 11:43:07AM -0500, Michael S. Tsirkin wrote:
> On Thu, Jan 31, 2019 at 04:50:46PM +0100, Stefano Garzarella wrote:
> > On Thu, Jan 31, 2019 at 03:40:38PM +0000, Dr. David Alan Gilbert wrote:
> > > * Stefano Garzarella (sgarzare@redhat.com) wrote:
> > > > In order to avoid migration issues, we enable DISCARD and
> > > > WRITE ZEROES features only for machine type >= 4.0
> > > > 
> > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > ---
> > > >  hw/block/virtio-blk.c          | 2 ++
> > > >  hw/core/machine.c              | 1 +
> > > >  include/hw/virtio/virtio-blk.h | 1 +
> > > >  3 files changed, 4 insertions(+)
> > > > 
> > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > index 8a6754d9a2..542ec52536 100644
> > > > --- a/hw/block/virtio-blk.c
> > > > +++ b/hw/block/virtio-blk.c
> > > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = {
> > > >      DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
> > > >      DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
> > > >                       IOThread *),
> > > > +    DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0,
> > > > +                     true),
> > > 
> > > I think that's OK, but do you really want a DEFINE_PROP_BOOL and
> > > a bool discard_wzeroes?
> > > I think DEFINE_PROP_BIT is mostly used for a flag word where each
> > > property is one more bit in the field.
> > > 
> > > Dave
> > > 
> > 
> > Hi Dave,
> > I was in doubt if to use DEFINE_PROP_BIT or DEFINE_PROP_BOOL, but
> > looking in the virtio-blk.c, I found that also other boolean like
> > "config-wce", "scsi", and "request-merging" was defined with
> > DEFINE_PROP_BIT, so I followed this trand.
> > 
> > But I agree with you, DEFINE_PROP_BOOL should be better, so I will change it!
> > 
> > Thanks,
> > Stefano
> 
> I wonder why doesn't virtio-blk set bits directly in host_features?
> For example this is how virtio-net does it.
> This would remove the need for virtio_add_feature calls.

Maybe this should be the best approach!

What do you think if I send a trivial patch to add host_features
variable like for virtio-net and change the "config_wce" and "scsi" definition?
Then I will change also this patch to set directly the bits.

Thanks,
Stefano

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

[Stefano Garzarella]

On Thu, Jan 31, 2019 at 12:15:17PM -0500, Michael S. Tsirkin wrote:
> On Thu, Jan 31, 2019 at 04:19:09PM +0100, Stefano Garzarella wrote:
> > This series adds the support of DISCARD and WRITE ZEROES commands
> > and extends the virtio-blk-test to test WRITE_ZEROES command when
> > the feature is enabled.
> > 
> > v2:
> > - added patch 1 to use virtio_blk_handle_rw_error() with discard operation
> > - added patch 2 to make those new features machine-type dependent (thanks David)
> > - fixed patch 3 (previously patch 1/2) adding more checks, block_acct_start()
> >                 for WRITE_ZEROES requests, and configurable parameters to
> >                 initialize the limits (max_sectors, wzeroes_may_unmap).
> >                 (thanks Stefan)
> >                 I moved in a new function the code to handle a single segment,
> >                 in order to simplify the support of multiple segments in the
> >                 future.
> > - added patch 4 to change the assert on data_size following the discussion with
> >                 Thomas, Changpeng, Michael, and Stefan (thanks all)
> > - fixed patch 5 (previously patch 2/2) using local dwz_hdr variable instead of
> >                 dynamic allocation (thanks Thomas)
> > 
> > This series requires the new virtio headers from linux v5.0-rc1
> > already imported by Paolo:
> > 
> > Based-on: <20190104082731.24967-1-pbonzini@redhat.com>
> > 
> > Thanks,
> > Stefano
> 
> series:
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 
> 
> merge through block tree I guess?
> 

I guess too.

Thanks for the review!

Stefano

Re: [Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Stefan Hajnoczi]

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

On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote:
> In order to avoid migration issues, we enable DISCARD and
> WRITE ZEROES features only for machine type >= 4.0

Please use two separate properties that correspond to the
VIRTIO_BLK_F_DISCARD and VIRTIO_BLK_F_WRITE_ZEROES virtio-blk feature
bits.

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

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

[Stefan Hajnoczi]

[-- 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 --]

Re: [Qemu-devel] [PATCH v2 4/5] tests/virtio-blk: change assert on data_size in virtio_blk_request()

[Stefan Hajnoczi]

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

On Thu, Jan 31, 2019 at 04:19:13PM +0100, Stefano Garzarella wrote:
> The size of data in the virtio_blk_request must be a multiple
> of 512 bytes for IN and OUT requests, or a multiple of the size
> of struct virtio_blk_discard_write_zeroes for DISCARD and
> WRITE_ZEROES requests.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  tests/virtio-blk-test.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 5/5] tests/virtio-blk: add test for WRITE_ZEROES command

[Stefan Hajnoczi]

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

On Thu, Jan 31, 2019 at 04:19:14PM +0100, Stefano Garzarella wrote:
> If the WRITE_ZEROES feature is enabled, we check this command
> in the test_basic().
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  tests/virtio-blk-test.c | 60 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 1/5] virtio-blk: add acct_failed param to virtio_blk_handle_rw_error()

[Stefan Hajnoczi]

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

On Thu, Jan 31, 2019 at 04:19:10PM +0100, Stefano Garzarella wrote:
> We add acct_failed param in order to use virtio_blk_handle_rw_error()
> also when is not required to call block_acct_failed(). (eg. a discard
> operation is failed)
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  hw/block/virtio-blk.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

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

Re: [Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Stefano Garzarella]

On Fri, Feb 01, 2019 at 12:29:28PM +0800, Stefan Hajnoczi wrote:
> On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote:
> > In order to avoid migration issues, we enable DISCARD and
> > WRITE ZEROES features only for machine type >= 4.0
> 
> Please use two separate properties that correspond to the
> VIRTIO_BLK_F_DISCARD and VIRTIO_BLK_F_WRITE_ZEROES virtio-blk feature
> bits.

Okay.
As Michael suggested, what do you think if I use a similar approach of
virtio-net, adding an host_features variable and setting a corresponding
feature bit?

Thanks,
Stefano

Re: [Qemu-devel] [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

Re: [Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Stefan Hajnoczi]

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

On Fri, Feb 01, 2019 at 10:09:08AM +0100, Stefano Garzarella wrote:
> On Fri, Feb 01, 2019 at 12:29:28PM +0800, Stefan Hajnoczi wrote:
> > On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote:
> > > In order to avoid migration issues, we enable DISCARD and
> > > WRITE ZEROES features only for machine type >= 4.0
> > 
> > Please use two separate properties that correspond to the
> > VIRTIO_BLK_F_DISCARD and VIRTIO_BLK_F_WRITE_ZEROES virtio-blk feature
> > bits.
> 
> Okay.
> As Michael suggested, what do you think if I use a similar approach of
> virtio-net, adding an host_features variable and setting a corresponding
> feature bit?

Yes, that's a clean way of supporting feature bit options.

Stefan

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

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

[Stefan Hajnoczi]

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

On Fri, Feb 01, 2019 at 10:54:30AM +0100, Stefano Garzarella wrote:
> 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:
> 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?

Yes, please.  Eventually a user will want full control so it makes sense
to expose what virtio offers instead of coming up with higher-level
options.

Stefan

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

Re: [Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Michael S. Tsirkin]

On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote:
> In order to avoid migration issues, we enable DISCARD and
> WRITE ZEROES features only for machine type >= 4.0
> 
> Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  hw/block/virtio-blk.c          | 2 ++
>  hw/core/machine.c              | 1 +
>  include/hw/virtio/virtio-blk.h | 1 +
>  3 files changed, 4 insertions(+)
> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 8a6754d9a2..542ec52536 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = {
>      DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
>      DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
>                       IOThread *),
> +    DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0,
> +                     true),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  

Thinking about it, are there security implications for discard?
Should we make it default false?

> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 2629515363..ce98857af0 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -30,6 +30,7 @@ GlobalProperty hw_compat_3_1[] = {
>      { "memory-backend-memfd", "x-use-canonical-path-for-ramblock-id", "true" },
>      { "tpm-crb", "ppi", "false" },
>      { "tpm-tis", "ppi", "false" },
> +    { "virtio-blk-device", "discard-wzeroes", "false" },
>  };
>  const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1);
>  
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 5117431d96..c336afb4cd 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -40,6 +40,7 @@ struct VirtIOBlkConf
>      uint32_t request_merging;
>      uint16_t num_queues;
>      uint16_t queue_size;
> +    uint32_t discard_wzeroes;
>  };
>  
>  struct VirtIOBlockDataPlane;
> -- 
> 2.20.1

Re: [Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Stefano Garzarella]

On Fri, Feb 1, 2019 at 4:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote:
> > In order to avoid migration issues, we enable DISCARD and
> > WRITE ZEROES features only for machine type >= 4.0
> >
> > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > ---
> >  hw/block/virtio-blk.c          | 2 ++
> >  hw/core/machine.c              | 1 +
> >  include/hw/virtio/virtio-blk.h | 1 +
> >  3 files changed, 4 insertions(+)
> >
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index 8a6754d9a2..542ec52536 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = {
> >      DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
> >      DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
> >                       IOThread *),
> > +    DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0,
> > +                     true),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
>
> Thinking about it, are there security implications for discard?
> Should we make it default false?

Hi Michael,

I'm not completely sure but if the guest can write on a specific sector,
discard or write_zeroes operations should not have a security implication.

Do I miss something?

Thanks,
Stefano

Re: [Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Stefan Hajnoczi]

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

On Fri, Feb 01, 2019 at 06:18:52PM +0100, Stefano Garzarella wrote:
> On Fri, Feb 1, 2019 at 4:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote:
> > > In order to avoid migration issues, we enable DISCARD and
> > > WRITE ZEROES features only for machine type >= 4.0
> > >
> > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > ---
> > >  hw/block/virtio-blk.c          | 2 ++
> > >  hw/core/machine.c              | 1 +
> > >  include/hw/virtio/virtio-blk.h | 1 +
> > >  3 files changed, 4 insertions(+)
> > >
> > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > index 8a6754d9a2..542ec52536 100644
> > > --- a/hw/block/virtio-blk.c
> > > +++ b/hw/block/virtio-blk.c
> > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = {
> > >      DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
> > >      DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
> > >                       IOThread *),
> > > +    DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0,
> > > +                     true),
> > >      DEFINE_PROP_END_OF_LIST(),
> > >  };
> > >
> >
> > Thinking about it, are there security implications for discard?
> > Should we make it default false?
> 
> Hi Michael,
> 
> I'm not completely sure but if the guest can write on a specific sector,
> discard or write_zeroes operations should not have a security implication.
> 
> Do I miss something?

Recently page cache attacks have been discussed in the Linux community:
https://arxiv.org/pdf/1901.01161.pdf

I guess the scenario Michael is thinking about involves either -drive
cache.direct=off (including cache=writeback or cache=writethrough) or
maybe a timing side-channel in the storage appliance.

The discard operation may allow a guest to evict the cache, an important
primitive for page cache attacks.

Most of the time disk images are not shared between guests at all.
Therefore the discard operation cannot be used to learn information
about other guests.

Let's focus on shared disk images: shared disk images are either
read-only (then discard isn't allowed anyway) or they are shared
writable (but this already implies a trust relationship between the
guests).

My opinion is that discard is safe because virtualization use cases are
quite different from the attacks possible with shared library files
between userspace processes.  I'm curious if anyone has figured out a
realistic scenario where it does matter though...

Stefan

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

Re: [Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Stefano Garzarella]

On Mon, Feb 04, 2019 at 11:33:07AM +0800, Stefan Hajnoczi wrote:
> On Fri, Feb 01, 2019 at 06:18:52PM +0100, Stefano Garzarella wrote:
> > On Fri, Feb 1, 2019 at 4:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote:
> > > > In order to avoid migration issues, we enable DISCARD and
> > > > WRITE ZEROES features only for machine type >= 4.0
> > > >
> > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > ---
> > > >  hw/block/virtio-blk.c          | 2 ++
> > > >  hw/core/machine.c              | 1 +
> > > >  include/hw/virtio/virtio-blk.h | 1 +
> > > >  3 files changed, 4 insertions(+)
> > > >
> > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > index 8a6754d9a2..542ec52536 100644
> > > > --- a/hw/block/virtio-blk.c
> > > > +++ b/hw/block/virtio-blk.c
> > > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = {
> > > >      DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
> > > >      DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
> > > >                       IOThread *),
> > > > +    DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0,
> > > > +                     true),
> > > >      DEFINE_PROP_END_OF_LIST(),
> > > >  };
> > > >
> > >
> > > Thinking about it, are there security implications for discard?
> > > Should we make it default false?
> > 
> > Hi Michael,
> > 
> > I'm not completely sure but if the guest can write on a specific sector,
> > discard or write_zeroes operations should not have a security implication.
> > 
> > Do I miss something?
> 
> Recently page cache attacks have been discussed in the Linux community:
> https://arxiv.org/pdf/1901.01161.pdf
> 
> I guess the scenario Michael is thinking about involves either -drive
> cache.direct=off (including cache=writeback or cache=writethrough) or
> maybe a timing side-channel in the storage appliance.
> 
> The discard operation may allow a guest to evict the cache, an important
> primitive for page cache attacks.
> 
> Most of the time disk images are not shared between guests at all.
> Therefore the discard operation cannot be used to learn information
> about other guests.
> 
> Let's focus on shared disk images: shared disk images are either
> read-only (then discard isn't allowed anyway) or they are shared
> writable (but this already implies a trust relationship between the
> guests).
> 
> My opinion is that discard is safe because virtualization use cases are
> quite different from the attacks possible with shared library files
> between userspace processes.  I'm curious if anyone has figured out a
> realistic scenario where it does matter though...

Many thanks for the explanation!

I'll wait to send the v3 in order to understand if Michael agrees to
leave discard feature enabled to default.

Thanks,
Stefano

Re: [Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Michael S. Tsirkin]

On Mon, Feb 04, 2019 at 11:16:14AM +0100, Stefano Garzarella wrote:
> On Mon, Feb 04, 2019 at 11:33:07AM +0800, Stefan Hajnoczi wrote:
> > On Fri, Feb 01, 2019 at 06:18:52PM +0100, Stefano Garzarella wrote:
> > > On Fri, Feb 1, 2019 at 4:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote:
> > > > > In order to avoid migration issues, we enable DISCARD and
> > > > > WRITE ZEROES features only for machine type >= 4.0
> > > > >
> > > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > ---
> > > > >  hw/block/virtio-blk.c          | 2 ++
> > > > >  hw/core/machine.c              | 1 +
> > > > >  include/hw/virtio/virtio-blk.h | 1 +
> > > > >  3 files changed, 4 insertions(+)
> > > > >
> > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > > index 8a6754d9a2..542ec52536 100644
> > > > > --- a/hw/block/virtio-blk.c
> > > > > +++ b/hw/block/virtio-blk.c
> > > > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = {
> > > > >      DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
> > > > >      DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
> > > > >                       IOThread *),
> > > > > +    DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0,
> > > > > +                     true),
> > > > >      DEFINE_PROP_END_OF_LIST(),
> > > > >  };
> > > > >
> > > >
> > > > Thinking about it, are there security implications for discard?
> > > > Should we make it default false?
> > > 
> > > Hi Michael,
> > > 
> > > I'm not completely sure but if the guest can write on a specific sector,
> > > discard or write_zeroes operations should not have a security implication.
> > > 
> > > Do I miss something?
> > 
> > Recently page cache attacks have been discussed in the Linux community:
> > https://arxiv.org/pdf/1901.01161.pdf
> > 
> > I guess the scenario Michael is thinking about involves either -drive
> > cache.direct=off (including cache=writeback or cache=writethrough) or
> > maybe a timing side-channel in the storage appliance.
> > 
> > The discard operation may allow a guest to evict the cache, an important
> > primitive for page cache attacks.
> > 
> > Most of the time disk images are not shared between guests at all.
> > Therefore the discard operation cannot be used to learn information
> > about other guests.
> > 
> > Let's focus on shared disk images: shared disk images are either
> > read-only (then discard isn't allowed anyway) or they are shared
> > writable (but this already implies a trust relationship between the
> > guests).
> > 
> > My opinion is that discard is safe because virtualization use cases are
> > quite different from the attacks possible with shared library files
> > between userspace processes.  I'm curious if anyone has figured out a
> > realistic scenario where it does matter though...
> 
> Many thanks for the explanation!
> 
> I'll wait to send the v3 in order to understand if Michael agrees to
> leave discard feature enabled to default.
> 
> Thanks,
> Stefano

OK. Maybe mention the above in the commit log.

-- 
MST

Re: [Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Stefano Garzarella]

On Mon, Feb 04, 2019 at 08:37:28AM -0500, Michael S. Tsirkin wrote:
> On Mon, Feb 04, 2019 at 11:16:14AM +0100, Stefano Garzarella wrote:
> > On Mon, Feb 04, 2019 at 11:33:07AM +0800, Stefan Hajnoczi wrote:
> > > On Fri, Feb 01, 2019 at 06:18:52PM +0100, Stefano Garzarella wrote:
> > > > On Fri, Feb 1, 2019 at 4:17 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > On Thu, Jan 31, 2019 at 04:19:11PM +0100, Stefano Garzarella wrote:
> > > > > > In order to avoid migration issues, we enable DISCARD and
> > > > > > WRITE ZEROES features only for machine type >= 4.0
> > > > > >
> > > > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > > ---
> > > > > >  hw/block/virtio-blk.c          | 2 ++
> > > > > >  hw/core/machine.c              | 1 +
> > > > > >  include/hw/virtio/virtio-blk.h | 1 +
> > > > > >  3 files changed, 4 insertions(+)
> > > > > >
> > > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > > > index 8a6754d9a2..542ec52536 100644
> > > > > > --- a/hw/block/virtio-blk.c
> > > > > > +++ b/hw/block/virtio-blk.c
> > > > > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = {
> > > > > >      DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
> > > > > >      DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
> > > > > >                       IOThread *),
> > > > > > +    DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0,
> > > > > > +                     true),
> > > > > >      DEFINE_PROP_END_OF_LIST(),
> > > > > >  };
> > > > > >
> > > > >
> > > > > Thinking about it, are there security implications for discard?
> > > > > Should we make it default false?
> > > > 
> > > > Hi Michael,
> > > > 
> > > > I'm not completely sure but if the guest can write on a specific sector,
> > > > discard or write_zeroes operations should not have a security implication.
> > > > 
> > > > Do I miss something?
> > > 
> > > Recently page cache attacks have been discussed in the Linux community:
> > > https://arxiv.org/pdf/1901.01161.pdf
> > > 
> > > I guess the scenario Michael is thinking about involves either -drive
> > > cache.direct=off (including cache=writeback or cache=writethrough) or
> > > maybe a timing side-channel in the storage appliance.
> > > 
> > > The discard operation may allow a guest to evict the cache, an important
> > > primitive for page cache attacks.
> > > 
> > > Most of the time disk images are not shared between guests at all.
> > > Therefore the discard operation cannot be used to learn information
> > > about other guests.
> > > 
> > > Let's focus on shared disk images: shared disk images are either
> > > read-only (then discard isn't allowed anyway) or they are shared
> > > writable (but this already implies a trust relationship between the
> > > guests).
> > > 
> > > My opinion is that discard is safe because virtualization use cases are
> > > quite different from the attacks possible with shared library files
> > > between userspace processes.  I'm curious if anyone has figured out a
> > > realistic scenario where it does matter though...
> > 
> > Many thanks for the explanation!
> > 
> > I'll wait to send the v3 in order to understand if Michael agrees to
> > leave discard feature enabled to default.
> > 
> > Thanks,
> > Stefano
> 
> OK. Maybe mention the above in the commit log.
> 

Ok, I'll do it!

Thanks,
Stefano

Re: [Qemu-devel] [PATCH v2 2/5] virtio-blk: add "discard-wzeroes" boolean property

[Michael S. Tsirkin]

On Thu, Jan 31, 2019 at 06:37:13PM +0100, Stefano Garzarella wrote:
> On Thu, Jan 31, 2019 at 11:43:07AM -0500, Michael S. Tsirkin wrote:
> > On Thu, Jan 31, 2019 at 04:50:46PM +0100, Stefano Garzarella wrote:
> > > On Thu, Jan 31, 2019 at 03:40:38PM +0000, Dr. David Alan Gilbert wrote:
> > > > * Stefano Garzarella (sgarzare@redhat.com) wrote:
> > > > > In order to avoid migration issues, we enable DISCARD and
> > > > > WRITE ZEROES features only for machine type >= 4.0
> > > > > 
> > > > > Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > > > Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > > ---
> > > > >  hw/block/virtio-blk.c          | 2 ++
> > > > >  hw/core/machine.c              | 1 +
> > > > >  include/hw/virtio/virtio-blk.h | 1 +
> > > > >  3 files changed, 4 insertions(+)
> > > > > 
> > > > > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > > > > index 8a6754d9a2..542ec52536 100644
> > > > > --- a/hw/block/virtio-blk.c
> > > > > +++ b/hw/block/virtio-blk.c
> > > > > @@ -1026,6 +1026,8 @@ static Property virtio_blk_properties[] = {
> > > > >      DEFINE_PROP_UINT16("queue-size", VirtIOBlock, conf.queue_size, 128),
> > > > >      DEFINE_PROP_LINK("iothread", VirtIOBlock, conf.iothread, TYPE_IOTHREAD,
> > > > >                       IOThread *),
> > > > > +    DEFINE_PROP_BIT("discard-wzeroes", VirtIOBlock, conf.discard_wzeroes, 0,
> > > > > +                     true),
> > > > 
> > > > I think that's OK, but do you really want a DEFINE_PROP_BOOL and
> > > > a bool discard_wzeroes?
> > > > I think DEFINE_PROP_BIT is mostly used for a flag word where each
> > > > property is one more bit in the field.
> > > > 
> > > > Dave
> > > > 
> > > 
> > > Hi Dave,
> > > I was in doubt if to use DEFINE_PROP_BIT or DEFINE_PROP_BOOL, but
> > > looking in the virtio-blk.c, I found that also other boolean like
> > > "config-wce", "scsi", and "request-merging" was defined with
> > > DEFINE_PROP_BIT, so I followed this trand.
> > > 
> > > But I agree with you, DEFINE_PROP_BOOL should be better, so I will change it!
> > > 
> > > Thanks,
> > > Stefano
> > 
> > I wonder why doesn't virtio-blk set bits directly in host_features?
> > For example this is how virtio-net does it.
> > This would remove the need for virtio_add_feature calls.
> 
> Maybe this should be the best approach!
> 
> What do you think if I send a trivial patch to add host_features
> variable like for virtio-net and change the "config_wce" and "scsi" definition?
> Then I will change also this patch to set directly the bits.
> 
> Thanks,
> Stefano

Sounds good to me.

-- 
MST