* [Qemu-devel] [PATCH v1 0/3] add zstd cluster compression
@ 2019-07-03 11:00 Denis Plotnikov
2019-07-03 11:00 ` [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature Denis Plotnikov
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Denis Plotnikov @ 2019-07-03 11:00 UTC (permalink / raw)
To: kwolf, mreitz, eblake, armbru; +Cc: vsementsov, den, qemu-block, qemu-devel
change log:
v1:
* extend qcow2 header instead of adding a new incompatible extension header
specification re-written accordingly
* enable zstd compression via config
* fix zstd (de)compression functions
* fix comments/description
* fix function naming
---
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: rework the cluster compression routine
qcow2: add zstd cluster compression
block/qcow2.c | 283 +++++++++++++++++++++++++++++++++++---
block/qcow2.h | 26 +++-
configure | 32 +++++
docs/interop/qcow2.txt | 22 ++-
include/block/block_int.h | 1 +
qapi/block-core.json | 23 +++-
6 files changed, 357 insertions(+), 30 deletions(-)
--
2.17.0
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature
2019-07-03 11:00 [Qemu-devel] [PATCH v1 0/3] add zstd cluster compression Denis Plotnikov
@ 2019-07-03 11:00 ` Denis Plotnikov
2019-07-03 14:14 ` Eric Blake
2019-07-09 11:27 ` Markus Armbruster
2019-07-03 11:00 ` [Qemu-devel] [PATCH v1 2/3] qcow2: rework the cluster compression routine Denis Plotnikov
2019-07-03 11:00 ` [Qemu-devel] [PATCH v1 3/3] qcow2: add zstd cluster compression Denis Plotnikov
2 siblings, 2 replies; 14+ messages in thread
From: Denis Plotnikov @ 2019-07-03 11:00 UTC (permalink / raw)
To: kwolf, mreitz, eblake, armbru; +Cc: vsementsov, den, qemu-block, qemu-devel
The patch adds some preparation parts for incompatible compression type
feature to QCOW2 header that indicates that *all* compressed clusters
must be (de)compressed using a certain compression type.
It is implied that the compression type is set on the image creation and
can be changed only later by image conversion, thus compression type
defines the only compression algorithm used for the image.
The goal of the feature is to add support of other compression algorithms
to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
It works roughly x2 faster than ZLIB providing a comparable compression ratio
and therefore provide a performance advantage in backup scenarios.
The default compression is ZLIB. Images created with ZLIB compression type
is backward compatible with older qemu versions.
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
block/qcow2.c | 94 +++++++++++++++++++++++++++++++++++++++
block/qcow2.h | 26 ++++++++---
docs/interop/qcow2.txt | 22 ++++++++-
include/block/block_int.h | 1 +
qapi/block-core.json | 22 ++++++++-
5 files changed, 155 insertions(+), 10 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 3ace3b2209..921eb67b80 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1202,6 +1202,47 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
return ret;
}
+static int check_compression_type(BDRVQcow2State *s, Error **errp)
+{
+ bool is_set;
+ int ret = 0;
+
+ switch (s->compression_type) {
+ case QCOW2_COMPRESSION_TYPE_ZLIB:
+ break;
+
+ default:
+ if (errp) {
+ error_setg(errp, "qcow2: unknown compression type: %u",
+ s->compression_type);
+ return -ENOTSUP;
+ }
+ }
+
+ /*
+ * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
+ * feature flag must be absent, with other compression types the
+ * incompatible feature flag must be set
+ */
+ is_set = s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE;
+
+ if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
+ if (is_set) {
+ ret = -EINVAL;
+ }
+ } else {
+ if (!is_set) {
+ ret = -EINVAL;
+ }
+ }
+
+ if (ret && errp) {
+ error_setg(errp, "qcow2: Illegal compression type setting");
+ }
+
+ return ret;
+}
+
/* Called with s->lock held. */
static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
int flags, Error **errp)
@@ -1318,6 +1359,24 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
s->compatible_features = header.compatible_features;
s->autoclear_features = header.autoclear_features;
+ /* Handle compression type */
+ if (header.header_length > 104) {
+ header.compression_type = be32_to_cpu(header.compression_type);
+ s->compression_type = header.compression_type;
+ } else {
+ /*
+ * older qcow2 images don't contain the compression type header
+ * field, distinguish them by the header length and use
+ * the only valid compression type in that case
+ */
+ s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
+ }
+
+ ret = check_compression_type(s, errp);
+ if (ret) {
+ goto fail;
+ }
+
if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
void *feature_table = NULL;
qcow2_read_extensions(bs, header.header_length, ext_end,
@@ -2434,6 +2493,13 @@ int qcow2_update_header(BlockDriverState *bs)
total_size = bs->total_sectors * BDRV_SECTOR_SIZE;
refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 3);
+ ret = check_compression_type(s, NULL);
+
+ if (ret) {
+ goto fail;
+ }
+
+
*header = (QCowHeader) {
/* Version 2 fields */
.magic = cpu_to_be32(QCOW_MAGIC),
@@ -2456,6 +2522,7 @@ int qcow2_update_header(BlockDriverState *bs)
.autoclear_features = cpu_to_be64(s->autoclear_features),
.refcount_order = cpu_to_be32(s->refcount_order),
.header_length = cpu_to_be32(header_length),
+ .compression_type = cpu_to_be32(s->compression_type),
};
/* For older versions, write a shorter header */
@@ -2553,6 +2620,11 @@ int qcow2_update_header(BlockDriverState *bs)
.bit = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
.name = "lazy refcounts",
},
+ {
+ .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
+ .bit = QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR,
+ .name = "compression type",
+ },
};
ret = header_ext_add(buf, QCOW2_EXT_MAGIC_FEATURE_TABLE,
@@ -3107,6 +3179,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
.refcount_table_offset = cpu_to_be64(cluster_size),
.refcount_table_clusters = cpu_to_be32(1),
.refcount_order = cpu_to_be32(refcount_order),
+ .compression_type = cpu_to_be32(QCOW2_COMPRESSION_TYPE_ZLIB),
.header_length = cpu_to_be32(sizeof(*header)),
};
@@ -3126,6 +3199,20 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
cpu_to_be64(QCOW2_AUTOCLEAR_DATA_FILE_RAW);
}
+ if (qcow2_opts->has_compression_type &&
+ qcow2_opts->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB) {
+
+ if (qcow2_opts->compression_type >= QCOW2_COMPRESSION_TYPE__MAX) {
+ error_setg_errno(errp, -EINVAL, "Unknown compression type");
+ goto out;
+ }
+
+ header->compression_type = cpu_to_be32(qcow2_opts->compression_type);
+
+ header->incompatible_features |=
+ cpu_to_be64(QCOW2_INCOMPAT_COMPRESSION_TYPE);
+ }
+
ret = blk_pwrite(blk, 0, header, cluster_size, 0);
g_free(header);
if (ret < 0) {
@@ -3307,6 +3394,7 @@ static int coroutine_fn qcow2_co_create_opts(const char *filename, QemuOpts *opt
{ BLOCK_OPT_ENCRYPT, BLOCK_OPT_ENCRYPT_FORMAT },
{ BLOCK_OPT_COMPAT_LEVEL, "version" },
{ BLOCK_OPT_DATA_FILE_RAW, "data-file-raw" },
+ { BLOCK_OPT_COMPRESSION_TYPE, "compression-type" },
{ NULL, NULL },
};
@@ -5239,6 +5327,12 @@ static QemuOptsList qcow2_create_opts = {
.help = "Width of a reference count entry in bits",
.def_value_str = "16"
},
+ {
+ .name = BLOCK_OPT_COMPRESSION_TYPE,
+ .type = QEMU_OPT_STRING,
+ .help = "Compression method used for image clusters compression",
+ .def_value_str = "0"
+ },
{ /* end of list */ }
}
};
diff --git a/block/qcow2.h b/block/qcow2.h
index fdee297f33..b44344c78e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -135,6 +135,7 @@ typedef struct QCowHeader {
uint32_t refcount_order;
uint32_t header_length;
+ uint32_t compression_type;
} QEMU_PACKED QCowHeader;
typedef struct QEMU_PACKED QCowSnapshotHeader {
@@ -198,16 +199,20 @@ enum {
/* Incompatible feature bits */
enum {
- QCOW2_INCOMPAT_DIRTY_BITNR = 0,
- QCOW2_INCOMPAT_CORRUPT_BITNR = 1,
- QCOW2_INCOMPAT_DATA_FILE_BITNR = 2,
- QCOW2_INCOMPAT_DIRTY = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
- QCOW2_INCOMPAT_CORRUPT = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
- QCOW2_INCOMPAT_DATA_FILE = 1 << QCOW2_INCOMPAT_DATA_FILE_BITNR,
+ QCOW2_INCOMPAT_DIRTY_BITNR = 0,
+ QCOW2_INCOMPAT_CORRUPT_BITNR = 1,
+ QCOW2_INCOMPAT_DATA_FILE_BITNR = 2,
+ QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR = 3,
+ QCOW2_INCOMPAT_DIRTY = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
+ QCOW2_INCOMPAT_CORRUPT = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
+ QCOW2_INCOMPAT_DATA_FILE = 1 << QCOW2_INCOMPAT_DATA_FILE_BITNR,
+ QCOW2_INCOMPAT_COMPRESSION_TYPE =
+ 1 << QCOW2_INCOMPAT_COMPRESSION_TYPE_BITNR,
QCOW2_INCOMPAT_MASK = QCOW2_INCOMPAT_DIRTY
| QCOW2_INCOMPAT_CORRUPT
- | QCOW2_INCOMPAT_DATA_FILE,
+ | QCOW2_INCOMPAT_DATA_FILE
+ | QCOW2_INCOMPAT_COMPRESSION_TYPE,
};
/* Compatible feature bits */
@@ -350,6 +355,13 @@ typedef struct BDRVQcow2State {
int nb_compress_threads;
BdrvChild *data_file;
+ /*
+ * Compression type used for the image. Default: 0 - ZLIB
+ * The image compression type is set on image creation.
+ * The only way to change the compression type is to convert the image
+ * with the desired compression type set
+ */
+ uint32_t compression_type;
} BDRVQcow2State;
typedef struct Qcow2COWRegion {
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index af5711e533..5b8a8d15fe 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -109,7 +109,12 @@ in the description of a field.
An External Data File Name header extension may
be present if this bit is set.
- Bits 3-63: Reserved (set to 0)
+ Bit 3: Compression type bit. The bit must be set if
+ the compression type differs from default: zlib.
+ If the compression type is default the bit must
+ be unset.
+
+ Bits 4-63: Reserved (set to 0)
80 - 87: compatible_features
Bitmask of compatible features. An implementation can
@@ -165,6 +170,21 @@ in the description of a field.
Length of the header structure in bytes. For version 2
images, the length is always assumed to be 72 bytes.
+ 104 - 107: compression_type
+ Defines the compression method used for compressed clusters.
+ A single compression type is applied to all compressed image
+ clusters.
+ The compression type is set on image creation only.
+ The default compression type is zlib.
+ When the deafult compression type is used the compression
+ type bit (incompatible feature bit 3) must be unset.
+ When any other compression type is used the compression
+ type bit must be set.
+ Qemu versions older than 4.1 can use images created with
+ the default compression type without any additional
+ preparations and cannot use images created with any other
+ compression type.
+
Directly after the image header, optional sections called header extensions can
be stored. Each extension has a structure like the following:
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 01e855a066..814917baec 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -58,6 +58,7 @@
#define BLOCK_OPT_REFCOUNT_BITS "refcount_bits"
#define BLOCK_OPT_DATA_FILE "data_file"
#define BLOCK_OPT_DATA_FILE_RAW "data_file_raw"
+#define BLOCK_OPT_COMPRESSION_TYPE "compression_type"
#define BLOCK_PROBE_BUF_SIZE 512
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 7ccbfff9d0..6aa8b99993 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -78,6 +78,8 @@
#
# @bitmaps: A list of qcow2 bitmap details (since 4.0)
#
+# @compression-type: the image cluster compression method (since 4.1)
+#
# Since: 1.7
##
{ 'struct': 'ImageInfoSpecificQCow2',
@@ -89,7 +91,8 @@
'*corrupt': 'bool',
'refcount-bits': 'int',
'*encrypt': 'ImageInfoSpecificQCow2Encryption',
- '*bitmaps': ['Qcow2BitmapInfo']
+ '*bitmaps': ['Qcow2BitmapInfo'],
+ '*compression-type': 'Qcow2CompressionType'
} }
##
@@ -4206,6 +4209,18 @@
'data': [ 'v2', 'v3' ] }
+##
+# @Qcow2CompressionType:
+#
+# Compression type used in qcow2 image file
+#
+# @zlib: zlib compression, see <http://zlib.net/>
+#
+# Since: 4.1
+##
+{ 'enum': 'Qcow2CompressionType',
+ 'data': [ 'zlib' ] }
+
##
# @BlockdevCreateOptionsQcow2:
#
@@ -4228,6 +4243,8 @@
# @preallocation Preallocation mode for the new image (default: off)
# @lazy-refcounts True if refcounts may be updated lazily (default: off)
# @refcount-bits Width of reference counts in bits (default: 16)
+# @compression-type The image cluster compression method
+# (default: zlib, since 4.1)
#
# Since: 2.12
##
@@ -4243,7 +4260,8 @@
'*cluster-size': 'size',
'*preallocation': 'PreallocMode',
'*lazy-refcounts': 'bool',
- '*refcount-bits': 'int' } }
+ '*refcount-bits': 'int',
+ '*compression-type': 'Qcow2CompressionType' } }
##
# @BlockdevCreateOptionsQed:
--
2.17.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v1 2/3] qcow2: rework the cluster compression routine
2019-07-03 11:00 [Qemu-devel] [PATCH v1 0/3] add zstd cluster compression Denis Plotnikov
2019-07-03 11:00 ` [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature Denis Plotnikov
@ 2019-07-03 11:00 ` Denis Plotnikov
2019-07-03 11:00 ` [Qemu-devel] [PATCH v1 3/3] qcow2: add zstd cluster compression Denis Plotnikov
2 siblings, 0 replies; 14+ messages in thread
From: Denis Plotnikov @ 2019-07-03 11:00 UTC (permalink / raw)
To: kwolf, mreitz, eblake, armbru; +Cc: vsementsov, den, qemu-block, qemu-devel
The patch allow to process image compression type defined
in the image header and choose an appropriate method for
image clusters (de)compression.
Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
block/qcow2.c | 93 ++++++++++++++++++++++++++++++++++++++++-----------
1 file changed, 73 insertions(+), 20 deletions(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 921eb67b80..37a563a671 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -4005,8 +4005,11 @@ fail:
}
/*
- * qcow2_compress()
+ * qcow2_zlib_compress()
*
+ * Compress @src_size bytes of data using zlib compression method
+ *
+ * @dest_size bytes.
* @dest - destination buffer, @dest_size bytes
* @src - source buffer, @src_size bytes
*
@@ -4014,8 +4017,8 @@ 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,
- const void *src, size_t src_size)
+static ssize_t qcow2_zlib_compress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
{
ssize_t ret;
z_stream strm;
@@ -4025,7 +4028,7 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION, Z_DEFLATED,
-12, 9, Z_DEFAULT_STRATEGY);
if (ret != Z_OK) {
- return -2;
+ return -EIO;
}
/* strm.next_in is not const in old zlib versions, such as those used on
@@ -4039,7 +4042,7 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
if (ret == Z_STREAM_END) {
ret = dest_size - strm.avail_out;
} else {
- ret = (ret == Z_OK ? -1 : -2);
+ ret = (ret == Z_OK ? -ENOMEM : -EIO);
}
deflateEnd(&strm);
@@ -4048,10 +4051,10 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
}
/*
- * qcow2_decompress()
+ * qcow2_zlib_decompress()
*
* Decompress some data (not more than @src_size bytes) to produce exactly
- * @dest_size bytes.
+ * @dest_size bytes using zlib compression method
*
* @dest - destination buffer, @dest_size bytes
* @src - source buffer, @src_size bytes
@@ -4059,8 +4062,8 @@ 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,
- const void *src, size_t src_size)
+static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
{
int ret = 0;
z_stream strm;
@@ -4073,7 +4076,7 @@ static ssize_t qcow2_decompress(void *dest, size_t dest_size,
ret = inflateInit2(&strm, -12);
if (ret != Z_OK) {
- return -1;
+ return -EIO;
}
ret = inflate(&strm, Z_FINISH);
@@ -4081,7 +4084,7 @@ static ssize_t qcow2_decompress(void *dest, size_t dest_size,
/* We approve Z_BUF_ERROR because we need @dest buffer to be filled, but
* @src buffer may be processed partly (because in qcow2 we know size of
* compressed data with precision of one sector) */
- ret = -1;
+ ret = -EIO;
}
inflateEnd(&strm);
@@ -4153,20 +4156,67 @@ qcow2_co_do_compress(BlockDriverState *bs, void *dest, size_t dest_size,
return arg.ret;
}
+/*
+ * qcow2_co_compress()
+ *
+ * Compress @src_size bytes of data using the compression
+ * method defined by the image compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ * a negative error code on fail
+ */
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 = qcow2_zlib_compress;
+ break;
+
+ default:
+ return -ENOTSUP;
+ }
+
+ return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
}
+/*
+ * qcow2_co_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes using the compression method defined by the image
+ * compression type
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ * a negative error code on fail
+ */
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 = qcow2_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
@@ -4178,7 +4228,7 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
BDRVQcow2State *s = bs->opaque;
QEMUIOVector hd_qiov;
int ret;
- size_t out_len;
+ ssize_t out_len;
uint8_t *buf, *out_buf;
uint64_t cluster_offset;
@@ -4217,16 +4267,19 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
out_len = qcow2_co_compress(bs, out_buf, s->cluster_size - 1,
buf, s->cluster_size);
- if (out_len == -2) {
- ret = -EINVAL;
- goto fail;
- } else if (out_len == -1) {
+ if (out_len == -ENOMEM) {
/* could not compress: write normal cluster */
ret = qcow2_co_pwritev(bs, offset, bytes, qiov, 0);
if (ret < 0) {
goto fail;
}
goto success;
+ } else if (out_len < 0) {
+ /*
+ * encounter other compression issues propagate error to the upper level
+ */
+ ret = out_len;
+ goto fail;
}
qemu_co_mutex_lock(&s->lock);
--
2.17.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [Qemu-devel] [PATCH v1 3/3] qcow2: add zstd cluster compression
2019-07-03 11:00 [Qemu-devel] [PATCH v1 0/3] add zstd cluster compression Denis Plotnikov
2019-07-03 11:00 ` [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature Denis Plotnikov
2019-07-03 11:00 ` [Qemu-devel] [PATCH v1 2/3] qcow2: rework the cluster compression routine Denis Plotnikov
@ 2019-07-03 11:00 ` Denis Plotnikov
2019-07-03 14:36 ` Eric Blake
2 siblings, 1 reply; 14+ messages in thread
From: Denis Plotnikov @ 2019-07-03 11:00 UTC (permalink / raw)
To: kwolf, mreitz, eblake, armbru; +Cc: vsementsov, den, qemu-block, qemu-devel
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 | 96 ++++++++++++++++++++++++++++++++++++++++++++
configure | 32 +++++++++++++++
qapi/block-core.json | 3 +-
3 files changed, 130 insertions(+), 1 deletion(-)
diff --git a/block/qcow2.c b/block/qcow2.c
index 37a563a671..caa04b0beb 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -27,6 +27,11 @@
#define ZLIB_CONST
#include <zlib.h>
+#ifdef CONFIG_ZSTD
+#include <zstd.h>
+#include <zstd_errors.h>
+#endif
+
#include "block/block_int.h"
#include "block/qdict.h"
#include "sysemu/block-backend.h"
@@ -1209,6 +1214,9 @@ static int check_compression_type(BDRVQcow2State *s, Error **errp)
switch (s->compression_type) {
case QCOW2_COMPRESSION_TYPE_ZLIB:
+#ifdef CONFIG_ZSTD
+ case QCOW2_COMPRESSION_TYPE_ZSTD:
+#endif
break;
default:
@@ -4092,6 +4100,84 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
return ret;
}
+#ifdef CONFIG_ZSTD
+/*
+ * qcow2_zstd_compress()
+ *
+ * Compress @src_size bytes of data using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: compressed size on success
+ * a negative error code on fail
+ */
+
+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
+{
+ ssize_t ret;
+ uint32_t *c_size = dest;
+ /* steal some bytes to store compressed chunk size */
+ char *d_buf = ((char *) dest) + sizeof(*c_size);
+
+ if (dest_size < sizeof(*c_size)) {
+ return -ENOMEM;
+ }
+
+ dest_size -= sizeof(*c_size);
+
+ ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
+
+ if (ZSTD_isError(ret)) {
+ if (ret == ZSTD_error_dstSize_tooSmall) {
+ return -ENOMEM;
+ } else {
+ return -EIO;
+ }
+ }
+
+ /* store the compressed chunk size in the very beginning of the buffer */
+ *c_size = ret;
+
+ return ret + sizeof(ret);
+}
+
+/*
+ * qcow2_zstd_decompress()
+ *
+ * Decompress some data (not more than @src_size bytes) to produce exactly
+ * @dest_size bytes using zstd compression method
+ *
+ * @dest - destination buffer, @dest_size bytes
+ * @src - source buffer, @src_size bytes
+ *
+ * Returns: 0 on success
+ * -EIO on fail
+ */
+
+static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
+{
+ ssize_t ret;
+ /*
+ * zstd decompress wants to know the exact lenght of the data
+ * for that purpose, on the compression the length is stored in
+ * the very beginning of the compressed buffer
+ */
+ const uint32_t *s_size = src;
+ const char *s_buf = ((char *) src) + sizeof(*s_size);
+
+ ret = ZSTD_decompress(dest, dest_size, s_buf, *s_size);
+
+ if (ZSTD_isError(ret)) {
+ return -EIO;
+ }
+
+ return 0;
+}
+#endif
+
#define MAX_COMPRESS_THREADS 4
typedef ssize_t (*Qcow2CompressFunc)(void *dest, size_t dest_size,
@@ -4180,6 +4266,11 @@ qcow2_co_compress(BlockDriverState *bs, void *dest, size_t dest_size,
fn = qcow2_zlib_compress;
break;
+#ifdef CONFIG_ZSTD
+ case QCOW2_COMPRESSION_TYPE_ZSTD:
+ fn = qcow2_zstd_compress;
+ break;
+#endif
default:
return -ENOTSUP;
}
@@ -4212,6 +4303,11 @@ qcow2_co_decompress(BlockDriverState *bs, void *dest, size_t dest_size,
fn = qcow2_zlib_decompress;
break;
+#ifdef CONFIG_ZSTD
+ case QCOW2_COMPRESSION_TYPE_ZSTD:
+ fn = qcow2_zstd_decompress;
+ break;
+#endif
default:
return -ENOTSUP;
}
diff --git a/configure b/configure
index 1c563a7027..57a80e38e7 100755
--- a/configure
+++ b/configure
@@ -433,6 +433,7 @@ opengl_dmabuf="no"
cpuid_h="no"
avx2_opt=""
zlib="yes"
+zstd=""
capstone=""
lzo=""
snappy=""
@@ -1333,6 +1334,10 @@ for opt do
;;
--disable-lzfse) lzfse="no"
;;
+ --enable-zstd) zstd="yes"
+ ;;
+ --disable-zstd) zstd="no"
+ ;;
--enable-guest-agent) guest_agent="yes"
;;
--disable-guest-agent) guest_agent="no"
@@ -1788,6 +1793,7 @@ disabled with --disable-FEATURE, default is enabled if available:
(for reading bzip2-compressed dmg images)
lzfse support of lzfse compression library
(for reading lzfse-compressed dmg images)
+ zstd support of zstd compression library
seccomp seccomp support
coroutine-pool coroutine freelist (better performance)
glusterfs GlusterFS backend
@@ -2374,6 +2380,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
+
##########################################
# libseccomp check
@@ -7253,6 +7282,9 @@ fi
if test "$sheepdog" = "yes" ; then
echo "CONFIG_SHEEPDOG=y" >> $config_host_mak
fi
+if test "$zstd" = "yes" ; then
+ echo "CONFIG_ZSTD=y" >> $config_host_mak
+fi
if test "$tcg_interpreter" = "yes"; then
QEMU_INCLUDES="-iquote \$(SRC_PATH)/tcg/tci $QEMU_INCLUDES"
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 6aa8b99993..2604f201ee 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4215,11 +4215,12 @@
# Compression type used in qcow2 image file
#
# @zlib: zlib compression, see <http://zlib.net/>
+# @zstd: zstd compression, see <http://github.com/facebook/zstd>
#
# Since: 4.1
##
{ 'enum': 'Qcow2CompressionType',
- 'data': [ 'zlib' ] }
+ 'data': [ 'zlib', 'zstd' ] }
##
# @BlockdevCreateOptionsQcow2:
--
2.17.0
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature
2019-07-03 11:00 ` [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature Denis Plotnikov
@ 2019-07-03 14:14 ` Eric Blake
2019-07-03 15:01 ` Denis Plotnikov
2019-07-09 11:27 ` Markus Armbruster
1 sibling, 1 reply; 14+ messages in thread
From: Eric Blake @ 2019-07-03 14:14 UTC (permalink / raw)
To: Denis Plotnikov, kwolf, mreitz, armbru
Cc: vsementsov, den, qemu-block, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 10920 bytes --]
On 7/3/19 6:00 AM, Denis Plotnikov wrote:
> The patch adds some preparation parts for incompatible compression type
> feature to QCOW2 header that indicates that *all* compressed clusters
> must be (de)compressed using a certain compression type.
>
> It is implied that the compression type is set on the image creation and
> can be changed only later by image conversion, thus compression type
> defines the only compression algorithm used for the image.
>
> The goal of the feature is to add support of other compression algorithms
> to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
> It works roughly x2 faster than ZLIB providing a comparable compression ratio
2x
> and therefore provide a performance advantage in backup scenarios.
>
> The default compression is ZLIB. Images created with ZLIB compression type
> is backward compatible with older qemu versions.
s/is/are/
>
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
> block/qcow2.c | 94 +++++++++++++++++++++++++++++++++++++++
> block/qcow2.h | 26 ++++++++---
> docs/interop/qcow2.txt | 22 ++++++++-
> include/block/block_int.h | 1 +
> qapi/block-core.json | 22 ++++++++-
> 5 files changed, 155 insertions(+), 10 deletions(-)
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 3ace3b2209..921eb67b80 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1202,6 +1202,47 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
> return ret;
> }
>
> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
> +{
> + bool is_set;
> + int ret = 0;
> +
> + switch (s->compression_type) {
> + case QCOW2_COMPRESSION_TYPE_ZLIB:
> + break;
> +
> + default:
> + if (errp) {
Useless check for errp being non-NULL. What's worse, if the caller
passes in NULL because they don't care about the error, then your code
behaves differently. Just call error_setg() and return -ENOTSUP
unconditionally.
> + error_setg(errp, "qcow2: unknown compression type: %u",
> + s->compression_type);
> + return -ENOTSUP;
> + }
> + }
> +
> + /*
> + * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
> + * feature flag must be absent, with other compression types the
> + * incompatible feature flag must be set
Is there a strong reason for forbid the incompatible feature flag with
ZLIB? Sure, it makes the image impossible to open with older qemu, but
it doesn't get in the way of newer qemu opening it. I'll have to see how
your spec changes documented this, to see if you could instead word it
as 'for ZLIB, the incompatible feature flag may be absent'.
> + */
> + is_set = s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE;
> +
> + if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
> + if (is_set) {
> + ret = -EINVAL;
> + }
> + } else {
> + if (!is_set) {
> + ret = -EINVAL;
> + }
> + }
More compact as:
if ((s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) == is_set)
> +
> + if (ret && errp) {
> + error_setg(errp, "qcow2: Illegal compression type setting");
s/Illegal/Invalid/ (the user isn't breaking a law)
Also, don't check whether errp is non-NULL. Just blindly call
error_setg() if ret is non-zero.
> + }
> +
> + return ret;
Or even shorter (psuedocode):
if ((compression == ZLIB) != is_set) {
error_setg();
return -EINVAL;
}
return 0;
> +}
> +
> /* Called with s->lock held. */
> static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> int flags, Error **errp)
> @@ -1318,6 +1359,24 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
> s->compatible_features = header.compatible_features;
> s->autoclear_features = header.autoclear_features;
>
> + /* Handle compression type */
> + if (header.header_length > 104) {
Magic number. Please spell this as header.header_length > offsetof (xxx,
compression_type) for the appropriate xxx type. Also, do you need to
sanity check for an image using a length of 105-107 (invalid) vs. an
image of 108 or larger?
> + header.compression_type = be32_to_cpu(header.compression_type);
> + s->compression_type = header.compression_type;
> + } else {
> + /*
> + * older qcow2 images don't contain the compression type header
> + * field, distinguish them by the header length and use
> + * the only valid compression type in that case
> + */
> + s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
> + }
> +
> + ret = check_compression_type(s, errp);
> + if (ret) {
> + goto fail;
> + }
> +
> if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
> void *feature_table = NULL;
> qcow2_read_extensions(bs, header.header_length, ext_end,
> @@ -2434,6 +2493,13 @@ int qcow2_update_header(BlockDriverState *bs)
> total_size = bs->total_sectors * BDRV_SECTOR_SIZE;
> refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 3);
>
> + ret = check_compression_type(s, NULL);
Why are you ignoring the error here?
> @@ -5239,6 +5327,12 @@ static QemuOptsList qcow2_create_opts = {
> .help = "Width of a reference count entry in bits",
> .def_value_str = "16"
> },
> + {
> + .name = BLOCK_OPT_COMPRESSION_TYPE,
> + .type = QEMU_OPT_STRING,
> + .help = "Compression method used for image clusters compression",
> + .def_value_str = "0"
Eww. Why are we exposing an integer rather than an enum value as the
default value? This should probably be "zlib".
> +++ b/docs/interop/qcow2.txt
> @@ -109,7 +109,12 @@ in the description of a field.
> An External Data File Name header extension may
> be present if this bit is set.
>
> - Bits 3-63: Reserved (set to 0)
> + Bit 3: Compression type bit. The bit must be set if
> + the compression type differs from default: zlib.
Maybe: "differs from the default of zlib".
Up to here is okay.
> + If the compression type is default the bit must
> + be unset.
Is this restriction necessary?
> +
> + Bits 4-63: Reserved (set to 0)
>
> 80 - 87: compatible_features
> Bitmask of compatible features. An implementation can
> @@ -165,6 +170,21 @@ in the description of a field.
> Length of the header structure in bytes. For version 2
> images, the length is always assumed to be 72 bytes.
>
> + 104 - 107: compression_type
> + Defines the compression method used for compressed clusters.
> + A single compression type is applied to all compressed image
> + clusters.
> + The compression type is set on image creation only.
> + The default compression type is zlib.
Where is the documentation that a value of 0 corresponds to zlib?
> + When the deafult compression type is used the compression
default
> + type bit (incompatible feature bit 3) must be unset.
Is this restriction necessary?
> + When any other compression type is used the compression
> + type bit must be set.
> + Qemu versions older than 4.1 can use images created with
> + the default compression type without any additional
> + preparations and cannot use images created with any other
> + compression type.
> +
> Directly after the image header, optional sections called header extensions can
> be stored. Each extension has a structure like the following:
>
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 01e855a066..814917baec 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -58,6 +58,7 @@
> #define BLOCK_OPT_REFCOUNT_BITS "refcount_bits"
> #define BLOCK_OPT_DATA_FILE "data_file"
> #define BLOCK_OPT_DATA_FILE_RAW "data_file_raw"
> +#define BLOCK_OPT_COMPRESSION_TYPE "compression_type"
>
> #define BLOCK_PROBE_BUF_SIZE 512
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7ccbfff9d0..6aa8b99993 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -78,6 +78,8 @@
> #
> # @bitmaps: A list of qcow2 bitmap details (since 4.0)
> #
> +# @compression-type: the image cluster compression method (since 4.1)
> +#
> # Since: 1.7
> ##
> { 'struct': 'ImageInfoSpecificQCow2',
> @@ -89,7 +91,8 @@
> '*corrupt': 'bool',
> 'refcount-bits': 'int',
> '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> - '*bitmaps': ['Qcow2BitmapInfo']
> + '*bitmaps': ['Qcow2BitmapInfo'],
> + '*compression-type': 'Qcow2CompressionType'
Why is this field optional? Can't we always populate it, even for older
images?
> } }
>
> ##
> @@ -4206,6 +4209,18 @@
> 'data': [ 'v2', 'v3' ] }
>
>
> +##
> +# @Qcow2CompressionType:
> +#
> +# Compression type used in qcow2 image file
> +#
> +# @zlib: zlib compression, see <http://zlib.net/>
> +#
> +# Since: 4.1
> +##
> +{ 'enum': 'Qcow2CompressionType',
> + 'data': [ 'zlib' ] }
> +
> ##
> # @BlockdevCreateOptionsQcow2:
> #
> @@ -4228,6 +4243,8 @@
> # @preallocation Preallocation mode for the new image (default: off)
> # @lazy-refcounts True if refcounts may be updated lazily (default: off)
> # @refcount-bits Width of reference counts in bits (default: 16)
> +# @compression-type The image cluster compression method
> +# (default: zlib, since 4.1)
> #
> # Since: 2.12
> ##
> @@ -4243,7 +4260,8 @@
> '*cluster-size': 'size',
> '*preallocation': 'PreallocMode',
> '*lazy-refcounts': 'bool',
> - '*refcount-bits': 'int' } }
> + '*refcount-bits': 'int',
> + '*compression-type': 'Qcow2CompressionType' } }
But this one must indeed be optional.
>
> ##
> # @BlockdevCreateOptionsQed:
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v1 3/3] qcow2: add zstd cluster compression
2019-07-03 11:00 ` [Qemu-devel] [PATCH v1 3/3] qcow2: add zstd cluster compression Denis Plotnikov
@ 2019-07-03 14:36 ` Eric Blake
2019-07-03 15:06 ` Denis Plotnikov
2019-07-03 16:07 ` Kevin Wolf
0 siblings, 2 replies; 14+ messages in thread
From: Eric Blake @ 2019-07-03 14:36 UTC (permalink / raw)
To: Denis Plotnikov, kwolf, mreitz, armbru
Cc: vsementsov, den, qemu-block, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 2659 bytes --]
On 7/3/19 6:00 AM, Denis Plotnikov wrote:
> 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.
>
> ---
> block/qcow2.c | 96 ++++++++++++++++++++++++++++++++++++++++++++
> configure | 32 +++++++++++++++
> qapi/block-core.json | 3 +-
> 3 files changed, 130 insertions(+), 1 deletion(-)
Where is the change to docs/interop/qcow2.txt to describe this new
compression format?
>
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 37a563a671..caa04b0beb 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -27,6 +27,11 @@
> #define ZLIB_CONST
> #include <zlib.h>
>
> +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
> + const void *src, size_t src_size)
> +{
> + ssize_t ret;
> + uint32_t *c_size = dest;
> + /* steal some bytes to store compressed chunk size */
> + char *d_buf = ((char *) dest) + sizeof(*c_size);
> +
Do you always want exactly 4 bytes for the compressed size? Or is it
worth some sort of variable-length encoding, since we're already dealing
with non-cacheline-aligned data? You can represent all sizes up to 4M
using a maximum of 3 bytes (set the high bit if the integer continues,
then sizes 0-127 take 1 byte [7 bits], 128-32767 take 2 bytes [15 bits],
and 32768-4194303 take 3 bytes [22 bits]).
> + if (dest_size < sizeof(*c_size)) {
> + return -ENOMEM;
> + }
> +
> + dest_size -= sizeof(*c_size);
> +
> + ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
The fact that you are storing the size separate from the data passed to
zstd MUST be documented in the qcow2 spec, for the next person to
produce/consume the same data.
> +++ b/qapi/block-core.json
> @@ -4215,11 +4215,12 @@
> # Compression type used in qcow2 image file
> #
> # @zlib: zlib compression, see <http://zlib.net/>
> +# @zstd: zstd compression, see <http://github.com/facebook/zstd>
> #
> # Since: 4.1
> ##
> { 'enum': 'Qcow2CompressionType',
> - 'data': [ 'zlib' ] }
> + 'data': [ 'zlib', 'zstd' ] }
Since you patched configure so that linking against zstd is optional,
this should use { 'name':'zstd', 'if':'CONDITIONAL' } so that during
introspection, the enum only advertises zstd on a build that linked
against the library.
--
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] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature
2019-07-03 14:14 ` Eric Blake
@ 2019-07-03 15:01 ` Denis Plotnikov
2019-07-03 15:13 ` Eric Blake
2019-07-03 15:46 ` Kevin Wolf
0 siblings, 2 replies; 14+ messages in thread
From: Denis Plotnikov @ 2019-07-03 15:01 UTC (permalink / raw)
To: Eric Blake, kwolf@redhat.com, mreitz@redhat.com,
armbru@redhat.com
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block@nongnu.org,
qemu-devel@nongnu.org
On 03.07.2019 17:14, Eric Blake wrote:
> On 7/3/19 6:00 AM, Denis Plotnikov wrote:
>> The patch adds some preparation parts for incompatible compression type
>> feature to QCOW2 header that indicates that*all* compressed clusters
>> must be (de)compressed using a certain compression type.
>>
>> It is implied that the compression type is set on the image creation and
>> can be changed only later by image conversion, thus compression type
>> defines the only compression algorithm used for the image.
>>
>> The goal of the feature is to add support of other compression algorithms
>> to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
>> It works roughly x2 faster than ZLIB providing a comparable compression ratio
> 2x
>
>> and therefore provide a performance advantage in backup scenarios.
>>
>> The default compression is ZLIB. Images created with ZLIB compression type
>> is backward compatible with older qemu versions.
> s/is/are/
grammar matters, will be changed
>
>> Signed-off-by: Denis Plotnikov<dplotnikov@virtuozzo.com>
>> ---
>> block/qcow2.c | 94 +++++++++++++++++++++++++++++++++++++++
>> block/qcow2.h | 26 ++++++++---
>> docs/interop/qcow2.txt | 22 ++++++++-
>> include/block/block_int.h | 1 +
>> qapi/block-core.json | 22 ++++++++-
>> 5 files changed, 155 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 3ace3b2209..921eb67b80 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1202,6 +1202,47 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
>> return ret;
>> }
>>
>> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
>> +{
>> + bool is_set;
>> + int ret = 0;
>> +
>> + switch (s->compression_type) {
>> + case QCOW2_COMPRESSION_TYPE_ZLIB:
>> + break;
>> +
>> + default:
>> + if (errp) {
> Useless check for errp being non-NULL. What's worse, if the caller
> passes in NULL because they don't care about the error, then your code
> behaves differently. Just call error_setg() and return -ENOTSUP
> unconditionally.
ok
>
>> + error_setg(errp, "qcow2: unknown compression type: %u",
>> + s->compression_type);
>> + return -ENOTSUP;
>> + }
>> + }
>> +
>> + /*
>> + * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
>> + * feature flag must be absent, with other compression types the
>> + * incompatible feature flag must be set
> Is there a strong reason for forbid the incompatible feature flag with
> ZLIB?
The main reason is to guarantee image opening for older qemu if
compression type is zlib.
> Sure, it makes the image impossible to open with older qemu, but
> it doesn't get in the way of newer qemu opening it. I'll have to see how
> your spec changes documented this, to see if you could instead word it
> as 'for ZLIB, the incompatible feature flag may be absent'.
I just can't imagine when and why we would want to set incompatible
feature flag with zlib. Suppose we have zlib with the flag set, then
older qemu can't open the image although it is able to work with the
zlib compression type. For now, I don't understand why we should make
such an artificial restriction.
>
>> + */
>> + is_set = s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE;
>> +
>> + if (s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) {
>> + if (is_set) {
>> + ret = -EINVAL;
>> + }
>> + } else {
>> + if (!is_set) {
>> + ret = -EINVAL;
>> + }
>> + }
> More compact as:
>
> if ((s->compression_type == QCOW2_COMPRESSION_TYPE_ZLIB) == is_set)
ok
>
>> +
>> + if (ret && errp) {
>> + error_setg(errp, "qcow2: Illegal compression type setting");
> s/Illegal/Invalid/ (the user isn't breaking a law)
Don't know, may be they do :) but, I'll change it
>
> Also, don't check whether errp is non-NULL. Just blindly call
> error_setg() if ret is non-zero.
ok
>
>> + }
>> +
>> + return ret;
> Or even shorter (psuedocode):
>
> if ((compression == ZLIB) != is_set) {
> error_setg();
> return -EINVAL;
> }
> return 0;
>
>> +}
>> +
>> /* Called with s->lock held. */
>> static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>> int flags, Error **errp)
>> @@ -1318,6 +1359,24 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>> s->compatible_features = header.compatible_features;
>> s->autoclear_features = header.autoclear_features;
>>
>> + /* Handle compression type */
>> + if (header.header_length > 104) {
> Magic number. Please spell this as header.header_length > offsetof (xxx,
> compression_type) for the appropriate xxx type. Also, do you need to
> sanity check for an image using a length of 105-107 (invalid) vs. an
> image of 108 or larger?
Nice, will change it
>
>> + header.compression_type = be32_to_cpu(header.compression_type);
>> + s->compression_type = header.compression_type;
>> + } else {
>> + /*
>> + * older qcow2 images don't contain the compression type header
>> + * field, distinguish them by the header length and use
>> + * the only valid compression type in that case
>> + */
>> + s->compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
>> + }
>> +
>> + ret = check_compression_type(s, errp);
>> + if (ret) {
>> + goto fail;
>> + }
>> +
>> if (s->incompatible_features & ~QCOW2_INCOMPAT_MASK) {
>> void *feature_table = NULL;
>> qcow2_read_extensions(bs, header.header_length, ext_end,
>> @@ -2434,6 +2493,13 @@ int qcow2_update_header(BlockDriverState *bs)
>> total_size = bs->total_sectors * BDRV_SECTOR_SIZE;
>> refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 3);
>>
>> + ret = check_compression_type(s, NULL);
> Why are you ignoring the error here?
qcow2_update_header() doesn't use the error and just returns an error
code or 0
>
>
>> @@ -5239,6 +5327,12 @@ static QemuOptsList qcow2_create_opts = {
>> .help = "Width of a reference count entry in bits",
>> .def_value_str = "16"
>> },
>> + {
>> + .name = BLOCK_OPT_COMPRESSION_TYPE,
>> + .type = QEMU_OPT_STRING,
>> + .help = "Compression method used for image clusters compression",
>> + .def_value_str = "0"
> Eww. Why are we exposing an integer rather than an enum value as the
> default value? This should probably be "zlib".
Sure
>
>
>> +++ b/docs/interop/qcow2.txt
>> @@ -109,7 +109,12 @@ in the description of a field.
>> An External Data File Name header extension may
>> be present if this bit is set.
>>
>> - Bits 3-63: Reserved (set to 0)
>> + Bit 3: Compression type bit. The bit must be set if
>> + the compression type differs from default: zlib.
> Maybe: "differs from the default of zlib".
>
> Up to here is okay.
>
>> + If the compression type is default the bit must
>> + be unset.
> Is this restriction necessary?
Please, see above about older qemu usage
>
>> +
>> + Bits 4-63: Reserved (set to 0)
>>
>> 80 - 87: compatible_features
>> Bitmask of compatible features. An implementation can
>> @@ -165,6 +170,21 @@ in the description of a field.
>> Length of the header structure in bytes. For version 2
>> images, the length is always assumed to be 72 bytes.
>>
>> + 104 - 107: compression_type
>> + Defines the compression method used for compressed clusters.
>> + A single compression type is applied to all compressed image
>> + clusters.
>> + The compression type is set on image creation only.
>> + The default compression type is zlib.
> Where is the documentation that a value of 0 corresponds to zlib?
Should I do it right here or it's better to add a separate chapter in
the docs/interop/qcow2.txt ?
>
>> + When the deafult compression type is used the compression
> default
>
>> + type bit (incompatible feature bit 3) must be unset.
> Is this restriction necessary?
>
>> + When any other compression type is used the compression
>> + type bit must be set.
>> + Qemu versions older than 4.1 can use images created with
>> + the default compression type without any additional
>> + preparations and cannot use images created with any other
>> + compression type.
>> +
>> Directly after the image header, optional sections called header extensions can
>> be stored. Each extension has a structure like the following:
>>
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index 01e855a066..814917baec 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -58,6 +58,7 @@
>> #define BLOCK_OPT_REFCOUNT_BITS "refcount_bits"
>> #define BLOCK_OPT_DATA_FILE "data_file"
>> #define BLOCK_OPT_DATA_FILE_RAW "data_file_raw"
>> +#define BLOCK_OPT_COMPRESSION_TYPE "compression_type"
>>
>> #define BLOCK_PROBE_BUF_SIZE 512
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 7ccbfff9d0..6aa8b99993 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -78,6 +78,8 @@
>> #
>> # @bitmaps: A list of qcow2 bitmap details (since 4.0)
>> #
>> +# @compression-type: the image cluster compression method (since 4.1)
>> +#
>> # Since: 1.7
>> ##
>> { 'struct': 'ImageInfoSpecificQCow2',
>> @@ -89,7 +91,8 @@
>> '*corrupt': 'bool',
>> 'refcount-bits': 'int',
>> '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>> - '*bitmaps': ['Qcow2BitmapInfo']
>> + '*bitmaps': ['Qcow2BitmapInfo'],
>> + '*compression-type': 'Qcow2CompressionType'
> Why is this field optional? Can't we always populate it, even for older
> images?
Why we should if we don't care ?
>
>> } }
>>
>> ##
>> @@ -4206,6 +4209,18 @@
>> 'data': [ 'v2', 'v3' ] }
>>
>>
>> +##
>> +# @Qcow2CompressionType:
>> +#
>> +# Compression type used in qcow2 image file
>> +#
>> +# @zlib: zlib compression, see<http://zlib.net/>
>> +#
>> +# Since: 4.1
>> +##
>> +{ 'enum': 'Qcow2CompressionType',
>> + 'data': [ 'zlib' ] }
>> +
>> ##
>> # @BlockdevCreateOptionsQcow2:
>> #
>> @@ -4228,6 +4243,8 @@
>> # @preallocation Preallocation mode for the new image (default: off)
>> # @lazy-refcounts True if refcounts may be updated lazily (default: off)
>> # @refcount-bits Width of reference counts in bits (default: 16)
>> +# @compression-type The image cluster compression method
>> +# (default: zlib, since 4.1)
>> #
>> # Since: 2.12
>> ##
>> @@ -4243,7 +4260,8 @@
>> '*cluster-size': 'size',
>> '*preallocation': 'PreallocMode',
>> '*lazy-refcounts': 'bool',
>> - '*refcount-bits': 'int' } }
>> + '*refcount-bits': 'int',
>> + '*compression-type': 'Qcow2CompressionType' } }
> But this one must indeed be optional.
>
>>
>> ##
>> # @BlockdevCreateOptionsQed:
>>
> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226
> Virtualization: qemu.org | libvirt.org
>
--
Best,
Denis
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v1 3/3] qcow2: add zstd cluster compression
2019-07-03 14:36 ` Eric Blake
@ 2019-07-03 15:06 ` Denis Plotnikov
2019-07-03 16:07 ` Kevin Wolf
1 sibling, 0 replies; 14+ messages in thread
From: Denis Plotnikov @ 2019-07-03 15:06 UTC (permalink / raw)
To: Eric Blake, kwolf@redhat.com, mreitz@redhat.com,
armbru@redhat.com
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block@nongnu.org,
qemu-devel@nongnu.org
On 03.07.2019 17:36, Eric Blake wrote:
> On 7/3/19 6:00 AM, Denis Plotnikov wrote:
>> 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.
>>
>
>> ---
>> block/qcow2.c | 96 ++++++++++++++++++++++++++++++++++++++++++++
>> configure | 32 +++++++++++++++
>> qapi/block-core.json | 3 +-
>> 3 files changed, 130 insertions(+), 1 deletion(-)
>
> Where is the change to docs/interop/qcow2.txt to describe this new
> compression format?
>
ok
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 37a563a671..caa04b0beb 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -27,6 +27,11 @@
>> #define ZLIB_CONST
>> #include <zlib.h>
>>
>
>> +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
>> + const void *src, size_t src_size)
>> +{
>> + ssize_t ret;
>> + uint32_t *c_size = dest;
>> + /* steal some bytes to store compressed chunk size */
>> + char *d_buf = ((char *) dest) + sizeof(*c_size);
>> +
>
> Do you always want exactly 4 bytes for the compressed size? Or is it
> worth some sort of variable-length encoding, since we're already dealing
> with non-cacheline-aligned data? You can represent all sizes up to 4M
> using a maximum of 3 bytes (set the high bit if the integer continues,
> then sizes 0-127 take 1 byte [7 bits], 128-32767 take 2 bytes [15 bits],
> and 32768-4194303 take 3 bytes [22 bits]).
Is it really worth to do that? I liked Kevin's comment that the size
should complain with some variable size
>
>> + if (dest_size < sizeof(*c_size)) {
>> + return -ENOMEM;
>> + }
>> +
>> + dest_size -= sizeof(*c_size);
>> +
>> + ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
>
> The fact that you are storing the size separate from the data passed to
> zstd MUST be documented in the qcow2 spec, for the next person to
> produce/consume the same data.
Should I add a separate chapter there?
>
>
>> +++ b/qapi/block-core.json
>> @@ -4215,11 +4215,12 @@
>> # Compression type used in qcow2 image file
>> #
>> # @zlib: zlib compression, see <http://zlib.net/>
>> +# @zstd: zstd compression, see <http://github.com/facebook/zstd>
>> #
>> # Since: 4.1
>> ##
>> { 'enum': 'Qcow2CompressionType',
>> - 'data': [ 'zlib' ] }
>> + 'data': [ 'zlib', 'zstd' ] }
>
> Since you patched configure so that linking against zstd is optional,
> this should use { 'name':'zstd', 'if':'CONDITIONAL' } so that during
> introspection, the enum only advertises zstd on a build that linked
> against the library.
Sure, will change it
>
--
Best,
Denis
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature
2019-07-03 15:01 ` Denis Plotnikov
@ 2019-07-03 15:13 ` Eric Blake
2019-07-03 15:37 ` Denis Plotnikov
2019-07-03 15:46 ` Kevin Wolf
1 sibling, 1 reply; 14+ messages in thread
From: Eric Blake @ 2019-07-03 15:13 UTC (permalink / raw)
To: Denis Plotnikov, kwolf@redhat.com, mreitz@redhat.com,
armbru@redhat.com
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block@nongnu.org,
qemu-devel@nongnu.org
[-- Attachment #1.1: Type: text/plain, Size: 4492 bytes --]
On 7/3/19 10:01 AM, Denis Plotnikov wrote:
>>> + * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
>>> + * feature flag must be absent, with other compression types the
>>> + * incompatible feature flag must be set
>> Is there a strong reason for forbid the incompatible feature flag with
>> ZLIB?
> The main reason is to guarantee image opening for older qemu if
> compression type is zlib.
>> Sure, it makes the image impossible to open with older qemu, but
>> it doesn't get in the way of newer qemu opening it. I'll have to see how
>> your spec changes documented this, to see if you could instead word it
>> as 'for ZLIB, the incompatible feature flag may be absent'.
> I just can't imagine when and why we would want to set incompatible
> feature flag with zlib. Suppose we have zlib with the flag set, then
> older qemu can't open the image although it is able to work with the
> zlib compression type. For now, I don't understand why we should make
> such an artificial restriction.
We have an artificial restriction one way or the other.
Method 1 - artificial restriction that incompatible bit must NOT be set
when compression type is zlib
Method 2 - artificial restriction that older qcow2 programs can't open a
zlib image with incompatible bit set, even though removing the bit makes
the image more portable.
It's desirable that qemu should NOT set the incompatible bit when it
does not need to (for the sake of portability to more apps), but
MANDATING that it must not set the bit for zlib is a stronger spec
restriction.
I tend to lean for the spec being looser unless there is a strong reason
for why it must be strict; just because qemu won't be setting the
incompatible bit does not necessarily mean that all other
implementations will try to be that careful; they may have valid reasons
for setting the bit even when using zlib, but only if the spec permits
them to do so.
>>> @@ -2434,6 +2493,13 @@ int qcow2_update_header(BlockDriverState *bs)
>>> total_size = bs->total_sectors * BDRV_SECTOR_SIZE;
>>> refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 3);
>>>
>>> + ret = check_compression_type(s, NULL);
>> Why are you ignoring the error here?
> qcow2_update_header() doesn't use the error and just returns an error
> code or 0
Are we potentially losing a valuable error message (in which case
qcow2_update_header should maybe be first patched to take an errp
parameter), or is it always going to succeed (in which case &error_abort
would document our intention that we know it can't fail), or is it
really a case where it may fail, but we don't care about losing the
message? Passing NULL is not wrong (as you say, we aren't plumbed to
pass the message back up to the caller), but does raise enough
suspicions as to be worth auditing the code while in the area.
>>> + 104 - 107: compression_type
>>> + Defines the compression method used for compressed clusters.
>>> + A single compression type is applied to all compressed image
>>> + clusters.
>>> + The compression type is set on image creation only.
>>> + The default compression type is zlib.
>> Where is the documentation that a value of 0 corresponds to zlib?
> Should I do it right here or it's better to add a separate chapter in
> the docs/interop/qcow2.txt ?
Right here.
>>> +++ b/qapi/block-core.json
>>> @@ -78,6 +78,8 @@
>>> #
>>> # @bitmaps: A list of qcow2 bitmap details (since 4.0)
>>> #
>>> +# @compression-type: the image cluster compression method (since 4.1)
>>> +#
>>> # Since: 1.7
>>> ##
>>> { 'struct': 'ImageInfoSpecificQCow2',
>>> @@ -89,7 +91,8 @@
>>> '*corrupt': 'bool',
>>> 'refcount-bits': 'int',
>>> '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>>> - '*bitmaps': ['Qcow2BitmapInfo']
>>> + '*bitmaps': ['Qcow2BitmapInfo'],
>>> + '*compression-type': 'Qcow2CompressionType'
>> Why is this field optional? Can't we always populate it, even for older
>> images?
> Why we should if we don't care ?
Because it shows that we are running a new enough qemu that knows about
the compression field (even when it is absent).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature
2019-07-03 15:13 ` Eric Blake
@ 2019-07-03 15:37 ` Denis Plotnikov
0 siblings, 0 replies; 14+ messages in thread
From: Denis Plotnikov @ 2019-07-03 15:37 UTC (permalink / raw)
To: Eric Blake, kwolf@redhat.com, mreitz@redhat.com,
armbru@redhat.com
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block@nongnu.org,
qemu-devel@nongnu.org
On 03.07.2019 18:13, Eric Blake wrote:
> On 7/3/19 10:01 AM, Denis Plotnikov wrote:
>
>>>> + * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
>>>> + * feature flag must be absent, with other compression types the
>>>> + * incompatible feature flag must be set
>>> Is there a strong reason for forbid the incompatible feature flag with
>>> ZLIB?
>> The main reason is to guarantee image opening for older qemu if
>> compression type is zlib.
>>> Sure, it makes the image impossible to open with older qemu, but
>>> it doesn't get in the way of newer qemu opening it. I'll have to see how
>>> your spec changes documented this, to see if you could instead word it
>>> as 'for ZLIB, the incompatible feature flag may be absent'.
>> I just can't imagine when and why we would want to set incompatible
>> feature flag with zlib. Suppose we have zlib with the flag set, then
>> older qemu can't open the image although it is able to work with the
>> zlib compression type. For now, I don't understand why we should make
>> such an artificial restriction.
>
> We have an artificial restriction one way or the other.
>
> Method 1 - artificial restriction that incompatible bit must NOT be set
> when compression type is zlib
>
> Method 2 - artificial restriction that older qcow2 programs can't open a
> zlib image with incompatible bit set, even though removing the bit makes
> the image more portable.
>
> It's desirable that qemu should NOT set the incompatible bit when it
> does not need to (for the sake of portability to more apps), but
> MANDATING that it must not set the bit for zlib is a stronger spec
> restriction.
>
> I tend to lean for the spec being looser unless there is a strong reason
> for why it must be strict; just because qemu won't be setting the
> incompatible bit does not necessarily mean that all other
> implementations will try to be that careful; they may have valid reasons
> for setting the bit even when using zlib, but only if the spec permits
> them to do so.
So, how I should implement that? Method 1 or Method 2?
How we can decide? Ask what other maintainers think about that?
>
>
>>>> @@ -2434,6 +2493,13 @@ int qcow2_update_header(BlockDriverState *bs)
>>>> total_size = bs->total_sectors * BDRV_SECTOR_SIZE;
>>>> refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 3);
>>>>
>>>> + ret = check_compression_type(s, NULL);
>>> Why are you ignoring the error here?
>> qcow2_update_header() doesn't use the error and just returns an error
>> code or 0
>
> Are we potentially losing a valuable error message (in which case
> qcow2_update_header should maybe be first patched to take an errp
> parameter), or is it always going to succeed (in which case &error_abort
> would document our intention that we know it can't fail), or is it
> really a case where it may fail, but we don't care about losing the
> message? Passing NULL is not wrong (as you say, we aren't plumbed to
> pass the message back up to the caller), but does raise enough
> suspicions as to be worth auditing the code while in the area.
Could we do it after adding this series since it already implemented
without the error?
>
>
>>>> + 104 - 107: compression_type
>>>> + Defines the compression method used for compressed clusters.
>>>> + A single compression type is applied to all compressed image
>>>> + clusters.
>>>> + The compression type is set on image creation only.
>>>> + The default compression type is zlib.
>>> Where is the documentation that a value of 0 corresponds to zlib?
>> Should I do it right here or it's better to add a separate chapter in
>> the docs/interop/qcow2.txt ?
>
> Right here.
ok
>
>
>>>> +++ b/qapi/block-core.json
>>>> @@ -78,6 +78,8 @@
>>>> #
>>>> # @bitmaps: A list of qcow2 bitmap details (since 4.0)
>>>> #
>>>> +# @compression-type: the image cluster compression method (since 4.1)
>>>> +#
>>>> # Since: 1.7
>>>> ##
>>>> { 'struct': 'ImageInfoSpecificQCow2',
>>>> @@ -89,7 +91,8 @@
>>>> '*corrupt': 'bool',
>>>> 'refcount-bits': 'int',
>>>> '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>>>> - '*bitmaps': ['Qcow2BitmapInfo']
>>>> + '*bitmaps': ['Qcow2BitmapInfo'],
>>>> + '*compression-type': 'Qcow2CompressionType'
>>> Why is this field optional? Can't we always populate it, even for older
>>> images?
>> Why we should if we don't care ?
>
> Because it shows that we are running a new enough qemu that knows about
> the compression field (even when it is absent).
ok, if everybody agree with that I'll implement whatever is better
>
--
Best,
Denis
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature
2019-07-03 15:01 ` Denis Plotnikov
2019-07-03 15:13 ` Eric Blake
@ 2019-07-03 15:46 ` Kevin Wolf
2019-07-03 15:54 ` Denis Plotnikov
1 sibling, 1 reply; 14+ messages in thread
From: Kevin Wolf @ 2019-07-03 15:46 UTC (permalink / raw)
To: Denis Plotnikov
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block@nongnu.org,
qemu-devel@nongnu.org, armbru@redhat.com, mreitz@redhat.com
Am 03.07.2019 um 17:01 hat Denis Plotnikov geschrieben:
> On 03.07.2019 17:14, Eric Blake wrote:
> > On 7/3/19 6:00 AM, Denis Plotnikov wrote:
> >> diff --git a/block/qcow2.c b/block/qcow2.c
> >> index 3ace3b2209..921eb67b80 100644
> >> --- a/block/qcow2.c
> >> +++ b/block/qcow2.c
> >> @@ -1202,6 +1202,47 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
> >> return ret;
> >> }
> >>
> >> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
> >> +{
> >> + bool is_set;
> >> + int ret = 0;
> >> +
> >> + switch (s->compression_type) {
> >> + case QCOW2_COMPRESSION_TYPE_ZLIB:
> >> + break;
> >> +
> >> + default:
> >> + if (errp) {
> > Useless check for errp being non-NULL. What's worse, if the caller
> > passes in NULL because they don't care about the error, then your code
> > behaves differently. Just call error_setg() and return -ENOTSUP
> > unconditionally.
> ok
> >
> >> + error_setg(errp, "qcow2: unknown compression type: %u",
> >> + s->compression_type);
> >> + return -ENOTSUP;
> >> + }
> >> + }
> >> +
> >> + /*
> >> + * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
> >> + * feature flag must be absent, with other compression types the
> >> + * incompatible feature flag must be set
> > Is there a strong reason for forbid the incompatible feature flag with
> > ZLIB?
> The main reason is to guarantee image opening for older qemu if
> compression type is zlib.
> > Sure, it makes the image impossible to open with older qemu, but
> > it doesn't get in the way of newer qemu opening it. I'll have to see how
> > your spec changes documented this, to see if you could instead word it
> > as 'for ZLIB, the incompatible feature flag may be absent'.
> I just can't imagine when and why we would want to set incompatible
> feature flag with zlib. Suppose we have zlib with the flag set, then
> older qemu can't open the image although it is able to work with the
> zlib compression type. For now, I don't understand why we should make
> such an artificial restriction.
We don't want to create such images, but we want to keep our code as
simple as possible.
As your patch shows, forbidding the case is more work than just allowing
it. So if external software can create such images, and it would just
automatically work in QEMU, then why do the extra work to articifially
forbid this?
(Actually, it's likely that on the first header update, QEMU would just
end up dropping the useless flag, so we even "fix" such images.)
> >> diff --git a/qapi/block-core.json b/qapi/block-core.json
> >> index 7ccbfff9d0..6aa8b99993 100644
> >> --- a/qapi/block-core.json
> >> +++ b/qapi/block-core.json
> >> @@ -78,6 +78,8 @@
> >> #
> >> # @bitmaps: A list of qcow2 bitmap details (since 4.0)
> >> #
> >> +# @compression-type: the image cluster compression method (since 4.1)
> >> +#
> >> # Since: 1.7
> >> ##
> >> { 'struct': 'ImageInfoSpecificQCow2',
> >> @@ -89,7 +91,8 @@
> >> '*corrupt': 'bool',
> >> 'refcount-bits': 'int',
> >> '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> >> - '*bitmaps': ['Qcow2BitmapInfo']
> >> + '*bitmaps': ['Qcow2BitmapInfo'],
> >> + '*compression-type': 'Qcow2CompressionType'
> > Why is this field optional? Can't we always populate it, even for older
> > images?
> Why we should if we don't care ?
I was trying too check what the condition is under which the field will
be present in the output, but I couldn't find any code for it.
So it looks like this patch never makes use of the field and it's dead
code?
Kevin
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature
2019-07-03 15:46 ` Kevin Wolf
@ 2019-07-03 15:54 ` Denis Plotnikov
0 siblings, 0 replies; 14+ messages in thread
From: Denis Plotnikov @ 2019-07-03 15:54 UTC (permalink / raw)
To: Kevin Wolf
Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block@nongnu.org,
qemu-devel@nongnu.org, armbru@redhat.com, mreitz@redhat.com
On 03.07.2019 18:46, Kevin Wolf wrote:
> Am 03.07.2019 um 17:01 hat Denis Plotnikov geschrieben:
>> On 03.07.2019 17:14, Eric Blake wrote:
>>> On 7/3/19 6:00 AM, Denis Plotnikov wrote:
>>>> diff --git a/block/qcow2.c b/block/qcow2.c
>>>> index 3ace3b2209..921eb67b80 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -1202,6 +1202,47 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
>>>> return ret;
>>>> }
>>>>
>>>> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
>>>> +{
>>>> + bool is_set;
>>>> + int ret = 0;
>>>> +
>>>> + switch (s->compression_type) {
>>>> + case QCOW2_COMPRESSION_TYPE_ZLIB:
>>>> + break;
>>>> +
>>>> + default:
>>>> + if (errp) {
>>> Useless check for errp being non-NULL. What's worse, if the caller
>>> passes in NULL because they don't care about the error, then your code
>>> behaves differently. Just call error_setg() and return -ENOTSUP
>>> unconditionally.
>> ok
>>>
>>>> + error_setg(errp, "qcow2: unknown compression type: %u",
>>>> + s->compression_type);
>>>> + return -ENOTSUP;
>>>> + }
>>>> + }
>>>> +
>>>> + /*
>>>> + * with QCOW2_COMPRESSION_TYPE_ZLIB the corresponding incompatible
>>>> + * feature flag must be absent, with other compression types the
>>>> + * incompatible feature flag must be set
>>> Is there a strong reason for forbid the incompatible feature flag with
>>> ZLIB?
>> The main reason is to guarantee image opening for older qemu if
>> compression type is zlib.
>>> Sure, it makes the image impossible to open with older qemu, but
>>> it doesn't get in the way of newer qemu opening it. I'll have to see how
>>> your spec changes documented this, to see if you could instead word it
>>> as 'for ZLIB, the incompatible feature flag may be absent'.
>> I just can't imagine when and why we would want to set incompatible
>> feature flag with zlib. Suppose we have zlib with the flag set, then
>> older qemu can't open the image although it is able to work with the
>> zlib compression type. For now, I don't understand why we should make
>> such an artificial restriction.
>
> We don't want to create such images, but we want to keep our code as
> simple as possible.
>
> As your patch shows, forbidding the case is more work than just allowing
> it. So if external software can create such images, and it would just
> automatically work in QEMU, then why do the extra work to articifially
> forbid this?
>
> (Actually, it's likely that on the first header update, QEMU would just
> end up dropping the useless flag, so we even "fix" such images.)
ok, removing the restriction
>
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 7ccbfff9d0..6aa8b99993 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -78,6 +78,8 @@
>>>> #
>>>> # @bitmaps: A list of qcow2 bitmap details (since 4.0)
>>>> #
>>>> +# @compression-type: the image cluster compression method (since 4.1)
>>>> +#
>>>> # Since: 1.7
>>>> ##
>>>> { 'struct': 'ImageInfoSpecificQCow2',
>>>> @@ -89,7 +91,8 @@
>>>> '*corrupt': 'bool',
>>>> 'refcount-bits': 'int',
>>>> '*encrypt': 'ImageInfoSpecificQCow2Encryption',
>>>> - '*bitmaps': ['Qcow2BitmapInfo']
>>>> + '*bitmaps': ['Qcow2BitmapInfo'],
>>>> + '*compression-type': 'Qcow2CompressionType'
>>> Why is this field optional? Can't we always populate it, even for older
>>> images?
>> Why we should if we don't care ?
>
> I was trying too check what the condition is under which the field will
> be present in the output, but I couldn't find any code for it.
>
> So it looks like this patch never makes use of the field and it's dead
> code?
ok
>
> Kevin
>
--
Best,
Denis
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v1 3/3] qcow2: add zstd cluster compression
2019-07-03 14:36 ` Eric Blake
2019-07-03 15:06 ` Denis Plotnikov
@ 2019-07-03 16:07 ` Kevin Wolf
1 sibling, 0 replies; 14+ messages in thread
From: Kevin Wolf @ 2019-07-03 16:07 UTC (permalink / raw)
To: Eric Blake
Cc: vsementsov, den, qemu-block, qemu-devel, armbru, mreitz,
Denis Plotnikov
[-- Attachment #1: Type: text/plain, Size: 2032 bytes --]
Am 03.07.2019 um 16:36 hat Eric Blake geschrieben:
> On 7/3/19 6:00 AM, Denis Plotnikov wrote:
> > 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.
> >
>
> > ---
> > block/qcow2.c | 96 ++++++++++++++++++++++++++++++++++++++++++++
> > configure | 32 +++++++++++++++
> > qapi/block-core.json | 3 +-
> > 3 files changed, 130 insertions(+), 1 deletion(-)
>
> Where is the change to docs/interop/qcow2.txt to describe this new
> compression format?
>
> >
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 37a563a671..caa04b0beb 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -27,6 +27,11 @@
> > #define ZLIB_CONST
> > #include <zlib.h>
> >
>
> > +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
> > + const void *src, size_t src_size)
> > +{
> > + ssize_t ret;
> > + uint32_t *c_size = dest;
> > + /* steal some bytes to store compressed chunk size */
> > + char *d_buf = ((char *) dest) + sizeof(*c_size);
> > +
>
> Do you always want exactly 4 bytes for the compressed size? Or is it
> worth some sort of variable-length encoding, since we're already dealing
> with non-cacheline-aligned data? You can represent all sizes up to 4M
> using a maximum of 3 bytes (set the high bit if the integer continues,
> then sizes 0-127 take 1 byte [7 bits], 128-32767 take 2 bytes [15 bits],
> and 32768-4194303 take 3 bytes [22 bits]).
I don't think the additional complexity is worth it. The combination of
endianness conversions (which are missing here, this is a bug!) and
variable lengths is almost guaranteed to not only give us headaches, but
also to result in corner case bugs.
Also, are we sure that we will always keep 2M as the maximum cluster
size?
Kevin
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature
2019-07-03 11:00 ` [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature Denis Plotnikov
2019-07-03 14:14 ` Eric Blake
@ 2019-07-09 11:27 ` Markus Armbruster
1 sibling, 0 replies; 14+ messages in thread
From: Markus Armbruster @ 2019-07-09 11:27 UTC (permalink / raw)
To: Denis Plotnikov; +Cc: kwolf, vsementsov, den, qemu-block, qemu-devel, mreitz
I'd like to recommend a few commas.
Denis Plotnikov <dplotnikov@virtuozzo.com> writes:
> The patch adds some preparation parts for incompatible compression type
> feature to QCOW2 header that indicates that *all* compressed clusters
> must be (de)compressed using a certain compression type.
>
> It is implied that the compression type is set on the image creation and
> can be changed only later by image conversion, thus compression type
> defines the only compression algorithm used for the image.
>
> The goal of the feature is to add support of other compression algorithms
> to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
> It works roughly x2 faster than ZLIB providing a comparable compression ratio
> and therefore provide a performance advantage in backup scenarios.
>
> The default compression is ZLIB. Images created with ZLIB compression type
> is backward compatible with older qemu versions.
>
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
[...]
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index af5711e533..5b8a8d15fe 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -109,7 +109,12 @@ in the description of a field.
> An External Data File Name header extension may
> be present if this bit is set.
>
> - Bits 3-63: Reserved (set to 0)
> + Bit 3: Compression type bit. The bit must be set if
> + the compression type differs from default: zlib.
> + If the compression type is default the bit must
> + be unset.
> +
> + Bits 4-63: Reserved (set to 0)
>
> 80 - 87: compatible_features
> Bitmask of compatible features. An implementation can
> @@ -165,6 +170,21 @@ in the description of a field.
> Length of the header structure in bytes. For version 2
> images, the length is always assumed to be 72 bytes.
>
> + 104 - 107: compression_type
> + Defines the compression method used for compressed clusters.
> + A single compression type is applied to all compressed image
> + clusters.
> + The compression type is set on image creation only.
> + The default compression type is zlib.
> + When the deafult compression type is used the compression
Comma after used, please.
> + type bit (incompatible feature bit 3) must be unset.
> + When any other compression type is used the compression
Likewise.
> + type bit must be set.
> + Qemu versions older than 4.1 can use images created with
> + the default compression type without any additional
> + preparations and cannot use images created with any other
Comma after preparations, please.
> + compression type.
> +
> Directly after the image header, optional sections called header extensions can
> be stored. Each extension has a structure like the following:
>
[...]
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-07-09 11:28 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-07-03 11:00 [Qemu-devel] [PATCH v1 0/3] add zstd cluster compression Denis Plotnikov
2019-07-03 11:00 ` [Qemu-devel] [PATCH v1 1/3] qcow2: introduce compression type feature Denis Plotnikov
2019-07-03 14:14 ` Eric Blake
2019-07-03 15:01 ` Denis Plotnikov
2019-07-03 15:13 ` Eric Blake
2019-07-03 15:37 ` Denis Plotnikov
2019-07-03 15:46 ` Kevin Wolf
2019-07-03 15:54 ` Denis Plotnikov
2019-07-09 11:27 ` Markus Armbruster
2019-07-03 11:00 ` [Qemu-devel] [PATCH v1 2/3] qcow2: rework the cluster compression routine Denis Plotnikov
2019-07-03 11:00 ` [Qemu-devel] [PATCH v1 3/3] qcow2: add zstd cluster compression Denis Plotnikov
2019-07-03 14:36 ` Eric Blake
2019-07-03 15:06 ` Denis Plotnikov
2019-07-03 16:07 ` Kevin Wolf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).