[PATCH v4 0/4] qcow2: advanced compression options

[PATCH v4 0/4] qcow2: advanced compression options

[Andrey Shinkevich]

New enhancements for writing compressed data to QCOW2 image.

v4:
    The 'compression' support at the block generic layer has been
    accumulated in the separate patch 1/4. A little code refactoring
    was made.
v3:
    Instead of introducing multiple key options for many drivers, the
    'compression' option has been introduced at the block generic layer
    as suggested by Roman Kagan. Discussed on the email thread with ID
    <1570026166-748566-1-git-send-email-andrey.shinkevich@virtuozzo.com>

Andrey Shinkevich (4):
  block: support compressed write at generic layer
  qcow2: Allow writing compressed data of multiple clusters
  tests/qemu-iotests: add case to write compressed data of multiple
    clusters
  tests/qemu-iotests: add case for block-stream compress

 block.c                    |  20 ++++++++-
 block/io.c                 |  14 ++++--
 block/qcow2.c              | 106 +++++++++++++++++++++++++++++++++------------
 blockdev.c                 |   9 +++-
 include/block/block.h      |   1 +
 include/block/block_int.h  |   2 +
 qapi/block-core.json       |   6 ++-
 qemu-options.hx            |   6 ++-
 tests/qemu-iotests/030     |  51 +++++++++++++++++++++-
 tests/qemu-iotests/030.out |   4 +-
 tests/qemu-iotests/214     |  35 +++++++++++++++
 tests/qemu-iotests/214.out |  15 +++++++
 12 files changed, 230 insertions(+), 39 deletions(-)

-- 
1.8.3.1

[PATCH v4 1/4] block: support compressed write at generic layer

[Andrey Shinkevich]

To inform the block layer about writing all the data compressed, we
introduce the 'compress' command line option. Based on that option, the
written data will be aligned by the cluster size at the generic layer.

Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 block.c                   | 20 +++++++++++++++++++-
 block/io.c                | 14 ++++++++++----
 block/qcow2.c             |  4 ++++
 blockdev.c                |  9 ++++++++-
 include/block/block.h     |  1 +
 include/block/block_int.h |  2 ++
 qapi/block-core.json      |  6 +++++-
 qemu-options.hx           |  6 ++++--
 8 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/block.c b/block.c
index 1946fc6..a674920 100644
--- a/block.c
+++ b/block.c
@@ -1418,6 +1418,11 @@ QemuOptsList bdrv_runtime_opts = {
             .type = QEMU_OPT_BOOL,
             .help = "always accept other writers (default: off)",
         },
+        {
+            .name = BDRV_OPT_COMPRESS,
+            .type = QEMU_OPT_BOOL,
+            .help = "compress all writes to the image (default: off)",
+        },
         { /* end of list */ }
     },
 };
@@ -1545,6 +1550,14 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
     }
     pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), bs->filename);
 
+    if (bs->all_write_compressed && !drv->bdrv_co_pwritev_compressed_part) {
+        error_setg(errp, "Compression is not supported for the driver '%s'",
+                   drv->format_name);
+        bs->all_write_compressed = false;
+        ret = -ENOTSUP;
+        goto fail_opts;
+    }
+
     /* Open the image, either directly or using a protocol */
     open_flags = bdrv_open_flags(bs, bs->open_flags);
     node_name = qemu_opt_get(opts, "node-name");
@@ -2983,6 +2996,11 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
         flags &= ~BDRV_O_RDWR;
     }
 
+    if (!g_strcmp0(qdict_get_try_str(options, BDRV_OPT_COMPRESS), "on") ||
+        qdict_get_try_bool(options, BDRV_OPT_COMPRESS, false)) {
+        bs->all_write_compressed = true;
+    }
+
     if (flags & BDRV_O_SNAPSHOT) {
         snapshot_options = qdict_new();
         bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
@@ -3208,7 +3226,7 @@ static int bdrv_reset_options_allowed(BlockDriverState *bs,
      * in bdrv_reopen_prepare() so they can be left out of @new_opts */
     const char *const common_options[] = {
         "node-name", "discard", "cache.direct", "cache.no-flush",
-        "read-only", "auto-read-only", "detect-zeroes", NULL
+        "read-only", "auto-read-only", "detect-zeroes", "compress", NULL
     };
 
     for (e = qdict_first(bs->options); e; e = qdict_next(bs->options, e)) {
diff --git a/block/io.c b/block/io.c
index f0b86c1..3743a13 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1360,9 +1360,15 @@ 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,
-                                          BDRV_REQ_WRITE_UNCHANGED);
+                if (bs->all_write_compressed) {
+                    ret = bdrv_driver_pwritev_compressed(bs, cluster_offset,
+                                                         pnum, &local_qiov,
+                                                         qiov_offset);
+                } else {
+                    ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
+                                              &local_qiov, 0,
+                                              BDRV_REQ_WRITE_UNCHANGED);
+                }
             }
 
             if (ret < 0) {
@@ -1954,7 +1960,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
     } else if (flags & BDRV_REQ_ZERO_WRITE) {
         bdrv_debug_event(bs, BLKDBG_PWRITEV_ZERO);
         ret = bdrv_co_do_pwrite_zeroes(bs, offset, bytes, flags);
-    } else if (flags & BDRV_REQ_WRITE_COMPRESSED) {
+    } else if (flags & BDRV_REQ_WRITE_COMPRESSED || bs->all_write_compressed) {
         ret = bdrv_driver_pwritev_compressed(bs, offset, bytes,
                                              qiov, qiov_offset);
     } else if (bytes <= max_transfer) {
diff --git a/block/qcow2.c b/block/qcow2.c
index 7961c05..6b29e16 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1787,6 +1787,10 @@ static void qcow2_refresh_limits(BlockDriverState *bs, Error **errp)
         /* Encryption works on a sector granularity */
         bs->bl.request_alignment = qcrypto_block_get_sector_size(s->crypto);
     }
+    if (bs->all_write_compressed) {
+        bs->bl.request_alignment = MAX(bs->bl.request_alignment,
+                                       s->cluster_size);
+    }
     bs->bl.pwrite_zeroes_alignment = s->cluster_size;
     bs->bl.pdiscard_alignment = s->cluster_size;
 }
diff --git a/blockdev.c b/blockdev.c
index f89e48f..0c0b398 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -471,7 +471,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
     int bdrv_flags = 0;
     int on_read_error, on_write_error;
     bool account_invalid, account_failed;
-    bool writethrough, read_only;
+    bool writethrough, read_only, compress;
     BlockBackend *blk;
     BlockDriverState *bs;
     ThrottleConfig cfg;
@@ -570,6 +570,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
     }
 
     read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false);
+    compress = qemu_opt_get_bool(opts, BDRV_OPT_COMPRESS, false);
 
     /* init */
     if ((!file || !*file) && !qdict_size(bs_opts)) {
@@ -595,6 +596,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
         qdict_set_default_str(bs_opts, BDRV_OPT_READ_ONLY,
                               read_only ? "on" : "off");
         qdict_set_default_str(bs_opts, BDRV_OPT_AUTO_READ_ONLY, "on");
+        qdict_set_default_str(bs_opts, BDRV_OPT_COMPRESS,
+                              compress ? "on" : "off");
         assert((bdrv_flags & BDRV_O_CACHE_MASK) == 0);
 
         if (runstate_check(RUN_STATE_INMIGRATE)) {
@@ -4683,6 +4686,10 @@ QemuOptsList qemu_common_drive_opts = {
             .name = BDRV_OPT_READ_ONLY,
             .type = QEMU_OPT_BOOL,
             .help = "open drive file as read-only",
+        },{
+            .name = BDRV_OPT_COMPRESS,
+            .type = QEMU_OPT_BOOL,
+            .help = "compress all writes to image",
         },
 
         THROTTLE_OPTS,
diff --git a/include/block/block.h b/include/block/block.h
index 792bb82..7e0a927 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -139,6 +139,7 @@ typedef struct HDGeometry {
 #define BDRV_OPT_AUTO_READ_ONLY "auto-read-only"
 #define BDRV_OPT_DISCARD        "discard"
 #define BDRV_OPT_FORCE_SHARE    "force-share"
+#define BDRV_OPT_COMPRESS       "compress"
 
 
 #define BDRV_SECTOR_BITS   9
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 05056b3..3fe8923 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -923,6 +923,8 @@ struct BlockDriverState {
 
     /* BdrvChild links to this node may never be frozen */
     bool never_freeze;
+    /* Compress all writes to the image */
+    bool all_write_compressed;
 };
 
 struct BlockBackendRootState {
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f66553a..6c1684f 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4014,6 +4014,9 @@
 # @force-share:   force share all permission on added nodes.
 #                 Requires read-only=true. (Since 2.10)
 #
+# @compress:      compress all writes to the image (Since 4.2)
+#                 (default: false)
+#
 # Remaining options are determined by the block driver.
 #
 # Since: 2.9
@@ -4026,7 +4029,8 @@
             '*read-only': 'bool',
             '*auto-read-only': 'bool',
             '*force-share': 'bool',
-            '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
+            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
+            '*compress': 'bool' },
   'discriminator': 'driver',
   'data': {
       'blkdebug':   'BlockdevOptionsBlkdebug',
diff --git a/qemu-options.hx b/qemu-options.hx
index 793d70f..1bfbf1a 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -850,7 +850,7 @@ DEF("blockdev", HAS_ARG, QEMU_OPTION_blockdev,
     "-blockdev [driver=]driver[,node-name=N][,discard=ignore|unmap]\n"
     "          [,cache.direct=on|off][,cache.no-flush=on|off]\n"
     "          [,read-only=on|off][,detect-zeroes=on|off|unmap]\n"
-    "          [,driver specific parameters...]\n"
+    "          [,compress=on|off][,driver specific parameters...]\n"
     "                configure a block backend\n", QEMU_ARCH_ALL)
 STEXI
 @item -blockdev @var{option}[,@var{option}[,@var{option}[,...]]]
@@ -905,6 +905,8 @@ discard requests.
 conversion of plain zero writes by the OS to driver specific optimized
 zero write commands. You may even choose "unmap" if @var{discard} is set
 to "unmap" to allow a zero write to be converted to an @code{unmap} operation.
+@item compress
+Compress all writes to the image.
 @end table
 
 @item Driver-specific options for @code{file}
@@ -1026,7 +1028,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
     "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
     "       [,snapshot=on|off][,rerror=ignore|stop|report]\n"
     "       [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n"
-    "       [,readonly=on|off][,copy-on-read=on|off]\n"
+    "       [,readonly=on|off][,copy-on-read=on|off][,compress=on|off]\n"
     "       [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
     "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n"
     "       [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n"
-- 
1.8.3.1

[PATCH v4 2/4] qcow2: Allow writing compressed data of multiple clusters

[Andrey Shinkevich]

QEMU currently supports writing compressed data of the size equal to
one cluster. This patch allows writing QCOW2 compressed data that
exceed one cluster. Now, we split buffered data into separate clusters
and write them compressed using the existing functionality.

Suggested-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 block/qcow2.c | 102 ++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 75 insertions(+), 27 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 6b29e16..9a85d73 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4156,10 +4156,8 @@ 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_part(BlockDriverState *bs,
+qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
                                  uint64_t offset, uint64_t bytes,
                                  QEMUIOVector *qiov, size_t qiov_offset)
 {
@@ -4169,32 +4167,11 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
     uint8_t *buf, *out_buf;
     uint64_t cluster_offset;
 
-    if (has_data_file(bs)) {
-        return -ENOTSUP;
-    }
-
-    if (bytes == 0) {
-        /* align end of file to a sector boundary to ease reading with
-           sector based I/Os */
-        int64_t len = bdrv_getlength(bs->file->bs);
-        if (len < 0) {
-            return len;
-        }
-        return bdrv_co_truncate(bs->file, len, PREALLOC_MODE_OFF, NULL);
-    }
-
-    if (offset_into_cluster(s, offset)) {
-        return -EINVAL;
-    }
+    assert(bytes == s->cluster_size || (bytes < s->cluster_size &&
+           (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS)));
 
     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)
-        {
-            qemu_vfree(buf);
-            return -EINVAL;
-        }
+    if (bytes < s->cluster_size) {
         /* Zero-pad last write if image size is not cluster aligned */
         memset(buf + bytes, 0, s->cluster_size - bytes);
     }
@@ -4243,6 +4220,77 @@ fail:
     return ret;
 }
 
+static coroutine_fn int qcow2_co_pwritev_compressed_task_entry(AioTask *task)
+{
+    Qcow2AioTask *t = container_of(task, Qcow2AioTask, task);
+
+    assert(!t->cluster_type && !t->l2meta);
+
+    return qcow2_co_pwritev_compressed_task(t->bs, t->offset, t->bytes, t->qiov,
+                                            t->qiov_offset);
+}
+
+/*
+ * 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_part(BlockDriverState *bs,
+                                 uint64_t offset, uint64_t bytes,
+                                 QEMUIOVector *qiov, size_t qiov_offset)
+{
+    BDRVQcow2State *s = bs->opaque;
+    AioTaskPool *aio = NULL;
+    int ret;
+
+    if (has_data_file(bs)) {
+        return -ENOTSUP;
+    }
+
+    if (bytes == 0) {
+        /*
+         * align end of file to a sector boundary to ease reading with
+         * sector based I/Os
+         */
+        int64_t len = bdrv_getlength(bs->file->bs);
+        if (len < 0) {
+            return len;
+        }
+        return bdrv_co_truncate(bs->file, len, PREALLOC_MODE_OFF, NULL);
+    }
+
+    if (offset_into_cluster(s, offset)) {
+        return -EINVAL;
+    }
+
+    while (bytes && aio_task_pool_status(aio) == 0) {
+        uint32_t chunk_size = MIN(bytes, s->cluster_size);
+
+        if (!aio && chunk_size != bytes) {
+            aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
+        }
+
+        ret = qcow2_add_task(bs, aio, qcow2_co_pwritev_compressed_task_entry,
+                             0, 0, offset, chunk_size, qiov, qiov_offset, NULL);
+        if (ret < 0) {
+            break;
+        }
+        qiov_offset += chunk_size;
+        offset += chunk_size;
+        bytes -= chunk_size;
+    }
+
+    if (aio) {
+        aio_task_pool_wait_all(aio);
+        if (ret == 0) {
+            ret = aio_task_pool_status(aio);
+        }
+        g_free(aio);
+    }
+
+    return ret;
+}
+
 static int coroutine_fn
 qcow2_co_preadv_compressed(BlockDriverState *bs,
                            uint64_t file_cluster_offset,
-- 
1.8.3.1

[PATCH v4 3/4] tests/qemu-iotests: add case to write compressed data of multiple clusters

[Andrey Shinkevich]

Add the test case to the iotest #214 that checks possibility of writing
compressed data of more than one cluster size.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/214     | 35 +++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/214.out | 15 +++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/tests/qemu-iotests/214 b/tests/qemu-iotests/214
index 21ec8a2..0003dc2 100755
--- a/tests/qemu-iotests/214
+++ b/tests/qemu-iotests/214
@@ -89,6 +89,41 @@ _check_test_img -r all
 $QEMU_IO -c "read  -P 0x11  0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
 $QEMU_IO -c "read  -P 0x22 4M 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
 
+echo
+echo "=== Write compressed data of multiple clusters ==="
+echo
+cluster_size=0x10000
+_make_test_img 2M -o cluster_size=$cluster_size
+
+echo "Uncompressed data:"
+let data_size="8 * $cluster_size"
+$QEMU_IO -c "write -P 0xaa 0 $data_size" "$TEST_IMG" \
+         2>&1 | _filter_qemu_io | _filter_testdir
+$QEMU_IMG info "$TEST_IMG" | sed -n '/disk size:/ s/^ *//p'
+
+_make_test_img 2M -o cluster_size=$cluster_size
+let data_size="3 * $cluster_size + ($cluster_size >> 1)"
+# Set compress=on. That will align the written data
+# by the cluster size and will write them compressed.
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT \
+$QEMU_IO -c "write -P 0xbb 0 $data_size" --image-opts \
+         driver=$IMGFMT,compress=on,file.filename=$TEST_IMG \
+         2>&1 | _filter_qemu_io | _filter_testdir
+
+let offset="4 * $cluster_size"
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT \
+$QEMU_IO -c "write -P 0xcc $offset $data_size" "json:{\
+    'driver': '$IMGFMT',
+    'file': {
+        'driver': 'file',
+        'filename': '$TEST_IMG'
+    },
+    'compress': true
+}" | _filter_qemu_io | _filter_testdir
+
+echo "After the multiple cluster data have been written compressed,"
+$QEMU_IMG info "$TEST_IMG" | sed -n '/disk size:/ s/^ *//p'
+
 # success, all done
 echo '*** done'
 rm -f $seq.full
diff --git a/tests/qemu-iotests/214.out b/tests/qemu-iotests/214.out
index 0fcd8dc..09a2e9a 100644
--- a/tests/qemu-iotests/214.out
+++ b/tests/qemu-iotests/214.out
@@ -32,4 +32,19 @@ read 4194304/4194304 bytes at offset 0
 4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 4194304/4194304 bytes at offset 4194304
 4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Write compressed data of multiple clusters ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2097152
+Uncompressed data:
+wrote 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+disk size: 772 KiB
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2097152
+wrote 229376/229376 bytes at offset 0
+224 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 229376/229376 bytes at offset 262144
+224 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+After the multiple cluster data have been written compressed,
+disk size: 268 KiB
 *** done
-- 
1.8.3.1

[PATCH v4 4/4] tests/qemu-iotests: add case for block-stream compress

[Andrey Shinkevich]

Add a case to the iotest #030 that tests the 'compress' option for a
block-stream job.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
---
 tests/qemu-iotests/030     | 51 +++++++++++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/030.out |  4 ++--
 2 files changed, 52 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index f3766f2..f0f0e26 100755
--- a/tests/qemu-iotests/030
+++ b/tests/qemu-iotests/030
@@ -21,7 +21,8 @@
 import time
 import os
 import iotests
-from iotests import qemu_img, qemu_io
+from iotests import qemu_img, qemu_io, qemu_img_pipe
+import json
 
 backing_img = os.path.join(iotests.test_dir, 'backing.img')
 mid_img = os.path.join(iotests.test_dir, 'mid.img')
@@ -956,6 +957,54 @@ class TestSetSpeed(iotests.QMPTestCase):
 
         self.cancel_and_wait(resume=True)
 
+class TestCompressed(iotests.QMPTestCase):
+    test_img_init_size = 0
+
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, backing_img, '1M')
+        qemu_img('create', '-f', iotests.imgfmt, '-o',
+                 'backing_file=%s' % backing_img, mid_img)
+        qemu_img('create', '-f', iotests.imgfmt, '-o',
+                 'backing_file=%s' % mid_img, test_img)
+        qemu_io('-c', 'write -P 0x1 0 512k', backing_img)
+        top = json.loads(qemu_img_pipe('info', '--output=json', test_img))
+        self.test_img_init_size = top['actual-size']
+        self.vm = iotests.VM().add_drive(test_img, "backing.node-name=mid," +
+                                         "backing.backing.node-name=base," +
+                                         "compress=on")
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(test_img)
+        os.remove(mid_img)
+        os.remove(backing_img)
+
+    def test_stream_compress(self):
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp('block-stream', device='mid', job_id='stream-mid')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_until_completed(drive='stream-mid')
+        # Remove other 'JOB_STATUS_CHANGE' events for the job 'stream-mid'
+        self.vm.get_qmp_events(wait=True)
+
+        result = self.vm.qmp('block-stream', device='drive0',
+                             job_id='stream-top')
+        self.assert_qmp(result, 'return', {})
+
+        self.wait_until_completed(drive='stream-top')
+        self.vm.shutdown()
+
+        top = json.loads(qemu_img_pipe('info', '--output=json', test_img))
+        mid = json.loads(qemu_img_pipe('info', '--output=json', mid_img))
+        base = json.loads(qemu_img_pipe('info', '--output=json', backing_img))
+
+        self.assertEqual(mid['actual-size'], base['actual-size'])
+        self.assertLess(top['actual-size'], mid['actual-size'])
+        self.assertLess(self.test_img_init_size, top['actual-size'])
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2', 'qed'],
                  supported_protocols=['file'])
diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
index 6d9bee1..af8dac1 100644
--- a/tests/qemu-iotests/030.out
+++ b/tests/qemu-iotests/030.out
@@ -1,5 +1,5 @@
-...........................
+............................
 ----------------------------------------------------------------------
-Ran 27 tests
+Ran 28 tests
 
 OK
-- 
1.8.3.1

Re: [PATCH v4 0/4] qcow2: advanced compression options

[no-reply]

Patchew URL: https://patchew.org/QEMU/1571243333-882302-1-git-send-email-andrey.shinkevich@virtuozzo.com/



Hi,

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

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      block/qed-table.o
  CC      block/qed-cluster.o
/tmp/qemu-test/src/block/qcow2.c: In function 'qcow2_co_pwritev_compressed_part':
/tmp/qemu-test/src/block/qcow2.c:4244:9: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     int ret;
         ^
cc1: all warnings being treated as errors
make: *** [block/qcow2.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 662, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=b109abacd8054efc992d66fa28ca7d8c', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-ge9ikvez/src/docker-src.2019-10-16-15.15.08.9832:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=b109abacd8054efc992d66fa28ca7d8c
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-ge9ikvez/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    2m4.543s
user    0m8.161s


The full log is available at
http://patchew.org/logs/1571243333-882302-1-git-send-email-andrey.shinkevich@virtuozzo.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v4 0/4] qcow2: advanced compression options

[no-reply]

Patchew URL: https://patchew.org/QEMU/1571243333-882302-1-git-send-email-andrey.shinkevich@virtuozzo.com/



Hi,

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

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      block/blkverify.o
  CC      block/blkreplay.o
/tmp/qemu-test/src/block/qcow2.c: In function 'qcow2_co_pwritev_compressed_part':
/tmp/qemu-test/src/block/qcow2.c:4244:9: error: 'ret' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     int ret;
         ^~~
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: block/qcow2.o] Error 1
make: *** Waiting for unfinished jobs....
  CC      block/parallels.o
Traceback (most recent call last):
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=2e359c4b7b0f403ca14825a9c4b067a8', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-hjz8elmo/src/docker-src.2019-10-16-15.17.37.16307:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=2e359c4b7b0f403ca14825a9c4b067a8
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-hjz8elmo/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    2m12.901s
user    0m7.708s


The full log is available at
http://patchew.org/logs/1571243333-882302-1-git-send-email-andrey.shinkevich@virtuozzo.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v4 1/4] block: support compressed write at generic layer

[Vladimir Sementsov-Ogievskiy]

16.10.2019 19:28, Andrey Shinkevich wrote:
> To inform the block layer about writing all the data compressed, we
> introduce the 'compress' command line option. Based on that option, the
> written data will be aligned by the cluster size at the generic layer.
> 
> Suggested-by: Roman Kagan <rkagan@virtuozzo.com>
> Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block.c                   | 20 +++++++++++++++++++-
>   block/io.c                | 14 ++++++++++----
>   block/qcow2.c             |  4 ++++
>   blockdev.c                |  9 ++++++++-
>   include/block/block.h     |  1 +
>   include/block/block_int.h |  2 ++
>   qapi/block-core.json      |  6 +++++-
>   qemu-options.hx           |  6 ++++--
>   8 files changed, 53 insertions(+), 9 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 1946fc6..a674920 100644
> --- a/block.c
> +++ b/block.c
> @@ -1418,6 +1418,11 @@ QemuOptsList bdrv_runtime_opts = {
>               .type = QEMU_OPT_BOOL,
>               .help = "always accept other writers (default: off)",
>           },
> +        {
> +            .name = BDRV_OPT_COMPRESS,
> +            .type = QEMU_OPT_BOOL,
> +            .help = "compress all writes to the image (default: off)",
> +        },
>           { /* end of list */ }
>       },
>   };
> @@ -1545,6 +1550,14 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
>       }
>       pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), bs->filename);
>   
> +    if (bs->all_write_compressed && !drv->bdrv_co_pwritev_compressed_part) {
> +        error_setg(errp, "Compression is not supported for the driver '%s'",
> +                   drv->format_name);
> +        bs->all_write_compressed = false;
> +        ret = -ENOTSUP;
> +        goto fail_opts;
> +    }
> +
>       /* Open the image, either directly or using a protocol */
>       open_flags = bdrv_open_flags(bs, bs->open_flags);
>       node_name = qemu_opt_get(opts, "node-name");
> @@ -2983,6 +2996,11 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
>           flags &= ~BDRV_O_RDWR;
>       }
>   
> +    if (!g_strcmp0(qdict_get_try_str(options, BDRV_OPT_COMPRESS), "on") ||
> +        qdict_get_try_bool(options, BDRV_OPT_COMPRESS, false)) {
> +        bs->all_write_compressed = true;
> +    }
> +
>       if (flags & BDRV_O_SNAPSHOT) {
>           snapshot_options = qdict_new();
>           bdrv_temp_snapshot_options(&snapshot_flags, snapshot_options,
> @@ -3208,7 +3226,7 @@ static int bdrv_reset_options_allowed(BlockDriverState *bs,
>        * in bdrv_reopen_prepare() so they can be left out of @new_opts */
>       const char *const common_options[] = {
>           "node-name", "discard", "cache.direct", "cache.no-flush",
> -        "read-only", "auto-read-only", "detect-zeroes", NULL
> +        "read-only", "auto-read-only", "detect-zeroes", "compress", NULL
>       };
>   
>       for (e = qdict_first(bs->options); e; e = qdict_next(bs->options, e)) {
> diff --git a/block/io.c b/block/io.c
> index f0b86c1..3743a13 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1360,9 +1360,15 @@ 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,
> -                                          BDRV_REQ_WRITE_UNCHANGED);
> +                if (bs->all_write_compressed) {
> +                    ret = bdrv_driver_pwritev_compressed(bs, cluster_offset,
> +                                                         pnum, &local_qiov,
> +                                                         qiov_offset);

last argument must be 0, we are using local_qiov, so, it's qiov_offset is 0.

> +                } else {
> +                    ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
> +                                              &local_qiov, 0,
> +                                              BDRV_REQ_WRITE_UNCHANGED);
> +                }
>               }
>   
>               if (ret < 0) {
> @@ -1954,7 +1960,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BdrvChild *child,
>       } else if (flags & BDRV_REQ_ZERO_WRITE) {
>           bdrv_debug_event(bs, BLKDBG_PWRITEV_ZERO);
>           ret = bdrv_co_do_pwrite_zeroes(bs, offset, bytes, flags);
> -    } else if (flags & BDRV_REQ_WRITE_COMPRESSED) {
> +    } else if (flags & BDRV_REQ_WRITE_COMPRESSED || bs->all_write_compressed) {
>           ret = bdrv_driver_pwritev_compressed(bs, offset, bytes,
>                                                qiov, qiov_offset);
>       } else if (bytes <= max_transfer) {
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7961c05..6b29e16 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1787,6 +1787,10 @@ static void qcow2_refresh_limits(BlockDriverState *bs, Error **errp)
>           /* Encryption works on a sector granularity */
>           bs->bl.request_alignment = qcrypto_block_get_sector_size(s->crypto);
>       }
> +    if (bs->all_write_compressed) {
> +        bs->bl.request_alignment = MAX(bs->bl.request_alignment,
> +                                       s->cluster_size);
> +    }
>       bs->bl.pwrite_zeroes_alignment = s->cluster_size;
>       bs->bl.pdiscard_alignment = s->cluster_size;
>   }
> diff --git a/blockdev.c b/blockdev.c
> index f89e48f..0c0b398 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -471,7 +471,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
>       int bdrv_flags = 0;
>       int on_read_error, on_write_error;
>       bool account_invalid, account_failed;
> -    bool writethrough, read_only;
> +    bool writethrough, read_only, compress;
>       BlockBackend *blk;
>       BlockDriverState *bs;
>       ThrottleConfig cfg;
> @@ -570,6 +570,7 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
>       }
>   
>       read_only = qemu_opt_get_bool(opts, BDRV_OPT_READ_ONLY, false);
> +    compress = qemu_opt_get_bool(opts, BDRV_OPT_COMPRESS, false);
>   
>       /* init */
>       if ((!file || !*file) && !qdict_size(bs_opts)) {

Do we need compress for root state here??

> @@ -595,6 +596,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
>           qdict_set_default_str(bs_opts, BDRV_OPT_READ_ONLY,
>                                 read_only ? "on" : "off");
>           qdict_set_default_str(bs_opts, BDRV_OPT_AUTO_READ_ONLY, "on");
> +        qdict_set_default_str(bs_opts, BDRV_OPT_COMPRESS,
> +                              compress ? "on" : "off");
>           assert((bdrv_flags & BDRV_O_CACHE_MASK) == 0);
>   
>           if (runstate_check(RUN_STATE_INMIGRATE)) {
> @@ -4683,6 +4686,10 @@ QemuOptsList qemu_common_drive_opts = {
>               .name = BDRV_OPT_READ_ONLY,
>               .type = QEMU_OPT_BOOL,
>               .help = "open drive file as read-only",
> +        },{
> +            .name = BDRV_OPT_COMPRESS,
> +            .type = QEMU_OPT_BOOL,
> +            .help = "compress all writes to image",
>           },
>   
>           THROTTLE_OPTS,
> diff --git a/include/block/block.h b/include/block/block.h
> index 792bb82..7e0a927 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -139,6 +139,7 @@ typedef struct HDGeometry {
>   #define BDRV_OPT_AUTO_READ_ONLY "auto-read-only"
>   #define BDRV_OPT_DISCARD        "discard"
>   #define BDRV_OPT_FORCE_SHARE    "force-share"
> +#define BDRV_OPT_COMPRESS       "compress"
>   
>   
>   #define BDRV_SECTOR_BITS   9
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 05056b3..3fe8923 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -923,6 +923,8 @@ struct BlockDriverState {
>   
>       /* BdrvChild links to this node may never be frozen */
>       bool never_freeze;
> +    /* Compress all writes to the image */
> +    bool all_write_compressed;
>   };
>   
>   struct BlockBackendRootState {
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index f66553a..6c1684f 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4014,6 +4014,9 @@
>   # @force-share:   force share all permission on added nodes.
>   #                 Requires read-only=true. (Since 2.10)
>   #

No newlines between parameters above, I think don't add one.

> +# @compress:      compress all writes to the image (Since 4.2)
> +#                 (default: false)
> +#
>   # Remaining options are determined by the block driver.
>   #
>   # Since: 2.9
> @@ -4026,7 +4029,8 @@
>               '*read-only': 'bool',
>               '*auto-read-only': 'bool',
>               '*force-share': 'bool',
> -            '*detect-zeroes': 'BlockdevDetectZeroesOptions' },
> +            '*detect-zeroes': 'BlockdevDetectZeroesOptions',
> +            '*compress': 'bool' },
>     'discriminator': 'driver',
>     'data': {
>         'blkdebug':   'BlockdevOptionsBlkdebug',
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 793d70f..1bfbf1a 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -850,7 +850,7 @@ DEF("blockdev", HAS_ARG, QEMU_OPTION_blockdev,
>       "-blockdev [driver=]driver[,node-name=N][,discard=ignore|unmap]\n"
>       "          [,cache.direct=on|off][,cache.no-flush=on|off]\n"
>       "          [,read-only=on|off][,detect-zeroes=on|off|unmap]\n"
> -    "          [,driver specific parameters...]\n"
> +    "          [,compress=on|off][,driver specific parameters...]\n"
>       "                configure a block backend\n", QEMU_ARCH_ALL)
>   STEXI
>   @item -blockdev @var{option}[,@var{option}[,@var{option}[,...]]]
> @@ -905,6 +905,8 @@ discard requests.
>   conversion of plain zero writes by the OS to driver specific optimized
>   zero write commands. You may even choose "unmap" if @var{discard} is set
>   to "unmap" to allow a zero write to be converted to an @code{unmap} operation.
> +@item compress
> +Compress all writes to the image.
>   @end table
>   
>   @item Driver-specific options for @code{file}
> @@ -1026,7 +1028,7 @@ DEF("drive", HAS_ARG, QEMU_OPTION_drive,
>       "       [,cache=writethrough|writeback|none|directsync|unsafe][,format=f]\n"
>       "       [,snapshot=on|off][,rerror=ignore|stop|report]\n"
>       "       [,werror=ignore|stop|report|enospc][,id=name][,aio=threads|native]\n"
> -    "       [,readonly=on|off][,copy-on-read=on|off]\n"
> +    "       [,readonly=on|off][,copy-on-read=on|off][,compress=on|off]\n"
>       "       [,discard=ignore|unmap][,detect-zeroes=on|off|unmap]\n"
>       "       [[,bps=b]|[[,bps_rd=r][,bps_wr=w]]]\n"
>       "       [[,iops=i]|[[,iops_rd=r][,iops_wr=w]]]\n"
> 

With extra new line dropped and qiov_offset fixed to zero:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

But, I can't be really sure, as I see from this patch, we have so many ways for
a simple option, and so many places where it should be added.. I just don't see
anything wrong (except for qiov_offset), so if tests work, it's OK for me.
Hope someone who knows the whole options architecture will look at it.

-- 
Best regards,
Vladimir

Re: [PATCH v4 2/4] qcow2: Allow writing compressed data of multiple clusters

[Vladimir Sementsov-Ogievskiy]

16.10.2019 19:28, Andrey Shinkevich wrote:
> QEMU currently supports writing compressed data of the size equal to
> one cluster. This patch allows writing QCOW2 compressed data that
> exceed one cluster. Now, we split buffered data into separate clusters
> and write them compressed using the existing functionality.
> 
> Suggested-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   block/qcow2.c | 102 ++++++++++++++++++++++++++++++++++++++++++----------------
>   1 file changed, 75 insertions(+), 27 deletions(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 6b29e16..9a85d73 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -4156,10 +4156,8 @@ 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_part(BlockDriverState *bs,
> +qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
>                                    uint64_t offset, uint64_t bytes,
>                                    QEMUIOVector *qiov, size_t qiov_offset)
>   {
> @@ -4169,32 +4167,11 @@ qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
>       uint8_t *buf, *out_buf;
>       uint64_t cluster_offset;
>   
> -    if (has_data_file(bs)) {
> -        return -ENOTSUP;
> -    }
> -
> -    if (bytes == 0) {
> -        /* align end of file to a sector boundary to ease reading with
> -           sector based I/Os */
> -        int64_t len = bdrv_getlength(bs->file->bs);
> -        if (len < 0) {
> -            return len;
> -        }
> -        return bdrv_co_truncate(bs->file, len, PREALLOC_MODE_OFF, NULL);
> -    }
> -
> -    if (offset_into_cluster(s, offset)) {
> -        return -EINVAL;
> -    }
> +    assert(bytes == s->cluster_size || (bytes < s->cluster_size &&
> +           (offset + bytes == bs->total_sectors << BDRV_SECTOR_BITS)));
>   
>       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)
> -        {
> -            qemu_vfree(buf);
> -            return -EINVAL;
> -        }
> +    if (bytes < s->cluster_size) {
>           /* Zero-pad last write if image size is not cluster aligned */
>           memset(buf + bytes, 0, s->cluster_size - bytes);
>       }
> @@ -4243,6 +4220,77 @@ fail:
>       return ret;
>   }
>   
> +static coroutine_fn int qcow2_co_pwritev_compressed_task_entry(AioTask *task)
> +{
> +    Qcow2AioTask *t = container_of(task, Qcow2AioTask, task);
> +
> +    assert(!t->cluster_type && !t->l2meta);
> +
> +    return qcow2_co_pwritev_compressed_task(t->bs, t->offset, t->bytes, t->qiov,
> +                                            t->qiov_offset);
> +}
> +
> +/*
> + * XXX: put compressed sectors first, then all the cluster aligned
> +   tables to avoid losing bytes in alignment

missed asterisk

> + */
> +static coroutine_fn int
> +qcow2_co_pwritev_compressed_part(BlockDriverState *bs,
> +                                 uint64_t offset, uint64_t bytes,
> +                                 QEMUIOVector *qiov, size_t qiov_offset)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    AioTaskPool *aio = NULL;
> +    int ret;
> +
> +    if (has_data_file(bs)) {
> +        return -ENOTSUP;
> +    }
> +
> +    if (bytes == 0) {
> +        /*
> +         * align end of file to a sector boundary to ease reading with
> +         * sector based I/Os
> +         */
> +        int64_t len = bdrv_getlength(bs->file->bs);
> +        if (len < 0) {
> +            return len;
> +        }
> +        return bdrv_co_truncate(bs->file, len, PREALLOC_MODE_OFF, NULL);
> +    }
> +
> +    if (offset_into_cluster(s, offset)) {
> +        return -EINVAL;
> +    }
> +
> +    while (bytes && aio_task_pool_status(aio) == 0) {
> +        uint32_t chunk_size = MIN(bytes, s->cluster_size);

Hmm. cluster_size is int. bytes is uint64_t. qcow2_add_task argument type
is uint64_t. I think better to choose from these types.. Abd, I'd prefer to
use uint64_t for chunk_size.. But, uint32_t should work too, of course.

> +
> +        if (!aio && chunk_size != bytes) {
> +            aio = aio_task_pool_new(QCOW2_MAX_WORKERS);
> +        }
> +
> +        ret = qcow2_add_task(bs, aio, qcow2_co_pwritev_compressed_task_entry,
> +                             0, 0, offset, chunk_size, qiov, qiov_offset, NULL);
> +        if (ret < 0) {
> +            break;
> +        }
> +        qiov_offset += chunk_size;
> +        offset += chunk_size;
> +        bytes -= chunk_size;
> +    }
> +
> +    if (aio) {
> +        aio_task_pool_wait_all(aio);
> +        if (ret == 0) {
> +            ret = aio_task_pool_status(aio);
> +        }
> +        g_free(aio);
> +    }
> +
> +    return ret;
> +}
> +
>   static int coroutine_fn
>   qcow2_co_preadv_compressed(BlockDriverState *bs,
>                              uint64_t file_cluster_offset,
> 


With asterisk in comment fixed, and optionally chunk_size type changed to uint64_t:

Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Also, I'd prefer this patch to go first, otherwise we added in previous patch an
option which doesn't work for requests larger than one cluster.

Or, otherwise, you can in previous patch set max_transfer to one cluster in case
of all_writes_compressed, and in this patch drop this restriction.

(Note, that this patch is needed: if we just set max_transfer to one cluster instead
for all_writes_compressed case, we'll miss benefits of aio_task_pool and will not
compress clusters in parallel threads for one request).

-- 
Best regards,
Vladimir

Re: [PATCH v4 3/4] tests/qemu-iotests: add case to write compressed data of multiple clusters

[Vladimir Sementsov-Ogievskiy]

16.10.2019 19:28, Andrey Shinkevich wrote:
> Add the test case to the iotest #214 that checks possibility of writing
> compressed data of more than one cluster size.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/214     | 35 +++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/214.out | 15 +++++++++++++++
>   2 files changed, 50 insertions(+)
> 
> diff --git a/tests/qemu-iotests/214 b/tests/qemu-iotests/214
> index 21ec8a2..0003dc2 100755
> --- a/tests/qemu-iotests/214
> +++ b/tests/qemu-iotests/214
> @@ -89,6 +89,41 @@ _check_test_img -r all
>   $QEMU_IO -c "read  -P 0x11  0 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
>   $QEMU_IO -c "read  -P 0x22 4M 4M" "$TEST_IMG" 2>&1 | _filter_qemu_io | _filter_testdir
>   
> +echo
> +echo "=== Write compressed data of multiple clusters ==="
> +echo
> +cluster_size=0x10000
> +_make_test_img 2M -o cluster_size=$cluster_size
> +
> +echo "Uncompressed data:"
> +let data_size="8 * $cluster_size"
> +$QEMU_IO -c "write -P 0xaa 0 $data_size" "$TEST_IMG" \
> +         2>&1 | _filter_qemu_io | _filter_testdir
> +$QEMU_IMG info "$TEST_IMG" | sed -n '/disk size:/ s/^ *//p'

hmm, I'm afraid, "disk size" is unstable thing for iotests. Consider
backporting: other size of metadata in qcow2 image, and we'll have to
fix this test often..

If you want to check, that clusters are compressed, you may use
qemu-img check (with or without --output=json), it shows amount of
compressed clusters.

> +
> +_make_test_img 2M -o cluster_size=$cluster_size
> +let data_size="3 * $cluster_size + ($cluster_size >> 1)"

I'd be a bit more happy with $cluster_size / 2, still, happiness and
bash scripts are incompatible anyway.

> +# Set compress=on. That will align the written data
> +# by the cluster size and will write them compressed.
> +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT \
> +$QEMU_IO -c "write -P 0xbb 0 $data_size" --image-opts \
> +         driver=$IMGFMT,compress=on,file.filename=$TEST_IMG \
> +         2>&1 | _filter_qemu_io | _filter_testdir
> +
> +let offset="4 * $cluster_size"
> +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT \
> +$QEMU_IO -c "write -P 0xcc $offset $data_size" "json:{\
> +    'driver': '$IMGFMT',
> +    'file': {
> +        'driver': 'file',
> +        'filename': '$TEST_IMG'
> +    },
> +    'compress': true
> +}" | _filter_qemu_io | _filter_testdir
> +

It would be good to add case with unaligned offset as well. And, maybe,
check that we don't rewrite existing data in partial clusters when writing
unaligned compressed data over it.

> +echo "After the multiple cluster data have been written compressed,"
> +$QEMU_IMG info "$TEST_IMG" | sed -n '/disk size:/ s/^ *//p'
> +
>   # success, all done
>   echo '*** done'
>   rm -f $seq.full
> diff --git a/tests/qemu-iotests/214.out b/tests/qemu-iotests/214.out
> index 0fcd8dc..09a2e9a 100644
> --- a/tests/qemu-iotests/214.out
> +++ b/tests/qemu-iotests/214.out
> @@ -32,4 +32,19 @@ read 4194304/4194304 bytes at offset 0
>   4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>   read 4194304/4194304 bytes at offset 4194304
>   4 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +
> +=== Write compressed data of multiple clusters ===
> +
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2097152
> +Uncompressed data:
> +wrote 524288/524288 bytes at offset 0
> +512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +disk size: 772 KiB
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2097152
> +wrote 229376/229376 bytes at offset 0
> +224 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 229376/229376 bytes at offset 262144
> +224 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +After the multiple cluster data have been written compressed,
> +disk size: 268 KiB
>   *** done
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 4/4] tests/qemu-iotests: add case for block-stream compress

[Vladimir Sementsov-Ogievskiy]

16.10.2019 19:28, Andrey Shinkevich wrote:
> Add a case to the iotest #030 that tests the 'compress' option for a
> block-stream job.
> 
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>   tests/qemu-iotests/030     | 51 +++++++++++++++++++++++++++++++++++++++++++++-
>   tests/qemu-iotests/030.out |  4 ++--
>   2 files changed, 52 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index f3766f2..f0f0e26 100755
> --- a/tests/qemu-iotests/030
> +++ b/tests/qemu-iotests/030
> @@ -21,7 +21,8 @@
>   import time
>   import os
>   import iotests
> -from iotests import qemu_img, qemu_io
> +from iotests import qemu_img, qemu_io, qemu_img_pipe
> +import json
>   
>   backing_img = os.path.join(iotests.test_dir, 'backing.img')
>   mid_img = os.path.join(iotests.test_dir, 'mid.img')
> @@ -956,6 +957,54 @@ class TestSetSpeed(iotests.QMPTestCase):
>   
>           self.cancel_and_wait(resume=True)
>   
> +class TestCompressed(iotests.QMPTestCase):
> +    test_img_init_size = 0
> +
> +    def setUp(self):
> +        qemu_img('create', '-f', iotests.imgfmt, backing_img, '1M')
> +        qemu_img('create', '-f', iotests.imgfmt, '-o',
> +                 'backing_file=%s' % backing_img, mid_img)
> +        qemu_img('create', '-f', iotests.imgfmt, '-o',
> +                 'backing_file=%s' % mid_img, test_img)
> +        qemu_io('-c', 'write -P 0x1 0 512k', backing_img)
> +        top = json.loads(qemu_img_pipe('info', '--output=json', test_img))
> +        self.test_img_init_size = top['actual-size']
> +        self.vm = iotests.VM().add_drive(test_img, "backing.node-name=mid," +
> +                                         "backing.backing.node-name=base," +

base node-name is unused actually

> +                                         "compress=on")
> +        self.vm.launch()
> +
> +    def tearDown(self):
> +        self.vm.shutdown()
> +        os.remove(test_img)
> +        os.remove(mid_img)
> +        os.remove(backing_img)
> +
> +    def test_stream_compress(self):
> +        self.assert_no_active_block_jobs()
> +
> +        result = self.vm.qmp('block-stream', device='mid', job_id='stream-mid')
> +        self.assert_qmp(result, 'return', {})
> +
> +        self.wait_until_completed(drive='stream-mid')
> +        # Remove other 'JOB_STATUS_CHANGE' events for the job 'stream-mid'
> +        self.vm.get_qmp_events(wait=True)

Hmm, I think it's unsafe.. One more event may be still in pipe...

I think, better is just do
self.vm.wait_event(name='BLOCK_JOB_COMPLETED), and ignore JOB_STATUS_CHANGE events.

> +
> +        result = self.vm.qmp('block-stream', device='drive0',
> +                             job_id='stream-top')
> +        self.assert_qmp(result, 'return', {})
> +
> +        self.wait_until_completed(drive='stream-top')
> +        self.vm.shutdown()
> +
> +        top = json.loads(qemu_img_pipe('info', '--output=json', test_img))
> +        mid = json.loads(qemu_img_pipe('info', '--output=json', mid_img))
> +        base = json.loads(qemu_img_pipe('info', '--output=json', backing_img))
> +
> +        self.assertEqual(mid['actual-size'], base['actual-size'])
> +        self.assertLess(top['actual-size'], mid['actual-size'])
> +        self.assertLess(self.test_img_init_size, top['actual-size'])

I think last assertion is covered by previous two.

And I'm still unsure about using actual-size. Direct checking number of compressed
clusters by qemu-img check maybe better.

> +
>   if __name__ == '__main__':
>       iotests.main(supported_fmts=['qcow2', 'qed'],
>                    supported_protocols=['file'])
> diff --git a/tests/qemu-iotests/030.out b/tests/qemu-iotests/030.out
> index 6d9bee1..af8dac1 100644
> --- a/tests/qemu-iotests/030.out
> +++ b/tests/qemu-iotests/030.out
> @@ -1,5 +1,5 @@
> -...........................
> +............................
>   ----------------------------------------------------------------------
> -Ran 27 tests
> +Ran 28 tests
>   
>   OK
> 


-- 
Best regards,
Vladimir

Re: [PATCH v4 3/4] tests/qemu-iotests: add case to write compressed data of multiple clusters

[Andrey Shinkevich]

>> +# Set compress=on. That will align the written data
>> +# by the cluster size and will write them compressed.
>> +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT \
>> +$QEMU_IO -c "write -P 0xbb 0 $data_size" --image-opts \
>> +         driver=$IMGFMT,compress=on,file.filename=$TEST_IMG \
>> +         2>&1 | _filter_qemu_io | _filter_testdir
>> +
>> +let offset="4 * $cluster_size"
>> +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT \
>> +$QEMU_IO -c "write -P 0xcc $offset $data_size" "json:{\
>> +    'driver': '$IMGFMT',
>> +    'file': {
>> +        'driver': 'file',
>> +        'filename': '$TEST_IMG'
>> +    },
>> +    'compress': true
>> +}" | _filter_qemu_io | _filter_testdir
>> +
> 
> It would be good to add case with unaligned offset as well. And, maybe,"
> check that we don't rewrite existing data in partial clusters when writing
> unaligned compressed data over it.
> 

The I/O error is raised in that case (see 
qcow2_alloc_compressed_cluster_offset()):
"Compression can't overwrite anything. Fail if the cluster was already 
allocated."

#0  qcow2_alloc_compressed_cluster_offset (bs=0x564669143390, 
offset=393216, compressed_size=79, host_offset=0x7f45289d9f00) at 
block/qcow2-cluster.c:767
#1  0x000056466703da7f in qcow2_co_pwritev_compressed_task 
(bs=0x564669143390, offset=393216, bytes=65536, qiov=0x7f4528dddec0, 
qiov_offset=196608) at block/qcow2.c:4198
#2  0x000056466703dc0c in qcow2_co_pwritev_compressed_task_entry 
(task=0x564669152ac0) at block/qcow2.c:4230
#3  0x00005646670a69d0 in aio_task_co (opaque=0x564669152ac0) at 
block/aio_task.c:45
#4  0x0000564667147a87 in coroutine_trampoline (i0=1762994976, i1=22086) 
at util/coroutine-ucontext.c:115

Andrey
-- 
With the best regards,
Andrey Shinkevich