[Qemu-devel] [PATCH v2 0/4] compressed block-stream

[Qemu-devel] [PATCH v2 0/4] compressed block-stream

[Anton Nefedov]

v2:
  - 1st patch omitted (applied to 2.11)
  - remarks applied

v1: http://lists.nongnu.org/archive/html/qemu-devel/2017-11/msg02515.html

It might be useful to compress images during block-stream;
this way the user can merge compressed images of a backing chain and
the result will remain compressed.

Anton Nefedov (3):
  block: support compressed write for copy-on-read
  block-stream: add compress option
  iotests: 030: add compressed block-stream test

Pavel Butsykin (1):
  qcow2: multiple clusters write compressed

 qapi/block-core.json       |  4 +++
 include/block/block_int.h  |  4 ++-
 block/io.c                 | 23 +++++++++++----
 block/qcow2.c              | 73 +++++++++++++++++++++++++++++++++-------------
 block/stream.c             | 16 +++++++---
 blockdev.c                 | 13 ++++++++-
 hmp.c                      |  2 ++
 block/trace-events         |  2 +-
 hmp-commands.hx            |  4 +--
 tests/qemu-iotests/030     | 66 ++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/030.out |  4 +--
 11 files changed, 172 insertions(+), 39 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/4] qcow2: multiple clusters write compressed

[Anton Nefedov]

From: Pavel Butsykin <pbutsykin@virtuozzo.com>

At the moment, qcow2_co_pwritev_compressed can process the requests size
less than or equal to one cluster. This patch added possibility to write
compressed data in the QCOW2 more than one cluster. The implementation
is simple, we just split large requests into separate clusters and write
using existing functionality.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 block/qcow2.c | 73 ++++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 52 insertions(+), 21 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index c276b24..f1e2759 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3332,11 +3332,9 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset,
     return 0;
 }
 
-/* XXX: put compressed sectors first, then all the cluster aligned
-   tables to avoid losing bytes in alignment */
 static coroutine_fn int
-qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
-                            uint64_t bytes, QEMUIOVector *qiov)
+qcow2_co_pwritev_cluster_compressed(BlockDriverState *bs, uint64_t offset,
+                                    uint64_t bytes, QEMUIOVector *qiov)
 {
     BDRVQcow2State *s = bs->opaque;
     QEMUIOVector hd_qiov;
@@ -3346,25 +3344,12 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
     uint8_t *buf, *out_buf;
     int64_t cluster_offset;
 
-    if (bytes == 0) {
-        /* align end of file to a sector boundary to ease reading with
-           sector based I/Os */
-        cluster_offset = bdrv_getlength(bs->file->bs);
-        if (cluster_offset < 0) {
-            return cluster_offset;
-        }
-        return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, NULL);
-    }
-
-    if (offset_into_cluster(s, offset)) {
-        return -EINVAL;
-    }
+    assert(bytes <= s->cluster_size);
+    assert(!offset_into_cluster(s, offset));
 
     buf = qemu_blockalign(bs, s->cluster_size);
-    if (bytes != s->cluster_size) {
-        if (bytes > s->cluster_size ||
-            offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS)
-        {
+    if (bytes < s->cluster_size) {
+        if (offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS) {
             qemu_vfree(buf);
             return -EINVAL;
         }
@@ -3444,6 +3429,52 @@ fail:
     return ret;
 }
 
+/* XXX: put compressed sectors first, then all the cluster aligned
+   tables to avoid losing bytes in alignment */
+static coroutine_fn int
+qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
+                            uint64_t bytes, QEMUIOVector *qiov)
+{
+    BDRVQcow2State *s = bs->opaque;
+    QEMUIOVector hd_qiov;
+    uint64_t curr_off = 0;
+    int ret;
+
+    if (bytes == 0) {
+        /* align end of file to a sector boundary to ease reading with
+           sector based I/Os */
+        int64_t cluster_offset = bdrv_getlength(bs->file->bs);
+        if (cluster_offset < 0) {
+            return cluster_offset;
+        }
+        return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, NULL);
+    }
+
+    if (offset_into_cluster(s, offset)) {
+        return -EINVAL;
+    }
+
+    qemu_iovec_init(&hd_qiov, qiov->niov);
+    do {
+        uint32_t chunk_size;
+
+        qemu_iovec_reset(&hd_qiov);
+        chunk_size = MIN(bytes, s->cluster_size);
+        qemu_iovec_concat(&hd_qiov, qiov, curr_off, chunk_size);
+
+        ret = qcow2_co_pwritev_cluster_compressed(bs, offset + curr_off,
+                                                  chunk_size, &hd_qiov);
+        if (ret < 0) {
+            break;
+        }
+        curr_off += chunk_size;
+        bytes -= chunk_size;
+    } while (bytes);
+    qemu_iovec_destroy(&hd_qiov);
+
+    return ret;
+}
+
 static int make_completely_empty(BlockDriverState *bs)
 {
     BDRVQcow2State *s = bs->opaque;
-- 
2.7.4

[Qemu-devel] [PATCH v2 2/4] block: support compressed write for copy-on-read

[Anton Nefedov]

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 block/io.c         | 23 +++++++++++++++++------
 block/trace-events |  2 +-
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/block/io.c b/block/io.c
index 3d5ef2c..2ba62a3 100644
--- a/block/io.c
+++ b/block/io.c
@@ -953,7 +953,7 @@ bdrv_driver_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
 }
 
 static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
-        int64_t offset, unsigned int bytes, QEMUIOVector *qiov)
+        int64_t offset, unsigned int bytes, QEMUIOVector *qiov, int flags)
 {
     BlockDriverState *bs = child->bs;
 
@@ -988,12 +988,13 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
      * allocating cluster in the image file.  Note that this value may exceed
      * BDRV_REQUEST_MAX_BYTES (even when the original read did not), which
      * is one reason we loop rather than doing it all at once.
+     * Also this is crucial for compressed copy-on-read.
      */
     bdrv_round_to_clusters(bs, offset, bytes, &cluster_offset, &cluster_bytes);
     skip_bytes = offset - cluster_offset;
 
     trace_bdrv_co_do_copy_on_readv(bs, offset, bytes,
-                                   cluster_offset, cluster_bytes);
+                                   cluster_offset, cluster_bytes, flags);
 
     bounce_buffer = qemu_try_blockalign(bs,
                                         MIN(MIN(max_transfer, cluster_bytes),
@@ -1041,8 +1042,13 @@ static int coroutine_fn bdrv_co_do_copy_on_readv(BdrvChild *child,
                 /* This does not change the data on the disk, it is not
                  * necessary to flush even in cache=writethrough mode.
                  */
-                ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
-                                          &local_qiov, 0);
+                if (flags & BDRV_REQ_WRITE_COMPRESSED) {
+                    ret = bdrv_driver_pwritev_compressed(bs, cluster_offset,
+                                                         pnum, &local_qiov);
+                } else {
+                    ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
+                                              &local_qiov, 0);
+                }
             }
 
             if (ret < 0) {
@@ -1107,7 +1113,12 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
      * potential fallback support, if we ever implement any read flags
      * to pass through to drivers.  For now, there aren't any
      * passthrough flags.  */
-    assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ)));
+    assert(!(flags & ~(BDRV_REQ_NO_SERIALISING | BDRV_REQ_COPY_ON_READ |
+                       BDRV_REQ_WRITE_COMPRESSED)));
+
+    /* write compressed only makes sense with copy on read */
+    assert(!(flags & BDRV_REQ_WRITE_COMPRESSED) ||
+           (flags & BDRV_REQ_COPY_ON_READ));
 
     /* Handle Copy on Read and associated serialisation */
     if (flags & BDRV_REQ_COPY_ON_READ) {
@@ -1132,7 +1143,7 @@ static int coroutine_fn bdrv_aligned_preadv(BdrvChild *child,
         }
 
         if (!ret || pnum != bytes) {
-            ret = bdrv_co_do_copy_on_readv(child, offset, bytes, qiov);
+            ret = bdrv_co_do_copy_on_readv(child, offset, bytes, qiov, flags);
             goto out;
         }
     }
diff --git a/block/trace-events b/block/trace-events
index 11c8d5f..12fe188 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -12,7 +12,7 @@ blk_co_pwritev(void *blk, void *bs, int64_t offset, unsigned int bytes, int flag
 bdrv_co_preadv(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
 bdrv_co_pwritev(void *bs, int64_t offset, int64_t nbytes, unsigned int flags) "bs %p offset %"PRId64" nbytes %"PRId64" flags 0x%x"
 bdrv_co_pwrite_zeroes(void *bs, int64_t offset, int count, int flags) "bs %p offset %"PRId64" count %d flags 0x%x"
-bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t cluster_offset, int64_t cluster_bytes) "bs %p offset %"PRId64" bytes %u cluster_offset %"PRId64" cluster_bytes %"PRId64
+bdrv_co_do_copy_on_readv(void *bs, int64_t offset, unsigned int bytes, int64_t cluster_offset, int64_t cluster_bytes, int flags) "bs %p offset %"PRId64" bytes %u cluster_offset %"PRId64" cluster_bytes %"PRId64" flags 0x%x"
 
 # block/stream.c
 stream_one_iteration(void *s, int64_t offset, uint64_t bytes, int is_allocated) "s %p offset %" PRId64 " bytes %" PRIu64 " is_allocated %d"
-- 
2.7.4

[Qemu-devel] [PATCH v2 3/4] block-stream: add compress option

[Anton Nefedov]

It might be useful to compress images during block-stream;
this way the user can merge compressed images of a backing chain and
the result will remain compressed.

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 qapi/block-core.json      |  4 ++++
 include/block/block_int.h |  4 +++-
 block/stream.c            | 16 ++++++++++++----
 blockdev.c                | 13 ++++++++++++-
 hmp.c                     |  2 ++
 hmp-commands.hx           |  4 ++--
 6 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index ab96e34..b7282cd 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2007,6 +2007,9 @@
 #
 # @speed:  the maximum speed, in bytes per second
 #
+# @compress: true to compress data; may only be set if the target format
+#            supports it (default: false). (Since 2.12)
+#
 # @on-error: the action to take on an error (default report).
 #            'stop' and 'enospc' can only be used if the block device
 #            supports io-status (see BlockInfo).  Since 1.3.
@@ -2026,6 +2029,7 @@
 { 'command': 'block-stream',
   'data': { '*job-id': 'str', 'device': 'str', '*base': 'str',
             '*base-node': 'str', '*backing-file': 'str', '*speed': 'int',
+            '*compress': 'bool',
             '*on-error': 'BlockdevOnError' } }
 
 ##
diff --git a/include/block/block_int.h b/include/block/block_int.h
index a548277..093bf9b 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -863,6 +863,7 @@ int is_windows_drive(const char *filename);
  * @backing_file_str: The file name that will be written to @bs as the
  * the new backing file if the job completes. Ignored if @base is %NULL.
  * @speed: The maximum speed, in bytes per second, or 0 for unlimited.
+ * @compress: True to compress data.
  * @on_error: The action to take upon error.
  * @errp: Error object.
  *
@@ -875,7 +876,8 @@ int is_windows_drive(const char *filename);
  */
 void stream_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *base, const char *backing_file_str,
-                  int64_t speed, BlockdevOnError on_error, Error **errp);
+                  int64_t speed, bool compress,
+                  BlockdevOnError on_error, Error **errp);
 
 /**
  * commit_start:
diff --git a/block/stream.c b/block/stream.c
index e6f7234..75c9d66 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -38,23 +38,29 @@ typedef struct StreamBlockJob {
     BlockdevOnError on_error;
     char *backing_file_str;
     int bs_flags;
+    bool compress;
 } StreamBlockJob;
 
 static int coroutine_fn stream_populate(BlockBackend *blk,
                                         int64_t offset, uint64_t bytes,
-                                        void *buf)
+                                        void *buf, bool compress)
 {
     struct iovec iov = {
         .iov_base = buf,
         .iov_len  = bytes,
     };
     QEMUIOVector qiov;
+    int flags = BDRV_REQ_COPY_ON_READ;
+
+    if (compress) {
+        flags |= BDRV_REQ_WRITE_COMPRESSED;
+    }
 
     assert(bytes < SIZE_MAX);
     qemu_iovec_init_external(&qiov, &iov, 1);
 
     /* Copy-on-read the unallocated clusters */
-    return blk_co_preadv(blk, offset, qiov.size, &qiov, BDRV_REQ_COPY_ON_READ);
+    return blk_co_preadv(blk, offset, qiov.size, &qiov, flags);
 }
 
 typedef struct {
@@ -166,7 +172,7 @@ static void coroutine_fn stream_run(void *opaque)
         }
         trace_stream_one_iteration(s, offset, n, ret);
         if (copy) {
-            ret = stream_populate(blk, offset, n, buf);
+            ret = stream_populate(blk, offset, n, buf, s->compress);
         }
         if (ret < 0) {
             BlockErrorAction action =
@@ -227,7 +233,8 @@ static const BlockJobDriver stream_job_driver = {
 
 void stream_start(const char *job_id, BlockDriverState *bs,
                   BlockDriverState *base, const char *backing_file_str,
-                  int64_t speed, BlockdevOnError on_error, Error **errp)
+                  int64_t speed, bool compress,
+                  BlockdevOnError on_error, Error **errp)
 {
     StreamBlockJob *s;
     BlockDriverState *iter;
@@ -267,6 +274,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
     s->base = base;
     s->backing_file_str = g_strdup(backing_file_str);
     s->bs_flags = orig_bs_flags;
+    s->compress = compress;
 
     s->on_error = on_error;
     trace_stream_start(bs, base, s);
diff --git a/blockdev.c b/blockdev.c
index 56a6b24..ae2a1d2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2968,6 +2968,7 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
                       bool has_base_node, const char *base_node,
                       bool has_backing_file, const char *backing_file,
                       bool has_speed, int64_t speed,
+                      bool has_compress, bool compress,
                       bool has_on_error, BlockdevOnError on_error,
                       Error **errp)
 {
@@ -2981,6 +2982,10 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         on_error = BLOCKDEV_ON_ERROR_REPORT;
     }
 
+    if (!has_compress) {
+        compress = false;
+    }
+
     bs = bdrv_lookup_bs(device, device, errp);
     if (!bs) {
         return;
@@ -3034,11 +3039,17 @@ void qmp_block_stream(bool has_job_id, const char *job_id, const char *device,
         goto out;
     }
 
+    if (compress && bs->drv->bdrv_co_pwritev_compressed == NULL) {
+        error_setg(errp, "Compression is not supported for this drive %s",
+                   device);
+        goto out;
+    }
+
     /* backing_file string overrides base bs filename */
     base_name = has_backing_file ? backing_file : base_name;
 
     stream_start(has_job_id ? job_id : NULL, bs, base_bs, base_name,
-                 has_speed ? speed : 0, on_error, &local_err);
+                 has_speed ? speed : 0, compress, on_error, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
         goto out;
diff --git a/hmp.c b/hmp.c
index 35a7041..854c88e 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1812,9 +1812,11 @@ void hmp_block_stream(Monitor *mon, const QDict *qdict)
     const char *device = qdict_get_str(qdict, "device");
     const char *base = qdict_get_try_str(qdict, "base");
     int64_t speed = qdict_get_try_int(qdict, "speed", 0);
+    bool compress = qdict_get_try_bool(qdict, "compress", false);
 
     qmp_block_stream(true, device, device, base != NULL, base, false, NULL,
                      false, NULL, qdict_haskey(qdict, "speed"), speed,
+                     qdict_haskey(qdict, "compress"), compress,
                      true, BLOCKDEV_ON_ERROR_REPORT, &error);
 
     hmp_handle_error(mon, &error);
diff --git a/hmp-commands.hx b/hmp-commands.hx
index 4afd57c..f6794bb 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -75,8 +75,8 @@ ETEXI
 
     {
         .name       = "block_stream",
-        .args_type  = "device:B,speed:o?,base:s?",
-        .params     = "device [speed [base]]",
+        .args_type  = "device:B,speed:o?,base:s?,compress:o?",
+        .params     = "device [speed [base]] [compress]",
         .help       = "copy data from a backing file into a block device",
         .cmd        = hmp_block_stream,
     },
-- 
2.7.4

[Qemu-devel] [PATCH v2 4/4] iotests: 030: add compressed block-stream test

[Anton Nefedov]

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 tests/qemu-iotests/030     | 66 +++++++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/030.out |  4 +--
 2 files changed, 67 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 457984b..831d6f3 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -21,7 +21,7 @@
 import time
 import os
 import iotests
-from iotests import qemu_img, qemu_io
+from iotests import qemu_img, qemu_img_pipe, qemu_io
 
 backing_img = os.path.join(iotests.test_dir, 'backing.img')
 mid_img = os.path.join(iotests.test_dir, 'mid.img')
@@ -804,5 +804,69 @@ class TestSetSpeed(iotests.QMPTestCase):
 
         self.cancel_and_wait(resume=True)
 
+class TestCompressed(iotests.QMPTestCase):
+    supported_fmts = ['qcow2']
+    cluster_size = 64 * 1024;
+    image_len = 1 * 1024 * 1024;
+
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d' % TestCompressed.cluster_size, backing_img, str(TestCompressed.image_len))
+        qemu_io('-c', 'write -P 1 0 ' + str(TestCompressed.image_len), backing_img)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s,cluster_size=%d' % (backing_img, TestCompressed.cluster_size), test_img)
+
+        # write '3' in every 3rd cluster
+        step = 3
+        for off in range(0, TestCompressed.image_len, TestCompressed.cluster_size * step):
+            qemu_io('-c', 'write -P %d %d %d' %
+                    (step, off, TestCompressed.cluster_size), test_img)
+
+        self.vm = iotests.VM().add_drive(test_img)
+        self.vm.launch()
+
+    def tearDown(self):
+        os.remove(test_img)
+        os.remove(backing_img)
+
+    def _first_divider(self, x, divs):
+        return divs[0] if not x%divs[0] else self._first_divider(x, divs[1:])
+
+    def test_compressed(self):
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp('block-stream', device='drive0', compress=True)
+        if iotests.imgfmt not in TestCompressed.supported_fmts:
+            self.assert_qmp(
+                result, 'error/desc',
+                'Compression is not supported for this drive drive0')
+            return
+        self.assert_qmp(result, 'return', {})
+
+        # write '4' in every 4th cluster
+        step = 4
+        for off in range(0, TestCompressed.image_len, TestCompressed.cluster_size * step):
+            result = self.vm.qmp('human-monitor-command',
+                                 command_line=
+                                 'qemu-io drive0 "write -P %d %d %d"' %
+                                 (step, off, TestCompressed.cluster_size))
+            self.assert_qmp(result, 'return', "")
+
+        self.wait_until_completed()
+        self.assert_no_active_block_jobs()
+
+        self.vm.shutdown()
+
+        for i in range(TestCompressed.image_len / TestCompressed.cluster_size):
+            outp = qemu_io('-c', 'read -P %d %d %d' %
+                           (self._first_divider(i, [4, 3, 1]),
+                            i * TestCompressed.cluster_size,
+                            TestCompressed.cluster_size),
+                           test_img)
+            self.assertTrue(not 'fail' in outp)
+            self.assertTrue('read' in outp and 'at offset' in outp)
+
+        self.assertTrue(
+            "File contains external, encrypted or compressed clusters."
+            in qemu_img_pipe('map', test_img))
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index 391c857..42314e9 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-.......................
+........................
 ----------------------------------------------------------------------
-Ran 23 tests
+Ran 24 tests
 
 OK
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 4/4] iotests: 030: add compressed block-stream test

[Stefan Hajnoczi]

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

On Thu, Nov 16, 2017 at 07:54:58PM +0300, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>  tests/qemu-iotests/030     | 66 +++++++++++++++++++++++++++++++++++++++++++++-
>  tests/qemu-iotests/030.out |  4 +--
>  2 files changed, 67 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index 457984b..831d6f3 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -21,7 +21,7 @@
>  import time
>  import os
>  import iotests
> -from iotests import qemu_img, qemu_io
> +from iotests import qemu_img, qemu_img_pipe, qemu_io
>  
>  backing_img = os.path.join(iotests.test_dir, 'backing.img')
>  mid_img = os.path.join(iotests.test_dir, 'mid.img')
> @@ -804,5 +804,69 @@ class TestSetSpeed(iotests.QMPTestCase):
>  
>          self.cancel_and_wait(resume=True)
>  
> +class TestCompressed(iotests.QMPTestCase):
> +    supported_fmts = ['qcow2']
> +    cluster_size = 64 * 1024;
> +    image_len = 1 * 1024 * 1024;

Please drop the unnecessary semicolons

> +
> +    def setUp(self):
> +        qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d' % TestCompressed.cluster_size, backing_img, str(TestCompressed.image_len))
> +        qemu_io('-c', 'write -P 1 0 ' + str(TestCompressed.image_len), backing_img)
> +        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s,cluster_size=%d' % (backing_img, TestCompressed.cluster_size), test_img)
> +
> +        # write '3' in every 3rd cluster
> +        step = 3
> +        for off in range(0, TestCompressed.image_len, TestCompressed.cluster_size * step):
> +            qemu_io('-c', 'write -P %d %d %d' %
> +                    (step, off, TestCompressed.cluster_size), test_img)
> +
> +        self.vm = iotests.VM().add_drive(test_img)
> +        self.vm.launch()
> +
> +    def tearDown(self):
> +        os.remove(test_img)
> +        os.remove(backing_img)
> +
> +    def _first_divider(self, x, divs):

"Divisor" or "factor":
https://en.wikipedia.org/wiki/Divisor

> +        return divs[0] if not x%divs[0] else self._first_divider(x, divs[1:])

An alternative that I find easier to read than conditional expressions:

  for divisor in divs:
      if x % divisor == 0:
          return divisor
  raise ValueError('No suitable divisor found')

> +
> +    def test_compressed(self):
> +        self.assert_no_active_block_jobs()
> +
> +        result = self.vm.qmp('block-stream', device='drive0', compress=True)
> +        if iotests.imgfmt not in TestCompressed.supported_fmts:
> +            self.assert_qmp(
> +                result, 'error/desc',
> +                'Compression is not supported for this drive drive0')
> +            return
> +        self.assert_qmp(result, 'return', {})
> +
> +        # write '4' in every 4th cluster
> +        step = 4
> +        for off in range(0, TestCompressed.image_len, TestCompressed.cluster_size * step):
> +            result = self.vm.qmp('human-monitor-command',
> +                                 command_line=
> +                                 'qemu-io drive0 "write -P %d %d %d"' %
> +                                 (step, off, TestCompressed.cluster_size))
> +            self.assert_qmp(result, 'return', "")
> +
> +        self.wait_until_completed()
> +        self.assert_no_active_block_jobs()
> +
> +        self.vm.shutdown()

It is safe to call self.vm.shutdown() multiple times.  Please call it
from tearDown() too so the QEMU process is cleaned up on failure.

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

Re: [Qemu-devel] [PATCH v2 1/4] qcow2: multiple clusters write compressed

[Stefan Hajnoczi]

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

On Thu, Nov 16, 2017 at 07:54:55PM +0300, Anton Nefedov wrote:
> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> 
> At the moment, qcow2_co_pwritev_compressed can process the requests size
> less than or equal to one cluster. This patch added possibility to write
> compressed data in the QCOW2 more than one cluster. The implementation
> is simple, we just split large requests into separate clusters and write
> using existing functionality.
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>  block/qcow2.c | 73 ++++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 52 insertions(+), 21 deletions(-)

Kevin: qcow2_alloc_compressed_cluster_offset() sets up an L2 table entry
before doing the compressed data write.  Can the following scenario
occur if there is another request racing with the compressed write?

1. Compressed cluster L2 table entry added to qcow2 in-memory cache
2. Another request forces cached L2 table to be written to disk
3. Power failure or crash before compressed data is written

Now there is an L2 table entry pointing to garbage data.  This violates
qcow2's data integrity model.

I'm not sure if compressed writes are safe...  It may have been okay for
qemu-img convert but the risk is increased when running a VM.

> diff --git a/block/qcow2.c b/block/qcow2.c
> index c276b24..f1e2759 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3332,11 +3332,9 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset,
>      return 0;
>  }
>  
> -/* XXX: put compressed sectors first, then all the cluster aligned
> -   tables to avoid losing bytes in alignment */
>  static coroutine_fn int
> -qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
> -                            uint64_t bytes, QEMUIOVector *qiov)
> +qcow2_co_pwritev_cluster_compressed(BlockDriverState *bs, uint64_t offset,
> +                                    uint64_t bytes, QEMUIOVector *qiov)
>  {
>      BDRVQcow2State *s = bs->opaque;
>      QEMUIOVector hd_qiov;
> @@ -3346,25 +3344,12 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
>      uint8_t *buf, *out_buf;
>      int64_t cluster_offset;
>  
> -    if (bytes == 0) {
> -        /* align end of file to a sector boundary to ease reading with
> -           sector based I/Os */
> -        cluster_offset = bdrv_getlength(bs->file->bs);
> -        if (cluster_offset < 0) {
> -            return cluster_offset;
> -        }
> -        return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, NULL);
> -    }
> -
> -    if (offset_into_cluster(s, offset)) {
> -        return -EINVAL;
> -    }
> +    assert(bytes <= s->cluster_size);
> +    assert(!offset_into_cluster(s, offset));
>  
>      buf = qemu_blockalign(bs, s->cluster_size);
> -    if (bytes != s->cluster_size) {
> -        if (bytes > s->cluster_size ||
> -            offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS)
> -        {
> +    if (bytes < s->cluster_size) {
> +        if (offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS) {
>              qemu_vfree(buf);
>              return -EINVAL;
>          }
> @@ -3444,6 +3429,52 @@ fail:
>      return ret;
>  }
>  
> +/* XXX: put compressed sectors first, then all the cluster aligned
> +   tables to avoid losing bytes in alignment */
> +static coroutine_fn int
> +qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
> +                            uint64_t bytes, QEMUIOVector *qiov)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    QEMUIOVector hd_qiov;
> +    uint64_t curr_off = 0;
> +    int ret;
> +
> +    if (bytes == 0) {
> +        /* align end of file to a sector boundary to ease reading with
> +           sector based I/Os */
> +        int64_t cluster_offset = bdrv_getlength(bs->file->bs);
> +        if (cluster_offset < 0) {
> +            return cluster_offset;
> +        }
> +        return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, NULL);
> +    }
> +
> +    if (offset_into_cluster(s, offset)) {
> +        return -EINVAL;
> +    }
> +
> +    qemu_iovec_init(&hd_qiov, qiov->niov);
> +    do {
> +        uint32_t chunk_size;
> +
> +        qemu_iovec_reset(&hd_qiov);
> +        chunk_size = MIN(bytes, s->cluster_size);
> +        qemu_iovec_concat(&hd_qiov, qiov, curr_off, chunk_size);
> +
> +        ret = qcow2_co_pwritev_cluster_compressed(bs, offset + curr_off,
> +                                                  chunk_size, &hd_qiov);
> +        if (ret < 0) {
> +            break;
> +        }
> +        curr_off += chunk_size;
> +        bytes -= chunk_size;
> +    } while (bytes);
> +    qemu_iovec_destroy(&hd_qiov);
> +
> +    return ret;
> +}
> +
>  static int make_completely_empty(BlockDriverState *bs)
>  {
>      BDRVQcow2State *s = bs->opaque;
> -- 
> 2.7.4
> 
> 

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

Re: [Qemu-devel] [Qemu-block] [PATCH v2 2/4] block: support compressed write for copy-on-read

[Stefan Hajnoczi]

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

On Thu, Nov 16, 2017 at 07:54:56PM +0300, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block/io.c         | 23 +++++++++++++++++------
>  block/trace-events |  2 +-
>  2 files changed, 18 insertions(+), 7 deletions(-)

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

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

Re: [Qemu-devel] [Qemu-block] [PATCH v2 3/4] block-stream: add compress option

[Stefan Hajnoczi]

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

On Thu, Nov 16, 2017 at 07:54:57PM +0300, Anton Nefedov wrote:
> It might be useful to compress images during block-stream;
> this way the user can merge compressed images of a backing chain and
> the result will remain compressed.
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>  qapi/block-core.json      |  4 ++++
>  include/block/block_int.h |  4 +++-
>  block/stream.c            | 16 ++++++++++++----
>  blockdev.c                | 13 ++++++++++++-
>  hmp.c                     |  2 ++
>  hmp-commands.hx           |  4 ++--
>  6 files changed, 35 insertions(+), 8 deletions(-)

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

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

Re: [Qemu-devel] [PATCH v2 1/4] qcow2: multiple clusters write compressed

[Denis V. Lunev]

On 11/20/2017 05:53 PM, Stefan Hajnoczi wrote:
> On Thu, Nov 16, 2017 at 07:54:55PM +0300, Anton Nefedov wrote:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> At the moment, qcow2_co_pwritev_compressed can process the requests size
>> less than or equal to one cluster. This patch added possibility to write
>> compressed data in the QCOW2 more than one cluster. The implementation
>> is simple, we just split large requests into separate clusters and write
>> using existing functionality.
>>
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> ---
>>  block/qcow2.c | 73 ++++++++++++++++++++++++++++++++++++++++++-----------------
>>  1 file changed, 52 insertions(+), 21 deletions(-)
> Kevin: qcow2_alloc_compressed_cluster_offset() sets up an L2 table entry
> before doing the compressed data write.  Can the following scenario
> occur if there is another request racing with the compressed write?
>
> 1. Compressed cluster L2 table entry added to qcow2 in-memory cache
> 2. Another request forces cached L2 table to be written to disk
> 3. Power failure or crash before compressed data is written
>
> Now there is an L2 table entry pointing to garbage data.  This violates
> qcow2's data integrity model.
>
> I'm not sure if compressed writes are safe...  It may have been okay for
> qemu-img convert but the risk is increased when running a VM.
qemu-img is now multi-coroutine, thus the danger is exactly the same.

Den



>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index c276b24..f1e2759 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -3332,11 +3332,9 @@ static int qcow2_truncate(BlockDriverState *bs, int64_t offset,
>>      return 0;
>>  }
>>  
>> -/* XXX: put compressed sectors first, then all the cluster aligned
>> -   tables to avoid losing bytes in alignment */
>>  static coroutine_fn int
>> -qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
>> -                            uint64_t bytes, QEMUIOVector *qiov)
>> +qcow2_co_pwritev_cluster_compressed(BlockDriverState *bs, uint64_t offset,
>> +                                    uint64_t bytes, QEMUIOVector *qiov)
>>  {
>>      BDRVQcow2State *s = bs->opaque;
>>      QEMUIOVector hd_qiov;
>> @@ -3346,25 +3344,12 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
>>      uint8_t *buf, *out_buf;
>>      int64_t cluster_offset;
>>  
>> -    if (bytes == 0) {
>> -        /* align end of file to a sector boundary to ease reading with
>> -           sector based I/Os */
>> -        cluster_offset = bdrv_getlength(bs->file->bs);
>> -        if (cluster_offset < 0) {
>> -            return cluster_offset;
>> -        }
>> -        return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, NULL);
>> -    }
>> -
>> -    if (offset_into_cluster(s, offset)) {
>> -        return -EINVAL;
>> -    }
>> +    assert(bytes <= s->cluster_size);
>> +    assert(!offset_into_cluster(s, offset));
>>  
>>      buf = qemu_blockalign(bs, s->cluster_size);
>> -    if (bytes != s->cluster_size) {
>> -        if (bytes > s->cluster_size ||
>> -            offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS)
>> -        {
>> +    if (bytes < s->cluster_size) {
>> +        if (offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS) {
>>              qemu_vfree(buf);
>>              return -EINVAL;
>>          }
>> @@ -3444,6 +3429,52 @@ fail:
>>      return ret;
>>  }
>>  
>> +/* XXX: put compressed sectors first, then all the cluster aligned
>> +   tables to avoid losing bytes in alignment */
>> +static coroutine_fn int
>> +qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
>> +                            uint64_t bytes, QEMUIOVector *qiov)
>> +{
>> +    BDRVQcow2State *s = bs->opaque;
>> +    QEMUIOVector hd_qiov;
>> +    uint64_t curr_off = 0;
>> +    int ret;
>> +
>> +    if (bytes == 0) {
>> +        /* align end of file to a sector boundary to ease reading with
>> +           sector based I/Os */
>> +        int64_t cluster_offset = bdrv_getlength(bs->file->bs);
>> +        if (cluster_offset < 0) {
>> +            return cluster_offset;
>> +        }
>> +        return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, NULL);
>> +    }
>> +
>> +    if (offset_into_cluster(s, offset)) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    qemu_iovec_init(&hd_qiov, qiov->niov);
>> +    do {
>> +        uint32_t chunk_size;
>> +
>> +        qemu_iovec_reset(&hd_qiov);
>> +        chunk_size = MIN(bytes, s->cluster_size);
>> +        qemu_iovec_concat(&hd_qiov, qiov, curr_off, chunk_size);
>> +
>> +        ret = qcow2_co_pwritev_cluster_compressed(bs, offset + curr_off,
>> +                                                  chunk_size, &hd_qiov);
>> +        if (ret < 0) {
>> +            break;
>> +        }
>> +        curr_off += chunk_size;
>> +        bytes -= chunk_size;
>> +    } while (bytes);
>> +    qemu_iovec_destroy(&hd_qiov);
>> +
>> +    return ret;
>> +}
>> +
>>  static int make_completely_empty(BlockDriverState *bs)
>>  {
>>      BDRVQcow2State *s = bs->opaque;
>> -- 
>> 2.7.4
>>
>>

Re: [Qemu-devel] [PATCH v2 4/4] iotests: 030: add compressed block-stream test

[Anton Nefedov]

On 20/11/2017 5:38 PM, Stefan Hajnoczi wrote:
> On Thu, Nov 16, 2017 at 07:54:58PM +0300, Anton Nefedov wrote:
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/030     | 66 +++++++++++++++++++++++++++++++++++++++++++++-
>>   tests/qemu-iotests/030.out |  4 +--
>>   2 files changed, 67 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
>> index 457984b..831d6f3 100755
>> --- a/tests/qemu-iotests/030
>> +++ b/tests/qemu-iotests/030
>> @@ -21,7 +21,7 @@
>>   import time
>>   import os
>>   import iotests
>> -from iotests import qemu_img, qemu_io
>> +from iotests import qemu_img, qemu_img_pipe, qemu_io
>>   
>>   backing_img = os.path.join(iotests.test_dir, 'backing.img')
>>   mid_img = os.path.join(iotests.test_dir, 'mid.img')
>> @@ -804,5 +804,69 @@ class TestSetSpeed(iotests.QMPTestCase):
>>   
>>           self.cancel_and_wait(resume=True)
>>   
>> +class TestCompressed(iotests.QMPTestCase):
>> +    supported_fmts = ['qcow2']
>> +    cluster_size = 64 * 1024;
>> +    image_len = 1 * 1024 * 1024;
> 
> Please drop the unnecessary semicolons
> 
>> +
>> +    def setUp(self):
>> +        qemu_img('create', '-f', iotests.imgfmt, '-o', 'cluster_size=%d' % TestCompressed.cluster_size, backing_img, str(TestCompressed.image_len))
>> +        qemu_io('-c', 'write -P 1 0 ' + str(TestCompressed.image_len), backing_img)
>> +        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s,cluster_size=%d' % (backing_img, TestCompressed.cluster_size), test_img)
>> +
>> +        # write '3' in every 3rd cluster
>> +        step = 3
>> +        for off in range(0, TestCompressed.image_len, TestCompressed.cluster_size * step):
>> +            qemu_io('-c', 'write -P %d %d %d' %
>> +                    (step, off, TestCompressed.cluster_size), test_img)
>> +
>> +        self.vm = iotests.VM().add_drive(test_img)
>> +        self.vm.launch()
>> +
>> +    def tearDown(self):
>> +        os.remove(test_img)
>> +        os.remove(backing_img)
>> +
>> +    def _first_divider(self, x, divs):
> 
> "Divisor" or "factor":
> https://en.wikipedia.org/wiki/Divisor
> 
>> +        return divs[0] if not x%divs[0] else self._first_divider(x, divs[1:])
> 
> An alternative that I find easier to read than conditional expressions:
> 
>    for divisor in divs:
>        if x % divisor == 0:
>            return divisor
>    raise ValueError('No suitable divisor found')
> 
>> +
>> +    def test_compressed(self):
>> +        self.assert_no_active_block_jobs()
>> +
>> +        result = self.vm.qmp('block-stream', device='drive0', compress=True)
>> +        if iotests.imgfmt not in TestCompressed.supported_fmts:
>> +            self.assert_qmp(
>> +                result, 'error/desc',
>> +                'Compression is not supported for this drive drive0')
>> +            return
>> +        self.assert_qmp(result, 'return', {})
>> +
>> +        # write '4' in every 4th cluster
>> +        step = 4
>> +        for off in range(0, TestCompressed.image_len, TestCompressed.cluster_size * step):
>> +            result = self.vm.qmp('human-monitor-command',
>> +                                 command_line=
>> +                                 'qemu-io drive0 "write -P %d %d %d"' %
>> +                                 (step, off, TestCompressed.cluster_size))
>> +            self.assert_qmp(result, 'return', "")
>> +
>> +        self.wait_until_completed()
>> +        self.assert_no_active_block_jobs()
>> +
>> +        self.vm.shutdown()
> 
> It is safe to call self.vm.shutdown() multiple times.  Please call it
> from tearDown() too so the QEMU process is cleaned up on failure.
> 

Thank you, done to all remarks.

Re: [Qemu-devel] [PATCH v2 1/4] qcow2: multiple clusters write compressed

[Stefan Hajnoczi]

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

On Mon, Nov 20, 2017 at 06:03:00PM +0300, Denis V. Lunev wrote:
> On 11/20/2017 05:53 PM, Stefan Hajnoczi wrote:
> > On Thu, Nov 16, 2017 at 07:54:55PM +0300, Anton Nefedov wrote:
> >> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
> >>
> >> At the moment, qcow2_co_pwritev_compressed can process the requests size
> >> less than or equal to one cluster. This patch added possibility to write
> >> compressed data in the QCOW2 more than one cluster. The implementation
> >> is simple, we just split large requests into separate clusters and write
> >> using existing functionality.
> >>
> >> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> >> ---
> >>  block/qcow2.c | 73 ++++++++++++++++++++++++++++++++++++++++++-----------------
> >>  1 file changed, 52 insertions(+), 21 deletions(-)
> > Kevin: qcow2_alloc_compressed_cluster_offset() sets up an L2 table entry
> > before doing the compressed data write.  Can the following scenario
> > occur if there is another request racing with the compressed write?
> >
> > 1. Compressed cluster L2 table entry added to qcow2 in-memory cache
> > 2. Another request forces cached L2 table to be written to disk
> > 3. Power failure or crash before compressed data is written
> >
> > Now there is an L2 table entry pointing to garbage data.  This violates
> > qcow2's data integrity model.
> >
> > I'm not sure if compressed writes are safe...  It may have been okay for
> > qemu-img convert but the risk is increased when running a VM.
> qemu-img is now multi-coroutine, thus the danger is exactly the same.

Yes, the the problem is exactly the same but users delete incomplete
output files when qemu-img covert is interrupted due to power failure.
That's probably why it hasn't been a priority to fix this qcow2 code
path.

Stefan

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