* [Qemu-devel] [PATCH v0 0/3] add zstd cluster compression
@ 2019-05-28 14:37 Denis Plotnikov
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature Denis Plotnikov
` (3 more replies)
0 siblings, 4 replies; 29+ messages in thread
From: Denis Plotnikov @ 2019-05-28 14:37 UTC (permalink / raw)
To: kwolf, mreitz, eblake, armbru, qemu-block; +Cc: vsementsov, qemu-devel, den
The goal of the patch-set is to enable qcow2 to use zstd compression for
clusters. ZSTD provides better (de)compression performance than currently
used ZLIB. Using it will improve perforamnce (reduce compression time)
when the compressed clusters is used, e.g backup scenarios.
Also, the patch-set extends qcow2 specification by adding compression_type
feature. The feature enables adding ZSTD and another compression algorithms
in the future.
Here is some measurements ZSTD vs ZLIB:
The test:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G
The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.
compress cmd:
time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
src.img [zlib|zstd]_compressed.img
decompress cmd
time ./qemu-img convert -O qcow2
[zlib|zstd]_compressed.img uncompressed.img
The results:
compression decompression
zlib zstd zlib zstd
------------------------------------------------------------
real 65.5 16.3 (-75 %) 1.9 1.6 (-16 %)
user 65.0 15.8 5.3 2.5
sys 3.3 0.2 2.0 2.0
Both ZLIB and ZSTD gave the same compression ratio: ~1.5
compressed image size in both cases: ~1.4G
Denis Plotnikov (3):
qcow2: introduce compression type feature
qcow2: add compression type processing
qcow2: add zstd cluster compression
block/qcow2.c | 240 ++++++++++++++++++++++++++++++++++++--
block/qcow2.h | 29 +++--
configure | 26 +++++
docs/interop/qcow2.txt | 37 +++++-
include/block/block_int.h | 1 +
qapi/block-core.json | 34 +++++-
6 files changed, 348 insertions(+), 19 deletions(-)
--
2.17.0
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature
2019-05-28 14:37 [Qemu-devel] [PATCH v0 0/3] add zstd cluster compression Denis Plotnikov
@ 2019-05-28 14:37 ` Denis Plotnikov
2019-05-29 11:40 ` Vladimir Sementsov-Ogievskiy
` (3 more replies)
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing Denis Plotnikov
` (2 subsequent siblings)
3 siblings, 4 replies; 29+ messages in thread
From: Denis Plotnikov @ 2019-05-28 14:37 UTC (permalink / raw)
To: kwolf, mreitz, eblake, armbru, qemu-block; +Cc: vsementsov, qemu-devel, den
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 | 61 +++++++++++++++++++++++++++++++++++++++
block/qcow2.h | 29 ++++++++++++++-----
docs/interop/qcow2.txt | 37 +++++++++++++++++++++++-
include/block/block_int.h | 1 +
qapi/block-core.json | 34 ++++++++++++++++++++--
5 files changed, 151 insertions(+), 11 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 3ace3b2209..c4b5b93408 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -74,6 +74,7 @@ typedef struct {
#define QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
#define QCOW2_EXT_MAGIC_BITMAPS 0x23852875
#define QCOW2_EXT_MAGIC_DATA_FILE 0x44415441
+#define QCOW2_EXT_MAGIC_COMPRESSION_TYPE 0x434D5052
static int coroutine_fn
qcow2_co_preadv_compressed(BlockDriverState *bs,
@@ -398,6 +399,13 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
#endif
break;
+ case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
+ /*
+ * Setting compression type to BDRVQcow2State->compression_type
+ * from the image header is going to be here
+ */
+ break;
+
case QCOW2_EXT_MAGIC_DATA_FILE:
{
s->image_data_file = g_malloc0(ext.len + 1);
@@ -2553,6 +2561,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,
@@ -2583,6 +2596,22 @@ int qcow2_update_header(BlockDriverState *bs)
buflen -= ret;
}
+ /* Compression type extension */
+ if (s->compression_type != 0) {
+ Qcow2CompressionTypeExt comp_header = {
+ .compression_type = cpu_to_be32(s->compression_type),
+ };
+ ret = header_ext_add(buf, QCOW2_EXT_MAGIC_COMPRESSION_TYPE,
+ &comp_header,
+ sizeof(comp_header),
+ buflen);
+ if (ret < 0) {
+ goto fail;
+ }
+ buf += ret;
+ buflen -= ret;
+ }
+
/* Keep unknown header extensions */
QLIST_FOREACH(uext, &s->unknown_header_ext, next) {
ret = header_ext_add(buf, uext->magic, uext->data, uext->len, buflen);
@@ -3184,6 +3213,29 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
s->image_data_file = g_strdup(data_bs->filename);
}
+ /*
+ * default compression type is ZLIB: 0
+ * to apply it, there is no need for any header modification
+ */
+ if (qcow2_opts->has_compression_type &&
+ qcow2_opts->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB) {
+
+ BDRVQcow2State *s = blk_bs(blk)->opaque;
+
+ /*
+ * check for known compression types
+ * ZLIB shouldn't be here since it's the default
+ */
+ switch (qcow2_opts->compression_type) {
+ default:
+ error_setg_errno(errp, -EINVAL, "Unknown compression type");
+ goto out;
+ }
+ s->incompatible_features |= QCOW2_INCOMPAT_COMPRESSION_TYPE;
+ s->compression_type = qcow2_opts->compression_type;
+ }
+
+
/* Create a full header (including things like feature table) */
ret = qcow2_update_header(blk_bs(blk));
if (ret < 0) {
@@ -3307,6 +3359,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 },
};
@@ -4675,6 +4728,8 @@ 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),
+ .has_compression_type = true,
+ .compression_type = s->compression_type
};
} else {
/* if this assertion fails, this probably means a new version was
@@ -5239,6 +5294,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..b70da3138d 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -198,16 +198,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 */
@@ -263,6 +267,10 @@ typedef struct Qcow2BitmapHeaderExt {
uint64_t bitmap_directory_offset;
} QEMU_PACKED Qcow2BitmapHeaderExt;
+typedef struct Qcow2CompressionTypeExt {
+ uint32_t compression_type;
+} QEMU_PACKED Qcow2CompressionTypeExt;
+
typedef struct BDRVQcow2State {
int cluster_bits;
int cluster_size;
@@ -350,6 +358,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..cebcbc4f2f 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -109,7 +109,14 @@ 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. If the bit is set, then the
+ type of compression the image uses is set in the
+ header extension. When the bit is set the
+ compression type extension header must be present.
+ When the bit is not set the compression type
+ header must absent.
+
+ Bits 4-63: Reserved (set to 0)
80 - 87: compatible_features
Bitmask of compatible features. An implementation can
@@ -175,6 +182,7 @@ be stored. Each extension has a structure like the following:
0x23852875 - Bitmaps extension
0x0537be77 - Full disk encryption header pointer
0x44415441 - External data file name string
+ 0x434D5052 - Compression type extension
other - Unknown header extension, can be safely
ignored
@@ -771,3 +779,30 @@ In the image file the 'enabled' state is reflected by the 'auto' flag. If this
flag is set, the software must consider the bitmap as 'enabled' and start
tracking virtual disk changes to this bitmap from the first write to the
virtual disk. If this flag is not set then the bitmap is disabled.
+
+
+== Compression type extension ==
+
+The compression type extension is an optional header extension. It stores the
+compression type used for disk clusters (de)compression.
+A single compression type is applied to all compressed disk clusters,
+with no way to change compression types per cluster. Two clusters of the image
+couldn't be compressed with different compression types.
+
+The compression type is set on image creation. The only way to change
+the compression type is to convert the image explicitly.
+
+The compression type extension is present if and only if the incompatible
+compression type bit is set. When the bit is not set the compression type
+header must be absent.
+
+When the compression type bit is not set and the compression type header
+extension is absent, ZLIB compression is used for compressed clusters.
+This defines default image compression type: ZLIB.
+Qemu < 4.1 can use images created with compression type ZLIB without any
+additional preparations and cannot use images created with compression
+types != ZLIB.
+
+Available compression types:
+ 0: ZLIB
+ 1: ZSTD
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..59610153fd 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -78,6 +78,9 @@
#
# @bitmaps: A list of qcow2 bitmap details (since 4.0)
#
+# @compression-type: the compression method used for image clusters
+# compression (since 4.1)
+#
# Since: 1.7
##
{ 'struct': 'ImageInfoSpecificQCow2',
@@ -89,7 +92,8 @@
'*corrupt': 'bool',
'refcount-bits': 'int',
'*encrypt': 'ImageInfoSpecificQCow2Encryption',
- '*bitmaps': ['Qcow2BitmapInfo']
+ '*bitmaps': ['Qcow2BitmapInfo'],
+ '*compression-type': 'Qcow2CompressionType'
} }
##
@@ -3119,6 +3123,10 @@
# an image, the data file name is loaded from the image
# file. (since 4.0)
#
+# @compression-type: compression method to use for image clusters compression
+# The comression method is set on image creation and can
+# be changed via image converting only. (since 4.1)
+#
# Since: 2.9
##
{ 'struct': 'BlockdevOptionsQcow2',
@@ -3134,7 +3142,8 @@
'*refcount-cache-size': 'int',
'*cache-clean-interval': 'int',
'*encrypt': 'BlockdevQcow2Encryption',
- '*data-file': 'BlockdevRef' } }
+ '*data-file': 'BlockdevRef',
+ '*compression-type': 'Qcow2CompressionType' } }
##
# @SshHostKeyCheckMode:
@@ -4206,6 +4215,19 @@
'data': [ 'v2', 'v3' ] }
+##
+# @Qcow2CompressionType:
+#
+# Compression type used in qcow2 image file
+#
+# @zlib : gzip compressor
+# @zstd : zstd compression
+#
+# Since: 4.1
+##
+{ 'enum': 'Qcow2CompressionType',
+ 'data': [ 'zlib', 'zstd' ] }
+
##
# @BlockdevCreateOptionsQcow2:
#
@@ -4228,6 +4250,11 @@
# @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 Compression method used for image compressed clusters
+# (default: zlib(gzip), since 4.1).
+# Available types:
+# zlib
+# zstd
#
# Since: 2.12
##
@@ -4243,7 +4270,8 @@
'*cluster-size': 'size',
'*preallocation': 'PreallocMode',
'*lazy-refcounts': 'bool',
- '*refcount-bits': 'int' } }
+ '*refcount-bits': 'int',
+ '*compression-type': 'Qcow2CompressionType' } }
##
# @BlockdevCreateOptionsQed:
--
2.17.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing
2019-05-28 14:37 [Qemu-devel] [PATCH v0 0/3] add zstd cluster compression Denis Plotnikov
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature Denis Plotnikov
@ 2019-05-28 14:37 ` Denis Plotnikov
2019-05-29 9:47 ` Vladimir Sementsov-Ogievskiy
2019-06-28 10:23 ` Kevin Wolf
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 3/3] qcow2: add zstd cluster compression Denis Plotnikov
2019-06-04 7:56 ` [Qemu-devel] [PING] [PATCH v0 0/3] " Denis Plotnikov
3 siblings, 2 replies; 29+ messages in thread
From: Denis Plotnikov @ 2019-05-28 14:37 UTC (permalink / raw)
To: kwolf, mreitz, eblake, armbru, qemu-block; +Cc: vsementsov, qemu-devel, den
With the patch, qcow2 is able to process image compression type
defined in the image header and choose the corresponding method
for clusters compressing.
Also, it rework the cluster compression code for adding more
compression types.
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
block/qcow2.c | 103 ++++++++++++++++++++++++++++++++++++++++++++------
1 file changed, 92 insertions(+), 11 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index c4b5b93408..90f15cc3c9 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -400,11 +400,39 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
break;
case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
+ /* Compression type always goes with the compression type bit set */
+ if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
+ error_setg(errp,
+ "compression_type_ext: "
+ "expect compression type bit set");
+ return -EINVAL;
+ }
+
+ ret = bdrv_pread(bs->file, offset, &s->compression_type, ext.len);
+ s->compression_type = be32_to_cpu(s->compression_type);
+
+ if (ret < 0) {
+ error_setg_errno(errp, -ret,
+ "ERROR: Could not read compression type");
+ return ret;
+ }
+
/*
- * Setting compression type to BDRVQcow2State->compression_type
- * from the image header is going to be here
+ * The default compression type is not allowed when the extension
+ * is present. ZLIB is used as the default compression type.
+ * When compression type extension header is present then
+ * compression_type should have a value different from the default.
*/
- break;
+ if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
+ error_setg(errp,
+ "compression_type_ext:"
+ "invalid compression type %d",
+ QCOW2_COMPRESSION_TYPE_ZLIB);
+ }
+#ifdef DEBUG_EXT
+ printf("Qcow2: image compression type %s\n", s->compression_type);
+#endif
+ break;
case QCOW2_EXT_MAGIC_DATA_FILE:
{
@@ -1492,6 +1520,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
QLIST_INIT(&s->cluster_allocs);
QTAILQ_INIT(&s->discards);
+ /* Set default compression type */
+ s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
+
/* read qcow2 extensions */
if (qcow2_read_extensions(bs, header.header_length, ext_end, NULL,
flags, &update_header, &local_err)) {
@@ -1500,6 +1531,34 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
goto fail;
}
+ /*
+ * The compression type is set on the extension header processing
+ * if the compression type extension header is present.
+ * When the compression type is different from ZLIB (default) there
+ * should be both the compression type bit and the compression
+ * type extension header set. When the ZLIB (default) compression
+ * type is used there should be neither the compression type bit nor
+ * the compression type extension header set.
+ */
+
+ if ((s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) &
+ (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB)) {
+ error_setg(errp, "Illegal compression type setting");
+ ret = -EINVAL;
+ goto fail;
+ }
+
+ /* Check available compression types */
+ switch (s->compression_type) {
+ case QCOW2_COMPRESSION_TYPE_ZLIB:
+ break;
+
+ default:
+ error_setg(errp, "Unknown compression type");
+ ret = -EINVAL;
+ goto fail;
+ }
+
/* Open external data file */
s->data_file = bdrv_open_child(NULL, options, "data-file", bs, &child_file,
true, &local_err);
@@ -3970,7 +4029,7 @@ fail:
}
/*
- * qcow2_compress()
+ * zlib_compress()
*
* @dest - destination buffer, @dest_size bytes
* @src - source buffer, @src_size bytes
@@ -3979,7 +4038,7 @@ fail:
* -1 destination buffer is not enough to store compressed data
* -2 on any other error
*/
-static ssize_t qcow2_compress(void *dest, size_t dest_size,
+static ssize_t zlib_compress(void *dest, size_t dest_size,
const void *src, size_t src_size)
{
ssize_t ret;
@@ -4013,7 +4072,7 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
}
/*
- * qcow2_decompress()
+ * zlib_decompress()
*
* Decompress some data (not more than @src_size bytes) to produce exactly
* @dest_size bytes.
@@ -4024,7 +4083,7 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
* Returns: 0 on success
* -1 on fail
*/
-static ssize_t qcow2_decompress(void *dest, size_t dest_size,
+static ssize_t zlib_decompress(void *dest, size_t dest_size,
const void *src, size_t src_size)
{
int ret = 0;
@@ -4122,16 +4181,38 @@ static ssize_t coroutine_fn
qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
const void *src, size_t src_size)
{
- return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
- qcow2_compress);
+ BDRVQcow2State *s = bs->opaque;
+ Qcow2CompressFunc fn;
+
+ switch (s->compression_type) {
+ case QCOW2_COMPRESSION_TYPE_ZLIB:
+ fn = zlib_compress;
+ break;
+
+ default:
+ return -ENOTSUP;
+ }
+
+ return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
}
static ssize_t coroutine_fn
qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
const void *src, size_t src_size)
{
- return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
- qcow2_decompress);
+ BDRVQcow2State *s = bs->opaque;
+ Qcow2CompressFunc fn;
+
+ switch (s->compression_type) {
+ case QCOW2_COMPRESSION_TYPE_ZLIB:
+ fn = zlib_decompress;
+ break;
+
+ default:
+ return -ENOTSUP;
+ }
+
+ return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
}
/* XXX: put compressed sectors first, then all the cluster aligned
--
2.17.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* [Qemu-devel] [PATCH v0 3/3] qcow2: add zstd cluster compression
2019-05-28 14:37 [Qemu-devel] [PATCH v0 0/3] add zstd cluster compression Denis Plotnikov
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature Denis Plotnikov
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing Denis Plotnikov
@ 2019-05-28 14:37 ` Denis Plotnikov
2019-06-28 11:57 ` Kevin Wolf
2019-06-04 7:56 ` [Qemu-devel] [PING] [PATCH v0 0/3] " Denis Plotnikov
3 siblings, 1 reply; 29+ messages in thread
From: Denis Plotnikov @ 2019-05-28 14:37 UTC (permalink / raw)
To: kwolf, mreitz, eblake, armbru, qemu-block; +Cc: vsementsov, qemu-devel, den
zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of compression ratio in comparison with
zlib, which, by the moment, has been the only compression
method available.
The performance test results:
Test compresses and decompresses qemu qcow2 image with just
installed rhel-7.6 guest.
Image cluster size: 64K. Image on disk size: 2.2G
The test was conducted with brd disk to reduce the influence
of disk subsystem to the test results.
The results is given in seconds.
compress cmd:
time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
src.img [zlib|zstd]_compressed.img
decompress cmd
time ./qemu-img convert -O qcow2
[zlib|zstd]_compressed.img uncompressed.img
compression decompression
zlib zstd zlib zstd
------------------------------------------------------------
real 65.5 16.3 (-75 %) 1.9 1.6 (-16 %)
user 65.0 15.8 5.3 2.5
sys 3.3 0.2 2.0 2.0
Both ZLIB and ZSTD gave the same compression ratio: 1.57
compressed image size in both cases: 1.4G
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
block/qcow2.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++
configure | 26 ++++++++++++++++
2 files changed, 108 insertions(+)
diff --git a/block/qcow2.c b/block/qcow2.c
index 90f15cc3c9..58901f9f79 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -26,6 +26,7 @@
#define ZLIB_CONST
#include <zlib.h>
+#include <zstd.h>
#include "block/block_int.h"
#include "block/qdict.h"
@@ -1553,6 +1554,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
case QCOW2_COMPRESSION_TYPE_ZLIB:
break;
+ case QCOW2_COMPRESSION_TYPE_ZSTD:
+ break;
+
default:
error_setg(errp, "Unknown compression type");
ret = -EINVAL;
@@ -3286,6 +3290,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
* ZLIB shouldn't be here since it's the default
*/
switch (qcow2_opts->compression_type) {
+ case QCOW2_COMPRESSION_TYPE_ZSTD:
+ break;
+
default:
error_setg_errno(errp, -EINVAL, "Unknown compression type");
goto out;
@@ -4113,6 +4120,73 @@ static ssize_t zlib_decompress(void *dest, size_t dest_size,
return ret;
}
+/*
+ * zstd_compress()
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ * -1 on any error
+ */
+
+static ssize_t zstd_compress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
+{
+ /* steal some bytes to store compressed chunk size */
+ size_t ret;
+ size_t *c_size = dest;
+ char *d_buf = dest;
+ d_buf += sizeof(ret);
+ dest_size -= sizeof(ret);
+
+ ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
+
+ if (ZSTD_isError(ret)) {
+ return -1;
+ }
+
+ /* store the compressed chunk size in the very beginning of the buffer */
+ *c_size = ret;
+
+ return ret + sizeof(ret);
+}
+
+/*
+ * zstd_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes.
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ * -1 on fail
+ */
+
+static ssize_t zstd_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
+{
+ size_t ret;
+ /*
+ * zstd decompress wants to know the exact lenght of the data
+ * for that purpose, zstd_compress stores the length in the
+ * very beginning of the compressed buffer
+ */
+ const size_t *s_size = src;
+ const char *s_buf = src;
+ s_buf += sizeof(size_t);
+
+ ret = ZSTD_decompress(dest, dest_size, s_buf, *s_size);
+
+ if (ZSTD_isError(ret)) {
+ return -1;
+ }
+
+ return 0;
+}
+
#define MAX_COMPRESS_THREADS 4
typedef ssize_t (*Qcow2CompressFunc)(void *dest, size_t dest_size,
@@ -4189,6 +4263,10 @@ qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
fn = zlib_compress;
break;
+ case QCOW2_COMPRESSION_TYPE_ZSTD:
+ fn = zstd_compress;
+ break;
+
default:
return -ENOTSUP;
}
@@ -4208,6 +4286,10 @@ qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
fn = zlib_decompress;
break;
+ case QCOW2_COMPRESSION_TYPE_ZSTD:
+ fn = zstd_decompress;
+ break;
+
default:
return -ENOTSUP;
}
diff --git a/configure b/configure
index 1c563a7027..c90716189c 100755
--- a/configure
+++ b/configure
@@ -433,6 +433,7 @@ opengl_dmabuf="no"
cpuid_h="no"
avx2_opt=""
zlib="yes"
+zstd="yes"
capstone=""
lzo=""
snappy=""
@@ -1317,6 +1318,8 @@ for opt do
;;
--disable-zlib-test) zlib="no"
;;
+ --disable-zstd-test) zstd="no"
+ ;;
--disable-lzo) lzo="no"
;;
--enable-lzo) lzo="yes"
@@ -3702,6 +3705,29 @@ EOF
fi
fi
+#########################################
+# zstd check
+
+if test "$zstd" != "no" ; then
+ if $pkg_config --exists libzstd; then
+ zstd_cflags=$($pkg_config --cflags libzstd)
+ zstd_libs=$($pkg_config --libs libzstd)
+ QEMU_CFLAGS="$zstd_cflags $QEMU_CFLAGS"
+ LIBS="$zstd_libs $LIBS"
+ else
+ cat > $TMPC << EOF
+#include <zstd.h>
+int main(void) { ZSTD_versionNumber(); return 0; }
+EOF
+ if compile_prog "" "-lzstd" ; then
+ LIBS="$LIBS -lzstd"
+ else
+ error_exit "zstd check failed" \
+ "Make sure to have the zstd libs and headers installed."
+ fi
+ fi
+fi
+
##########################################
# SHA command probe for modules
if test "$modules" = yes; then
--
2.17.0
^ permalink raw reply related [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing Denis Plotnikov
@ 2019-05-29 9:47 ` Vladimir Sementsov-Ogievskiy
2019-06-28 10:23 ` Kevin Wolf
1 sibling, 0 replies; 29+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-05-29 9:47 UTC (permalink / raw)
To: Denis Plotnikov, kwolf, mreitz, eblake, armbru, qemu-block
Cc: qemu-devel, Denis Lunev
28.05.2019 17:37, Denis Plotnikov wrote:
> With the patch, qcow2 is able to process image compression type
> defined in the image header and choose the corresponding method
> for clusters compressing.
>
> Also, it rework the cluster compression code for adding more
> compression types.
>
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
> block/qcow2.c | 103 ++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 92 insertions(+), 11 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c4b5b93408..90f15cc3c9 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -400,11 +400,39 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> break;
>
> case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
> + /* Compression type always goes with the compression type bit set */
> + if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
> + error_setg(errp,
> + "compression_type_ext: "
> + "expect compression type bit set");
> + return -EINVAL;
> + }
> +
> + ret = bdrv_pread(bs->file, offset, &s->compression_type, ext.len);
> + s->compression_type = be32_to_cpu(s->compression_type);
> +
> + if (ret < 0) {
> + error_setg_errno(errp, -ret,
> + "ERROR: Could not read compression type");
> + return ret;
> + }
> +
> /*
> - * Setting compression type to BDRVQcow2State->compression_type
> - * from the image header is going to be here
> + * The default compression type is not allowed when the extension
> + * is present. ZLIB is used as the default compression type.
> + * When compression type extension header is present then
> + * compression_type should have a value different from the default.
> */
> - break;
> + if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
> + error_setg(errp,
> + "compression_type_ext:"
> + "invalid compression type %d",
> + QCOW2_COMPRESSION_TYPE_ZLIB);
> + }
> +#ifdef DEBUG_EXT
> + printf("Qcow2: image compression type %s\n", s->compression_type);
> +#endif
> + break;
>
> case QCOW2_EXT_MAGIC_DATA_FILE:
> {
> @@ -1492,6 +1520,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> QLIST_INIT(&s->cluster_allocs);
> QTAILQ_INIT(&s->discards);
>
> + /* Set default compression type */
> + s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
> +
> /* read qcow2 extensions */
> if (qcow2_read_extensions(bs, header.header_length, ext_end, NULL,
> flags, &update_header, &local_err)) {
> @@ -1500,6 +1531,34 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> goto fail;
> }
>
> + /*
> + * The compression type is set on the extension header processing
> + * if the compression type extension header is present.
> + * When the compression type is different from ZLIB (default) there
> + * should be both the compression type bit and the compression
> + * type extension header set. When the ZLIB (default) compression
> + * type is used there should be neither the compression type bit nor
> + * the compression type extension header set.
> + */
> +
> + if ((s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) &
> + (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB)) {
> + error_setg(errp, "Illegal compression type setting");
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + /* Check available compression types */
> + switch (s->compression_type) {
> + case QCOW2_COMPRESSION_TYPE_ZLIB:
> + break;
> +
> + default:
> + error_setg(errp, "Unknown compression type");
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> /* Open external data file */
> s->data_file = bdrv_open_child(NULL, options, "data-file", bs, &child_file,
> true, &local_err);
> @@ -3970,7 +4029,7 @@ fail:
> }
>
> /*
> - * qcow2_compress()
> + * zlib_compress()
> *
> * @dest - destination buffer, @dest_size bytes
> * @src - source buffer, @src_size bytes
> @@ -3979,7 +4038,7 @@ fail:
> * -1 destination buffer is not enough to store compressed data
> * -2 on any other error
> */
> -static ssize_t qcow2_compress(void *dest, size_t dest_size,
> +static ssize_t zlib_compress(void *dest, size_t dest_size,
> const void *src, size_t src_size)
> {
> ssize_t ret;
> @@ -4013,7 +4072,7 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
> }
>
> /*
> - * qcow2_decompress()
> + * zlib_decompress()
> *
> * Decompress some data (not more than @src_size bytes) to produce exactly
> * @dest_size bytes.
> @@ -4024,7 +4083,7 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
> * Returns: 0 on success
> * -1 on fail
> */
> -static ssize_t qcow2_decompress(void *dest, size_t dest_size,
> +static ssize_t zlib_decompress(void *dest, size_t dest_size,
> const void *src, size_t src_size)
> {
> int ret = 0;
> @@ -4122,16 +4181,38 @@ static ssize_t coroutine_fn
> qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
> const void *src, size_t src_size)
> {
> - return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
> - qcow2_compress);
> + BDRVQcow2State *s = bs->opaque;
> + Qcow2CompressFunc fn;
> +
> + switch (s->compression_type) {
> + case QCOW2_COMPRESSION_TYPE_ZLIB:
> + fn = zlib_compress;
> + break;
> +
> + default:
> + return -ENOTSUP;
> + }
> +
> + return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
> }
>
> static ssize_t coroutine_fn
> qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
> const void *src, size_t src_size)
> {
> - return qcow2_co_do_compress(bs, dest, dest_size, src, src_size,
> - qcow2_decompress);
> + BDRVQcow2State *s = bs->opaque;
> + Qcow2CompressFunc fn;
> +
> + switch (s->compression_type) {
> + case QCOW2_COMPRESSION_TYPE_ZLIB:
> + fn = zlib_decompress;
> + break;
> +
> + default:
> + return -ENOTSUP;
> + }
> +
> + return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
> }
>
> /* XXX: put compressed sectors first, then all the cluster aligned
>
These things (compression) are moved to separate file by my patches, staged in Max's block branch:
https://git.xanclic.moe/XanClic/qemu/commits/branch/block
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature Denis Plotnikov
@ 2019-05-29 11:40 ` Vladimir Sementsov-Ogievskiy
2019-06-28 9:45 ` Kevin Wolf
2019-06-27 16:35 ` Markus Armbruster
` (2 subsequent siblings)
3 siblings, 1 reply; 29+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2019-05-29 11:40 UTC (permalink / raw)
To: Denis Plotnikov, kwolf, mreitz, eblake, armbru, qemu-block
Cc: qemu-devel, Denis Lunev
28.05.2019 17:37, 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
> 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..cebcbc4f2f 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -109,7 +109,14 @@ 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. If the bit is set, then the
> + type of compression the image uses is set in the
> + header extension. When the bit is set the
> + compression type extension header must be present.
> + When the bit is not set the compression type
> + header must absent.
> +
> + Bits 4-63: Reserved (set to 0)
>
> 80 - 87: compatible_features
> Bitmask of compatible features. An implementation can
> @@ -175,6 +182,7 @@ be stored. Each extension has a structure like the following:
> 0x23852875 - Bitmaps extension
> 0x0537be77 - Full disk encryption header pointer
> 0x44415441 - External data file name string
> + 0x434D5052 - Compression type extension
> other - Unknown header extension, can be safely
> ignored
>
> @@ -771,3 +779,30 @@ In the image file the 'enabled' state is reflected by the 'auto' flag. If this
> flag is set, the software must consider the bitmap as 'enabled' and start
> tracking virtual disk changes to this bitmap from the first write to the
> virtual disk. If this flag is not set then the bitmap is disabled.
> +
> +
> +== Compression type extension ==
> +
> +The compression type extension is an optional header extension. It stores the
> +compression type used for disk clusters (de)compression.
> +A single compression type is applied to all compressed disk clusters,
> +with no way to change compression types per cluster. Two clusters of the image
> +couldn't be compressed with different compression types.
> +
> +The compression type is set on image creation. The only way to change
> +the compression type is to convert the image explicitly.
> +
> +The compression type extension is present if and only if the incompatible
> +compression type bit is set. When the bit is not set the compression type
> +header must be absent.
Hmm, not the first time we introduce incompatible bit to make incompatible
header extension, as all header extensions are compatible by default, as for
unknown header extension we have:
other - Unknown header extension, can be safely
ignored
Hm. Should we instead define one incompatible bit for all such future cases,
i.e. add incompatible bit HEADER_EXTENSION_FLAGS, add flag field to header
extension declaration, and define one flag in it: COMPATIBLE, which will mean,
that this extension may be safely ignored (old default behavior).
> +
> +When the compression type bit is not set and the compression type header
> +extension is absent, ZLIB compression is used for compressed clusters.
> +This defines default image compression type: ZLIB.
> +Qemu < 4.1 can use images created with compression type ZLIB without any
> +additional preparations and cannot use images created with compression
> +types != ZLIB.
> +
> +Available compression types:
> + 0: ZLIB
> + 1: ZSTD
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PING] [PATCH v0 0/3] add zstd cluster compression
2019-05-28 14:37 [Qemu-devel] [PATCH v0 0/3] add zstd cluster compression Denis Plotnikov
` (2 preceding siblings ...)
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 3/3] qcow2: add zstd cluster compression Denis Plotnikov
@ 2019-06-04 7:56 ` Denis Plotnikov
2019-06-27 15:04 ` [Qemu-devel] [PING PING] " Denis Plotnikov
3 siblings, 1 reply; 29+ messages in thread
From: Denis Plotnikov @ 2019-06-04 7:56 UTC (permalink / raw)
To: kwolf, mreitz, eblake, armbru, qemu-block
Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, Denis Lunev
On 28.05.2019 17:37, Denis Plotnikov wrote:
> The goal of the patch-set is to enable qcow2 to use zstd compression for
> clusters. ZSTD provides better (de)compression performance than currently
> used ZLIB. Using it will improve perforamnce (reduce compression time)
> when the compressed clusters is used, e.g backup scenarios.
>
> Also, the patch-set extends qcow2 specification by adding compression_type
> feature. The feature enables adding ZSTD and another compression algorithms
> in the future.
>
> Here is some measurements ZSTD vs ZLIB:
>
> The test:
> Test compresses and decompresses qemu qcow2 image with just
> installed rhel-7.6 guest.
> Image cluster size: 64K. Image on disk size: 2.2G
>
> The test was conducted with brd disk to reduce the influence
> of disk subsystem to the test results.
> The results is given in seconds.
>
> compress cmd:
> time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
> src.img [zlib|zstd]_compressed.img
> decompress cmd
> time ./qemu-img convert -O qcow2
> [zlib|zstd]_compressed.img uncompressed.img
>
>
> The results:
> compression decompression
> zlib zstd zlib zstd
> ------------------------------------------------------------
> real 65.5 16.3 (-75 %) 1.9 1.6 (-16 %)
> user 65.0 15.8 5.3 2.5
> sys 3.3 0.2 2.0 2.0
>
> Both ZLIB and ZSTD gave the same compression ratio: ~1.5
> compressed image size in both cases: ~1.4G
>
> Denis Plotnikov (3):
> qcow2: introduce compression type feature
> qcow2: add compression type processing
> qcow2: add zstd cluster compression
>
> block/qcow2.c | 240 ++++++++++++++++++++++++++++++++++++--
> block/qcow2.h | 29 +++--
> configure | 26 +++++
> docs/interop/qcow2.txt | 37 +++++-
> include/block/block_int.h | 1 +
> qapi/block-core.json | 34 +++++-
> 6 files changed, 348 insertions(+), 19 deletions(-)
>
--
Best,
Denis
^ permalink raw reply [flat|nested] 29+ messages in thread
* [Qemu-devel] [PING PING] [PATCH v0 0/3] add zstd cluster compression
2019-06-04 7:56 ` [Qemu-devel] [PING] [PATCH v0 0/3] " Denis Plotnikov
@ 2019-06-27 15:04 ` Denis Plotnikov
0 siblings, 0 replies; 29+ messages in thread
From: Denis Plotnikov @ 2019-06-27 15:04 UTC (permalink / raw)
To: kwolf, mreitz, eblake, armbru, qemu-block
Cc: Vladimir Sementsov-Ogievskiy, qemu-devel, Denis Lunev
On 04.06.2019 10:56, Denis Plotnikov wrote:
>
>
> On 28.05.2019 17:37, Denis Plotnikov wrote:
>> The goal of the patch-set is to enable qcow2 to use zstd compression for
>> clusters. ZSTD provides better (de)compression performance than currently
>> used ZLIB. Using it will improve perforamnce (reduce compression time)
>> when the compressed clusters is used, e.g backup scenarios.
>>
>> Also, the patch-set extends qcow2 specification by adding compression_type
>> feature. The feature enables adding ZSTD and another compression algorithms
>> in the future.
>>
>> Here is some measurements ZSTD vs ZLIB:
>>
>> The test:
>> Test compresses and decompresses qemu qcow2 image with just
>> installed rhel-7.6 guest.
>> Image cluster size: 64K. Image on disk size: 2.2G
>>
>> The test was conducted with brd disk to reduce the influence
>> of disk subsystem to the test results.
>> The results is given in seconds.
>>
>> compress cmd:
>> time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
>> src.img [zlib|zstd]_compressed.img
>> decompress cmd
>> time ./qemu-img convert -O qcow2
>> [zlib|zstd]_compressed.img uncompressed.img
>>
>>
>> The results:
>> compression decompression
>> zlib zstd zlib zstd
>> ------------------------------------------------------------
>> real 65.5 16.3 (-75 %) 1.9 1.6 (-16 %)
>> user 65.0 15.8 5.3 2.5
>> sys 3.3 0.2 2.0 2.0
>>
>> Both ZLIB and ZSTD gave the same compression ratio: ~1.5
>> compressed image size in both cases: ~1.4G
>>
>> Denis Plotnikov (3):
>> qcow2: introduce compression type feature
>> qcow2: add compression type processing
>> qcow2: add zstd cluster compression
>>
>> block/qcow2.c | 240 ++++++++++++++++++++++++++++++++++++--
>> block/qcow2.h | 29 +++--
>> configure | 26 +++++
>> docs/interop/qcow2.txt | 37 +++++-
>> include/block/block_int.h | 1 +
>> qapi/block-core.json | 34 +++++-
>> 6 files changed, 348 insertions(+), 19 deletions(-)
>>
>
--
Best,
Denis
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature Denis Plotnikov
2019-05-29 11:40 ` Vladimir Sementsov-Ogievskiy
@ 2019-06-27 16:35 ` Markus Armbruster
2019-06-28 9:54 ` Kevin Wolf
2019-06-28 10:10 ` Kevin Wolf
3 siblings, 0 replies; 29+ messages in thread
From: Markus Armbruster @ 2019-06-27 16:35 UTC (permalink / raw)
To: Denis Plotnikov; +Cc: kwolf, vsementsov, den, qemu-block, qemu-devel, mreitz
Doc & QAPI schema review only.
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>
> ---
> block/qcow2.c | 61 +++++++++++++++++++++++++++++++++++++++
> block/qcow2.h | 29 ++++++++++++++-----
> docs/interop/qcow2.txt | 37 +++++++++++++++++++++++-
> include/block/block_int.h | 1 +
> qapi/block-core.json | 34 ++++++++++++++++++++--
> 5 files changed, 151 insertions(+), 11 deletions(-)
[...]
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index af5711e533..cebcbc4f2f 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -109,7 +109,14 @@ in the description of a field.
Bit 0: Dirty bit. If this bit is set then refcounts
may be inconsistent, make sure to scan L1/L2
tables to repair refcounts before accessing the
image.
Bit 1: Corrupt bit. If this bit is set then any data
structure may be corrupt and the image must not
be written to (unless for regaining
consistency).
Bit 2: External data file bit. If this bit is set, an
external data file is used. Guest clusters are
then stored in the external data file. For such
images, clusters in the external data file are
not refcounted. The offset field in the
Standard Cluster Descriptor must match the
guest offset and neither compressed clusters
nor internal snapshots are supported.
> 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. If the bit is set, then the
"Compression type bit", for consistency with the other three.
> + type of compression the image uses is set in the
> + header extension. When the bit is set the
> + compression type extension header must be present.
> + When the bit is not set the compression type
> + header must absent.
> +
> + Bits 4-63: Reserved (set to 0)
>
> 80 - 87: compatible_features
> Bitmask of compatible features. An implementation can
> @@ -175,6 +182,7 @@ be stored. Each extension has a structure like the following:
> 0x23852875 - Bitmaps extension
> 0x0537be77 - Full disk encryption header pointer
> 0x44415441 - External data file name string
> + 0x434D5052 - Compression type extension
> other - Unknown header extension, can be safely
> ignored
>
> @@ -771,3 +779,30 @@ In the image file the 'enabled' state is reflected by the 'auto' flag. If this
> flag is set, the software must consider the bitmap as 'enabled' and start
> tracking virtual disk changes to this bitmap from the first write to the
> virtual disk. If this flag is not set then the bitmap is disabled.
> +
> +
> +== Compression type extension ==
> +
> +The compression type extension is an optional header extension. It stores the
> +compression type used for disk clusters (de)compression.
s/clusters/cluster/
> +A single compression type is applied to all compressed disk clusters,
> +with no way to change compression types per cluster. Two clusters of the image
> +couldn't be compressed with different compression types.
Is the text after the first comma useful?
> +
> +The compression type is set on image creation. The only way to change
> +the compression type is to convert the image explicitly.
Suggest to scratch "explicitly".
> +
> +The compression type extension is present if and only if the incompatible
> +compression type bit is set.
Suggest "if the compression type bit (incompatible feature bit 3) is
set", for consistency with existing references to incompatible feature
bits.
> When the bit is not set the compression type
> +header must be absent.
This sentence is redundant with "if and only if". Suggest to drop it.
> +
> +When the compression type bit is not set and the compression type header
Suggest "not set, and".
> +extension is absent, ZLIB compression is used for compressed clusters.
> +This defines default image compression type: ZLIB.
I find this sentence confusing. Can we drop it?
> +Qemu < 4.1 can use images created with compression type ZLIB without any
> +additional preparations and cannot use images created with compression
> +types != ZLIB.
Suggest "QEMU versions older than 4.1 can only use images created with
compression type ZLIB".
> +
> +Available compression types:
> + 0: ZLIB
> + 1: ZSTD
Please add brief explanations with pointers to additional information
for both. Something like
0: zlib compression, see <http://zlib.net/>
1: zstd compression, see FIXME
Mention RFC 1950, 1951 and 1952 for zlib if you like.
> 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..59610153fd 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -78,6 +78,9 @@
> #
> # @bitmaps: A list of qcow2 bitmap details (since 4.0)
> #
> +# @compression-type: the compression method used for image clusters
> +# compression (since 4.1)
s/clusters/cluster/, I think.
More laconic: the image cluster compression method.
> +#
> # Since: 1.7
> ##
> { 'struct': 'ImageInfoSpecificQCow2',
> @@ -89,7 +92,8 @@
> '*corrupt': 'bool',
> 'refcount-bits': 'int',
> '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> - '*bitmaps': ['Qcow2BitmapInfo']
> + '*bitmaps': ['Qcow2BitmapInfo'],
> + '*compression-type': 'Qcow2CompressionType'
> } }
>
> ##
> @@ -3119,6 +3123,10 @@
> # an image, the data file name is loaded from the image
> # file. (since 4.0)
> #
> +# @compression-type: compression method to use for image clusters compression
Likewise.
> +# The comression method is set on image creation and can
Typo: s/comression/compression/
> +# be changed via image converting only. (since 4.1)
I.e. it can't be changed. Perhaps: "The compression method is fixed at
image creation time. To change it, you have to use qemu-img convert."
Hmm. BlockdevOptionsQcow2 has the qcow2-specific options for -blockdev.
What does passing @compression-type to -blockdev mean? The actual
compression type is determined by the qcow2 image... Am I confused?
> +#
> # Since: 2.9
> ##
> { 'struct': 'BlockdevOptionsQcow2',
> @@ -3134,7 +3142,8 @@
> '*refcount-cache-size': 'int',
> '*cache-clean-interval': 'int',
> '*encrypt': 'BlockdevQcow2Encryption',
> - '*data-file': 'BlockdevRef' } }
> + '*data-file': 'BlockdevRef',
> + '*compression-type': 'Qcow2CompressionType' } }
>
> ##
> # @SshHostKeyCheckMode:
> @@ -4206,6 +4215,19 @@
> 'data': [ 'v2', 'v3' ] }
>
>
> +##
> +# @Qcow2CompressionType:
> +#
> +# Compression type used in qcow2 image file
> +#
> +# @zlib : gzip compressor
> +# @zstd : zstd compression
Two nits. One, we don't normally put spaces before the colon. Two,
compressor vs. compression.
But I'd suggest something like
@zlib: zlib compression, see <http://zlib.net/>
Mention RFC 1950, 1951 and 1952 if you like.
Same for @zstd.
> +#
> +# Since: 4.1
> +##
> +{ 'enum': 'Qcow2CompressionType',
> + 'data': [ 'zlib', 'zstd' ] }
> +
> ##
> # @BlockdevCreateOptionsQcow2:
> #
> @@ -4228,6 +4250,11 @@
> # @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 Compression method used for image compressed clusters
More laconic: the image cluster compression method.
> +# (default: zlib(gzip), since 4.1).
The default is "zlib", not "zlib(gzip)". If you want to explain what
zlib is, do it in Qcow2CompressionType's doc string.
> +# Available types:
> +# zlib
> +# zstd
Isn't this redundant?
> #
> # Since: 2.12
> ##
> @@ -4243,7 +4270,8 @@
> '*cluster-size': 'size',
> '*preallocation': 'PreallocMode',
> '*lazy-refcounts': 'bool',
> - '*refcount-bits': 'int' } }
> + '*refcount-bits': 'int',
> + '*compression-type': 'Qcow2CompressionType' } }
>
> ##
> # @BlockdevCreateOptionsQed:
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature
2019-05-29 11:40 ` Vladimir Sementsov-Ogievskiy
@ 2019-06-28 9:45 ` Kevin Wolf
0 siblings, 0 replies; 29+ messages in thread
From: Kevin Wolf @ 2019-06-28 9:45 UTC (permalink / raw)
To: Vladimir Sementsov-Ogievskiy
Cc: Denis Lunev, qemu-block, qemu-devel, armbru, Denis Plotnikov, mreitz
Am 29.05.2019 um 13:40 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 28.05.2019 17:37, 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
> > 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..cebcbc4f2f 100644
> > --- a/docs/interop/qcow2.txt
> > +++ b/docs/interop/qcow2.txt
> > @@ -109,7 +109,14 @@ 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. If the bit is set, then the
> > + type of compression the image uses is set in the
> > + header extension. When the bit is set the
> > + compression type extension header must be present.
> > + When the bit is not set the compression type
> > + header must absent.
> > +
> > + Bits 4-63: Reserved (set to 0)
> >
> > 80 - 87: compatible_features
> > Bitmask of compatible features. An implementation can
> > @@ -175,6 +182,7 @@ be stored. Each extension has a structure like the following:
> > 0x23852875 - Bitmaps extension
> > 0x0537be77 - Full disk encryption header pointer
> > 0x44415441 - External data file name string
> > + 0x434D5052 - Compression type extension
> > other - Unknown header extension, can be safely
> > ignored
> >
> > @@ -771,3 +779,30 @@ In the image file the 'enabled' state is reflected by the 'auto' flag. If this
> > flag is set, the software must consider the bitmap as 'enabled' and start
> > tracking virtual disk changes to this bitmap from the first write to the
> > virtual disk. If this flag is not set then the bitmap is disabled.
> > +
> > +
> > +== Compression type extension ==
> > +
> > +The compression type extension is an optional header extension. It stores the
> > +compression type used for disk clusters (de)compression.
> > +A single compression type is applied to all compressed disk clusters,
> > +with no way to change compression types per cluster. Two clusters of the image
> > +couldn't be compressed with different compression types.
> > +
> > +The compression type is set on image creation. The only way to change
> > +the compression type is to convert the image explicitly.
> > +
> > +The compression type extension is present if and only if the incompatible
> > +compression type bit is set. When the bit is not set the compression type
> > +header must be absent.
>
> Hmm, not the first time we introduce incompatible bit to make incompatible
> header extension, as all header extensions are compatible by default, as for
> unknown header extension we have:
>
> other - Unknown header extension, can be safely
> ignored
>
> Hm. Should we instead define one incompatible bit for all such future cases,
> i.e. add incompatible bit HEADER_EXTENSION_FLAGS, add flag field to header
> extension declaration, and define one flag in it: COMPATIBLE, which will mean,
> that this extension may be safely ignored (old default behavior).
Would this actually make the implementation simpler, though?
I mean, the original idea was anyway that we'd just add new fields to
the header instead of using header extensions. Header extensions were
only meant for dynamically sized things like strings. That we would add
new fields to the header is why the header_length field even exists.
So in this case, we could consider using bytes 104-107 of the header for
compression_type instead of adding a new header extension.
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature Denis Plotnikov
2019-05-29 11:40 ` Vladimir Sementsov-Ogievskiy
2019-06-27 16:35 ` Markus Armbruster
@ 2019-06-28 9:54 ` Kevin Wolf
2019-06-28 11:07 ` Denis Plotnikov
2019-06-28 10:10 ` Kevin Wolf
3 siblings, 1 reply; 29+ messages in thread
From: Kevin Wolf @ 2019-06-28 9:54 UTC (permalink / raw)
To: Denis Plotnikov; +Cc: vsementsov, den, qemu-block, qemu-devel, armbru, mreitz
Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
> 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>
> @@ -3119,6 +3123,10 @@
> # an image, the data file name is loaded from the image
> # file. (since 4.0)
> #
> +# @compression-type: compression method to use for image clusters compression
> +# The comression method is set on image creation and can
> +# be changed via image converting only. (since 4.1)
> +#
> # Since: 2.9
> ##
> { 'struct': 'BlockdevOptionsQcow2',
> @@ -3134,7 +3142,8 @@
> '*refcount-cache-size': 'int',
> '*cache-clean-interval': 'int',
> '*encrypt': 'BlockdevQcow2Encryption',
> - '*data-file': 'BlockdevRef' } }
> + '*data-file': 'BlockdevRef',
> + '*compression-type': 'Qcow2CompressionType' } }
qcow2_open() doesn't actually parse this option (and it couldn't do
anything useful with it because the image is fixed to a single
compression type), so this shouldn't be added.
> ##
> # @SshHostKeyCheckMode:
> @@ -4206,6 +4215,19 @@
> 'data': [ 'v2', 'v3' ] }
>
>
> +##
> +# @Qcow2CompressionType:
> +#
> +# Compression type used in qcow2 image file
> +#
> +# @zlib : gzip compressor
> +# @zstd : zstd compression
> +#
> +# Since: 4.1
> +##
> +{ 'enum': 'Qcow2CompressionType',
> + 'data': [ 'zlib', 'zstd' ] }
I think it would be cleaner to start with only 'zlib' here, like your C
code that doesn't implement anything for non-zlib compression types yet.
'zstd' can be added to the enum when it's actually implemented. This
will also make schema introspection provide the right information with a
build that includes this patch, but not the zstd compression type
patch.
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature Denis Plotnikov
` (2 preceding siblings ...)
2019-06-28 9:54 ` Kevin Wolf
@ 2019-06-28 10:10 ` Kevin Wolf
3 siblings, 0 replies; 29+ messages in thread
From: Kevin Wolf @ 2019-06-28 10:10 UTC (permalink / raw)
To: Denis Plotnikov; +Cc: vsementsov, den, qemu-block, qemu-devel, armbru, mreitz
Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
> 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>
> @@ -771,3 +779,30 @@ In the image file the 'enabled' state is reflected by the 'auto' flag. If this
> flag is set, the software must consider the bitmap as 'enabled' and start
> tracking virtual disk changes to this bitmap from the first write to the
> virtual disk. If this flag is not set then the bitmap is disabled.
> +
> +
> +== Compression type extension ==
> +
> +The compression type extension is an optional header extension. It stores the
> +compression type used for disk clusters (de)compression.
> +A single compression type is applied to all compressed disk clusters,
> +with no way to change compression types per cluster. Two clusters of the image
> +couldn't be compressed with different compression types.
> +
> +The compression type is set on image creation. The only way to change
> +the compression type is to convert the image explicitly.
> +
> +The compression type extension is present if and only if the incompatible
> +compression type bit is set. When the bit is not set the compression type
> +header must be absent.
> +
> +When the compression type bit is not set and the compression type header
> +extension is absent, ZLIB compression is used for compressed clusters.
> +This defines default image compression type: ZLIB.
> +Qemu < 4.1 can use images created with compression type ZLIB without any
> +additional preparations and cannot use images created with compression
> +types != ZLIB.
> +
> +Available compression types:
> + 0: ZLIB
> + 1: ZSTD
This section shouldn't be added at the end of the document, but after
the last section that describes a header extension (I think this is
'Full disk encryption header pointer').
Also, let me mention that I'm not reviewing the documentation wording in
detail because Markus has already given a lot of good feedback that I
don't want to duplicate.
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing Denis Plotnikov
2019-05-29 9:47 ` Vladimir Sementsov-Ogievskiy
@ 2019-06-28 10:23 ` Kevin Wolf
2019-06-28 11:24 ` Denis Plotnikov
1 sibling, 1 reply; 29+ messages in thread
From: Kevin Wolf @ 2019-06-28 10:23 UTC (permalink / raw)
To: Denis Plotnikov; +Cc: vsementsov, den, qemu-block, qemu-devel, armbru, mreitz
Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
> With the patch, qcow2 is able to process image compression type
> defined in the image header and choose the corresponding method
> for clusters compressing.
>
> Also, it rework the cluster compression code for adding more
> compression types.
>
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
> block/qcow2.c | 103 ++++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 92 insertions(+), 11 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index c4b5b93408..90f15cc3c9 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -400,11 +400,39 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> break;
>
> case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
> + /* Compression type always goes with the compression type bit set */
> + if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
> + error_setg(errp,
> + "compression_type_ext: "
> + "expect compression type bit set");
> + return -EINVAL;
> + }
> +
> + ret = bdrv_pread(bs->file, offset, &s->compression_type, ext.len);
> + s->compression_type = be32_to_cpu(s->compression_type);
> +
> + if (ret < 0) {
> + error_setg_errno(errp, -ret,
> + "ERROR: Could not read compression type");
> + return ret;
> + }
> +
> /*
> - * Setting compression type to BDRVQcow2State->compression_type
> - * from the image header is going to be here
> + * The default compression type is not allowed when the extension
> + * is present. ZLIB is used as the default compression type.
> + * When compression type extension header is present then
> + * compression_type should have a value different from the default.
> */
> - break;
> + if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
> + error_setg(errp,
> + "compression_type_ext:"
> + "invalid compression type %d",
> + QCOW2_COMPRESSION_TYPE_ZLIB);
> + }
This is a restriction that the spec doesn't make, so strictly speaking
this implementation wouldn't be compliant to the spec.
We can discuss whether the code or the spec should be changed. At the
moment, I don't see a good reason to make the restriction
> +#ifdef DEBUG_EXT
> + printf("Qcow2: image compression type %s\n", s->compression_type);
> +#endif
> + break;
>
> case QCOW2_EXT_MAGIC_DATA_FILE:
> {
We would save most of this code if we added a new field to the header
instead of adding a header extension. Not saying that we should
definitely do this, but let's discuss it at least.
> @@ -1492,6 +1520,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> QLIST_INIT(&s->cluster_allocs);
> QTAILQ_INIT(&s->discards);
>
> + /* Set default compression type */
> + s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
> +
> /* read qcow2 extensions */
> if (qcow2_read_extensions(bs, header.header_length, ext_end, NULL,
> flags, &update_header, &local_err)) {
> @@ -1500,6 +1531,34 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> goto fail;
> }
>
> + /*
> + * The compression type is set on the extension header processing
> + * if the compression type extension header is present.
> + * When the compression type is different from ZLIB (default) there
> + * should be both the compression type bit and the compression
> + * type extension header set. When the ZLIB (default) compression
> + * type is used there should be neither the compression type bit nor
> + * the compression type extension header set.
> + */
> +
> + if ((s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) &
> + (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB)) {
> + error_setg(errp, "Illegal compression type setting");
> + ret = -EINVAL;
> + goto fail;
> + }
Almost the same restriction as above, implemented a second time.
The difference is that you get the first one only for compression type
extensions that specify zlib, and you get this one also for images that
have the compression type feature flag, but are missing the extension.
So in the current shape of the patch, this is in fact just the wrong
error message. It should be something like "compression type header
extension missing".
This restriction (if the flag is set, the header extension must be
present) actually makes sense to me. Switching to a new header field
would get rid of the whole case.
> + /* Check available compression types */
> + switch (s->compression_type) {
> + case QCOW2_COMPRESSION_TYPE_ZLIB:
> + break;
> +
> + default:
> + error_setg(errp, "Unknown compression type");
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> /* Open external data file */
> s->data_file = bdrv_open_child(NULL, options, "data-file", bs, &child_file,
> true, &local_err);
> @@ -3970,7 +4029,7 @@ fail:
> }
>
> /*
> - * qcow2_compress()
> + * zlib_compress()
> *
> * @dest - destination buffer, @dest_size bytes
> * @src - source buffer, @src_size bytes
> @@ -3979,7 +4038,7 @@ fail:
> * -1 destination buffer is not enough to store compressed data
> * -2 on any other error
> */
> -static ssize_t qcow2_compress(void *dest, size_t dest_size,
> +static ssize_t zlib_compress(void *dest, size_t dest_size,
> const void *src, size_t src_size)
Indentation is now off in the second line.
> {
> ssize_t ret;
> @@ -4013,7 +4072,7 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
> }
>
> /*
> - * qcow2_decompress()
> + * zlib_decompress()
> *
> * Decompress some data (not more than @src_size bytes) to produce exactly
> * @dest_size bytes.
Should the description be changed, too? (qcow2_compress() doesn't have
any - should it have some?)
> @@ -4024,7 +4083,7 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
> * Returns: 0 on success
> * -1 on fail
> */
> -static ssize_t qcow2_decompress(void *dest, size_t dest_size,
> +static ssize_t zlib_decompress(void *dest, size_t dest_size,
> const void *src, size_t src_size)
Indentation again.
Maybe keep the qcow2_ prefix and make it qcow2_(de)compress_zlib()?
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature
2019-06-28 9:54 ` Kevin Wolf
@ 2019-06-28 11:07 ` Denis Plotnikov
0 siblings, 0 replies; 29+ messages in thread
From: Denis Plotnikov @ 2019-06-28 11:07 UTC (permalink / raw)
To: Kevin Wolf
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block,
qemu-devel, armbru, mreitz
On 28.06.2019 12:54, Kevin Wolf wrote:
> Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
>> 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>
>
>> @@ -3119,6 +3123,10 @@
>> # an image, the data file name is loaded from the image
>> # file. (since 4.0)
>> #
>> +# @compression-type: compression method to use for image clusters compression
>> +# The comression method is set on image creation and can
>> +# be changed via image converting only. (since 4.1)
>> +#
>> # Since: 2.9
>> ##
>> { 'struct': 'BlockdevOptionsQcow2',
>> @@ -3134,7 +3142,8 @@
>> '*refcount-cache-size': 'int',
>> '*cache-clean-interval': 'int',
>> '*encrypt': 'BlockdevQcow2Encryption',
>> - '*data-file': 'BlockdevRef' } }
>> + '*data-file': 'BlockdevRef',
>> + '*compression-type': 'Qcow2CompressionType' } }
>
> qcow2_open() doesn't actually parse this option (and it couldn't do
> anything useful with it because the image is fixed to a single
> compression type), so this shouldn't be added.
Yes, that's true. Will remove it.
>
>> ##
>> # @SshHostKeyCheckMode:
>> @@ -4206,6 +4215,19 @@
>> 'data': [ 'v2', 'v3' ] }
>>
>>
>> +##
>> +# @Qcow2CompressionType:
>> +#
>> +# Compression type used in qcow2 image file
>> +#
>> +# @zlib : gzip compressor
>> +# @zstd : zstd compression
>> +#
>> +# Since: 4.1
>> +##
>> +{ 'enum': 'Qcow2CompressionType',
>> + 'data': [ 'zlib', 'zstd' ] }
>
> I think it would be cleaner to start with only 'zlib' here, like your C
> code that doesn't implement anything for non-zlib compression types yet.
>
> 'zstd' can be added to the enum when it's actually implemented. This
> will also make schema introspection provide the right information with a
> build that includes this patch, but not the zstd compression type
> patch.
Make sense. Will be fixed
>
> Kevin
>
--
Best,
Denis
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing
2019-06-28 10:23 ` Kevin Wolf
@ 2019-06-28 11:24 ` Denis Plotnikov
2019-06-28 12:06 ` Kevin Wolf
0 siblings, 1 reply; 29+ messages in thread
From: Denis Plotnikov @ 2019-06-28 11:24 UTC (permalink / raw)
To: Kevin Wolf
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block,
qemu-devel, armbru, mreitz
On 28.06.2019 13:23, Kevin Wolf wrote:
> Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
>> With the patch, qcow2 is able to process image compression type
>> defined in the image header and choose the corresponding method
>> for clusters compressing.
>>
>> Also, it rework the cluster compression code for adding more
>> compression types.
>>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> ---
>> block/qcow2.c | 103 ++++++++++++++++++++++++++++++++++++++++++++------
>> 1 file changed, 92 insertions(+), 11 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index c4b5b93408..90f15cc3c9 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -400,11 +400,39 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>> break;
>>
>> case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
>> + /* Compression type always goes with the compression type bit set */
>> + if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
>> + error_setg(errp,
>> + "compression_type_ext: "
>> + "expect compression type bit set");
>> + return -EINVAL;
>> + }
>> +
>> + ret = bdrv_pread(bs->file, offset, &s->compression_type, ext.len);
>> + s->compression_type = be32_to_cpu(s->compression_type);
>> +
>> + if (ret < 0) {
>> + error_setg_errno(errp, -ret,
>> + "ERROR: Could not read compression type");
>> + return ret;
>> + }
>> +
>> /*
>> - * Setting compression type to BDRVQcow2State->compression_type
>> - * from the image header is going to be here
>> + * The default compression type is not allowed when the extension
>> + * is present. ZLIB is used as the default compression type.
>> + * When compression type extension header is present then
>> + * compression_type should have a value different from the default.
>> */
>> - break;
>> + if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
>> + error_setg(errp,
>> + "compression_type_ext:"
>> + "invalid compression type %d",
>> + QCOW2_COMPRESSION_TYPE_ZLIB);
>> + }
>
> This is a restriction that the spec doesn't make, so strictly speaking
> this implementation wouldn't be compliant to the spec.
The idea is that ZLIB shouldn't appear in the compression type
extension. This allows image backward compatibility with an older qemu
if zlib is used.
There is no reason to set ZLIB in the extension because an older qemu
knows how to tread ZLIB compressed clusters.
The restriction aims to guarantee that.
I tried to describe this case in the specification:
...
When the compression type bit is not set, and the compression type
header extension is absent, ZLIB compression is used for compressed
clusters.
Qemu versions older than 4.1 can use images created with compression
type ZLIB without any additional preparations and cannot use images
created with compression types != ZLIB.
...
Does it makes sense?
>
> We can discuss whether the code or the spec should be changed. At the
> moment, I don't see a good reason to make the restriction
>
>> +#ifdef DEBUG_EXT
>> + printf("Qcow2: image compression type %s\n", s->compression_type);
>> +#endif
>> + break;
>>
>> case QCOW2_EXT_MAGIC_DATA_FILE:
>> {
>
> We would save most of this code if we added a new field to the header
> instead of adding a header extension. Not saying that we should
> definitely do this, but let's discuss it at least.
If we add the new field to the header will the older qemu be able to use
it. Or we will add the header only if needed, i.e. if compression_type
!= zlib
>
>> @@ -1492,6 +1520,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>> QLIST_INIT(&s->cluster_allocs);
>> QTAILQ_INIT(&s->discards);
>>
>> + /* Set default compression type */
>> + s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
>> +
>> /* read qcow2 extensions */
>> if (qcow2_read_extensions(bs, header.header_length, ext_end, NULL,
>> flags, &update_header, &local_err)) {
>> @@ -1500,6 +1531,34 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>> goto fail;
>> }
>>
>> + /*
>> + * The compression type is set on the extension header processing
>> + * if the compression type extension header is present.
>> + * When the compression type is different from ZLIB (default) there
>> + * should be both the compression type bit and the compression
>> + * type extension header set. When the ZLIB (default) compression
>> + * type is used there should be neither the compression type bit nor
>> + * the compression type extension header set.
>> + */
>> +
>> + if ((s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE) &
>> + (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB)) {
>> + error_setg(errp, "Illegal compression type setting");
>> + ret = -EINVAL;
>> + goto fail;
>> + }
>
> Almost the same restriction as above, implemented a second time.
>
> The difference is that you get the first one only for compression type
> extensions that specify zlib, and you get this one also for images that
> have the compression type feature flag, but are missing the extension.
>
> So in the current shape of the patch, this is in fact just the wrong
> error message. It should be something like "compression type header
> extension missing".
>
> This restriction (if the flag is set, the header extension must be
> present) actually makes sense to me. Switching to a new header field
> would get rid of the whole case.
>
>> + /* Check available compression types */
>> + switch (s->compression_type) {
>> + case QCOW2_COMPRESSION_TYPE_ZLIB:
>> + break;
>> +
>> + default:
>> + error_setg(errp, "Unknown compression type");
>> + ret = -EINVAL;
>> + goto fail;
>> + }
>> +
>> /* Open external data file */
>> s->data_file = bdrv_open_child(NULL, options, "data-file", bs, &child_file,
>> true, &local_err);
>> @@ -3970,7 +4029,7 @@ fail:
>> }
>>
>> /*
>> - * qcow2_compress()
>> + * zlib_compress()
>> *
>> * @dest - destination buffer, @dest_size bytes
>> * @src - source buffer, @src_size bytes
>> @@ -3979,7 +4038,7 @@ fail:
>> * -1 destination buffer is not enough to store compressed data
>> * -2 on any other error
>> */
>> -static ssize_t qcow2_compress(void *dest, size_t dest_size,
>> +static ssize_t zlib_compress(void *dest, size_t dest_size,
>> const void *src, size_t src_size)
>
> Indentation is now off in the second line.
>
>> {
>> ssize_t ret;
>> @@ -4013,7 +4072,7 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
>> }
>>
>> /*
>> - * qcow2_decompress()
>> + * zlib_decompress()
>> *
>> * Decompress some data (not more than @src_size bytes) to produce exactly
>> * @dest_size bytes.
>
> Should the description be changed, too? (qcow2_compress() doesn't have
> any - should it have some?)
>
>> @@ -4024,7 +4083,7 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
>> * Returns: 0 on success
>> * -1 on fail
>> */
>> -static ssize_t qcow2_decompress(void *dest, size_t dest_size,
>> +static ssize_t zlib_decompress(void *dest, size_t dest_size,
>> const void *src, size_t src_size)
>
> Indentation again.
>
> Maybe keep the qcow2_ prefix and make it qcow2_(de)compress_zlib()?
ok
Denis
>
> Kevin
>
--
Best,
Denis
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 3/3] qcow2: add zstd cluster compression
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 3/3] qcow2: add zstd cluster compression Denis Plotnikov
@ 2019-06-28 11:57 ` Kevin Wolf
2019-07-02 12:33 ` Denis Plotnikov
2019-07-02 12:49 ` Denis Plotnikov
0 siblings, 2 replies; 29+ messages in thread
From: Kevin Wolf @ 2019-06-28 11:57 UTC (permalink / raw)
To: Denis Plotnikov; +Cc: vsementsov, den, qemu-block, qemu-devel, armbru, mreitz
Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
> zstd significantly reduces cluster compression time.
> It provides better compression performance maintaining
> the same level of compression ratio in comparison with
> zlib, which, by the moment, has been the only compression
> method available.
>
> The performance test results:
> Test compresses and decompresses qemu qcow2 image with just
> installed rhel-7.6 guest.
> Image cluster size: 64K. Image on disk size: 2.2G
>
> The test was conducted with brd disk to reduce the influence
> of disk subsystem to the test results.
> The results is given in seconds.
>
> compress cmd:
> time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
> src.img [zlib|zstd]_compressed.img
> decompress cmd
> time ./qemu-img convert -O qcow2
> [zlib|zstd]_compressed.img uncompressed.img
>
> compression decompression
> zlib zstd zlib zstd
> ------------------------------------------------------------
> real 65.5 16.3 (-75 %) 1.9 1.6 (-16 %)
> user 65.0 15.8 5.3 2.5
> sys 3.3 0.2 2.0 2.0
>
> Both ZLIB and ZSTD gave the same compression ratio: 1.57
> compressed image size in both cases: 1.4G
>
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
> block/qcow2.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++
> configure | 26 ++++++++++++++++
> 2 files changed, 108 insertions(+)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 90f15cc3c9..58901f9f79 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -26,6 +26,7 @@
>
> #define ZLIB_CONST
> #include <zlib.h>
> +#include <zstd.h>
>
> #include "block/block_int.h"
> #include "block/qdict.h"
> @@ -1553,6 +1554,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> case QCOW2_COMPRESSION_TYPE_ZLIB:
> break;
>
> + case QCOW2_COMPRESSION_TYPE_ZSTD:
> + break;
If we don't intend to add any code here, why not just add another case
label to the existing break?
> default:
> error_setg(errp, "Unknown compression type");
> ret = -EINVAL;
> @@ -3286,6 +3290,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
> * ZLIB shouldn't be here since it's the default
> */
> switch (qcow2_opts->compression_type) {
> + case QCOW2_COMPRESSION_TYPE_ZSTD:
> + break;
> +
> default:
> error_setg_errno(errp, -EINVAL, "Unknown compression type");
> goto out;
> @@ -4113,6 +4120,73 @@ static ssize_t zlib_decompress(void *dest, size_t dest_size,
> return ret;
> }
>
> +/*
> + * zstd_compress()
> + *
> + * @dest - destination buffer, @dest_size bytes
> + * @src - source buffer, @src_size bytes
> + *
> + * Returns: compressed size on success
> + * -1 on any error
> + */
> +
> +static ssize_t zstd_compress(void *dest, size_t dest_size,
> + const void *src, size_t src_size)
> +{
> + /* steal some bytes to store compressed chunk size */
> + size_t ret;
This ends up in a file, so this needs to be a fixed number of bytes.
size_t varies between different platforms, so it is not acceptable.
If I understand correctly, the maximum for this is the cluster size, so
uint32_t should be right.
This isn't plain the zstd compression format any more, so it needs to be
described in the qcow2 spec.
> + size_t *c_size = dest;
> + char *d_buf = dest;
> + d_buf += sizeof(ret);
char *d_bug = dest + sizeof(ret);
> + dest_size -= sizeof(ret);
We don't want to end up with an integer overflow, so before this:
if (dest_size < sizeof(ret)) {
return -ENOMEM;
}
> + ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
> +
> + if (ZSTD_isError(ret)) {
> + return -1;
> + }
Need an error code here, not just -1. In particular, we need to
distinguish cases where the buffer was too small and uncompressed data
should be written instead (ENOMEM) from real errors that should be
returned to the caller (EIO).
> +
> + /* store the compressed chunk size in the very beginning of the buffer */
> + *c_size = ret;
> +
> + return ret + sizeof(ret);
> +}
> +
> +/*
> + * zstd_decompress()
> + *
> + * Decompress some data (not more than @src_size bytes) to produce exactly
> + * @dest_size bytes.
> + *
> + * @dest - destination buffer, @dest_size bytes
> + * @src - source buffer, @src_size bytes
> + *
> + * Returns: 0 on success
> + * -1 on fail
> + */
> +
> +static ssize_t zstd_decompress(void *dest, size_t dest_size,
> + const void *src, size_t src_size)
Indentation is off.
> +{
> + size_t ret;
> + /*
> + * zstd decompress wants to know the exact lenght of the data
> + * for that purpose, zstd_compress stores the length in the
> + * very beginning of the compressed buffer
> + */
> + const size_t *s_size = src;
> + const char *s_buf = src;
> + s_buf += sizeof(size_t);
Single line: const char *s_buf = src + sizeof(size_t);
Of course, size_t is wrong here, too. And above you used sizeof() on a
variable and here it's on the type. I think we should stay consistent.
You're lacking a check against src_size. A malicious image could make
use read beyond the end of the buffer. (Also consider that src_size
could be smaller than sizeof(size_t).)
> + ret = ZSTD_decompress(dest, dest_size, s_buf, *s_size);
> +
> + if (ZSTD_isError(ret)) {
> + return -1;
> + }
> +
> + return 0;
> +}
> +
> #define MAX_COMPRESS_THREADS 4
>
> typedef ssize_t (*Qcow2CompressFunc)(void *dest, size_t dest_size,
> @@ -4189,6 +4263,10 @@ qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
> fn = zlib_compress;
> break;
>
> + case QCOW2_COMPRESSION_TYPE_ZSTD:
> + fn = zstd_compress;
> + break;
> +
> default:
> return -ENOTSUP;
> }
> @@ -4208,6 +4286,10 @@ qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
> fn = zlib_decompress;
> break;
>
> + case QCOW2_COMPRESSION_TYPE_ZSTD:
> + fn = zstd_decompress;
> + break;
> +
> default:
> return -ENOTSUP;
> }
> diff --git a/configure b/configure
> index 1c563a7027..c90716189c 100755
> --- a/configure
> +++ b/configure
> @@ -433,6 +433,7 @@ opengl_dmabuf="no"
> cpuid_h="no"
> avx2_opt=""
> zlib="yes"
> +zstd="yes"
This should be zstd="" so that a missing library will automatically
disable it instead of producing an error. (Building QEMU without zlib is
impossible, but building it without ZSTD should work.)
> capstone=""
> lzo=""
> snappy=""
> @@ -1317,6 +1318,8 @@ for opt do
> ;;
> --disable-zlib-test) zlib="no"
> ;;
> + --disable-zstd-test) zstd="no"
> + ;;
Instead of this one, after making the above change, options
--disable-zstd and --enable-zstd should be introduced that set
zstd="yes" (that actually does produce an error if it's not available)
or zstd="no".
> --disable-lzo) lzo="no"
> ;;
> --enable-lzo) lzo="yes"
> @@ -3702,6 +3705,29 @@ EOF
> fi
> fi
>
> +#########################################
> +# zstd check
> +
> +if test "$zstd" != "no" ; then
> + if $pkg_config --exists libzstd; then
> + zstd_cflags=$($pkg_config --cflags libzstd)
> + zstd_libs=$($pkg_config --libs libzstd)
> + QEMU_CFLAGS="$zstd_cflags $QEMU_CFLAGS"
> + LIBS="$zstd_libs $LIBS"
> + else
> + cat > $TMPC << EOF
> +#include <zstd.h>
> +int main(void) { ZSTD_versionNumber(); return 0; }
> +EOF
> + if compile_prog "" "-lzstd" ; then
> + LIBS="$LIBS -lzstd"
> + else
> + error_exit "zstd check failed" \
> + "Make sure to have the zstd libs and headers installed."
> + fi
This needs to be changed, too, to get the desired behaviour. Model it
after bzip2 or lzo support checks:
if compile_prog "" "-lbz2" ; then
bzip2="yes"
else
if test "$bzip2" = "yes"; then
feature_not_found "libbzip2" "Install libbzip2 devel"
fi
bzip2="no"
fi
> + fi
> +fi
> +
> ##########################################
> # SHA command probe for modules
> if test "$modules" = yes; then
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing
2019-06-28 11:24 ` Denis Plotnikov
@ 2019-06-28 12:06 ` Kevin Wolf
2019-06-28 12:56 ` Denis Plotnikov
0 siblings, 1 reply; 29+ messages in thread
From: Kevin Wolf @ 2019-06-28 12:06 UTC (permalink / raw)
To: Denis Plotnikov
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block,
qemu-devel, armbru, mreitz
Am 28.06.2019 um 13:24 hat Denis Plotnikov geschrieben:
>
>
> On 28.06.2019 13:23, Kevin Wolf wrote:
> > Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
> >> With the patch, qcow2 is able to process image compression type
> >> defined in the image header and choose the corresponding method
> >> for clusters compressing.
> >>
> >> Also, it rework the cluster compression code for adding more
> >> compression types.
> >>
> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> >> ---
> >> block/qcow2.c | 103 ++++++++++++++++++++++++++++++++++++++++++++------
> >> 1 file changed, 92 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/block/qcow2.c b/block/qcow2.c
> >> index c4b5b93408..90f15cc3c9 100644
> >> --- a/block/qcow2.c
> >> +++ b/block/qcow2.c
> >> @@ -400,11 +400,39 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> >> break;
> >>
> >> case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
> >> + /* Compression type always goes with the compression type bit set */
> >> + if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
> >> + error_setg(errp,
> >> + "compression_type_ext: "
> >> + "expect compression type bit set");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + ret = bdrv_pread(bs->file, offset, &s->compression_type, ext.len);
> >> + s->compression_type = be32_to_cpu(s->compression_type);
> >> +
> >> + if (ret < 0) {
> >> + error_setg_errno(errp, -ret,
> >> + "ERROR: Could not read compression type");
> >> + return ret;
> >> + }
> >> +
> >> /*
> >> - * Setting compression type to BDRVQcow2State->compression_type
> >> - * from the image header is going to be here
> >> + * The default compression type is not allowed when the extension
> >> + * is present. ZLIB is used as the default compression type.
> >> + * When compression type extension header is present then
> >> + * compression_type should have a value different from the default.
> >> */
> >> - break;
> >> + if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
> >> + error_setg(errp,
> >> + "compression_type_ext:"
> >> + "invalid compression type %d",
> >> + QCOW2_COMPRESSION_TYPE_ZLIB);
> >> + }
> >
> > This is a restriction that the spec doesn't make, so strictly speaking
> > this implementation wouldn't be compliant to the spec.
> The idea is that ZLIB shouldn't appear in the compression type
> extension. This allows image backward compatibility with an older qemu
> if zlib is used.
>
> There is no reason to set ZLIB in the extension because an older qemu
> knows how to tread ZLIB compressed clusters.
>
> The restriction aims to guarantee that.
>
> I tried to describe this case in the specification:
> ...
> When the compression type bit is not set, and the compression type
> header extension is absent, ZLIB compression is used for compressed
> clusters.
>
> Qemu versions older than 4.1 can use images created with compression
> type ZLIB without any additional preparations and cannot use images
> created with compression types != ZLIB.
> ...
>
> Does it makes sense?
This text says that using zlib in the extension is not necessary because
it's the default. But it doesn't say that using zlib in the extension is
illegal.
I agree that there is no good reason to create a compression type
extension if you have zlib. But is there a good reason to forbid it? It
only requires us to add artificial restrictions to code that would work
fine without them.
Either way, if we want to reject such extensions, the spec needs to say
that it's illegal. And if the spec allows such images, we must accept
them.
> > We can discuss whether the code or the spec should be changed. At the
> > moment, I don't see a good reason to make the restriction
> >
> >> +#ifdef DEBUG_EXT
> >> + printf("Qcow2: image compression type %s\n", s->compression_type);
> >> +#endif
> >> + break;
> >>
> >> case QCOW2_EXT_MAGIC_DATA_FILE:
> >> {
> >
> > We would save most of this code if we added a new field to the header
> > instead of adding a header extension. Not saying that we should
> > definitely do this, but let's discuss it at least.
>
> If we add the new field to the header will the older qemu be able to use
> it. Or we will add the header only if needed, i.e. if compression_type
> != zlib
Increasing the header size is backwards compatible. Older qemu versions
should handle such images correctly. They would store the unknown part
of the header in s->unknown_header_fields and keep it unmodified when
updating the image header.
We would still add the incompatible feature flag for non-zlib, of
course.
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing
2019-06-28 12:06 ` Kevin Wolf
@ 2019-06-28 12:56 ` Denis Plotnikov
2019-06-28 14:24 ` Kevin Wolf
0 siblings, 1 reply; 29+ messages in thread
From: Denis Plotnikov @ 2019-06-28 12:56 UTC (permalink / raw)
To: Kevin Wolf
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block,
qemu-devel, armbru, mreitz
On 28.06.2019 15:06, Kevin Wolf wrote:
> Am 28.06.2019 um 13:24 hat Denis Plotnikov geschrieben:
>>
>>
>> On 28.06.2019 13:23, Kevin Wolf wrote:
>>> Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
>>>> With the patch, qcow2 is able to process image compression type
>>>> defined in the image header and choose the corresponding method
>>>> for clusters compressing.
>>>>
>>>> Also, it rework the cluster compression code for adding more
>>>> compression types.
>>>>
>>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>>>> ---
>>>> block/qcow2.c | 103 ++++++++++++++++++++++++++++++++++++++++++++------
>>>> 1 file changed, 92 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index c4b5b93408..90f15cc3c9 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -400,11 +400,39 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>>> break;
>>>>
>>>> case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
>>>> + /* Compression type always goes with the compression type bit set */
>>>> + if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
>>>> + error_setg(errp,
>>>> + "compression_type_ext: "
>>>> + "expect compression type bit set");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> + ret = bdrv_pread(bs->file, offset, &s->compression_type, ext.len);
>>>> + s->compression_type = be32_to_cpu(s->compression_type);
>>>> +
>>>> + if (ret < 0) {
>>>> + error_setg_errno(errp, -ret,
>>>> + "ERROR: Could not read compression type");
>>>> + return ret;
>>>> + }
>>>> +
>>>> /*
>>>> - * Setting compression type to BDRVQcow2State->compression_type
>>>> - * from the image header is going to be here
>>>> + * The default compression type is not allowed when the extension
>>>> + * is present. ZLIB is used as the default compression type.
>>>> + * When compression type extension header is present then
>>>> + * compression_type should have a value different from the default.
>>>> */
>>>> - break;
>>>> + if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
>>>> + error_setg(errp,
>>>> + "compression_type_ext:"
>>>> + "invalid compression type %d",
>>>> + QCOW2_COMPRESSION_TYPE_ZLIB);
>>>> + }
>>>
>>> This is a restriction that the spec doesn't make, so strictly speaking
>>> this implementation wouldn't be compliant to the spec.
>> The idea is that ZLIB shouldn't appear in the compression type
>> extension. This allows image backward compatibility with an older qemu
>> if zlib is used.
>>
>> There is no reason to set ZLIB in the extension because an older qemu
>> knows how to tread ZLIB compressed clusters.
>>
>> The restriction aims to guarantee that.
>>
>> I tried to describe this case in the specification:
>> ...
>> When the compression type bit is not set, and the compression type
>> header extension is absent, ZLIB compression is used for compressed
>> clusters.
>>
>> Qemu versions older than 4.1 can use images created with compression
>> type ZLIB without any additional preparations and cannot use images
>> created with compression types != ZLIB.
>> ...
>>
>> Does it makes sense?
>
> This text says that using zlib in the extension is not necessary because
> it's the default. But it doesn't say that using zlib in the extension is
> illegal.
>
> I agree that there is no good reason to create a compression type
> extension if you have zlib. But is there a good reason to forbid it?
I think yes, if we create image with the extension set to zlib we
prevent an older qemu from using that image. Furthermore, to allow older
qemu using such images we need to create special conversion procedure
which has to remove the extension header.
If zlib is a "special compression type" which is always set by default
without the extension header we'll get rid of such image conversion
procedure and an older qemu could use it "as is"
Might it work as a good reason?
> It
> only requires us to add artificial restrictions to code that would work
> fine without them.
>
> Either way, if we want to reject such extensions, the spec needs to say
> that it's illegal. And if the spec allows such images, we must accept
> them.
Yes, it's true
The only reasons that zlib compression type even exists in the
enumeration is to avoid ambiguity for users.
For them it may be hard to understand why they can set zstd and cannot
set zlib as compression type and to really set zlib they have to set no
compression type to make the default zlib to apply.
When a user set zlib as compression type the image is created as before
the extension header were introduced.
Reasonable?
>
>>> We can discuss whether the code or the spec should be changed. At the
>>> moment, I don't see a good reason to make the restriction
>>>
>>>> +#ifdef DEBUG_EXT
>>>> + printf("Qcow2: image compression type %s\n", s->compression_type);
>>>> +#endif
>>>> + break;
>>>>
>>>> case QCOW2_EXT_MAGIC_DATA_FILE:
>>>> {
>>>
>>> We would save most of this code if we added a new field to the header
>>> instead of adding a header extension. Not saying that we should
>>> definitely do this, but let's discuss it at least.
>>
>> If we add the new field to the header will the older qemu be able to use
>> it. Or we will add the header only if needed, i.e. if compression_type
>> != zlib
>
> Increasing the header size is backwards compatible. Older qemu versions
> should handle such images correctly. They would store the unknown part
> of the header in s->unknown_header_fields and keep it unmodified when
> updating the image header.
>
> We would still add the incompatible feature flag for non-zlib, of
> course.
so, we basically need to do the same: store compression type and forbid
to use because of flag if not zlib.
Sounds like it doesn't differ that much from the extension header approach.
May be I'm missing something. Please correct my if so.
Denis
>
> Kevin
>
--
Best,
Denis
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing
2019-06-28 12:56 ` Denis Plotnikov
@ 2019-06-28 14:24 ` Kevin Wolf
2019-06-28 14:40 ` Denis Plotnikov
0 siblings, 1 reply; 29+ messages in thread
From: Kevin Wolf @ 2019-06-28 14:24 UTC (permalink / raw)
To: Denis Plotnikov
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block,
qemu-devel, armbru, mreitz
Am 28.06.2019 um 14:56 hat Denis Plotnikov geschrieben:
>
>
> On 28.06.2019 15:06, Kevin Wolf wrote:
> > Am 28.06.2019 um 13:24 hat Denis Plotnikov geschrieben:
> >>
> >>
> >> On 28.06.2019 13:23, Kevin Wolf wrote:
> >>> Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
> >>>> With the patch, qcow2 is able to process image compression type
> >>>> defined in the image header and choose the corresponding method
> >>>> for clusters compressing.
> >>>>
> >>>> Also, it rework the cluster compression code for adding more
> >>>> compression types.
> >>>>
> >>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> >>>> ---
> >>>> block/qcow2.c | 103 ++++++++++++++++++++++++++++++++++++++++++++------
> >>>> 1 file changed, 92 insertions(+), 11 deletions(-)
> >>>>
> >>>> diff --git a/block/qcow2.c b/block/qcow2.c
> >>>> index c4b5b93408..90f15cc3c9 100644
> >>>> --- a/block/qcow2.c
> >>>> +++ b/block/qcow2.c
> >>>> @@ -400,11 +400,39 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> >>>> break;
> >>>>
> >>>> case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
> >>>> + /* Compression type always goes with the compression type bit set */
> >>>> + if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
> >>>> + error_setg(errp,
> >>>> + "compression_type_ext: "
> >>>> + "expect compression type bit set");
> >>>> + return -EINVAL;
> >>>> + }
> >>>> +
> >>>> + ret = bdrv_pread(bs->file, offset, &s->compression_type, ext.len);
> >>>> + s->compression_type = be32_to_cpu(s->compression_type);
> >>>> +
> >>>> + if (ret < 0) {
> >>>> + error_setg_errno(errp, -ret,
> >>>> + "ERROR: Could not read compression type");
> >>>> + return ret;
> >>>> + }
> >>>> +
> >>>> /*
> >>>> - * Setting compression type to BDRVQcow2State->compression_type
> >>>> - * from the image header is going to be here
> >>>> + * The default compression type is not allowed when the extension
> >>>> + * is present. ZLIB is used as the default compression type.
> >>>> + * When compression type extension header is present then
> >>>> + * compression_type should have a value different from the default.
> >>>> */
> >>>> - break;
> >>>> + if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
> >>>> + error_setg(errp,
> >>>> + "compression_type_ext:"
> >>>> + "invalid compression type %d",
> >>>> + QCOW2_COMPRESSION_TYPE_ZLIB);
> >>>> + }
> >>>
> >>> This is a restriction that the spec doesn't make, so strictly speaking
> >>> this implementation wouldn't be compliant to the spec.
> >> The idea is that ZLIB shouldn't appear in the compression type
> >> extension. This allows image backward compatibility with an older qemu
> >> if zlib is used.
> >>
> >> There is no reason to set ZLIB in the extension because an older qemu
> >> knows how to tread ZLIB compressed clusters.
> >>
> >> The restriction aims to guarantee that.
> >>
> >> I tried to describe this case in the specification:
> >> ...
> >> When the compression type bit is not set, and the compression type
> >> header extension is absent, ZLIB compression is used for compressed
> >> clusters.
> >>
> >> Qemu versions older than 4.1 can use images created with compression
> >> type ZLIB without any additional preparations and cannot use images
> >> created with compression types != ZLIB.
> >> ...
> >>
> >> Does it makes sense?
> >
> > This text says that using zlib in the extension is not necessary because
> > it's the default. But it doesn't say that using zlib in the extension is
> > illegal.
> >
> > I agree that there is no good reason to create a compression type
> > extension if you have zlib. But is there a good reason to forbid it?
> I think yes, if we create image with the extension set to zlib we
> prevent an older qemu from using that image. Furthermore, to allow older
> qemu using such images we need to create special conversion procedure
> which has to remove the extension header.
>
> If zlib is a "special compression type" which is always set by default
> without the extension header we'll get rid of such image conversion
> procedure and an older qemu could use it "as is"
>
> Might it work as a good reason?
>
> > It
> > only requires us to add artificial restrictions to code that would work
> > fine without them.
> >
> > Either way, if we want to reject such extensions, the spec needs to say
> > that it's illegal. And if the spec allows such images, we must accept
> > them.
> Yes, it's true
>
> The only reasons that zlib compression type even exists in the
> enumeration is to avoid ambiguity for users.
> For them it may be hard to understand why they can set zstd and cannot
> set zlib as compression type and to really set zlib they have to set no
> compression type to make the default zlib to apply.
>
> When a user set zlib as compression type the image is created as before
> the extension header were introduced.
>
> Reasonable?
> >
> >>> We can discuss whether the code or the spec should be changed. At the
> >>> moment, I don't see a good reason to make the restriction
> >>>
> >>>> +#ifdef DEBUG_EXT
> >>>> + printf("Qcow2: image compression type %s\n", s->compression_type);
> >>>> +#endif
> >>>> + break;
> >>>>
> >>>> case QCOW2_EXT_MAGIC_DATA_FILE:
> >>>> {
> >>>
> >>> We would save most of this code if we added a new field to the header
> >>> instead of adding a header extension. Not saying that we should
> >>> definitely do this, but let's discuss it at least.
> >>
> >> If we add the new field to the header will the older qemu be able to use
> >> it. Or we will add the header only if needed, i.e. if compression_type
> >> != zlib
> >
> > Increasing the header size is backwards compatible. Older qemu versions
> > should handle such images correctly. They would store the unknown part
> > of the header in s->unknown_header_fields and keep it unmodified when
> > updating the image header.
> >
> > We would still add the incompatible feature flag for non-zlib, of
> > course.
> so, we basically need to do the same: store compression type and forbid
> to use because of flag if not zlib.
>
> Sounds like it doesn't differ that much from the extension header approach.
It provides more or less the same functionality, but would probably make
this patch half the size because all of the code related to reading and
checking the header extension would go away. It also saves a few bytes
in the header cluster (4 bytes vs. 16 bytes).
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing
2019-06-28 14:24 ` Kevin Wolf
@ 2019-06-28 14:40 ` Denis Plotnikov
2019-06-28 14:54 ` Kevin Wolf
0 siblings, 1 reply; 29+ messages in thread
From: Denis Plotnikov @ 2019-06-28 14:40 UTC (permalink / raw)
To: Kevin Wolf
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block,
qemu-devel, armbru, mreitz
On 28.06.2019 17:24, Kevin Wolf wrote:
> Am 28.06.2019 um 14:56 hat Denis Plotnikov geschrieben:
>>
>>
>> On 28.06.2019 15:06, Kevin Wolf wrote:
>>> Am 28.06.2019 um 13:24 hat Denis Plotnikov geschrieben:
>>>>
>>>>
>>>> On 28.06.2019 13:23, Kevin Wolf wrote:
>>>>> Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
>>>>>> With the patch, qcow2 is able to process image compression type
>>>>>> defined in the image header and choose the corresponding method
>>>>>> for clusters compressing.
>>>>>>
>>>>>> Also, it rework the cluster compression code for adding more
>>>>>> compression types.
>>>>>>
>>>>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>>>>>> ---
>>>>>> block/qcow2.c | 103 ++++++++++++++++++++++++++++++++++++++++++++------
>>>>>> 1 file changed, 92 insertions(+), 11 deletions(-)
>>>>>>
>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>> index c4b5b93408..90f15cc3c9 100644
>>>>>> --- a/block/qcow2.c
>>>>>> +++ b/block/qcow2.c
>>>>>> @@ -400,11 +400,39 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>>>>> break;
>>>>>>
>>>>>> case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
>>>>>> + /* Compression type always goes with the compression type bit set */
>>>>>> + if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
>>>>>> + error_setg(errp,
>>>>>> + "compression_type_ext: "
>>>>>> + "expect compression type bit set");
>>>>>> + return -EINVAL;
>>>>>> + }
>>>>>> +
>>>>>> + ret = bdrv_pread(bs->file, offset, &s->compression_type, ext.len);
>>>>>> + s->compression_type = be32_to_cpu(s->compression_type);
>>>>>> +
>>>>>> + if (ret < 0) {
>>>>>> + error_setg_errno(errp, -ret,
>>>>>> + "ERROR: Could not read compression type");
>>>>>> + return ret;
>>>>>> + }
>>>>>> +
>>>>>> /*
>>>>>> - * Setting compression type to BDRVQcow2State->compression_type
>>>>>> - * from the image header is going to be here
>>>>>> + * The default compression type is not allowed when the extension
>>>>>> + * is present. ZLIB is used as the default compression type.
>>>>>> + * When compression type extension header is present then
>>>>>> + * compression_type should have a value different from the default.
>>>>>> */
>>>>>> - break;
>>>>>> + if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
>>>>>> + error_setg(errp,
>>>>>> + "compression_type_ext:"
>>>>>> + "invalid compression type %d",
>>>>>> + QCOW2_COMPRESSION_TYPE_ZLIB);
>>>>>> + }
>>>>>
>>>>> This is a restriction that the spec doesn't make, so strictly speaking
>>>>> this implementation wouldn't be compliant to the spec.
>>>> The idea is that ZLIB shouldn't appear in the compression type
>>>> extension. This allows image backward compatibility with an older qemu
>>>> if zlib is used.
>>>>
>>>> There is no reason to set ZLIB in the extension because an older qemu
>>>> knows how to tread ZLIB compressed clusters.
>>>>
>>>> The restriction aims to guarantee that.
>>>>
>>>> I tried to describe this case in the specification:
>>>> ...
>>>> When the compression type bit is not set, and the compression type
>>>> header extension is absent, ZLIB compression is used for compressed
>>>> clusters.
>>>>
>>>> Qemu versions older than 4.1 can use images created with compression
>>>> type ZLIB without any additional preparations and cannot use images
>>>> created with compression types != ZLIB.
>>>> ...
>>>>
>>>> Does it makes sense?
>>>
>>> This text says that using zlib in the extension is not necessary because
>>> it's the default. But it doesn't say that using zlib in the extension is
>>> illegal.
>>>
>>> I agree that there is no good reason to create a compression type
>>> extension if you have zlib. But is there a good reason to forbid it?
>> I think yes, if we create image with the extension set to zlib we
>> prevent an older qemu from using that image. Furthermore, to allow older
>> qemu using such images we need to create special conversion procedure
>> which has to remove the extension header.
>>
>> If zlib is a "special compression type" which is always set by default
>> without the extension header we'll get rid of such image conversion
>> procedure and an older qemu could use it "as is"
>>
>> Might it work as a good reason?
>>
>>> It
>>> only requires us to add artificial restrictions to code that would work
>>> fine without them.
>>>
>>> Either way, if we want to reject such extensions, the spec needs to say
>>> that it's illegal. And if the spec allows such images, we must accept
>>> them.
>> Yes, it's true
>>
>> The only reasons that zlib compression type even exists in the
>> enumeration is to avoid ambiguity for users.
>> For them it may be hard to understand why they can set zstd and cannot
>> set zlib as compression type and to really set zlib they have to set no
>> compression type to make the default zlib to apply.
>>
>> When a user set zlib as compression type the image is created as before
>> the extension header were introduced.
>>
>> Reasonable?
>>>
>>>>> We can discuss whether the code or the spec should be changed. At the
>>>>> moment, I don't see a good reason to make the restriction
>>>>>
>>>>>> +#ifdef DEBUG_EXT
>>>>>> + printf("Qcow2: image compression type %s\n", s->compression_type);
>>>>>> +#endif
>>>>>> + break;
>>>>>>
>>>>>> case QCOW2_EXT_MAGIC_DATA_FILE:
>>>>>> {
>>>>>
>>>>> We would save most of this code if we added a new field to the header
>>>>> instead of adding a header extension. Not saying that we should
>>>>> definitely do this, but let's discuss it at least.
>>>>
>>>> If we add the new field to the header will the older qemu be able to use
>>>> it. Or we will add the header only if needed, i.e. if compression_type
>>>> != zlib
>>>
>>> Increasing the header size is backwards compatible. Older qemu versions
>>> should handle such images correctly. They would store the unknown part
>>> of the header in s->unknown_header_fields and keep it unmodified when
>>> updating the image header.
>>>
>>> We would still add the incompatible feature flag for non-zlib, of
>>> course.
>> so, we basically need to do the same: store compression type and forbid
>> to use because of flag if not zlib.
>>
>> Sounds like it doesn't differ that much from the extension header approach.
>
> It provides more or less the same functionality, but would probably make
> this patch half the size because all of the code related to reading and
> checking the header extension would go away. It also saves a few bytes
> in the header cluster (4 bytes vs. 16 bytes).
ok, will re-do it that way.
Do you agree in general with how zlib compression type is treated?
Denis
>
> Kevin
>
--
Best,
Denis
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing
2019-06-28 14:40 ` Denis Plotnikov
@ 2019-06-28 14:54 ` Kevin Wolf
2019-06-28 15:03 ` Max Reitz
2019-06-28 19:34 ` Eric Blake
0 siblings, 2 replies; 29+ messages in thread
From: Kevin Wolf @ 2019-06-28 14:54 UTC (permalink / raw)
To: Denis Plotnikov
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block,
qemu-devel, armbru, mreitz
Am 28.06.2019 um 16:40 hat Denis Plotnikov geschrieben:
>
>
> On 28.06.2019 17:24, Kevin Wolf wrote:
> > Am 28.06.2019 um 14:56 hat Denis Plotnikov geschrieben:
> >>
> >>
> >> On 28.06.2019 15:06, Kevin Wolf wrote:
> >>> Am 28.06.2019 um 13:24 hat Denis Plotnikov geschrieben:
> >>>>
> >>>>
> >>>> On 28.06.2019 13:23, Kevin Wolf wrote:
> >>>>> Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
> >>>>>> With the patch, qcow2 is able to process image compression type
> >>>>>> defined in the image header and choose the corresponding method
> >>>>>> for clusters compressing.
> >>>>>>
> >>>>>> Also, it rework the cluster compression code for adding more
> >>>>>> compression types.
> >>>>>>
> >>>>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> >>>>>> ---
> >>>>>> block/qcow2.c | 103 ++++++++++++++++++++++++++++++++++++++++++++------
> >>>>>> 1 file changed, 92 insertions(+), 11 deletions(-)
> >>>>>>
> >>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
> >>>>>> index c4b5b93408..90f15cc3c9 100644
> >>>>>> --- a/block/qcow2.c
> >>>>>> +++ b/block/qcow2.c
> >>>>>> @@ -400,11 +400,39 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
> >>>>>> break;
> >>>>>>
> >>>>>> case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
> >>>>>> + /* Compression type always goes with the compression type bit set */
> >>>>>> + if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
> >>>>>> + error_setg(errp,
> >>>>>> + "compression_type_ext: "
> >>>>>> + "expect compression type bit set");
> >>>>>> + return -EINVAL;
> >>>>>> + }
> >>>>>> +
> >>>>>> + ret = bdrv_pread(bs->file, offset, &s->compression_type, ext.len);
> >>>>>> + s->compression_type = be32_to_cpu(s->compression_type);
> >>>>>> +
> >>>>>> + if (ret < 0) {
> >>>>>> + error_setg_errno(errp, -ret,
> >>>>>> + "ERROR: Could not read compression type");
> >>>>>> + return ret;
> >>>>>> + }
> >>>>>> +
> >>>>>> /*
> >>>>>> - * Setting compression type to BDRVQcow2State->compression_type
> >>>>>> - * from the image header is going to be here
> >>>>>> + * The default compression type is not allowed when the extension
> >>>>>> + * is present. ZLIB is used as the default compression type.
> >>>>>> + * When compression type extension header is present then
> >>>>>> + * compression_type should have a value different from the default.
> >>>>>> */
> >>>>>> - break;
> >>>>>> + if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
> >>>>>> + error_setg(errp,
> >>>>>> + "compression_type_ext:"
> >>>>>> + "invalid compression type %d",
> >>>>>> + QCOW2_COMPRESSION_TYPE_ZLIB);
> >>>>>> + }
> >>>>>
> >>>>> This is a restriction that the spec doesn't make, so strictly speaking
> >>>>> this implementation wouldn't be compliant to the spec.
> >>>> The idea is that ZLIB shouldn't appear in the compression type
> >>>> extension. This allows image backward compatibility with an older qemu
> >>>> if zlib is used.
> >>>>
> >>>> There is no reason to set ZLIB in the extension because an older qemu
> >>>> knows how to tread ZLIB compressed clusters.
> >>>>
> >>>> The restriction aims to guarantee that.
> >>>>
> >>>> I tried to describe this case in the specification:
> >>>> ...
> >>>> When the compression type bit is not set, and the compression type
> >>>> header extension is absent, ZLIB compression is used for compressed
> >>>> clusters.
> >>>>
> >>>> Qemu versions older than 4.1 can use images created with compression
> >>>> type ZLIB without any additional preparations and cannot use images
> >>>> created with compression types != ZLIB.
> >>>> ...
> >>>>
> >>>> Does it makes sense?
> >>>
> >>> This text says that using zlib in the extension is not necessary because
> >>> it's the default. But it doesn't say that using zlib in the extension is
> >>> illegal.
> >>>
> >>> I agree that there is no good reason to create a compression type
> >>> extension if you have zlib. But is there a good reason to forbid it?
> >> I think yes, if we create image with the extension set to zlib we
> >> prevent an older qemu from using that image. Furthermore, to allow older
> >> qemu using such images we need to create special conversion procedure
> >> which has to remove the extension header.
> >>
> >> If zlib is a "special compression type" which is always set by default
> >> without the extension header we'll get rid of such image conversion
> >> procedure and an older qemu could use it "as is"
> >>
> >> Might it work as a good reason?
> >>
> >>> It
> >>> only requires us to add artificial restrictions to code that would work
> >>> fine without them.
> >>>
> >>> Either way, if we want to reject such extensions, the spec needs to say
> >>> that it's illegal. And if the spec allows such images, we must accept
> >>> them.
> >> Yes, it's true
> >>
> >> The only reasons that zlib compression type even exists in the
> >> enumeration is to avoid ambiguity for users.
> >> For them it may be hard to understand why they can set zstd and cannot
> >> set zlib as compression type and to really set zlib they have to set no
> >> compression type to make the default zlib to apply.
> >>
> >> When a user set zlib as compression type the image is created as before
> >> the extension header were introduced.
> >>
> >> Reasonable?
> >>>
> >>>>> We can discuss whether the code or the spec should be changed. At the
> >>>>> moment, I don't see a good reason to make the restriction
> >>>>>
> >>>>>> +#ifdef DEBUG_EXT
> >>>>>> + printf("Qcow2: image compression type %s\n", s->compression_type);
> >>>>>> +#endif
> >>>>>> + break;
> >>>>>>
> >>>>>> case QCOW2_EXT_MAGIC_DATA_FILE:
> >>>>>> {
> >>>>>
> >>>>> We would save most of this code if we added a new field to the header
> >>>>> instead of adding a header extension. Not saying that we should
> >>>>> definitely do this, but let's discuss it at least.
> >>>>
> >>>> If we add the new field to the header will the older qemu be able to use
> >>>> it. Or we will add the header only if needed, i.e. if compression_type
> >>>> != zlib
> >>>
> >>> Increasing the header size is backwards compatible. Older qemu versions
> >>> should handle such images correctly. They would store the unknown part
> >>> of the header in s->unknown_header_fields and keep it unmodified when
> >>> updating the image header.
> >>>
> >>> We would still add the incompatible feature flag for non-zlib, of
> >>> course.
> >> so, we basically need to do the same: store compression type and forbid
> >> to use because of flag if not zlib.
> >>
> >> Sounds like it doesn't differ that much from the extension header approach.
> >
> > It provides more or less the same functionality, but would probably make
> > this patch half the size because all of the code related to reading and
> > checking the header extension would go away. It also saves a few bytes
> > in the header cluster (4 bytes vs. 16 bytes).
> ok, will re-do it that way.
>
> Do you agree in general with how zlib compression type is treated?
As I said, I think both ways are justifiable as long as we stay
consistent between qemu and spec.
I'd prefer to allow zlib in the extension, you'd prefer to forbid it.
So I'd like to hear opinions from some more people on which way they
prefer.
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing
2019-06-28 14:54 ` Kevin Wolf
@ 2019-06-28 15:03 ` Max Reitz
2019-06-28 15:14 ` Denis Plotnikov
2019-06-28 19:34 ` Eric Blake
1 sibling, 1 reply; 29+ messages in thread
From: Max Reitz @ 2019-06-28 15:03 UTC (permalink / raw)
To: Kevin Wolf, Denis Plotnikov
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block, armbru,
qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 8537 bytes --]
On 28.06.19 16:54, Kevin Wolf wrote:
> Am 28.06.2019 um 16:40 hat Denis Plotnikov geschrieben:
>>
>>
>> On 28.06.2019 17:24, Kevin Wolf wrote:
>>> Am 28.06.2019 um 14:56 hat Denis Plotnikov geschrieben:
>>>>
>>>>
>>>> On 28.06.2019 15:06, Kevin Wolf wrote:
>>>>> Am 28.06.2019 um 13:24 hat Denis Plotnikov geschrieben:
>>>>>>
>>>>>>
>>>>>> On 28.06.2019 13:23, Kevin Wolf wrote:
>>>>>>> Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
>>>>>>>> With the patch, qcow2 is able to process image compression type
>>>>>>>> defined in the image header and choose the corresponding method
>>>>>>>> for clusters compressing.
>>>>>>>>
>>>>>>>> Also, it rework the cluster compression code for adding more
>>>>>>>> compression types.
>>>>>>>>
>>>>>>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>>>>>>>> ---
>>>>>>>> block/qcow2.c | 103 ++++++++++++++++++++++++++++++++++++++++++++------
>>>>>>>> 1 file changed, 92 insertions(+), 11 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>>>> index c4b5b93408..90f15cc3c9 100644
>>>>>>>> --- a/block/qcow2.c
>>>>>>>> +++ b/block/qcow2.c
>>>>>>>> @@ -400,11 +400,39 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>>>>>>> break;
>>>>>>>>
>>>>>>>> case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
>>>>>>>> + /* Compression type always goes with the compression type bit set */
>>>>>>>> + if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
>>>>>>>> + error_setg(errp,
>>>>>>>> + "compression_type_ext: "
>>>>>>>> + "expect compression type bit set");
>>>>>>>> + return -EINVAL;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + ret = bdrv_pread(bs->file, offset, &s->compression_type, ext.len);
>>>>>>>> + s->compression_type = be32_to_cpu(s->compression_type);
>>>>>>>> +
>>>>>>>> + if (ret < 0) {
>>>>>>>> + error_setg_errno(errp, -ret,
>>>>>>>> + "ERROR: Could not read compression type");
>>>>>>>> + return ret;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> /*
>>>>>>>> - * Setting compression type to BDRVQcow2State->compression_type
>>>>>>>> - * from the image header is going to be here
>>>>>>>> + * The default compression type is not allowed when the extension
>>>>>>>> + * is present. ZLIB is used as the default compression type.
>>>>>>>> + * When compression type extension header is present then
>>>>>>>> + * compression_type should have a value different from the default.
>>>>>>>> */
>>>>>>>> - break;
>>>>>>>> + if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
>>>>>>>> + error_setg(errp,
>>>>>>>> + "compression_type_ext:"
>>>>>>>> + "invalid compression type %d",
>>>>>>>> + QCOW2_COMPRESSION_TYPE_ZLIB);
>>>>>>>> + }
>>>>>>>
>>>>>>> This is a restriction that the spec doesn't make, so strictly speaking
>>>>>>> this implementation wouldn't be compliant to the spec.
>>>>>> The idea is that ZLIB shouldn't appear in the compression type
>>>>>> extension. This allows image backward compatibility with an older qemu
>>>>>> if zlib is used.
>>>>>>
>>>>>> There is no reason to set ZLIB in the extension because an older qemu
>>>>>> knows how to tread ZLIB compressed clusters.
>>>>>>
>>>>>> The restriction aims to guarantee that.
>>>>>>
>>>>>> I tried to describe this case in the specification:
>>>>>> ...
>>>>>> When the compression type bit is not set, and the compression type
>>>>>> header extension is absent, ZLIB compression is used for compressed
>>>>>> clusters.
>>>>>>
>>>>>> Qemu versions older than 4.1 can use images created with compression
>>>>>> type ZLIB without any additional preparations and cannot use images
>>>>>> created with compression types != ZLIB.
>>>>>> ...
>>>>>>
>>>>>> Does it makes sense?
>>>>>
>>>>> This text says that using zlib in the extension is not necessary because
>>>>> it's the default. But it doesn't say that using zlib in the extension is
>>>>> illegal.
>>>>>
>>>>> I agree that there is no good reason to create a compression type
>>>>> extension if you have zlib. But is there a good reason to forbid it?
>>>> I think yes, if we create image with the extension set to zlib we
>>>> prevent an older qemu from using that image. Furthermore, to allow older
>>>> qemu using such images we need to create special conversion procedure
>>>> which has to remove the extension header.
>>>>
>>>> If zlib is a "special compression type" which is always set by default
>>>> without the extension header we'll get rid of such image conversion
>>>> procedure and an older qemu could use it "as is"
>>>>
>>>> Might it work as a good reason?
>>>>
>>>>> It
>>>>> only requires us to add artificial restrictions to code that would work
>>>>> fine without them.
>>>>>
>>>>> Either way, if we want to reject such extensions, the spec needs to say
>>>>> that it's illegal. And if the spec allows such images, we must accept
>>>>> them.
>>>> Yes, it's true
>>>>
>>>> The only reasons that zlib compression type even exists in the
>>>> enumeration is to avoid ambiguity for users.
>>>> For them it may be hard to understand why they can set zstd and cannot
>>>> set zlib as compression type and to really set zlib they have to set no
>>>> compression type to make the default zlib to apply.
>>>>
>>>> When a user set zlib as compression type the image is created as before
>>>> the extension header were introduced.
>>>>
>>>> Reasonable?
>>>>>
>>>>>>> We can discuss whether the code or the spec should be changed. At the
>>>>>>> moment, I don't see a good reason to make the restriction
>>>>>>>
>>>>>>>> +#ifdef DEBUG_EXT
>>>>>>>> + printf("Qcow2: image compression type %s\n", s->compression_type);
>>>>>>>> +#endif
>>>>>>>> + break;
>>>>>>>>
>>>>>>>> case QCOW2_EXT_MAGIC_DATA_FILE:
>>>>>>>> {
>>>>>>>
>>>>>>> We would save most of this code if we added a new field to the header
>>>>>>> instead of adding a header extension. Not saying that we should
>>>>>>> definitely do this, but let's discuss it at least.
>>>>>>
>>>>>> If we add the new field to the header will the older qemu be able to use
>>>>>> it. Or we will add the header only if needed, i.e. if compression_type
>>>>>> != zlib
>>>>>
>>>>> Increasing the header size is backwards compatible. Older qemu versions
>>>>> should handle such images correctly. They would store the unknown part
>>>>> of the header in s->unknown_header_fields and keep it unmodified when
>>>>> updating the image header.
>>>>>
>>>>> We would still add the incompatible feature flag for non-zlib, of
>>>>> course.
>>>> so, we basically need to do the same: store compression type and forbid
>>>> to use because of flag if not zlib.
>>>>
>>>> Sounds like it doesn't differ that much from the extension header approach.
>>>
>>> It provides more or less the same functionality, but would probably make
>>> this patch half the size because all of the code related to reading and
>>> checking the header extension would go away. It also saves a few bytes
>>> in the header cluster (4 bytes vs. 16 bytes).
>> ok, will re-do it that way.
>>
>> Do you agree in general with how zlib compression type is treated?
>
> As I said, I think both ways are justifiable as long as we stay
> consistent between qemu and spec.
>
> I'd prefer to allow zlib in the extension, you'd prefer to forbid it.
> So I'd like to hear opinions from some more people on which way they
> prefer.
I don’t think it’s any better to completely forbid it than to just
recommend in the spec that software should not set this field to zlib to
ensure backwards compatibility.
I see the point of forbidding it, but if I were to know nothing of qcow2
and read the spec, I guess I’d find it a bit weird to read “If this
field is not present, the compression type is zlib; if it is, it is not
zlib, but the specified value.” I’d ask myself why it isn’t simply “The
compression type is given by this field, it defaults to zlib.”
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing
2019-06-28 15:03 ` Max Reitz
@ 2019-06-28 15:14 ` Denis Plotnikov
0 siblings, 0 replies; 29+ messages in thread
From: Denis Plotnikov @ 2019-06-28 15:14 UTC (permalink / raw)
To: Max Reitz, Kevin Wolf
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block, armbru,
qemu-devel
On 28.06.2019 18:03, Max Reitz wrote:
> On 28.06.19 16:54, Kevin Wolf wrote:
>> Am 28.06.2019 um 16:40 hat Denis Plotnikov geschrieben:
>>>
>>>
>>> On 28.06.2019 17:24, Kevin Wolf wrote:
>>>> Am 28.06.2019 um 14:56 hat Denis Plotnikov geschrieben:
>>>>>
>>>>>
>>>>> On 28.06.2019 15:06, Kevin Wolf wrote:
>>>>>> Am 28.06.2019 um 13:24 hat Denis Plotnikov geschrieben:
>>>>>>>
>>>>>>>
>>>>>>> On 28.06.2019 13:23, Kevin Wolf wrote:
>>>>>>>> Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
>>>>>>>>> With the patch, qcow2 is able to process image compression type
>>>>>>>>> defined in the image header and choose the corresponding method
>>>>>>>>> for clusters compressing.
>>>>>>>>>
>>>>>>>>> Also, it rework the cluster compression code for adding more
>>>>>>>>> compression types.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>>>>>>>>> ---
>>>>>>>>> block/qcow2.c | 103 ++++++++++++++++++++++++++++++++++++++++++++------
>>>>>>>>> 1 file changed, 92 insertions(+), 11 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>>>>>>> index c4b5b93408..90f15cc3c9 100644
>>>>>>>>> --- a/block/qcow2.c
>>>>>>>>> +++ b/block/qcow2.c
>>>>>>>>> @@ -400,11 +400,39 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
>>>>>>>>> break;
>>>>>>>>>
>>>>>>>>> case QCOW2_EXT_MAGIC_COMPRESSION_TYPE:
>>>>>>>>> + /* Compression type always goes with the compression type bit set */
>>>>>>>>> + if (!(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
>>>>>>>>> + error_setg(errp,
>>>>>>>>> + "compression_type_ext: "
>>>>>>>>> + "expect compression type bit set");
>>>>>>>>> + return -EINVAL;
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + ret = bdrv_pread(bs->file, offset, &s->compression_type, ext.len);
>>>>>>>>> + s->compression_type = be32_to_cpu(s->compression_type);
>>>>>>>>> +
>>>>>>>>> + if (ret < 0) {
>>>>>>>>> + error_setg_errno(errp, -ret,
>>>>>>>>> + "ERROR: Could not read compression type");
>>>>>>>>> + return ret;
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> /*
>>>>>>>>> - * Setting compression type to BDRVQcow2State->compression_type
>>>>>>>>> - * from the image header is going to be here
>>>>>>>>> + * The default compression type is not allowed when the extension
>>>>>>>>> + * is present. ZLIB is used as the default compression type.
>>>>>>>>> + * When compression type extension header is present then
>>>>>>>>> + * compression_type should have a value different from the default.
>>>>>>>>> */
>>>>>>>>> - break;
>>>>>>>>> + if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
>>>>>>>>> + error_setg(errp,
>>>>>>>>> + "compression_type_ext:"
>>>>>>>>> + "invalid compression type %d",
>>>>>>>>> + QCOW2_COMPRESSION_TYPE_ZLIB);
>>>>>>>>> + }
>>>>>>>>
>>>>>>>> This is a restriction that the spec doesn't make, so strictly speaking
>>>>>>>> this implementation wouldn't be compliant to the spec.
>>>>>>> The idea is that ZLIB shouldn't appear in the compression type
>>>>>>> extension. This allows image backward compatibility with an older qemu
>>>>>>> if zlib is used.
>>>>>>>
>>>>>>> There is no reason to set ZLIB in the extension because an older qemu
>>>>>>> knows how to tread ZLIB compressed clusters.
>>>>>>>
>>>>>>> The restriction aims to guarantee that.
>>>>>>>
>>>>>>> I tried to describe this case in the specification:
>>>>>>> ...
>>>>>>> When the compression type bit is not set, and the compression type
>>>>>>> header extension is absent, ZLIB compression is used for compressed
>>>>>>> clusters.
>>>>>>>
>>>>>>> Qemu versions older than 4.1 can use images created with compression
>>>>>>> type ZLIB without any additional preparations and cannot use images
>>>>>>> created with compression types != ZLIB.
>>>>>>> ...
>>>>>>>
>>>>>>> Does it makes sense?
>>>>>>
>>>>>> This text says that using zlib in the extension is not necessary because
>>>>>> it's the default. But it doesn't say that using zlib in the extension is
>>>>>> illegal.
>>>>>>
>>>>>> I agree that there is no good reason to create a compression type
>>>>>> extension if you have zlib. But is there a good reason to forbid it?
>>>>> I think yes, if we create image with the extension set to zlib we
>>>>> prevent an older qemu from using that image. Furthermore, to allow older
>>>>> qemu using such images we need to create special conversion procedure
>>>>> which has to remove the extension header.
>>>>>
>>>>> If zlib is a "special compression type" which is always set by default
>>>>> without the extension header we'll get rid of such image conversion
>>>>> procedure and an older qemu could use it "as is"
>>>>>
>>>>> Might it work as a good reason?
>>>>>
>>>>>> It
>>>>>> only requires us to add artificial restrictions to code that would work
>>>>>> fine without them.
>>>>>>
>>>>>> Either way, if we want to reject such extensions, the spec needs to say
>>>>>> that it's illegal. And if the spec allows such images, we must accept
>>>>>> them.
>>>>> Yes, it's true
>>>>>
>>>>> The only reasons that zlib compression type even exists in the
>>>>> enumeration is to avoid ambiguity for users.
>>>>> For them it may be hard to understand why they can set zstd and cannot
>>>>> set zlib as compression type and to really set zlib they have to set no
>>>>> compression type to make the default zlib to apply.
>>>>>
>>>>> When a user set zlib as compression type the image is created as before
>>>>> the extension header were introduced.
>>>>>
>>>>> Reasonable?
>>>>>>
>>>>>>>> We can discuss whether the code or the spec should be changed. At the
>>>>>>>> moment, I don't see a good reason to make the restriction
>>>>>>>>
>>>>>>>>> +#ifdef DEBUG_EXT
>>>>>>>>> + printf("Qcow2: image compression type %s\n", s->compression_type);
>>>>>>>>> +#endif
>>>>>>>>> + break;
>>>>>>>>>
>>>>>>>>> case QCOW2_EXT_MAGIC_DATA_FILE:
>>>>>>>>> {
>>>>>>>>
>>>>>>>> We would save most of this code if we added a new field to the header
>>>>>>>> instead of adding a header extension. Not saying that we should
>>>>>>>> definitely do this, but let's discuss it at least.
>>>>>>>
>>>>>>> If we add the new field to the header will the older qemu be able to use
>>>>>>> it. Or we will add the header only if needed, i.e. if compression_type
>>>>>>> != zlib
>>>>>>
>>>>>> Increasing the header size is backwards compatible. Older qemu versions
>>>>>> should handle such images correctly. They would store the unknown part
>>>>>> of the header in s->unknown_header_fields and keep it unmodified when
>>>>>> updating the image header.
>>>>>>
>>>>>> We would still add the incompatible feature flag for non-zlib, of
>>>>>> course.
>>>>> so, we basically need to do the same: store compression type and forbid
>>>>> to use because of flag if not zlib.
>>>>>
>>>>> Sounds like it doesn't differ that much from the extension header approach.
>>>>
>>>> It provides more or less the same functionality, but would probably make
>>>> this patch half the size because all of the code related to reading and
>>>> checking the header extension would go away. It also saves a few bytes
>>>> in the header cluster (4 bytes vs. 16 bytes).
>>> ok, will re-do it that way.
>>>
>>> Do you agree in general with how zlib compression type is treated?
>>
>> As I said, I think both ways are justifiable as long as we stay
>> consistent between qemu and spec.
>>
>> I'd prefer to allow zlib in the extension, you'd prefer to forbid it.
>> So I'd like to hear opinions from some more people on which way they
>> prefer.
>
> I don’t think it’s any better to completely forbid it than to just
> recommend in the spec that software should not set this field to zlib to
> ensure backwards compatibility.
>
> I see the point of forbidding it, but if I were to know nothing of qcow2
> and read the spec, I guess I’d find it a bit weird to read “If this
> field is not present, the compression type is zlib; if it is, it is not
> zlib, but the specified value.” I’d ask myself why it isn’t simply “The
> compression type is given by this field, it defaults to zlib.”
>
As I understood, if we go with the header extending we'll add a new
field in the header anyway which always hold some value. By, default it
will be zlib.
So, the only question now is whether we allow zlib value with the
incompatible feature flag set. My point is if compression type is zlib
the corresponding incompatible feature flag should NOT be set.
In that case an older qemu will put the compression type to
s->unknown_header_fields and run with zlib, otherwise it couldn't open
the file.
Denis
> Max
>
--
Best,
Denis
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing
2019-06-28 14:54 ` Kevin Wolf
2019-06-28 15:03 ` Max Reitz
@ 2019-06-28 19:34 ` Eric Blake
2019-07-02 12:34 ` Denis Plotnikov
1 sibling, 1 reply; 29+ messages in thread
From: Eric Blake @ 2019-06-28 19:34 UTC (permalink / raw)
To: Kevin Wolf, Denis Plotnikov
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block,
qemu-devel, armbru, mreitz
[-- Attachment #1.1: Type: text/plain, Size: 3924 bytes --]
On 6/28/19 9:54 AM, Kevin Wolf wrote:
>>>>>>> We would save most of this code if we added a new field to the header
>>>>>>> instead of adding a header extension. Not saying that we should
>>>>>>> definitely do this, but let's discuss it at least.
>>>>>>
>>>>>> If we add the new field to the header will the older qemu be able to use
>>>>>> it. Or we will add the header only if needed, i.e. if compression_type
>>>>>> != zlib
>>>>>
>>>>> Increasing the header size is backwards compatible. Older qemu versions
>>>>> should handle such images correctly. They would store the unknown part
>>>>> of the header in s->unknown_header_fields and keep it unmodified when
>>>>> updating the image header.
>>>>>
>>>>> We would still add the incompatible feature flag for non-zlib, of
>>>>> course.
>>>> so, we basically need to do the same: store compression type and forbid
>>>> to use because of flag if not zlib.
>>>>
>>>> Sounds like it doesn't differ that much from the extension header approach.
>>>
>>> It provides more or less the same functionality, but would probably make
>>> this patch half the size because all of the code related to reading and
>>> checking the header extension would go away. It also saves a few bytes
>>> in the header cluster (4 bytes vs. 16 bytes).
>> ok, will re-do it that way.
>>
>> Do you agree in general with how zlib compression type is treated?
>
> As I said, I think both ways are justifiable as long as we stay
> consistent between qemu and spec.
>
> I'd prefer to allow zlib in the extension, you'd prefer to forbid it.
> So I'd like to hear opinions from some more people on which way they
> prefer.
My preferences - use a 4 byte header field, and require the incompatible
feature bit if the field is non-zero. The standard should allow someone
to explicitly request zlib compression (done by leaving the incompatible
bit clear, then specifying a header length of 108 instead of 104, but
leaving the compression field at 104-107 at 0), to implicitly request
zlib compression (done by leaving the incompatible bit clear, and
specifying a header length of 104); or to explicitly request some other
compression (done by setting the incompatible bit, specifying a header
length of 108, and putting a non-zero value in the compression field
104-107).
Under these rules, if you implicitly or explicitly request zlib, your
image can be opened without problems by both older and newer qemu. If
you explicitly request zstd, your image will fail to open by older qemu
(good, because they would misinterpret compressed clusters), and work
with newer qemu. And since providing a 108-byte header works just fine
with older qemu as long as the header contains 0, I recommend that we
just always make newer qemu provide that field (even if it is explicitly
set to zlib), as that is less complicated than only providing the larger
header for non-zlib files (we still have to parse 104-byte headers, but
that doesn't mean we have to create brand-new files that way).
There's one more corner case. I recommend that the standard require that
it be an error to set the incompatible feature bit but use a header size
of 104 - if you have an imabe like that, the image would be treated as
using zlib (implicitly due to the header size), so older images _could_
use it other than the fact that they don't recognize the incompatible
feature bit. On the other hand, requiring such an image to be rejected
is a bit of a stretch - no qemu (whether one that understands the
feature bit or one that does not) would misinterpret the image contents
as being zlib compressed, if it had not been for the bit being set. So
in this corner case, I'm fine if you end up documenting whatever is
easiest to code.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 3/3] qcow2: add zstd cluster compression
2019-06-28 11:57 ` Kevin Wolf
@ 2019-07-02 12:33 ` Denis Plotnikov
2019-07-02 12:49 ` Denis Plotnikov
1 sibling, 0 replies; 29+ messages in thread
From: Denis Plotnikov @ 2019-07-02 12:33 UTC (permalink / raw)
To: Kevin Wolf
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block,
qemu-devel, armbru, mreitz
Thanks for reviewing, I'll take into account the suggestions below and
send the next version of the series soon.
On 28.06.2019 14:57, Kevin Wolf wrote:
> Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
>> zstd significantly reduces cluster compression time.
>> It provides better compression performance maintaining
>> the same level of compression ratio in comparison with
>> zlib, which, by the moment, has been the only compression
>> method available.
>>
>> The performance test results:
>> Test compresses and decompresses qemu qcow2 image with just
>> installed rhel-7.6 guest.
>> Image cluster size: 64K. Image on disk size: 2.2G
>>
>> The test was conducted with brd disk to reduce the influence
>> of disk subsystem to the test results.
>> The results is given in seconds.
>>
>> compress cmd:
>> time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
>> src.img [zlib|zstd]_compressed.img
>> decompress cmd
>> time ./qemu-img convert -O qcow2
>> [zlib|zstd]_compressed.img uncompressed.img
>>
>> compression decompression
>> zlib zstd zlib zstd
>> ------------------------------------------------------------
>> real 65.5 16.3 (-75 %) 1.9 1.6 (-16 %)
>> user 65.0 15.8 5.3 2.5
>> sys 3.3 0.2 2.0 2.0
>>
>> Both ZLIB and ZSTD gave the same compression ratio: 1.57
>> compressed image size in both cases: 1.4G
>>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> ---
>> block/qcow2.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> configure | 26 ++++++++++++++++
>> 2 files changed, 108 insertions(+)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 90f15cc3c9..58901f9f79 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -26,6 +26,7 @@
>>
>> #define ZLIB_CONST
>> #include <zlib.h>
>> +#include <zstd.h>
>>
>> #include "block/block_int.h"
>> #include "block/qdict.h"
>> @@ -1553,6 +1554,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>> case QCOW2_COMPRESSION_TYPE_ZLIB:
>> break;
>>
>> + case QCOW2_COMPRESSION_TYPE_ZSTD:
>> + break;
>
> If we don't intend to add any code here, why not just add another case
> label to the existing break?
>
>> default:
>> error_setg(errp, "Unknown compression type");
>> ret = -EINVAL;
>> @@ -3286,6 +3290,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
>> * ZLIB shouldn't be here since it's the default
>> */
>> switch (qcow2_opts->compression_type) {
>> + case QCOW2_COMPRESSION_TYPE_ZSTD:
>> + break;
>> +
>> default:
>> error_setg_errno(errp, -EINVAL, "Unknown compression type");
>> goto out;
>> @@ -4113,6 +4120,73 @@ static ssize_t zlib_decompress(void *dest, size_t dest_size,
>> return ret;
>> }
>>
>> +/*
>> + * zstd_compress()
>> + *
>> + * @dest - destination buffer, @dest_size bytes
>> + * @src - source buffer, @src_size bytes
>> + *
>> + * Returns: compressed size on success
>> + * -1 on any error
>> + */
>> +
>> +static ssize_t zstd_compress(void *dest, size_t dest_size,
>> + const void *src, size_t src_size)
>> +{
>> + /* steal some bytes to store compressed chunk size */
>> + size_t ret;
>
> This ends up in a file, so this needs to be a fixed number of bytes.
> size_t varies between different platforms, so it is not acceptable.
>
> If I understand correctly, the maximum for this is the cluster size, so
> uint32_t should be right.
>
> This isn't plain the zstd compression format any more, so it needs to be
> described in the qcow2 spec.
>
>> + size_t *c_size = dest;
>> + char *d_buf = dest;
>> + d_buf += sizeof(ret);
>
> char *d_bug = dest + sizeof(ret);
>
>> + dest_size -= sizeof(ret);
>
> We don't want to end up with an integer overflow, so before this:
>
> if (dest_size < sizeof(ret)) {
> return -ENOMEM;
> }
>
>> + ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
>> +
>> + if (ZSTD_isError(ret)) {
>> + return -1;
>> + }
>
> Need an error code here, not just -1. In particular, we need to
> distinguish cases where the buffer was too small and uncompressed data
> should be written instead (ENOMEM) from real errors that should be
> returned to the caller (EIO).
>
>> +
>> + /* store the compressed chunk size in the very beginning of the buffer */
>> + *c_size = ret;
>> +
>> + return ret + sizeof(ret);
>> +}
>> +
>> +/*
>> + * zstd_decompress()
>> + *
>> + * Decompress some data (not more than @src_size bytes) to produce exactly
>> + * @dest_size bytes.
>> + *
>> + * @dest - destination buffer, @dest_size bytes
>> + * @src - source buffer, @src_size bytes
>> + *
>> + * Returns: 0 on success
>> + * -1 on fail
>> + */
>> +
>> +static ssize_t zstd_decompress(void *dest, size_t dest_size,
>> + const void *src, size_t src_size)
>
> Indentation is off.
>
>> +{
>> + size_t ret;
>> + /*
>> + * zstd decompress wants to know the exact lenght of the data
>> + * for that purpose, zstd_compress stores the length in the
>> + * very beginning of the compressed buffer
>> + */
>> + const size_t *s_size = src;
>> + const char *s_buf = src;
>> + s_buf += sizeof(size_t);
>
> Single line: const char *s_buf = src + sizeof(size_t);
>
> Of course, size_t is wrong here, too. And above you used sizeof() on a
> variable and here it's on the type. I think we should stay consistent.
>
> You're lacking a check against src_size. A malicious image could make
> use read beyond the end of the buffer. (Also consider that src_size
> could be smaller than sizeof(size_t).)
>
>> + ret = ZSTD_decompress(dest, dest_size, s_buf, *s_size);
>> +
>> + if (ZSTD_isError(ret)) {
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> #define MAX_COMPRESS_THREADS 4
>>
>> typedef ssize_t (*Qcow2CompressFunc)(void *dest, size_t dest_size,
>> @@ -4189,6 +4263,10 @@ qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
>> fn = zlib_compress;
>> break;
>>
>> + case QCOW2_COMPRESSION_TYPE_ZSTD:
>> + fn = zstd_compress;
>> + break;
>> +
>> default:
>> return -ENOTSUP;
>> }
>> @@ -4208,6 +4286,10 @@ qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
>> fn = zlib_decompress;
>> break;
>>
>> + case QCOW2_COMPRESSION_TYPE_ZSTD:
>> + fn = zstd_decompress;
>> + break;
>> +
>> default:
>> return -ENOTSUP;
>> }
>> diff --git a/configure b/configure
>> index 1c563a7027..c90716189c 100755
>> --- a/configure
>> +++ b/configure
>> @@ -433,6 +433,7 @@ opengl_dmabuf="no"
>> cpuid_h="no"
>> avx2_opt=""
>> zlib="yes"
>> +zstd="yes"
>
> This should be zstd="" so that a missing library will automatically
> disable it instead of producing an error. (Building QEMU without zlib is
> impossible, but building it without ZSTD should work.)
>
>> capstone=""
>> lzo=""
>> snappy=""
>> @@ -1317,6 +1318,8 @@ for opt do
>> ;;
>> --disable-zlib-test) zlib="no"
>> ;;
>> + --disable-zstd-test) zstd="no"
>> + ;;
>
> Instead of this one, after making the above change, options
> --disable-zstd and --enable-zstd should be introduced that set
> zstd="yes" (that actually does produce an error if it's not available)
> or zstd="no".
>
>> --disable-lzo) lzo="no"
>> ;;
>> --enable-lzo) lzo="yes"
>> @@ -3702,6 +3705,29 @@ EOF
>> fi
>> fi
>>
>> +#########################################
>> +# zstd check
>> +
>> +if test "$zstd" != "no" ; then
>> + if $pkg_config --exists libzstd; then
>> + zstd_cflags=$($pkg_config --cflags libzstd)
>> + zstd_libs=$($pkg_config --libs libzstd)
>> + QEMU_CFLAGS="$zstd_cflags $QEMU_CFLAGS"
>> + LIBS="$zstd_libs $LIBS"
>> + else
>> + cat > $TMPC << EOF
>> +#include <zstd.h>
>> +int main(void) { ZSTD_versionNumber(); return 0; }
>> +EOF
>> + if compile_prog "" "-lzstd" ; then
>> + LIBS="$LIBS -lzstd"
>> + else
>> + error_exit "zstd check failed" \
>> + "Make sure to have the zstd libs and headers installed."
>> + fi
>
> This needs to be changed, too, to get the desired behaviour. Model it
> after bzip2 or lzo support checks:
>
> if compile_prog "" "-lbz2" ; then
> bzip2="yes"
> else
> if test "$bzip2" = "yes"; then
> feature_not_found "libbzip2" "Install libbzip2 devel"
> fi
> bzip2="no"
> fi
>
>> + fi
>> +fi
>> +
>> ##########################################
>> # SHA command probe for modules
>> if test "$modules" = yes; then
>
> Kevin
>
--
Best,
Denis
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing
2019-06-28 19:34 ` Eric Blake
@ 2019-07-02 12:34 ` Denis Plotnikov
0 siblings, 0 replies; 29+ messages in thread
From: Denis Plotnikov @ 2019-07-02 12:34 UTC (permalink / raw)
To: Eric Blake, Kevin Wolf
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block,
qemu-devel, armbru, mreitz
On 28.06.2019 22:34, Eric Blake wrote:
> On 6/28/19 9:54 AM, Kevin Wolf wrote:
>
>>>>>>>> We would save most of this code if we added a new field to the header
>>>>>>>> instead of adding a header extension. Not saying that we should
>>>>>>>> definitely do this, but let's discuss it at least.
>>>>>>>
>>>>>>> If we add the new field to the header will the older qemu be able to use
>>>>>>> it. Or we will add the header only if needed, i.e. if compression_type
>>>>>>> != zlib
>>>>>>
>>>>>> Increasing the header size is backwards compatible. Older qemu versions
>>>>>> should handle such images correctly. They would store the unknown part
>>>>>> of the header in s->unknown_header_fields and keep it unmodified when
>>>>>> updating the image header.
>>>>>>
>>>>>> We would still add the incompatible feature flag for non-zlib, of
>>>>>> course.
>>>>> so, we basically need to do the same: store compression type and forbid
>>>>> to use because of flag if not zlib.
>>>>>
>>>>> Sounds like it doesn't differ that much from the extension header approach.
>>>>
>>>> It provides more or less the same functionality, but would probably make
>>>> this patch half the size because all of the code related to reading and
>>>> checking the header extension would go away. It also saves a few bytes
>>>> in the header cluster (4 bytes vs. 16 bytes).
>>> ok, will re-do it that way.
>>>
>>> Do you agree in general with how zlib compression type is treated?
>>
>> As I said, I think both ways are justifiable as long as we stay
>> consistent between qemu and spec.
>>
>> I'd prefer to allow zlib in the extension, you'd prefer to forbid it.
>> So I'd like to hear opinions from some more people on which way they
>> prefer.
>
> My preferences - use a 4 byte header field, and require the incompatible
> feature bit if the field is non-zero. The standard should allow someone
> to explicitly request zlib compression (done by leaving the incompatible
> bit clear, then specifying a header length of 108 instead of 104, but
> leaving the compression field at 104-107 at 0), to implicitly request
> zlib compression (done by leaving the incompatible bit clear, and
> specifying a header length of 104); or to explicitly request some other
> compression (done by setting the incompatible bit, specifying a header
> length of 108, and putting a non-zero value in the compression field
> 104-107).
>
> Under these rules, if you implicitly or explicitly request zlib, your
> image can be opened without problems by both older and newer qemu. If
> you explicitly request zstd, your image will fail to open by older qemu
> (good, because they would misinterpret compressed clusters), and work
> with newer qemu. And since providing a 108-byte header works just fine
> with older qemu as long as the header contains 0, I recommend that we
> just always make newer qemu provide that field (even if it is explicitly
> set to zlib), as that is less complicated than only providing the larger
> header for non-zlib files (we still have to parse 104-byte headers, but
> that doesn't mean we have to create brand-new files that way).
>
> There's one more corner case. I recommend that the standard require that
> it be an error to set the incompatible feature bit but use a header size
> of 104 - if you have an imabe like that, the image would be treated as
> using zlib (implicitly due to the header size), so older images _could_
> use it other than the fact that they don't recognize the incompatible
> feature bit. On the other hand, requiring such an image to be rejected
> is a bit of a stretch - no qemu (whether one that understands the
> feature bit or one that does not) would misinterpret the image contents
> as being zlib compressed, if it had not been for the bit being set. So
> in this corner case, I'm fine if you end up documenting whatever is
> easiest to code.
>
Ok, I'll re-do the series introducing compression type in the header.
Thanks!
Denis
--
Best,
Denis
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 3/3] qcow2: add zstd cluster compression
2019-06-28 11:57 ` Kevin Wolf
2019-07-02 12:33 ` Denis Plotnikov
@ 2019-07-02 12:49 ` Denis Plotnikov
2019-07-02 14:37 ` Kevin Wolf
1 sibling, 1 reply; 29+ messages in thread
From: Denis Plotnikov @ 2019-07-02 12:49 UTC (permalink / raw)
To: Kevin Wolf
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block,
qemu-devel, armbru, mreitz
On 28.06.2019 14:57, Kevin Wolf wrote:
> Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
>> zstd significantly reduces cluster compression time.
>> It provides better compression performance maintaining
>> the same level of compression ratio in comparison with
>> zlib, which, by the moment, has been the only compression
>> method available.
>>
>> The performance test results:
>> Test compresses and decompresses qemu qcow2 image with just
>> installed rhel-7.6 guest.
>> Image cluster size: 64K. Image on disk size: 2.2G
>>
>> The test was conducted with brd disk to reduce the influence
>> of disk subsystem to the test results.
>> The results is given in seconds.
>>
>> compress cmd:
>> time ./qemu-img convert -O qcow2 -c -o compression_type=[zlib|zstd]
>> src.img [zlib|zstd]_compressed.img
>> decompress cmd
>> time ./qemu-img convert -O qcow2
>> [zlib|zstd]_compressed.img uncompressed.img
>>
>> compression decompression
>> zlib zstd zlib zstd
>> ------------------------------------------------------------
>> real 65.5 16.3 (-75 %) 1.9 1.6 (-16 %)
>> user 65.0 15.8 5.3 2.5
>> sys 3.3 0.2 2.0 2.0
>>
>> Both ZLIB and ZSTD gave the same compression ratio: 1.57
>> compressed image size in both cases: 1.4G
>>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
>> ---
>> block/qcow2.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> configure | 26 ++++++++++++++++
>> 2 files changed, 108 insertions(+)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 90f15cc3c9..58901f9f79 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -26,6 +26,7 @@
>>
>> #define ZLIB_CONST
>> #include <zlib.h>
>> +#include <zstd.h>
>>
>> #include "block/block_int.h"
>> #include "block/qdict.h"
>> @@ -1553,6 +1554,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>> case QCOW2_COMPRESSION_TYPE_ZLIB:
>> break;
>>
>> + case QCOW2_COMPRESSION_TYPE_ZSTD:
>> + break;
>
> If we don't intend to add any code here, why not just add another case
> label to the existing break?
>
>> default:
>> error_setg(errp, "Unknown compression type");
>> ret = -EINVAL;
>> @@ -3286,6 +3290,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
>> * ZLIB shouldn't be here since it's the default
>> */
>> switch (qcow2_opts->compression_type) {
>> + case QCOW2_COMPRESSION_TYPE_ZSTD:
>> + break;
>> +
>> default:
>> error_setg_errno(errp, -EINVAL, "Unknown compression type");
>> goto out;
>> @@ -4113,6 +4120,73 @@ static ssize_t zlib_decompress(void *dest, size_t dest_size,
>> return ret;
>> }
>>
>> +/*
>> + * zstd_compress()
>> + *
>> + * @dest - destination buffer, @dest_size bytes
>> + * @src - source buffer, @src_size bytes
>> + *
>> + * Returns: compressed size on success
>> + * -1 on any error
>> + */
>> +
>> +static ssize_t zstd_compress(void *dest, size_t dest_size,
>> + const void *src, size_t src_size)
>> +{
>> + /* steal some bytes to store compressed chunk size */
>> + size_t ret;
>
> This ends up in a file, so this needs to be a fixed number of bytes.
> size_t varies between different platforms, so it is not acceptable.
>
> If I understand correctly, the maximum for this is the cluster size, so
> uint32_t should be right.
>
> This isn't plain the zstd compression format any more, so it needs to be
> described in the qcow2 spec.
>
>> + size_t *c_size = dest;
>> + char *d_buf = dest;
>> + d_buf += sizeof(ret);
>
> char *d_bug = dest + sizeof(ret);
>
>> + dest_size -= sizeof(ret);
>
> We don't want to end up with an integer overflow, so before this:
>
> if (dest_size < sizeof(ret)) {
> return -ENOMEM;
> }
>
>> + ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
>> +
>> + if (ZSTD_isError(ret)) {
>> + return -1;
>> + }
>
> Need an error code here, not just -1. In particular, we need to
> distinguish cases where the buffer was too small and uncompressed data
> should be written instead (ENOMEM) from real errors that should be
> returned to the caller (EIO).
>
>> +
>> + /* store the compressed chunk size in the very beginning of the buffer */
>> + *c_size = ret;
>> +
>> + return ret + sizeof(ret);
>> +}
>> +
>> +/*
>> + * zstd_decompress()
>> + *
>> + * Decompress some data (not more than @src_size bytes) to produce exactly
>> + * @dest_size bytes.
>> + *
>> + * @dest - destination buffer, @dest_size bytes
>> + * @src - source buffer, @src_size bytes
>> + *
>> + * Returns: 0 on success
>> + * -1 on fail
>> + */
>> +
>> +static ssize_t zstd_decompress(void *dest, size_t dest_size,
>> + const void *src, size_t src_size)
>
> Indentation is off.
>
>> +{
>> + size_t ret;
>> + /*
>> + * zstd decompress wants to know the exact lenght of the data
>> + * for that purpose, zstd_compress stores the length in the
>> + * very beginning of the compressed buffer
>> + */
>> + const size_t *s_size = src;
>> + const char *s_buf = src;
>> + s_buf += sizeof(size_t);
>
> Single line: const char *s_buf = src + sizeof(size_t);
>
> Of course, size_t is wrong here, too. And above you used sizeof() on a
> variable and here it's on the type. I think we should stay consistent.
>
> You're lacking a check against src_size. A malicious image could make
> use read beyond the end of the buffer. (Also consider that src_size
> could be smaller than sizeof(size_t).)
>
>> + ret = ZSTD_decompress(dest, dest_size, s_buf, *s_size);
>> +
>> + if (ZSTD_isError(ret)) {
>> + return -1;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> #define MAX_COMPRESS_THREADS 4
>>
>> typedef ssize_t (*Qcow2CompressFunc)(void *dest, size_t dest_size,
>> @@ -4189,6 +4263,10 @@ qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
>> fn = zlib_compress;
>> break;
>>
>> + case QCOW2_COMPRESSION_TYPE_ZSTD:
>> + fn = zstd_compress;
>> + break;
>> +
>> default:
>> return -ENOTSUP;
>> }
>> @@ -4208,6 +4286,10 @@ qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
>> fn = zlib_decompress;
>> break;
>>
>> + case QCOW2_COMPRESSION_TYPE_ZSTD:
>> + fn = zstd_decompress;
>> + break;
>> +
>> default:
>> return -ENOTSUP;
>> }
>> diff --git a/configure b/configure
>> index 1c563a7027..c90716189c 100755
>> --- a/configure
>> +++ b/configure
>> @@ -433,6 +433,7 @@ opengl_dmabuf="no"
>> cpuid_h="no"
>> avx2_opt=""
>> zlib="yes"
>> +zstd="yes"
>
> This should be zstd="" so that a missing library will automatically
> disable it instead of producing an error. (Building QEMU without zlib is
> impossible, but building it without ZSTD should work.)
But if we add zstd for clusters compression we have to build with zstd
each time. If we want to chose whether we want to build zstd we need to
enclose all zstd related code with "ifdef"-s.
I don't think it's good because we can end up with mess of version
supporting and not supporting zstd compression.
Another point is what the benefit of building qemu without zstd support
is since it's available and provides better performance than zlib (i.e.
the replacement for zlib) ?
Denis
>
>> capstone=""
>> lzo=""
>> snappy=""
>> @@ -1317,6 +1318,8 @@ for opt do
>> ;;
>> --disable-zlib-test) zlib="no"
>> ;;
>> + --disable-zstd-test) zstd="no"
>> + ;;
>
> Instead of this one, after making the above change, options
> --disable-zstd and --enable-zstd should be introduced that set
> zstd="yes" (that actually does produce an error if it's not available)
> or zstd="no".
>
>> --disable-lzo) lzo="no"
>> ;;
>> --enable-lzo) lzo="yes"
>> @@ -3702,6 +3705,29 @@ EOF
>> fi
>> fi
>>
>> +#########################################
>> +# zstd check
>> +
>> +if test "$zstd" != "no" ; then
>> + if $pkg_config --exists libzstd; then
>> + zstd_cflags=$($pkg_config --cflags libzstd)
>> + zstd_libs=$($pkg_config --libs libzstd)
>> + QEMU_CFLAGS="$zstd_cflags $QEMU_CFLAGS"
>> + LIBS="$zstd_libs $LIBS"
>> + else
>> + cat > $TMPC << EOF
>> +#include <zstd.h>
>> +int main(void) { ZSTD_versionNumber(); return 0; }
>> +EOF
>> + if compile_prog "" "-lzstd" ; then
>> + LIBS="$LIBS -lzstd"
>> + else
>> + error_exit "zstd check failed" \
>> + "Make sure to have the zstd libs and headers installed."
>> + fi
>
> This needs to be changed, too, to get the desired behaviour. Model it
> after bzip2 or lzo support checks:
>
> if compile_prog "" "-lbz2" ; then
> bzip2="yes"
> else
> if test "$bzip2" = "yes"; then
> feature_not_found "libbzip2" "Install libbzip2 devel"
> fi
> bzip2="no"
> fi
>
>> + fi
>> +fi
>> +
>> ##########################################
>> # SHA command probe for modules
>> if test "$modules" = yes; then
>
> Kevin
>
--
Best,
Denis
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 3/3] qcow2: add zstd cluster compression
2019-07-02 12:49 ` Denis Plotnikov
@ 2019-07-02 14:37 ` Kevin Wolf
2019-07-02 14:48 ` Denis Plotnikov
0 siblings, 1 reply; 29+ messages in thread
From: Kevin Wolf @ 2019-07-02 14:37 UTC (permalink / raw)
To: Denis Plotnikov
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block,
qemu-devel, armbru, mreitz
Am 02.07.2019 um 14:49 hat Denis Plotnikov geschrieben:
> On 28.06.2019 14:57, Kevin Wolf wrote:
> > Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
> >> diff --git a/configure b/configure
> >> index 1c563a7027..c90716189c 100755
> >> --- a/configure
> >> +++ b/configure
> >> @@ -433,6 +433,7 @@ opengl_dmabuf="no"
> >> cpuid_h="no"
> >> avx2_opt=""
> >> zlib="yes"
> >> +zstd="yes"
> >
> > This should be zstd="" so that a missing library will automatically
> > disable it instead of producing an error. (Building QEMU without zlib is
> > impossible, but building it without ZSTD should work.)
> But if we add zstd for clusters compression we have to build with zstd
> each time. If we want to chose whether we want to build zstd we need to
> enclose all zstd related code with "ifdef"-s.
> I don't think it's good because we can end up with mess of version
> supporting and not supporting zstd compression.
Yes, we'll need ifdefs. Or we could do it like the dmg compression
formats, a spearate source file for zstd that is compiled conditionally.
Anyway, I don't think making zstd a hard dependency for qemu is
acceptable.
> Another point is what the benefit of building qemu without zstd support
> is since it's available and provides better performance than zlib (i.e.
> the replacement for zlib) ?
Is it available in all relevant distros and even non-Linux platforms
that we support?
Another reason for wanting to compile it out even when it is available
is startup time. Each shared library to be loaded takes some time, and
there are use cases where you want a minimal guest to boot up really
fast.
Kevin
^ permalink raw reply [flat|nested] 29+ messages in thread
* Re: [Qemu-devel] [PATCH v0 3/3] qcow2: add zstd cluster compression
2019-07-02 14:37 ` Kevin Wolf
@ 2019-07-02 14:48 ` Denis Plotnikov
0 siblings, 0 replies; 29+ messages in thread
From: Denis Plotnikov @ 2019-07-02 14:48 UTC (permalink / raw)
To: Kevin Wolf
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block,
qemu-devel, armbru, mreitz
On 02.07.2019 17:37, Kevin Wolf wrote:
> Am 02.07.2019 um 14:49 hat Denis Plotnikov geschrieben:
>> On 28.06.2019 14:57, Kevin Wolf wrote:
>>> Am 28.05.2019 um 16:37 hat Denis Plotnikov geschrieben:
>>>> diff --git a/configure b/configure
>>>> index 1c563a7027..c90716189c 100755
>>>> --- a/configure
>>>> +++ b/configure
>>>> @@ -433,6 +433,7 @@ opengl_dmabuf="no"
>>>> cpuid_h="no"
>>>> avx2_opt=""
>>>> zlib="yes"
>>>> +zstd="yes"
>>>
>>> This should be zstd="" so that a missing library will automatically
>>> disable it instead of producing an error. (Building QEMU without zlib is
>>> impossible, but building it without ZSTD should work.)
>> But if we add zstd for clusters compression we have to build with zstd
>> each time. If we want to chose whether we want to build zstd we need to
>> enclose all zstd related code with "ifdef"-s.
>> I don't think it's good because we can end up with mess of version
>> supporting and not supporting zstd compression.
>
> Yes, we'll need ifdefs. Or we could do it like the dmg compression
> formats, a spearate source file for zstd that is compiled conditionally.
>
> Anyway, I don't think making zstd a hard dependency for qemu is
> acceptable.
>
>> Another point is what the benefit of building qemu without zstd support
>> is since it's available and provides better performance than zlib (i.e.
>> the replacement for zlib) ?
>
> Is it available in all relevant distros and even non-Linux platforms
> that we support?
>
> Another reason for wanting to compile it out even when it is available
> is startup time. Each shared library to be loaded takes some time, and
> there are use cases where you want a minimal guest to boot up really
> fast.
>
> Kevin
Sounds like important points. Thanks for the explanation!
>
--
Best,
Denis
^ permalink raw reply [flat|nested] 29+ messages in thread
end of thread, other threads:[~2019-07-02 14:53 UTC | newest]
Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-28 14:37 [Qemu-devel] [PATCH v0 0/3] add zstd cluster compression Denis Plotnikov
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature Denis Plotnikov
2019-05-29 11:40 ` Vladimir Sementsov-Ogievskiy
2019-06-28 9:45 ` Kevin Wolf
2019-06-27 16:35 ` Markus Armbruster
2019-06-28 9:54 ` Kevin Wolf
2019-06-28 11:07 ` Denis Plotnikov
2019-06-28 10:10 ` Kevin Wolf
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 2/3] qcow2: add compression type processing Denis Plotnikov
2019-05-29 9:47 ` Vladimir Sementsov-Ogievskiy
2019-06-28 10:23 ` Kevin Wolf
2019-06-28 11:24 ` Denis Plotnikov
2019-06-28 12:06 ` Kevin Wolf
2019-06-28 12:56 ` Denis Plotnikov
2019-06-28 14:24 ` Kevin Wolf
2019-06-28 14:40 ` Denis Plotnikov
2019-06-28 14:54 ` Kevin Wolf
2019-06-28 15:03 ` Max Reitz
2019-06-28 15:14 ` Denis Plotnikov
2019-06-28 19:34 ` Eric Blake
2019-07-02 12:34 ` Denis Plotnikov
2019-05-28 14:37 ` [Qemu-devel] [PATCH v0 3/3] qcow2: add zstd cluster compression Denis Plotnikov
2019-06-28 11:57 ` Kevin Wolf
2019-07-02 12:33 ` Denis Plotnikov
2019-07-02 12:49 ` Denis Plotnikov
2019-07-02 14:37 ` Kevin Wolf
2019-07-02 14:48 ` Denis Plotnikov
2019-06-04 7:56 ` [Qemu-devel] [PING] [PATCH v0 0/3] " Denis Plotnikov
2019-06-27 15:04 ` [Qemu-devel] [PING PING] " Denis Plotnikov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.