Re: [PATCH v6 1/4] qcow2: introduce compression type feature

[Eric Blake]

On 3/12/20 4:22 AM, Denis Plotnikov wrote:
> The patch adds some preparation parts for incompatible compression type
> feature to qcow2 allowing the use different compression methods for
> image clusters (de)compressing.
>
> 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, and thus,
> for all image clusters.
>
> The goal of the feature is to add support of other compression methods
> to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
>
> The default compression is ZLIB. Images created with ZLIB compression type
> are backward compatible with older qemu versions.
>
> Adding of the compression type breaks a number of tests because now the
> compression type is reported on image creation and there are some changes
> in the qcow2 header in size and offsets.
>
> The tests are fixed in the following ways:
>      * filter out compression_type for all the tests

Presumably this filter is optional, and we will not use it on the 
specific new tests that prove zstd compression works - but that should 
be later in the series, so for this patch it is okay.

>      * fix header size, feature table size and backing file offset
>        affected tests: 031, 036, 061, 080
>        header_size +=8: 1 byte compression type
>                         7 bytes padding
>        feature_table += 48: incompatible feture compression type

feature

>        backing_file_offset += 56 (8 + 48 -> header_change + fature_table_change)

feature

(interesting that you have two different changed spellings ;)

>      * add "compression type" for test output matching when it isn't filtered
>        affected tests: 049, 060, 061, 065, 144, 182, 242, 255

Or maybe the comment above should be changed to "many tests" rather than 
"all the tests".

> Signed-off-by: Denis Plotnikov <address@hidden>
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>   qapi/block-core.json             |  22 +++++-
>   block/qcow2.h                    |  20 ++++-
>   include/block/block_int.h        |   1 +
>   block/qcow2.c                    | 121 +++++++++++++++++++++++++++++++
>   tests/qemu-iotests/031.out       |  14 ++--
>   tests/qemu-iotests/036.out       |   4 +-
>   tests/qemu-iotests/049.out       | 102 +++++++++++++-------------
>   tests/qemu-iotests/060.out       |   1 +
>   tests/qemu-iotests/061.out       |  34 +++++----
>   tests/qemu-iotests/065           |  28 ++++---
>   tests/qemu-iotests/080           |   2 +-
>   tests/qemu-iotests/144.out       |   4 +-
>   tests/qemu-iotests/182.out       |   2 +-
>   tests/qemu-iotests/242.out       |   5 ++
>   tests/qemu-iotests/255.out       |   8 +-
>   tests/qemu-iotests/common.filter |   3 +-
>   16 files changed, 275 insertions(+), 96 deletions(-)

> +++ b/block/qcow2.h
> @@ -146,8 +146,16 @@ typedef struct QCowHeader {
>   
>       uint32_t refcount_order;
>       uint32_t header_length;
> +
> +    /* Additional fields */
> +    uint8_t  compression_type;
> +
> +    /* header must be a multiple of 8 */
> +    uint8_t  padding[7];

Why two spaces after uint8_t (twice)?


> @@ -369,6 +380,13 @@ typedef struct BDRVQcow2State {
>   
>       bool metadata_preallocation_checked;
>       bool metadata_preallocation;
> +    /*
> +     * Compression type used for the image. Default: 0 - ZLIB
> +     * The image compression type is set on image creation.
> +     * The only way to change the compression type is to convert the image
> +     * with the desired compression type set

Missing trailing '.'.  Maybe someday we can get 'qemu-img amend' to also 
adjust the compression type in-place; if that's something we think we 
might do, then this could be better worded as "For now, the only way to 
change...".

> +++ b/block/qcow2.c
> @@ -1242,6 +1242,48 @@ static int qcow2_update_options(BlockDriverState *bs, 
> QDict *options,
>       return ret;
>   }
>   
> +static int validate_compression_type(BDRVQcow2State *s, Error **errp)

> +
> +static int qcow2_compression_type_from_format(const char *ct)
> +{
> +    if (g_str_equal(ct, "zlib")) {
> +        return QCOW2_COMPRESSION_TYPE_ZLIB;
> +    } else {
> +        return -EINVAL;
> +    }

Why are you open-coding this?

qapi_enum_parse(&Qcow2CompressionType_lookup, ct, -1, errp)

should do what you use this for, and automatically updates itself when 
you add zstd to the qapi enum later.


> @@ -3401,6 +3493,8 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
> Error **errp)
>           .refcount_table_offset      = cpu_to_be64(cluster_size),
>           .refcount_table_clusters    = cpu_to_be32(1),
>           .refcount_order             = cpu_to_be32(refcount_order),
> +        /* don't deal with endians since compression_type is 1 byte long */

endianness

> +        .compression_type           = compression_type,
>           .header_length              = cpu_to_be32(sizeof(*header)),
>       };
>   
> @@ -5516,6 +5631,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",

s/clusters/cluster/

Overall, getting closer. I assume this would go through Max's tree, if 
he has time to include it before soft freeze...

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org