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

[Max Reitz]

On 04.07.19 15:09, 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 2x faster than ZLIB providing a comparable compression ratio
> and therefore provide a performance advantage in backup scenarios.
> 
> The default compression is ZLIB. Images created with ZLIB compression type
> are backward compatible with older qemu versions.
> 
> Signed-off-by: Denis Plotnikov <address@hidden>
> ---
>  block/qcow2.c             | 95 +++++++++++++++++++++++++++++++++++++++
>  block/qcow2.h             | 26 ++++++++---
>  docs/interop/qcow2.txt    | 21 ++++++++-
>  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..8fa932a349 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1202,6 +1202,32 @@ static int qcow2_update_options(BlockDriverState *bs, 
> QDict *options,
>      return ret;
>  }
>  
> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
> +{
> +    switch (s->compression_type) {
> +    case QCOW2_COMPRESSION_TYPE_ZLIB:
> +        break;
> +
> +    default:
> +        error_setg(errp, "qcow2: unknown compression type: %u",
> +                   s->compression_type);
> +        return -ENOTSUP;
> +    }
> +
> +    /*
> +     * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
> +     * the incompatible feature flag must be set
> +     */
> +
> +    if (s->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB &&
> +        !(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
> +            error_setg(errp, "qcow2: Invalid compression type setting");
> +            return -EINVAL;

(1) Why is this block indented twice?

(2) Do we need this at all?  Sure, it’s a corruption, but do we need to
reject the image because of it?

> +    }
> +
> +    return 0;
> +}
> +

Overall, I don’t see the purpose of this function.  I don’t see any need
to call it in qcow2_update_header().  And without “does non-zlib
compression imply the respective incompatible flag?” check, you can just
inline the rest (checking that we recognize the compression type) into
qcow2_do_open().

>  /* Called with s->lock held.  */
>  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>                                        int flags, Error **errp)
> @@ -1318,6 +1344,35 @@ 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
> +     * Older qcow2 images don't contain the compression type header.
> +     * Distinguish them by the header length and use
> +     * the only valid (default) compression type in that case
> +     */
> +    if (header.header_length > offsetof(QCowHeader, compression_type)) {
> +        /* sanity check that we can read a compression type */
> +        size_t min_len = offsetof(QCowHeader, compression_type) +
> +                         sizeof(header.compression_type);
> +        if (header.header_length < min_len) {
> +            error_setg(errp,
> +                       "Could not read compression type."
> +                       "qcow2 header is too short");

This will read as “Could not read compression type.qcow2 header is too
short”.  There should be a space after the full stop (and the full stop
should maybe be a comma instead).

> +           ret = -EINVAL;
> +           goto fail;

These two lines are incorrectly aligned.

> +        }
> +
> +        header.compression_type = be32_to_cpu(header.compression_type);
> +        s->compression_type = header.compression_type;
> +    } else {
> +        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 +2489,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);
> +
> +    if (ret) {
> +        goto fail;
> +    }
> +
> +

Again, I don’t see why this function should be called here.  If
anything, we should set the non-zlib incompatible flag here
automatically if a non-zlib compression type is used.

(And I don’t really see the point in checking that s->compression_type
is valid – because why shouldn’t it be?)

>      *header = (QCowHeader) {
>          /* Version 2 fields */
>          .magic                  = cpu_to_be32(QCOW_MAGIC),

[...]

> @@ -3126,6 +3195,24 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
> Error **errp)
>              cpu_to_be64(QCOW2_AUTOCLEAR_DATA_FILE_RAW);
>      }
>  
> +    if (qcow2_opts->has_compression_type &&
> +        qcow2_opts->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB) {
> +
> +        switch(qcow2_opts->compression_type) {

In qemu, we generally put a space between “switch” and the opening
parenthesis.


Also, just a hint: If you don’t like the visual appearance of

    if (long &&
        condition) {
        block_statement;

(I know I don’t)

you can (whenever a condition spans multiple lines) put the opening
curly bracket on a separate line:

    if (long &&
        condition)
    {
        block_statement;

(I personally prefer that over an empty line.)

> +        case QCOW2_COMPRESSION_TYPE_ZLIB:

Well, a bit useless considering you just excluded this case in the if
condition, but I suppose the compiler forced you to include this arm.

I suppose we should abort() here because you made the specification
state that the incompatible features must not be set with zlib
compression mode, so it looks weird to just accept this case here and
then set the incompatible flag below.

> +            break;
> +
> +        default:
> +            error_setg_errno(errp, -EINVAL, "Unknown compression type");

I think a plain abort() is fine here.  This is an enum after all, it
cannot have any other values.

> +            goto out;
> +        }
> +
> +        header->compression_type = cpu_to_be32(qcow2_opts->compression_type);
> +
> +        header->incompatible_features |=
> +            cpu_to_be64(QCOW2_INCOMPAT_COMPRESSION_TYPE);
> +    }
> +
>      ret = blk_pwrite(blk, 0, header, cluster_size, 0);
>      g_free(header);
>      if (ret < 0) {

[...]

> 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..835dd3c37f 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)

s/1/2/

> +#
>  # Since: 1.7
>  ##
>  { 'struct': 'ImageInfoSpecificQCow2',
> @@ -89,7 +91,8 @@
>        '*corrupt': 'bool',
>        'refcount-bits': 'int',
>        '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> -      '*bitmaps': ['Qcow2BitmapInfo']
> +      '*bitmaps': ['Qcow2BitmapInfo'],
> +      'compression-type': 'Qcow2CompressionType'
>    } }
>  
>  ##
> @@ -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

s/1/2/

> +##
> +{ '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)

s/1/2/

Max

>  #
>  # Since: 2.12
>  ##
> @@ -4243,7 +4260,8 @@
>              '*cluster-size':    'size',
>              '*preallocation':   'PreallocMode',
>              '*lazy-refcounts':  'bool',
> -            '*refcount-bits':   'int' } }
> +            '*refcount-bits':   'int',
> +            '*compression-type': 'Qcow2CompressionType' } }
>  
>  ##
>  # @BlockdevCreateOptionsQed:
>

signature.asc
Description: OpenPGP digital signature