Re: [PATCH v22 3/4] qcow2: add zstd cluster compression

[Max Reitz <mreitz@redhat.com>]

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

On 29.04.20 15:02, Vladimir Sementsov-Ogievskiy wrote:
> 29.04.2020 15:17, Max Reitz wrote:
>> On 29.04.20 12:37, Vladimir Sementsov-Ogievskiy wrote:
>>> 29.04.2020 13:24, Max Reitz wrote:
>>>> On 28.04.20 22:00, Denis Plotnikov wrote:
>>>>> zstd significantly reduces cluster compression time.
>>>>> It provides better compression performance maintaining
>>>>> the same level of the compression ratio in comparison with
>>>>> zlib, which, at the moment, is the only compression
>>>>> method available.
>>>>>
>>>>> The performance test results:
>>>>> Test compresses and decompresses qemu qcow2 image with just
>>>>> installed rhel-7.6 guest.
>>>>> Image cluster size: 64K. Image on disk size: 2.2G
>>>>>
>>>>> The test was conducted with brd disk to reduce the influence
>>>>> of disk subsystem to the test results.
>>>>> The results is given in seconds.
>>>>>
>>>>> compress cmd:
>>>>>     time ./qemu-img convert -O qcow2 -c -o
>>>>> compression_type=[zlib|zstd]
>>>>>                     src.img [zlib|zstd]_compressed.img
>>>>> decompress cmd
>>>>>     time ./qemu-img convert -O qcow2
>>>>>                     [zlib|zstd]_compressed.img uncompressed.img
>>>>>
>>>>>              compression               decompression
>>>>>            zlib       zstd           zlib         zstd
>>>>> ------------------------------------------------------------
>>>>> real     65.5       16.3 (-75 %)    1.9          1.6 (-16 %)
>>>>> user     65.0       15.8            5.3          2.5
>>>>> sys       3.3        0.2            2.0          2.0
>>>>>
>>>>> Both ZLIB and ZSTD gave the same compression ratio: 1.57
>>>>> compressed image size in both cases: 1.4G
>>>>>
>>>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>>>>> QAPI part:
>>>>> Acked-by: Markus Armbruster <armbru@redhat.com>
>>>>> ---
>>>>>    docs/interop/qcow2.txt |   1 +
>>>>>    configure              |   2 +-
>>>>>    qapi/block-core.json   |   3 +-
>>>>>    block/qcow2-threads.c  | 169
>>>>> +++++++++++++++++++++++++++++++++++++++++
>>>>>    block/qcow2.c          |   7 ++
>>>>>    slirp                  |   2 +-
>>>>>    6 files changed, 181 insertions(+), 3 deletions(-)
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
>>>>> index 7dbaf53489..a0b12e1b15 100644
>>>>> --- a/block/qcow2-threads.c
>>>>> +++ b/block/qcow2-threads.c
>>>>
>>>> [...]
>>>>
>>>>> +static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size,
>>>>> +                                     const void *src, size_t
>>>>> src_size)
>>>>> +{
>>>>
>>>> [...]
>>>>
>>>>> +    /*
>>>>> +     * The compressed stream from the input buffer may consist of
>>>>> more
>>>>> +     * than one zstd frame.
>>>>
>>>> Can it?
>>>
>>> If not, we must require it in the specification.
>>
>> Actually, now that you mention it, it would make sense anyway to add
>> some note to the specification on what exactly compressed with zstd
>> means.
>>
>>> Hmm. If at some point
>>> we'll want multi-threaded compression of one big (2M) cluster.. Could
>>> this be implemented with zstd lib, if multiple frames are allowed, will
>>> allowing multiple frames help? I don't know actually, but I think better
>>> not to forbid it. On the other hand, I don't see any benefit in large
>>> compressed clusters. At least, in our scenarios (for compressed backups)
>>> we use 64k compressed clusters, for good granularity of incremental
>>> backups (when for running vm we use 1M clusters).
>>
>> Is it really that important?  Naïvely, it sounds rather complicated to
>> introduce multithreading into block drivers.
> 
> It is already here: compression and encryption already multithreaded.
> But of course, one cluster is handled in one thread.

Ah, good.  I forgot.

>> (Also, as for compression, it can only be used in backup scenarios
>> anyway, where you write many clusters at once.  So parallelism on the
>> cluster level should sufficient to get high usage, and it would benefit
>> all compression types and cluster sizes.)
>>
> 
> Yes it works in this way already :)

Well, OK then.

> So, we don't know do we want one frame restriction or not. Do you have a
> preference?

*shrug*

Seems like it would be preferential to allow multiple frames still.  A
note in the spec would be nice (i.e., streaming format, multiple frames
per cluster possible).

Max


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