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

[Max Reitz]

On 30.04.20 11:48, Denis Plotnikov wrote:
> 
> 
> On 30.04.2020 11:26, Max Reitz wrote:
>> 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 <address@hidden>
>>>>>>> QAPI part:
>>>>>>> Acked-by: Markus Armbruster <address@hidden>
>>>>>>> ---
>>>>>>>     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).
> 
> We don't mention anything about zlib compressing details in the spec.

Yep.  Which is bad enough.

> If we mention anything about ZSTD compressing details we'll have to do
> it for
> zlib as well.

I personally don’t like “If you can’t make it perfect, you shouldn’t do
it at all”.  Mentioning it for zstd would still be an improvement.

Also, I believe that “zlib compression” is pretty much unambiguous,
considering all the code does is call deflate()/inflate().

I’m not saying we need to amend the spec in this series, but I don’t see
a good argument against doing so at some point.

> So, I think we have two possibilities for the spec:
> 1. mention for both
> 2. don't mention at all
> 
> I think the 2nd is better. It gives more degree of freedom for the
> future improvements.

No, it absolutely doesn’t.  There is a de-facto format, namely what qemu
accepts.  Changing that would be an incompatible change.  Just because
we don’t write what’s allowed into the spec doesn’t change that fact.

> and we've already covered both cases (one frame, may frames) in the code.
There are still different zstd formats, namely streaming or not
streaming.  That’s what should be mentioned.

Max

signature.asc
Description: OpenPGP digital signature