[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH V4 08/10] block/qcow2: start using the compress
From: |
Peter Lieven |
Subject: |
Re: [Qemu-block] [PATCH V4 08/10] block/qcow2: start using the compress format extension |
Date: |
Thu, 20 Jul 2017 18:30:13 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.1 |
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
- Re: [Qemu-block] [PATCH V4 01/10] specs/qcow2: add compress format extension, (continued)
[Qemu-block] [PATCH V4 10/10] block/qcow2: add compress info to image specific info, Peter Lieven, 2017/07/20
[Qemu-block] [PATCH V4 02/10] qapi/block-core: add Qcow2Compress parameters, Peter Lieven, 2017/07/20
[Qemu-block] [PATCH V4 07/10] block/qcow2: optimize qcow2_co_pwritev_compressed, Peter Lieven, 2017/07/20
[Qemu-block] [PATCH V4 08/10] block/qcow2: start using the compress format extension, Peter Lieven, 2017/07/20
[Qemu-block] [PATCH V4 03/10] block/qcow2: parse compress create options, Peter Lieven, 2017/07/20
[Qemu-block] [PATCH V4 09/10] block/qcow2: add lzo compress format, Peter Lieven, 2017/07/20
[Qemu-block] [PATCH V4 06/10] block/qcow2: simplify ret usage in qcow2_create, Peter Lieven, 2017/07/20
[Qemu-block] [PATCH V4 05/10] block/qcow2: read and write the compress format extension, Peter Lieven, 2017/07/20