The patch adds some preparation parts for incompatible compression type
feature to Qcow2 that indicates which allow to 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.
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
block/qcow2.c | 105 ++++++++++++++++++++++++++++++++++++++
block/qcow2.h | 31 ++++++++---
include/block/block_int.h | 1 +
qapi/block-core.json | 22 +++++++-
4 files changed, 150 insertions(+), 9 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 3c754f616b..2ccb2cabd1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1242,6 +1242,50 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
return ret;
}
+static int validate_compression_type(BDRVQcow2State *s, Error **errp)
+{
+ /*
+ * Sanity check
+ * according to qcow2 spec, the compression type is 1-byte field
+ * but in BDRVQcow2State the compression_type is enum sizeof(int)
+ * so, the max compression_type value is 255.
+ */
+ if (s->compression_type > 0xff) {
+ error_setg(errp, "qcow2: compression type value is too big");
+ return -EINVAL;
+ }
+
+ 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) {
+ if (s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) {
+ error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must not be set");
+ return -EINVAL;
+ }
+ } else {
+ if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
+ error_setg(errp, "qcow2: Compression type incompatible feature "
+ "bit must be set");
+ return -EINVAL;
+ }
+ }
+
+ return 0;
+}
+
/* Called with s->lock held. */
static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
int flags, Error **errp)
@@ -1357,6 +1401,26 @@ 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)) {
+ /*
+ * don't deal with endians since compression_type is 1 byte long
+ */
+ s->compression_type = header.compression_type;
+ } else {
+ s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
+ }
+
+ ret = validate_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,
@@ -2720,6 +2784,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 = validate_compression_type(s, NULL);
+
+ if (ret) {
+ goto fail;
+ }
+
*header = (QCowHeader) {
/* Version 2 fields */
.magic = cpu_to_be32(QCOW_MAGIC),
@@ -2742,6 +2812,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 = (uint8_t) s->compression_type,
};
/* For older versions, write a shorter header */
@@ -2839,6 +2910,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,
@@ -3401,6 +3477,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 = (uint8_t) QCOW2_COMPRESSION_TYPE_ZLIB,
.header_length = cpu_to_be32(sizeof(*header)),
};
@@ -3420,6 +3497,26 @@ 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) {
+
+ if (qcow2_opts->compression_type > 0xff) {
+ error_setg_errno(errp, -EINVAL, "Too big compression type value");
+ goto out;
+ }
+
+ switch (qcow2_opts->compression_type) {
+ default:
+ error_setg_errno(errp, -EINVAL, "Unknown compression type");
+ goto out;
+ }
+
+ header->compression_type = (uint8_t) 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) {
@@ -3602,6 +3699,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" },
{ NULL, NULL },
};
@@ -4859,6 +4957,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
@@ -5516,6 +5615,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 0942126232..f92412ed5e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -146,6 +146,12 @@ 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;
typedef struct QEMU_PACKED QCowSnapshotHeader {
@@ -213,16 +219,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 */
@@ -369,6 +379,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
+ */
+ Qcow2CompressionType compression_type;
} BDRVQcow2State;
typedef struct Qcow2COWRegion {
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 6f9fd5e20e..2c6bb9dc99 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -57,6 +57,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 85e27bb61f..873fbef3b5 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 5.0)
+#
# Since: 1.7
##
{ 'struct': 'ImageInfoSpecificQCow2',
@@ -89,7 +91,8 @@
'*corrupt': 'bool',
'refcount-bits': 'int',
'*encrypt': 'ImageInfoSpecificQCow2Encryption',
- '*bitmaps': ['Qcow2BitmapInfo']
+ '*bitmaps': ['Qcow2BitmapInfo'],
+ 'compression-type': 'Qcow2CompressionType'
} }
##
@@ -4392,6 +4395,18 @@
'data': [ 'v2', 'v3' ] }
+##
+# @Qcow2CompressionType:
+#
+# Compression type used in qcow2 image file
+#
+# @zlib: zlib compression, see <http://zlib.net/>
+#
+# Since: 5.0
+##
+{ 'enum': 'Qcow2CompressionType',
+ 'data': [ 'zlib' ] }
+
##
# @BlockdevCreateOptionsQcow2:
#
@@ -4415,6 +4430,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 5.0)
#
# Since: 2.12
##
@@ -4430,7 +4447,8 @@
'*cluster-size': 'size',
'*preallocation': 'PreallocMode',
'*lazy-refcounts': 'bool',
- '*refcount-bits': 'int' } }
+ '*refcount-bits': 'int',
+ '*compression-type': 'Qcow2CompressionType' } }
##
# @BlockdevCreateOptionsQed:
--
2.17.0
On 2/27/20 1:29 AM, Denis Plotnikov wrote:
> The patch adds some preparation parts for incompatible compression type
> feature to Qcow2 that indicates which allow to use different compression
to qcow2, allowing the use of different
> 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.
>
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
> block/qcow2.c | 105 ++++++++++++++++++++++++++++++++++++++
> block/qcow2.h | 31 ++++++++---
> include/block/block_int.h | 1 +
> qapi/block-core.json | 22 +++++++-
> 4 files changed, 150 insertions(+), 9 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 3c754f616b..2ccb2cabd1 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1242,6 +1242,50 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
> return ret;
> }
>
> +static int validate_compression_type(BDRVQcow2State *s, Error **errp)
> +{
> + /*
> + * Sanity check
> + * according to qcow2 spec, the compression type is 1-byte field
> + * but in BDRVQcow2State the compression_type is enum sizeof(int)
> + * so, the max compression_type value is 255.
> + */
> + if (s->compression_type > 0xff) {
> + error_setg(errp, "qcow2: compression type value is too big");
> + return -EINVAL;
> + }
Hmm - I think it may be worth a tweak to qcow2.txt to call out:
104: compression_type
105 - 111: padding, must be 0
or else call out:
104-111: compression type
and just blindly use all 8 bytes for the value even though really only 1
or two values will ever be defined. Of course, that moves the byte in
question from 104 to 111, thanks to our big endian encoding, but as this
series is the first one installing a non-zero value in those 8 bytes,
and as we just finished documenting that the header length must be a
multiple of 8, there is no real impact - we can make such tweaks up
until the 5.0 release.
> +
> + switch (s->compression_type) {
> + case QCOW2_COMPRESSION_TYPE_ZLIB:
> + break;
> +
> + default:
> + error_setg(errp, "qcow2: unknown compression type: %u",
> + s->compression_type);
> + return -ENOTSUP;
> + }
Having two checks feels redundant, compared to just letting the default
catch all unrecognized values in that field.
> +
> + /*
> + * 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) {
> + if (s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) {
> + error_setg(errp, "qcow2: Compression type incompatible feature "
> + "bit must not be set");
> + return -EINVAL;
> + }
> + } else {
> + if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
> + error_setg(errp, "qcow2: Compression type incompatible feature "
> + "bit must be set");
> + return -EINVAL;
> + }
> + }
Matches what we documented in the spec.
> +
> + return 0;
> +}
> +
> /* Called with s->lock held. */
> static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> int flags, Error **errp)
> @@ -1357,6 +1401,26 @@ 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)) {
> + /*
> + * don't deal with endians since compression_type is 1 byte long
> + */
> + s->compression_type = header.compression_type;
Changes if you go with my suggestion of just making the compression_type
field occupy 8 bytes in the qcow2 header. (And if you want to keep it 1
byte, I still think the spec should call out explicit padding bytes).
> + } else {
> + s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
> + }
> +
> + ret = validate_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,
> @@ -2720,6 +2784,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 = validate_compression_type(s, NULL);
> +
> + if (ret) {
> + goto fail;
> + }
> +
> *header = (QCowHeader) {
> /* Version 2 fields */
> .magic = cpu_to_be32(QCOW_MAGIC),
> @@ -2742,6 +2812,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 = (uint8_t) s->compression_type,
Is the cast necessary?
> };
>
> /* For older versions, write a shorter header */
> @@ -2839,6 +2910,11 @@ int qcow2_update_header(BlockDriverState *bs)
> .bit = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
> .name = "lazy refcounts",
> },
> + {
> + .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
Ordering: please group all the incompatible bits side-by-side (this
should come before the lazy refcount bit).
> + .bit = QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR,
> + .name = "compression type",
This change breaks iotests; at least 31, 36, and 61 need updates (I've
got a similar patch pending which fixes the fact that we forgot the
autoclear bit [1]). You'll need to squash in fixes for those at the
same time.
[1] https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg08069.html
> + },
> };
>
> ret = header_ext_add(buf, QCOW2_EXT_MAGIC_FEATURE_TABLE,
> @@ -3401,6 +3477,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 = (uint8_t) QCOW2_COMPRESSION_TYPE_ZLIB,
Is the cast necessary?
> .header_length = cpu_to_be32(sizeof(*header)),
> };
>
> @@ -3420,6 +3497,26 @@ 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) {
> +
> + if (qcow2_opts->compression_type > 0xff) {
> + error_setg_errno(errp, -EINVAL, "Too big compression type value");
> + goto out;
> + }
> +
> + switch (qcow2_opts->compression_type) {
> + default:
> + error_setg_errno(errp, -EINVAL, "Unknown compression type");
> + goto out;
> + }
This should probably be an assert that qcow2_opts->compression_type is
in range, rather than a switch statement and error_setg. Callers of
qcow2_co_create should not be handing us unknown values.
> +
> + header->compression_type = (uint8_t) qcow2_opts->compression_type;
Why the cast?
> +
> + 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) {
> @@ -3602,6 +3699,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
> +++ b/block/qcow2.h
> @@ -146,6 +146,12 @@ 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;
You're changing the size of this struct, which WILL break iotests (and
even more than just the 3 I pointed out above for the feature name table).
/me looks ahead
Aha - you even noticed it: patch 7/8 fixes test 80. That fix needs to
be squashed in here, where the change is made.
>
> typedef struct QEMU_PACKED QCowSnapshotHeader {
> @@ -213,16 +219,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,
Uggh. I hate realigning = just because we added a longer name,
especially when you then can't even keep things on one line because of
length. If it were me, I'd leave the existing lines alone, and/or
switch everything to just use 'BITNR = ' rather than trying to align =.
Bikeshedding - since the new name is so long, can you get by with the
shorter QCOW2_INCOMPAT_COMPRESSION_BITNR (drop the _TYPE)?
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
27.02.2020 16:48, Eric Blake wrote:
> On 2/27/20 1:29 AM, Denis Plotnikov wrote:
>> The patch adds some preparation parts for incompatible compression type
>> feature to Qcow2 that indicates which allow to use different compression
>
> to qcow2, allowing the use of different
>
>> 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.
>>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> ---
>> block/qcow2.c | 105 ++++++++++++++++++++++++++++++++++++++
>> block/qcow2.h | 31 ++++++++---
>> include/block/block_int.h | 1 +
>> qapi/block-core.json | 22 +++++++-
>> 4 files changed, 150 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 3c754f616b..2ccb2cabd1 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1242,6 +1242,50 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
>> return ret;
>> }
>> +static int validate_compression_type(BDRVQcow2State *s, Error **errp)
>> +{
>> + /*
>> + * Sanity check
>> + * according to qcow2 spec, the compression type is 1-byte field
>> + * but in BDRVQcow2State the compression_type is enum sizeof(int)
>> + * so, the max compression_type value is 255.
>> + */
>> + if (s->compression_type > 0xff) {
>> + error_setg(errp, "qcow2: compression type value is too big");
>> + return -EINVAL;
>> + }
>
> Hmm - I think it may be worth a tweak to qcow2.txt to call out:
>
> 104: compression_type
> 105 - 111: padding, must be 0
>
> or else call out:
>
> 104-111: compression type
>
> and just blindly use all 8 bytes for the value even though really only 1 or two values will ever be defined. Of course, that moves the byte in question from 104 to 111, thanks to our big endian encoding, but as this series is the first one installing a non-zero value in those 8 bytes, and as we just finished documenting that the header length must be a multiple of 8, there is no real impact - we can make such tweaks up until the 5.0 release.
But what is the benefit? We have already documented padding in the spec, and discussed it so much time... What is the problem with padding? And why to add 8 bytes for every new feature which needs only one byte?
>
>> +
>> + switch (s->compression_type) {
>> + case QCOW2_COMPRESSION_TYPE_ZLIB:
>> + break;
>> +
>> + default:
>> + error_setg(errp, "qcow2: unknown compression type: %u",
>> + s->compression_type);
>> + return -ENOTSUP;
>> + }
>
> Having two checks feels redundant, compared to just letting the default catch all unrecognized values in that field.
>
>> +
>> + /*
>> + * 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) {
>> + if (s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) {
>> + error_setg(errp, "qcow2: Compression type incompatible feature "
>> + "bit must not be set");
>> + return -EINVAL;
>> + }
>> + } else {
>> + if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
>> + error_setg(errp, "qcow2: Compression type incompatible feature "
>> + "bit must be set");
>> + return -EINVAL;
>> + }
>> + }
>
> Matches what we documented in the spec.
>
>> +
>> + return 0;
>> +}
>> +
>> /* Called with s->lock held. */
>> static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>> int flags, Error **errp)
>> @@ -1357,6 +1401,26 @@ 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)) {
>> + /*
>> + * don't deal with endians since compression_type is 1 byte long
>> + */
>> + s->compression_type = header.compression_type;
>
> Changes if you go with my suggestion of just making the compression_type field occupy 8 bytes in the qcow2 header. (And if you want to keep it 1 byte, I still think the spec should call out explicit padding bytes).
>
>> + } else {
>> + s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
>> + }
>> +
>> + ret = validate_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,
>> @@ -2720,6 +2784,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 = validate_compression_type(s, NULL);
>> +
>> + if (ret) {
>> + goto fail;
>> + }
>> +
>> *header = (QCowHeader) {
>> /* Version 2 fields */
>> .magic = cpu_to_be32(QCOW_MAGIC),
>> @@ -2742,6 +2812,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 = (uint8_t) s->compression_type,
>
> Is the cast necessary?
>
>> };
>> /* For older versions, write a shorter header */
>> @@ -2839,6 +2910,11 @@ int qcow2_update_header(BlockDriverState *bs)
>> .bit = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
>> .name = "lazy refcounts",
>> },
>> + {
>> + .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
>
> Ordering: please group all the incompatible bits side-by-side (this should come before the lazy refcount bit).
>
>> + .bit = QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR,
>> + .name = "compression type",
>
> This change breaks iotests; at least 31, 36, and 61 need updates (I've got a similar patch pending which fixes the fact that we forgot the autoclear bit [1]). You'll need to squash in fixes for those at the same time.
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg08069.html
>
>> + },
>> };
>> ret = header_ext_add(buf, QCOW2_EXT_MAGIC_FEATURE_TABLE,
>> @@ -3401,6 +3477,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 = (uint8_t) QCOW2_COMPRESSION_TYPE_ZLIB,
>
> Is the cast necessary?
>
>> .header_length = cpu_to_be32(sizeof(*header)),
>> };
>> @@ -3420,6 +3497,26 @@ 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) {
>> +
>> + if (qcow2_opts->compression_type > 0xff) {
>> + error_setg_errno(errp, -EINVAL, "Too big compression type value");
>> + goto out;
>> + }
>> +
>> + switch (qcow2_opts->compression_type) {
>> + default:
>> + error_setg_errno(errp, -EINVAL, "Unknown compression type");
>> + goto out;
>> + }
>
> This should probably be an assert that qcow2_opts->compression_type is in range, rather than a switch statement and error_setg. Callers of qcow2_co_create should not be handing us unknown values.
>
>> +
>> + header->compression_type = (uint8_t) qcow2_opts->compression_type;
>
> Why the cast?
>
>> +
>> + 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) {
>> @@ -3602,6 +3699,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
>
>> +++ b/block/qcow2.h
>> @@ -146,6 +146,12 @@ 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;
>
> You're changing the size of this struct, which WILL break iotests (and even more than just the 3 I pointed out above for the feature name table).
>
> /me looks ahead
>
> Aha - you even noticed it: patch 7/8 fixes test 80. That fix needs to be squashed in here, where the change is made.
>
>> typedef struct QEMU_PACKED QCowSnapshotHeader {
>> @@ -213,16 +219,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,
>
> Uggh. I hate realigning = just because we added a longer name, especially when you then can't even keep things on one line because of length. If it were me, I'd leave the existing lines alone, and/or switch everything to just use 'BITNR = ' rather than trying to align =.
>
> Bikeshedding - since the new name is so long, can you get by with the shorter QCOW2_INCOMPAT_COMPRESSION_BITNR (drop the _TYPE)?
>
--
Best regards,
Vladimir
On 2/27/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote: >> Hmm - I think it may be worth a tweak to qcow2.txt to call out: >> >> 104: compression_type >> 105 - 111: padding, must be 0 >> >> or else call out: >> >> 104-111: compression type >> >> and just blindly use all 8 bytes for the value even though really only >> 1 or two values will ever be defined. Of course, that moves the byte >> in question from 104 to 111, thanks to our big endian encoding, but as >> this series is the first one installing a non-zero value in those 8 >> bytes, and as we just finished documenting that the header length must >> be a multiple of 8, there is no real impact - we can make such tweaks >> up until the 5.0 release. > > But what is the benefit? We have already documented padding in the spec, > and discussed it so much time... What is the problem with padding? And > why to add 8 bytes for every new feature which needs only one byte? Okay, so requiring an 8-byte field is not necessary. But still, at least mentioning padding bytes (that may be assigned meanings later) is consistent with the rest of the document (for example, we have padding bits documented for the compatible/incompatible/autoclear feature bits), and reminds implementers to keep size rounded to a multiple of 8. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
27.02.2020 17:13, Eric Blake wrote: > On 2/27/20 7:59 AM, Vladimir Sementsov-Ogievskiy wrote: > >>> Hmm - I think it may be worth a tweak to qcow2.txt to call out: >>> >>> 104: compression_type >>> 105 - 111: padding, must be 0 >>> >>> or else call out: >>> >>> 104-111: compression type >>> >>> and just blindly use all 8 bytes for the value even though really only 1 or two values will ever be defined. Of course, that moves the byte in question from 104 to 111, thanks to our big endian encoding, but as this series is the first one installing a non-zero value in those 8 bytes, and as we just finished documenting that the header length must be a multiple of 8, there is no real impact - we can make such tweaks up until the 5.0 release. >> >> But what is the benefit? We have already documented padding in the spec, and discussed it so much time... What is the problem with padding? And why to add 8 bytes for every new feature which needs only one byte? > > Okay, so requiring an 8-byte field is not necessary. But still, at least mentioning padding bytes (that may be assigned meanings later) is consistent with the rest of the document (for example, we have padding bits documented for the compatible/incompatible/autoclear feature bits), and reminds implementers to keep size rounded to a multiple of 8. > Yes, we can add something about it.. But I'm not sure we need, and I can't imaging correct short wording. We have section about padding: " === Header padding === @header_length must be a multiple of 8, which means that if the end of the last additional field is not aligned, some padding is needed. This padding must be zeroed, so that if some existing (or future) additional field will fall into the padding, it will be interpreted accordingly to point [3.] of the previous paragraph, i.e. in the same manner as when this field is not present. " So, if we want to add something about 104-111, it should be added to this section, not to previous "=== Additional fields (version 3 and higher) ===". And, if we want, to add something, we should consider both cases when compression type field exists and when not... What to write? "105 - 111: These bytes are padding, if header length > 104. May be turned into new additional fields in future." Sounds a bit strange... Keeping in mind that different versions of qemu may consider the same bytes as additional field or as padding, and it is correct. -- Best regards, Vladimir
On 2/27/20 8:30 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> But what is the benefit? We have already documented padding in the
>>> spec, and discussed it so much time... What is the problem with
>>> padding? And why to add 8 bytes for every new feature which needs
>>> only one byte?
>>
>> Okay, so requiring an 8-byte field is not necessary. But still, at
>> least mentioning padding bytes (that may be assigned meanings later)
>> is consistent with the rest of the document (for example, we have
>> padding bits documented for the compatible/incompatible/autoclear
>> feature bits), and reminds implementers to keep size rounded to a
>> multiple of 8.
>>
>
> Yes, we can add something about it.. But I'm not sure we need, and I
> can't imaging correct short wording.
>
>
> We have section about padding:
>
> "
> === Header padding ===
>
> @header_length must be a multiple of 8, which means that if the end of
> the last
> additional field is not aligned, some padding is needed. This padding
> must be
> zeroed, so that if some existing (or future) additional field will fall
> into
> the padding, it will be interpreted accordingly to point [3.] of the
> previous
> paragraph, i.e. in the same manner as when this field is not present.
> "
>
>
> So, if we want to add something about 104-111, it should be added to
> this section, not to previous "=== Additional fields (version 3 and
> higher) ===".
>
> And, if we want, to add something, we should consider both cases when
> compression type field exists and when not... What to write?
>
> "105 - 111: These bytes are padding, if header length > 104. May be
> turned into new additional fields in future."
>
> Sounds a bit strange... Keeping in mind that different versions of qemu
> may consider the same bytes as additional field or as padding, and it is
> correct.
Looking at the header extension, it can probably be as simple as:
105 - m: Zero padding to round up the header size to the next
multiple of 8.
I guess I'll propose a patch to make my idea concrete.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 27.02.2020 16:48, Eric Blake wrote:
> On 2/27/20 1:29 AM, Denis Plotnikov wrote:
>> The patch adds some preparation parts for incompatible compression type
>> feature to Qcow2 that indicates which allow to use different compression
>
> to qcow2, allowing the use of different
>
>> 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.
>>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> ---
>> block/qcow2.c | 105 ++++++++++++++++++++++++++++++++++++++
>> block/qcow2.h | 31 ++++++++---
>> include/block/block_int.h | 1 +
>> qapi/block-core.json | 22 +++++++-
>> 4 files changed, 150 insertions(+), 9 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 3c754f616b..2ccb2cabd1 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1242,6 +1242,50 @@ static int
>> qcow2_update_options(BlockDriverState *bs, QDict *options,
>> return ret;
>> }
>> +static int validate_compression_type(BDRVQcow2State *s, Error **errp)
>> +{
>> + /*
>> + * Sanity check
>> + * according to qcow2 spec, the compression type is 1-byte field
>> + * but in BDRVQcow2State the compression_type is enum sizeof(int)
>> + * so, the max compression_type value is 255.
>> + */
>> + if (s->compression_type > 0xff) {
>> + error_setg(errp, "qcow2: compression type value is too big");
>> + return -EINVAL;
>> + }
>
> Hmm - I think it may be worth a tweak to qcow2.txt to call out:
>
> 104: compression_type
> 105 - 111: padding, must be 0
>
> or else call out:
>
> 104-111: compression type
>
> and just blindly use all 8 bytes for the value even though really only
> 1 or two values will ever be defined. Of course, that moves the byte
> in question from 104 to 111, thanks to our big endian encoding, but as
> this series is the first one installing a non-zero value in those 8
> bytes, and as we just finished documenting that the header length must
> be a multiple of 8, there is no real impact - we can make such tweaks
> up until the 5.0 release.
>
>> +
>> + switch (s->compression_type) {
>> + case QCOW2_COMPRESSION_TYPE_ZLIB:
>> + break;
>> +
>> + default:
>> + error_setg(errp, "qcow2: unknown compression type: %u",
>> + s->compression_type);
>> + return -ENOTSUP;
>> + }
>
> Having two checks feels redundant, compared to just letting the
> default catch all unrecognized values in that field.
Looks like it is.
>
>> +
>> + /*
>> + * 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) {
>> + if (s->incompatible_features &
>> QCOW2_INCOMPAT_COMPRESSION_TYPE) {
>> + error_setg(errp, "qcow2: Compression type incompatible
>> feature "
>> + "bit must not be set");
>> + return -EINVAL;
>> + }
>> + } else {
>> + if (!(s->incompatible_features &
>> QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
>> + error_setg(errp, "qcow2: Compression type incompatible
>> feature "
>> + "bit must be set");
>> + return -EINVAL;
>> + }
>> + }
>
> Matches what we documented in the spec.
>
>> +
>> + return 0;
>> +}
>> +
>> /* Called with s->lock held. */
>> static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict
>> *options,
>> int flags, Error **errp)
>> @@ -1357,6 +1401,26 @@ 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)) {
>> + /*
>> + * don't deal with endians since compression_type is 1 byte
>> long
>> + */
>> + s->compression_type = header.compression_type;
>
> Changes if you go with my suggestion of just making the
> compression_type field occupy 8 bytes in the qcow2 header. (And if
> you want to keep it 1 byte, I still think the spec should call out
> explicit padding bytes).
>
>> + } else {
>> + s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
>> + }
>> +
>> + ret = validate_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,
>> @@ -2720,6 +2784,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 = validate_compression_type(s, NULL);
>> +
>> + if (ret) {
>> + goto fail;
>> + }
>> +
>> *header = (QCowHeader) {
>> /* Version 2 fields */
>> .magic = cpu_to_be32(QCOW_MAGIC),
>> @@ -2742,6 +2812,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 = (uint8_t) s->compression_type,
>
> Is the cast necessary?
s->compression_type is enum, I thought it would be good to explicitly
show that in the code.
If it's totally useless I'll remove it.
>
>> };
>> /* For older versions, write a shorter header */
>> @@ -2839,6 +2910,11 @@ int qcow2_update_header(BlockDriverState *bs)
>> .bit = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
>> .name = "lazy refcounts",
>> },
>> + {
>> + .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
>
> Ordering: please group all the incompatible bits side-by-side (this
> should come before the lazy refcount bit).
>
>> + .bit = QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR,
>> + .name = "compression type",
>
> This change breaks iotests; at least 31, 36, and 61 need updates (I've
> got a similar patch pending which fixes the fact that we forgot the
> autoclear bit [1]). You'll need to squash in fixes for those at the
> same time.
ok
>
> [1] https://lists.gnu.org/archive/html/qemu-devel/2020-01/msg08069.html
>
>> + },
>> };
>> ret = header_ext_add(buf, QCOW2_EXT_MAGIC_FEATURE_TABLE,
>> @@ -3401,6 +3477,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 = (uint8_t)
>> QCOW2_COMPRESSION_TYPE_ZLIB,
>
> Is the cast necessary?
>
>> .header_length = cpu_to_be32(sizeof(*header)),
>> };
>> @@ -3420,6 +3497,26 @@ 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) {
>> +
>> + if (qcow2_opts->compression_type > 0xff) {
>> + error_setg_errno(errp, -EINVAL, "Too big compression
>> type value");
>> + goto out;
>> + }
>> +
>> + switch (qcow2_opts->compression_type) {
>> + default:
>> + error_setg_errno(errp, -EINVAL, "Unknown compression
>> type");
>> + goto out;
>> + }
>
> This should probably be an assert that qcow2_opts->compression_type is
> in range, rather than a switch statement and error_setg. Callers of
> qcow2_co_create should not be handing us unknown values.
The intention was to express what has actually happened in the case of a
"bad" image to easy the problem investigation.
>
>> +
>> + header->compression_type = (uint8_t)
>> qcow2_opts->compression_type;
>
> Why the cast?
>
>> +
>> + 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) {
>> @@ -3602,6 +3699,7 @@ static int coroutine_fn
>> qcow2_co_create_opts(const char *filename, QemuOpts *opt
>
>> +++ b/block/qcow2.h
>> @@ -146,6 +146,12 @@ 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;
>
> You're changing the size of this struct, which WILL break iotests (and
> even more than just the 3 I pointed out above for the feature name
> table).
>
> /me looks ahead
>
> Aha - you even noticed it: patch 7/8 fixes test 80. That fix needs to
> be squashed in here, where the change is made.
ok, but the patch will be pretty long
>
>> typedef struct QEMU_PACKED QCowSnapshotHeader {
>> @@ -213,16 +219,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,
>
> Uggh. I hate realigning = just because we added a longer name,
> especially when you then can't even keep things on one line because of
> length. If it were me, I'd leave the existing lines alone, and/or
> switch everything to just use 'BITNR = ' rather than trying to align =.
>
> Bikeshedding - since the new name is so long, can you get by with the
> shorter QCOW2_INCOMPAT_COMPRESSION_BITNR (drop the _TYPE)?
Good idea, I'll certainly redo that part, because I don't like ether.
Denis
27.02.2020 10:29, Denis Plotnikov wrote:
> The patch adds some preparation parts for incompatible compression type
> feature to Qcow2 that indicates which allow to 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.
>
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
> block/qcow2.c | 105 ++++++++++++++++++++++++++++++++++++++
> block/qcow2.h | 31 ++++++++---
> include/block/block_int.h | 1 +
> qapi/block-core.json | 22 +++++++-
> 4 files changed, 150 insertions(+), 9 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 3c754f616b..2ccb2cabd1 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
Please, add to .git/config:
[diff]
orderFile = /path/to/qemu/scripts/git.orderfile
This will force git format-patch to sort files in more comfortable order (header changes first, etc).
> @@ -1242,6 +1242,50 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
> return ret;
> }
>
> +static int validate_compression_type(BDRVQcow2State *s, Error **errp)
> +{
> + /*
> + * Sanity check
> + * according to qcow2 spec, the compression type is 1-byte field
> + * but in BDRVQcow2State the compression_type is enum sizeof(int)
> + * so, the max compression_type value is 255.
> + */
> + if (s->compression_type > 0xff) {
> + error_setg(errp, "qcow2: compression type value is too big");
> + return -EINVAL;
> + }
> +
> + switch (s->compression_type) {
> + case QCOW2_COMPRESSION_TYPE_ZLIB:
> + break;
> +
> + default:
> + error_setg(errp, "qcow2: unknown compression type: %u",
> + s->compression_type);
> + return -ENOTSUP;
> + }
honestly, I think that just
if (s->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB) {
error out
}
is enough, and don't see why to check > 0xff in separate..
But it works as is.
> +
> + /*
> + * 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) {
> + if (s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) {
> + error_setg(errp, "qcow2: Compression type incompatible feature "
> + "bit must not be set");
> + return -EINVAL;
> + }
> + } else {
This is unreachable now.. But it's OK as preparation for further patches I think.
> + if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
> + error_setg(errp, "qcow2: Compression type incompatible feature "
> + "bit must be set");
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +
> /* Called with s->lock held. */
> static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> int flags, Error **errp)
> @@ -1357,6 +1401,26 @@ 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)) {
> + /*
> + * don't deal with endians since compression_type is 1 byte long
> + */
this comment would be more appropriate above, where be-to-cpu transformation is done,
as actually previous "s->autoclear_features = header.autoclear_features" doesn't
deal with endians too.
> + s->compression_type = header.compression_type;
> + } else {
> + s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
> + }
> +
> + ret = validate_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,
> @@ -2720,6 +2784,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 = validate_compression_type(s, NULL);
> +
> + if (ret) {
> + goto fail;
> + }
> +
> *header = (QCowHeader) {
> /* Version 2 fields */
> .magic = cpu_to_be32(QCOW_MAGIC),
> @@ -2742,6 +2812,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 = (uint8_t) s->compression_type,
> };
>
> /* For older versions, write a shorter header */
> @@ -2839,6 +2910,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,
> @@ -3401,6 +3477,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 = (uint8_t) QCOW2_COMPRESSION_TYPE_ZLIB,
Hmm. Following qcow2_co_create existing code, you'd better
1. do check compression_type correctnes before header initialization, may be add local variable
compression_type, don't forget to check version < 3, and here you'll have only
.compression_type = compression_type
> .header_length = cpu_to_be32(sizeof(*header)),
> };
>
> @@ -3420,6 +3497,26 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
> cpu_to_be64(QCOW2_AUTOCLEAR_DATA_FILE_RAW);
> }
>
2. Here you'll need only set incompatible_features if needed.
> + if (qcow2_opts->has_compression_type &&
> + qcow2_opts->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB) {
> +
> + if (qcow2_opts->compression_type > 0xff) {
> + error_setg_errno(errp, -EINVAL, "Too big compression type value");
I still doubt that we need seperate check for this thing. Moreover, here we can get
only element of enum, so we can do assertion, not if.
> + goto out;
going out here, you leak header
> + }
> +
> + switch (qcow2_opts->compression_type) {
> + default:
> + error_setg_errno(errp, -EINVAL, "Unknown compression type");
> + goto out;
> + }
> +
> + header->compression_type = (uint8_t) 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) {
> @@ -3602,6 +3699,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" },
> { NULL, NULL },
> };
>
> @@ -4859,6 +4957,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
> @@ -5516,6 +5615,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 0942126232..f92412ed5e 100644
> --- a/block/qcow2.h
> +++ b/block/qcow2.h
> @@ -146,6 +146,12 @@ 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;
>
> typedef struct QEMU_PACKED QCowSnapshotHeader {
> @@ -213,16 +219,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 */
> @@ -369,6 +379,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
> + */
> + Qcow2CompressionType compression_type;
> } BDRVQcow2State;
>
> typedef struct Qcow2COWRegion {
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 6f9fd5e20e..2c6bb9dc99 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -57,6 +57,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 85e27bb61f..873fbef3b5 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 5.0)
> +#
> # Since: 1.7
> ##
> { 'struct': 'ImageInfoSpecificQCow2',
> @@ -89,7 +91,8 @@
> '*corrupt': 'bool',
> 'refcount-bits': 'int',
> '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> - '*bitmaps': ['Qcow2BitmapInfo']
> + '*bitmaps': ['Qcow2BitmapInfo'],
> + 'compression-type': 'Qcow2CompressionType'
> } }
>
> ##
> @@ -4392,6 +4395,18 @@
> 'data': [ 'v2', 'v3' ] }
>
>
> +##
> +# @Qcow2CompressionType:
> +#
> +# Compression type used in qcow2 image file
> +#
> +# @zlib: zlib compression, see <http://zlib.net/>
> +#
> +# Since: 5.0
> +##
> +{ 'enum': 'Qcow2CompressionType',
> + 'data': [ 'zlib' ] }
> +
> ##
> # @BlockdevCreateOptionsQcow2:
> #
> @@ -4415,6 +4430,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 5.0)
> #
> # Since: 2.12
> ##
> @@ -4430,7 +4447,8 @@
> '*cluster-size': 'size',
> '*preallocation': 'PreallocMode',
> '*lazy-refcounts': 'bool',
> - '*refcount-bits': 'int' } }
> + '*refcount-bits': 'int',
> + '*compression-type': 'Qcow2CompressionType' } }
>
> ##
> # @BlockdevCreateOptionsQed:
>
--
Best regards,
Vladimir
On 2/27/20 2:21 AM, Vladimir Sementsov-Ogievskiy wrote: > 27.02.2020 10:29, Denis Plotnikov wrote: >> The patch adds some preparation parts for incompatible compression type >> feature to Qcow2 that indicates which allow to 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. >> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> >> --- >> block/qcow2.c | 105 ++++++++++++++++++++++++++++++++++++++ >> block/qcow2.h | 31 ++++++++--- >> include/block/block_int.h | 1 + >> qapi/block-core.json | 22 +++++++- >> 4 files changed, 150 insertions(+), 9 deletions(-) >> >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 3c754f616b..2ccb2cabd1 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c > > Please, add to .git/config: > > [diff] > orderFile = /path/to/qemu/scripts/git.orderfile > > This will force git format-patch to sort files in more comfortable order > (header changes first, etc). As I learned yesterday, git 2.23 and 2.24 have a bug where git format-patch fails to honor diff.orderfile (fixed in 2.25): https://bugzilla.redhat.com/show_bug.cgi?id=1807681 (and that explains why some of my recent patches have not been ordered the way I wanted, as Fedora 31 currently has a git from the broken window in time) -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
© 2016 - 2025 Red Hat, Inc.