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

[Vladimir Sementsov-Ogievskiy]

23.03.2020 17:25, 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 many tests
>      * 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 feature compression type
>        backing_file_offset += 56 (8 + 48 -> header_change + 
> feature_table_change)
>      * add "compression type" for test output matching when it isn't filtered
>        affected tests: 049, 060, 061, 065, 144, 182, 242, 255
>
> Signed-off-by: Denis Plotnikov <address@hidden>
> ---
>   qapi/block-core.json             |  22 +++++-
>   block/qcow2.h                    |  20 +++++-
>   include/block/block_int.h        |   1 +
>   block/qcow2.c                    | 113 +++++++++++++++++++++++++++++++
>   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, 267 insertions(+), 96 deletions(-)


[..]

> --- a/block/qcow2.h
> +++ 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];
>   } QEMU_PACKED QCowHeader;
>   
> +QEMU_BUILD_BUG_ON(sizeof(QCowHeader) % 8 != 0);

Better write

QEMU_BUILD_BUG_ON(QEMU_IS_ALIGNED(sizeof(QCowHeader), 8);

with or without this:

Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> +
>   typedef struct QEMU_PACKED QCowSnapshotHeader {
>       /* header is 8 byte aligned */
>       uint64_t l1_table_offset;
> @@ -216,13 +224,16 @@ enum {
>       QCOW2_INCOMPAT_DIRTY_BITNR      = 0,
>       QCOW2_INCOMPAT_CORRUPT_BITNR    = 1,
>       QCOW2_INCOMPAT_DATA_FILE_BITNR  = 2,
> +    QCOW2_INCOMPAT_COMPRESSION_BITNR = 3,
>       QCOW2_INCOMPAT_DIRTY            = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
>       QCOW2_INCOMPAT_CORRUPT          = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
>       QCOW2_INCOMPAT_DATA_FILE        = 1 << QCOW2_INCOMPAT_DATA_FILE_BITNR,
> +    QCOW2_INCOMPAT_COMPRESSION      = 1 << QCOW2_INCOMPAT_COMPRESSION_BITNR,
>   
>       QCOW2_INCOMPAT_MASK             = QCOW2_INCOMPAT_DIRTY
>                                       | QCOW2_INCOMPAT_CORRUPT
> -                                    | QCOW2_INCOMPAT_DATA_FILE,
> +                                    | QCOW2_INCOMPAT_DATA_FILE
> +                                    | QCOW2_INCOMPAT_COMPRESSION,
>   };
>   
>   /* Compatible feature bits */
> @@ -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.
> +     * For now, the only way to change the compression type
> +     * is to convert the image with the desired compression type set.
> +     */
> +    Qcow2CompressionType compression_type;
>   } BDRVQcow2State;
>   
>   typedef struct Qcow2COWRegion {


[..]


--
Best regards,
Vladimir