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


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