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 x2 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
is backward compatible with older qemu versions.
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
block/qcow2.c | 94 +++++++++++++++++++++++++++++++++++++++
block/qcow2.h | 26 ++++++++---
docs/interop/qcow2.txt | 22 ++++++++-
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..921eb67b80 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1202,6 +1202,47 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
return ret;
}
+static int check_compression_type(BDRVQcow2State *s, Error **errp)
+{
+ bool is_set;
+ int ret = 0;
+
+ switch (s->compression_type) {
+ case QCOW2_COMPRESSION_TYPE_ZLIB:
+ break;
+
+ default:
+ if (errp) {
+ error_setg(errp, "qcow2: unknown compression type: %u",
+ s->compression_type);
+ return -ENOTSUP;
+ }
+ }
+
+ /*
+ * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
+ * feature flag must be absent, with other compression types the
+ * incompatible feature flag must be set
+ */
+ is_set = s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE;
+
+ if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
+ if (is_set) {
+ ret = -EINVAL;
+ }
+ } else {
+ if (!is_set) {
+ ret = -EINVAL;
+ }
+ }
+
+ if (ret && errp) {
+ error_setg(errp, "qcow2: Illegal compression type setting");
+ }
+
+ return ret;
+}
+
/* Called with s->lock held. */
static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
int flags, Error **errp)
@@ -1318,6 +1359,24 @@ 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 */
+ if (header.header_length > 104) {
+ header.compression_type = be32_to_cpu(header.compression_type);
+ s->compression_type = header.compression_type;
+ } else {
+ /*
+ * older qcow2 images don't contain the compression type header
+ * field, distinguish them by the header length and use
+ * the only valid compression type in that case
+ */
+ 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 +2493,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;
+ }
+
+
*header = (QCowHeader) {
/* Version 2 fields */
.magic = cpu_to_be32(QCOW_MAGIC),
@@ -2456,6 +2522,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 */
@@ -2553,6 +2620,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,
@@ -3107,6 +3179,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)),
};
@@ -3126,6 +3199,20 @@ 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 >= QCOW2_COMPRESSION_TYPE__MAX) {
+ 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);
+ }
+
ret = blk_pwrite(blk, 0, header, cluster_size, 0);
g_free(header);
if (ret < 0) {
@@ -3307,6 +3394,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 },
};
@@ -5239,6 +5327,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 = "0"
+ },
{ /* end of list */ }
}
};
diff --git a/block/qcow2.h b/block/qcow2.h
index fdee297f33..b44344c78e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -135,6 +135,7 @@ typedef struct QCowHeader {
uint32_t refcount_order;
uint32_t header_length;
+ uint32_t compression_type;
} QEMU_PACKED QCowHeader;
typedef struct QEMU_PACKED QCowSnapshotHeader {
@@ -198,16 +199,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 */
@@ -350,6 +355,13 @@ typedef struct BDRVQcow2State {
int nb_compress_threads;
BdrvChild *data_file;
+ /*
+ * 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..5b8a8d15fe 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: zlib.
+ If the compression type is default the bit must
+ be unset.
+
+ Bits 4-63: Reserved (set to 0)
80 - 87: compatible_features
Bitmask of compatible features. An implementation can
@@ -165,6 +170,21 @@ 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
+ 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.
+ The default compression type is zlib.
+ When the deafult compression type is used the compression
+ type bit (incompatible feature bit 3) must be unset.
+ When any other compression type is used the compression
+ type bit must be set.
+ Qemu versions older than 4.1 can use images created with
+ the default compression type without any additional
+ preparations and cannot use images created with any other
+ compression type.
+
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 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..6aa8b99993 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)
+#
# 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
+##
+{ '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)
#
# 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:
--
2.17.0
I'd like to recommend a few commas. Denis Plotnikov <dplotnikov@virtuozzo.com> writes: > 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 x2 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 > is backward compatible with older qemu versions. > > Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com> [...] > diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt > index af5711e533..5b8a8d15fe 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: zlib. > + If the compression type is default the bit must > + be unset. > + > + Bits 4-63: Reserved (set to 0) > > 80 - 87: compatible_features > Bitmask of compatible features. An implementation can > @@ -165,6 +170,21 @@ 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 > + 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. > + The default compression type is zlib. > + When the deafult compression type is used the compression Comma after used, please. > + type bit (incompatible feature bit 3) must be unset. > + When any other compression type is used the compression Likewise. > + type bit must be set. > + Qemu versions older than 4.1 can use images created with > + the default compression type without any additional > + preparations and cannot use images created with any other Comma after preparations, please. > + compression type. > + > Directly after the image header, optional sections called header extensions can > be stored. Each extension has a structure like the following: > [...]
On 7/3/19 6:00 AM, 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 x2 faster than ZLIB providing a comparable compression ratio
2x
> and therefore provide a performance advantage in backup scenarios.
>
> The default compression is ZLIB. Images created with ZLIB compression type
> is backward compatible with older qemu versions.
s/is/are/
>
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
> block/qcow2.c | 94 +++++++++++++++++++++++++++++++++++++++
> block/qcow2.h | 26 ++++++++---
> docs/interop/qcow2.txt | 22 ++++++++-
> 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..921eb67b80 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1202,6 +1202,47 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
> return ret;
> }
>
> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
> +{
> + bool is_set;
> + int ret = 0;
> +
> + switch (s->compression_type) {
> + case QCOW2_COMPRESSION_TYPE_ZLIB:
> + break;
> +
> + default:
> + if (errp) {
Useless check for errp being non-NULL. What's worse, if the caller
passes in NULL because they don't care about the error, then your code
behaves differently. Just call error_setg() and return -ENOTSUP
unconditionally.
> + error_setg(errp, "qcow2: unknown compression type: %u",
> + s->compression_type);
> + return -ENOTSUP;
> + }
> + }
> +
> + /*
> + * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
> + * feature flag must be absent, with other compression types the
> + * incompatible feature flag must be set
Is there a strong reason for forbid the incompatible feature flag with
ZLIB? Sure, it makes the image impossible to open with older qemu, but
it doesn't get in the way of newer qemu opening it. I'll have to see how
your spec changes documented this, to see if you could instead word it
as 'for ZLIB, the incompatible feature flag may be absent'.
> + */
> + is_set = s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE;
> +
> + if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
> + if (is_set) {
> + ret = -EINVAL;
> + }
> + } else {
> + if (!is_set) {
> + ret = -EINVAL;
> + }
> + }
More compact as:
if ((s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) == is_set)
> +
> + if (ret && errp) {
> + error_setg(errp, "qcow2: Illegal compression type setting");
s/Illegal/Invalid/ (the user isn't breaking a law)
Also, don't check whether errp is non-NULL. Just blindly call
error_setg() if ret is non-zero.
> + }
> +
> + return ret;
Or even shorter (psuedocode):
if ((compression == ZLIB) != is_set) {
error_setg();
return -EINVAL;
}
return 0;
> +}
> +
> /* Called with s->lock held. */
> static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> int flags, Error **errp)
> @@ -1318,6 +1359,24 @@ 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 */
> + if (header.header_length > 104) {
Magic number. Please spell this as header.header_length > offsetof (xxx,
compression_type) for the appropriate xxx type. Also, do you need to
sanity check for an image using a length of 105-107 (invalid) vs. an
image of 108 or larger?
> + header.compression_type = be32_to_cpu(header.compression_type);
> + s->compression_type = header.compression_type;
> + } else {
> + /*
> + * older qcow2 images don't contain the compression type header
> + * field, distinguish them by the header length and use
> + * the only valid compression type in that case
> + */
> + 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 +2493,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);
Why are you ignoring the error here?
> @@ -5239,6 +5327,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 = "0"
Eww. Why are we exposing an integer rather than an enum value as the
default value? This should probably be "zlib".
> +++ 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: zlib.
Maybe: "differs from the default of zlib".
Up to here is okay.
> + If the compression type is default the bit must
> + be unset.
Is this restriction necessary?
> +
> + Bits 4-63: Reserved (set to 0)
>
> 80 - 87: compatible_features
> Bitmask of compatible features. An implementation can
> @@ -165,6 +170,21 @@ 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
> + 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.
> + The default compression type is zlib.
Where is the documentation that a value of 0 corresponds to zlib?
> + When the deafult compression type is used the compression
default
> + type bit (incompatible feature bit 3) must be unset.
Is this restriction necessary?
> + When any other compression type is used the compression
> + type bit must be set.
> + Qemu versions older than 4.1 can use images created with
> + the default compression type without any additional
> + preparations and cannot use images created with any other
> + compression type.
> +
> 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 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..6aa8b99993 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)
> +#
> # Since: 1.7
> ##
> { 'struct': 'ImageInfoSpecificQCow2',
> @@ -89,7 +91,8 @@
> '*corrupt': 'bool',
> 'refcount-bits': 'int',
> '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> - '*bitmaps': ['Qcow2BitmapInfo']
> + '*bitmaps': ['Qcow2BitmapInfo'],
> + '*compression-type': 'Qcow2CompressionType'
Why is this field optional? Can't we always populate it, even for older
images?
> } }
>
> ##
> @@ -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
> +##
> +{ '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)
> #
> # Since: 2.12
> ##
> @@ -4243,7 +4260,8 @@
> '*cluster-size': 'size',
> '*preallocation': 'PreallocMode',
> '*lazy-refcounts': 'bool',
> - '*refcount-bits': 'int' } }
> + '*refcount-bits': 'int',
> + '*compression-type': 'Qcow2CompressionType' } }
But this one must indeed be optional.
>
> ##
> # @BlockdevCreateOptionsQed:
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 03.07.2019 17:14, Eric Blake wrote:
> On 7/3/19 6:00 AM, 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 x2 faster than ZLIB providing a comparable compression ratio
> 2x
>
>> and therefore provide a performance advantage in backup scenarios.
>>
>> The default compression is ZLIB. Images created with ZLIB compression type
>> is backward compatible with older qemu versions.
> s/is/are/
grammar matters, will be changed
>
>> Signed-off-by: Denis Plotnikov<dplotnikov@virtuozzo.com>
>> ---
>> block/qcow2.c | 94 +++++++++++++++++++++++++++++++++++++++
>> block/qcow2.h | 26 ++++++++---
>> docs/interop/qcow2.txt | 22 ++++++++-
>> 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..921eb67b80 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1202,6 +1202,47 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
>> return ret;
>> }
>>
>> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
>> +{
>> + bool is_set;
>> + int ret = 0;
>> +
>> + switch (s->compression_type) {
>> + case QCOW2_COMPRESSION_TYPE_ZLIB:
>> + break;
>> +
>> + default:
>> + if (errp) {
> Useless check for errp being non-NULL. What's worse, if the caller
> passes in NULL because they don't care about the error, then your code
> behaves differently. Just call error_setg() and return -ENOTSUP
> unconditionally.
ok
>
>> + error_setg(errp, "qcow2: unknown compression type: %u",
>> + s->compression_type);
>> + return -ENOTSUP;
>> + }
>> + }
>> +
>> + /*
>> + * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
>> + * feature flag must be absent, with other compression types the
>> + * incompatible feature flag must be set
> Is there a strong reason for forbid the incompatible feature flag with
> ZLIB?
The main reason is to guarantee image opening for older qemu if
compression type is zlib.
> Sure, it makes the image impossible to open with older qemu, but
> it doesn't get in the way of newer qemu opening it. I'll have to see how
> your spec changes documented this, to see if you could instead word it
> as 'for ZLIB, the incompatible feature flag may be absent'.
I just can't imagine when and why we would want to set incompatible
feature flag with zlib. Suppose we have zlib with the flag set, then
older qemu can't open the image although it is able to work with the
zlib compression type. For now, I don't understand why we should make
such an artificial restriction.
>
>> + */
>> + is_set = s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE;
>> +
>> + if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
>> + if (is_set) {
>> + ret = -EINVAL;
>> + }
>> + } else {
>> + if (!is_set) {
>> + ret = -EINVAL;
>> + }
>> + }
> More compact as:
>
> if ((s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) == is_set)
ok
>
>> +
>> + if (ret && errp) {
>> + error_setg(errp, "qcow2: Illegal compression type setting");
> s/Illegal/Invalid/ (the user isn't breaking a law)
Don't know, may be they do :) but, I'll change it
>
> Also, don't check whether errp is non-NULL. Just blindly call
> error_setg() if ret is non-zero.
ok
>
>> + }
>> +
>> + return ret;
> Or even shorter (psuedocode):
>
> if ((compression == ZLIB) != is_set) {
> error_setg();
> return -EINVAL;
> }
> return 0;
>
>> +}
>> +
>> /* Called with s->lock held. */
>> static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>> int flags, Error **errp)
>> @@ -1318,6 +1359,24 @@ 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 */
>> + if (header.header_length > 104) {
> Magic number. Please spell this as header.header_length > offsetof (xxx,
> compression_type) for the appropriate xxx type. Also, do you need to
> sanity check for an image using a length of 105-107 (invalid) vs. an
> image of 108 or larger?
Nice, will change it
>
>> + header.compression_type = be32_to_cpu(header.compression_type);
>> + s->compression_type = header.compression_type;
>> + } else {
>> + /*
>> + * older qcow2 images don't contain the compression type header
>> + * field, distinguish them by the header length and use
>> + * the only valid compression type in that case
>> + */
>> + 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 +2493,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);
> Why are you ignoring the error here?
qcow2_update_header() doesn't use the error and just returns an error
code or 0
>
>
>> @@ -5239,6 +5327,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 = "0"
> Eww. Why are we exposing an integer rather than an enum value as the
> default value? This should probably be "zlib".
Sure
>
>
>> +++ 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: zlib.
> Maybe: "differs from the default of zlib".
>
> Up to here is okay.
>
>> + If the compression type is default the bit must
>> + be unset.
> Is this restriction necessary?
Please, see above about older qemu usage
>
>> +
>> + Bits 4-63: Reserved (set to 0)
>>
>> 80 - 87: compatible_features
>> Bitmask of compatible features. An implementation can
>> @@ -165,6 +170,21 @@ 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
>> + 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.
>> + The default compression type is zlib.
> Where is the documentation that a value of 0 corresponds to zlib?
Should I do it right here or it's better to add a separate chapter in
the docs/interop/qcow2.txt ?
>
>> + When the deafult compression type is used the compression
> default
>
>> + type bit (incompatible feature bit 3) must be unset.
> Is this restriction necessary?
>
>> + When any other compression type is used the compression
>> + type bit must be set.
>> + Qemu versions older than 4.1 can use images created with
>> + the default compression type without any additional
>> + preparations and cannot use images created with any other
>> + compression type.
>> +
>> 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 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..6aa8b99993 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)
>> +#
>> # Since: 1.7
>> ##
>> { 'struct': 'ImageInfoSpecificQCow2',
>> @@ -89,7 +91,8 @@
>> '*corrupt': 'bool',
>> 'refcount-bits': 'int',
>> '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>> - '*bitmaps': ['Qcow2BitmapInfo']
>> + '*bitmaps': ['Qcow2BitmapInfo'],
>> + '*compression-type': 'Qcow2CompressionType'
> Why is this field optional? Can't we always populate it, even for older
> images?
Why we should if we don't care ?
>
>> } }
>>
>> ##
>> @@ -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
>> +##
>> +{ '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)
>> #
>> # Since: 2.12
>> ##
>> @@ -4243,7 +4260,8 @@
>> '*cluster-size': 'size',
>> '*preallocation': 'PreallocMode',
>> '*lazy-refcounts': 'bool',
>> - '*refcount-bits': 'int' } }
>> + '*refcount-bits': 'int',
>> + '*compression-type': 'Qcow2CompressionType' } }
> But this one must indeed be optional.
>
>>
>> ##
>> # @BlockdevCreateOptionsQed:
>>
> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226
> Virtualization: qemu.org | libvirt.org
>
--
Best,
Denis
Am 03.07.2019 um 17:01 hat Denis Plotnikov geschrieben:
> On 03.07.2019 17:14, Eric Blake wrote:
> > On 7/3/19 6:00 AM, Denis Plotnikov wrote:
> >> diff --git a/block/qcow2.c b/block/qcow2.c
> >> index 3ace3b2209..921eb67b80 100644
> >> --- a/block/qcow2.c
> >> +++ b/block/qcow2.c
> >> @@ -1202,6 +1202,47 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
> >> return ret;
> >> }
> >>
> >> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
> >> +{
> >> + bool is_set;
> >> + int ret = 0;
> >> +
> >> + switch (s->compression_type) {
> >> + case QCOW2_COMPRESSION_TYPE_ZLIB:
> >> + break;
> >> +
> >> + default:
> >> + if (errp) {
> > Useless check for errp being non-NULL. What's worse, if the caller
> > passes in NULL because they don't care about the error, then your code
> > behaves differently. Just call error_setg() and return -ENOTSUP
> > unconditionally.
> ok
> >
> >> + error_setg(errp, "qcow2: unknown compression type: %u",
> >> + s->compression_type);
> >> + return -ENOTSUP;
> >> + }
> >> + }
> >> +
> >> + /*
> >> + * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
> >> + * feature flag must be absent, with other compression types the
> >> + * incompatible feature flag must be set
> > Is there a strong reason for forbid the incompatible feature flag with
> > ZLIB?
> The main reason is to guarantee image opening for older qemu if
> compression type is zlib.
> > Sure, it makes the image impossible to open with older qemu, but
> > it doesn't get in the way of newer qemu opening it. I'll have to see how
> > your spec changes documented this, to see if you could instead word it
> > as 'for ZLIB, the incompatible feature flag may be absent'.
> I just can't imagine when and why we would want to set incompatible
> feature flag with zlib. Suppose we have zlib with the flag set, then
> older qemu can't open the image although it is able to work with the
> zlib compression type. For now, I don't understand why we should make
> such an artificial restriction.
We don't want to create such images, but we want to keep our code as
simple as possible.
As your patch shows, forbidding the case is more work than just allowing
it. So if external software can create such images, and it would just
automatically work in QEMU, then why do the extra work to articifially
forbid this?
(Actually, it's likely that on the first header update, QEMU would just
end up dropping the useless flag, so we even "fix" such images.)
> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> index 7ccbfff9d0..6aa8b99993 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)
> >> +#
> >> # Since: 1.7
> >> ##
> >> { 'struct': 'ImageInfoSpecificQCow2',
> >> @@ -89,7 +91,8 @@
> >> '*corrupt': 'bool',
> >> 'refcount-bits': 'int',
> >> '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> >> - '*bitmaps': ['Qcow2BitmapInfo']
> >> + '*bitmaps': ['Qcow2BitmapInfo'],
> >> + '*compression-type': 'Qcow2CompressionType'
> > Why is this field optional? Can't we always populate it, even for older
> > images?
> Why we should if we don't care ?
I was trying too check what the condition is under which the field will
be present in the output, but I couldn't find any code for it.
So it looks like this patch never makes use of the field and it's dead
code?
Kevin
On 03.07.2019 18:46, Kevin Wolf wrote:
> Am 03.07.2019 um 17:01 hat Denis Plotnikov geschrieben:
>> On 03.07.2019 17:14, Eric Blake wrote:
>>> On 7/3/19 6:00 AM, Denis Plotnikov wrote:
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index 3ace3b2209..921eb67b80 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -1202,6 +1202,47 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
>>>> return ret;
>>>> }
>>>>
>>>> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
>>>> +{
>>>> + bool is_set;
>>>> + int ret = 0;
>>>> +
>>>> + switch (s->compression_type) {
>>>> + case QCOW2_COMPRESSION_TYPE_ZLIB:
>>>> + break;
>>>> +
>>>> + default:
>>>> + if (errp) {
>>> Useless check for errp being non-NULL. What's worse, if the caller
>>> passes in NULL because they don't care about the error, then your code
>>> behaves differently. Just call error_setg() and return -ENOTSUP
>>> unconditionally.
>> ok
>>>
>>>> + error_setg(errp, "qcow2: unknown compression type: %u",
>>>> + s->compression_type);
>>>> + return -ENOTSUP;
>>>> + }
>>>> + }
>>>> +
>>>> + /*
>>>> + * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
>>>> + * feature flag must be absent, with other compression types the
>>>> + * incompatible feature flag must be set
>>> Is there a strong reason for forbid the incompatible feature flag with
>>> ZLIB?
>> The main reason is to guarantee image opening for older qemu if
>> compression type is zlib.
>>> Sure, it makes the image impossible to open with older qemu, but
>>> it doesn't get in the way of newer qemu opening it. I'll have to see how
>>> your spec changes documented this, to see if you could instead word it
>>> as 'for ZLIB, the incompatible feature flag may be absent'.
>> I just can't imagine when and why we would want to set incompatible
>> feature flag with zlib. Suppose we have zlib with the flag set, then
>> older qemu can't open the image although it is able to work with the
>> zlib compression type. For now, I don't understand why we should make
>> such an artificial restriction.
>
> We don't want to create such images, but we want to keep our code as
> simple as possible.
>
> As your patch shows, forbidding the case is more work than just allowing
> it. So if external software can create such images, and it would just
> automatically work in QEMU, then why do the extra work to articifially
> forbid this?
>
> (Actually, it's likely that on the first header update, QEMU would just
> end up dropping the useless flag, so we even "fix" such images.)
ok, removing the restriction
>
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 7ccbfff9d0..6aa8b99993 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)
>>>> +#
>>>> # Since: 1.7
>>>> ##
>>>> { 'struct': 'ImageInfoSpecificQCow2',
>>>> @@ -89,7 +91,8 @@
>>>> '*corrupt': 'bool',
>>>> 'refcount-bits': 'int',
>>>> '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>>>> - '*bitmaps': ['Qcow2BitmapInfo']
>>>> + '*bitmaps': ['Qcow2BitmapInfo'],
>>>> + '*compression-type': 'Qcow2CompressionType'
>>> Why is this field optional? Can't we always populate it, even for older
>>> images?
>> Why we should if we don't care ?
>
> I was trying too check what the condition is under which the field will
> be present in the output, but I couldn't find any code for it.
>
> So it looks like this patch never makes use of the field and it's dead
> code?
ok
>
> Kevin
>
--
Best,
Denis
On 7/3/19 10:01 AM, Denis Plotnikov wrote:
>>> + * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
>>> + * feature flag must be absent, with other compression types the
>>> + * incompatible feature flag must be set
>> Is there a strong reason for forbid the incompatible feature flag with
>> ZLIB?
> The main reason is to guarantee image opening for older qemu if
> compression type is zlib.
>> Sure, it makes the image impossible to open with older qemu, but
>> it doesn't get in the way of newer qemu opening it. I'll have to see how
>> your spec changes documented this, to see if you could instead word it
>> as 'for ZLIB, the incompatible feature flag may be absent'.
> I just can't imagine when and why we would want to set incompatible
> feature flag with zlib. Suppose we have zlib with the flag set, then
> older qemu can't open the image although it is able to work with the
> zlib compression type. For now, I don't understand why we should make
> such an artificial restriction.
We have an artificial restriction one way or the other.
Method 1 - artificial restriction that incompatible bit must NOT be set
when compression type is zlib
Method 2 - artificial restriction that older qcow2 programs can't open a
zlib image with incompatible bit set, even though removing the bit makes
the image more portable.
It's desirable that qemu should NOT set the incompatible bit when it
does not need to (for the sake of portability to more apps), but
MANDATING that it must not set the bit for zlib is a stronger spec
restriction.
I tend to lean for the spec being looser unless there is a strong reason
for why it must be strict; just because qemu won't be setting the
incompatible bit does not necessarily mean that all other
implementations will try to be that careful; they may have valid reasons
for setting the bit even when using zlib, but only if the spec permits
them to do so.
>>> @@ -2434,6 +2493,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);
>> Why are you ignoring the error here?
> qcow2_update_header() doesn't use the error and just returns an error
> code or 0
Are we potentially losing a valuable error message (in which case
qcow2_update_header should maybe be first patched to take an errp
parameter), or is it always going to succeed (in which case &error_abort
would document our intention that we know it can't fail), or is it
really a case where it may fail, but we don't care about losing the
message? Passing NULL is not wrong (as you say, we aren't plumbed to
pass the message back up to the caller), but does raise enough
suspicions as to be worth auditing the code while in the area.
>>> + 104 - 107: compression_type
>>> + 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.
>>> + The default compression type is zlib.
>> Where is the documentation that a value of 0 corresponds to zlib?
> Should I do it right here or it's better to add a separate chapter in
> the docs/interop/qcow2.txt ?
Right here.
>>> +++ 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)
>>> +#
>>> # Since: 1.7
>>> ##
>>> { 'struct': 'ImageInfoSpecificQCow2',
>>> @@ -89,7 +91,8 @@
>>> '*corrupt': 'bool',
>>> 'refcount-bits': 'int',
>>> '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>>> - '*bitmaps': ['Qcow2BitmapInfo']
>>> + '*bitmaps': ['Qcow2BitmapInfo'],
>>> + '*compression-type': 'Qcow2CompressionType'
>> Why is this field optional? Can't we always populate it, even for older
>> images?
> Why we should if we don't care ?
Because it shows that we are running a new enough qemu that knows about
the compression field (even when it is absent).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 03.07.2019 18:13, Eric Blake wrote:
> On 7/3/19 10:01 AM, Denis Plotnikov wrote:
>
>>>> + * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
>>>> + * feature flag must be absent, with other compression types the
>>>> + * incompatible feature flag must be set
>>> Is there a strong reason for forbid the incompatible feature flag with
>>> ZLIB?
>> The main reason is to guarantee image opening for older qemu if
>> compression type is zlib.
>>> Sure, it makes the image impossible to open with older qemu, but
>>> it doesn't get in the way of newer qemu opening it. I'll have to see how
>>> your spec changes documented this, to see if you could instead word it
>>> as 'for ZLIB, the incompatible feature flag may be absent'.
>> I just can't imagine when and why we would want to set incompatible
>> feature flag with zlib. Suppose we have zlib with the flag set, then
>> older qemu can't open the image although it is able to work with the
>> zlib compression type. For now, I don't understand why we should make
>> such an artificial restriction.
>
> We have an artificial restriction one way or the other.
>
> Method 1 - artificial restriction that incompatible bit must NOT be set
> when compression type is zlib
>
> Method 2 - artificial restriction that older qcow2 programs can't open a
> zlib image with incompatible bit set, even though removing the bit makes
> the image more portable.
>
> It's desirable that qemu should NOT set the incompatible bit when it
> does not need to (for the sake of portability to more apps), but
> MANDATING that it must not set the bit for zlib is a stronger spec
> restriction.
>
> I tend to lean for the spec being looser unless there is a strong reason
> for why it must be strict; just because qemu won't be setting the
> incompatible bit does not necessarily mean that all other
> implementations will try to be that careful; they may have valid reasons
> for setting the bit even when using zlib, but only if the spec permits
> them to do so.
So, how I should implement that? Method 1 or Method 2?
How we can decide? Ask what other maintainers think about that?
>
>
>>>> @@ -2434,6 +2493,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);
>>> Why are you ignoring the error here?
>> qcow2_update_header() doesn't use the error and just returns an error
>> code or 0
>
> Are we potentially losing a valuable error message (in which case
> qcow2_update_header should maybe be first patched to take an errp
> parameter), or is it always going to succeed (in which case &error_abort
> would document our intention that we know it can't fail), or is it
> really a case where it may fail, but we don't care about losing the
> message? Passing NULL is not wrong (as you say, we aren't plumbed to
> pass the message back up to the caller), but does raise enough
> suspicions as to be worth auditing the code while in the area.
Could we do it after adding this series since it already implemented
without the error?
>
>
>>>> + 104 - 107: compression_type
>>>> + 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.
>>>> + The default compression type is zlib.
>>> Where is the documentation that a value of 0 corresponds to zlib?
>> Should I do it right here or it's better to add a separate chapter in
>> the docs/interop/qcow2.txt ?
>
> Right here.
ok
>
>
>>>> +++ 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)
>>>> +#
>>>> # Since: 1.7
>>>> ##
>>>> { 'struct': 'ImageInfoSpecificQCow2',
>>>> @@ -89,7 +91,8 @@
>>>> '*corrupt': 'bool',
>>>> 'refcount-bits': 'int',
>>>> '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>>>> - '*bitmaps': ['Qcow2BitmapInfo']
>>>> + '*bitmaps': ['Qcow2BitmapInfo'],
>>>> + '*compression-type': 'Qcow2CompressionType'
>>> Why is this field optional? Can't we always populate it, even for older
>>> images?
>> Why we should if we don't care ?
>
> Because it shows that we are running a new enough qemu that knows about
> the compression field (even when it is absent).
ok, if everybody agree with that I'll implement whatever is better
>
--
Best,
Denis
© 2016 - 2026 Red Hat, Inc.