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

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

[Andrey Shinkevich]

New enhancements for writing compressed data to QCOW2 image.

v5: The new iotests cases were amended and 'qiov_offset' parameter
    value in the function bdrv_driver_pwritev_compressed() invoked
    from the bdrv_co_do_copy_on_readv() fixed to 0.
    Discussed on the email thread with ID:
    <1571243333-882302-1-git-send-email-andrey.shinkevich@virtuozzo.com>

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                 |  13 ++++--
 block/qcow2.c              | 106 +++++++++++++++++++++++++++++++++------------
 blockdev.c                 |   9 +++-
 include/block/block.h      |   1 +
 include/block/block_int.h  |   2 +
 qapi/block-core.json       |   5 ++-
 qemu-options.hx            |   6 ++-
 tests/qemu-iotests/030     |  34 ++++++++++++++-
 tests/qemu-iotests/030.out |   4 +-
 tests/qemu-iotests/214     |  45 +++++++++++++++++++
 tests/qemu-iotests/214.out |  14 ++++++
 12 files changed, 220 insertions(+), 39 deletions(-)

-- 
1.8.3.1

[PATCH v5 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c                   | 20 +++++++++++++++++++-
 block/io.c                | 13 +++++++++----
 block/qcow2.c             |  4 ++++
 blockdev.c                |  9 ++++++++-
 include/block/block.h     |  1 +
 include/block/block_int.h |  2 ++
 qapi/block-core.json      |  5 ++++-
 qemu-options.hx           |  6 ++++--
 8 files changed, 51 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..eb2ed36 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1360,9 +1360,14 @@ 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, 0);
+                } else {
+                    ret = bdrv_driver_pwritev(bs, cluster_offset, pnum,
+                                              &local_qiov, 0,
+                                              BDRV_REQ_WRITE_UNCHANGED);
+                }
             }
 
             if (ret < 0) {
@@ -1954,7 +1959,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..d57064a 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4013,6 +4013,8 @@
 #                 (default: off)
 # @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.
 #
@@ -4026,7 +4028,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

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

[Max Reitz]

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

On 20.10.19 22:37, 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>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block.c                   | 20 +++++++++++++++++++-
>  block/io.c                | 13 +++++++++----
>  block/qcow2.c             |  4 ++++
>  blockdev.c                |  9 ++++++++-
>  include/block/block.h     |  1 +
>  include/block/block_int.h |  2 ++
>  qapi/block-core.json      |  5 ++++-
>  qemu-options.hx           |  6 ++++--
>  8 files changed, 51 insertions(+), 9 deletions(-)

The problem with compression is that there are such tight constraints on
it that it can really only work for very defined use cases.  Those
constraints are:

- Only write whole clusters,
- Clusters can be written to only once.

The first point is addressed in this patch by setting request_alignment.
 But I don’t see how the second one can be addressed.  Well, maybe by
allowing it in all drivers that support compression.  But if I just look
at qcow2, that isn’t going to be trivial: You need to allocate a
completely new cluster where you write the data (in case it grows), and
thus you leave behind a hole, which kind of defeats the purpose of
compression.

(For demonstration:

$ ./qemu-img create -f qcow2 test.qcow2 64M
Formatting 'test.qcow2', fmt=qcow2 size=67108864 cluster_size=65536
lazy_refcounts=off refcount_bits=16
$ x86_64-softmmu/qemu-system-x86_64 \
    -blockdev "{'node-name': 'drv0', 'driver': 'qcow2',
                'compress': true,
                'file': {'driver': 'file', 'filename': 'test.qcow2'}}" \
    -monitor stdio
QEMU 4.1.50 monitor - type 'help' for more information
(qemu) qemu-io drv0 "write -P 42 0 64k"
wrote 65536/65536 bytes at offset 0
64 KiB, 1 ops; 00.02 sec (4.055 MiB/sec and 64.8793 ops/sec)
(qemu) qemu-io drv0 "write -P 23 0 64k"
write failed: Input/output error

)

Compression really only works when you fully write all of an image
exactly once; i.e. as the qemu-img convert or as a backup target.  For
both cases we already have a compression option.  So I’m wondering where
this new option is really useful.

(You do add a test for stream, but I don’t know whether that’s really a
good example, see my response there.)

Max


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

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

[Andrey Shinkevich]

On 22/10/2019 12:28, Max Reitz wrote:
> On 20.10.19 22:37, 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>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block.c                   | 20 +++++++++++++++++++-
>>   block/io.c                | 13 +++++++++----
>>   block/qcow2.c             |  4 ++++
>>   blockdev.c                |  9 ++++++++-
>>   include/block/block.h     |  1 +
>>   include/block/block_int.h |  2 ++
>>   qapi/block-core.json      |  5 ++++-
>>   qemu-options.hx           |  6 ++++--
>>   8 files changed, 51 insertions(+), 9 deletions(-)
> 
> The problem with compression is that there are such tight constraints on
> it that it can really only work for very defined use cases.  Those
> constraints are:
> 
> - Only write whole clusters,
> - Clusters can be written to only once.
> 
> The first point is addressed in this patch by setting request_alignment.
>   But I don’t see how the second one can be addressed.  Well, maybe by
> allowing it in all drivers that support compression.  But if I just look
> at qcow2, that isn’t going to be trivial: You need to allocate a
> completely new cluster where you write the data (in case it grows), and
> thus you leave behind a hole, which kind of defeats the purpose of
> compression.
> 
> (For demonstration:
> 
> $ ./qemu-img create -f qcow2 test.qcow2 64M
> Formatting 'test.qcow2', fmt=qcow2 size=67108864 cluster_size=65536
> lazy_refcounts=off refcount_bits=16
> $ x86_64-softmmu/qemu-system-x86_64 \
>      -blockdev "{'node-name': 'drv0', 'driver': 'qcow2',
>                  'compress': true,
>                  'file': {'driver': 'file', 'filename': 'test.qcow2'}}" \
>      -monitor stdio
> QEMU 4.1.50 monitor - type 'help' for more information
> (qemu) qemu-io drv0 "write -P 42 0 64k"
> wrote 65536/65536 bytes at offset 0
> 64 KiB, 1 ops; 00.02 sec (4.055 MiB/sec and 64.8793 ops/sec)
> (qemu) qemu-io drv0 "write -P 23 0 64k"
> write failed: Input/output error
> 
> )
> 
> Compression really only works when you fully write all of an image
> exactly once; i.e. as the qemu-img convert or as a backup target.  For
> both cases we already have a compression option.  So I’m wondering where
> this new option is really useful.
> 
> (You do add a test for stream, but I don’t know whether that’s really a
> good example, see my response there.)
> 
> Max
> 

Thank you very much Max for your detailed response.

1) You are right that compression is used with the backup mostly. The 
option for the compression with backup would be replaced by usage at the 
block layer, with no duplication. Also, it can be useful for NBD for 
instance,

$ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
$ sudo ./qemu-nbd -c /dev/nbd0 ./image.qcow2
$ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
10+0 records in
10+0 records out
104857600 bytes (105 MB) copied, 0,0890581 s, 1,2 GB/s
$ sudo ./qemu-nbd -d /dev/nbd0
$ du -sh ./image.qcow2
101M    ./image.qcow2

and with the compression

$ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
$ sudo ./qemu-nbd -C -c /dev/nbd0 ./image.qcow2
$ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
10+0 records in
10+0 records out
104857600 bytes (105 MB) copied, 0,076046 s, 1,4 GB/s
$ sudo ./qemu-nbd -d /dev/nbd0
$ du -sh ./image.qcow2
5,3M    ./image.qcow2

The idea behind introducing the new 'compress' option is to use that 
only one instead of many other ones of such a kind.

2) You are right also that "Compression can't overwrite anything..."
It can be seen in the commit message 
b0b6862e5e1a1394e0ab3d5da94ba8b0da8664e2

I am not sure if data should be written compressed to the active layer.
I made the tests with the idea of bringing examples of usage the 
'compress' option because passing an option is a tricky thing in QEMU.
But the issue takes place anyway if we want to rewrite to allocated 
clusters.
I would like to investigate the matter and make a patch that resolves 
that issue.
Do you agree with that?

Andrey
-- 
With the best regards,
Andrey Shinkevich

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

[Vladimir Sementsov-Ogievskiy]

22.10.2019 13:21, Andrey Shinkevich wrote:
> 
> On 22/10/2019 12:28, Max Reitz wrote:
>> On 20.10.19 22:37, 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>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> ---
>>>    block.c                   | 20 +++++++++++++++++++-
>>>    block/io.c                | 13 +++++++++----
>>>    block/qcow2.c             |  4 ++++
>>>    blockdev.c                |  9 ++++++++-
>>>    include/block/block.h     |  1 +
>>>    include/block/block_int.h |  2 ++
>>>    qapi/block-core.json      |  5 ++++-
>>>    qemu-options.hx           |  6 ++++--
>>>    8 files changed, 51 insertions(+), 9 deletions(-)
>>
>> The problem with compression is that there are such tight constraints on
>> it that it can really only work for very defined use cases.  Those
>> constraints are:
>>
>> - Only write whole clusters,
>> - Clusters can be written to only once.
>>
>> The first point is addressed in this patch by setting request_alignment.
>>    But I don’t see how the second one can be addressed.  Well, maybe by
>> allowing it in all drivers that support compression.  But if I just look
>> at qcow2, that isn’t going to be trivial: You need to allocate a
>> completely new cluster where you write the data (in case it grows), and
>> thus you leave behind a hole, which kind of defeats the purpose of
>> compression.
>>
>> (For demonstration:
>>
>> $ ./qemu-img create -f qcow2 test.qcow2 64M
>> Formatting 'test.qcow2', fmt=qcow2 size=67108864 cluster_size=65536
>> lazy_refcounts=off refcount_bits=16
>> $ x86_64-softmmu/qemu-system-x86_64 \
>>       -blockdev "{'node-name': 'drv0', 'driver': 'qcow2',
>>                   'compress': true,
>>                   'file': {'driver': 'file', 'filename': 'test.qcow2'}}" \
>>       -monitor stdio
>> QEMU 4.1.50 monitor - type 'help' for more information
>> (qemu) qemu-io drv0 "write -P 42 0 64k"
>> wrote 65536/65536 bytes at offset 0
>> 64 KiB, 1 ops; 00.02 sec (4.055 MiB/sec and 64.8793 ops/sec)
>> (qemu) qemu-io drv0 "write -P 23 0 64k"
>> write failed: Input/output error
>>
>> )
>>
>> Compression really only works when you fully write all of an image
>> exactly once; i.e. as the qemu-img convert or as a backup target.  For
>> both cases we already have a compression option.  So I’m wondering where
>> this new option is really useful.
>>
>> (You do add a test for stream, but I don’t know whether that’s really a
>> good example, see my response there.)
>>
>> Max
>>
> 
> Thank you very much Max for your detailed response.
> 
> 1) You are right that compression is used with the backup mostly. The
> option for the compression with backup would be replaced by usage at the
> block layer, with no duplication. Also, it can be useful for NBD for
> instance,
> 
> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
> $ sudo ./qemu-nbd -c /dev/nbd0 ./image.qcow2
> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
> 10+0 records in
> 10+0 records out
> 104857600 bytes (105 MB) copied, 0,0890581 s, 1,2 GB/s
> $ sudo ./qemu-nbd -d /dev/nbd0
> $ du -sh ./image.qcow2
> 101M    ./image.qcow2
> 
> and with the compression
> 
> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
> $ sudo ./qemu-nbd -C -c /dev/nbd0 ./image.qcow2
> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
> 10+0 records in
> 10+0 records out
> 104857600 bytes (105 MB) copied, 0,076046 s, 1,4 GB/s
> $ sudo ./qemu-nbd -d /dev/nbd0
> $ du -sh ./image.qcow2
> 5,3M    ./image.qcow2
> 
> The idea behind introducing the new 'compress' option is to use that
> only one instead of many other ones of such a kind.
> 
> 2) You are right also that "Compression can't overwrite anything..."
> It can be seen in the commit message
> b0b6862e5e1a1394e0ab3d5da94ba8b0da8664e2
> 
> I am not sure if data should be written compressed to the active layer.
> I made the tests with the idea of bringing examples of usage the
> 'compress' option because passing an option is a tricky thing in QEMU.
> But the issue takes place anyway if we want to rewrite to allocated
> clusters.
> I would like to investigate the matter and make a patch that resolves
> that issue.
> Do you agree with that?
> 
> Andrey
> 


Yes, we want this option not to allow compressed writes for guests, but to
allow
- stream with compression (used to remove compressed incremental backup, we
need to merge it to the next incremental)
- backup with compression (we have an optional already, so it works)
- backup to nbd server with compression: enable compression on nbd server

So instead of adding two options (for stream and for nbd), it seems better to
add only one for generic layer.

Then, it becomes possible to run guest on image with compress=on. It's a side
effect, but still it should work correctly.

I think the simplest thing is to just run normal write, if compressed write
failed because of reallocation. We should check that on that failure-path
ENOTSUP is returned and handle it for compress=on option by fallback to
normal write in generic block/io.c

(Note, that in case with stream we rewrite only unallocated clusters)



-- 
Best regards,
Vladimir

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

[Max Reitz]

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

On 22.10.19 12:46, Vladimir Sementsov-Ogievskiy wrote:
> 22.10.2019 13:21, Andrey Shinkevich wrote:
>>
>> On 22/10/2019 12:28, Max Reitz wrote:
>>> On 20.10.19 22:37, 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>
>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>    block.c                   | 20 +++++++++++++++++++-
>>>>    block/io.c                | 13 +++++++++----
>>>>    block/qcow2.c             |  4 ++++
>>>>    blockdev.c                |  9 ++++++++-
>>>>    include/block/block.h     |  1 +
>>>>    include/block/block_int.h |  2 ++
>>>>    qapi/block-core.json      |  5 ++++-
>>>>    qemu-options.hx           |  6 ++++--
>>>>    8 files changed, 51 insertions(+), 9 deletions(-)
>>>
>>> The problem with compression is that there are such tight constraints on
>>> it that it can really only work for very defined use cases.  Those
>>> constraints are:
>>>
>>> - Only write whole clusters,
>>> - Clusters can be written to only once.
>>>
>>> The first point is addressed in this patch by setting request_alignment.
>>>    But I don’t see how the second one can be addressed.  Well, maybe by
>>> allowing it in all drivers that support compression.  But if I just look
>>> at qcow2, that isn’t going to be trivial: You need to allocate a
>>> completely new cluster where you write the data (in case it grows), and
>>> thus you leave behind a hole, which kind of defeats the purpose of
>>> compression.
>>>
>>> (For demonstration:
>>>
>>> $ ./qemu-img create -f qcow2 test.qcow2 64M
>>> Formatting 'test.qcow2', fmt=qcow2 size=67108864 cluster_size=65536
>>> lazy_refcounts=off refcount_bits=16
>>> $ x86_64-softmmu/qemu-system-x86_64 \
>>>       -blockdev "{'node-name': 'drv0', 'driver': 'qcow2',
>>>                   'compress': true,
>>>                   'file': {'driver': 'file', 'filename': 'test.qcow2'}}" \
>>>       -monitor stdio
>>> QEMU 4.1.50 monitor - type 'help' for more information
>>> (qemu) qemu-io drv0 "write -P 42 0 64k"
>>> wrote 65536/65536 bytes at offset 0
>>> 64 KiB, 1 ops; 00.02 sec (4.055 MiB/sec and 64.8793 ops/sec)
>>> (qemu) qemu-io drv0 "write -P 23 0 64k"
>>> write failed: Input/output error
>>>
>>> )
>>>
>>> Compression really only works when you fully write all of an image
>>> exactly once; i.e. as the qemu-img convert or as a backup target.  For
>>> both cases we already have a compression option.  So I’m wondering where
>>> this new option is really useful.
>>>
>>> (You do add a test for stream, but I don’t know whether that’s really a
>>> good example, see my response there.)
>>>
>>> Max
>>>
>>
>> Thank you very much Max for your detailed response.
>>
>> 1) You are right that compression is used with the backup mostly. The
>> option for the compression with backup would be replaced by usage at the
>> block layer, with no duplication. Also, it can be useful for NBD for
>> instance,
>>
>> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
>> $ sudo ./qemu-nbd -c /dev/nbd0 ./image.qcow2
>> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
>> 10+0 records in
>> 10+0 records out
>> 104857600 bytes (105 MB) copied, 0,0890581 s, 1,2 GB/s
>> $ sudo ./qemu-nbd -d /dev/nbd0
>> $ du -sh ./image.qcow2
>> 101M    ./image.qcow2
>>
>> and with the compression
>>
>> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
>> $ sudo ./qemu-nbd -C -c /dev/nbd0 ./image.qcow2
>> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
>> 10+0 records in
>> 10+0 records out
>> 104857600 bytes (105 MB) copied, 0,076046 s, 1,4 GB/s
>> $ sudo ./qemu-nbd -d /dev/nbd0
>> $ du -sh ./image.qcow2
>> 5,3M    ./image.qcow2

That seems wrong to me.  Why not use qemu-img convert for this case?

Attaching an NBD server to a compressed disk has exactly the same
problem as attaching a compressed disk to a VM.  It won’t work unless
the client/guest is aware of the limitations.

>> The idea behind introducing the new 'compress' option is to use that
>> only one instead of many other ones of such a kind.
>>
>> 2) You are right also that "Compression can't overwrite anything..."
>> It can be seen in the commit message
>> b0b6862e5e1a1394e0ab3d5da94ba8b0da8664e2
>>
>> I am not sure if data should be written compressed to the active layer.
>> I made the tests with the idea of bringing examples of usage the
>> 'compress' option because passing an option is a tricky thing in QEMU.
>> But the issue takes place anyway if we want to rewrite to allocated
>> clusters.
>> I would like to investigate the matter and make a patch that resolves
>> that issue.
>> Do you agree with that?

What seems wrong to me is that this series just adds a generic compress
option without ensuring that it works generically.

Either (1) it only works in well-defined cases, then either (1A) we have
to ensure that we only allow it then (as we do now, because only
qemu-img convert and the backup job have such an option; and those two
are safe), or (1B) we have to clearly give a big warning for the new
option that it doesn’t work correctly.  I don’t know whether such a
warning is even possible with just a generic node-level option.

Or (2) we make it work in generic cases.  Well, that might be possible
for qcow2, but who’s going to make it work for VMDK’s streamOptimized
subformat?

More on all of that below.

> Yes, we want this option not to allow compressed writes for guests, but to
> allow
> - stream with compression (used to remove compressed incremental backup, we
> need to merge it to the next incremental)

Based on the assumption that one shouldn’t attach a compressed disk to a
VM, I don’t see how streaming makes sense then.  Well, I suppose
intermediate streaming would work.

> - backup with compression (we have an optional already, so it works)
> - backup to nbd server with compression: enable compression on nbd server

The problem is clearly that if a generic client were to connect to the
NBD server, it wouldn’t work.  In this case, compression will work only
if the clients understands the limitation.

(The safe way would be to make the NBD server provide an option that
clients can see so they know this limitation and agree they’ll adhere to
it.  It’s also a stupid way.)

> So instead of adding two options (for stream and for nbd), it seems better to
> add only one for generic layer.

I don’t know.  It doesn’t work generically, so I don’t know whether it
should be a generic option.

> Then, it becomes possible to run guest on image with compress=on. It's a side
> effect, but still it should work correctly.

How so?  compress=on only works if every party involved only writes to
any cluster of the image exactly once.  That is just not the case for
guests unless they know this limitation, and even I don’t see a use case.

> I think the simplest thing is to just run normal write, if compressed write
> failed because of reallocation. We should check that on that failure-path
> ENOTSUP is returned and handle it for compress=on option by fallback to
> normal write in generic block/io.c

It seems wrong to not compress data with compress=on.

> (Note, that in case with stream we rewrite only unallocated clusters)

Yes, but if the stream job writes the cluster before the guest, the
guest gets an I/O error.


By the way, the other thing I wondered was whether this should be a
filter driver instead (if it makes sense at all).  Such a filter driver
would at least be sufficiently cumbersome to use that probably only
users who understand the implications would make use of it (1B above).


I’m not against adding a generic compress=on option if we ensure that it
actually works generically (2 above).  It doesn’t right now, so I don’t
think this is right.

I’m already not sure whether it’s really possible to support generic
compressed writes in qcow2.  I suppose it’d be at least awkward.  In
theory it should work, it’s just that we can’t keep track of subcluster
allocations, which in the worst case means that some compressed clusters
take up a whole cluster of space.

For VMDK...  I suppose we could consider a new flag for block drivers
that flags whether a driver supports arbitrary compressed writes or has
the old limitations.  compress=on could then refuse to work on any block
driver but the ones that support arbitrary compressed writes.


Our current model (1A) is simply to ensure that all compressed writes
can adhere to the limitations.  As I’ve said above, extending this to
NBD would mean adding some NBD negotiation so both client and server
agree on this limitation.  That seems kind of reasonable from the qemu
side, but probably very unreasonable from an NBD side.  Why would NBD
bother to reproduce qemu’s limitations?


So these are the three ways (1A, 1B, 2) I can imagine.  But just adding
a generic option that unsuspecting users are not unlikely to use but
that simply doesn’t work generically doesn’t seem right to me.

Max


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

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

[Vladimir Sementsov-Ogievskiy]

22.10.2019 14:31, Max Reitz wrote:
> On 22.10.19 12:46, Vladimir Sementsov-Ogievskiy wrote:
>> 22.10.2019 13:21, Andrey Shinkevich wrote:
>>>
>>> On 22/10/2019 12:28, Max Reitz wrote:
>>>> On 20.10.19 22:37, 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>
>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>     block.c                   | 20 +++++++++++++++++++-
>>>>>     block/io.c                | 13 +++++++++----
>>>>>     block/qcow2.c             |  4 ++++
>>>>>     blockdev.c                |  9 ++++++++-
>>>>>     include/block/block.h     |  1 +
>>>>>     include/block/block_int.h |  2 ++
>>>>>     qapi/block-core.json      |  5 ++++-
>>>>>     qemu-options.hx           |  6 ++++--
>>>>>     8 files changed, 51 insertions(+), 9 deletions(-)
>>>>
>>>> The problem with compression is that there are such tight constraints on
>>>> it that it can really only work for very defined use cases.  Those
>>>> constraints are:
>>>>
>>>> - Only write whole clusters,
>>>> - Clusters can be written to only once.
>>>>
>>>> The first point is addressed in this patch by setting request_alignment.
>>>>     But I don’t see how the second one can be addressed.  Well, maybe by
>>>> allowing it in all drivers that support compression.  But if I just look
>>>> at qcow2, that isn’t going to be trivial: You need to allocate a
>>>> completely new cluster where you write the data (in case it grows), and
>>>> thus you leave behind a hole, which kind of defeats the purpose of
>>>> compression.
>>>>
>>>> (For demonstration:
>>>>
>>>> $ ./qemu-img create -f qcow2 test.qcow2 64M
>>>> Formatting 'test.qcow2', fmt=qcow2 size=67108864 cluster_size=65536
>>>> lazy_refcounts=off refcount_bits=16
>>>> $ x86_64-softmmu/qemu-system-x86_64 \
>>>>        -blockdev "{'node-name': 'drv0', 'driver': 'qcow2',
>>>>                    'compress': true,
>>>>                    'file': {'driver': 'file', 'filename': 'test.qcow2'}}" \
>>>>        -monitor stdio
>>>> QEMU 4.1.50 monitor - type 'help' for more information
>>>> (qemu) qemu-io drv0 "write -P 42 0 64k"
>>>> wrote 65536/65536 bytes at offset 0
>>>> 64 KiB, 1 ops; 00.02 sec (4.055 MiB/sec and 64.8793 ops/sec)
>>>> (qemu) qemu-io drv0 "write -P 23 0 64k"
>>>> write failed: Input/output error
>>>>
>>>> )
>>>>
>>>> Compression really only works when you fully write all of an image
>>>> exactly once; i.e. as the qemu-img convert or as a backup target.  For
>>>> both cases we already have a compression option.  So I’m wondering where
>>>> this new option is really useful.
>>>>
>>>> (You do add a test for stream, but I don’t know whether that’s really a
>>>> good example, see my response there.)
>>>>
>>>> Max
>>>>
>>>
>>> Thank you very much Max for your detailed response.
>>>
>>> 1) You are right that compression is used with the backup mostly. The
>>> option for the compression with backup would be replaced by usage at the
>>> block layer, with no duplication. Also, it can be useful for NBD for
>>> instance,
>>>
>>> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
>>> $ sudo ./qemu-nbd -c /dev/nbd0 ./image.qcow2
>>> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
>>> 10+0 records in
>>> 10+0 records out
>>> 104857600 bytes (105 MB) copied, 0,0890581 s, 1,2 GB/s
>>> $ sudo ./qemu-nbd -d /dev/nbd0
>>> $ du -sh ./image.qcow2
>>> 101M    ./image.qcow2
>>>
>>> and with the compression
>>>
>>> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
>>> $ sudo ./qemu-nbd -C -c /dev/nbd0 ./image.qcow2
>>> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
>>> 10+0 records in
>>> 10+0 records out
>>> 104857600 bytes (105 MB) copied, 0,076046 s, 1,4 GB/s
>>> $ sudo ./qemu-nbd -d /dev/nbd0
>>> $ du -sh ./image.qcow2
>>> 5,3M    ./image.qcow2
> 
> That seems wrong to me.  Why not use qemu-img convert for this case?
> 
> Attaching an NBD server to a compressed disk has exactly the same
> problem as attaching a compressed disk to a VM.  It won’t work unless
> the client/guest is aware of the limitations.
> 
>>> The idea behind introducing the new 'compress' option is to use that
>>> only one instead of many other ones of such a kind.
>>>
>>> 2) You are right also that "Compression can't overwrite anything..."
>>> It can be seen in the commit message
>>> b0b6862e5e1a1394e0ab3d5da94ba8b0da8664e2
>>>
>>> I am not sure if data should be written compressed to the active layer.
>>> I made the tests with the idea of bringing examples of usage the
>>> 'compress' option because passing an option is a tricky thing in QEMU.
>>> But the issue takes place anyway if we want to rewrite to allocated
>>> clusters.
>>> I would like to investigate the matter and make a patch that resolves
>>> that issue.
>>> Do you agree with that?
> 
> What seems wrong to me is that this series just adds a generic compress
> option without ensuring that it works generically.
> 
> Either (1) it only works in well-defined cases, then either (1A) we have
> to ensure that we only allow it then (as we do now, because only
> qemu-img convert and the backup job have such an option; and those two
> are safe), or (1B) we have to clearly give a big warning for the new
> option that it doesn’t work correctly.  I don’t know whether such a
> warning is even possible with just a generic node-level option.
> 
> Or (2) we make it work in generic cases.  Well, that might be possible
> for qcow2, but who’s going to make it work for VMDK’s streamOptimized
> subformat?
> 
> More on all of that below.
> 
>> Yes, we want this option not to allow compressed writes for guests, but to
>> allow
>> - stream with compression (used to remove compressed incremental backup, we
>> need to merge it to the next incremental)
> 
> Based on the assumption that one shouldn’t attach a compressed disk to a
> VM, I don’t see how streaming makes sense then.  Well, I suppose
> intermediate streaming would work.
> 
>> - backup with compression (we have an optional already, so it works)
>> - backup to nbd server with compression: enable compression on nbd server
> 
> The problem is clearly that if a generic client were to connect to the
> NBD server, it wouldn’t work.  In this case, compression will work only
> if the clients understands the limitation.
> 
> (The safe way would be to make the NBD server provide an option that
> clients can see so they know this limitation and agree they’ll adhere to
> it.  It’s also a stupid way.)
> 
>> So instead of adding two options (for stream and for nbd), it seems better to
>> add only one for generic layer.
> 
> I don’t know.  It doesn’t work generically, so I don’t know whether it
> should be a generic option.
> 
>> Then, it becomes possible to run guest on image with compress=on. It's a side
>> effect, but still it should work correctly.
> 
> How so?  compress=on only works if every party involved only writes to
> any cluster of the image exactly once.  That is just not the case for
> guests unless they know this limitation, and even I don’t see a use case.
> 
>> I think the simplest thing is to just run normal write, if compressed write
>> failed because of reallocation. We should check that on that failure-path
>> ENOTSUP is returned and handle it for compress=on option by fallback to
>> normal write in generic block/io.c
> 
> It seems wrong to not compress data with compress=on.

We already fallback to normal write if can't compress in qcow2.

> 
>> (Note, that in case with stream we rewrite only unallocated clusters)
> 
> Yes, but if the stream job writes the cluster before the guest, the
> guest gets an I/O error.

I don't think that using compressed writes by guest is really usable thing.
In our case with stream there is no guest (just use qemu binary to operate
on block layer)

> 
> 
> By the way, the other thing I wondered was whether this should be a
> filter driver instead (if it makes sense at all).  Such a filter driver
> would at least be sufficiently cumbersome to use that probably only
> users who understand the implications would make use of it (1B above).
> 
> 
> I’m not against adding a generic compress=on option if we ensure that it
> actually works generically (2 above).  It doesn’t right now, so I don’t
> think this is right.
> 
> I’m already not sure whether it’s really possible to support generic
> compressed writes in qcow2.  I suppose it’d be at least awkward.  In
> theory it should work, it’s just that we can’t keep track of subcluster
> allocations, which in the worst case means that some compressed clusters
> take up a whole cluster of space.
> 
> For VMDK...  I suppose we could consider a new flag for block drivers
> that flags whether a driver supports arbitrary compressed writes or has
> the old limitations.  compress=on could then refuse to work on any block
> driver but the ones that support arbitrary compressed writes.
> 
> 
> Our current model (1A) is simply to ensure that all compressed writes
> can adhere to the limitations.  As I’ve said above, extending this to
> NBD would mean adding some NBD negotiation so both client and server
> agree on this limitation.

In this case, I'd prefere simple cmdline option fro qemu-nbd.

> That seems kind of reasonable from the qemu
> side, but probably very unreasonable from an NBD side.  Why would NBD
> bother to reproduce qemu’s limitations?
> 
> 
> So these are the three ways (1A, 1B, 2) I can imagine.  But just adding
> a generic option that unsuspecting users are not unlikely to use but
> that simply doesn’t work generically doesn’t seem right to me.
> 

Firstly, 1A already doesn't work correctly: we have compress option for backup.
So, it will not work if backup target is not empty.
So, 1A is impossible, as it's already broken, adding separate options for stream
and qemu-nbd is not better than just add one generic option.

I don't like (2) as it means a lot of effort to support actually not needed case.

I don't really like filter solution (as at seems too much to add a filter for simple
boolean option), but I think, we can live with it.

1B is OK for me, that is, just document what the option does in fact.

May be, name the option "compress-new-writes" instead of just "compress"?
And document, that writes to clusters, which was not written before will be
compressed?

Or make the option to be compress=<x>, where x may be 'no' or 'new-writes',
reserving 'all-writes' for future?


-- 
Best regards,
Vladimir

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

[Max Reitz]

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

On 22.10.19 14:23, Vladimir Sementsov-Ogievskiy wrote:
> 22.10.2019 14:31, Max Reitz wrote:
>> On 22.10.19 12:46, Vladimir Sementsov-Ogievskiy wrote:
>>> 22.10.2019 13:21, Andrey Shinkevich wrote:
>>>>
>>>> On 22/10/2019 12:28, Max Reitz wrote:
>>>>> On 20.10.19 22:37, 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>
>>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>> ---
>>>>>>     block.c                   | 20 +++++++++++++++++++-
>>>>>>     block/io.c                | 13 +++++++++----
>>>>>>     block/qcow2.c             |  4 ++++
>>>>>>     blockdev.c                |  9 ++++++++-
>>>>>>     include/block/block.h     |  1 +
>>>>>>     include/block/block_int.h |  2 ++
>>>>>>     qapi/block-core.json      |  5 ++++-
>>>>>>     qemu-options.hx           |  6 ++++--
>>>>>>     8 files changed, 51 insertions(+), 9 deletions(-)
>>>>>
>>>>> The problem with compression is that there are such tight constraints on
>>>>> it that it can really only work for very defined use cases.  Those
>>>>> constraints are:
>>>>>
>>>>> - Only write whole clusters,
>>>>> - Clusters can be written to only once.
>>>>>
>>>>> The first point is addressed in this patch by setting request_alignment.
>>>>>     But I don’t see how the second one can be addressed.  Well, maybe by
>>>>> allowing it in all drivers that support compression.  But if I just look
>>>>> at qcow2, that isn’t going to be trivial: You need to allocate a
>>>>> completely new cluster where you write the data (in case it grows), and
>>>>> thus you leave behind a hole, which kind of defeats the purpose of
>>>>> compression.
>>>>>
>>>>> (For demonstration:
>>>>>
>>>>> $ ./qemu-img create -f qcow2 test.qcow2 64M
>>>>> Formatting 'test.qcow2', fmt=qcow2 size=67108864 cluster_size=65536
>>>>> lazy_refcounts=off refcount_bits=16
>>>>> $ x86_64-softmmu/qemu-system-x86_64 \
>>>>>        -blockdev "{'node-name': 'drv0', 'driver': 'qcow2',
>>>>>                    'compress': true,
>>>>>                    'file': {'driver': 'file', 'filename': 'test.qcow2'}}" \
>>>>>        -monitor stdio
>>>>> QEMU 4.1.50 monitor - type 'help' for more information
>>>>> (qemu) qemu-io drv0 "write -P 42 0 64k"
>>>>> wrote 65536/65536 bytes at offset 0
>>>>> 64 KiB, 1 ops; 00.02 sec (4.055 MiB/sec and 64.8793 ops/sec)
>>>>> (qemu) qemu-io drv0 "write -P 23 0 64k"
>>>>> write failed: Input/output error
>>>>>
>>>>> )
>>>>>
>>>>> Compression really only works when you fully write all of an image
>>>>> exactly once; i.e. as the qemu-img convert or as a backup target.  For
>>>>> both cases we already have a compression option.  So I’m wondering where
>>>>> this new option is really useful.
>>>>>
>>>>> (You do add a test for stream, but I don’t know whether that’s really a
>>>>> good example, see my response there.)
>>>>>
>>>>> Max
>>>>>
>>>>
>>>> Thank you very much Max for your detailed response.
>>>>
>>>> 1) You are right that compression is used with the backup mostly. The
>>>> option for the compression with backup would be replaced by usage at the
>>>> block layer, with no duplication. Also, it can be useful for NBD for
>>>> instance,
>>>>
>>>> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
>>>> $ sudo ./qemu-nbd -c /dev/nbd0 ./image.qcow2
>>>> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
>>>> 10+0 records in
>>>> 10+0 records out
>>>> 104857600 bytes (105 MB) copied, 0,0890581 s, 1,2 GB/s
>>>> $ sudo ./qemu-nbd -d /dev/nbd0
>>>> $ du -sh ./image.qcow2
>>>> 101M    ./image.qcow2
>>>>
>>>> and with the compression
>>>>
>>>> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
>>>> $ sudo ./qemu-nbd -C -c /dev/nbd0 ./image.qcow2
>>>> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
>>>> 10+0 records in
>>>> 10+0 records out
>>>> 104857600 bytes (105 MB) copied, 0,076046 s, 1,4 GB/s
>>>> $ sudo ./qemu-nbd -d /dev/nbd0
>>>> $ du -sh ./image.qcow2
>>>> 5,3M    ./image.qcow2
>>
>> That seems wrong to me.  Why not use qemu-img convert for this case?
>>
>> Attaching an NBD server to a compressed disk has exactly the same
>> problem as attaching a compressed disk to a VM.  It won’t work unless
>> the client/guest is aware of the limitations.
>>
>>>> The idea behind introducing the new 'compress' option is to use that
>>>> only one instead of many other ones of such a kind.
>>>>
>>>> 2) You are right also that "Compression can't overwrite anything..."
>>>> It can be seen in the commit message
>>>> b0b6862e5e1a1394e0ab3d5da94ba8b0da8664e2
>>>>
>>>> I am not sure if data should be written compressed to the active layer.
>>>> I made the tests with the idea of bringing examples of usage the
>>>> 'compress' option because passing an option is a tricky thing in QEMU.
>>>> But the issue takes place anyway if we want to rewrite to allocated
>>>> clusters.
>>>> I would like to investigate the matter and make a patch that resolves
>>>> that issue.
>>>> Do you agree with that?
>>
>> What seems wrong to me is that this series just adds a generic compress
>> option without ensuring that it works generically.
>>
>> Either (1) it only works in well-defined cases, then either (1A) we have
>> to ensure that we only allow it then (as we do now, because only
>> qemu-img convert and the backup job have such an option; and those two
>> are safe), or (1B) we have to clearly give a big warning for the new
>> option that it doesn’t work correctly.  I don’t know whether such a
>> warning is even possible with just a generic node-level option.
>>
>> Or (2) we make it work in generic cases.  Well, that might be possible
>> for qcow2, but who’s going to make it work for VMDK’s streamOptimized
>> subformat?
>>
>> More on all of that below.
>>
>>> Yes, we want this option not to allow compressed writes for guests, but to
>>> allow
>>> - stream with compression (used to remove compressed incremental backup, we
>>> need to merge it to the next incremental)
>>
>> Based on the assumption that one shouldn’t attach a compressed disk to a
>> VM, I don’t see how streaming makes sense then.  Well, I suppose
>> intermediate streaming would work.
>>
>>> - backup with compression (we have an optional already, so it works)
>>> - backup to nbd server with compression: enable compression on nbd server
>>
>> The problem is clearly that if a generic client were to connect to the
>> NBD server, it wouldn’t work.  In this case, compression will work only
>> if the clients understands the limitation.
>>
>> (The safe way would be to make the NBD server provide an option that
>> clients can see so they know this limitation and agree they’ll adhere to
>> it.  It’s also a stupid way.)
>>
>>> So instead of adding two options (for stream and for nbd), it seems better to
>>> add only one for generic layer.
>>
>> I don’t know.  It doesn’t work generically, so I don’t know whether it
>> should be a generic option.
>>
>>> Then, it becomes possible to run guest on image with compress=on. It's a side
>>> effect, but still it should work correctly.
>>
>> How so?  compress=on only works if every party involved only writes to
>> any cluster of the image exactly once.  That is just not the case for
>> guests unless they know this limitation, and even I don’t see a use case.
>>
>>> I think the simplest thing is to just run normal write, if compressed write
>>> failed because of reallocation. We should check that on that failure-path
>>> ENOTSUP is returned and handle it for compress=on option by fallback to
>>> normal write in generic block/io.c
>>
>> It seems wrong to not compress data with compress=on.
> 
> We already fallback to normal write if can't compress in qcow2.

In a very specific case, namely where the compressed size is larger than
the uncompressed size.  It would be a whole different story to only
compress the first write to a given cluster but not any following one.

>>> (Note, that in case with stream we rewrite only unallocated clusters)
>>
>> Yes, but if the stream job writes the cluster before the guest, the
>> guest gets an I/O error.
> 
> I don't think that using compressed writes by guest is really usable thing.
> In our case with stream there is no guest (just use qemu binary to operate
> on block layer)
> 
>>
>>
>> By the way, the other thing I wondered was whether this should be a
>> filter driver instead (if it makes sense at all).  Such a filter driver
>> would at least be sufficiently cumbersome to use that probably only
>> users who understand the implications would make use of it (1B above).
>>
>>
>> I’m not against adding a generic compress=on option if we ensure that it
>> actually works generically (2 above).  It doesn’t right now, so I don’t
>> think this is right.
>>
>> I’m already not sure whether it’s really possible to support generic
>> compressed writes in qcow2.  I suppose it’d be at least awkward.  In
>> theory it should work, it’s just that we can’t keep track of subcluster
>> allocations, which in the worst case means that some compressed clusters
>> take up a whole cluster of space.
>>
>> For VMDK...  I suppose we could consider a new flag for block drivers
>> that flags whether a driver supports arbitrary compressed writes or has
>> the old limitations.  compress=on could then refuse to work on any block
>> driver but the ones that support arbitrary compressed writes.
>>
>>
>> Our current model (1A) is simply to ensure that all compressed writes
>> can adhere to the limitations.  As I’ve said above, extending this to
>> NBD would mean adding some NBD negotiation so both client and server
>> agree on this limitation.
> 
> In this case, I'd prefere simple cmdline option fro qemu-nbd.
> 
>> That seems kind of reasonable from the qemu
>> side, but probably very unreasonable from an NBD side.  Why would NBD
>> bother to reproduce qemu’s limitations?
>>
>>
>> So these are the three ways (1A, 1B, 2) I can imagine.  But just adding
>> a generic option that unsuspecting users are not unlikely to use but
>> that simply doesn’t work generically doesn’t seem right to me.
>>
> 
> Firstly, 1A already doesn't work correctly: we have compress option for backup.
> So, it will not work if backup target is not empty.

I’m not sure whether that qualifies because the user is simply
responsible to ensure that the target is empty.

Otherwise, you could also argue that backup doesn’t necessarily create a
real backup because with sync=top it won’t copy unallocated clusters, so
if the target already has data in such a place, it won’t be overwritten.

Furthermore I’d argue this would hardly be an accident.  Both
drive-backup with mode=existing and qemu-img convert -n are most likely
not used blindly but only when the situation requires it.

My point is that using compress=on can be an accident because people see
that option and think “That sounds useful!” without reading the warning.

> So, 1A is impossible, as it's already broken, adding separate options for stream
> and qemu-nbd is not better than just add one generic option.

I disagree that 1A is broken, and I disagree a generic option would not
be worse.  I still believe a simple generic option is prone to
accidental misuse.

> I don't like (2) as it means a lot of effort to support actually not needed case.

Well, you say it isn’t needed.  I suppose if it were supported, people
would indeed use it.

> I don't really like filter solution (as at seems too much to add a filter for simple
> boolean option), but I think, we can live with it.

First, it is not a simple boolean option.  This patch brings
implementation with it that converts writes to compressed writes and
sets a request alignment.  I actually think it’s better to do things
like that in a dedicated block driver than in the generic block layer code.

Second, we already this basically this for copy-on-read (which is also a
boolean option for -drive).

(FWIW, I’d prefer if detect-zeroes were a block driver instead of
generic block layer code.)

> 1B is OK for me, that is, just document what the option does in fact.
> 
> May be, name the option "compress-new-writes" instead of just "compress"?
> And document, that writes to clusters, which was not written before will be
> compressed?
> 
> Or make the option to be compress=<x>, where x may be 'no' or 'new-writes',
> reserving 'all-writes' for future?

I don’t know whether the limitations can be captured in three words, but
it would at least make users pay closer attention to what the option is
really doing.

OTOH, the only downsides to a dedicated block driver to me appear that
it’s more cumbersome to use (which I actually consider a benefit) and
that it requires the usual filter driver boilerplate – but that can be
copied from other drivers[1].

(The benefits are that we don’t need to modify generic code and that
people need to read into the filter and its caveats before they can use it.)

Max


[1] Actually, we could provide default implementations somewhere and
make block filters use them.  That should reduce the boilerplate size.


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

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

[Andrey Shinkevich]

On 22/10/2019 15:56, Max Reitz wrote:
> On 22.10.19 14:23, Vladimir Sementsov-Ogievskiy wrote:
>> 22.10.2019 14:31, Max Reitz wrote:
>>> On 22.10.19 12:46, Vladimir Sementsov-Ogievskiy wrote:
>>>> 22.10.2019 13:21, Andrey Shinkevich wrote:
>>>>>
>>>>> On 22/10/2019 12:28, Max Reitz wrote:
>>>>>> On 20.10.19 22:37, 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>
>>>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>> ---
>>>>>>>      block.c                   | 20 +++++++++++++++++++-
>>>>>>>      block/io.c                | 13 +++++++++----
>>>>>>>      block/qcow2.c             |  4 ++++
>>>>>>>      blockdev.c                |  9 ++++++++-
>>>>>>>      include/block/block.h     |  1 +
>>>>>>>      include/block/block_int.h |  2 ++
>>>>>>>      qapi/block-core.json      |  5 ++++-
>>>>>>>      qemu-options.hx           |  6 ++++--
>>>>>>>      8 files changed, 51 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> The problem with compression is that there are such tight constraints on
>>>>>> it that it can really only work for very defined use cases.  Those
>>>>>> constraints are:
>>>>>>
>>>>>> - Only write whole clusters,
>>>>>> - Clusters can be written to only once.
>>>>>>
>>>>>> The first point is addressed in this patch by setting request_alignment.
>>>>>>      But I don’t see how the second one can be addressed.  Well, maybe by
>>>>>> allowing it in all drivers that support compression.  But if I just look
>>>>>> at qcow2, that isn’t going to be trivial: You need to allocate a
>>>>>> completely new cluster where you write the data (in case it grows), and
>>>>>> thus you leave behind a hole, which kind of defeats the purpose of
>>>>>> compression.
>>>>>>
>>>>>> (For demonstration:
>>>>>>
>>>>>> $ ./qemu-img create -f qcow2 test.qcow2 64M
>>>>>> Formatting 'test.qcow2', fmt=qcow2 size=67108864 cluster_size=65536
>>>>>> lazy_refcounts=off refcount_bits=16
>>>>>> $ x86_64-softmmu/qemu-system-x86_64 \
>>>>>>         -blockdev "{'node-name': 'drv0', 'driver': 'qcow2',
>>>>>>                     'compress': true,
>>>>>>                     'file': {'driver': 'file', 'filename': 'test.qcow2'}}" \
>>>>>>         -monitor stdio
>>>>>> QEMU 4.1.50 monitor - type 'help' for more information
>>>>>> (qemu) qemu-io drv0 "write -P 42 0 64k"
>>>>>> wrote 65536/65536 bytes at offset 0
>>>>>> 64 KiB, 1 ops; 00.02 sec (4.055 MiB/sec and 64.8793 ops/sec)
>>>>>> (qemu) qemu-io drv0 "write -P 23 0 64k"
>>>>>> write failed: Input/output error
>>>>>>
>>>>>> )
>>>>>>
>>>>>> Compression really only works when you fully write all of an image
>>>>>> exactly once; i.e. as the qemu-img convert or as a backup target.  For
>>>>>> both cases we already have a compression option.  So I’m wondering where
>>>>>> this new option is really useful.
>>>>>>
>>>>>> (You do add a test for stream, but I don’t know whether that’s really a
>>>>>> good example, see my response there.)
>>>>>>
>>>>>> Max
>>>>>>
>>>>>
>>>>> Thank you very much Max for your detailed response.
>>>>>
>>>>> 1) You are right that compression is used with the backup mostly. The
>>>>> option for the compression with backup would be replaced by usage at the
>>>>> block layer, with no duplication. Also, it can be useful for NBD for
>>>>> instance,
>>>>>
>>>>> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
>>>>> $ sudo ./qemu-nbd -c /dev/nbd0 ./image.qcow2
>>>>> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
>>>>> 10+0 records in
>>>>> 10+0 records out
>>>>> 104857600 bytes (105 MB) copied, 0,0890581 s, 1,2 GB/s
>>>>> $ sudo ./qemu-nbd -d /dev/nbd0
>>>>> $ du -sh ./image.qcow2
>>>>> 101M    ./image.qcow2
>>>>>
>>>>> and with the compression
>>>>>
>>>>> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
>>>>> $ sudo ./qemu-nbd -C -c /dev/nbd0 ./image.qcow2
>>>>> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
>>>>> 10+0 records in
>>>>> 10+0 records out
>>>>> 104857600 bytes (105 MB) copied, 0,076046 s, 1,4 GB/s
>>>>> $ sudo ./qemu-nbd -d /dev/nbd0
>>>>> $ du -sh ./image.qcow2
>>>>> 5,3M    ./image.qcow2
>>>
>>> That seems wrong to me.  Why not use qemu-img convert for this case?
>>>
>>> Attaching an NBD server to a compressed disk has exactly the same
>>> problem as attaching a compressed disk to a VM.  It won’t work unless
>>> the client/guest is aware of the limitations.
>>>
>>>>> The idea behind introducing the new 'compress' option is to use that
>>>>> only one instead of many other ones of such a kind.
>>>>>
>>>>> 2) You are right also that "Compression can't overwrite anything..."
>>>>> It can be seen in the commit message
>>>>> b0b6862e5e1a1394e0ab3d5da94ba8b0da8664e2
>>>>>
>>>>> I am not sure if data should be written compressed to the active layer.
>>>>> I made the tests with the idea of bringing examples of usage the
>>>>> 'compress' option because passing an option is a tricky thing in QEMU.
>>>>> But the issue takes place anyway if we want to rewrite to allocated
>>>>> clusters.
>>>>> I would like to investigate the matter and make a patch that resolves
>>>>> that issue.
>>>>> Do you agree with that?
>>>
>>> What seems wrong to me is that this series just adds a generic compress
>>> option without ensuring that it works generically.
>>>
>>> Either (1) it only works in well-defined cases, then either (1A) we have
>>> to ensure that we only allow it then (as we do now, because only
>>> qemu-img convert and the backup job have such an option; and those two
>>> are safe), or (1B) we have to clearly give a big warning for the new
>>> option that it doesn’t work correctly.  I don’t know whether such a
>>> warning is even possible with just a generic node-level option.
>>>
>>> Or (2) we make it work in generic cases.  Well, that might be possible
>>> for qcow2, but who’s going to make it work for VMDK’s streamOptimized
>>> subformat?
>>>
>>> More on all of that below.
>>>
>>>> Yes, we want this option not to allow compressed writes for guests, but to
>>>> allow
>>>> - stream with compression (used to remove compressed incremental backup, we
>>>> need to merge it to the next incremental)
>>>
>>> Based on the assumption that one shouldn’t attach a compressed disk to a
>>> VM, I don’t see how streaming makes sense then.  Well, I suppose
>>> intermediate streaming would work.
>>>
>>>> - backup with compression (we have an optional already, so it works)
>>>> - backup to nbd server with compression: enable compression on nbd server
>>>
>>> The problem is clearly that if a generic client were to connect to the
>>> NBD server, it wouldn’t work.  In this case, compression will work only
>>> if the clients understands the limitation.
>>>
>>> (The safe way would be to make the NBD server provide an option that
>>> clients can see so they know this limitation and agree they’ll adhere to
>>> it.  It’s also a stupid way.)
>>>
>>>> So instead of adding two options (for stream and for nbd), it seems better to
>>>> add only one for generic layer.
>>>
>>> I don’t know.  It doesn’t work generically, so I don’t know whether it
>>> should be a generic option.
>>>
>>>> Then, it becomes possible to run guest on image with compress=on. It's a side
>>>> effect, but still it should work correctly.
>>>
>>> How so?  compress=on only works if every party involved only writes to
>>> any cluster of the image exactly once.  That is just not the case for
>>> guests unless they know this limitation, and even I don’t see a use case.
>>>
>>>> I think the simplest thing is to just run normal write, if compressed write
>>>> failed because of reallocation. We should check that on that failure-path
>>>> ENOTSUP is returned and handle it for compress=on option by fallback to
>>>> normal write in generic block/io.c
>>>
>>> It seems wrong to not compress data with compress=on.
>>
>> We already fallback to normal write if can't compress in qcow2.
> 
> In a very specific case, namely where the compressed size is larger than
> the uncompressed size.  It would be a whole different story to only
> compress the first write to a given cluster but not any following one.
> 
>>>> (Note, that in case with stream we rewrite only unallocated clusters)
>>>
>>> Yes, but if the stream job writes the cluster before the guest, the
>>> guest gets an I/O error.
>>
>> I don't think that using compressed writes by guest is really usable thing.
>> In our case with stream there is no guest (just use qemu binary to operate
>> on block layer)
>>
>>>
>>>
>>> By the way, the other thing I wondered was whether this should be a
>>> filter driver instead (if it makes sense at all).  Such a filter driver
>>> would at least be sufficiently cumbersome to use that probably only
>>> users who understand the implications would make use of it (1B above).
>>>
>>>
>>> I’m not against adding a generic compress=on option if we ensure that it
>>> actually works generically (2 above).  It doesn’t right now, so I don’t
>>> think this is right.
>>>
>>> I’m already not sure whether it’s really possible to support generic
>>> compressed writes in qcow2.  I suppose it’d be at least awkward.  In
>>> theory it should work, it’s just that we can’t keep track of subcluster
>>> allocations, which in the worst case means that some compressed clusters
>>> take up a whole cluster of space.
>>>
>>> For VMDK...  I suppose we could consider a new flag for block drivers
>>> that flags whether a driver supports arbitrary compressed writes or has
>>> the old limitations.  compress=on could then refuse to work on any block
>>> driver but the ones that support arbitrary compressed writes.
>>>
>>>
>>> Our current model (1A) is simply to ensure that all compressed writes
>>> can adhere to the limitations.  As I’ve said above, extending this to
>>> NBD would mean adding some NBD negotiation so both client and server
>>> agree on this limitation.
>>
>> In this case, I'd prefere simple cmdline option fro qemu-nbd.
>>
>>> That seems kind of reasonable from the qemu
>>> side, but probably very unreasonable from an NBD side.  Why would NBD
>>> bother to reproduce qemu’s limitations?
>>>
>>>
>>> So these are the three ways (1A, 1B, 2) I can imagine.  But just adding
>>> a generic option that unsuspecting users are not unlikely to use but
>>> that simply doesn’t work generically doesn’t seem right to me.
>>>
>>
>> Firstly, 1A already doesn't work correctly: we have compress option for backup.
>> So, it will not work if backup target is not empty.
> 
> I’m not sure whether that qualifies because the user is simply
> responsible to ensure that the target is empty.
> 
> Otherwise, you could also argue that backup doesn’t necessarily create a
> real backup because with sync=top it won’t copy unallocated clusters, so
> if the target already has data in such a place, it won’t be overwritten.
> 
> Furthermore I’d argue this would hardly be an accident.  Both
> drive-backup with mode=existing and qemu-img convert -n are most likely
> not used blindly but only when the situation requires it.
> 
> My point is that using compress=on can be an accident because people see
> that option and think “That sounds useful!” without reading the warning.
> 
>> So, 1A is impossible, as it's already broken, adding separate options for stream
>> and qemu-nbd is not better than just add one generic option.
> 
> I disagree that 1A is broken, and I disagree a generic option would not
> be worse.  I still believe a simple generic option is prone to
> accidental misuse.
> 
>> I don't like (2) as it means a lot of effort to support actually not needed case.
> 
> Well, you say it isn’t needed.  I suppose if it were supported, people
> would indeed use it.
> 
>> I don't really like filter solution (as at seems too much to add a filter for simple
>> boolean option), but I think, we can live with it.
> 
> First, it is not a simple boolean option.  This patch brings
> implementation with it that converts writes to compressed writes and
> sets a request alignment.  I actually think it’s better to do things
> like that in a dedicated block driver than in the generic block layer code.
> 
> Second, we already this basically this for copy-on-read (which is also a
> boolean option for -drive).
> 
> (FWIW, I’d prefer if detect-zeroes were a block driver instead of
> generic block layer code.)
> 
>> 1B is OK for me, that is, just document what the option does in fact.
>>
>> May be, name the option "compress-new-writes" instead of just "compress"?
>> And document, that writes to clusters, which was not written before will be
>> compressed?
>>
>> Or make the option to be compress=<x>, where x may be 'no' or 'new-writes',
>> reserving 'all-writes' for future?
> 
> I don’t know whether the limitations can be captured in three words, but
> it would at least make users pay closer attention to what the option is
> really doing.
> 
> OTOH, the only downsides to a dedicated block driver to me appear that
> it’s more cumbersome to use (which I actually consider a benefit) and
> that it requires the usual filter driver boilerplate – but that can be
> copied from other drivers[1].
> 
> (The benefits are that we don’t need to modify generic code and that
> people need to read into the filter and its caveats before they can use it.)
> 
> Max
> 
> 
> [1] Actually, we could provide default implementations somewhere and
> make block filters use them.  That should reduce the boilerplate size.
> 

If the support of COW for compressed writes is found feasible, will it 
make a sense to implement? Then this series will follow.
A comment for the option would warn a user that guest writing will work 
slower for if compress selected.

Andrey
-- 
With the best regards,
Andrey Shinkevich

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

[Max Reitz]

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

On 22.10.19 15:53, Andrey Shinkevich wrote:

[...]

> If the support of COW for compressed writes is found feasible, will it 
> make a sense to implement? Then this series will follow.

Hm, what exactly do you mean by support of COW for compressed writes?

> A comment for the option would warn a user that guest writing will work 
> slower for if compress selected.

I’m not sure whether that’s necessary.  As a user, I would assume that
enabling compression will always make something slower.

Max


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

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

[Andrey Shinkevich]

On 24/10/2019 12:34, Max Reitz wrote:
> On 22.10.19 15:53, Andrey Shinkevich wrote:
> 
> [...]
> 
>> If the support of COW for compressed writes is found feasible, will it
>> make a sense to implement? Then this series will follow.
> 
> Hm, what exactly do you mean by support of COW for compressed writes?
> 

I spoke in terms of the commit message with the following ID:

b0b6862e5e1a1394e0ab3d5da94ba8b0da8664e2

"qcow2: Fail write_compressed when overwriting data"

"...qcow2_write_compressed() doesn't perform COW as it would have to do..."

So, I suggest that we implement writing compressed data to the allocated 
clusters rather than qcow2_alloc_compressed_cluster_offset() returns the 
error. Particularly, when it comes to NBD server connection failure for 
writhing a compressed cluster, it may not be rewritten after the 
connection is restored.
Are there any issues with that implementation idea?

Andrey

>> A comment for the option would warn a user that guest writing will work
>> slower for if compress selected.
> 
> I’m not sure whether that’s necessary.  As a user, I would assume that
> enabling compression will always make something slower.
> 
> Max
> 

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

[Max Reitz]

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

On 24.10.19 14:56, Andrey Shinkevich wrote:
> 
> 
> On 24/10/2019 12:34, Max Reitz wrote:
>> On 22.10.19 15:53, Andrey Shinkevich wrote:
>>
>> [...]
>>
>>> If the support of COW for compressed writes is found feasible, will it
>>> make a sense to implement? Then this series will follow.
>>
>> Hm, what exactly do you mean by support of COW for compressed writes?
>>
> 
> I spoke in terms of the commit message with the following ID:
> 
> b0b6862e5e1a1394e0ab3d5da94ba8b0da8664e2
> 
> "qcow2: Fail write_compressed when overwriting data"
> 
> "...qcow2_write_compressed() doesn't perform COW as it would have to do..."
> 
> So, I suggest that we implement writing compressed data to the allocated 
> clusters rather than qcow2_alloc_compressed_cluster_offset() returns the 
> error. Particularly, when it comes to NBD server connection failure for 
> writhing a compressed cluster, it may not be rewritten after the 
> connection is restored.
> Are there any issues with that implementation idea?

Well, the COW in that commit is meant differently, because it refers to
the COW that’s required when writing to a cluster shared by an internal
snapshot.

OTOH, you could say that all compressed writes to a cluster that is
already allocated would need to do COW because we’d always have to fully
rewrite that cluster in an RMW cycle.

I don’t see how letting qcow2_alloc_compressed_cluster_offset() use the
existing cluster would solve the problem, though.  You’d generally need
to allocate a new cluster; or attempt to reuse the existing space in a
compressed cluster.

Max


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

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

[Andrey Shinkevich]

On 24/10/2019 16:48, Max Reitz wrote:
> On 24.10.19 14:56, Andrey Shinkevich wrote:
>>
>>
>> On 24/10/2019 12:34, Max Reitz wrote:
>>> On 22.10.19 15:53, Andrey Shinkevich wrote:
>>>
>>> [...]
>>>
>>>> If the support of COW for compressed writes is found feasible, will it
>>>> make a sense to implement? Then this series will follow.
>>>
>>> Hm, what exactly do you mean by support of COW for compressed writes?
>>>
>>
>> I spoke in terms of the commit message with the following ID:
>>
>> b0b6862e5e1a1394e0ab3d5da94ba8b0da8664e2
>>
>> "qcow2: Fail write_compressed when overwriting data"
>>
>> "...qcow2_write_compressed() doesn't perform COW as it would have to do..."
>>
>> So, I suggest that we implement writing compressed data to the allocated
>> clusters rather than qcow2_alloc_compressed_cluster_offset() returns the
>> error. Particularly, when it comes to NBD server connection failure for
>> writhing a compressed cluster, it may not be rewritten after the
>> connection is restored.
>> Are there any issues with that implementation idea?
> 
> Well, the COW in that commit is meant differently, because it refers to
> the COW that’s required when writing to a cluster shared by an internal
> snapshot.
> 
> OTOH, you could say that all compressed writes to a cluster that is
> already allocated would need to do COW because we’d always have to fully
> rewrite that cluster in an RMW cycle.
> 
> I don’t see how letting qcow2_alloc_compressed_cluster_offset() use the
> existing cluster would solve the problem, though.  You’d generally need
> to allocate a new cluster; or attempt to reuse the existing space in a
> compressed cluster.
> 
> Max
> 

Yes, new clusters would be allocated for the compressed RMW and the 
existing ones would be reused if possible. It seams to be ineffective 
but users are supposed to know what they do.
So, does it worth to check a feasibility of the implementation?

Andrey

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

[Max Reitz]

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

On 24.10.19 16:07, Andrey Shinkevich wrote:
> 
> 
> On 24/10/2019 16:48, Max Reitz wrote:
>> On 24.10.19 14:56, Andrey Shinkevich wrote:
>>>
>>>
>>> On 24/10/2019 12:34, Max Reitz wrote:
>>>> On 22.10.19 15:53, Andrey Shinkevich wrote:
>>>>
>>>> [...]
>>>>
>>>>> If the support of COW for compressed writes is found feasible, will it
>>>>> make a sense to implement? Then this series will follow.
>>>>
>>>> Hm, what exactly do you mean by support of COW for compressed writes?
>>>>
>>>
>>> I spoke in terms of the commit message with the following ID:
>>>
>>> b0b6862e5e1a1394e0ab3d5da94ba8b0da8664e2
>>>
>>> "qcow2: Fail write_compressed when overwriting data"
>>>
>>> "...qcow2_write_compressed() doesn't perform COW as it would have to do..."
>>>
>>> So, I suggest that we implement writing compressed data to the allocated
>>> clusters rather than qcow2_alloc_compressed_cluster_offset() returns the
>>> error. Particularly, when it comes to NBD server connection failure for
>>> writhing a compressed cluster, it may not be rewritten after the
>>> connection is restored.
>>> Are there any issues with that implementation idea?
>>
>> Well, the COW in that commit is meant differently, because it refers to
>> the COW that’s required when writing to a cluster shared by an internal
>> snapshot.
>>
>> OTOH, you could say that all compressed writes to a cluster that is
>> already allocated would need to do COW because we’d always have to fully
>> rewrite that cluster in an RMW cycle.
>>
>> I don’t see how letting qcow2_alloc_compressed_cluster_offset() use the
>> existing cluster would solve the problem, though.  You’d generally need
>> to allocate a new cluster; or attempt to reuse the existing space in a
>> compressed cluster.
>>
>> Max
>>
> 
> Yes, new clusters would be allocated for the compressed RMW and the 
> existing ones would be reused if possible. It seams to be ineffective 
> but users are supposed to know what they do.
> So, does it worth to check a feasibility of the implementation?

I don’t know, Vladimir said that use case wouldn’t be needed.

I think if the option was there, people would actually use it.  But I
doubt that anyone misses it sufficiently to warrant the effort.

In addition, there’s still VMDK to consider, too.

Max


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

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

[Andrey Shinkevich]

On 24/10/2019 16:48, Max Reitz wrote:
> On 24.10.19 14:56, Andrey Shinkevich wrote:
>>
>>
>> On 24/10/2019 12:34, Max Reitz wrote:
>>> On 22.10.19 15:53, Andrey Shinkevich wrote:
>>>
>>> [...]
>>>
>>>> If the support of COW for compressed writes is found feasible, will it
>>>> make a sense to implement? Then this series will follow.
>>>
>>> Hm, what exactly do you mean by support of COW for compressed writes?
>>>
>>
>> I spoke in terms of the commit message with the following ID:
>>
>> b0b6862e5e1a1394e0ab3d5da94ba8b0da8664e2
>>
>> "qcow2: Fail write_compressed when overwriting data"
>>
>> "...qcow2_write_compressed() doesn't perform COW as it would have to do..."
>>
>> So, I suggest that we implement writing compressed data to the allocated
>> clusters rather than qcow2_alloc_compressed_cluster_offset() returns the
>> error. Particularly, when it comes to NBD server connection failure for
>> writhing a compressed cluster, it may not be rewritten after the
>> connection is restored.
>> Are there any issues with that implementation idea?
> 
> Well, the COW in that commit is meant differently, because it refers to
> the COW that’s required when writing to a cluster shared by an internal
> snapshot.
> 
> OTOH, you could say that all compressed writes to a cluster that is
> already allocated would need to do COW because we’d always have to fully
> rewrite that cluster in an RMW cycle.
> 
> I don’t see how letting qcow2_alloc_compressed_cluster_offset() use the
> existing cluster would solve the problem, though.  You’d generally need
> to allocate a new cluster; or attempt to reuse the existing space in a
> compressed cluster.
> 
> Max
> 

The idea was to avoid the following error:

$ ./qemu-img create -f qcow2 -o size=10M ./image.qcow2
Formatting './image.qcow2', fmt=qcow2 size=10485760 cluster_size=65536 
lazy_refcounts=off refcount_bits=16
[andrey@dhcp-172-16-25-136 qemu]$ git diff --name-only
nbd/server.c
$ ./qemu-io -c "write -P 0x12 0 64k" ./image.qcow2
wrote 65536/65536 bytes at offset 0
64 KiB, 1 ops; 0.0152 sec (4.101 MiB/sec and 65.6082 ops/sec)
$ ./qemu-io -c "write -c -P 0x12 0 64k" ./image.qcow2
write failed: Input/output error

If it incurs a high cost, we can choose the filter option as discussed 
above.

Andrey

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

[Vladimir Sementsov-Ogievskiy]

22.10.2019 15:56, Max Reitz wrote:
> On 22.10.19 14:23, Vladimir Sementsov-Ogievskiy wrote:
>> 22.10.2019 14:31, Max Reitz wrote:
>>> On 22.10.19 12:46, Vladimir Sementsov-Ogievskiy wrote:
>>>> 22.10.2019 13:21, Andrey Shinkevich wrote:
>>>>>
>>>>> On 22/10/2019 12:28, Max Reitz wrote:
>>>>>> On 20.10.19 22:37, 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>
>>>>>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>>> ---
>>>>>>>      block.c                   | 20 +++++++++++++++++++-
>>>>>>>      block/io.c                | 13 +++++++++----
>>>>>>>      block/qcow2.c             |  4 ++++
>>>>>>>      blockdev.c                |  9 ++++++++-
>>>>>>>      include/block/block.h     |  1 +
>>>>>>>      include/block/block_int.h |  2 ++
>>>>>>>      qapi/block-core.json      |  5 ++++-
>>>>>>>      qemu-options.hx           |  6 ++++--
>>>>>>>      8 files changed, 51 insertions(+), 9 deletions(-)
>>>>>>
>>>>>> The problem with compression is that there are such tight constraints on
>>>>>> it that it can really only work for very defined use cases.  Those
>>>>>> constraints are:
>>>>>>
>>>>>> - Only write whole clusters,
>>>>>> - Clusters can be written to only once.
>>>>>>
>>>>>> The first point is addressed in this patch by setting request_alignment.
>>>>>>      But I don’t see how the second one can be addressed.  Well, maybe by
>>>>>> allowing it in all drivers that support compression.  But if I just look
>>>>>> at qcow2, that isn’t going to be trivial: You need to allocate a
>>>>>> completely new cluster where you write the data (in case it grows), and
>>>>>> thus you leave behind a hole, which kind of defeats the purpose of
>>>>>> compression.
>>>>>>
>>>>>> (For demonstration:
>>>>>>
>>>>>> $ ./qemu-img create -f qcow2 test.qcow2 64M
>>>>>> Formatting 'test.qcow2', fmt=qcow2 size=67108864 cluster_size=65536
>>>>>> lazy_refcounts=off refcount_bits=16
>>>>>> $ x86_64-softmmu/qemu-system-x86_64 \
>>>>>>         -blockdev "{'node-name': 'drv0', 'driver': 'qcow2',
>>>>>>                     'compress': true,
>>>>>>                     'file': {'driver': 'file', 'filename': 'test.qcow2'}}" \
>>>>>>         -monitor stdio
>>>>>> QEMU 4.1.50 monitor - type 'help' for more information
>>>>>> (qemu) qemu-io drv0 "write -P 42 0 64k"
>>>>>> wrote 65536/65536 bytes at offset 0
>>>>>> 64 KiB, 1 ops; 00.02 sec (4.055 MiB/sec and 64.8793 ops/sec)
>>>>>> (qemu) qemu-io drv0 "write -P 23 0 64k"
>>>>>> write failed: Input/output error
>>>>>>
>>>>>> )
>>>>>>
>>>>>> Compression really only works when you fully write all of an image
>>>>>> exactly once; i.e. as the qemu-img convert or as a backup target.  For
>>>>>> both cases we already have a compression option.  So I’m wondering where
>>>>>> this new option is really useful.
>>>>>>
>>>>>> (You do add a test for stream, but I don’t know whether that’s really a
>>>>>> good example, see my response there.)
>>>>>>
>>>>>> Max
>>>>>>
>>>>>
>>>>> Thank you very much Max for your detailed response.
>>>>>
>>>>> 1) You are right that compression is used with the backup mostly. The
>>>>> option for the compression with backup would be replaced by usage at the
>>>>> block layer, with no duplication. Also, it can be useful for NBD for
>>>>> instance,
>>>>>
>>>>> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
>>>>> $ sudo ./qemu-nbd -c /dev/nbd0 ./image.qcow2
>>>>> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
>>>>> 10+0 records in
>>>>> 10+0 records out
>>>>> 104857600 bytes (105 MB) copied, 0,0890581 s, 1,2 GB/s
>>>>> $ sudo ./qemu-nbd -d /dev/nbd0
>>>>> $ du -sh ./image.qcow2
>>>>> 101M    ./image.qcow2
>>>>>
>>>>> and with the compression
>>>>>
>>>>> $ ./qemu-img create -f qcow2 -o size=10G ./image.qcow2
>>>>> $ sudo ./qemu-nbd -C -c /dev/nbd0 ./image.qcow2
>>>>> $ sudo dd if=/dev/sda1 of=/dev/nbd0 bs=10M count=10
>>>>> 10+0 records in
>>>>> 10+0 records out
>>>>> 104857600 bytes (105 MB) copied, 0,076046 s, 1,4 GB/s
>>>>> $ sudo ./qemu-nbd -d /dev/nbd0
>>>>> $ du -sh ./image.qcow2
>>>>> 5,3M    ./image.qcow2
>>>
>>> That seems wrong to me.  Why not use qemu-img convert for this case?
>>>
>>> Attaching an NBD server to a compressed disk has exactly the same
>>> problem as attaching a compressed disk to a VM.  It won’t work unless
>>> the client/guest is aware of the limitations.
>>>
>>>>> The idea behind introducing the new 'compress' option is to use that
>>>>> only one instead of many other ones of such a kind.
>>>>>
>>>>> 2) You are right also that "Compression can't overwrite anything..."
>>>>> It can be seen in the commit message
>>>>> b0b6862e5e1a1394e0ab3d5da94ba8b0da8664e2
>>>>>
>>>>> I am not sure if data should be written compressed to the active layer.
>>>>> I made the tests with the idea of bringing examples of usage the
>>>>> 'compress' option because passing an option is a tricky thing in QEMU.
>>>>> But the issue takes place anyway if we want to rewrite to allocated
>>>>> clusters.
>>>>> I would like to investigate the matter and make a patch that resolves
>>>>> that issue.
>>>>> Do you agree with that?
>>>
>>> What seems wrong to me is that this series just adds a generic compress
>>> option without ensuring that it works generically.
>>>
>>> Either (1) it only works in well-defined cases, then either (1A) we have
>>> to ensure that we only allow it then (as we do now, because only
>>> qemu-img convert and the backup job have such an option; and those two
>>> are safe), or (1B) we have to clearly give a big warning for the new
>>> option that it doesn’t work correctly.  I don’t know whether such a
>>> warning is even possible with just a generic node-level option.
>>>
>>> Or (2) we make it work in generic cases.  Well, that might be possible
>>> for qcow2, but who’s going to make it work for VMDK’s streamOptimized
>>> subformat?
>>>
>>> More on all of that below.
>>>
>>>> Yes, we want this option not to allow compressed writes for guests, but to
>>>> allow
>>>> - stream with compression (used to remove compressed incremental backup, we
>>>> need to merge it to the next incremental)
>>>
>>> Based on the assumption that one shouldn’t attach a compressed disk to a
>>> VM, I don’t see how streaming makes sense then.  Well, I suppose
>>> intermediate streaming would work.
>>>
>>>> - backup with compression (we have an optional already, so it works)
>>>> - backup to nbd server with compression: enable compression on nbd server
>>>
>>> The problem is clearly that if a generic client were to connect to the
>>> NBD server, it wouldn’t work.  In this case, compression will work only
>>> if the clients understands the limitation.
>>>
>>> (The safe way would be to make the NBD server provide an option that
>>> clients can see so they know this limitation and agree they’ll adhere to
>>> it.  It’s also a stupid way.)
>>>
>>>> So instead of adding two options (for stream and for nbd), it seems better to
>>>> add only one for generic layer.
>>>
>>> I don’t know.  It doesn’t work generically, so I don’t know whether it
>>> should be a generic option.
>>>
>>>> Then, it becomes possible to run guest on image with compress=on. It's a side
>>>> effect, but still it should work correctly.
>>>
>>> How so?  compress=on only works if every party involved only writes to
>>> any cluster of the image exactly once.  That is just not the case for
>>> guests unless they know this limitation, and even I don’t see a use case.
>>>
>>>> I think the simplest thing is to just run normal write, if compressed write
>>>> failed because of reallocation. We should check that on that failure-path
>>>> ENOTSUP is returned and handle it for compress=on option by fallback to
>>>> normal write in generic block/io.c
>>>
>>> It seems wrong to not compress data with compress=on.
>>
>> We already fallback to normal write if can't compress in qcow2.
> 
> In a very specific case, namely where the compressed size is larger than
> the uncompressed size.  It would be a whole different story to only
> compress the first write to a given cluster but not any following one.
> 
>>>> (Note, that in case with stream we rewrite only unallocated clusters)
>>>
>>> Yes, but if the stream job writes the cluster before the guest, the
>>> guest gets an I/O error.
>>
>> I don't think that using compressed writes by guest is really usable thing.
>> In our case with stream there is no guest (just use qemu binary to operate
>> on block layer)
>>
>>>
>>>
>>> By the way, the other thing I wondered was whether this should be a
>>> filter driver instead (if it makes sense at all).  Such a filter driver
>>> would at least be sufficiently cumbersome to use that probably only
>>> users who understand the implications would make use of it (1B above).
>>>
>>>
>>> I’m not against adding a generic compress=on option if we ensure that it
>>> actually works generically (2 above).  It doesn’t right now, so I don’t
>>> think this is right.
>>>
>>> I’m already not sure whether it’s really possible to support generic
>>> compressed writes in qcow2.  I suppose it’d be at least awkward.  In
>>> theory it should work, it’s just that we can’t keep track of subcluster
>>> allocations, which in the worst case means that some compressed clusters
>>> take up a whole cluster of space.
>>>
>>> For VMDK...  I suppose we could consider a new flag for block drivers
>>> that flags whether a driver supports arbitrary compressed writes or has
>>> the old limitations.  compress=on could then refuse to work on any block
>>> driver but the ones that support arbitrary compressed writes.
>>>
>>>
>>> Our current model (1A) is simply to ensure that all compressed writes
>>> can adhere to the limitations.  As I’ve said above, extending this to
>>> NBD would mean adding some NBD negotiation so both client and server
>>> agree on this limitation.
>>
>> In this case, I'd prefere simple cmdline option fro qemu-nbd.
>>
>>> That seems kind of reasonable from the qemu
>>> side, but probably very unreasonable from an NBD side.  Why would NBD
>>> bother to reproduce qemu’s limitations?
>>>
>>>
>>> So these are the three ways (1A, 1B, 2) I can imagine.  But just adding
>>> a generic option that unsuspecting users are not unlikely to use but
>>> that simply doesn’t work generically doesn’t seem right to me.
>>>
>>
>> Firstly, 1A already doesn't work correctly: we have compress option for backup.
>> So, it will not work if backup target is not empty.
> 
> I’m not sure whether that qualifies because the user is simply
> responsible to ensure that the target is empty.
> 
> Otherwise, you could also argue that backup doesn’t necessarily create a
> real backup because with sync=top it won’t copy unallocated clusters, so
> if the target already has data in such a place, it won’t be overwritten.
> 
> Furthermore I’d argue this would hardly be an accident.  Both
> drive-backup with mode=existing and qemu-img convert -n are most likely
> not used blindly but only when the situation requires it.
> 
> My point is that using compress=on can be an accident because people see
> that option and think “That sounds useful!” without reading the warning.
> 
>> So, 1A is impossible, as it's already broken, adding separate options for stream
>> and qemu-nbd is not better than just add one generic option.
> 
> I disagree that 1A is broken, and I disagree a generic option would not
> be worse.  I still believe a simple generic option is prone to
> accidental misuse.
> 
>> I don't like (2) as it means a lot of effort to support actually not needed case.
> 
> Well, you say it isn’t needed.  I suppose if it were supported, people
> would indeed use it.
> 
>> I don't really like filter solution (as at seems too much to add a filter for simple
>> boolean option), but I think, we can live with it.
> 
> First, it is not a simple boolean option.  This patch brings
> implementation with it that converts writes to compressed writes and
> sets a request alignment.  I actually think it’s better to do things
> like that in a dedicated block driver than in the generic block layer code.
> 
> Second, we already this basically this for copy-on-read (which is also a
> boolean option for -drive).
> 
> (FWIW, I’d prefer if detect-zeroes were a block driver instead of
> generic block layer code.)

So, if we want all three options, it should be three filter drivers? Hmm..

> 
>> 1B is OK for me, that is, just document what the option does in fact.
>>
>> May be, name the option "compress-new-writes" instead of just "compress"?
>> And document, that writes to clusters, which was not written before will be
>> compressed?
>>
>> Or make the option to be compress=<x>, where x may be 'no' or 'new-writes',
>> reserving 'all-writes' for future?
> 
> I don’t know whether the limitations can be captured in three words, but
> it would at least make users pay closer attention to what the option is
> really doing.
> 
> OTOH, the only downsides to a dedicated block driver to me appear that
> it’s more cumbersome to use (which I actually consider a benefit) and
> that it requires the usual filter driver boilerplate – but that can be
> copied from other drivers[1].
> 
> (The benefits are that we don’t need to modify generic code and that
> people need to read into the filter and its caveats before they can use it.)
> 
> Max
> 
> 
> [1] Actually, we could provide default implementations somewhere and
> make block filters use them.  That should reduce the boilerplate size.
> 

OK, let's continue with a filter driver.

-- 
Best regards,
Vladimir

[PATCH v5 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>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@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..a1e7279 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 = 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, 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

[PATCH v5 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     | 45 +++++++++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/214.out | 14 ++++++++++++++
 2 files changed, 59 insertions(+)

diff --git a/tests/qemu-iotests/214 b/tests/qemu-iotests/214
index 21ec8a2..ab9b862 100755
--- a/tests/qemu-iotests/214
+++ b/tests/qemu-iotests/214
@@ -89,6 +89,51 @@ _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=$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
+
+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..4a2ec33 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 262144
+224 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+"compressed-clusters": 8
 *** done
-- 
1.8.3.1

[PATCH v5 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     | 34 +++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/030.out |  4 ++--
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
index f3766f2..f33fd21 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,37 @@ class TestSetSpeed(iotests.QMPTestCase):
 
         self.cancel_and_wait(resume=True)
 
+class TestCompressed(iotests.QMPTestCase):
+    allocated_clusters = 8
+
+    def setUp(self):
+        qemu_img('create', '-f', iotests.imgfmt, backing_img, '1M')
+        qemu_img('create', '-f', iotests.imgfmt, '-o',
+                 'backing_file={}'.format(backing_img), test_img)
+        cluster_size = 0x10000
+        data_size = self.allocated_clusters * cluster_size
+        qemu_io('-c', 'write -P 0x1 0 {}'.format(data_size), backing_img)
+        self.vm = iotests.VM().add_drive(test_img, "compress=on")
+        self.vm.launch()
+
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(test_img)
+        os.remove(backing_img)
+
+    def test_stream_compress(self):
+        self.assert_no_active_block_jobs()
+
+        result = self.vm.qmp('block-stream', device='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        match = {'data': {'type': 'stream', 'device': 'drive0'}}
+        self.vm.event_wait(name='BLOCK_JOB_COMPLETED', match=match)
+        self.vm.shutdown()
+
+        top = json.loads(qemu_img_pipe('check', '--output=json', test_img))
+        self.assertEqual(top['compressed-clusters'], self.allocated_clusters)
+
 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 v5 4/4] tests/qemu-iotests: add case for block-stream compress

[Max Reitz]

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

On 20.10.19 22:37, 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     | 34 +++++++++++++++++++++++++++++++++-
>  tests/qemu-iotests/030.out |  4 ++--
>  2 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030
> index f3766f2..f33fd21 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,37 @@ class TestSetSpeed(iotests.QMPTestCase):
>  
>          self.cancel_and_wait(resume=True)
>  
> +class TestCompressed(iotests.QMPTestCase):
> +    allocated_clusters = 8
> +
> +    def setUp(self):
> +        qemu_img('create', '-f', iotests.imgfmt, backing_img, '1M')
> +        qemu_img('create', '-f', iotests.imgfmt, '-o',
> +                 'backing_file={}'.format(backing_img), test_img)
> +        cluster_size = 0x10000
> +        data_size = self.allocated_clusters * cluster_size
> +        qemu_io('-c', 'write -P 0x1 0 {}'.format(data_size), backing_img)
> +        self.vm = iotests.VM().add_drive(test_img, "compress=on")

I don’t think it makes sense to add a drive with compress=on to a VM.
If the VM writes to any cluster more than once, the request will fail.

> +        self.vm.launch()
> +
> +    def tearDown(self):
> +        self.vm.shutdown()
> +        os.remove(test_img)
> +        os.remove(backing_img)
> +
> +    def test_stream_compress(self):
> +        self.assert_no_active_block_jobs()
> +
> +        result = self.vm.qmp('block-stream', device='drive0')
> +        self.assert_qmp(result, 'return', {})

In this example, it’s actually even a bit worse: The VM might be aware
that it may only write to each cluster exactly once.  But if it writes
to any of the first eight clusters after the stream job as done so
(invisibly to the VM guest), it will get an error.

You could see that by adding a qemu-io write here and see that it fails.
 (In practice you won’t because the error goes to stdout and that is
lost in Python tests).

Max

> +        match = {'data': {'type': 'stream', 'device': 'drive0'}}
> +        self.vm.event_wait(name='BLOCK_JOB_COMPLETED', match=match)
> +        self.vm.shutdown()
> +
> +        top = json.loads(qemu_img_pipe('check', '--output=json', test_img))
> +        self.assertEqual(top['compressed-clusters'], self.allocated_clusters)
> +
>  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
> 



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