All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features
@ 2019-02-06 11:27 Stefano Garzarella
  2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 1/6] virtio-blk: add acct_failed param to virtio_blk_handle_rw_error() Stefano Garzarella
                   ` (11 more replies)
  0 siblings, 12 replies; 17+ messages in thread
From: Stefano Garzarella @ 2019-02-06 11:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Dr . David Alan Gilbert, qemu-block, Thomas Huth,
	Michael S. Tsirkin, Max Reitz, Marcel Apfelbaum, Kevin Wolf,
	Eduardo Habkost, Stefan Hajnoczi, Laurent Vivier

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.

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 (6):
  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-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          | 213 +++++++++++++++++++++++++++++++--
 hw/core/machine.c              |   2 +
 include/hw/virtio/virtio-blk.h |   5 +-
 tests/virtio-blk-test.c        |  75 +++++++++++-
 4 files changed, 281 insertions(+), 14 deletions(-)

-- 
2.20.1

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH v3 1/6] virtio-blk: add acct_failed param to virtio_blk_handle_rw_error()
  2019-02-06 11:27 [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
@ 2019-02-06 11:27 ` Stefano Garzarella
  2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 2/6] virtio-blk: add host_features field in VirtIOBlock Stefano Garzarella
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 17+ messages in thread
From: Stefano Garzarella @ 2019-02-06 11:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Dr . David Alan Gilbert, qemu-block, Thomas Huth,
	Michael S. Tsirkin, Max Reitz, Marcel Apfelbaum, Kevin Wolf,
	Eduardo Habkost, Stefan Hajnoczi, Laurent Vivier

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 9a87b3bfac..b2bb129efa 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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH v3 2/6] virtio-blk: add host_features field in VirtIOBlock
  2019-02-06 11:27 [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
  2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 1/6] virtio-blk: add acct_failed param to virtio_blk_handle_rw_error() Stefano Garzarella
@ 2019-02-06 11:27 ` Stefano Garzarella
  2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 3/6] virtio-blk: add "discard" and "write-zeroes" properties Stefano Garzarella
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 17+ messages in thread
From: Stefano Garzarella @ 2019-02-06 11:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Dr . David Alan Gilbert, qemu-block, Thomas Huth,
	Michael S. Tsirkin, Max Reitz, Marcel Apfelbaum, Kevin Wolf,
	Eduardo Habkost, Stefan Hajnoczi, Laurent Vivier

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>
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 b2bb129efa..938273c63c 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -242,7 +242,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;
     }
@@ -783,12 +783,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;
         }
@@ -797,9 +800,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);
     }
@@ -1014,9 +1014,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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH v3 3/6] virtio-blk: add "discard" and "write-zeroes" properties
  2019-02-06 11:27 [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
  2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 1/6] virtio-blk: add acct_failed param to virtio_blk_handle_rw_error() Stefano Garzarella
  2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 2/6] virtio-blk: add host_features field in VirtIOBlock Stefano Garzarella
@ 2019-02-06 11:27 ` Stefano Garzarella
  2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 4/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 17+ messages in thread
From: Stefano Garzarella @ 2019-02-06 11:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Dr . David Alan Gilbert, qemu-block, Thomas Huth,
	Michael S. Tsirkin, Max Reitz, Marcel Apfelbaum, Kevin Wolf,
	Eduardo Habkost, Stefan Hajnoczi, Laurent Vivier

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>
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 938273c63c..6f2e86264d 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -1026,6 +1026,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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH v3 4/6] virtio-blk: add DISCARD and WRITE_ZEROES features
  2019-02-06 11:27 [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
                   ` (2 preceding siblings ...)
  2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 3/6] virtio-blk: add "discard" and "write-zeroes" properties Stefano Garzarella
@ 2019-02-06 11:27 ` Stefano Garzarella
  2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 5/6] tests/virtio-blk: change assert on data_size in virtio_blk_request() Stefano Garzarella
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 17+ messages in thread
From: Stefano Garzarella @ 2019-02-06 11:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Dr . David Alan Gilbert, qemu-block, Thomas Huth,
	Michael S. Tsirkin, Max Reitz, Marcel Apfelbaum, Kevin Wolf,
	Eduardo Habkost, Stefan Hajnoczi, Laurent Vivier

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>
Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
---
 hw/block/virtio-blk.c          | 183 +++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-blk.h |   2 +
 2 files changed, 185 insertions(+)

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 6f2e86264d..c9ab9b3a52 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_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 {
@@ -480,6 +504,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;
@@ -584,6 +686,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);
@@ -763,6 +906,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, sizeof(struct virtio_blk_config));
 }
 
@@ -954,6 +1115,24 @@ 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 %lu",
+                   conf->max_discard_sectors, 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 %lu",
+                   conf->max_write_zeroes_sectors, BDRV_REQUEST_MAX_SECTORS);
+        return;
+    }
+
     virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK,
                 sizeof(struct virtio_blk_config));
 
@@ -1030,6 +1209,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 f7345b0511..015b523fe0 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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH v3 5/6] tests/virtio-blk: change assert on data_size in virtio_blk_request()
  2019-02-06 11:27 [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
                   ` (3 preceding siblings ...)
  2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 4/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
@ 2019-02-06 11:27 ` Stefano Garzarella
  2019-02-06 11:32   ` Thomas Huth
  2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 6/6] tests/virtio-blk: add test for WRITE_ZEROES command Stefano Garzarella
                   ` (6 subsequent siblings)
  11 siblings, 1 reply; 17+ messages in thread
From: Stefano Garzarella @ 2019-02-06 11:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Dr . David Alan Gilbert, qemu-block, Thomas Huth,
	Michael S. Tsirkin, Max Reitz, Marcel Apfelbaum, Kevin Wolf,
	Eduardo Habkost, Stefan Hajnoczi, Laurent Vivier

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [Qemu-devel] [PATCH v3 6/6] tests/virtio-blk: add test for WRITE_ZEROES command
  2019-02-06 11:27 [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
                   ` (4 preceding siblings ...)
  2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 5/6] tests/virtio-blk: change assert on data_size in virtio_blk_request() Stefano Garzarella
@ 2019-02-06 11:27 ` Stefano Garzarella
  2019-02-06 11:39   ` Thomas Huth
  2019-02-07 19:21 ` [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features no-reply
                   ` (5 subsequent siblings)
  11 siblings, 1 reply; 17+ messages in thread
From: Stefano Garzarella @ 2019-02-06 11:27 UTC (permalink / raw)
  To: qemu-devel
  Cc: Paolo Bonzini, Dr . David Alan Gilbert, qemu-block, Thomas Huth,
	Michael S. Tsirkin, Max Reitz, Marcel Apfelbaum, Kevin Wolf,
	Eduardo Habkost, Stefan Hajnoczi, Laurent Vivier

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

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v3 5/6] tests/virtio-blk: change assert on data_size in virtio_blk_request()
  2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 5/6] tests/virtio-blk: change assert on data_size in virtio_blk_request() Stefano Garzarella
@ 2019-02-06 11:32   ` Thomas Huth
  0 siblings, 0 replies; 17+ messages in thread
From: Thomas Huth @ 2019-02-06 11:32 UTC (permalink / raw)
  To: Stefano Garzarella, qemu-devel
  Cc: Paolo Bonzini, Dr . David Alan Gilbert, qemu-block,
	Michael S. Tsirkin, Max Reitz, Marcel Apfelbaum, Kevin Wolf,
	Eduardo Habkost, Stefan Hajnoczi, Laurent Vivier

On 2019-02-06 12:27, 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.
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@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);

Reviewed-by: Thomas Huth <thuth@redhat.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v3 6/6] tests/virtio-blk: add test for WRITE_ZEROES command
  2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 6/6] tests/virtio-blk: add test for WRITE_ZEROES command Stefano Garzarella
@ 2019-02-06 11:39   ` Thomas Huth
  0 siblings, 0 replies; 17+ messages in thread
From: Thomas Huth @ 2019-02-06 11:39 UTC (permalink / raw)
  To: Stefano Garzarella, qemu-devel
  Cc: Paolo Bonzini, Dr . David Alan Gilbert, qemu-block,
	Michael S. Tsirkin, Max Reitz, Marcel Apfelbaum, Kevin Wolf,
	Eduardo Habkost, Stefan Hajnoczi, Laurent Vivier

On 2019-02-06 12:27, Stefano Garzarella wrote:
> 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>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
>  tests/virtio-blk-test.c | 60 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)

Acked-by: Thomas Huth <thuth@redhat.com>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features
  2019-02-06 11:27 [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
                   ` (5 preceding siblings ...)
  2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 6/6] tests/virtio-blk: add test for WRITE_ZEROES command Stefano Garzarella
@ 2019-02-07 19:21 ` no-reply
  2019-02-07 19:30 ` Michael S. Tsirkin
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 17+ messages in thread
From: no-reply @ 2019-02-07 19:21 UTC (permalink / raw)
  To: sgarzare
  Cc: fam, qemu-devel, kwolf, lvivier, thuth, ehabkost, qemu-block,
	mst, stefanha, dgilbert, mreitz, pbonzini

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



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14
=== TEST SCRIPT END ===

  CC      aarch64-softmmu/hw/arm/smmuv3.o
In file included from /tmp/qemu-test/src/hw/block/virtio-blk.c:15:
/tmp/qemu-test/src/hw/block/virtio-blk.c: In function 'virtio_blk_device_realize':
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1121:9: note: in expansion of macro 'error_setg'
         error_setg(errp, "invalid max-discard-sectors property (%" PRIu32 ")"
         ^~~~~~~~~~
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1130:9: note: in expansion of macro 'error_setg'
---
  CC      aarch64-softmmu/target/arm/translate-sve.o
In file included from /tmp/qemu-test/src/hw/block/virtio-blk.c:15:
/tmp/qemu-test/src/hw/block/virtio-blk.c: In function 'virtio_blk_device_realize':
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1121:9: note: in expansion of macro 'error_setg'
         error_setg(errp, "invalid max-discard-sectors property (%" PRIu32 ")"
         ^~~~~~~~~~
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1130:9: note: in expansion of macro 'error_setg'


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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features
  2019-02-06 11:27 [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
                   ` (6 preceding siblings ...)
  2019-02-07 19:21 ` [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features no-reply
@ 2019-02-07 19:30 ` Michael S. Tsirkin
  2019-02-07 20:17   ` Stefano Garzarella
  2019-02-08  0:28 ` no-reply
                   ` (3 subsequent siblings)
  11 siblings, 1 reply; 17+ messages in thread
From: Michael S. Tsirkin @ 2019-02-07 19:30 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: qemu-devel, Paolo Bonzini, Dr . David Alan Gilbert, qemu-block,
	Thomas Huth, Max Reitz, Marcel Apfelbaum, Kevin Wolf,
	Eduardo Habkost, Stefan Hajnoczi, Laurent Vivier

On Wed, Feb 06, 2019 at 12:27:23PM +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.

pls note and fix patchew errors:

https://patchew.org/QEMU/20190206112729.37761-1-sgarzare@redhat.com/

> 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 (6):
>   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-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          | 213 +++++++++++++++++++++++++++++++--
>  hw/core/machine.c              |   2 +
>  include/hw/virtio/virtio-blk.h |   5 +-
>  tests/virtio-blk-test.c        |  75 +++++++++++-
>  4 files changed, 281 insertions(+), 14 deletions(-)
> 
> -- 
> 2.20.1

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features
  2019-02-07 19:30 ` Michael S. Tsirkin
@ 2019-02-07 20:17   ` Stefano Garzarella
  0 siblings, 0 replies; 17+ messages in thread
From: Stefano Garzarella @ 2019-02-07 20:17 UTC (permalink / raw)
  To: Michael S. Tsirkin
  Cc: qemu devel list, Paolo Bonzini, Dr . David Alan Gilbert,
	qemu-block, Thomas Huth, Max Reitz, Marcel Apfelbaum, Kevin Wolf,
	Eduardo Habkost, Stefan Hajnoczi, Laurent Vivier

On Thu, Feb 7, 2019 at 8:30 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Feb 06, 2019 at 12:27:23PM +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.
>
> pls note and fix patchew errors:
>
> https://patchew.org/QEMU/20190206112729.37761-1-sgarzare@redhat.com/

Oh, thanks! I'll fix on v4.

Stefano

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features
  2019-02-06 11:27 [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
                   ` (7 preceding siblings ...)
  2019-02-07 19:30 ` Michael S. Tsirkin
@ 2019-02-08  0:28 ` no-reply
  2019-02-08  1:18 ` no-reply
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 17+ messages in thread
From: no-reply @ 2019-02-08  0:28 UTC (permalink / raw)
  To: sgarzare
  Cc: fam, qemu-devel, kwolf, lvivier, thuth, ehabkost, qemu-block,
	mst, stefanha, dgilbert, mreitz, pbonzini

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



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14
=== TEST SCRIPT END ===

  CC      x86_64-softmmu/target/i386/gdbstub.o
In file included from /tmp/qemu-test/src/hw/block/virtio-blk.c:15:
/tmp/qemu-test/src/hw/block/virtio-blk.c: In function 'virtio_blk_device_realize':
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1121:9: note: in expansion of macro 'error_setg'
         error_setg(errp, "invalid max-discard-sectors property (%" PRIu32 ")"
         ^~~~~~~~~~
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1130:9: note: in expansion of macro 'error_setg'
---
  CC      aarch64-softmmu/hw/misc/omap_l4.o
In file included from /tmp/qemu-test/src/hw/block/virtio-blk.c:15:
/tmp/qemu-test/src/hw/block/virtio-blk.c: In function 'virtio_blk_device_realize':
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1121:9: note: in expansion of macro 'error_setg'
         error_setg(errp, "invalid max-discard-sectors property (%" PRIu32 ")"
         ^~~~~~~~~~
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1130:9: note: in expansion of macro 'error_setg'


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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features
  2019-02-06 11:27 [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
                   ` (8 preceding siblings ...)
  2019-02-08  0:28 ` no-reply
@ 2019-02-08  1:18 ` no-reply
  2019-02-08  1:34 ` no-reply
  2019-02-08  6:06 ` Stefan Hajnoczi
  11 siblings, 0 replies; 17+ messages in thread
From: no-reply @ 2019-02-08  1:18 UTC (permalink / raw)
  To: sgarzare
  Cc: fam, qemu-devel, kwolf, lvivier, thuth, ehabkost, qemu-block,
	mst, stefanha, dgilbert, mreitz, pbonzini

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



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14
=== TEST SCRIPT END ===

  CC      aarch64-softmmu/hw/misc/tz-msc.o
In file included from /tmp/qemu-test/src/hw/block/virtio-blk.c:15:
/tmp/qemu-test/src/hw/block/virtio-blk.c: In function 'virtio_blk_device_realize':
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1121:9: note: in expansion of macro 'error_setg'
         error_setg(errp, "invalid max-discard-sectors property (%" PRIu32 ")"
         ^~~~~~~~~~
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1130:9: note: in expansion of macro 'error_setg'
---
  CC      aarch64-softmmu/hw/misc/aspeed_scu.o
In file included from /tmp/qemu-test/src/hw/block/virtio-blk.c:15:
/tmp/qemu-test/src/hw/block/virtio-blk.c: In function 'virtio_blk_device_realize':
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1121:9: note: in expansion of macro 'error_setg'
         error_setg(errp, "invalid max-discard-sectors property (%" PRIu32 ")"
         ^~~~~~~~~~
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1130:9: note: in expansion of macro 'error_setg'


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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features
  2019-02-06 11:27 [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
                   ` (9 preceding siblings ...)
  2019-02-08  1:18 ` no-reply
@ 2019-02-08  1:34 ` no-reply
  2019-02-08  6:06 ` Stefan Hajnoczi
  11 siblings, 0 replies; 17+ messages in thread
From: no-reply @ 2019-02-08  1:34 UTC (permalink / raw)
  To: sgarzare
  Cc: fam, qemu-devel, kwolf, lvivier, thuth, ehabkost, qemu-block,
	mst, stefanha, dgilbert, mreitz, pbonzini

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



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14
=== TEST SCRIPT END ===

  CC      x86_64-softmmu/target/i386/int_helper.o
In file included from /tmp/qemu-test/src/hw/block/virtio-blk.c:15:
/tmp/qemu-test/src/hw/block/virtio-blk.c: In function 'virtio_blk_device_realize':
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1121:9: note: in expansion of macro 'error_setg'
         error_setg(errp, "invalid max-discard-sectors property (%" PRIu32 ")"
         ^~~~~~~~~~
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1130:9: note: in expansion of macro 'error_setg'
---
  CC      aarch64-softmmu/hw/misc/auxbus.o
In file included from /tmp/qemu-test/src/hw/block/virtio-blk.c:15:
/tmp/qemu-test/src/hw/block/virtio-blk.c: In function 'virtio_blk_device_realize':
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1121:9: note: in expansion of macro 'error_setg'
         error_setg(errp, "invalid max-discard-sectors property (%" PRIu32 ")"
         ^~~~~~~~~~
/tmp/qemu-test/src/include/qapi/error.h:166:25: error: format '%lu' expects argument of type 'long unsigned int', but argument 7 has type 'long long unsigned int' [-Werror=format=]
                         (fmt), ## __VA_ARGS__)
                         ^~~~~
/tmp/qemu-test/src/hw/block/virtio-blk.c:1130:9: note: in expansion of macro 'error_setg'


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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features
  2019-02-06 11:27 [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
                   ` (10 preceding siblings ...)
  2019-02-08  1:34 ` no-reply
@ 2019-02-08  6:06 ` Stefan Hajnoczi
  2019-02-08  8:04   ` Stefano Garzarella
  11 siblings, 1 reply; 17+ messages in thread
From: Stefan Hajnoczi @ 2019-02-08  6:06 UTC (permalink / raw)
  To: Stefano Garzarella
  Cc: qemu-devel, Paolo Bonzini, Dr . David Alan Gilbert, qemu-block,
	Thomas Huth, Michael S. Tsirkin, Max Reitz, Marcel Apfelbaum,
	Kevin Wolf, Eduardo Habkost, Laurent Vivier

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

On Wed, Feb 06, 2019 at 12:27:23PM +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.
> 
> 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 (6):
>   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-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          | 213 +++++++++++++++++++++++++++++++--
>  hw/core/machine.c              |   2 +
>  include/hw/virtio/virtio-blk.h |   5 +-
>  tests/virtio-blk-test.c        |  75 +++++++++++-
>  4 files changed, 281 insertions(+), 14 deletions(-)
> 
> -- 
> 2.20.1
> 

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

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features
  2019-02-08  6:06 ` Stefan Hajnoczi
@ 2019-02-08  8:04   ` Stefano Garzarella
  0 siblings, 0 replies; 17+ messages in thread
From: Stefano Garzarella @ 2019-02-08  8:04 UTC (permalink / raw)
  To: Stefan Hajnoczi
  Cc: qemu-devel, Paolo Bonzini, Dr . David Alan Gilbert, qemu-block,
	Thomas Huth, Michael S. Tsirkin, Max Reitz, Marcel Apfelbaum,
	Kevin Wolf, Eduardo Habkost, Laurent Vivier

On Fri, Feb 08, 2019 at 02:06:18PM +0800, Stefan Hajnoczi wrote:
> On Wed, Feb 06, 2019 at 12:27:23PM +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.
> > 
> 
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Thanks Stefan!

I'll send a v4 to fix the error with mingw compiler.

Cheers,
Stefano

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2019-02-08  8:05 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-02-06 11:27 [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 1/6] virtio-blk: add acct_failed param to virtio_blk_handle_rw_error() Stefano Garzarella
2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 2/6] virtio-blk: add host_features field in VirtIOBlock Stefano Garzarella
2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 3/6] virtio-blk: add "discard" and "write-zeroes" properties Stefano Garzarella
2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 4/6] virtio-blk: add DISCARD and WRITE_ZEROES features Stefano Garzarella
2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 5/6] tests/virtio-blk: change assert on data_size in virtio_blk_request() Stefano Garzarella
2019-02-06 11:32   ` Thomas Huth
2019-02-06 11:27 ` [Qemu-devel] [PATCH v3 6/6] tests/virtio-blk: add test for WRITE_ZEROES command Stefano Garzarella
2019-02-06 11:39   ` Thomas Huth
2019-02-07 19:21 ` [Qemu-devel] [PATCH v3 0/6] virtio-blk: add DISCARD and WRITE_ZEROES features no-reply
2019-02-07 19:30 ` Michael S. Tsirkin
2019-02-07 20:17   ` Stefano Garzarella
2019-02-08  0:28 ` no-reply
2019-02-08  1:18 ` no-reply
2019-02-08  1:34 ` no-reply
2019-02-08  6:06 ` Stefan Hajnoczi
2019-02-08  8:04   ` Stefano Garzarella

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.