[Qemu-devel] [PATCH 0/5] compressed block-stream

[Qemu-devel] [PATCH 0/5] compressed block-stream

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

Anton Nefedov (4):
  qcow2: reject unaligned offsets in write compressed
  block: support compressed write for copy-on-read
  block-stream: add compress option
  iotest 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                 | 30 +++++++++++++++----
 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     | 69 ++++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/030.out |  4 +--
 11 files changed, 186 insertions(+), 35 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/5] qcow2: reject unaligned offsets in write compressed

[Anton Nefedov]

Misaligned compressed write is not supported.

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

diff --git a/block/qcow2.c b/block/qcow2.c
index 92cb9f9..45c5651 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3349,6 +3349,10 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
         return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, NULL);
     }
 
+    if (offset_into_cluster(s, offset)) {
+        return -EINVAL;
+    }
+
     buf = qemu_blockalign(bs, s->cluster_size);
     if (bytes != s->cluster_size) {
         if (bytes > s->cluster_size ||
-- 
2.7.4

[Qemu-devel] [PATCH 2/5] 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. For unaligned requests we use a workaround
and do write data without compression.

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

diff --git a/block/qcow2.c b/block/qcow2.c
index 45c5651..3d5f17d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3325,11 +3325,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;
@@ -3339,25 +3337,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;
         }
@@ -3437,6 +3422,56 @@ 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 == -ENOTSUP) {
+            ret = qcow2_co_pwritev(bs, offset + curr_off, chunk_size,
+                                   &hd_qiov, 0);
+        }
+        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 3/5] block: support compressed write for copy-on-read

[Anton Nefedov]

Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
---
 block/io.c         | 30 ++++++++++++++++++++++++------
 block/trace-events |  2 +-
 2 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/block/io.c b/block/io.c
index 3d5ef2c..93c6b24 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;
         }
     }
@@ -1209,6 +1220,13 @@ int coroutine_fn bdrv_co_preadv(BdrvChild *child,
         return ret;
     }
 
+    /* write compressed only makes sense with copy on read */
+    if ((flags & BDRV_REQ_WRITE_COMPRESSED) &&
+        !(flags & BDRV_REQ_COPY_ON_READ))
+    {
+        return -EINVAL;
+    }
+
     bdrv_inc_in_flight(bs);
 
     /* Don't do copy-on-read if we read data before write operation */
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 4/5] block-stream: add compress option

[Anton Nefedov]

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..b0d022f 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, 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..18a56d9 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",
+                   bdrv_get_device_name(bs));
+        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 5/5] iotest 030: add compressed block-stream test

[Anton Nefedov]

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

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index 1883894..52cbe5d 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')
@@ -800,5 +800,72 @@ class TestSetSpeed(iotests.QMPTestCase):
 
         self.cancel_and_wait()
 
+class TestCompressed(iotests.QMPTestCase):
+    supported_fmts = ['qcow2']
+    cluster_size = 64 * 1024;
+    image_len = 1 * 1024 * 1024;
+
+    def setUp(self):
+        qemu_img('create', backing_img, str(TestCompressed.image_len))
+        qemu_io('-f', 'raw', '-c', 'write -P 1 0 ' + str(TestCompressed.image_len), backing_img)
+        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
+
+        # write '4' in every 4th cluster
+        step=4
+        for i in range(TestCompressed.image_len / TestCompressed.cluster_size / step + 1):
+            qemu_io('-c', 'write -P %d %d %d' %
+                    (step, step * i * TestCompressed.cluster_size,
+                     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 _pattern(self, x, divs):
+        return divs[-1] if not x%divs[-1] else self._pattern(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 '2' in every 2nd cluster
+        step = 2
+        for i in range(TestCompressed.image_len / TestCompressed.cluster_size / step + 1):
+            result = self.vm.qmp('human-monitor-command',
+                                 command_line=
+                                 'qemu-io drive0 \"write -P %d %d %d\"' %
+                                 (step, step * i * TestCompressed.cluster_size,
+                                  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._pattern(i, [1,4,2]),
+                            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 1/5 for-2.11?] qcow2: reject unaligned offsets in write compressed

[Eric Blake]

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

On 11/14/2017 04:16 AM, Anton Nefedov wrote:
> Misaligned compressed write is not supported.
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>  block/qcow2.c | 4 ++++
>  1 file changed, 4 insertions(+)

Should this one be applied in 2.11?

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 92cb9f9..45c5651 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3349,6 +3349,10 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
>          return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, NULL);
>      }
>  
> +    if (offset_into_cluster(s, offset)) {
> +        return -EINVAL;
> +    }
> +
>      buf = qemu_blockalign(bs, s->cluster_size);
>      if (bytes != s->cluster_size) {
>          if (bytes > s->cluster_size ||
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 1/5 for-2.11?] qcow2: reject unaligned offsets in write compressed

[Anton Nefedov]

On 14/11/2017 7:50 PM, Eric Blake wrote:
> On 11/14/2017 04:16 AM, Anton Nefedov wrote:
>> Misaligned compressed write is not supported.
>>
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> ---
>>   block/qcow2.c | 4 ++++
>>   1 file changed, 4 insertions(+)
> 
> Should this one be applied in 2.11?
> 

For the record, this one is pretty hard to trigger; backup and qemu-img
convert currently use compressed write, both make sure they operate in
clusters.

qemu-io is almighty though

qemu-io> write -c -P 7 512 64k
wrote 65536/65536 bytes at offset 512
64 KiB, 1 ops; 0.0187 sec (3.329 MiB/sec and 53.2566 ops/sec)
qemu-io> read -P 7 512 64k
Pattern verification failed at offset 512, 65536 bytes
read 65536/65536 bytes at offset 512
64 KiB, 1 ops; 0.0002 sec (248.016 MiB/sec and 3968.2540 ops/sec)
qemu-io> read -P 7 0 64k
read 65536/65536 bytes at offset 0
64 KiB, 1 ops; 0.0000 sec (1.606 GiB/sec and 26315.7895 ops/sec)

/Anton

Re: [Qemu-devel] [PATCH 1/5 for-2.11?] qcow2: reject unaligned offsets in write compressed

[Eric Blake]

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

On 11/14/2017 12:30 PM, Anton Nefedov wrote:
> On 14/11/2017 7:50 PM, Eric Blake wrote:
>> On 11/14/2017 04:16 AM, Anton Nefedov wrote:
>>> Misaligned compressed write is not supported.
>>>
>>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>>> ---
>>>   block/qcow2.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>
>> Should this one be applied in 2.11?
>>
> 
> For the record, this one is pretty hard to trigger; backup and qemu-img
> convert currently use compressed write, both make sure they operate in
> clusters.
> 
> qemu-io is almighty though

Okay, then we definitely have a bug, and this patch is definitely 2.11
material, especially if you update the commit message to show the
trigger case:

> 
> qemu-io> write -c -P 7 512 64k
> wrote 65536/65536 bytes at offset 512
> 64 KiB, 1 ops; 0.0187 sec (3.329 MiB/sec and 53.2566 ops/sec)
> qemu-io> read -P 7 512 64k
> Pattern verification failed at offset 512, 65536 bytes
> read 65536/65536 bytes at offset 512
> 64 KiB, 1 ops; 0.0002 sec (248.016 MiB/sec and 3968.2540 ops/sec)
> qemu-io> read -P 7 0 64k
> read 65536/65536 bytes at offset 0
> 64 KiB, 1 ops; 0.0000 sec (1.606 GiB/sec and 26315.7895 ops/sec)
> 
> /Anton
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH 2/5] qcow2: multiple clusters write compressed

[Max Reitz]

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

On 2017-11-14 11:16, 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. For unaligned requests we use a workaround
> and do write data without compression.
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>  block/qcow2.c | 77 +++++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 56 insertions(+), 21 deletions(-)

On one hand, it might be better to do this centrally somewhere in
block/io.c.  On the other, that would require more work because it would
probably mean having to introduce another field in BlockLimits, and it
wouldn't do much -- because qcow (v1) is, well, qcow v1...  And vmdk
seems to completely not care about this single cluster limitation.  So
for now we probably wouldn't even gain anything by doing this in block/io.c.

So long story short, it's OK to do this locally in qcow2, yes.

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 45c5651..3d5f17d 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -3325,11 +3325,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;
> @@ -3339,25 +3337,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;
>          }
> @@ -3437,6 +3422,56 @@ 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 == -ENOTSUP) {

Why this?  I mean, I can see the appeal, but then we should probably
actually return -ENOTSUP somewhere (e.g. for unaligned clusters and the
like) -- and we should not abort if offset_into_cluster(s, cluster) is
true, but we should write the header uncompressed and compress the main
bulk.

Max

> +            ret = qcow2_co_pwritev(bs, offset + curr_off, chunk_size,
> +                                   &hd_qiov, 0);
> +        }
> +        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;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 1/5] qcow2: reject unaligned offsets in write compressed

[Max Reitz]

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

On 2017-11-14 11:16, Anton Nefedov wrote:
> Misaligned compressed write is not supported.
> 
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>  block/qcow2.c | 4 ++++
>  1 file changed, 4 insertions(+)

Thanks, applied to my block branch for 2.11:

https://github.com/XanClic/qemu/commits/block

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 2/5] qcow2: multiple clusters write compressed

[Anton Nefedov]

On 15/11/2017 6:11 PM, Max Reitz wrote:
> On 2017-11-14 11:16, 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. For unaligned requests we use a workaround
>> and do write data without compression.
>>
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> ---
>>   block/qcow2.c | 77 +++++++++++++++++++++++++++++++++++++++++++----------------
>>   1 file changed, 56 insertions(+), 21 deletions(-)
> 
> On one hand, it might be better to do this centrally somewhere in
> block/io.c.  On the other, that would require more work because it would
> probably mean having to introduce another field in BlockLimits, and it
> wouldn't do much -- because qcow (v1) is, well, qcow v1...  And vmdk
> seems to completely not care about this single cluster limitation.  So
> for now we probably wouldn't even gain anything by doing this in block/io.c.
> 
> So long story short, it's OK to do this locally in qcow2, yes.
> 

[..]

>> +        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 == -ENOTSUP) {
> 
> Why this?  I mean, I can see the appeal, but then we should probably
> actually return -ENOTSUP somewhere (e.g. for unaligned clusters and the
> like) -- and we should not abort if offset_into_cluster(s, cluster) is
> true, but we should write the header uncompressed and compress the main
> bulk.
> 
> Max
> 

Right, sorry, missed this part when porting the patch.

I think this needs to be removed (and the commit message fixed
accordingly).
Returning an error, rather than silently falling back to uncompressed
seems preferable to me. "Compressing writers" (backup, img convert and
now stream) are aware that they have to cluster-align, and if they fail
to do so that means there is an error somewhere.
If it won't return an error anymore, then the unaligned tail shouldn't
either. So we can end up 'letting' the callers send small unaligned
requests which will never get compressed.

/Anton

>> +            ret = qcow2_co_pwritev(bs, offset + curr_off, chunk_size,
>> +                                   &hd_qiov, 0);

Re: [Qemu-devel] [PATCH 2/5] qcow2: multiple clusters write compressed

[Max Reitz]

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

On 2017-11-15 17:28, Anton Nefedov wrote:
> On 15/11/2017 6:11 PM, Max Reitz wrote:
>> On 2017-11-14 11:16, 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. For unaligned requests we use a workaround
>>> and do write data without compression.
>>>
>>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>>> ---
>>>   block/qcow2.c | 77
>>> +++++++++++++++++++++++++++++++++++++++++++----------------
>>>   1 file changed, 56 insertions(+), 21 deletions(-)
>>
>> On one hand, it might be better to do this centrally somewhere in
>> block/io.c.  On the other, that would require more work because it would
>> probably mean having to introduce another field in BlockLimits, and it
>> wouldn't do much -- because qcow (v1) is, well, qcow v1...  And vmdk
>> seems to completely not care about this single cluster limitation.  So
>> for now we probably wouldn't even gain anything by doing this in
>> block/io.c.
>>
>> So long story short, it's OK to do this locally in qcow2, yes.
>>
> 
> [..]
> 
>>> +        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 == -ENOTSUP) {
>>
>> Why this?  I mean, I can see the appeal, but then we should probably
>> actually return -ENOTSUP somewhere (e.g. for unaligned clusters and the
>> like) -- and we should not abort if offset_into_cluster(s, cluster) is
>> true, but we should write the header uncompressed and compress the main
>> bulk.
>>
>> Max
>>
> 
> Right, sorry, missed this part when porting the patch.
> 
> I think this needs to be removed (and the commit message fixed
> accordingly).
> Returning an error, rather than silently falling back to uncompressed
> seems preferable to me. "Compressing writers" (backup, img convert and
> now stream) are aware that they have to cluster-align, and if they fail
> to do so that means there is an error somewhere.

OK for me.

> If it won't return an error anymore, then the unaligned tail shouldn't
> either. So we can end up 'letting' the callers send small unaligned
> requests which will never get compressed.

Either way is fine.  It just looks to me like vmdk falls back to
uncompressed writes, so it's not like it would be completely new behavior...

(But I won't judge whether what vmdk does is a good idea.)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 3/5] block: support compressed write for copy-on-read

[Max Reitz]

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

On 2017-11-14 11:16, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>  block/io.c         | 30 ++++++++++++++++++++++++------
>  block/trace-events |  2 +-
>  2 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 3d5ef2c..93c6b24 100644
> --- a/block/io.c
> +++ b/block/io.c

[...]

> @@ -1209,6 +1220,13 @@ int coroutine_fn bdrv_co_preadv(BdrvChild *child,
>          return ret;
>      }
>  
> +    /* write compressed only makes sense with copy on read */
> +    if ((flags & BDRV_REQ_WRITE_COMPRESSED) &&
> +        !(flags & BDRV_REQ_COPY_ON_READ))
> +    {
> +        return -EINVAL;
> +    }
> +

I think the assertion in bdrv_aligned_preadv() should be enough, but
either way:

Reviewed-by: Max Reitz <mreitz@redhat.com>

>      bdrv_inc_in_flight(bs);
>  
>      /* Don't do copy-on-read if we read data before write operation */


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

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

[Max Reitz]

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

On 2017-11-14 11:16, Anton Nefedov wrote:
> 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(-)

Looks good to me overall, two notes below.

Max

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index ab96e34..b0d022f 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, if the target format supports it.
> +#            (default: false). Since 2.12.
> +#

Too many full stops (one before, one after the parentheses).  Also, this
makes it sound a bit like it would still be possible to specify this
parameter even if the target doesn't support it (and it would just be
ignored then), but that's not true.

Also, the format most parameter documentation follows for block-stream
is more "@parameter: Description. Default. (Since version)".

So I'd suggest:

"true to compress data; may only be set if the target format supports
it.  Defaults to false if omitted.  (Since 2.12)"

But I won't argue too much over the format, so the following is OK, too:

"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..18a56d9 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",
> +                   bdrv_get_device_name(bs));

I think just device instead of bdrv_get_device_name(bs) is better (or
bdrv_get_device_or_node_name(bs) at least).

> +        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,
>      },
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 5/5] iotest 030: add compressed block-stream test

[Max Reitz]

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

On 2017-11-14 11:16, Anton Nefedov wrote:
> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> ---
>  tests/qemu-iotests/030     | 69 +++++++++++++++++++++++++++++++++++++++++++++-
>  tests/qemu-iotests/030.out |  4 +--
>  2 files changed, 70 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index 1883894..52cbe5d 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')
> @@ -800,5 +800,72 @@ class TestSetSpeed(iotests.QMPTestCase):
>  
>          self.cancel_and_wait()
>  
> +class TestCompressed(iotests.QMPTestCase):
> +    supported_fmts = ['qcow2']
> +    cluster_size = 64 * 1024;
> +    image_len = 1 * 1024 * 1024;
> +
> +    def setUp(self):
> +        qemu_img('create', backing_img, str(TestCompressed.image_len))

First, this is missing the '-f raw', if you want to keep it in line with
the next command.

> +        qemu_io('-f', 'raw', '-c', 'write -P 1 0 ' + str(TestCompressed.image_len), backing_img)

But why would you want this to be raw?

> +        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
> +
> +        # write '4' in every 4th cluster
> +        step=4

I'd prefer spaces around operators.  (Also in _pattern() and in the call
to pattern ([1,4,2] should be [1, 4, 2]).)

> +        for i in range(TestCompressed.image_len / TestCompressed.cluster_size / step + 1):

As far as I know, range() has an actual step parameter, maybe that would
be simpler -- and I don't know what the +1 is supposed to mean.

How about "for ofs in range(0, TestCompressed.image_len,
                            TestCompressed.cluster_size * step)"?
Would that work?

> +            qemu_io('-c', 'write -P %d %d %d' %
> +                    (step, step * i * TestCompressed.cluster_size,> +                     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 _pattern(self, x, divs):
> +        return divs[-1] if not x%divs[-1] else self._pattern(x, divs[:-1])

I have no idea what this function does.

...OK, having looked into how it's used, I understand better.  A comment
would certainly be helpful, though.

Maybe also call it differently.  It doesn't really return a pattern.
What this function does is it returns the first integer from @divs
(starting from the end, which is weird) which is a divider of @x.  So
maybe call it _first_divider(self, x, dividers), that would help already.

> +
> +    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 '2' in every 2nd cluster
> +        step = 2
> +        for i in range(TestCompressed.image_len / TestCompressed.cluster_size / step + 1):
> +            result = self.vm.qmp('human-monitor-command',
> +                                 command_line=
> +                                 'qemu-io drive0 \"write -P %d %d %d\"' %

Just " instead of \" would be enough, I think.

> +                                 (step, step * i * TestCompressed.cluster_size,
> +                                  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._pattern(i, [1,4,2]),

The 4 is useless, because everything that's divisible by 4 is divisible
by 2 already.

Also, I don't quite see what this should achieve exactly.  You first
write the backing image full of 1s, OK.  Then you write 4s to every
fourth cluster in the top image.  Then you start streaming, and then you
write 2s to ever second cluster in the top image, which overwrites all
of the 4s you wrote.

Do you maybe not want to write 2 into every second cluster of the
backing image in setUp() instead of 4 into the top image?  (And then 4th
into every fourth cluster here in test_compressed().)  Then you would
need [1, 2, 4] here, which makes much more sense.

Max

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH 0/5] compressed block-stream

[Fam Zheng]

On Tue, 11/14 13:16, 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.

I haven't looked at the patches yet so maybe the answer is obvious, but still:
would the "compress" option be still necessary if what we have is
blockdev-stream instead?

Fam

Re: [Qemu-devel] [PATCH 0/5] compressed block-stream

[Anton Nefedov]

On 16/11/2017 6:26 AM, Fam Zheng wrote:
> On Tue, 11/14 13:16, 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.
> 
> I haven't looked at the patches yet so maybe the answer is obvious, but still:
> would the "compress" option be still necessary if what we have is
> blockdev-stream instead?
> 
> Fam
> 

Sorry, I can't see blockdev-stream available now. How that would be
different? Would it imply the blocks are pulled up to the top (active)
image?

Or did you mean block-commit? The option might be useful there as well,
and needs to be implemented separately

/Anton

Re: [Qemu-devel] [PATCH 3/5] block: support compressed write for copy-on-read

[Anton Nefedov]

On 15/11/2017 9:49 PM, Max Reitz wrote:
> On 2017-11-14 11:16, Anton Nefedov wrote:
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> ---
>>   block/io.c         | 30 ++++++++++++++++++++++++------
>>   block/trace-events |  2 +-
>>   2 files changed, 25 insertions(+), 7 deletions(-)
>>
>> diff --git a/block/io.c b/block/io.c
>> index 3d5ef2c..93c6b24 100644
>> --- a/block/io.c
>> +++ b/block/io.c
> 
> [...]
> 
>> @@ -1209,6 +1220,13 @@ int coroutine_fn bdrv_co_preadv(BdrvChild *child,
>>           return ret;
>>       }
>>   
>> +    /* write compressed only makes sense with copy on read */
>> +    if ((flags & BDRV_REQ_WRITE_COMPRESSED) &&
>> +        !(flags & BDRV_REQ_COPY_ON_READ))
>> +    {
>> +        return -EINVAL;
>> +    }
>> +
> 
> I think the assertion in bdrv_aligned_preadv() should be enough, but
> either way:
> 
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> 

Ok, and it will fail more loudly. Will remove.

>>       bdrv_inc_in_flight(bs);
>>   
>>       /* Don't do copy-on-read if we read data before write operation */
> 

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

[Anton Nefedov]

On 15/11/2017 10:16 PM, Max Reitz wrote:
> On 2017-11-14 11:16, Anton Nefedov wrote:
>> 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(-)
> 
> Looks good to me overall, two notes below.
> 
> Max
> 
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index ab96e34..b0d022f 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, if the target format supports it.
>> +#            (default: false). Since 2.12.
>> +#
> 
> Too many full stops (one before, one after the parentheses).  Also, this
> makes it sound a bit like it would still be possible to specify this
> parameter even if the target doesn't support it (and it would just be
> ignored then), but that's not true.
> 
> Also, the format most parameter documentation follows for block-stream
> is more "@parameter: Description. Default. (Since version)".
> 
> So I'd suggest:
> 
> "true to compress data; may only be set if the target format supports
> it.  Defaults to false if omitted.  (Since 2.12)"
> 
> But I won't argue too much over the format, so the following is OK, too:
> 
> "true to compress data; may only be set if the target format supports it
> (default: false). (Since 2.12)"
> 

Thanks, will fix.

[..]

>> @@ -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",
>> +                   bdrv_get_device_name(bs));
> 
> I think just device instead of bdrv_get_device_name(bs) is better (or
> bdrv_get_device_or_node_name(bs) at least).
> 

device should be enough indeed, done.

/Anton

Re: [Qemu-devel] [PATCH 5/5] iotest 030: add compressed block-stream test

[Anton Nefedov]

On 15/11/2017 10:51 PM, Max Reitz wrote:
> On 2017-11-14 11:16, Anton Nefedov wrote:
>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/030     | 69 +++++++++++++++++++++++++++++++++++++++++++++-
>>   tests/qemu-iotests/030.out |  4 +--
>>   2 files changed, 70 insertions(+), 3 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
>> index 1883894..52cbe5d 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')
>> @@ -800,5 +800,72 @@ class TestSetSpeed(iotests.QMPTestCase):
>>   
>>           self.cancel_and_wait()
>>   
>> +class TestCompressed(iotests.QMPTestCase):
>> +    supported_fmts = ['qcow2']
>> +    cluster_size = 64 * 1024;
>> +    image_len = 1 * 1024 * 1024;
>> +
>> +    def setUp(self):
>> +        qemu_img('create', backing_img, str(TestCompressed.image_len))
> 
> First, this is missing the '-f raw', if you want to keep it in line with
> the next command.
> 
>> +        qemu_io('-f', 'raw', '-c', 'write -P 1 0 ' + str(TestCompressed.image_len), backing_img)
> 
> But why would you want this to be raw?
> 

Copied this from another test in this file :)
The source image format does not really matter. Will fix though.

>> +        qemu_img('create', '-f', iotests.imgfmt, '-o', 'backing_file=%s' % backing_img, test_img)
>> +
>> +        # write '4' in every 4th cluster
>> +        step=4
> 
> I'd prefer spaces around operators.  (Also in _pattern() and in the call
> to pattern ([1,4,2] should be [1, 4, 2]).)
> 
>> +        for i in range(TestCompressed.image_len / TestCompressed.cluster_size / step + 1):
> 
> As far as I know, range() has an actual step parameter, maybe that would
> be simpler -- and I don't know what the +1 is supposed to mean.
> 
> How about "for ofs in range(0, TestCompressed.image_len,
>                              TestCompressed.cluster_size * step)"?
> Would that work?
> 

It does, thank you.

>> +            qemu_io('-c', 'write -P %d %d %d' %
>> +                    (step, step * i * TestCompressed.cluster_size,> +                     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 _pattern(self, x, divs):
>> +        return divs[-1] if not x%divs[-1] else self._pattern(x, divs[:-1])
> 
> I have no idea what this function does.
> 
> ...OK, having looked into how it's used, I understand better.  A comment
> would certainly be helpful, though.
> 
> Maybe also call it differently.  It doesn't really return a pattern.
> What this function does is it returns the first integer from @divs
> (starting from the end, which is weird) which is a divider of @x.  So
> maybe call it _first_divider(self, x, dividers), that would help already.
> 

Yeah, I really had to comment.

Exactly, it starts from the end as I meant it to accept numbers in the 
order they were written. So 'first_divider' wouldn't be right.
Not that important though, will rename and reorder the input.


>> +
>> +    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 '2' in every 2nd cluster
>> +        step = 2
>> +        for i in range(TestCompressed.image_len / TestCompressed.cluster_size / step + 1):
>> +            result = self.vm.qmp('human-monitor-command',
>> +                                 command_line=
>> +                                 'qemu-io drive0 \"write -P %d %d %d\"' %
> 
> Just " instead of \" would be enough, I think.
> 
Done.

>> +                                 (step, step * i * TestCompressed.cluster_size,
>> +                                  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._pattern(i, [1,4,2]),
> 
> The 4 is useless, because everything that's divisible by 4 is divisible
> by 2 already.
> 
> Also, I don't quite see what this should achieve exactly.  You first
> write the backing image full of 1s, OK.  Then you write 4s to every
> fourth cluster in the top image.  Then you start streaming, and then you
> write 2s to ever second cluster in the top image, which overwrites all
> of the 4s you wrote.
> 
> Do you maybe not want to write 2 into every second cluster of the
> backing image in setUp() instead of 4 into the top image?  (And then 4th
> into every fourth cluster here in test_compressed().)  Then you would
> need [1, 2, 4] here, which makes much more sense.
> 
> Max
> 

Then the top image would be empty before stream starts.
Not very good as qmp-driven writes may be late and just overwrite
clusters of the image fully copied from backing.

I meant the concurrent write touching both top and backing clusters.
2 and 4 were a bad choice though.

I think setUp() should write every 3 to the top (so we have unallocated 
cluster pairs to cover qcow2 writing several clusters compressed).
And test_compressed() write every 4 with qmp.

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

Re: [Qemu-devel] [PATCH 2/5] qcow2: multiple clusters write compressed

[Kevin Wolf]

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

Am 15.11.2017 um 17:30 hat Max Reitz geschrieben:
> On 2017-11-15 17:28, Anton Nefedov wrote:
> > On 15/11/2017 6:11 PM, Max Reitz wrote:
> >> On 2017-11-14 11:16, 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. For unaligned requests we use a workaround
> >>> and do write data without compression.
> >>>
> >>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> >>> ---
> >>>   block/qcow2.c | 77
> >>> +++++++++++++++++++++++++++++++++++++++++++----------------
> >>>   1 file changed, 56 insertions(+), 21 deletions(-)
> >>
> >> On one hand, it might be better to do this centrally somewhere in
> >> block/io.c.  On the other, that would require more work because it would
> >> probably mean having to introduce another field in BlockLimits, and it
> >> wouldn't do much -- because qcow (v1) is, well, qcow v1...  And vmdk
> >> seems to completely not care about this single cluster limitation.  So
> >> for now we probably wouldn't even gain anything by doing this in
> >> block/io.c.
> >>
> >> So long story short, it's OK to do this locally in qcow2, yes.
> >>
> > 
> > [..]
> > 
> >>> +        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 == -ENOTSUP) {
> >>
> >> Why this?  I mean, I can see the appeal, but then we should probably
> >> actually return -ENOTSUP somewhere (e.g. for unaligned clusters and the
> >> like) -- and we should not abort if offset_into_cluster(s, cluster) is
> >> true, but we should write the header uncompressed and compress the main
> >> bulk.
> >>
> >> Max
> >>
> > 
> > Right, sorry, missed this part when porting the patch.
> > 
> > I think this needs to be removed (and the commit message fixed
> > accordingly).
> > Returning an error, rather than silently falling back to uncompressed
> > seems preferable to me. "Compressing writers" (backup, img convert and
> > now stream) are aware that they have to cluster-align, and if they fail
> > to do so that means there is an error somewhere.
> 
> OK for me.
> 
> > If it won't return an error anymore, then the unaligned tail shouldn't
> > either. So we can end up 'letting' the callers send small unaligned
> > requests which will never get compressed.
> 
> Either way is fine.  It just looks to me like vmdk falls back to
> uncompressed writes, so it's not like it would be completely new behavior...
> 
> (But I won't judge whether what vmdk does is a good idea.)

Probably not.

If we let io.c know about the cluster-size alignment requirement for
compressed writes, the usual RMW code path could kick in. Wouldn't this
actually be a better solution than uncompressed writes or erroring out?

In fact, with this, we might even be very close to an option that turns
every write into a compressed write, so you get images that stay
compressed even while they are in use.

Kevin

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

Re: [Qemu-devel] [PATCH 2/5] qcow2: multiple clusters write compressed

[Anton Nefedov]

On 21/11/2017 8:42 PM, Kevin Wolf wrote:
> Am 15.11.2017 um 17:30 hat Max Reitz geschrieben:
>> On 2017-11-15 17:28, Anton Nefedov wrote:
>>> On 15/11/2017 6:11 PM, Max Reitz wrote:
>>>> On 2017-11-14 11:16, 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. For unaligned requests we use a workaround
>>>>> and do write data without compression.
>>>>>
>>>>> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
>>>>> ---
>>>>>    block/qcow2.c | 77
>>>>> +++++++++++++++++++++++++++++++++++++++++++----------------
>>>>>    1 file changed, 56 insertions(+), 21 deletions(-)
>>>>
>>>> On one hand, it might be better to do this centrally somewhere in
>>>> block/io.c.  On the other, that would require more work because it would
>>>> probably mean having to introduce another field in BlockLimits, and it
>>>> wouldn't do much -- because qcow (v1) is, well, qcow v1...  And vmdk
>>>> seems to completely not care about this single cluster limitation.  So
>>>> for now we probably wouldn't even gain anything by doing this in
>>>> block/io.c.
>>>>
>>>> So long story short, it's OK to do this locally in qcow2, yes.
>>>>
>>>
>>> [..]
>>>
>>>>> +        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 == -ENOTSUP) {
>>>>
>>>> Why this?  I mean, I can see the appeal, but then we should probably
>>>> actually return -ENOTSUP somewhere (e.g. for unaligned clusters and the
>>>> like) -- and we should not abort if offset_into_cluster(s, cluster) is
>>>> true, but we should write the header uncompressed and compress the main
>>>> bulk.
>>>>
>>>> Max
>>>>
>>>
>>> Right, sorry, missed this part when porting the patch.
>>>
>>> I think this needs to be removed (and the commit message fixed
>>> accordingly).
>>> Returning an error, rather than silently falling back to uncompressed
>>> seems preferable to me. "Compressing writers" (backup, img convert and
>>> now stream) are aware that they have to cluster-align, and if they fail
>>> to do so that means there is an error somewhere.
>>
>> OK for me.
>>
>>> If it won't return an error anymore, then the unaligned tail shouldn't
>>> either. So we can end up 'letting' the callers send small unaligned
>>> requests which will never get compressed.
>>
>> Either way is fine.  It just looks to me like vmdk falls back to
>> uncompressed writes, so it's not like it would be completely new behavior...
>>
>> (But I won't judge whether what vmdk does is a good idea.)
> 
> Probably not.
> 
> If we let io.c know about the cluster-size alignment requirement for
> compressed writes, the usual RMW code path could kick in. Wouldn't this
> actually be a better solution than uncompressed writes or erroring out?
> 
> In fact, with this, we might even be very close to an option that turns
> every write into a compressed write, so you get images that stay
> compressed even while they are in use.
> 
> Kevin
> 

That's alignment, and indeed it would be nice to have that block limit
and I think this patch does not contradict with that (now that in v2 it
doesn't fall back to uncompressed but returns EINVAL).
Unless we also want a max request limit so io.c does the request split?

/Anton

Re: [Qemu-devel] [PATCH 2/5] qcow2: multiple clusters write compressed

[Kevin Wolf]

Am 23.11.2017 um 10:04 hat Anton Nefedov geschrieben:
> 
> 
> On 21/11/2017 8:42 PM, Kevin Wolf wrote:
> > Am 15.11.2017 um 17:30 hat Max Reitz geschrieben:
> > > On 2017-11-15 17:28, Anton Nefedov wrote:
> > > > On 15/11/2017 6:11 PM, Max Reitz wrote:
> > > > > On 2017-11-14 11:16, 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. For unaligned requests we use a workaround
> > > > > > and do write data without compression.
> > > > > > 
> > > > > > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com>
> > > > > > ---
> > > > > >    block/qcow2.c | 77
> > > > > > +++++++++++++++++++++++++++++++++++++++++++----------------
> > > > > >    1 file changed, 56 insertions(+), 21 deletions(-)
> > > > > 
> > > > > On one hand, it might be better to do this centrally somewhere in
> > > > > block/io.c.  On the other, that would require more work because it would
> > > > > probably mean having to introduce another field in BlockLimits, and it
> > > > > wouldn't do much -- because qcow (v1) is, well, qcow v1...  And vmdk
> > > > > seems to completely not care about this single cluster limitation.  So
> > > > > for now we probably wouldn't even gain anything by doing this in
> > > > > block/io.c.
> > > > > 
> > > > > So long story short, it's OK to do this locally in qcow2, yes.
> > > > > 
> > > > 
> > > > [..]
> > > > 
> > > > > > +        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 == -ENOTSUP) {
> > > > > 
> > > > > Why this?  I mean, I can see the appeal, but then we should probably
> > > > > actually return -ENOTSUP somewhere (e.g. for unaligned clusters and the
> > > > > like) -- and we should not abort if offset_into_cluster(s, cluster) is
> > > > > true, but we should write the header uncompressed and compress the main
> > > > > bulk.
> > > > > 
> > > > > Max
> > > > > 
> > > > 
> > > > Right, sorry, missed this part when porting the patch.
> > > > 
> > > > I think this needs to be removed (and the commit message fixed
> > > > accordingly).
> > > > Returning an error, rather than silently falling back to uncompressed
> > > > seems preferable to me. "Compressing writers" (backup, img convert and
> > > > now stream) are aware that they have to cluster-align, and if they fail
> > > > to do so that means there is an error somewhere.
> > > 
> > > OK for me.
> > > 
> > > > If it won't return an error anymore, then the unaligned tail shouldn't
> > > > either. So we can end up 'letting' the callers send small unaligned
> > > > requests which will never get compressed.
> > > 
> > > Either way is fine.  It just looks to me like vmdk falls back to
> > > uncompressed writes, so it's not like it would be completely new behavior...
> > > 
> > > (But I won't judge whether what vmdk does is a good idea.)
> > 
> > Probably not.
> > 
> > If we let io.c know about the cluster-size alignment requirement for
> > compressed writes, the usual RMW code path could kick in. Wouldn't this
> > actually be a better solution than uncompressed writes or erroring out?
> > 
> > In fact, with this, we might even be very close to an option that turns
> > every write into a compressed write, so you get images that stay
> > compressed even while they are in use.
> > 
> > Kevin
> 
> That's alignment, and indeed it would be nice to have that block limit
> and I think this patch does not contradict with that (now that in v2 it
> doesn't fall back to uncompressed but returns EINVAL).

Yes, I agree.

> Unless we also want a max request limit so io.c does the request split?

I'm not sure about this one. We might want to change the qcow2 code
later so that we can actually write multiple compressed clusters at once
as an performance optimisation, and then we would give up the splitting
in io.c again. So maybe it's not really worth it.

Kevin