Re: [Qemu-devel] [PATCH V4 08/10] block/qcow2: start using the compress format extension

[Peter Lieven]

Am 20.07.2017 um 18:00 schrieb Eric Blake:
> On 07/20/2017 09:20 AM, Peter Lieven wrote:
>> we now pass the parameters to the zlib compressor if the
>> extension is present and use the old default values if
>> the extension is absent.
>>
>> Signed-off-by: Peter Lieven <address@hidden>
>> ---
>>  block/qcow2-cluster.c | 58 
>> ++++++++++++++++++++++++++++++---------------------
>>  block/qcow2.c         | 57 
>> +++++++++++++++++++++++++++-----------------------
>>  2 files changed, 65 insertions(+), 50 deletions(-)
>>  static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
>> -                             const uint8_t *buf, int buf_size)
>> +                             const uint8_t *buf, int buf_size,
>> +                             int compress_format)
>>  {
>> -
>> -    ret = inflateInit2(strm, -12);
> The old code initializes with 12,
>
>
>> +    switch (compress_format) {
>> +    case QCOW2_COMPRESS_FORMAT_ZLIB:
>> +    case -1: {
> What is case -1 supposed to be? When the compression format was not
> specified?  Again, I'd really like there a way to encode the default
> (when no format is encoded in the qcow2 file) to be representable
> alongside the new code, and then we just special-case writing the
> compression format to the file to omit the extension header if the user
> requested parameters that match the old default.
>
>> +        z_stream z_strm = {};
>> +
>> +        z_strm.next_in = (uint8_t *)buf;
>> +        z_strm.avail_in = buf_size;
>> +        z_strm.next_out = out_buf;
>> +        z_strm.avail_out = out_buf_size;
>> +
>> +        ret = inflateInit2(&z_strm, -15);
> The new code is now unconditionally initializing with -15 instead of
> -12.  Does that matter, or does decompression work regardless of window
> size used at creation, as long as the initialized size at decompression
> is at least as large?  On the other hand, I guess that means if someone
> compresses with a large window, and then I initialize the decompressor
> with a small window, my decompression will fail?  That's why knowing the
> minimum window size should be part of the spec, whether or not we make
> it a tunable.

The decompression is supposed to fail if you compress with 15 and
decompress with 12. In fact it doesn't.

>
>> +++ b/block/qcow2.c
>> @@ -3391,9 +3391,10 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
>> uint64_t offset,
>>      BDRVQcow2State *s = bs->opaque;
>>      QEMUIOVector hd_qiov;
>>      struct iovec iov;
>> -    z_stream strm;
>> -    int ret, out_len;
>> -    uint8_t *buf, *out_buf, *local_buf = NULL;
>> +    z_stream z_strm = {};
>> +    int z_windowBits = -15, z_level = Z_DEFAULT_COMPRESSION;
>> +    int ret, out_len = 0;
>> +    uint8_t *buf, *out_buf = NULL, *local_buf = NULL;
>>      uint64_t cluster_offset;
>>  
>>      if (bytes == 0) {
>> @@ -3418,34 +3419,38 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, 
>> uint64_t offset,
>>          buf = qiov->iov[0].iov_base;
>>      }
>>  
>> -    out_buf = g_malloc(s->cluster_size);
>> +    switch (s->compress_format) {
>> +    case -1:
>> +        z_windowBits = -12;
>> +    case QCOW2_COMPRESS_FORMAT_ZLIB:
>> +        out_buf = g_malloc(s->cluster_size);
>> +        if (s->compress_level > 0) {
>> +            z_level = s->compress_level;
>> +        }
> And this is where I wonder if z_windowBits should be explicitly encoded
> in the qcow2 format, rather than just magic numbers we use under the
> hood, so that someone else trying to reimplement things will create
> compatible files.
>
I would like to avoid the windowBits in the qcow2 header as it makes

the code to read and write it more complicated. If you don't like the change

of the windowBits we can even stick with 12. If someone wants fast compression

he will likely not use zlib at all and use lzo.

I just changed the windowBits to 15 as it increases speed and improves 
compression.

(likely at the cost of memory during compression/decompression)


Peter