zstd significantly reduces cluster compression time.
It provides better compression performance maintaining
the same level of the compression ratio in comparison with
zlib, which, at the moment, is 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-threads.c | 122 +++++++++++++++++++++++++++++++++++++++++
block/qcow2.c | 7 +++
configure | 29 ++++++++++
docs/interop/qcow2.txt | 18 ++++++
qapi/block-core.json | 3 +-
5 files changed, 178 insertions(+), 1 deletion(-)
diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
index 1c128e9840..e942c4d7e5 100644
--- a/block/qcow2-threads.c
+++ b/block/qcow2-threads.c
@@ -28,6 +28,11 @@
#define ZLIB_CONST
#include <zlib.h>
+#ifdef CONFIG_ZSTD
+#include <zstd.h>
+#include <zstd_errors.h>
+#endif
+
#include "qcow2.h"
#include "block/thread-pool.h"
#include "crypto.h"
@@ -164,6 +169,113 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
return ret;
}
+#ifdef CONFIG_ZSTD
+
+#define ZSTD_LEN_BUF 4
+
+/*
+ * 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
+ * -ENOMEM destination buffer is not enough to store compressed data
+ * -EIO on any other error
+ */
+
+static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
+{
+ size_t ret;
+
+ /*
+ * steal ZSTD_LEN_BUF bytes in the very beginng of the buffer
+ * to store compressed chunk size
+ */
+ char *d_buf = ((char *) dest) + ZSTD_LEN_BUF;
+
+ /*
+ * sanity check that we can store the compressed data length,
+ * and there is some space left for the compressor buffer
+ */
+ if (dest_size <= ZSTD_LEN_BUF) {
+ return -ENOMEM;
+ }
+
+ dest_size -= ZSTD_LEN_BUF;
+
+ ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
+
+ if (ZSTD_isError(ret)) {
+ if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
+ return -ENOMEM;
+ } else {
+ return -EIO;
+ }
+ }
+
+ /* paraniod sanity check that we can store the commpressed size */
+ if (ret > UINT_MAX) {
+ return -ENOMEM;
+ }
+
+ /* store the compressed chunk size in the very beginning of the buffer */
+ stl_be_p(dest, ret);
+
+ return ret + ZSTD_LEN_BUF;
+}
+
+/*
+ * 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 any error
+ */
+
+static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size,
+ const void *src, size_t src_size)
+{
+ /*
+ * zstd decompress wants to know the exact length of the data.
+ * For that purpose, on compression, the length is stored in
+ * the very beginning of the compressed buffer
+ */
+ size_t s_size;
+ const char *s_buf = ((const char *) src) + ZSTD_LEN_BUF;
+
+ /*
+ * sanity check that we can read 4 byte the content length and
+ * and there is some content to decompress
+ */
+ if (src_size <= ZSTD_LEN_BUF) {
+ return -EIO;
+ }
+
+ s_size = ldl_be_p(src);
+
+ /* sanity check that the buffer is big enough to read the content from */
+ if (src_size - ZSTD_LEN_BUF < s_size) {
+ return -EIO;
+ }
+
+ if (ZSTD_isError(
+ ZSTD_decompress(dest, dest_size, s_buf, s_size))) {
+ return -EIO;
+ }
+
+ return 0;
+}
+#endif
+
static int qcow2_compress_pool_func(void *opaque)
{
Qcow2CompressData *data = opaque;
@@ -215,6 +327,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;
}
@@ -247,6 +364,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/block/qcow2.c b/block/qcow2.c
index 2ccb2cabd1..9c8ad9d580 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1257,6 +1257,9 @@ static int validate_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:
@@ -3506,6 +3509,10 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
}
switch (qcow2_opts->compression_type) {
+#ifdef CONFIG_ZSTD
+ case QCOW2_COMPRESSION_TYPE_ZSTD:
+ break;
+#endif
default:
error_setg_errno(errp, -EINVAL, "Unknown compression type");
goto out;
diff --git a/configure b/configure
index 48d6f89d57..4690a7ea9f 100755
--- a/configure
+++ b/configure
@@ -444,6 +444,7 @@ opengl_dmabuf="no"
cpuid_h="no"
avx2_opt=""
zlib="yes"
+zstd=""
capstone=""
lzo=""
snappy=""
@@ -1371,6 +1372,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"
@@ -1829,6 +1834,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
@@ -2453,6 +2459,25 @@ EOF
fi
fi
+#########################################
+# zstd check
+
+if test "$zstd" != "no" ; then
+ cat > $TMPC << EOF
+#include <zstd.h>
+int main(void) { ZSTD_versionNumber(); return 0; }
+EOF
+ if compile_prog "" "-lzstd" ; then
+ LIBS="$LIBS -lzstd"
+ zstd="yes"
+ else
+ if test "$zstd" = "yes"; then
+ feature_not_found "zstd" "Install libzstd-devel"
+ fi
+ zstd="no"
+ fi
+fi
+
##########################################
# libseccomp check
@@ -6668,6 +6693,7 @@ echo "lzo support $lzo"
echo "snappy support $snappy"
echo "bzip2 support $bzip2"
echo "lzfse support $lzfse"
+echo "zstd support $zstd"
echo "NUMA host support $numa"
echo "libxml2 $libxml2"
echo "tcmalloc support $tcmalloc"
@@ -7559,6 +7585,9 @@ if test "$plugins" = "yes" ; then
"\$ld_exported_symbols_list should have been set to 'yes'."
fi
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/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 5597e24474..aeca2ddebd 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -208,6 +208,7 @@ version 2.
Available compression type values:
0: zlib <https://www.zlib.net/>
+ 1: zstd <http://github.com/facebook/zstd>
=== Header padding ===
@@ -575,11 +576,28 @@ Compressed Clusters Descriptor (x = 62 - (cluster_bits - 8)):
Another compressed cluster may map to the tail of the final
sector used by this compressed cluster.
+ The layout of the compressed data depends on the compression
+ type used for the image (see compressed cluster layout).
+
If a cluster is unallocated, read requests shall read the data from the backing
file (except if bit 0 in the Standard Cluster Descriptor is set). If there is
no backing file or the backing file is smaller than the image, they shall read
zeros for all parts that are not covered by the backing file.
+=== Compressed Cluster Layout ===
+
+The compressed cluster data has a layout depending on the compression
+type used for the image, as follows:
+
+Compressed data layout for the available compression types:
+(x = data_space_length - 1)
+
+ 0: (default) zlib <http://zlib.net/>:
+ Byte 0 - x: the compressed data content
+ all the space provided used for compressed data
+ 1: zstd <http://github.com/facebook/zstd>:
+ Byte 0 - 3: the length of compressed data in bytes
+ 4 - x: the compressed data content
== Snapshots ==
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 873fbef3b5..4b6e576c44 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -4401,11 +4401,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: 5.0
##
{ 'enum': 'Qcow2CompressionType',
- 'data': [ 'zlib' ] }
+ 'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
##
# @BlockdevCreateOptionsQcow2:
--
2.17.0
On 2/27/20 1:29 AM, Denis Plotnikov wrote:
> zstd significantly reduces cluster compression time.
> It provides better compression performance maintaining
> the same level of the compression ratio in comparison with
> zlib, which, at the moment, is the only compression
> method available.
>
> +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
> + const void *src, size_t src_size)
> +{
> + size_t ret;
> +
> + /*
> + * steal ZSTD_LEN_BUF bytes in the very beginng of the buffer
beginning
> + * to store compressed chunk size
> + */
> + char *d_buf = ((char *) dest) + ZSTD_LEN_BUF;
> +
> + /*
> + * sanity check that we can store the compressed data length,
> + * and there is some space left for the compressor buffer
> + */
> + if (dest_size <= ZSTD_LEN_BUF) {
> + return -ENOMEM;
> + }
> +
> + dest_size -= ZSTD_LEN_BUF;
> +
> + ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
> +
> + if (ZSTD_isError(ret)) {
> + if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
> + return -ENOMEM;
> + } else {
> + return -EIO;
> + }
> + }
> +
> + /* paraniod sanity check that we can store the commpressed size */
paranoid, compressed
> + if (ret > UINT_MAX) {
> + return -ENOMEM;
> + }
This is pointless. Better is to ensure that we actually compressed data
(the pigeonhole principle states that there are some inputs that MUST
result in inflation, in order for most other inputs to result in
compression). But that check was satisfied by checking for
ZSTD_error_dstSize_tooSmall, which is what happens for one of those
uncompressible inputs. Namely, zstd will never return a result larger
than dest_size, and since dest_size is smaller than UINT_MAX on entry,
this check is pointless. But if you want something, I'd be okay with:
assert(ret <= dest_size).
> +++ b/docs/interop/qcow2.txt
> @@ -208,6 +208,7 @@ version 2.
>
> Available compression type values:
> 0: zlib <https://www.zlib.net/>
> + 1: zstd <http://github.com/facebook/zstd>
>
>
> === Header padding ===
> @@ -575,11 +576,28 @@ Compressed Clusters Descriptor (x = 62 - (cluster_bits - 8)):
> Another compressed cluster may map to the tail of the final
> sector used by this compressed cluster.
>
> + The layout of the compressed data depends on the compression
> + type used for the image (see compressed cluster layout).
> +
> If a cluster is unallocated, read requests shall read the data from the backing
> file (except if bit 0 in the Standard Cluster Descriptor is set). If there is
> no backing file or the backing file is smaller than the image, they shall read
> zeros for all parts that are not covered by the backing file.
>
> +=== Compressed Cluster Layout ===
> +
> +The compressed cluster data has a layout depending on the compression
> +type used for the image, as follows:
> +
> +Compressed data layout for the available compression types:
> +(x = data_space_length - 1)
> +
> + 0: (default) zlib <http://zlib.net/>:
> + Byte 0 - x: the compressed data content
> + all the space provided used for compressed data
> + 1: zstd <http://github.com/facebook/zstd>:
> + Byte 0 - 3: the length of compressed data in bytes
> + 4 - x: the compressed data content
>
> == Snapshots ==
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 873fbef3b5..4b6e576c44 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4401,11 +4401,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: 5.0
> ##
> { 'enum': 'Qcow2CompressionType',
> - 'data': [ 'zlib' ] }
> + 'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
>
The spec and UI changes are okay.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
On 27.02.2020 17:01, Eric Blake wrote:
> On 2/27/20 1:29 AM, Denis Plotnikov wrote:
>> zstd significantly reduces cluster compression time.
>> It provides better compression performance maintaining
>> the same level of the compression ratio in comparison with
>> zlib, which, at the moment, is the only compression
>> method available.
>>
>
>> +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
>> + const void *src, size_t src_size)
>> +{
>> + size_t ret;
>> +
>> + /*
>> + * steal ZSTD_LEN_BUF bytes in the very beginng of the buffer
>
> beginning
>
>> + * to store compressed chunk size
>> + */
>> + char *d_buf = ((char *) dest) + ZSTD_LEN_BUF;
>> +
>> + /*
>> + * sanity check that we can store the compressed data length,
>> + * and there is some space left for the compressor buffer
>> + */
>> + if (dest_size <= ZSTD_LEN_BUF) {
>> + return -ENOMEM;
>> + }
>> +
>> + dest_size -= ZSTD_LEN_BUF;
>> +
>> + ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
>> +
>> + if (ZSTD_isError(ret)) {
>> + if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
>> + return -ENOMEM;
>> + } else {
>> + return -EIO;
>> + }
>> + }
>> +
>> + /* paraniod sanity check that we can store the commpressed size */
>
> paranoid, compressed
>
>> + if (ret > UINT_MAX) {
>> + return -ENOMEM;
>> + }
>
> This is pointless. Better is to ensure that we actually compressed
> data (the pigeonhole principle states that there are some inputs that
> MUST result in inflation, in order for most other inputs to result in
> compression). But that check was satisfied by checking for
> ZSTD_error_dstSize_tooSmall, which is what happens for one of those
> uncompressible inputs. Namely, zstd will never return a result larger
> than dest_size, and since dest_size is smaller than UINT_MAX on entry,
> this check is pointless. But if you want something, I'd be okay with:
> assert(ret <= dest_size).
Taking into account that this is "just in case" and I'm trying to
protect the first 4 bytes of the buffer from the overflow and
I can't imagine the situation when we deal with cluster sizes greater
than UINT32_MAX but the input size is size_t which can be > UINT32_MAX
on 64bit archs.
I'd rather stick with
if (ret > UINT32_MAX) {
return -ENOMEM;
}
as Vladimir suggested.
I'm not sure that the assert is good here, since it stops the system
operating and this isn't potential error but a limitation
Does it work for you?
Denis
>
>> +++ b/docs/interop/qcow2.txt
>> @@ -208,6 +208,7 @@ version 2.
>> Available compression type values:
>> 0: zlib <https://www.zlib.net/>
>> + 1: zstd <http://github.com/facebook/zstd>
>> === Header padding ===
>> @@ -575,11 +576,28 @@ Compressed Clusters Descriptor (x = 62 -
>> (cluster_bits - 8)):
>> Another compressed cluster may map to the tail
>> of the final
>> sector used by this compressed cluster.
>> + The layout of the compressed data depends on
>> the compression
>> + type used for the image (see compressed cluster
>> layout).
>> +
>> If a cluster is unallocated, read requests shall read the data from
>> the backing
>> file (except if bit 0 in the Standard Cluster Descriptor is set).
>> If there is
>> no backing file or the backing file is smaller than the image, they
>> shall read
>> zeros for all parts that are not covered by the backing file.
>> +=== Compressed Cluster Layout ===
>> +
>> +The compressed cluster data has a layout depending on the compression
>> +type used for the image, as follows:
>> +
>> +Compressed data layout for the available compression types:
>> +(x = data_space_length - 1)
>> +
>> + 0: (default) zlib <http://zlib.net/>:
>> + Byte 0 - x: the compressed data content
>> + all the space provided used for
>> compressed data
>> + 1: zstd <http://github.com/facebook/zstd>:
>> + Byte 0 - 3: the length of compressed data in bytes
>> + 4 - x: the compressed data content
>> == Snapshots ==
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 873fbef3b5..4b6e576c44 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -4401,11 +4401,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: 5.0
>> ##
>> { 'enum': 'Qcow2CompressionType',
>> - 'data': [ 'zlib' ] }
>> + 'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' }
>> ] }
>
> The spec and UI changes are okay.
>
27.02.2020 10:29, Denis Plotnikov wrote:
> zstd significantly reduces cluster compression time.
> It provides better compression performance maintaining
> the same level of the compression ratio in comparison with
> zlib, which, at the moment, is 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-threads.c | 122 +++++++++++++++++++++++++++++++++++++++++
> block/qcow2.c | 7 +++
> configure | 29 ++++++++++
> docs/interop/qcow2.txt | 18 ++++++
> qapi/block-core.json | 3 +-
> 5 files changed, 178 insertions(+), 1 deletion(-)
>
> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
> index 1c128e9840..e942c4d7e5 100644
> --- a/block/qcow2-threads.c
> +++ b/block/qcow2-threads.c
> @@ -28,6 +28,11 @@
> #define ZLIB_CONST
> #include <zlib.h>
>
> +#ifdef CONFIG_ZSTD
> +#include <zstd.h>
> +#include <zstd_errors.h>
> +#endif
> +
> #include "qcow2.h"
> #include "block/thread-pool.h"
> #include "crypto.h"
> @@ -164,6 +169,113 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
> return ret;
> }
>
> +#ifdef CONFIG_ZSTD
> +
> +#define ZSTD_LEN_BUF 4
> +
> +/*
> + * 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
This doesn't match qcow2_co_compress definition. You should return 0 on success.
> + * -ENOMEM destination buffer is not enough to store compressed data
> + * -EIO on any other error
> + */
> +
> +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
> + const void *src, size_t src_size)
> +{
> + size_t ret;
> +
> + /*
> + * steal ZSTD_LEN_BUF bytes in the very beginng of the buffer
beginning
> + * to store compressed chunk size
> + */
> + char *d_buf = ((char *) dest) + ZSTD_LEN_BUF;
> +
> + /*
> + * sanity check that we can store the compressed data length,
> + * and there is some space left for the compressor buffer
> + */
> + if (dest_size <= ZSTD_LEN_BUF) {
> + return -ENOMEM;
> + }
> +
> + dest_size -= ZSTD_LEN_BUF;
> +
> + ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
> +
> + if (ZSTD_isError(ret)) {
> + if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
> + return -ENOMEM;
> + } else {
> + return -EIO;
> + }
> + }
> +
> + /* paraniod sanity check that we can store the commpressed size */
> + if (ret > UINT_MAX) {
> + return -ENOMEM;
> + }
I'd use UINT32_MAX, possibly even more paranoid)
> +
> + /* store the compressed chunk size in the very beginning of the buffer */
> + stl_be_p(dest, ret);
> +
> + return ret + ZSTD_LEN_BUF;
return 0;
> +}
> +
> +/*
> + * 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 any error
> + */
> +
extra empty line.
> +static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size,
> + const void *src, size_t src_size)
> +{
> + /*
> + * zstd decompress wants to know the exact length of the data.
> + * For that purpose, on compression, the length is stored in
> + * the very beginning of the compressed buffer
> + */
> + size_t s_size;
> + const char *s_buf = ((const char *) src) + ZSTD_LEN_BUF;
> +
> + /*
> + * sanity check that we can read 4 byte the content length and
> + * and there is some content to decompress
> + */
> + if (src_size <= ZSTD_LEN_BUF) {
> + return -EIO;
> + }
> +
> + s_size = ldl_be_p(src);
> +
> + /* sanity check that the buffer is big enough to read the content from */
> + if (src_size - ZSTD_LEN_BUF < s_size) {
> + return -EIO;
> + }
> +
> + if (ZSTD_isError(
> + ZSTD_decompress(dest, dest_size, s_buf, s_size))) {
hmm, it fit into one line actually
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +#endif
> +
> static int qcow2_compress_pool_func(void *opaque)
> {
> Qcow2CompressData *data = opaque;
> @@ -215,6 +327,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;
> }
> @@ -247,6 +364,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/block/qcow2.c b/block/qcow2.c
> index 2ccb2cabd1..9c8ad9d580 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1257,6 +1257,9 @@ static int validate_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:
> @@ -3506,6 +3509,10 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
> }
>
> switch (qcow2_opts->compression_type) {
> +#ifdef CONFIG_ZSTD
> + case QCOW2_COMPRESSION_TYPE_ZSTD:
> + break;
> +#endif
> default:
> error_setg_errno(errp, -EINVAL, "Unknown compression type");
> goto out;
> diff --git a/configure b/configure
> index 48d6f89d57..4690a7ea9f 100755
> --- a/configure
> +++ b/configure
> @@ -444,6 +444,7 @@ opengl_dmabuf="no"
> cpuid_h="no"
> avx2_opt=""
> zlib="yes"
> +zstd=""
> capstone=""
> lzo=""
> snappy=""
> @@ -1371,6 +1372,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"
> @@ -1829,6 +1834,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
> @@ -2453,6 +2459,25 @@ EOF
> fi
> fi
>
> +#########################################
> +# zstd check
> +
> +if test "$zstd" != "no" ; then
> + cat > $TMPC << EOF
> +#include <zstd.h>
> +int main(void) { ZSTD_versionNumber(); return 0; }
> +EOF
> + if compile_prog "" "-lzstd" ; then
> + LIBS="$LIBS -lzstd"
> + zstd="yes"
> + else
> + if test "$zstd" = "yes"; then
> + feature_not_found "zstd" "Install libzstd-devel"
to correspond to style used around: s/libzstd-devel/libzstd devel/
> + fi
> + zstd="no"
> + fi
> +fi
> +
> ##########################################
> # libseccomp check
>
> @@ -6668,6 +6693,7 @@ echo "lzo support $lzo"
> echo "snappy support $snappy"
> echo "bzip2 support $bzip2"
> echo "lzfse support $lzfse"
> +echo "zstd support $zstd"
> echo "NUMA host support $numa"
> echo "libxml2 $libxml2"
> echo "tcmalloc support $tcmalloc"
> @@ -7559,6 +7585,9 @@ if test "$plugins" = "yes" ; then
> "\$ld_exported_symbols_list should have been set to 'yes'."
> fi
> 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/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index 5597e24474..aeca2ddebd 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -208,6 +208,7 @@ version 2.
>
> Available compression type values:
> 0: zlib <https://www.zlib.net/>
> + 1: zstd <http://github.com/facebook/zstd>
>
>
> === Header padding ===
> @@ -575,11 +576,28 @@ Compressed Clusters Descriptor (x = 62 - (cluster_bits - 8)):
> Another compressed cluster may map to the tail of the final
> sector used by this compressed cluster.
>
> + The layout of the compressed data depends on the compression
> + type used for the image (see compressed cluster layout).
> +
> If a cluster is unallocated, read requests shall read the data from the backing
> file (except if bit 0 in the Standard Cluster Descriptor is set). If there is
> no backing file or the backing file is smaller than the image, they shall read
> zeros for all parts that are not covered by the backing file.
>
> +=== Compressed Cluster Layout ===
> +
> +The compressed cluster data has a layout depending on the compression
> +type used for the image, as follows:
> +
> +Compressed data layout for the available compression types:
> +(x = data_space_length - 1)
Note, that term 'data_space_lenght' is not defined in the spec..
Hmm. And it's not trivial to define it correctly, all we have is offset and
"number of additional sectors", and may be not all bytes of final sector are
occupied... What about something like this:
In the following layout description byte 0 corresponds to byte at host cluster offset,
as defined by "Compressed Clusters Descriptor" paragraph above. The whole layout occupy
space starting from this offset, using additional 512-byte sectors defined by
"Compressed Clusters Descriptor" paragraph, not necessarily occupying all of the bytes
in the final sector. Let x be number of last byte of the layout.
> +
> + 0: (default) zlib <http://zlib.net/>:
> + Byte 0 - x: the compressed data content
> + all the space provided used for compressed data
> + 1: zstd <http://github.com/facebook/zstd>:
> + Byte 0 - 3: the length of compressed data in bytes
> + 4 - x: the compressed data content
Maybe, note that x+! == 4 + length, where length is value of first field of the layout.
>
> == Snapshots ==
>
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 873fbef3b5..4b6e576c44 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4401,11 +4401,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: 5.0
> ##
> { 'enum': 'Qcow2CompressionType',
> - 'data': [ 'zlib' ] }
> + 'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
>
> ##
> # @BlockdevCreateOptionsQcow2:
>
--
Best regards,
Vladimir
On 27.02.2020 12:55, Vladimir Sementsov-Ogievskiy wrote:
> 27.02.2020 10:29, Denis Plotnikov wrote:
>> zstd significantly reduces cluster compression time.
>> It provides better compression performance maintaining
>> the same level of the compression ratio in comparison with
>> zlib, which, at the moment, is 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-threads.c | 122 +++++++++++++++++++++++++++++++++++++++++
>> block/qcow2.c | 7 +++
>> configure | 29 ++++++++++
>> docs/interop/qcow2.txt | 18 ++++++
>> qapi/block-core.json | 3 +-
>> 5 files changed, 178 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
>> index 1c128e9840..e942c4d7e5 100644
>> --- a/block/qcow2-threads.c
>> +++ b/block/qcow2-threads.c
>> @@ -28,6 +28,11 @@
>> #define ZLIB_CONST
>> #include <zlib.h>
>> +#ifdef CONFIG_ZSTD
>> +#include <zstd.h>
>> +#include <zstd_errors.h>
>> +#endif
>> +
>> #include "qcow2.h"
>> #include "block/thread-pool.h"
>> #include "crypto.h"
>> @@ -164,6 +169,113 @@ static ssize_t qcow2_zlib_decompress(void
>> *dest, size_t dest_size,
>> return ret;
>> }
>> +#ifdef CONFIG_ZSTD
>> +
>> +#define ZSTD_LEN_BUF 4
>> +
>> +/*
>> + * 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
>
> This doesn't match qcow2_co_compress definition. You should return 0
> on success.
does it? I'd rather say it doesn't match to qcow2_co_compress
description in the function comment, which we can change actually,
because qcow2_co_compress is used like:
block/qcow2.c:
static coroutine_fn int
qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
uint64_t offset, uint64_t bytes,
QEMUIOVector *qiov, size_t qiov_offset)
{
...
out_buf = g_malloc(s->cluster_size);
out_len = qcow2_co_compress(bs, out_buf, s->cluster_size - 1,
buf, s->cluster_size);
if (out_len == -ENOMEM) {
/* could not compress: write normal cluster */
ret = qcow2_co_pwritev_part(bs, offset, bytes, qiov,
qiov_offset, 0);
if (ret < 0) {
goto fail;
}
goto success;
} else if (out_len < 0) {
ret = -EINVAL;
goto fail;
}
qemu_co_mutex_lock(&s->lock);
ret = qcow2_alloc_compressed_cluster_offset(bs, offset, out_len,
<<<<<<<<<<<<
&cluster_offset);
...
}
>
>> + * -ENOMEM destination buffer is not enough to store
>> compressed data
>> + * -EIO on any other error
>> + */
>> +
>> +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
>> + const void *src, size_t src_size)
>> +{
>> + size_t ret;
>> +
>> + /*
>> + * steal ZSTD_LEN_BUF bytes in the very beginng of the buffer
>
> beginning
>
>> + * to store compressed chunk size
>> + */
>> + char *d_buf = ((char *) dest) + ZSTD_LEN_BUF;
>> +
>> + /*
>> + * sanity check that we can store the compressed data length,
>> + * and there is some space left for the compressor buffer
>> + */
>> + if (dest_size <= ZSTD_LEN_BUF) {
>> + return -ENOMEM;
>> + }
>> +
>> + dest_size -= ZSTD_LEN_BUF;
>> +
>> + ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
>> +
>> + if (ZSTD_isError(ret)) {
>> + if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
>> + return -ENOMEM;
>> + } else {
>> + return -EIO;
>> + }
>> + }
>> +
>> + /* paraniod sanity check that we can store the commpressed size */
>> + if (ret > UINT_MAX) {
>> + return -ENOMEM;
>> + }
>
> I'd use UINT32_MAX, possibly even more paranoid)
ok
>
>
>> +
>> + /* store the compressed chunk size in the very beginning of the
>> buffer */
>> + stl_be_p(dest, ret);
>> +
>> + return ret + ZSTD_LEN_BUF;
>
> return 0;
>
>> +}
>> +
>> +/*
>> + * 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 any error
>> + */
>> +
>
> extra empty line.
will be removed
>
>
>> +static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size,
>> + const void *src, size_t src_size)
>> +{
>> + /*
>> + * zstd decompress wants to know the exact length of the data.
>> + * For that purpose, on compression, the length is stored in
>> + * the very beginning of the compressed buffer
>> + */
>> + size_t s_size;
>> + const char *s_buf = ((const char *) src) + ZSTD_LEN_BUF;
>> +
>> + /*
>> + * sanity check that we can read 4 byte the content length and
>> + * and there is some content to decompress
>> + */
>> + if (src_size <= ZSTD_LEN_BUF) {
>> + return -EIO;
>> + }
>> +
>> + s_size = ldl_be_p(src);
>> +
>> + /* sanity check that the buffer is big enough to read the
>> content from */
>> + if (src_size - ZSTD_LEN_BUF < s_size) {
>> + return -EIO;
>> + }
>> +
>> + if (ZSTD_isError(
>> + ZSTD_decompress(dest, dest_size, s_buf, s_size))) {
>
> hmm, it fit into one line actually
will be fixed if so
>
>
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +#endif
>> +
>> static int qcow2_compress_pool_func(void *opaque)
>> {
>> Qcow2CompressData *data = opaque;
>> @@ -215,6 +327,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;
>> }
>> @@ -247,6 +364,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/block/qcow2.c b/block/qcow2.c
>> index 2ccb2cabd1..9c8ad9d580 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1257,6 +1257,9 @@ static int
>> validate_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:
>> @@ -3506,6 +3509,10 @@ qcow2_co_create(BlockdevCreateOptions
>> *create_options, Error **errp)
>> }
>> switch (qcow2_opts->compression_type) {
>> +#ifdef CONFIG_ZSTD
>> + case QCOW2_COMPRESSION_TYPE_ZSTD:
>> + break;
>> +#endif
>> default:
>> error_setg_errno(errp, -EINVAL, "Unknown compression
>> type");
>> goto out;
>> diff --git a/configure b/configure
>> index 48d6f89d57..4690a7ea9f 100755
>> --- a/configure
>> +++ b/configure
>> @@ -444,6 +444,7 @@ opengl_dmabuf="no"
>> cpuid_h="no"
>> avx2_opt=""
>> zlib="yes"
>> +zstd=""
>> capstone=""
>> lzo=""
>> snappy=""
>> @@ -1371,6 +1372,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"
>> @@ -1829,6 +1834,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
>> @@ -2453,6 +2459,25 @@ EOF
>> fi
>> fi
>> +#########################################
>> +# zstd check
>> +
>> +if test "$zstd" != "no" ; then
>> + cat > $TMPC << EOF
>> +#include <zstd.h>
>> +int main(void) { ZSTD_versionNumber(); return 0; }
>> +EOF
>> + if compile_prog "" "-lzstd" ; then
>> + LIBS="$LIBS -lzstd"
>> + zstd="yes"
>> + else
>> + if test "$zstd" = "yes"; then
>> + feature_not_found "zstd" "Install libzstd-devel"
>
> to correspond to style used around: s/libzstd-devel/libzstd devel/
My intention was to give a package name for a user encountered this
message to copy-paste it to "yum install".
If this doesn't make things easier, because the package name can be
changed in the future (although I doubt) I'll fix it.
If the style isn't that important I'd rather leave it as is.
>
>> + fi
>> + zstd="no"
>> + fi
>> +fi
>> +
>> ##########################################
>> # libseccomp check
>> @@ -6668,6 +6693,7 @@ echo "lzo support $lzo"
>> echo "snappy support $snappy"
>> echo "bzip2 support $bzip2"
>> echo "lzfse support $lzfse"
>> +echo "zstd support $zstd"
>> echo "NUMA host support $numa"
>> echo "libxml2 $libxml2"
>> echo "tcmalloc support $tcmalloc"
>> @@ -7559,6 +7585,9 @@ if test "$plugins" = "yes" ; then
>> "\$ld_exported_symbols_list should have been set to 'yes'."
>> fi
>> 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/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index 5597e24474..aeca2ddebd 100644
>> --- a/docs/interop/qcow2.txt
>> +++ b/docs/interop/qcow2.txt
>> @@ -208,6 +208,7 @@ version 2.
>> Available compression type values:
>> 0: zlib <https://www.zlib.net/>
>> + 1: zstd <http://github.com/facebook/zstd>
>> === Header padding ===
>> @@ -575,11 +576,28 @@ Compressed Clusters Descriptor (x = 62 -
>> (cluster_bits - 8)):
>> Another compressed cluster may map to the tail
>> of the final
>> sector used by this compressed cluster.
>> + The layout of the compressed data depends on
>> the compression
>> + type used for the image (see compressed cluster
>> layout).
>> +
>> If a cluster is unallocated, read requests shall read the data from
>> the backing
>> file (except if bit 0 in the Standard Cluster Descriptor is set).
>> If there is
>> no backing file or the backing file is smaller than the image, they
>> shall read
>> zeros for all parts that are not covered by the backing file.
>> +=== Compressed Cluster Layout ===
>> +
>> +The compressed cluster data has a layout depending on the compression
>> +type used for the image, as follows:
>> +
>> +Compressed data layout for the available compression types:
>> +(x = data_space_length - 1)
>
> Note, that term 'data_space_lenght' is not defined in the spec..
>
> Hmm. And it's not trivial to define it correctly, all we have is
> offset and
> "number of additional sectors", and may be not all bytes of final
> sector are
> occupied... What about something like this:
>
> In the following layout description byte 0 corresponds to byte at host
> cluster offset,
> as defined by "Compressed Clusters Descriptor" paragraph above. The
> whole layout occupy
> space starting from this offset, using additional 512-byte sectors
> defined by
> "Compressed Clusters Descriptor" paragraph, not necessarily occupying
> all of the bytes
> in the final sector. Let x be number of last byte of the layout.
Sounds too complicated for me, may be
x - data chunk length available to store a compressed cluster. (for more
details see "Compressed Clusters Descriptor")
>
>
>> +
>> + 0: (default) zlib <http://zlib.net/>:
>> + Byte 0 - x: the compressed data content
>> + all the space provided used for
>> compressed data
>> + 1: zstd <http://github.com/facebook/zstd>:
>> + Byte 0 - 3: the length of compressed data in bytes
>> + 4 - x: the compressed data content
>
> Maybe, note that x+! == 4 + length, where length is value of first
> field of the layout.
>
>> == Snapshots ==
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 873fbef3b5..4b6e576c44 100644
>> --- a/qapi/block-core.json
>> +++ b/qapi/block-core.json
>> @@ -4401,11 +4401,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: 5.0
>> ##
>> { 'enum': 'Qcow2CompressionType',
>> - 'data': [ 'zlib' ] }
>> + 'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' }
>> ] }
>> ##
>> # @BlockdevCreateOptionsQcow2:
>>
>
>
27.02.2020 17:11, Denis Plotnikov wrote:
>
>
> On 27.02.2020 12:55, Vladimir Sementsov-Ogievskiy wrote:
>> 27.02.2020 10:29, Denis Plotnikov wrote:
>>> zstd significantly reduces cluster compression time.
>>> It provides better compression performance maintaining
>>> the same level of the compression ratio in comparison with
>>> zlib, which, at the moment, is 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-threads.c | 122 +++++++++++++++++++++++++++++++++++++++++
>>> block/qcow2.c | 7 +++
>>> configure | 29 ++++++++++
>>> docs/interop/qcow2.txt | 18 ++++++
>>> qapi/block-core.json | 3 +-
>>> 5 files changed, 178 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
>>> index 1c128e9840..e942c4d7e5 100644
>>> --- a/block/qcow2-threads.c
>>> +++ b/block/qcow2-threads.c
>>> @@ -28,6 +28,11 @@
>>> #define ZLIB_CONST
>>> #include <zlib.h>
>>> +#ifdef CONFIG_ZSTD
>>> +#include <zstd.h>
>>> +#include <zstd_errors.h>
>>> +#endif
>>> +
>>> #include "qcow2.h"
>>> #include "block/thread-pool.h"
>>> #include "crypto.h"
>>> @@ -164,6 +169,113 @@ static ssize_t qcow2_zlib_decompress(void *dest, size_t dest_size,
>>> return ret;
>>> }
>>> +#ifdef CONFIG_ZSTD
>>> +
>>> +#define ZSTD_LEN_BUF 4
>>> +
>>> +/*
>>> + * 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
>>
>> This doesn't match qcow2_co_compress definition. You should return 0 on success.
> does it? I'd rather say it doesn't match to qcow2_co_compress description in the function comment, which we can change actually,
> because qcow2_co_compress is used like:
Oh, yes, you are right. Then we should change the comment.
>
> block/qcow2.c:
>
> static coroutine_fn int
> qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
> uint64_t offset, uint64_t bytes,
> QEMUIOVector *qiov, size_t qiov_offset)
> {
> ...
> out_buf = g_malloc(s->cluster_size);
>
> out_len = qcow2_co_compress(bs, out_buf, s->cluster_size - 1,
> buf, s->cluster_size);
> if (out_len == -ENOMEM) {
> /* could not compress: write normal cluster */
> ret = qcow2_co_pwritev_part(bs, offset, bytes, qiov, qiov_offset, 0);
> if (ret < 0) {
> goto fail;
> }
> goto success;
> } else if (out_len < 0) {
> ret = -EINVAL;
> goto fail;
> }
>
> qemu_co_mutex_lock(&s->lock);
> ret = qcow2_alloc_compressed_cluster_offset(bs, offset, out_len, <<<<<<<<<<<<
> &cluster_offset);
> ...
> }
>
>>
>>> + * -ENOMEM destination buffer is not enough to store compressed data
>>> + * -EIO on any other error
>>> + */
>>> +
>>> +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
>>> + const void *src, size_t src_size)
>>> +{
>>> + size_t ret;
>>> +
>>> + /*
>>> + * steal ZSTD_LEN_BUF bytes in the very beginng of the buffer
>>
>> beginning
>>
>>> + * to store compressed chunk size
>>> + */
>>> + char *d_buf = ((char *) dest) + ZSTD_LEN_BUF;
>>> +
>>> + /*
>>> + * sanity check that we can store the compressed data length,
>>> + * and there is some space left for the compressor buffer
>>> + */
>>> + if (dest_size <= ZSTD_LEN_BUF) {
>>> + return -ENOMEM;
>>> + }
>>> +
>>> + dest_size -= ZSTD_LEN_BUF;
>>> +
>>> + ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
>>> +
>>> + if (ZSTD_isError(ret)) {
>>> + if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
>>> + return -ENOMEM;
>>> + } else {
>>> + return -EIO;
>>> + }
>>> + }
>>> +
>>> + /* paraniod sanity check that we can store the commpressed size */
>>> + if (ret > UINT_MAX) {
>>> + return -ENOMEM;
>>> + }
>>
>> I'd use UINT32_MAX, possibly even more paranoid)
> ok
>>
>>
>>> +
>>> + /* store the compressed chunk size in the very beginning of the buffer */
>>> + stl_be_p(dest, ret);
>>> +
>>> + return ret + ZSTD_LEN_BUF;
>>
>> return 0;
>>
>>> +}
>>> +
>>> +/*
>>> + * 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 any error
>>> + */
>>> +
>>
>> extra empty line.
> will be removed
>>
>>
>>> +static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size,
>>> + const void *src, size_t src_size)
>>> +{
>>> + /*
>>> + * zstd decompress wants to know the exact length of the data.
>>> + * For that purpose, on compression, the length is stored in
>>> + * the very beginning of the compressed buffer
>>> + */
>>> + size_t s_size;
>>> + const char *s_buf = ((const char *) src) + ZSTD_LEN_BUF;
>>> +
>>> + /*
>>> + * sanity check that we can read 4 byte the content length and
>>> + * and there is some content to decompress
>>> + */
>>> + if (src_size <= ZSTD_LEN_BUF) {
>>> + return -EIO;
>>> + }
>>> +
>>> + s_size = ldl_be_p(src);
>>> +
>>> + /* sanity check that the buffer is big enough to read the content from */
>>> + if (src_size - ZSTD_LEN_BUF < s_size) {
>>> + return -EIO;
>>> + }
>>> +
>>> + if (ZSTD_isError(
>>> + ZSTD_decompress(dest, dest_size, s_buf, s_size))) {
>>
>> hmm, it fit into one line actually
> will be fixed if so
>>
>>
>>> + return -EIO;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +#endif
>>> +
>>> static int qcow2_compress_pool_func(void *opaque)
>>> {
>>> Qcow2CompressData *data = opaque;
>>> @@ -215,6 +327,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;
>>> }
>>> @@ -247,6 +364,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/block/qcow2.c b/block/qcow2.c
>>> index 2ccb2cabd1..9c8ad9d580 100644
>>> --- a/block/qcow2.c
>>> +++ b/block/qcow2.c
>>> @@ -1257,6 +1257,9 @@ static int validate_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:
>>> @@ -3506,6 +3509,10 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
>>> }
>>> switch (qcow2_opts->compression_type) {
>>> +#ifdef CONFIG_ZSTD
>>> + case QCOW2_COMPRESSION_TYPE_ZSTD:
>>> + break;
>>> +#endif
>>> default:
>>> error_setg_errno(errp, -EINVAL, "Unknown compression type");
>>> goto out;
>>> diff --git a/configure b/configure
>>> index 48d6f89d57..4690a7ea9f 100755
>>> --- a/configure
>>> +++ b/configure
>>> @@ -444,6 +444,7 @@ opengl_dmabuf="no"
>>> cpuid_h="no"
>>> avx2_opt=""
>>> zlib="yes"
>>> +zstd=""
>>> capstone=""
>>> lzo=""
>>> snappy=""
>>> @@ -1371,6 +1372,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"
>>> @@ -1829,6 +1834,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
>>> @@ -2453,6 +2459,25 @@ EOF
>>> fi
>>> fi
>>> +#########################################
>>> +# zstd check
>>> +
>>> +if test "$zstd" != "no" ; then
>>> + cat > $TMPC << EOF
>>> +#include <zstd.h>
>>> +int main(void) { ZSTD_versionNumber(); return 0; }
>>> +EOF
>>> + if compile_prog "" "-lzstd" ; then
>>> + LIBS="$LIBS -lzstd"
>>> + zstd="yes"
>>> + else
>>> + if test "$zstd" = "yes"; then
>>> + feature_not_found "zstd" "Install libzstd-devel"
>>
>> to correspond to style used around: s/libzstd-devel/libzstd devel/
> My intention was to give a package name for a user encountered this message to copy-paste it to "yum install".
What about apt users? :)
> If this doesn't make things easier, because the package name can be changed in the future (although I doubt) I'll fix it.
> If the style isn't that important I'd rather leave it as is.
I don't care, keep it as is.
>>
>>> + fi
>>> + zstd="no"
>>> + fi
>>> +fi
>>> +
>>> ##########################################
>>> # libseccomp check
>>> @@ -6668,6 +6693,7 @@ echo "lzo support $lzo"
>>> echo "snappy support $snappy"
>>> echo "bzip2 support $bzip2"
>>> echo "lzfse support $lzfse"
>>> +echo "zstd support $zstd"
>>> echo "NUMA host support $numa"
>>> echo "libxml2 $libxml2"
>>> echo "tcmalloc support $tcmalloc"
>>> @@ -7559,6 +7585,9 @@ if test "$plugins" = "yes" ; then
>>> "\$ld_exported_symbols_list should have been set to 'yes'."
>>> fi
>>> 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/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>>> index 5597e24474..aeca2ddebd 100644
>>> --- a/docs/interop/qcow2.txt
>>> +++ b/docs/interop/qcow2.txt
>>> @@ -208,6 +208,7 @@ version 2.
>>> Available compression type values:
>>> 0: zlib <https://www.zlib.net/>
>>> + 1: zstd <http://github.com/facebook/zstd>
>>> === Header padding ===
>>> @@ -575,11 +576,28 @@ Compressed Clusters Descriptor (x = 62 - (cluster_bits - 8)):
>>> Another compressed cluster may map to the tail of the final
>>> sector used by this compressed cluster.
>>> + The layout of the compressed data depends on the compression
>>> + type used for the image (see compressed cluster layout).
>>> +
>>> If a cluster is unallocated, read requests shall read the data from the backing
>>> file (except if bit 0 in the Standard Cluster Descriptor is set). If there is
>>> no backing file or the backing file is smaller than the image, they shall read
>>> zeros for all parts that are not covered by the backing file.
>>> +=== Compressed Cluster Layout ===
>>> +
>>> +The compressed cluster data has a layout depending on the compression
>>> +type used for the image, as follows:
>>> +
>>> +Compressed data layout for the available compression types:
>>> +(x = data_space_length - 1)
>>
>> Note, that term 'data_space_lenght' is not defined in the spec..
>>
>> Hmm. And it's not trivial to define it correctly, all we have is offset and
>> "number of additional sectors", and may be not all bytes of final sector are
>> occupied... What about something like this:
>>
>> In the following layout description byte 0 corresponds to byte at host cluster offset,
>> as defined by "Compressed Clusters Descriptor" paragraph above. The whole layout occupy
>> space starting from this offset, using additional 512-byte sectors defined by
>> "Compressed Clusters Descriptor" paragraph, not necessarily occupying all of the bytes
>> in the final sector. Let x be number of last byte of the layout.
> Sounds too complicated for me, may be
> x - data chunk length available to store a compressed cluster. (for more details see "Compressed Clusters Descriptor")
Ok
>>
>>
>>> +
>>> + 0: (default) zlib <http://zlib.net/>:
>>> + Byte 0 - x: the compressed data content
>>> + all the space provided used for compressed data
>>> + 1: zstd <http://github.com/facebook/zstd>:
>>> + Byte 0 - 3: the length of compressed data in bytes
>>> + 4 - x: the compressed data content
>>
>> Maybe, note that x+! == 4 + length, where length is value of first field of the layout.
>>
>>> == Snapshots ==
>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>> index 873fbef3b5..4b6e576c44 100644
>>> --- a/qapi/block-core.json
>>> +++ b/qapi/block-core.json
>>> @@ -4401,11 +4401,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: 5.0
>>> ##
>>> { 'enum': 'Qcow2CompressionType',
>>> - 'data': [ 'zlib' ] }
>>> + 'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
>>> ##
>>> # @BlockdevCreateOptionsQcow2:
>>>
>>
>>
>
--
Best regards,
Vladimir
On 27.02.2020 17:18, Vladimir Sementsov-Ogievskiy wrote:
> 27.02.2020 17:11, Denis Plotnikov wrote:
>>
>>
>> On 27.02.2020 12:55, Vladimir Sementsov-Ogievskiy wrote:
>>> 27.02.2020 10:29, Denis Plotnikov wrote:
>>>> zstd significantly reduces cluster compression time.
>>>> It provides better compression performance maintaining
>>>> the same level of the compression ratio in comparison with
>>>> zlib, which, at the moment, is 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-threads.c | 122
>>>> +++++++++++++++++++++++++++++++++++++++++
>>>> block/qcow2.c | 7 +++
>>>> configure | 29 ++++++++++
>>>> docs/interop/qcow2.txt | 18 ++++++
>>>> qapi/block-core.json | 3 +-
>>>> 5 files changed, 178 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/block/qcow2-threads.c b/block/qcow2-threads.c
>>>> index 1c128e9840..e942c4d7e5 100644
>>>> --- a/block/qcow2-threads.c
>>>> +++ b/block/qcow2-threads.c
>>>> @@ -28,6 +28,11 @@
>>>> #define ZLIB_CONST
>>>> #include <zlib.h>
>>>> +#ifdef CONFIG_ZSTD
>>>> +#include <zstd.h>
>>>> +#include <zstd_errors.h>
>>>> +#endif
>>>> +
>>>> #include "qcow2.h"
>>>> #include "block/thread-pool.h"
>>>> #include "crypto.h"
>>>> @@ -164,6 +169,113 @@ static ssize_t qcow2_zlib_decompress(void
>>>> *dest, size_t dest_size,
>>>> return ret;
>>>> }
>>>> +#ifdef CONFIG_ZSTD
>>>> +
>>>> +#define ZSTD_LEN_BUF 4
>>>> +
>>>> +/*
>>>> + * 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
>>>
>>> This doesn't match qcow2_co_compress definition. You should return 0
>>> on success.
>> does it? I'd rather say it doesn't match to qcow2_co_compress
>> description in the function comment, which we can change actually,
>> because qcow2_co_compress is used like:
>
> Oh, yes, you are right. Then we should change the comment.
>
>>
>> block/qcow2.c:
>>
>> static coroutine_fn int
>> qcow2_co_pwritev_compressed_task(BlockDriverState *bs,
>> uint64_t offset, uint64_t bytes,
>> QEMUIOVector *qiov, size_t
>> qiov_offset)
>> {
>> ...
>> out_buf = g_malloc(s->cluster_size);
>>
>> out_len = qcow2_co_compress(bs, out_buf, s->cluster_size - 1,
>> buf, s->cluster_size);
>> if (out_len == -ENOMEM) {
>> /* could not compress: write normal cluster */
>> ret = qcow2_co_pwritev_part(bs, offset, bytes, qiov,
>> qiov_offset, 0);
>> if (ret < 0) {
>> goto fail;
>> }
>> goto success;
>> } else if (out_len < 0) {
>> ret = -EINVAL;
>> goto fail;
>> }
>>
>> qemu_co_mutex_lock(&s->lock);
>> ret = qcow2_alloc_compressed_cluster_offset(bs, offset, out_len,
>> <<<<<<<<<<<<
>> &cluster_offset);
>> ...
>> }
>>
>>>
>>>> + * -ENOMEM destination buffer is not enough to store
>>>> compressed data
>>>> + * -EIO on any other error
>>>> + */
>>>> +
>>>> +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
>>>> + const void *src, size_t src_size)
>>>> +{
>>>> + size_t ret;
>>>> +
>>>> + /*
>>>> + * steal ZSTD_LEN_BUF bytes in the very beginng of the buffer
>>>
>>> beginning
>>>
>>>> + * to store compressed chunk size
>>>> + */
>>>> + char *d_buf = ((char *) dest) + ZSTD_LEN_BUF;
>>>> +
>>>> + /*
>>>> + * sanity check that we can store the compressed data length,
>>>> + * and there is some space left for the compressor buffer
>>>> + */
>>>> + if (dest_size <= ZSTD_LEN_BUF) {
>>>> + return -ENOMEM;
>>>> + }
>>>> +
>>>> + dest_size -= ZSTD_LEN_BUF;
>>>> +
>>>> + ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);
>>>> +
>>>> + if (ZSTD_isError(ret)) {
>>>> + if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
>>>> + return -ENOMEM;
>>>> + } else {
>>>> + return -EIO;
>>>> + }
>>>> + }
>>>> +
>>>> + /* paraniod sanity check that we can store the commpressed
>>>> size */
>>>> + if (ret > UINT_MAX) {
>>>> + return -ENOMEM;
>>>> + }
>>>
>>> I'd use UINT32_MAX, possibly even more paranoid)
>> ok
>>>
>>>
>>>> +
>>>> + /* store the compressed chunk size in the very beginning of
>>>> the buffer */
>>>> + stl_be_p(dest, ret);
>>>> +
>>>> + return ret + ZSTD_LEN_BUF;
>>>
>>> return 0;
>>>
>>>> +}
>>>> +
>>>> +/*
>>>> + * 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 any error
>>>> + */
>>>> +
>>>
>>> extra empty line.
>> will be removed
>>>
>>>
>>>> +static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size,
>>>> + const void *src, size_t
>>>> src_size)
>>>> +{
>>>> + /*
>>>> + * zstd decompress wants to know the exact length of the data.
>>>> + * For that purpose, on compression, the length is stored in
>>>> + * the very beginning of the compressed buffer
>>>> + */
>>>> + size_t s_size;
>>>> + const char *s_buf = ((const char *) src) + ZSTD_LEN_BUF;
>>>> +
>>>> + /*
>>>> + * sanity check that we can read 4 byte the content length and
>>>> + * and there is some content to decompress
>>>> + */
>>>> + if (src_size <= ZSTD_LEN_BUF) {
>>>> + return -EIO;
>>>> + }
>>>> +
>>>> + s_size = ldl_be_p(src);
>>>> +
>>>> + /* sanity check that the buffer is big enough to read the
>>>> content from */
>>>> + if (src_size - ZSTD_LEN_BUF < s_size) {
>>>> + return -EIO;
>>>> + }
>>>> +
>>>> + if (ZSTD_isError(
>>>> + ZSTD_decompress(dest, dest_size, s_buf, s_size))) {
>>>
>>> hmm, it fit into one line actually
>> will be fixed if so
If there is no other objections I'd leave it as is since
if (ZSTD_isError(ZSTD_decompress(dest, dest_size, s_buf, s_size))) {
looks too messy to me although it fits into 80 chars
>>>
>>>
>>>> + return -EIO;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>> static int qcow2_compress_pool_func(void *opaque)
>>>> {
>>>> Qcow2CompressData *data = opaque;
>>>> @@ -215,6 +327,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;
>>>> }
>>>> @@ -247,6 +364,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/block/qcow2.c b/block/qcow2.c
>>>> index 2ccb2cabd1..9c8ad9d580 100644
>>>> --- a/block/qcow2.c
>>>> +++ b/block/qcow2.c
>>>> @@ -1257,6 +1257,9 @@ static int
>>>> validate_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:
>>>> @@ -3506,6 +3509,10 @@ qcow2_co_create(BlockdevCreateOptions
>>>> *create_options, Error **errp)
>>>> }
>>>> switch (qcow2_opts->compression_type) {
>>>> +#ifdef CONFIG_ZSTD
>>>> + case QCOW2_COMPRESSION_TYPE_ZSTD:
>>>> + break;
>>>> +#endif
>>>> default:
>>>> error_setg_errno(errp, -EINVAL, "Unknown compression
>>>> type");
>>>> goto out;
>>>> diff --git a/configure b/configure
>>>> index 48d6f89d57..4690a7ea9f 100755
>>>> --- a/configure
>>>> +++ b/configure
>>>> @@ -444,6 +444,7 @@ opengl_dmabuf="no"
>>>> cpuid_h="no"
>>>> avx2_opt=""
>>>> zlib="yes"
>>>> +zstd=""
>>>> capstone=""
>>>> lzo=""
>>>> snappy=""
>>>> @@ -1371,6 +1372,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"
>>>> @@ -1829,6 +1834,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
>>>> @@ -2453,6 +2459,25 @@ EOF
>>>> fi
>>>> fi
>>>> +#########################################
>>>> +# zstd check
>>>> +
>>>> +if test "$zstd" != "no" ; then
>>>> + cat > $TMPC << EOF
>>>> +#include <zstd.h>
>>>> +int main(void) { ZSTD_versionNumber(); return 0; }
>>>> +EOF
>>>> + if compile_prog "" "-lzstd" ; then
>>>> + LIBS="$LIBS -lzstd"
>>>> + zstd="yes"
>>>> + else
>>>> + if test "$zstd" = "yes"; then
>>>> + feature_not_found "zstd" "Install libzstd-devel"
>>>
>>> to correspond to style used around: s/libzstd-devel/libzstd devel/
>> My intention was to give a package name for a user encountered this
>> message to copy-paste it to "yum install".
>
> What about apt users? :)
>
>> If this doesn't make things easier, because the package name can be
>> changed in the future (although I doubt) I'll fix it.
>> If the style isn't that important I'd rather leave it as is.
>
> I don't care, keep it as is.
>
>>>
>>>> + fi
>>>> + zstd="no"
>>>> + fi
>>>> +fi
>>>> +
>>>> ##########################################
>>>> # libseccomp check
>>>> @@ -6668,6 +6693,7 @@ echo "lzo support $lzo"
>>>> echo "snappy support $snappy"
>>>> echo "bzip2 support $bzip2"
>>>> echo "lzfse support $lzfse"
>>>> +echo "zstd support $zstd"
>>>> echo "NUMA host support $numa"
>>>> echo "libxml2 $libxml2"
>>>> echo "tcmalloc support $tcmalloc"
>>>> @@ -7559,6 +7585,9 @@ if test "$plugins" = "yes" ; then
>>>> "\$ld_exported_symbols_list should have been set to 'yes'."
>>>> fi
>>>> 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/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>>>> index 5597e24474..aeca2ddebd 100644
>>>> --- a/docs/interop/qcow2.txt
>>>> +++ b/docs/interop/qcow2.txt
>>>> @@ -208,6 +208,7 @@ version 2.
>>>> Available compression type values:
>>>> 0: zlib <https://www.zlib.net/>
>>>> + 1: zstd <http://github.com/facebook/zstd>
>>>> === Header padding ===
>>>> @@ -575,11 +576,28 @@ Compressed Clusters Descriptor (x = 62 -
>>>> (cluster_bits - 8)):
>>>> Another compressed cluster may map to the
>>>> tail of the final
>>>> sector used by this compressed cluster.
>>>> + The layout of the compressed data depends on
>>>> the compression
>>>> + type used for the image (see compressed
>>>> cluster layout).
>>>> +
>>>> If a cluster is unallocated, read requests shall read the data
>>>> from the backing
>>>> file (except if bit 0 in the Standard Cluster Descriptor is set).
>>>> If there is
>>>> no backing file or the backing file is smaller than the image,
>>>> they shall read
>>>> zeros for all parts that are not covered by the backing file.
>>>> +=== Compressed Cluster Layout ===
>>>> +
>>>> +The compressed cluster data has a layout depending on the compression
>>>> +type used for the image, as follows:
>>>> +
>>>> +Compressed data layout for the available compression types:
>>>> +(x = data_space_length - 1)
>>>
>>> Note, that term 'data_space_lenght' is not defined in the spec..
>>>
>>> Hmm. And it's not trivial to define it correctly, all we have is
>>> offset and
>>> "number of additional sectors", and may be not all bytes of final
>>> sector are
>>> occupied... What about something like this:
>>>
>>> In the following layout description byte 0 corresponds to byte at
>>> host cluster offset,
>>> as defined by "Compressed Clusters Descriptor" paragraph above. The
>>> whole layout occupy
>>> space starting from this offset, using additional 512-byte sectors
>>> defined by
>>> "Compressed Clusters Descriptor" paragraph, not necessarily
>>> occupying all of the bytes
>>> in the final sector. Let x be number of last byte of the layout.
>> Sounds too complicated for me, may be
>> x - data chunk length available to store a compressed cluster. (for
>> more details see "Compressed Clusters Descriptor")
>
> Ok
>
>>>
>>>
>>>> +
>>>> + 0: (default) zlib <http://zlib.net/>:
>>>> + Byte 0 - x: the compressed data content
>>>> + all the space provided used for
>>>> compressed data
>>>> + 1: zstd <http://github.com/facebook/zstd>:
>>>> + Byte 0 - 3: the length of compressed data in bytes
>>>> + 4 - x: the compressed data content
>>>
>>> Maybe, note that x+! == 4 + length, where length is value of first
>>> field of the layout.
>>>
>>>> == Snapshots ==
>>>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>>>> index 873fbef3b5..4b6e576c44 100644
>>>> --- a/qapi/block-core.json
>>>> +++ b/qapi/block-core.json
>>>> @@ -4401,11 +4401,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: 5.0
>>>> ##
>>>> { 'enum': 'Qcow2CompressionType',
>>>> - 'data': [ 'zlib' ] }
>>>> + 'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)'
>>>> } ] }
>>>> ##
>>>> # @BlockdevCreateOptionsQcow2:
>>>>
>>>
>>>
>>
>
>
© 2016 - 2025 Red Hat, Inc.