[PATCH v12 0/3] qcow2: advanced compression options

[PATCH v12 0/3] qcow2: advanced compression options

[Andrey Shinkevich]

The compression filter driver is introduced as suggested by Max.
A sample usage of the filter can be found in the test #214.
Now, multiple clusters can be written compressed.
It is useful for the backup job.

v12:
  01: Missed to change the driver interface .bdrv_co_block_status
      from _status_from_backing to _status_from_file (noticed by
      Vladimir).

Andrey Shinkevich (3):
  block: introduce compress filter driver
  qcow2: Allow writing compressed data of multiple clusters
  tests/qemu-iotests: add case to write compressed data of multiple
    clusters

 block/Makefile.objs        |   1 +
 block/filter-compress.c    | 168 +++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.c              | 102 +++++++++++++++++++--------
 qapi/block-core.json       |  10 +--
 tests/qemu-iotests/214     |  43 ++++++++++++
 tests/qemu-iotests/214.out |  14 ++++
 6 files changed, 307 insertions(+), 31 deletions(-)
 create mode 100644 block/filter-compress.c

-- 
1.8.3.1

[PATCH v12 1/3] block: introduce compress filter driver

[Andrey Shinkevich]

Allow writing all the data compressed through the filter driver.
The written data will be aligned by the cluster size.
Based on the QEMU current implementation, that data can be written to
unallocated clusters only. May be used for a backup job.

Suggested-by: Max Reitz <mreitz@redhat.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/Makefile.objs     |   1 +
 block/filter-compress.c | 168 ++++++++++++++++++++++++++++++++++++++++++++++++
 qapi/block-core.json    |  10 +--
 3 files changed, 175 insertions(+), 4 deletions(-)
 create mode 100644 block/filter-compress.c

diff --git a/block/Makefile.objs b/block/Makefile.objs
index e394fe0..330529b 100644
--- a/block/Makefile.objs
+++ b/block/Makefile.objs
@@ -43,6 +43,7 @@ block-obj-y += crypto.o
 
 block-obj-y += aio_task.o
 block-obj-y += backup-top.o
+block-obj-y += filter-compress.o
 
 common-obj-y += stream.o
 
diff --git a/block/filter-compress.c b/block/filter-compress.c
new file mode 100644
index 0000000..4d756ea
--- /dev/null
+++ b/block/filter-compress.c
@@ -0,0 +1,168 @@
+/*
+ * Compress filter block driver
+ *
+ * Copyright (c) 2019 Virtuozzo International GmbH
+ *
+ * Author:
+ *   Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
+ *   (based on block/copy-on-read.c by Max Reitz)
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 or
+ * (at your option) any later version of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "qemu/osdep.h"
+#include "block/block_int.h"
+#include "qemu/module.h"
+#include "qapi/error.h"
+
+
+static int compress_open(BlockDriverState *bs, QDict *options, int flags,
+                         Error **errp)
+{
+    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, false,
+                               errp);
+    if (!bs->file) {
+        return -EINVAL;
+    }
+
+    if (!bs->file->bs->drv || !block_driver_can_compress(bs->file->bs->drv)) {
+        error_setg(errp,
+                   "Compression is not supported for underlying format: %s",
+                   bdrv_get_format_name(bs->file->bs));
+
+        return -ENOTSUP;
+    }
+
+    bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
+        (BDRV_REQ_FUA & bs->file->bs->supported_write_flags);
+
+    bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
+        ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
+            bs->file->bs->supported_zero_flags);
+
+    return 0;
+}
+
+
+static int64_t compress_getlength(BlockDriverState *bs)
+{
+    return bdrv_getlength(bs->file->bs);
+}
+
+
+static int coroutine_fn compress_co_preadv_part(BlockDriverState *bs,
+                                                uint64_t offset, uint64_t bytes,
+                                                QEMUIOVector *qiov,
+                                                size_t qiov_offset,
+                                                int flags)
+{
+    return bdrv_co_preadv_part(bs->file, offset, bytes, qiov, qiov_offset,
+                               flags);
+}
+
+
+static int coroutine_fn compress_co_pwritev_part(BlockDriverState *bs,
+                                                 uint64_t offset,
+                                                 uint64_t bytes,
+                                                 QEMUIOVector *qiov,
+                                                 size_t qiov_offset, int flags)
+{
+    return bdrv_co_pwritev_part(bs->file, offset, bytes, qiov, qiov_offset,
+                                flags | BDRV_REQ_WRITE_COMPRESSED);
+}
+
+
+static int coroutine_fn compress_co_pwrite_zeroes(BlockDriverState *bs,
+                                                  int64_t offset, int bytes,
+                                                  BdrvRequestFlags flags)
+{
+    return bdrv_co_pwrite_zeroes(bs->file, offset, bytes, flags);
+}
+
+
+static int coroutine_fn compress_co_pdiscard(BlockDriverState *bs,
+                                             int64_t offset, int bytes)
+{
+    return bdrv_co_pdiscard(bs->file, offset, bytes);
+}
+
+
+static void compress_refresh_limits(BlockDriverState *bs, Error **errp)
+{
+    BlockDriverInfo bdi;
+    int ret;
+
+    if (!bs->file) {
+        return;
+    }
+
+    ret = bdrv_get_info(bs->file->bs, &bdi);
+    if (ret < 0 || bdi.cluster_size == 0) {
+        return;
+    }
+
+    bs->bl.request_alignment = bdi.cluster_size;
+}
+
+
+static void compress_eject(BlockDriverState *bs, bool eject_flag)
+{
+    bdrv_eject(bs->file->bs, eject_flag);
+}
+
+
+static void compress_lock_medium(BlockDriverState *bs, bool locked)
+{
+    bdrv_lock_medium(bs->file->bs, locked);
+}
+
+
+static bool compress_recurse_is_first_non_filter(BlockDriverState *bs,
+                                                 BlockDriverState *candidate)
+{
+    return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
+}
+
+
+static BlockDriver bdrv_compress = {
+    .format_name                        = "compress",
+
+    .bdrv_open                          = compress_open,
+    .bdrv_child_perm                    = bdrv_filter_default_perms,
+
+    .bdrv_getlength                     = compress_getlength,
+
+    .bdrv_co_preadv_part                = compress_co_preadv_part,
+    .bdrv_co_pwritev_part               = compress_co_pwritev_part,
+    .bdrv_co_pwrite_zeroes              = compress_co_pwrite_zeroes,
+    .bdrv_co_pdiscard                   = compress_co_pdiscard,
+    .bdrv_refresh_limits                = compress_refresh_limits,
+
+    .bdrv_eject                         = compress_eject,
+    .bdrv_lock_medium                   = compress_lock_medium,
+
+    .bdrv_co_block_status               = bdrv_co_block_status_from_file,
+
+    .bdrv_recurse_is_first_non_filter   = compress_recurse_is_first_non_filter,
+
+    .has_variable_length                = true,
+    .is_filter                          = true,
+};
+
+static void bdrv_compress_init(void)
+{
+    bdrv_register(&bdrv_compress);
+}
+
+block_init(bdrv_compress_init);
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 0cf68fe..93ee04e 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2884,15 +2884,16 @@
 # @copy-on-read: Since 3.0
 # @blklogwrites: Since 3.0
 # @blkreplay: Since 4.2
+# @compress: Since 5.0
 #
 # Since: 2.9
 ##
 { 'enum': 'BlockdevDriver',
   'data': [ 'blkdebug', 'blklogwrites', 'blkreplay', 'blkverify', 'bochs',
-            'cloop', 'copy-on-read', 'dmg', 'file', 'ftp', 'ftps', 'gluster',
-            'host_cdrom', 'host_device', 'http', 'https', 'iscsi', 'luks',
-            'nbd', 'nfs', 'null-aio', 'null-co', 'nvme', 'parallels', 'qcow',
-            'qcow2', 'qed', 'quorum', 'raw', 'rbd',
+            'cloop', 'compress', 'copy-on-read', 'dmg', 'file', 'ftp', 'ftps',
+            'gluster', 'host_cdrom', 'host_device', 'http', 'https', 'iscsi',
+            'luks', 'nbd', 'nfs', 'null-aio', 'null-co', 'nvme', 'parallels',
+            'qcow', 'qcow2', 'qed', 'quorum', 'raw', 'rbd',
             { 'name': 'replication', 'if': 'defined(CONFIG_REPLICATION)' },
             'sheepdog',
             'ssh', 'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs' ] }
@@ -4044,6 +4045,7 @@
       'blkreplay':  'BlockdevOptionsBlkreplay',
       'bochs':      'BlockdevOptionsGenericFormat',
       'cloop':      'BlockdevOptionsGenericFormat',
+      'compress':   'BlockdevOptionsGenericFormat',
       'copy-on-read':'BlockdevOptionsGenericFormat',
       'dmg':        'BlockdevOptionsGenericFormat',
       'file':       'BlockdevOptionsFile',
-- 
1.8.3.1

Re: [PATCH v12 1/3] block: introduce compress filter driver

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 2124 bytes --]

On 02.12.19 13:15, Andrey Shinkevich wrote:
> Allow writing all the data compressed through the filter driver.
> The written data will be aligned by the cluster size.
> Based on the QEMU current implementation, that data can be written to
> unallocated clusters only. May be used for a backup job.
> 
> Suggested-by: Max Reitz <mreitz@redhat.com>
> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/Makefile.objs     |   1 +
>  block/filter-compress.c | 168 ++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json    |  10 +--
>  3 files changed, 175 insertions(+), 4 deletions(-)
>  create mode 100644 block/filter-compress.c

[...]

> diff --git a/block/filter-compress.c b/block/filter-compress.c
> new file mode 100644
> index 0000000..4d756ea
> --- /dev/null
> +++ b/block/filter-compress.c
> @@ -0,0 +1,168 @@

[...]

> +static int compress_open(BlockDriverState *bs, QDict *options, int flags,
> +                         Error **errp)
> +{
> +    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, false,
> +                               errp);
> +    if (!bs->file) {
> +        return -EINVAL;
> +    }
> +
> +    if (!bs->file->bs->drv || !block_driver_can_compress(bs->file->bs->drv)) {
> +        error_setg(errp,
> +                   "Compression is not supported for underlying format: %s",
> +                   bdrv_get_format_name(bs->file->bs));

bdrv_get_format_name() returns NULL if bs->file->bs->drv is NULL.  I’m
sure g_strdup_vprintf() handles %s with a NULL string gracefully in
practice, but I can’t find that specified anywhere.  So even though I’m
well aware I’m being a bit stupid about a minor edge case, I’m hesitant
to accept this patch as-is.

Obviously the solution can be as simple as bdrv_get_format_name(...) ?:
"(no format)".

Well, actually, I can be a bit less stupid about it and just propose
merging that change in myself.  Would that be OK for you?

(The rest looks good to me.)

Max


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

Re: [PATCH v12 1/3] block: introduce compress filter driver

[Andrey Shinkevich]

On 20/12/2019 17:52, Max Reitz wrote:
> On 02.12.19 13:15, Andrey Shinkevich wrote:
>> Allow writing all the data compressed through the filter driver.
>> The written data will be aligned by the cluster size.
>> Based on the QEMU current implementation, that data can be written to
>> unallocated clusters only. May be used for a backup job.
>>
>> Suggested-by: Max Reitz <mreitz@redhat.com>
>> Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/Makefile.objs     |   1 +
>>   block/filter-compress.c | 168 ++++++++++++++++++++++++++++++++++++++++++++++++
>>   qapi/block-core.json    |  10 +--
>>   3 files changed, 175 insertions(+), 4 deletions(-)
>>   create mode 100644 block/filter-compress.c
> 
> [...]
> 
>> diff --git a/block/filter-compress.c b/block/filter-compress.c
>> new file mode 100644
>> index 0000000..4d756ea
>> --- /dev/null
>> +++ b/block/filter-compress.c
>> @@ -0,0 +1,168 @@
> 
> [...]
> 
>> +static int compress_open(BlockDriverState *bs, QDict *options, int flags,
>> +                         Error **errp)
>> +{
>> +    bs->file = bdrv_open_child(NULL, options, "file", bs, &child_file, false,
>> +                               errp);
>> +    if (!bs->file) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (!bs->file->bs->drv || !block_driver_can_compress(bs->file->bs->drv)) {
>> +        error_setg(errp,
>> +                   "Compression is not supported for underlying format: %s",
>> +                   bdrv_get_format_name(bs->file->bs));
> 
> bdrv_get_format_name() returns NULL if bs->file->bs->drv is NULL.  I’m
> sure g_strdup_vprintf() handles %s with a NULL string gracefully in
> practice, but I can’t find that specified anywhere.  So even though I’m
> well aware I’m being a bit stupid about a minor edge case, I’m hesitant
> to accept this patch as-is.
> 
> Obviously the solution can be as simple as bdrv_get_format_name(...) ?:
> "(no format)".
> 
> Well, actually, I can be a bit less stupid about it and just propose
> merging that change in myself.  Would that be OK for you?

Yes, please.
Thank you, Max.

Andrey

> 
> (The rest looks good to me.)
> 
> Max
> 

-- 
With the best regards,
Andrey Shinkevich

[PATCH v12 2/3] 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 block/aio_task API.

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

diff --git a/block/qcow2.c b/block/qcow2.c
index 7c18721..0e03a1a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4222,10 +4222,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)
 {
@@ -4235,32 +4233,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, false, 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);
     }
@@ -4309,6 +4286,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 = 0;
+
+    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, false, PREALLOC_MODE_OFF, NULL);
+    }
+
+    if (offset_into_cluster(s, offset)) {
+        return -EINVAL;
+    }
+
+    while (bytes && aio_task_pool_status(aio) == 0) {
+        uint64_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

Re: [PATCH v12 2/3] qcow2: Allow writing compressed data of multiple clusters

[Alberto Garcia]

On Mon 02 Dec 2019 01:15:05 PM CET, Andrey Shinkevich wrote:
> +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 = 0;
> +
> +    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, false, PREALLOC_MODE_OFF, NULL);
> +    }
> +
> +    if (offset_into_cluster(s, offset)) {
> +        return -EINVAL;
> +    }
> +
> +    while (bytes && aio_task_pool_status(aio) == 0) {
> +        uint64_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;
> +    }

This patch allows the user to write more than one cluster of compressed
data at a time, and it does so by splitting the request into many
cluster-sized requests and using qcow2_add_task() for each one of them.

What happens however is that there's no guarantee that the requests are
processed in the same order that they were added.

One consequence is that running on an empty qcow2 file a command as
simple as this one:

   qemu-io -c 'write -c 0 256k' image.qcow2

does not always produce the same results.

This does not have any user-visible consequences for the guest. In all
cases the data is correctly written, it's just that the ordering of the
compressed clusters (and therefore the contents of the L2 entries) will
be different each time.

Because of this a test cannot expect that running the same commands on
an empty image produces always the same results.

Is this something that we should be concerned about?

Berto

Re: [PATCH v12 2/3] qcow2: Allow writing compressed data of multiple clusters

[Vladimir Sementsov-Ogievskiy]

09.04.2020 19:50, Alberto Garcia wrote:
> On Mon 02 Dec 2019 01:15:05 PM CET, Andrey Shinkevich wrote:
>> +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 = 0;
>> +
>> +    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, false, PREALLOC_MODE_OFF, NULL);
>> +    }
>> +
>> +    if (offset_into_cluster(s, offset)) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    while (bytes && aio_task_pool_status(aio) == 0) {
>> +        uint64_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;
>> +    }
> 
> This patch allows the user to write more than one cluster of compressed
> data at a time, and it does so by splitting the request into many
> cluster-sized requests and using qcow2_add_task() for each one of them.
> 
> What happens however is that there's no guarantee that the requests are
> processed in the same order that they were added.
> 
> One consequence is that running on an empty qcow2 file a command as
> simple as this one:
> 
>     qemu-io -c 'write -c 0 256k' image.qcow2
> 
> does not always produce the same results.
> 
> This does not have any user-visible consequences for the guest. In all
> cases the data is correctly written, it's just that the ordering of the
> compressed clusters (and therefore the contents of the L2 entries) will
> be different each time.
> 
> Because of this a test cannot expect that running the same commands on
> an empty image produces always the same results.
> 
> Is this something that we should be concerned about?
> 

Parallel writing compressed clusters is significant improvement, as it allow compressing in really parallel threads.

Generally, async parallel issuing of several requests gives more performance than handling peaces one-by-one, mirror works on this basis and it is fast. I've already moved qcow2 to this idea (aio tasks in qcow2 code), and in progress of moving backup job. So, I think that asynchrony and ambiguity would be native for block-layer anyway.

Hmm. Still, what about cluster sequence? For normal clusters there may be simple thing to do: preallocation (at least of metadata). So, we can pre-create cluster sequence.. But what to do with compressed clusters if we want specific order for them, I don't know. On the other hand, ordering of normal cluster may make sence: it should increase performnace of following IO. But for compressed clusters it's not the case.

So, I don't think we should make specific workaround for testing... What exactly is the case?

-- 
Best regards,
Vladimir

Re: [PATCH v12 2/3] qcow2: Allow writing compressed data of multiple clusters

[Andrey Shinkevich]

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

We could assign indices to the clusters/chunks and improve the algorithm to write them down on the disk in the same order adjacently. If you find it feasible for QEMU, I'd like to create a task for doing that, shall I?

Andrey

________________________________
From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Sent: Thursday, April 9, 2020 9:39 PM
To: Alberto Garcia <berto@igalia.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>; qemu-devel@nongnu.org <qemu-devel@nongnu.org>; qemu-block@nongnu.org <qemu-block@nongnu.org>
Cc: kwolf@redhat.com <kwolf@redhat.com>; armbru@redhat.com <armbru@redhat.com>; mreitz@redhat.com <mreitz@redhat.com>; Denis Lunev <den@virtuozzo.com>
Subject: Re: [PATCH v12 2/3] qcow2: Allow writing compressed data of multiple clusters

09.04.2020 19:50, Alberto Garcia wrote:
> On Mon 02 Dec 2019 01:15:05 PM CET, Andrey Shinkevich wrote:
>> +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 = 0;
>> +
>> +    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, false, PREALLOC_MODE_OFF, NULL);
>> +    }
>> +
>> +    if (offset_into_cluster(s, offset)) {
>> +        return -EINVAL;
>> +    }
>> +
>> +    while (bytes && aio_task_pool_status(aio) == 0) {
>> +        uint64_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;
>> +    }
>
> This patch allows the user to write more than one cluster of compressed
> data at a time, and it does so by splitting the request into many
> cluster-sized requests and using qcow2_add_task() for each one of them.
>
> What happens however is that there's no guarantee that the requests are
> processed in the same order that they were added.
>
> One consequence is that running on an empty qcow2 file a command as
> simple as this one:
>
>     qemu-io -c 'write -c 0 256k' image.qcow2
>
> does not always produce the same results.
>
> This does not have any user-visible consequences for the guest. In all
> cases the data is correctly written, it's just that the ordering of the
> compressed clusters (and therefore the contents of the L2 entries) will
> be different each time.
>
> Because of this a test cannot expect that running the same commands on
> an empty image produces always the same results.
>
> Is this something that we should be concerned about?
>

Parallel writing compressed clusters is significant improvement, as it allow compressing in really parallel threads.

Generally, async parallel issuing of several requests gives more performance than handling peaces one-by-one, mirror works on this basis and it is fast. I've already moved qcow2 to this idea (aio tasks in qcow2 code), and in progress of moving backup job. So, I think that asynchrony and ambiguity would be native for block-layer anyway.

Hmm. Still, what about cluster sequence? For normal clusters there may be simple thing to do: preallocation (at least of metadata). So, we can pre-create cluster sequence.. But what to do with compressed clusters if we want specific order for them, I don't know. On the other hand, ordering of normal cluster may make sence: it should increase performnace of following IO. But for compressed clusters it's not the case.

So, I don't think we should make specific workaround for testing... What exactly is the case?

--
Best regards,
Vladimir

[-- Attachment #2: Type: text/html, Size: 7955 bytes --]

Re: [PATCH v12 2/3] qcow2: Allow writing compressed data of multiple clusters

[Vladimir Sementsov-Ogievskiy]

10.04.2020 3:12, Andrey Shinkevich wrote:
> We could assign indices to the clusters/chunks and improve the algorithm to write them down on the disk in the same order adjacently. If you find it feasible for QEMU, I'd like to create a task for doing that, shall I?
> 

Compressed cluster occupy different size chunks in the image. How are you going to preallocate? Anyway, I don't see any benefit in ordering compressed clusters, I think it's not worth doing.

> 
> ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
> *From:* Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> *Sent:* Thursday, April 9, 2020 9:39 PM
> *To:* Alberto Garcia <berto@igalia.com>; Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>; qemu-devel@nongnu.org <qemu-devel@nongnu.org>; qemu-block@nongnu.org <qemu-block@nongnu.org>
> *Cc:* kwolf@redhat.com <kwolf@redhat.com>; armbru@redhat.com <armbru@redhat.com>; mreitz@redhat.com <mreitz@redhat.com>; Denis Lunev <den@virtuozzo.com>
> *Subject:* Re: [PATCH v12 2/3] qcow2: Allow writing compressed data of multiple clusters
> 09.04.2020 19:50, Alberto Garcia wrote:
>> On Mon 02 Dec 2019 01:15:05 PM CET, Andrey Shinkevich wrote:
>>> +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 = 0;
>>> +
>>> +    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, false, PREALLOC_MODE_OFF, NULL);
>>> +    }
>>> +
>>> +    if (offset_into_cluster(s, offset)) {
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    while (bytes && aio_task_pool_status(aio) == 0) {
>>> +        uint64_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;
>>> +    }
>> 
>> This patch allows the user to write more than one cluster of compressed
>> data at a time, and it does so by splitting the request into many
>> cluster-sized requests and using qcow2_add_task() for each one of them.
>> 
>> What happens however is that there's no guarantee that the requests are
>> processed in the same order that they were added.
>> 
>> One consequence is that running on an empty qcow2 file a command as
>> simple as this one:
>> 
>>     qemu-io -c 'write -c 0 256k' image.qcow2
>> 
>> does not always produce the same results.
>> 
>> This does not have any user-visible consequences for the guest. In all
>> cases the data is correctly written, it's just that the ordering of the
>> compressed clusters (and therefore the contents of the L2 entries) will
>> be different each time.
>> 
>> Because of this a test cannot expect that running the same commands on
>> an empty image produces always the same results.
>> 
>> Is this something that we should be concerned about?
>> 
> 
> Parallel writing compressed clusters is significant improvement, as it allow compressing in really parallel threads.
> 
> Generally, async parallel issuing of several requests gives more performance than handling peaces one-by-one, mirror works on this basis and it is fast. I've already moved qcow2 to this idea (aio tasks in qcow2 code), and in progress of moving backup job. So, I think that asynchrony and ambiguity would be native for block-layer anyway.
> 
> Hmm. Still, what about cluster sequence? For normal clusters there may be simple thing to do: preallocation (at least of metadata). So, we can pre-create cluster sequence.. But what to do with compressed clusters if we want specific order for them, I don't know. On the other hand, ordering of normal cluster may make sence: it should increase performnace of following IO. But for compressed clusters it's not the case.
> 
> So, I don't think we should make specific workaround for testing... What exactly is the case?
> 
> -- 
> Best regards,
> Vladimir


-- 
Best regards,
Vladimir

Re: [PATCH v12 2/3] qcow2: Allow writing compressed data of multiple clusters

[Alberto Garcia]

On Thu 09 Apr 2020 08:39:12 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
>> Because of this a test cannot expect that running the same commands on
>> an empty image produces always the same results.
>> 
>> Is this something that we should be concerned about?
>
> Parallel writing compressed clusters is significant improvement, as it
> allow compressing in really parallel threads.

I see, I just wasn't sure if you were aware of this side effect.

> So, I don't think we should make specific workaround for
> testing... What exactly is the case?

I noticed this while writing some tests for the subcluster allocation
feature, but this is not a problem for me. Many of our iotests make
assumptions about the location of L2 and refcount tables so changing
those would break a lot of them. This thing only changes the offset of
the compressed data clusters (and their L2 entries), but as far as I'm
aware no one relies on them being predictable. I just need to make sure
that I don't do it either.

Berto

Re: [PATCH v12 2/3] qcow2: Allow writing compressed data of multiple clusters

[Vladimir Sementsov-Ogievskiy]

10.04.2020 14:12, Alberto Garcia wrote:
> On Thu 09 Apr 2020 08:39:12 PM CEST, Vladimir Sementsov-Ogievskiy wrote:
>>> Because of this a test cannot expect that running the same commands on
>>> an empty image produces always the same results.
>>>
>>> Is this something that we should be concerned about?
>>
>> Parallel writing compressed clusters is significant improvement, as it
>> allow compressing in really parallel threads.
> 
> I see, I just wasn't sure if you were aware of this side effect.

No, we didn't thought about it, so good to know, thanks.

> 
>> So, I don't think we should make specific workaround for
>> testing... What exactly is the case?
> 
> I noticed this while writing some tests for the subcluster allocation
> feature, but this is not a problem for me. Many of our iotests make
> assumptions about the location of L2 and refcount tables so changing
> those would break a lot of them. This thing only changes the offset of
> the compressed data clusters (and their L2 entries), but as far as I'm
> aware no one relies on them being predictable. I just need to make sure
> that I don't do it either.
> 

OK. I had similar problems (because of asynchronicity) with existing iotests
in may series for backup. As I remember, I had to add options to just disable
asynchronicity for some tests. So, if needed, we can add some options for
qcow2 (which can be used to justify number of parallel requests, not only to
disable them at all). Still, of course, it's better to avoid testing only
sequential IO when it is async without options.



-- 
Best regards,
Vladimir

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

[Andrey Shinkevich]

Add the case to the iotest #214 that checks possibility of writing
compressed data of more than one cluster size. The test case involves
the compress filter driver showing a sample usage of that.

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/214     | 43 +++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/214.out | 14 ++++++++++++++
 2 files changed, 57 insertions(+)

diff --git a/tests/qemu-iotests/214 b/tests/qemu-iotests/214
index 21ec8a2..0b6ea0b 100755
--- a/tests/qemu-iotests/214
+++ b/tests/qemu-iotests/214
@@ -89,6 +89,49 @@ _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 "Write 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
+sizeA=$($QEMU_IMG info --output=json "$TEST_IMG" |
+        sed -n '/"actual-size":/ s/[^0-9]//gp')
+
+_make_test_img 2M -o cluster_size=$cluster_size
+echo "Write compressed data:"
+let data_size="3 * $cluster_size + $cluster_size / 2"
+# 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=compress,file.driver=$IMGFMT,file.file.driver=file,file.file.filename=$TEST_IMG" \
+         2>&1 | _filter_qemu_io | _filter_testdir
+
+let offset="4 * $cluster_size + $cluster_size / 4"
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT \
+$QEMU_IO -c "write -P 0xcc $offset $data_size" "json:{\
+    'driver': 'compress',
+    'file': {'driver': '$IMGFMT',
+             'file': {'driver': 'file',
+                      'filename': '$TEST_IMG'}}}" | \
+                          _filter_qemu_io | _filter_testdir
+
+sizeB=$($QEMU_IMG info --output=json "$TEST_IMG" |
+        sed -n '/"actual-size":/ s/[^0-9]//gp')
+
+if [ $sizeA -le $sizeB ]
+then
+    echo "Compression ERROR"
+fi
+
+$QEMU_IMG check --output=json "$TEST_IMG" |
+          sed -n 's/,$//; /"compressed-clusters":/ 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..9fc6728 100644
--- a/tests/qemu-iotests/214.out
+++ b/tests/qemu-iotests/214.out
@@ -32,4 +32,18 @@ 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
+Write uncompressed data:
+wrote 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=2097152
+Write compressed data:
+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 278528
+224 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+"compressed-clusters": 8
 *** done
-- 
1.8.3.1

Re: [PATCH v12 0/3] qcow2: advanced compression options

[Andrey Shinkevich]

Pinging...
(please)

On 02/12/2019 15:15, Andrey Shinkevich wrote:
> The compression filter driver is introduced as suggested by Max.
> A sample usage of the filter can be found in the test #214.
> Now, multiple clusters can be written compressed.
> It is useful for the backup job.
> 
> v12:
>    01: Missed to change the driver interface .bdrv_co_block_status
>        from _status_from_backing to _status_from_file (noticed by
>        Vladimir).
> 
> Andrey Shinkevich (3):
>    block: introduce compress filter driver
>    qcow2: Allow writing compressed data of multiple clusters
>    tests/qemu-iotests: add case to write compressed data of multiple
>      clusters
> 
>   block/Makefile.objs        |   1 +
>   block/filter-compress.c    | 168 +++++++++++++++++++++++++++++++++++++++++++++
>   block/qcow2.c              | 102 +++++++++++++++++++--------
>   qapi/block-core.json       |  10 +--
>   tests/qemu-iotests/214     |  43 ++++++++++++
>   tests/qemu-iotests/214.out |  14 ++++
>   6 files changed, 307 insertions(+), 31 deletions(-)
>   create mode 100644 block/filter-compress.c
> 

-- 
With the best regards,
Andrey Shinkevich

Re: [PATCH v12 0/3] qcow2: advanced compression options

[Max Reitz]

[-- Attachment #1.1: Type: text/plain, Size: 1213 bytes --]

On 02.12.19 13:15, Andrey Shinkevich wrote:
> The compression filter driver is introduced as suggested by Max.
> A sample usage of the filter can be found in the test #214.
> Now, multiple clusters can be written compressed.
> It is useful for the backup job.
> 
> v12:
>   01: Missed to change the driver interface .bdrv_co_block_status
>       from _status_from_backing to _status_from_file (noticed by
>       Vladimir).
> 
> Andrey Shinkevich (3):
>   block: introduce compress filter driver
>   qcow2: Allow writing compressed data of multiple clusters
>   tests/qemu-iotests: add case to write compressed data of multiple
>     clusters
> 
>  block/Makefile.objs        |   1 +
>  block/filter-compress.c    | 168 +++++++++++++++++++++++++++++++++++++++++++++
>  block/qcow2.c              | 102 +++++++++++++++++++--------
>  qapi/block-core.json       |  10 +--
>  tests/qemu-iotests/214     |  43 ++++++++++++
>  tests/qemu-iotests/214.out |  14 ++++
>  6 files changed, 307 insertions(+), 31 deletions(-)
>  create mode 100644 block/filter-compress.c

Thanks, fixed patch 1 and applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max


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