Re: [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature

[Denis Plotnikov]

On 03.07.2019 17:14, Eric Blake wrote:
> On 7/3/19 6:00 AM, Denis Plotnikov wrote:
>> The patch adds some preparation parts for incompatible compression type
>> feature to QCOW2 header that indicates that*all*  compressed clusters
>> must be (de)compressed using a certain compression type.
>>
>> It is implied that the compression type is set on the image creation and
>> can be changed only later by image conversion, thus compression type
>> defines the only compression algorithm used for the image.
>>
>> The goal of the feature is to add support of other compression algorithms
>> to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
>> It works roughly x2 faster than ZLIB providing a comparable compression ratio
> 2x
> 
>> and therefore provide a performance advantage in backup scenarios.
>>
>> The default compression is ZLIB. Images created with ZLIB compression type
>> is backward compatible with older qemu versions.
> s/is/are/
grammar matters, will be changed
> 
>> Signed-off-by: Denis Plotnikov<address@hidden>
>> ---
>>   block/qcow2.c             | 94 +++++++++++++++++++++++++++++++++++++++
>>   block/qcow2.h             | 26 ++++++++---
>>   docs/interop/qcow2.txt    | 22 ++++++++-
>>   include/block/block_int.h |  1 +
>>   qapi/block-core.json      | 22 ++++++++-
>>   5 files changed, 155 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 3ace3b2209..921eb67b80 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1202,6 +1202,47 @@ static int qcow2_update_options(BlockDriverState *bs, 
>> QDict *options,
>>       return ret;
>>   }
>>   
>> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
>> +{
>> +    bool is_set;
>> +    int ret = 0;
>> +
>> +    switch (s->compression_type) {
>> +    case QCOW2_COMPRESSION_TYPE_ZLIB:
>> +        break;
>> +
>> +    default:
>> +        if (errp) {
> Useless check for errp being non-NULL.  What's worse, if the caller
> passes in NULL because they don't care about the error, then your code
> behaves differently.  Just call error_setg() and return -ENOTSUP
> unconditionally.
ok
> 
>> +            error_setg(errp, "qcow2: unknown compression type: %u",
>> +                       s->compression_type);
>> +            return -ENOTSUP;
>> +        }
>> +    }
>> +
>> +    /*
>> +     * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
>> +     * feature flag must be absent, with other compression types the
>> +     * incompatible feature flag must be set
> Is there a strong reason for forbid the incompatible feature flag with
> ZLIB?
The main reason is to guarantee image opening for older qemu if 
compression type is zlib.
> Sure, it makes the image impossible to open with older qemu, but
> it doesn't get in the way of newer qemu opening it. I'll have to see how
> your spec changes documented this, to see if you could instead word it
> as 'for ZLIB, the incompatible feature flag may be absent'.
I just can't imagine when and why we would want to set incompatible 
feature flag with zlib. Suppose we have zlib with the flag set, then
older qemu can't open the image although it is able to work with the 
zlib compression type. For now, I don't understand why we should make 
such an artificial restriction.
> 
>> +     */
>> +    is_set = s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE;
>> +
>> +    if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
>> +        if (is_set) {
>> +            ret = -EINVAL;
>> +        }
>> +    } else {
>> +        if (!is_set) {
>> +            ret = -EINVAL;
>> +        }
>> +    }
> More compact as:
> 
> if ((s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) == is_set)
ok
> 
>> +
>> +    if (ret && errp) {
>> +        error_setg(errp, "qcow2: Illegal compression type setting");
> s/Illegal/Invalid/ (the user isn't breaking a law)
Don't know, may be they do :) but, I'll change it
> 
> Also, don't check whether errp is non-NULL.  Just blindly call
> error_setg() if ret is non-zero.
ok
> 
>> +    }
>> +
>> +    return ret;
> Or even shorter (psuedocode):
> 
> if ((compression == ZLIB) != is_set) {
>      error_setg();
>      return -EINVAL;
> }
> return 0;
> 
>> +}
>> +
>>   /* Called with s->lock held.  */
>>   static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>>                                         int flags, Error **errp)
>> @@ -1318,6 +1359,24 @@ static int coroutine_fn 
>> qcow2_do_open(BlockDriverState *bs, QDict *options,
>>       s->compatible_features      = header.compatible_features;
>>       s->autoclear_features       = header.autoclear_features;
>>   
>> +    /* Handle compression type */
>> +    if (header.header_length > 104) {
> Magic number. Please spell this as header.header_length > offsetof (xxx,
> compression_type) for the appropriate xxx type.  Also, do you need to
> sanity check for an image using a length of 105-107 (invalid) vs. an
> image of 108 or larger?
Nice, will change it
> 
>> +        header.compression_type = be32_to_cpu(header.compression_type);
>> +        s->compression_type = header.compression_type;
>> +    } else {
>> +        /*
>> +         * older qcow2 images don't contain the compression type header
>> +         * field, distinguish them by the header length and use
>> +         * the only valid compression type in that case
>> +         */
>> +        s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
>> +    }
>> +
>> +    ret = check_compression_type(s, errp);
>> +    if (ret) {
>> +        goto fail;
>> +    }
>> +
>>       if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
>>           void *feature_table = NULL;
>>           qcow2_read_extensions(bs, header.header_length, ext_end,
>> @@ -2434,6 +2493,13 @@ int qcow2_update_header(BlockDriverState *bs)
>>       total_size = bs->total_sectors * BDRV_SECTOR_SIZE;
>>       refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 
>> 3);
>>   
>> +    ret = check_compression_type(s, NULL);
> Why are you ignoring the error here?
qcow2_update_header() doesn't use the error and just returns an error 
code or 0

> 
> 
>> @@ -5239,6 +5327,12 @@ static QemuOptsList qcow2_create_opts = {
>>               .help = "Width of a reference count entry in bits",
>>               .def_value_str = "16"
>>           },
>> +        {
>> +            .name = BLOCK_OPT_COMPRESSION_TYPE,
>> +            .type = QEMU_OPT_STRING,
>> +            .help = "Compression method used for image clusters 
>> compression",
>> +            .def_value_str = "0"
> Eww. Why are we exposing an integer rather than an enum value as the
> default value?  This should probably be "zlib".
Sure
> 
> 
>> +++ b/docs/interop/qcow2.txt
>> @@ -109,7 +109,12 @@ in the description of a field.
>>                                   An External Data File Name header 
>> extension may
>>                                   be present if this bit is set.
>>   
>> -                    Bits 3-63:  Reserved (set to 0)
>> +                    Bit 3:      Compression type bit. The bit must be set if
>> +                                the compression type differs from default: 
>> zlib.
> Maybe: "differs from the default of zlib".
> 
> Up to here is okay.
> 
>> +                                If the compression type is default the bit 
>> must
>> +                                be unset.
> Is this restriction necessary?
Please, see above about older qemu usage
> 
>> +
>> +                    Bits 4-63:  Reserved (set to 0)
>>   
>>            80 -  87:  compatible_features
>>                       Bitmask of compatible features. An implementation can
>> @@ -165,6 +170,21 @@ in the description of a field.
>>                       Length of the header structure in bytes. For version 2
>>                       images, the length is always assumed to be 72 bytes.
>>   
>> +        104 - 107:  compression_type
>> +                    Defines the compression method used for compressed 
>> clusters.
>> +                    A single compression type is applied to all compressed 
>> image
>> +                    clusters.
>> +                    The compression type is set on image creation only.
>> +                    The default compression type is zlib.
> Where is the documentation that a value of 0 corresponds to zlib?
Should I do it right here or it's better to add a separate chapter in 
the  docs/interop/qcow2.txt ?
> 
>> +                    When the deafult compression type is used the 
>> compression
> default
> 
>> +                    type bit (incompatible feature bit 3) must be unset.
> Is this restriction necessary?
> 
>> +                    When any other compression type is used the compression
>> +                    type bit must be set.
>> +                    Qemu versions older than 4.1 can use images created with
>> +                    the default compression type without any additional
>> +                    preparations and cannot use images created with any 
>> other
>> +                    compression type.
>> +
>>   Directly after the image header, optional sections called header 
>> extensions can
>>   be stored. Each extension has a structure like the following:
>>   
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 01e855a066..814917baec 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -58,6 +58,7 @@
>>   #define BLOCK_OPT_REFCOUNT_BITS     "refcount_bits"
>>   #define BLOCK_OPT_DATA_FILE         "data_file"
>>   #define BLOCK_OPT_DATA_FILE_RAW     "data_file_raw"
>> +#define BLOCK_OPT_COMPRESSION_TYPE  "compression_type"
>>   
>>   #define BLOCK_PROBE_BUF_SIZE        512
>>   
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 7ccbfff9d0..6aa8b99993 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -78,6 +78,8 @@
>>   #
>>   # @bitmaps: A list of qcow2 bitmap details (since 4.0)
>>   #
>> +# @compression-type: the image cluster compression method (since 4.1)
>> +#
>>   # Since: 1.7
>>   ##
>>   { 'struct': 'ImageInfoSpecificQCow2',
>> @@ -89,7 +91,8 @@
>>         '*corrupt': 'bool',
>>         'refcount-bits': 'int',
>>         '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>> -      '*bitmaps': ['Qcow2BitmapInfo']
>> +      '*bitmaps': ['Qcow2BitmapInfo'],
>> +      '*compression-type': 'Qcow2CompressionType'
> Why is this field optional? Can't we always populate it, even for older
> images?
Why we should if we don't care ?
> 
>>     } }
>>   
>>   ##
>> @@ -4206,6 +4209,18 @@
>>     'data': [ 'v2', 'v3' ] }
>>   
>>   
>> +##
>> +# @Qcow2CompressionType:
>> +#
>> +# Compression type used in qcow2 image file
>> +#
>> +# @zlib:  zlib compression, see<http://zlib.net/>
>> +#
>> +# Since: 4.1
>> +##
>> +{ 'enum': 'Qcow2CompressionType',
>> +  'data': [ 'zlib' ] }
>> +
>>   ##
>>   # @BlockdevCreateOptionsQcow2:
>>   #
>> @@ -4228,6 +4243,8 @@
>>   # @preallocation    Preallocation mode for the new image (default: off)
>>   # @lazy-refcounts   True if refcounts may be updated lazily (default: off)
>>   # @refcount-bits    Width of reference counts in bits (default: 16)
>> +# @compression-type The image cluster compression method
>> +#                   (default: zlib, since 4.1)
>>   #
>>   # Since: 2.12
>>   ##
>> @@ -4243,7 +4260,8 @@
>>               '*cluster-size':    'size',
>>               '*preallocation':   'PreallocMode',
>>               '*lazy-refcounts':  'bool',
>> -            '*refcount-bits':   'int' } }
>> +            '*refcount-bits':   'int',
>> +            '*compression-type': 'Qcow2CompressionType' } }
> But this one must indeed be optional.
> 
>>   
>>   ##
>>   # @BlockdevCreateOptionsQed:
>>
> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 
> Virtualization: qemu.org | libvirt.org
> 

-- 
Best,
Denis