[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v3 1/3] qcow2: introduce compression type featur
|
From: |
Denis Plotnikov |
|
Subject: |
Re: [Qemu-devel] [PATCH v3 1/3] qcow2: introduce compression type feature |
|
Date: |
Tue, 27 Aug 2019 14:52:53 +0000 |
On 27.08.2019 14:49, Vladimir Sementsov-Ogievskiy wrote:
> 19.08.2019 18:00, 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 provides 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 | 94 +++++++++++++++++++++++++++++++++++++++
>> block/qcow2.h | 26 ++++++++---
>> docs/interop/qcow2.txt | 19 +++++++-
>> include/block/block_int.h | 1 +
>> qapi/block-core.json | 22 ++++++++-
>> 5 files changed, 152 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 039bdc2f7e..4e07b7e9ec 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1197,6 +1197,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;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> /* Called with s->lock held. */
>> static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict
>> *options,
>> int flags, Error **errp)
>> @@ -1312,6 +1338,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");
>> + ret = -EINVAL;
>> + goto fail;
>> + }
>> +
>> + 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,
>> @@ -2516,6 +2571,12 @@ 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;
>> + }
>> +
>> *header = (QCowHeader) {
>> /* Version 2 fields */
>> .magic = cpu_to_be32(QCOW_MAGIC),
>> @@ -2538,6 +2599,7 @@ int qcow2_update_header(BlockDriverState *bs)
>> .autoclear_features = cpu_to_be64(s->autoclear_features),
>> .refcount_order = cpu_to_be32(s->refcount_order),
>> .header_length = cpu_to_be32(header_length),
>> + .compression_type = cpu_to_be32(s->compression_type),
>> };
>>
>> /* For older versions, write a shorter header */
>> @@ -2635,6 +2697,11 @@ int qcow2_update_header(BlockDriverState *bs)
>> .bit = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
>> .name = "lazy refcounts",
>> },
>> + {
>> + .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
>> + .bit = QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR,
>> + .name = "compression type",
>> + },
>> };
>>
>> ret = header_ext_add(buf, QCOW2_EXT_MAGIC_FEATURE_TABLE,
>> @@ -3202,6 +3269,7 @@ 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),
>> + .compression_type =
>> cpu_to_be32(QCOW2_COMPRESSION_TYPE_ZLIB),
>> .header_length = cpu_to_be32(sizeof(*header)),
>> };
>>
>> @@ -3221,6 +3289,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) {
>> + case QCOW2_COMPRESSION_TYPE_ZLIB:
>
> a bit confusing, as it's unreachable because of if (), and because if we are
> here we are going to
> do a wrong thing: to set incompatible feature (so, I firstly thought that
> this is a bug and then
> looked above at if() condition)
Yes, there should be QCOW2_COMPRESSION_TYPE_ZSTD being added in later
patches in the series
>> + break;
>> +
>> + default:
>> + error_setg_errno(errp, -EINVAL, "Unknown compression type");
>> + goto out;
>> + }
>> +
>> + header->compression_type =
>> cpu_to_be32(qcow2_opts->compression_type);
>> +
>> + header->incompatible_features |=
>> + cpu_to_be64(QCOW2_INCOMPAT_COMPRESSION_TYPE);
>
> cpu_to_be32 actually
true
>
>> + }
>> +
>> ret = blk_pwrite(blk, 0, header, cluster_size, 0);
>> g_free(header);
>> if (ret < 0) {
>> @@ -3402,6 +3488,7 @@ static int coroutine_fn qcow2_co_create_opts(const
>> char *filename, QemuOpts *opt
>> { BLOCK_OPT_ENCRYPT, BLOCK_OPT_ENCRYPT_FORMAT },
>> { BLOCK_OPT_COMPAT_LEVEL, "version" },
>> { BLOCK_OPT_DATA_FILE_RAW, "data-file-raw" },
>> + { BLOCK_OPT_COMPRESSION_TYPE, "compression-type" },
>
> I think we don't need it. This array is commented as:
> /* Change legacy command line options into QMP ones */
>
> but compression-type is not a legacy option, it's a new one. Why to tolerate
> old notation for it?
>
>> { NULL, NULL },
>> };
>>
>> @@ -4598,6 +4685,7 @@ static ImageInfoSpecific
>> *qcow2_get_specific_info(BlockDriverState *bs,
>> .data_file = g_strdup(s->image_data_file),
>> .has_data_file_raw = has_data_file(bs),
>> .data_file_raw = data_file_is_raw(bs),
>> + .compression_type = s->compression_type,
>> };
>> } else {
>> /* if this assertion fails, this probably means a new version was
>> @@ -5163,6 +5251,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 = "zlib"
>> + },
>> { /* end of list */ }
>> }
>> };
>> diff --git a/block/qcow2.h b/block/qcow2.h
>> index fc1b0d3c1e..9a241e4b9a 100644
>> --- a/block/qcow2.h
>> +++ b/block/qcow2.h
>> @@ -140,6 +140,7 @@ typedef struct QCowHeader {
>>
>> uint32_t refcount_order;
>> uint32_t header_length;
>> + uint32_t compression_type;
>> } QEMU_PACKED QCowHeader;
>>
>> typedef struct QEMU_PACKED QCowSnapshotHeader {
>> @@ -203,16 +204,20 @@ enum {
>>
>> /* Incompatible feature bits */
>> enum {
>> - QCOW2_INCOMPAT_DIRTY_BITNR = 0,
>> - QCOW2_INCOMPAT_CORRUPT_BITNR = 1,
>> - QCOW2_INCOMPAT_DATA_FILE_BITNR = 2,
>> - 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_DIRTY_BITNR = 0,
>> + QCOW2_INCOMPAT_CORRUPT_BITNR = 1,
>> + QCOW2_INCOMPAT_DATA_FILE_BITNR = 2,
>> + QCOW2_INCOMPAT_COMPRESSION_TYPE_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_TYPE =
>> + 1 << QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR,
>>
>> QCOW2_INCOMPAT_MASK = QCOW2_INCOMPAT_DIRTY
>> | QCOW2_INCOMPAT_CORRUPT
>> - | QCOW2_INCOMPAT_DATA_FILE,
>> + | QCOW2_INCOMPAT_DATA_FILE
>> + | QCOW2_INCOMPAT_COMPRESSION_TYPE,
>> };
>>
>> /* Compatible feature bits */
>> @@ -359,6 +364,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
>> + */
>> + uint32_t compression_type;
>> } BDRVQcow2State;
>>
>> typedef struct Qcow2COWRegion {
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index af5711e533..e1be8bd5c3 100644
>> --- a/docs/interop/qcow2.txt
>> +++ 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
>> of zlib.
>> + If the compression type is default the bit
>> should
>> + be unset.
>> +
>> + Bits 4-63: Reserved (set to 0)
>>
>> 80 - 87: compatible_features
>> Bitmask of compatible features. An implementation can
>> @@ -165,6 +170,18 @@ 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
>
> Why 4 bytes? 1 is enough and 2 are enough for sure. Or we need to align all
> fields to 4 bytes?
Each field in the header is at least 4 bytes in size including "32-35
crypt_method" which also is not expected to have a big variety of values
(now it's 3). I'm not sure but there should be some kind of reason for that.
>> + 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.
>
> this sentence is not needed, why to abandon inplace conversion? And anyway,
> it's not
> about specification of format.
But on the other hand, it makes the intentions clearer.>
>
>
>> + The default compression type is zlib (value: 0).
>> + When the compression type differs from the default
>> + the compression type bit (incompatible feature bit 3)
>> + must be set.
>> + Available compression type values:
>> + 0: zlib <https://www.zlib.net/> (default)
>> +
>> 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 3aa1e832a8..4b254802e5 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 0d43d4f37c..2c002ca6a9 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.2)
>> +#
>> # Since: 1.7
>> ##
>> { 'struct': 'ImageInfoSpecificQCow2',
>> @@ -89,7 +91,8 @@
>> '*corrupt': 'bool',
>> 'refcount-bits': 'int',
>> '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>> - '*bitmaps': ['Qcow2BitmapInfo']
>> + '*bitmaps': ['Qcow2BitmapInfo'],
>> + 'compression-type': 'Qcow2CompressionType'
>> } }
>>
>> ##
>> @@ -4274,6 +4277,18 @@
>> 'data': [ 'v2', 'v3' ] }
>>
>>
>> +##
>> +# @Qcow2CompressionType:
>> +#
>> +# Compression type used in qcow2 image file
>> +#
>> +# @zlib: zlib compression, see <http://zlib.net/>
>> +#
>> +# Since: 4.2
>> +##
>> +{ 'enum': 'Qcow2CompressionType',
>> + 'data': [ 'zlib' ] }
>> +
>> ##
>> # @BlockdevCreateOptionsQcow2:
>> #
>> @@ -4297,6 +4312,8 @@
>> # allowed values: off, falloc, full, metadata)
>> # @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.2)
>> #
>> # Since: 2.12
>> ##
>> @@ -4312,7 +4329,8 @@
>> '*cluster-size': 'size',
>> '*preallocation': 'PreallocMode',
>> '*lazy-refcounts': 'bool',
>> - '*refcount-bits': 'int' } }
>> + '*refcount-bits': 'int',
>> + '*compression-type': 'Qcow2CompressionType' } }
>>
>> ##
>> # @BlockdevCreateOptionsQed:
>>
>
>
--
Best,
Denis