[Qemu-devel] [PATCH v5 00/10] virtio-blk: add DISCARD and WRITE_ZEROES features

[Qemu-devel] [PATCH v5 00/10] 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 these new commands.

v5:
- rebased on master
- handled the config size for DISCARD and WRITE_ZEROES features as in
  virtio-net (patches 4 and 5) [Michael, Stefan]
- fixed an endianness issues on the WRITE_ZEROES test (patches 8 and 9)
- added a test for DISCARD command to only test the status returned by the
  request (patch 10)
- others patches are unchanged (patches 1, 2, 3, 6, 7)

v4:
- fixed error with mingw compiler in patch 4
  gcc and clang want %lu, but mingw wants %llu for BDRV_REQUEST_MAX_SECTORS.
  Since is less than INT_MAX, I casted it to integer and I used %d in the
  format string of error_setg. (mingw now is happy)

v3:
- rebased on master (I removed Based-on tag since the new virtio headers from
  linux v5.0-rc1 are merged)
- added patch 2 to add host_features field (as in virtio-net) [Michael]
- fixed patch 3 (previously 2/5) using the new host_features field
- fixed patch 4 (previously 3/5) following the Stefan's comments:
                - fixed name of functions and fields
                - used vdev and s pointers
                - removed "wz-may-unmap" property
                - split "dwz-max-sectors" in two properties

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)

Thanks,
Stefano

Stefano Garzarella (10):
  virtio-blk: add acct_failed param to virtio_blk_handle_rw_error()
  virtio-blk: add host_features field in VirtIOBlock
  virtio-blk: add "discard" and "write-zeroes" properties
  virtio-net: make VirtIOFeature usable for other virtio devices
  virtio-blk: set config size depending on the features enabled
  virtio-blk: add DISCARD and WRITE_ZEROES features
  tests/virtio-blk: change assert on data_size in virtio_blk_request()
  tests/virtio-blk: add virtio_blk_fix_dwz_hdr() function
  tests/virtio-blk: add test for WRITE_ZEROES command
  tests/virtio-blk: add test for DISCARD command

 hw/block/virtio-blk.c          | 245 ++++++++++++++++++++++++++++++---
 hw/core/machine.c              |   2 +
 hw/net/virtio-net.c            |  31 +----
 hw/virtio/virtio.c             |  15 ++
 include/hw/virtio/virtio-blk.h |   6 +-
 include/hw/virtio/virtio.h     |  15 ++
 tests/virtio-blk-test.c        | 127 ++++++++++++++++-
 7 files changed, 391 insertions(+), 50 deletions(-)

-- 
2.20.1

[Qemu-devel] [PATCH v5 01/10] 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)

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
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 cf7f47eaba..afedf48ca6 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -65,7 +65,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)
 {
     VirtIOBlock *s = req->dev;
     BlockErrorAction action = blk_get_error_action(s->blk, is_read, error);
@@ -78,7 +78,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);
     }
 
@@ -116,7 +118,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;
             }
         }
@@ -135,7 +137,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 v5 02/10] virtio-blk: add host_features field in VirtIOBlock

[Stefano Garzarella]

Since configurable features for virtio-blk are growing, this patch
adds host_features field in the struct VirtIOBlock. (as in virtio-net)
In this way, we can avoid to add new fields for new properties and
we can directly set VIRTIO_BLK_F* flags in the host_features.

We update "config-wce" and "scsi" property definition to use the new
host_features field without change the behaviour.

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 hw/block/virtio-blk.c          | 16 +++++++++-------
 include/hw/virtio/virtio-blk.h |  3 +--
 2 files changed, 10 insertions(+), 9 deletions(-)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index afedf48ca6..7813237756 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -245,7 +245,7 @@ static int virtio_blk_handle_scsi_req(VirtIOBlockReq *req)
      */
     scsi = (void *)elem->in_sg[elem->in_num - 2].iov_base;
 
-    if (!blk->conf.scsi) {
+    if (!virtio_has_feature(blk->host_features, VIRTIO_BLK_F_SCSI)) {
         status = VIRTIO_BLK_S_UNSUPP;
         goto fail;
     }
@@ -785,12 +785,15 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
 {
     VirtIOBlock *s = VIRTIO_BLK(vdev);
 
+    /* Firstly sync all virtio-blk possible supported features */
+    features |= s->host_features;
+
     virtio_add_feature(&features, VIRTIO_BLK_F_SEG_MAX);
     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);
     if (virtio_has_feature(features, VIRTIO_F_VERSION_1)) {
-        if (s->conf.scsi) {
+        if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_SCSI)) {
             error_setg(errp, "Please set scsi=off for virtio-blk devices in order to use virtio 1.0");
             return 0;
         }
@@ -799,9 +802,6 @@ static uint64_t virtio_blk_get_features(VirtIODevice *vdev, uint64_t features,
         virtio_add_feature(&features, VIRTIO_BLK_F_SCSI);
     }
 
-    if (s->conf.config_wce) {
-        virtio_add_feature(&features, VIRTIO_BLK_F_CONFIG_WCE);
-    }
     if (blk_enable_write_cache(s->blk)) {
         virtio_add_feature(&features, VIRTIO_BLK_F_WCE);
     }
@@ -1015,9 +1015,11 @@ static Property virtio_blk_properties[] = {
     DEFINE_BLOCK_ERROR_PROPERTIES(VirtIOBlock, conf.conf),
     DEFINE_BLOCK_CHS_PROPERTIES(VirtIOBlock, conf.conf),
     DEFINE_PROP_STRING("serial", VirtIOBlock, conf.serial),
-    DEFINE_PROP_BIT("config-wce", VirtIOBlock, conf.config_wce, 0, true),
+    DEFINE_PROP_BIT64("config-wce", VirtIOBlock, host_features,
+                      VIRTIO_BLK_F_CONFIG_WCE, true),
 #ifdef __linux__
-    DEFINE_PROP_BIT("scsi", VirtIOBlock, conf.scsi, 0, false),
+    DEFINE_PROP_BIT64("scsi", VirtIOBlock, host_features,
+                      VIRTIO_BLK_F_SCSI, false),
 #endif
     DEFINE_PROP_BIT("request-merging", VirtIOBlock, conf.request_merging, 0,
                     true),
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 5117431d96..f7345b0511 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -35,8 +35,6 @@ struct VirtIOBlkConf
     BlockConf conf;
     IOThread *iothread;
     char *serial;
-    uint32_t scsi;
-    uint32_t config_wce;
     uint32_t request_merging;
     uint16_t num_queues;
     uint16_t queue_size;
@@ -57,6 +55,7 @@ typedef struct VirtIOBlock {
     bool dataplane_disabled;
     bool dataplane_started;
     struct VirtIOBlockDataPlane *dataplane;
+    uint64_t host_features;
 } VirtIOBlock;
 
 typedef struct VirtIOBlockReq {
-- 
2.20.1

[Qemu-devel] [PATCH v5 03/10] virtio-blk: add "discard" and "write-zeroes" properties

[Stefano Garzarella]

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

As discussed with Michael S. Tsirkin and Stefan Hajnoczi on the
list [1], DISCARD operation should not have security implications
(eg. page cache attacks), so we can enable it by default.

[1] https://lists.gnu.org/archive/html/qemu-devel/2019-02/msg00504.html

Suggested-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 hw/block/virtio-blk.c | 4 ++++
 hw/core/machine.c     | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 7813237756..f7cd322811 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1027,6 +1027,10 @@ 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_BIT64("discard", VirtIOBlock, host_features,
+                      VIRTIO_BLK_F_DISCARD, true),
+    DEFINE_PROP_BIT64("write-zeroes", VirtIOBlock, host_features,
+                      VIRTIO_BLK_F_WRITE_ZEROES, true),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/hw/core/machine.c b/hw/core/machine.c
index 077fbd182a..766ca5899d 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -33,6 +33,8 @@ GlobalProperty hw_compat_3_1[] = {
     { "usb-kbd", "serial", "42" },
     { "usb-mouse", "serial", "42" },
     { "usb-kbd", "serial", "42" },
+    { "virtio-blk-device", "discard", "false" },
+    { "virtio-blk-device", "write-zeroes", "false" },
 };
 const size_t hw_compat_3_1_len = G_N_ELEMENTS(hw_compat_3_1);
 
-- 
2.20.1

[Qemu-devel] [PATCH v5 04/10] virtio-net: make VirtIOFeature usable for other virtio devices

[Stefano Garzarella]

In order to use VirtIOFeature also in other virtio devices, we move
its declaration and the endof() macro (renamed in virtio_endof())
in virtio.h.
We add virtio_feature_get_config_size() function to iterate the array
of VirtIOFeature and to return the config size depending on the
features enabled. (as virtio_net_set_config_size() did)

Suggested-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 hw/net/virtio-net.c        | 31 +++++++------------------------
 hw/virtio/virtio.c         | 15 +++++++++++++++
 include/hw/virtio/virtio.h | 15 +++++++++++++++
 3 files changed, 37 insertions(+), 24 deletions(-)

diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
index 3f319ef723..6e6b146022 100644
--- a/hw/net/virtio-net.c
+++ b/hw/net/virtio-net.c
@@ -82,29 +82,17 @@ static inline __virtio16 *virtio_net_rsc_ext_num_dupacks(
 
 #endif
 
-/*
- * Calculate the number of bytes up to and including the given 'field' of
- * 'container'.
- */
-#define endof(container, field) \
-    (offsetof(container, field) + sizeof_field(container, field))
-
-typedef struct VirtIOFeature {
-    uint64_t flags;
-    size_t end;
-} VirtIOFeature;
-
 static VirtIOFeature feature_sizes[] = {
     {.flags = 1ULL << VIRTIO_NET_F_MAC,
-     .end = endof(struct virtio_net_config, mac)},
+     .end = virtio_endof(struct virtio_net_config, mac)},
     {.flags = 1ULL << VIRTIO_NET_F_STATUS,
-     .end = endof(struct virtio_net_config, status)},
+     .end = virtio_endof(struct virtio_net_config, status)},
     {.flags = 1ULL << VIRTIO_NET_F_MQ,
-     .end = endof(struct virtio_net_config, max_virtqueue_pairs)},
+     .end = virtio_endof(struct virtio_net_config, max_virtqueue_pairs)},
     {.flags = 1ULL << VIRTIO_NET_F_MTU,
-     .end = endof(struct virtio_net_config, mtu)},
+     .end = virtio_endof(struct virtio_net_config, mtu)},
     {.flags = 1ULL << VIRTIO_NET_F_SPEED_DUPLEX,
-     .end = endof(struct virtio_net_config, duplex)},
+     .end = virtio_endof(struct virtio_net_config, duplex)},
     {}
 };
 
@@ -2580,15 +2568,10 @@ static void virtio_net_guest_notifier_mask(VirtIODevice *vdev, int idx,
 
 static void virtio_net_set_config_size(VirtIONet *n, uint64_t host_features)
 {
-    int i, config_size = 0;
     virtio_add_feature(&host_features, VIRTIO_NET_F_MAC);
 
-    for (i = 0; feature_sizes[i].flags != 0; i++) {
-        if (host_features & feature_sizes[i].flags) {
-            config_size = MAX(feature_sizes[i].end, config_size);
-        }
-    }
-    n->config_size = config_size;
+    n->config_size = virtio_feature_get_config_size(feature_sizes,
+                                                    host_features);
 }
 
 void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index a1ff647a66..2626a895cb 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -2036,6 +2036,21 @@ int virtio_set_features(VirtIODevice *vdev, uint64_t val)
     return ret;
 }
 
+size_t virtio_feature_get_config_size(VirtIOFeature *feature_sizes,
+                                      uint64_t host_features)
+{
+    size_t config_size = 0;
+    int i;
+
+    for (i = 0; feature_sizes[i].flags != 0; i++) {
+        if (host_features & feature_sizes[i].flags) {
+            config_size = MAX(feature_sizes[i].end, config_size);
+        }
+    }
+
+    return config_size;
+}
+
 int virtio_load(VirtIODevice *vdev, QEMUFile *f, int version_id)
 {
     int i, ret;
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 9c1fa07d6d..ce9516236a 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -37,6 +37,21 @@ static inline hwaddr vring_align(hwaddr addr,
     return QEMU_ALIGN_UP(addr, align);
 }
 
+/*
+ * Calculate the number of bytes up to and including the given 'field' of
+ * 'container'.
+ */
+#define virtio_endof(container, field) \
+    (offsetof(container, field) + sizeof_field(container, field))
+
+typedef struct VirtIOFeature {
+    uint64_t flags;
+    size_t end;
+} VirtIOFeature;
+
+size_t virtio_feature_get_config_size(VirtIOFeature *features,
+                                      uint64_t host_features);
+
 typedef struct VirtQueue VirtQueue;
 
 #define VIRTQUEUE_MAX_SIZE 1024
-- 
2.20.1

Re: [Qemu-devel] [Qemu-block] [PATCH v5 04/10] virtio-net: make VirtIOFeature usable for other virtio devices

[Stefan Hajnoczi]

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

On Mon, Feb 18, 2019 at 03:02:55PM +0100, Stefano Garzarella wrote:
> In order to use VirtIOFeature also in other virtio devices, we move
> its declaration and the endof() macro (renamed in virtio_endof())
> in virtio.h.
> We add virtio_feature_get_config_size() function to iterate the array
> of VirtIOFeature and to return the config size depending on the
> features enabled. (as virtio_net_set_config_size() did)
> 
> Suggested-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  hw/net/virtio-net.c        | 31 +++++++------------------------
>  hw/virtio/virtio.c         | 15 +++++++++++++++
>  include/hw/virtio/virtio.h | 15 +++++++++++++++
>  3 files changed, 37 insertions(+), 24 deletions(-)

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

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

[Qemu-devel] [PATCH v5 05/10] virtio-blk: set config size depending on the features enabled

[Stefano Garzarella]

Starting from DISABLE and WRITE_ZEROES features, we use an array of
VirtIOFeature (as virtio-net) to properly set the config size
depending on the features enabled.

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

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index f7cd322811..8798d13bc4 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -28,9 +28,28 @@
 #include "hw/virtio/virtio-bus.h"
 #include "hw/virtio/virtio-access.h"
 
-/* We don't support discard yet, hide associated config fields. */
+/* Config size before the discard support (hide associated config fields) */
 #define VIRTIO_BLK_CFG_SIZE offsetof(struct virtio_blk_config, \
                                      max_discard_sectors)
+/*
+ * Starting from the discard feature, we can use this array to properly
+ * set the config size depending on the features enabled.
+ */
+static VirtIOFeature feature_sizes[] = {
+    {.flags = 1ULL << VIRTIO_BLK_F_DISCARD,
+     .end = virtio_endof(struct virtio_blk_config, discard_sector_alignment)},
+    {.flags = 1ULL << VIRTIO_BLK_F_WRITE_ZEROES,
+     .end = virtio_endof(struct virtio_blk_config, write_zeroes_may_unmap)},
+    {}
+};
+
+static void virtio_blk_set_config_size(VirtIOBlock *s, uint64_t host_features)
+{
+    s->config_size = MAX(VIRTIO_BLK_CFG_SIZE,
+        virtio_feature_get_config_size(feature_sizes, host_features));
+
+    assert(s->config_size <= sizeof(struct virtio_blk_config));
+}
 
 static void virtio_blk_init_request(VirtIOBlock *s, VirtQueue *vq,
                                     VirtIOBlockReq *req)
@@ -763,8 +782,7 @@ 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);
-    memcpy(config, &blkcfg, VIRTIO_BLK_CFG_SIZE);
-    QEMU_BUILD_BUG_ON(VIRTIO_BLK_CFG_SIZE > sizeof(blkcfg));
+    memcpy(config, &blkcfg, s->config_size);
 }
 
 static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
@@ -772,8 +790,7 @@ static void virtio_blk_set_config(VirtIODevice *vdev, const uint8_t *config)
     VirtIOBlock *s = VIRTIO_BLK(vdev);
     struct virtio_blk_config blkcfg;
 
-    memcpy(&blkcfg, config, VIRTIO_BLK_CFG_SIZE);
-    QEMU_BUILD_BUG_ON(VIRTIO_BLK_CFG_SIZE > sizeof(blkcfg));
+    memcpy(&blkcfg, config, s->config_size);
 
     aio_context_acquire(blk_get_aio_context(s->blk));
     blk_set_enable_write_cache(s->blk, blkcfg.wce != 0);
@@ -956,7 +973,9 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
-    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, VIRTIO_BLK_CFG_SIZE);
+    virtio_blk_set_config_size(s, s->host_features);
+
+    virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size);
 
     s->blk = conf->conf.blk;
     s->rq = NULL;
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index f7345b0511..7877ae67ae 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -56,6 +56,7 @@ typedef struct VirtIOBlock {
     bool dataplane_started;
     struct VirtIOBlockDataPlane *dataplane;
     uint64_t host_features;
+    size_t config_size;
 } VirtIOBlock;
 
 typedef struct VirtIOBlockReq {
-- 
2.20.1

Re: [Qemu-devel] [Qemu-block] [PATCH v5 05/10] virtio-blk: set config size depending on the features enabled

[Stefan Hajnoczi]

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

On Mon, Feb 18, 2019 at 03:02:56PM +0100, Stefano Garzarella wrote:
> Starting from DISABLE and WRITE_ZEROES features, we use an array of
> VirtIOFeature (as virtio-net) to properly set the config size
> depending on the features enabled.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  hw/block/virtio-blk.c          | 31 +++++++++++++++++++++++++------
>  include/hw/virtio/virtio-blk.h |  1 +
>  2 files changed, 26 insertions(+), 6 deletions(-)

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

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

[Qemu-devel] [PATCH v5 06/10] 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.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 hw/block/virtio-blk.c          | 184 +++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-blk.h |   2 +
 2 files changed, 186 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 8798d13bc4..c159a3d5f7 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -169,6 +169,30 @@ out:
     aio_context_release(blk_get_aio_context(s->conf.conf.blk));
 }
 
+static void virtio_blk_discard_write_zeroes_complete(void *opaque, int ret)
+{
+    VirtIOBlockReq *req = opaque;
+    VirtIOBlock *s = req->dev;
+    bool is_write_zeroes = (virtio_ldl_p(VIRTIO_DEVICE(s), &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, false, is_write_zeroes)) {
+            goto out;
+        }
+    }
+
+    virtio_blk_req_complete(req, VIRTIO_BLK_S_OK);
+    if (is_write_zeroes) {
+        block_acct_done(blk_get_stats(s->blk), &req->acct);
+    }
+    virtio_blk_free_request(req);
+
+out:
+    aio_context_release(blk_get_aio_context(s->conf.conf.blk));
+}
+
 #ifdef __linux__
 
 typedef struct {
@@ -502,6 +526,84 @@ static bool virtio_blk_sect_range_ok(VirtIOBlock *dev,
     return true;
 }
 
+static uint8_t virtio_blk_handle_discard_write_zeroes(VirtIOBlockReq *req,
+    struct virtio_blk_discard_write_zeroes *dwz_hdr, bool is_write_zeroes)
+{
+    VirtIOBlock *s = req->dev;
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+    uint64_t sector;
+    uint32_t num_sectors, flags, max_sectors;
+    uint8_t err_status;
+    int bytes;
+
+    sector = virtio_ldq_p(vdev, &dwz_hdr->sector);
+    num_sectors = virtio_ldl_p(vdev, &dwz_hdr->num_sectors);
+    flags = virtio_ldl_p(vdev, &dwz_hdr->flags);
+    max_sectors = is_write_zeroes ? s->conf.max_write_zeroes_sectors :
+                  s->conf.max_discard_sectors;
+
+    /*
+     * 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 > max_sectors)) {
+        err_status = VIRTIO_BLK_S_IOERR;
+        goto err;
+    }
+
+    bytes = num_sectors << BDRV_SECTOR_BITS;
+
+    if (unlikely(!virtio_blk_sect_range_ok(s, 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_write_zeroes) { /* VIRTIO_BLK_T_WRITE_ZEROES */
+        int blk_aio_flags = 0;
+
+        if (flags & VIRTIO_BLK_WRITE_ZEROES_FLAG_UNMAP) {
+            blk_aio_flags |= BDRV_REQ_MAY_UNMAP;
+        }
+
+        block_acct_start(blk_get_stats(s->blk), &req->acct, bytes,
+                         BLOCK_ACCT_WRITE);
+
+        blk_aio_pwrite_zeroes(s->blk, sector << BDRV_SECTOR_BITS,
+                              bytes, blk_aio_flags,
+                              virtio_blk_discard_write_zeroes_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(s->blk, sector << BDRV_SECTOR_BITS, bytes,
+                         virtio_blk_discard_write_zeroes_complete, req);
+    }
+
+    return VIRTIO_BLK_S_OK;
+
+err:
+    if (is_write_zeroes) {
+        block_acct_invalid(blk_get_stats(s->blk), BLOCK_ACCT_WRITE);
+    }
+    return err_status;
+}
+
 static int virtio_blk_handle_request(VirtIOBlockReq *req, MultiReqBuffer *mrb)
 {
     uint32_t type;
@@ -603,6 +705,47 @@ 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_write_zeroes = (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/write_zeroes header"
+                         " too short");
+            return -1;
+        }
+
+        err_status = virtio_blk_handle_discard_write_zeroes(req, &dwz_hdr,
+                                                            is_write_zeroes);
+        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);
@@ -782,6 +925,24 @@ 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 (virtio_has_feature(s->host_features, VIRTIO_BLK_F_DISCARD)) {
+        virtio_stl_p(vdev, &blkcfg.max_discard_sectors,
+                     s->conf.max_discard_sectors);
+        virtio_stl_p(vdev, &blkcfg.discard_sector_alignment,
+                     blk_size >> BDRV_SECTOR_BITS);
+        /*
+         * 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);
+    }
+    if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_WRITE_ZEROES)) {
+        virtio_stl_p(vdev, &blkcfg.max_write_zeroes_sectors,
+                     s->conf.max_write_zeroes_sectors);
+        blkcfg.write_zeroes_may_unmap = 1;
+        virtio_stl_p(vdev, &blkcfg.max_write_zeroes_seg, 1);
+    }
     memcpy(config, &blkcfg, s->config_size);
 }
 
@@ -973,6 +1134,25 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
         return;
     }
 
+    if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_DISCARD) &&
+        (!conf->max_discard_sectors ||
+         conf->max_discard_sectors > BDRV_REQUEST_MAX_SECTORS)) {
+        error_setg(errp, "invalid max-discard-sectors property (%" PRIu32 ")"
+                   ", must be between 1 and %d",
+                   conf->max_discard_sectors, (int)BDRV_REQUEST_MAX_SECTORS);
+        return;
+    }
+
+    if (virtio_has_feature(s->host_features, VIRTIO_BLK_F_WRITE_ZEROES) &&
+        (!conf->max_write_zeroes_sectors ||
+         conf->max_write_zeroes_sectors > BDRV_REQUEST_MAX_SECTORS)) {
+        error_setg(errp, "invalid max-write-zeroes-sectors property (%" PRIu32
+                   "), must be between 1 and %d",
+                   conf->max_write_zeroes_sectors,
+                   (int)BDRV_REQUEST_MAX_SECTORS);
+        return;
+    }
+
     virtio_blk_set_config_size(s, s->host_features);
 
     virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, s->config_size);
@@ -1050,6 +1230,10 @@ static Property virtio_blk_properties[] = {
                       VIRTIO_BLK_F_DISCARD, true),
     DEFINE_PROP_BIT64("write-zeroes", VirtIOBlock, host_features,
                       VIRTIO_BLK_F_WRITE_ZEROES, true),
+    DEFINE_PROP_UINT32("max-discard-sectors", VirtIOBlock,
+                       conf.max_discard_sectors, BDRV_REQUEST_MAX_SECTORS),
+    DEFINE_PROP_UINT32("max-write-zeroes-sectors", VirtIOBlock,
+                       conf.max_write_zeroes_sectors, BDRV_REQUEST_MAX_SECTORS),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 7877ae67ae..cddcfbebe9 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -38,6 +38,8 @@ struct VirtIOBlkConf
     uint32_t request_merging;
     uint16_t num_queues;
     uint16_t queue_size;
+    uint32_t max_discard_sectors;
+    uint32_t max_write_zeroes_sectors;
 };
 
 struct VirtIOBlockDataPlane;
-- 
2.20.1

[Qemu-devel] [PATCH v5 07/10] 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.

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
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 v5 08/10] tests/virtio-blk: add virtio_blk_fix_dwz_hdr() function

[Stefano Garzarella]

This function is useful to fix the endianness of struct
virtio_blk_discard_write_zeroes headers.

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

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index 0739498da7..b51c4f2697 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -46,6 +46,12 @@ typedef struct QVirtioBlkReq {
     uint8_t status;
 } QVirtioBlkReq;
 
+#ifdef HOST_WORDS_BIGENDIAN
+const bool host_is_big_endian = true;
+#else
+const bool host_is_big_endian = false;
+#endif
+
 static char *drive_create(void)
 {
     int fd, ret;
@@ -125,12 +131,6 @@ static QVirtioPCIDevice *virtio_blk_pci_init(QPCIBus *bus, int slot)
 
 static inline void virtio_blk_fix_request(QVirtioDevice *d, QVirtioBlkReq *req)
 {
-#ifdef HOST_WORDS_BIGENDIAN
-    const bool host_is_big_endian = true;
-#else
-    const bool host_is_big_endian = false;
-#endif
-
     if (qvirtio_is_big_endian(d) != host_is_big_endian) {
         req->type = bswap32(req->type);
         req->ioprio = bswap32(req->ioprio);
@@ -138,6 +138,17 @@ static inline void virtio_blk_fix_request(QVirtioDevice *d, QVirtioBlkReq *req)
     }
 }
 
+
+static inline void virtio_blk_fix_dwz_hdr(QVirtioDevice *d,
+    struct virtio_blk_discard_write_zeroes *dwz_hdr)
+{
+    if (qvirtio_is_big_endian(d) != host_is_big_endian) {
+        dwz_hdr->sector = bswap64(dwz_hdr->sector);
+        dwz_hdr->num_sectors = bswap32(dwz_hdr->num_sectors);
+        dwz_hdr->flags = bswap32(dwz_hdr->flags);
+    }
+}
+
 static uint64_t virtio_blk_request(QGuestAllocator *alloc, QVirtioDevice *d,
                                    QVirtioBlkReq *req, uint64_t data_size)
 {
-- 
2.20.1

Re: [Qemu-devel] [Qemu-block] [PATCH v5 08/10] tests/virtio-blk: add virtio_blk_fix_dwz_hdr() function

[Stefan Hajnoczi]

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

On Mon, Feb 18, 2019 at 03:02:59PM +0100, Stefano Garzarella wrote:
> This function is useful to fix the endianness of struct
> virtio_blk_discard_write_zeroes headers.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  tests/virtio-blk-test.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)

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

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

[Qemu-devel] [PATCH v5 09/10] 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().

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Acked-by: Thomas Huth <thuth@redhat.com>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 tests/virtio-blk-test.c | 62 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index b51c4f2697..cfc560be46 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -255,6 +255,68 @@ 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;
+
+        virtio_blk_fix_dwz_hdr(dev, &dwz_hdr);
+
+        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

[Qemu-devel] [PATCH v5 10/10] tests/virtio-blk: add test for DISCARD command

[Stefano Garzarella]

If the DISCARD feature is enabled, we try this command in the
test_basic(), checking only the status returned by the request.

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

diff --git a/tests/virtio-blk-test.c b/tests/virtio-blk-test.c
index cfc560be46..0bdbee32ba 100644
--- a/tests/virtio-blk-test.c
+++ b/tests/virtio-blk-test.c
@@ -317,6 +317,33 @@ static void test_basic(QVirtioDevice *dev, QGuestAllocator *alloc,
         guest_free(alloc, req_addr);
     }
 
+    if (features & (1u << VIRTIO_BLK_F_DISCARD)) {
+        struct virtio_blk_discard_write_zeroes dwz_hdr;
+
+        req.type = VIRTIO_BLK_T_DISCARD;
+        req.data = (char *) &dwz_hdr;
+        dwz_hdr.sector = 0;
+        dwz_hdr.num_sectors = 1;
+        dwz_hdr.flags = 0;
+
+        virtio_blk_fix_dwz_hdr(dev, &dwz_hdr);
+
+        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);
+    }
+
     if (features & (1u << VIRTIO_F_ANY_LAYOUT)) {
         /* Write and read with 2 descriptor layout */
         /* Write request */
-- 
2.20.1

Re: [Qemu-devel] [PATCH v5 10/10] tests/virtio-blk: add test for DISCARD command

[Stefan Hajnoczi]

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

On Mon, Feb 18, 2019 at 03:03:01PM +0100, Stefano Garzarella wrote:
> If the DISCARD feature is enabled, we try this command in the
> test_basic(), checking only the status returned by the request.
> 
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  tests/virtio-blk-test.c | 27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)

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

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

Re: [Qemu-devel] [PATCH v5 00/10] virtio-blk: add DISCARD and WRITE_ZEROES features

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190218140301.197408-1-sgarzare@redhat.com/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Message-id: 20190218140301.197408-1-sgarzare@redhat.com
Subject: [Qemu-devel] [PATCH v5 00/10] virtio-blk: add DISCARD and WRITE_ZEROES features
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
   1e36232994..1c5d9d8f11  master     -> master
 * [new tag]               patchew/20190218140301.197408-1-sgarzare@redhat.com -> patchew/20190218140301.197408-1-sgarzare@redhat.com
 * [new tag]               patchew/20190218140607.31998-1-armbru@redhat.com -> patchew/20190218140607.31998-1-armbru@redhat.com
Switched to a new branch 'test'
4e387bff7b tests/virtio-blk: add test for DISCARD command
9dbf66647f tests/virtio-blk: add test for WRITE_ZEROES command
1e74575a2e tests/virtio-blk: add virtio_blk_fix_dwz_hdr() function
0677a06531 tests/virtio-blk: change assert on data_size in virtio_blk_request()
f2d45c1a14 virtio-blk: add DISCARD and WRITE_ZEROES features
7cc46c25d2 virtio-blk: set config size depending on the features enabled
26f46a5550 virtio-net: make VirtIOFeature usable for other virtio devices
23b07b39fb virtio-blk: add "discard" and "write-zeroes" properties
9d6734a0fe virtio-blk: add host_features field in VirtIOBlock
97427f4d38 virtio-blk: add acct_failed param to virtio_blk_handle_rw_error()

=== OUTPUT BEGIN ===
1/10 Checking commit 97427f4d38a6 (virtio-blk: add acct_failed param to virtio_blk_handle_rw_error())
2/10 Checking commit 9d6734a0fed5 (virtio-blk: add host_features field in VirtIOBlock)
3/10 Checking commit 23b07b39fb93 (virtio-blk: add "discard" and "write-zeroes" properties)
4/10 Checking commit 26f46a5550f8 (virtio-net: make VirtIOFeature usable for other virtio devices)
5/10 Checking commit 7cc46c25d2e3 (virtio-blk: set config size depending on the features enabled)
6/10 Checking commit f2d45c1a148c (virtio-blk: add DISCARD and WRITE_ZEROES features)
7/10 Checking commit 0677a0653115 (tests/virtio-blk: change assert on data_size in virtio_blk_request())
8/10 Checking commit 1e74575a2e89 (tests/virtio-blk: add virtio_blk_fix_dwz_hdr() function)
ERROR: do not initialise globals to 0 or NULL
#24: FILE: tests/virtio-blk-test.c:52:
+const bool host_is_big_endian = false;

total: 1 errors, 0 warnings, 41 lines checked

Patch 8/10 has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

9/10 Checking commit 9dbf66647f0b (tests/virtio-blk: add test for WRITE_ZEROES command)
10/10 Checking commit 4e387bff7bb9 (tests/virtio-blk: add test for DISCARD command)
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190218140301.197408-1-sgarzare@redhat.com/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [Qemu-block] [PATCH v5 00/10] virtio-blk: add DISCARD and WRITE_ZEROES features

[Stefan Hajnoczi]

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

On Mon, Feb 18, 2019 at 03:02:51PM +0100, Stefano Garzarella wrote:
> This series adds the support of DISCARD and WRITE_ZEROES commands
> and extends the virtio-blk-test to test these new commands.
> 
> v5:
> - rebased on master
> - handled the config size for DISCARD and WRITE_ZEROES features as in
>   virtio-net (patches 4 and 5) [Michael, Stefan]
> - fixed an endianness issues on the WRITE_ZEROES test (patches 8 and 9)
> - added a test for DISCARD command to only test the status returned by the
>   request (patch 10)
> - others patches are unchanged (patches 1, 2, 3, 6, 7)

Looks good.  Please fix the patchew failure and resend.

Stefan

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

Re: [Qemu-devel] [Qemu-block] [PATCH v5 00/10] virtio-blk: add DISCARD and WRITE_ZEROES features

[Stefano Garzarella]

Il giorno mer 20 feb 2019 alle 17:31 Stefan Hajnoczi <stefanha@gmail.com>
ha scritto:

> On Mon, Feb 18, 2019 at 03:02:51PM +0100, Stefano Garzarella wrote:
> > This series adds the support of DISCARD and WRITE_ZEROES commands
> > and extends the virtio-blk-test to test these new commands.
> >
> > v5:
> > - rebased on master
> > - handled the config size for DISCARD and WRITE_ZEROES features as in
> >   virtio-net (patches 4 and 5) [Michael, Stefan]
> > - fixed an endianness issues on the WRITE_ZEROES test (patches 8 and 9)
> > - added a test for DISCARD command to only test the status returned by
> the
> >   request (patch 10)
> > - others patches are unchanged (patches 1, 2, 3, 6, 7)
>
> Looks good.  Please fix the patchew failure and resend.


Thanks for the review!
I’ll send the new version with the fix.

Cheers,
Stefano
-- 
Stefano Garzarella
Software Engineer @ Red Hat