On 9/4/19 10:29 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, at 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 >
>
> Signed-off-by: Denis Plotnikov <dplotni...@virtuozzo.com>
> ---
> +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;
Potentially unaligned pointer...
> + /* steal some bytes to store compressed chunk size */
> + char *d_buf = ((char *) dest) + sizeof(*c_size);
> +
> + /* sanity check that we can store the compressed data length */
> + 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 (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
> + return -ENOMEM;
> + } else {
> + return -EIO;
> + }
> + }
> +
> + /* store the compressed chunk size in the very beginning of the buffer */
> + *c_size = cpu_to_be32(ret);
...and you are storing into it. You are using the wrong conversion
function; you want stl_be_p(dest, ret) or similar.
> +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 length of the data
> + * for that purpose, on the compression the length is stored in
s/data for/data. For/
s/on the compression/on compression,/
> + * the very beginning of the compressed buffer
> + */
> + uint32_t s_size;
> + const char *s_buf = ((const char *) src) + sizeof(s_size);
> +
> + /* sanity check that we can read the content length */
> + if (src_size < sizeof(s_size)) {
Should this use <=? After all, I seriously doubt you can get a 0-byte
compression stream.
> + return -EIO;
> + }
> +
> + s_size = be32_to_cpu(*(const uint32_t *) src);
As written, this looks like you may be dereferencing an unaligned
pointer. It so happens that be32_to_cpu() applies & to your * to get
back at the raw pointer, and then is careful to handle unaligned
pointers, so it works; but it would look a lot nicer as merely:
s_size = be32_to_cpu(src);
> +
> + /* sanity check that the buffer is big enough to read the content */
> + if (src_size - sizeof(s_size) < s_size) {
Why < and not !=? As written, you are silently ignoring trailing
garbage, instead of treating it as a client that did not write the data
correctly.
> +++ b/docs/interop/qcow2.txt
> @@ -181,6 +181,7 @@ in the description of a field.
> must be set.
> Available compression type values:
> 0: zlib <https://www.zlib.net/> (default)
> + 1: zstd <http://github.com/facebook/zstd>
At this point, this listing is almost redundant with the more-detailed
header below. Maybe it is worth just forward referencing that section
for a listing of valid values, and then mentioning the values 0 and 1 in
that section?
>
> Directly after the image header, optional sections called header extensions
> can
> be stored. Each extension has a structure like the following:
> @@ -536,6 +537,9 @@ 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
> @@ -788,3 +792,19 @@ In the image file the 'enabled' state is reflected by
> the 'auto' flag. If this
> flag is set, the software must consider the bitmap as 'enabled' and start
> tracking virtual disk changes to this bitmap from the first write to the
> virtual disk. If this flag is not set then the bitmap is disabled.
> +
> +=== Compressed cluster layout ===
As written, you have made this a child to '== Bitmaps ==' (and sibling
to '=== Dirty tracking bitmaps ==='); that feels wrong. I would place
this subsection belong under '== Cluster mapping ==' right after the
'Compressed Clusters Descriptor'.
> +
> +The compressed cluster data may have a different layout depending on the
> +compression type used for the image, and store specific data for the
> particular
> +compression type.
> +
> +Compressed data layout for the available compression types:
Wordy; maybe:
The compressed cluster data has a layout depending on the compression
type used for the image, as follows:
> +(x = data_space_length - 1)
> +
> + zlib <http://zlib.net/>:
> + Byte 0 - x: the compressed data content
> + all the space provided used for compressed data
Worth a mention that this is compression type 0?
> + zstd <http://github.com/facebook/zstd>:
> + Byte 0 - 3: the length of compressed data in bytes
> + 4 - x: the compressed data content
Worth a mention that this is compression type 1?
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 2c002ca6a9..9e458d5b40 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -4283,11 +4283,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.2
> ##
> { 'enum': 'Qcow2CompressionType',
> - 'data': [ 'zlib' ] }
> + 'data': [ 'zlib', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
>
> ##
> # @BlockdevCreateOptionsQcow2:
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature
