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 <address@hidden>
>> Suggested-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> Signed-off-by: Andrey Shinkevich <address@hidden>
>> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>> ---
>>   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