All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v2 0/3] add zstd cluster compression
@ 2019-07-04 13:09 Denis Plotnikov
  2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature Denis Plotnikov
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Denis Plotnikov @ 2019-07-04 13:09 UTC (permalink / raw)
  To: kwolf, mreitz, eblake, armbru; +Cc: vsementsov, den, qemu-block, qemu-devel

change log:

v2:
    * relax the compression type setting restriction in the spec
    * fix qcow2 header size checking
    * fix error processing and messaging
    * fix qcow2 image specific info reporting
    * set Qcow2CompressionType zstd config dependant
    * add zstd compressed cluster format description to the spec

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             | 287 +++++++++++++++++++++++++++++++++++---
 block/qcow2.h             |  26 +++-
 configure                 |  32 +++++
 docs/interop/qcow2.txt    |  40 +++++-
 include/block/block_int.h |   1 +
 qapi/block-core.json      |  23 ++-
 6 files changed, 379 insertions(+), 30 deletions(-)

-- 
2.17.0



^ permalink raw reply	[flat|nested] 19+ messages in thread

* [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature
  2019-07-04 13:09 [Qemu-devel] [PATCH v2 0/3] add zstd cluster compression Denis Plotnikov
@ 2019-07-04 13:09 ` Denis Plotnikov
  2019-07-09  6:15   ` Markus Armbruster
                     ` (2 more replies)
  2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 2/3] qcow2: rework the cluster compression routine Denis Plotnikov
                   ` (2 subsequent siblings)
  3 siblings, 3 replies; 19+ messages in thread
From: Denis Plotnikov @ 2019-07-04 13:09 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 2x 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
are backward compatible with older qemu versions.

Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
---
 block/qcow2.c             | 95 +++++++++++++++++++++++++++++++++++++++
 block/qcow2.h             | 26 ++++++++---
 docs/interop/qcow2.txt    | 21 ++++++++-
 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..8fa932a349 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1202,6 +1202,32 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
     return ret;
 }
 
+static int check_compression_type(BDRVQcow2State *s, Error **errp)
+{
+    switch (s->compression_type) {
+    case QCOW2_COMPRESSION_TYPE_ZLIB:
+        break;
+
+    default:
+        error_setg(errp, "qcow2: unknown compression type: %u",
+                   s->compression_type);
+        return -ENOTSUP;
+    }
+
+    /*
+     * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
+     * the incompatible feature flag must be set
+     */
+
+    if (s->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB &&
+        !(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
+            error_setg(errp, "qcow2: Invalid compression type setting");
+            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 +1344,35 @@ 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
+     * Older qcow2 images don't contain the compression type header.
+     * Distinguish them by the header length and use
+     * the only valid (default) compression type in that case
+     */
+    if (header.header_length > offsetof(QCowHeader, compression_type)) {
+        /* sanity check that we can read a compression type */
+        size_t min_len = offsetof(QCowHeader, compression_type) +
+                         sizeof(header.compression_type);
+        if (header.header_length < min_len) {
+            error_setg(errp,
+                       "Could not read compression type."
+                       "qcow2 header is too short");
+           ret = -EINVAL;
+           goto fail;
+        }
+
+        header.compression_type = be32_to_cpu(header.compression_type);
+        s->compression_type = header.compression_type;
+    } else {
+        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 +2489,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 +2518,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 +2616,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 +3175,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 +3195,24 @@ 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) {
+
+        switch(qcow2_opts->compression_type) {
+        case QCOW2_COMPRESSION_TYPE_ZLIB:
+            break;
+
+        default:
+            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 },
     };
 
@@ -4675,6 +4763,7 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
             .data_file          = g_strdup(s->image_data_file),
             .has_data_file_raw  = has_data_file(bs),
             .data_file_raw      = data_file_is_raw(bs),
+            .compression_type   = s->compression_type,
         };
     } else {
         /* if this assertion fails, this probably means a new version was
@@ -5239,6 +5328,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 = "zlib"
+        },
         { /* 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..7cf068f814 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,20 @@ 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 (value: 0).
+                    When the compression type differs from the default
+                    the compression type bit (incompatible feature bit 3)
+                    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..835dd3c37f 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] 19+ messages in thread

* [Qemu-devel] [PATCH v2 2/3] qcow2: rework the cluster compression routine
  2019-07-04 13:09 [Qemu-devel] [PATCH v2 0/3] add zstd cluster compression Denis Plotnikov
  2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature Denis Plotnikov
@ 2019-07-04 13:09 ` Denis Plotnikov
  2019-08-08 13:30   ` Max Reitz
  2019-08-08 14:39   ` Max Reitz
  2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 3/3] qcow2: add zstd cluster compression Denis Plotnikov
  2019-07-30 14:45 ` [Qemu-devel] [PATCH v2 0/3] " Denis Plotnikov
  3 siblings, 2 replies; 19+ messages in thread
From: Denis Plotnikov @ 2019-07-04 13:09 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 8fa932a349..a107f76e98 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] 19+ messages in thread

* [Qemu-devel] [PATCH v2 3/3] qcow2: add zstd cluster compression
  2019-07-04 13:09 [Qemu-devel] [PATCH v2 0/3] add zstd cluster compression Denis Plotnikov
  2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature Denis Plotnikov
  2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 2/3] qcow2: rework the cluster compression routine Denis Plotnikov
@ 2019-07-04 13:09 ` Denis Plotnikov
  2019-07-09  6:18   ` Markus Armbruster
                     ` (2 more replies)
  2019-07-30 14:45 ` [Qemu-devel] [PATCH v2 0/3] " Denis Plotnikov
  3 siblings, 3 replies; 19+ messages in thread
From: Denis Plotnikov @ 2019-07-04 13:09 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          | 99 ++++++++++++++++++++++++++++++++++++++++++
 configure              | 32 ++++++++++++++
 docs/interop/qcow2.txt | 19 ++++++++
 qapi/block-core.json   |  3 +-
 4 files changed, 152 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index a107f76e98..252eba636f 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"
@@ -1206,6 +1211,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:
@@ -3200,6 +3208,9 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp)
 
         switch(qcow2_opts->compression_type) {
         case QCOW2_COMPRESSION_TYPE_ZLIB:
+#ifdef CONFIG_ZSTD
+        case QCOW2_COMPRESSION_TYPE_ZSTD:
+#endif
             break;
 
         default:
@@ -4092,6 +4103,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 +4269,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 +4306,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/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 7cf068f814..4344e858cb 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -538,6 +538,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
@@ -790,3 +793,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 ===
+
+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:
+(x = data_space_length - 1)
+
+    zlib:
+        Byte  0 -  x:     the compressed data content
+                          all the space provided used for compressed data
+    zstd:
+        Byte  0 -  3:     the length of compressed data
+              4 -  x:     the compressed data content
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 835dd3c37f..2021e03a84 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', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
 
 ##
 # @BlockdevCreateOptionsQcow2:
-- 
2.17.0



^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature
  2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature Denis Plotnikov
@ 2019-07-09  6:15   ` Markus Armbruster
  2019-08-07 23:12   ` Max Reitz
  2019-08-08  0:18   ` Eric Blake
  2 siblings, 0 replies; 19+ messages in thread
From: Markus Armbruster @ 2019-07-09  6:15 UTC (permalink / raw)
  To: Denis Plotnikov; +Cc: kwolf, vsementsov, den, qemu-block, qemu-devel, mreitz

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 2x 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
> are backward compatible with older qemu versions.
>
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>

QAPI schema
Acked-by: Markus Armbruster <armbru@redhat.com>


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/3] qcow2: add zstd cluster compression
  2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 3/3] qcow2: add zstd cluster compression Denis Plotnikov
@ 2019-07-09  6:18   ` Markus Armbruster
  2019-07-30 14:42     ` Denis Plotnikov
  2019-08-08  0:24   ` Eric Blake
  2019-08-08 14:44   ` Max Reitz
  2 siblings, 1 reply; 19+ messages in thread
From: Markus Armbruster @ 2019-07-09  6:18 UTC (permalink / raw)
  To: Denis Plotnikov; +Cc: kwolf, vsementsov, den, qemu-block, qemu-devel, mreitz

Denis Plotnikov <dplotnikov@virtuozzo.com> writes:

> 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>
[...]
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index 7cf068f814..4344e858cb 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -538,6 +538,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
> @@ -790,3 +793,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 ===
> +
> +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:
> +(x = data_space_length - 1)
> +
> +    zlib:
> +        Byte  0 -  x:     the compressed data content
> +                          all the space provided used for compressed data
> +    zstd:
> +        Byte  0 -  3:     the length of compressed data
> +              4 -  x:     the compressed data content

Adding <http://zlib.net/> and <http://github.com/facebook/zstd> here as
well wouldn't hurt, would it?

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 835dd3c37f..2021e03a84 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', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }
>  
>  ##
>  # @BlockdevCreateOptionsQcow2:

QAPI schema
Acked-by: Markus Armbruster <armbru@redhat.com>


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/3] qcow2: add zstd cluster compression
  2019-07-09  6:18   ` Markus Armbruster
@ 2019-07-30 14:42     ` Denis Plotnikov
  0 siblings, 0 replies; 19+ messages in thread
From: Denis Plotnikov @ 2019-07-30 14:42 UTC (permalink / raw)
  To: Markus Armbruster
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block,
	qemu-devel, mreitz

On Jul 9 2019, at 9:18 am, Markus Armbruster <armbru@redhat.com> wrote:
Denis Plotnikov <dplotnikov@virtuozzo.com> writes:

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>
[...]
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 7cf068f814..4344e858cb 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -538,6 +538,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
@@ -790,3 +793,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 ===
+
+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:
+(x = data_space_length - 1)
+
+ zlib:
+ Byte 0 - x: the compressed data content
+ all the space provided used for compressed data
+ zstd:
+ Byte 0 - 3: the length of compressed data
+ 4 - x: the compressed data content

Adding <http://zlib.net/> and <http://github.com/facebook/zstd> here as
well wouldn't hurt, would it?
ok

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 835dd3c37f..2021e03a84 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', { 'name': 'zstd', 'if': 'defined(CONFIG_ZSTD)' } ] }

##
# @BlockdevCreateOptionsQcow2:

QAPI schema
Acked-by: Markus Armbruster <armbru@redhat.com>


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v2 0/3] add zstd cluster compression
  2019-07-04 13:09 [Qemu-devel] [PATCH v2 0/3] add zstd cluster compression Denis Plotnikov
                   ` (2 preceding siblings ...)
  2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 3/3] qcow2: add zstd cluster compression Denis Plotnikov
@ 2019-07-30 14:45 ` Denis Plotnikov
  2019-08-07  7:12   ` Denis Plotnikov
  3 siblings, 1 reply; 19+ messages in thread
From: Denis Plotnikov @ 2019-07-30 14:45 UTC (permalink / raw)
  To: kwolf, mreitz, eblake, armbru
  Cc: Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block, qemu-devel

Hi all! Is there any other comments besides Markus's one about adding zlib/zstd links to compressed cluster layout description?

On Jul 4 2019, at 4:09 pm, Denis Plotnikov <dplotnikov@virtuozzo.com> wrote:
change log:

v2:
* relax the compression type setting restriction in the spec
* fix qcow2 header size checking
* fix error processing and messaging
* fix qcow2 image specific info reporting
* set Qcow2CompressionType zstd config dependant
* add zstd compressed cluster format description to the spec

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 | 287 +++++++++++++++++++++++++++++++++++---
block/qcow2.h | 26 +++-
configure | 32 +++++
docs/interop/qcow2.txt | 40 +++++-
include/block/block_int.h | 1 +
qapi/block-core.json | 23 ++-
6 files changed, 379 insertions(+), 30 deletions(-)

--
2.17.0


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v2 0/3] add zstd cluster compression
  2019-07-30 14:45 ` [Qemu-devel] [PATCH v2 0/3] " Denis Plotnikov
@ 2019-08-07  7:12   ` Denis Plotnikov
  0 siblings, 0 replies; 19+ messages in thread
From: Denis Plotnikov @ 2019-08-07  7:12 UTC (permalink / raw)
  To: qemu-devel
  Cc: kwolf, Vladimir Sementsov-Ogievskiy, Denis Lunev, qemu-block,
	armbru, mreitz

Ping!

On Jul 30 2019, at 5:45 pm, Denis Plotnikov <dplotnikov@virtuozzo.com> wrote:
Hi all! Is there any other comments besides Markus's one about adding zlib/zstd links to compressed cluster layout description?

On Jul 4 2019, at 4:09 pm, Denis Plotnikov <dplotnikov@virtuozzo.com> wrote:
change log:

v2:
* relax the compression type setting restriction in the spec
* fix qcow2 header size checking
* fix error processing and messaging
* fix qcow2 image specific info reporting
* set Qcow2CompressionType zstd config dependant
* add zstd compressed cluster format description to the spec

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 | 287 +++++++++++++++++++++++++++++++++++---
block/qcow2.h | 26 +++-
configure | 32 +++++
docs/interop/qcow2.txt | 40 +++++-
include/block/block_int.h | 1 +
qapi/block-core.json | 23 ++-
6 files changed, 379 insertions(+), 30 deletions(-)

--
2.17.0


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature
  2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature Denis Plotnikov
  2019-07-09  6:15   ` Markus Armbruster
@ 2019-08-07 23:12   ` Max Reitz
  2019-08-08  0:09     ` Eric Blake
  2019-08-08  0:18   ` Eric Blake
  2 siblings, 1 reply; 19+ messages in thread
From: Max Reitz @ 2019-08-07 23:12 UTC (permalink / raw)
  To: Denis Plotnikov, kwolf, eblake, armbru
  Cc: vsementsov, den, qemu-block, qemu-devel


[-- Attachment #1.1: Type: text/plain, Size: 9799 bytes --]

On 04.07.19 15:09, 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 2x 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
> are backward compatible with older qemu versions.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> ---
>  block/qcow2.c             | 95 +++++++++++++++++++++++++++++++++++++++
>  block/qcow2.h             | 26 ++++++++---
>  docs/interop/qcow2.txt    | 21 ++++++++-
>  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..8fa932a349 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1202,6 +1202,32 @@ static int qcow2_update_options(BlockDriverState *bs, QDict *options,
>      return ret;
>  }
>  
> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
> +{
> +    switch (s->compression_type) {
> +    case QCOW2_COMPRESSION_TYPE_ZLIB:
> +        break;
> +
> +    default:
> +        error_setg(errp, "qcow2: unknown compression type: %u",
> +                   s->compression_type);
> +        return -ENOTSUP;
> +    }
> +
> +    /*
> +     * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
> +     * the incompatible feature flag must be set
> +     */
> +
> +    if (s->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB &&
> +        !(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
> +            error_setg(errp, "qcow2: Invalid compression type setting");
> +            return -EINVAL;

(1) Why is this block indented twice?

(2) Do we need this at all?  Sure, it’s a corruption, but do we need to
reject the image because of it?

> +    }
> +
> +    return 0;
> +}
> +

Overall, I don’t see the purpose of this function.  I don’t see any need
to call it in qcow2_update_header().  And without “does non-zlib
compression imply the respective incompatible flag?” check, you can just
inline the rest (checking that we recognize the compression type) into
qcow2_do_open().

>  /* Called with s->lock held.  */
>  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>                                        int flags, Error **errp)
> @@ -1318,6 +1344,35 @@ 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
> +     * Older qcow2 images don't contain the compression type header.
> +     * Distinguish them by the header length and use
> +     * the only valid (default) compression type in that case
> +     */
> +    if (header.header_length > offsetof(QCowHeader, compression_type)) {
> +        /* sanity check that we can read a compression type */
> +        size_t min_len = offsetof(QCowHeader, compression_type) +
> +                         sizeof(header.compression_type);
> +        if (header.header_length < min_len) {
> +            error_setg(errp,
> +                       "Could not read compression type."
> +                       "qcow2 header is too short");

This will read as “Could not read compression type.qcow2 header is too
short”.  There should be a space after the full stop (and the full stop
should maybe be a comma instead).

> +           ret = -EINVAL;
> +           goto fail;

These two lines are incorrectly aligned.

> +        }
> +
> +        header.compression_type = be32_to_cpu(header.compression_type);
> +        s->compression_type = header.compression_type;
> +    } else {
> +        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 +2489,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;
> +    }
> +
> +

Again, I don’t see why this function should be called here.  If
anything, we should set the non-zlib incompatible flag here
automatically if a non-zlib compression type is used.

(And I don’t really see the point in checking that s->compression_type
is valid – because why shouldn’t it be?)

>      *header = (QCowHeader) {
>          /* Version 2 fields */
>          .magic                  = cpu_to_be32(QCOW_MAGIC),

[...]

> @@ -3126,6 +3195,24 @@ 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) {
> +
> +        switch(qcow2_opts->compression_type) {

In qemu, we generally put a space between “switch” and the opening
parenthesis.


Also, just a hint: If you don’t like the visual appearance of

    if (long &&
        condition) {
        block_statement;

(I know I don’t)

you can (whenever a condition spans multiple lines) put the opening
curly bracket on a separate line:

    if (long &&
        condition)
    {
        block_statement;

(I personally prefer that over an empty line.)

> +        case QCOW2_COMPRESSION_TYPE_ZLIB:

Well, a bit useless considering you just excluded this case in the if
condition, but I suppose the compiler forced you to include this arm.

I suppose we should abort() here because you made the specification
state that the incompatible features must not be set with zlib
compression mode, so it looks weird to just accept this case here and
then set the incompatible flag below.

> +            break;
> +
> +        default:
> +            error_setg_errno(errp, -EINVAL, "Unknown compression type");

I think a plain abort() is fine here.  This is an enum after all, it
cannot have any other values.

> +            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) {

[...]

> 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..835dd3c37f 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)

s/1/2/

> +#
>  # 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

s/1/2/

> +##
> +{ '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)

s/1/2/

Max

>  #
>  # 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:
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature
  2019-08-07 23:12   ` Max Reitz
@ 2019-08-08  0:09     ` Eric Blake
  2019-08-08 12:48       ` Max Reitz
  0 siblings, 1 reply; 19+ messages in thread
From: Eric Blake @ 2019-08-08  0:09 UTC (permalink / raw)
  To: Max Reitz, Denis Plotnikov, kwolf, armbru
  Cc: vsementsov, den, qemu-block, qemu-devel


[-- Attachment #1.1: Type: text/plain, Size: 3648 bytes --]

On 8/7/19 6:12 PM, Max Reitz wrote:

>>  
>> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
>> +{
>> +    switch (s->compression_type) {
>> +    case QCOW2_COMPRESSION_TYPE_ZLIB:
>> +        break;
>> +
>> +    default:
>> +        error_setg(errp, "qcow2: unknown compression type: %u",
>> +                   s->compression_type);
>> +        return -ENOTSUP;
>> +    }
>> +
>> +    /*
>> +     * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
>> +     * the incompatible feature flag must be set
>> +     */
>> +
>> +    if (s->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB &&
>> +        !(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
>> +            error_setg(errp, "qcow2: Invalid compression type setting");
>> +            return -EINVAL;
> 
> (1) Why is this block indented twice?
> 
> (2) Do we need this at all?  Sure, it’s a corruption, but do we need to
> reject the image because of it?

Yes, because otherwise there is a high risk of some application
misinterpreting the contents (whether an older qemu that silently
ignores unrecognized headers, and so assumes it can decode compressed
clusters with zlib even though the decode will only succeed with zstd,
or can write a compressed cluster with zlib which then causes corruption
when the newer qemu tries to read it with zstd).  The whole point of an
incompatible bit is to reject opening an image that can't be interpreted
correctly, and where writing may break later readers.

> 
>> +    }
>> +
>> +    return 0;
>> +}
>> +
> 
> Overall, I don’t see the purpose of this function.  I don’t see any need
> to call it in qcow2_update_header().  And without “does non-zlib
> compression imply the respective incompatible flag?” check, you can just
> inline the rest (checking that we recognize the compression type) into
> qcow2_do_open().
> 

Inlining may indeed be possible; the real question is whether the
function expands later in the series to the point that inlining no
longer makes sense.

>>  /* Called with s->lock held.  */
>>  static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>>                                        int flags, Error **errp)
>> @@ -1318,6 +1344,35 @@ 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
>> +     * Older qcow2 images don't contain the compression type header.
>> +     * Distinguish them by the header length and use
>> +     * the only valid (default) compression type in that case
>> +     */
>> +    if (header.header_length > offsetof(QCowHeader, compression_type)) {
>> +        /* sanity check that we can read a compression type */
>> +        size_t min_len = offsetof(QCowHeader, compression_type) +
>> +                         sizeof(header.compression_type);
>> +        if (header.header_length < min_len) {
>> +            error_setg(errp,
>> +                       "Could not read compression type."
>> +                       "qcow2 header is too short");
> 
> This will read as “Could not read compression type.qcow2 header is too
> short”.  There should be a space after the full stop (and the full stop
> should maybe be a comma instead).

Indeed, error_setg() should generally not contain '.'

-- 
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] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature
  2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature Denis Plotnikov
  2019-07-09  6:15   ` Markus Armbruster
  2019-08-07 23:12   ` Max Reitz
@ 2019-08-08  0:18   ` Eric Blake
  2019-08-08 12:50     ` Max Reitz
  2 siblings, 1 reply; 19+ messages in thread
From: Eric Blake @ 2019-08-08  0:18 UTC (permalink / raw)
  To: Denis Plotnikov, kwolf, mreitz, armbru
  Cc: vsementsov, den, qemu-block, qemu-devel


[-- Attachment #1.1: Type: text/plain, Size: 3632 bytes --]

On 7/4/19 8:09 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 2x faster than ZLIB providing a comparable compression ratio
> and therefore provide a performance advantage in backup scenarios.

provides

> 
> The default compression is ZLIB. Images created with ZLIB compression type
> are backward compatible with older qemu versions.
> 
> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>

> +++ 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.

I'd word this 'from the default of zlib.'

> +                                If the compression type is default the bit must
> +                                be unset.

Why? I see no reason to forbid a qcow2 image that has the incompatible
bit set but still uses zlib compression.  True, such an image is not as
friendly to older clients, but it is not technically wrong, and newer
clients would still be able to use the image if not for this sentence
telling them they must not.  I'd drop this sentence.

> +
> +                    Bits 4-63:  Reserved (set to 0)
>  
>           80 -  87:  compatible_features
>                      Bitmask of compatible features. An implementation can
> @@ -165,6 +170,20 @@ 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 (value: 0).
> +                    When the compression type differs from the default
> +                    the compression type bit (incompatible feature bit 3)
> +                    must be set.

So far, so good.

> +                    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.

I'm wondering whether we need to spell this out in the spec.  Yes, I
know we spell out other qemu limitations elsewhere, but with a version
number.  But the spec would not be any less correct if you omitted this
sentence.

-- 
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] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/3] qcow2: add zstd cluster compression
  2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 3/3] qcow2: add zstd cluster compression Denis Plotnikov
  2019-07-09  6:18   ` Markus Armbruster
@ 2019-08-08  0:24   ` Eric Blake
  2019-08-08 14:44   ` Max Reitz
  2 siblings, 0 replies; 19+ messages in thread
From: Eric Blake @ 2019-08-08  0:24 UTC (permalink / raw)
  To: Denis Plotnikov, kwolf, mreitz, armbru
  Cc: vsementsov, den, qemu-block, qemu-devel


[-- Attachment #1.1: Type: text/plain, Size: 3313 bytes --]

On 7/4/19 8:09 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

s/by/at/

> 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
> 

Nice numbers.

> +++ b/docs/interop/qcow2.txt
> @@ -538,6 +538,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
> @@ -790,3 +793,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 ===
> +
> +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:
> +(x = data_space_length - 1)
> +
> +    zlib:
> +        Byte  0 -  x:     the compressed data content
> +                          all the space provided used for compressed data
> +    zstd:
> +        Byte  0 -  3:     the length of compressed data
> +              4 -  x:     the compressed data content

Missing a change to the header description at bytes 104-107 calling out
'1' as meaning zstd (it only calls out '0' or absent as meaning zlib).

-- 
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] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature
  2019-08-08  0:09     ` Eric Blake
@ 2019-08-08 12:48       ` Max Reitz
  0 siblings, 0 replies; 19+ messages in thread
From: Max Reitz @ 2019-08-08 12:48 UTC (permalink / raw)
  To: Eric Blake, Denis Plotnikov, kwolf, armbru
  Cc: vsementsov, den, qemu-block, qemu-devel


[-- Attachment #1.1: Type: text/plain, Size: 2239 bytes --]

On 08.08.19 02:09, Eric Blake wrote:
> On 8/7/19 6:12 PM, Max Reitz wrote:
> 
>>>  
>>> +static int check_compression_type(BDRVQcow2State *s, Error **errp)
>>> +{
>>> +    switch (s->compression_type) {
>>> +    case QCOW2_COMPRESSION_TYPE_ZLIB:
>>> +        break;
>>> +
>>> +    default:
>>> +        error_setg(errp, "qcow2: unknown compression type: %u",
>>> +                   s->compression_type);
>>> +        return -ENOTSUP;
>>> +    }
>>> +
>>> +    /*
>>> +     * if the compression type differs from QCOW2_COMPRESSION_TYPE_ZLIB
>>> +     * the incompatible feature flag must be set
>>> +     */
>>> +
>>> +    if (s->compression_type != QCOW2_COMPRESSION_TYPE_ZLIB &&
>>> +        !(s->incompatible_features & QCOW2_INCOMPAT_COMPRESSION_TYPE)) {
>>> +            error_setg(errp, "qcow2: Invalid compression type setting");
>>> +            return -EINVAL;
>>
>> (1) Why is this block indented twice?
>>
>> (2) Do we need this at all?  Sure, it’s a corruption, but do we need to
>> reject the image because of it?
> 
> Yes, because otherwise there is a high risk of some application
> misinterpreting the contents (whether an older qemu that silently
> ignores unrecognized headers, and so assumes it can decode compressed
> clusters with zlib even though the decode will only succeed with zstd,
> or can write a compressed cluster with zlib which then causes corruption
> when the newer qemu tries to read it with zstd).  The whole point of an
> incompatible bit is to reject opening an image that can't be interpreted
> correctly, and where writing may break later readers.

Fair enough.

>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>
>> Overall, I don’t see the purpose of this function.  I don’t see any need
>> to call it in qcow2_update_header().  And without “does non-zlib
>> compression imply the respective incompatible flag?” check, you can just
>> inline the rest (checking that we recognize the compression type) into
>> qcow2_do_open().
>>
> 
> Inlining may indeed be possible; the real question is whether the
> function expands later in the series to the point that inlining no
> longer makes sense.

A quick search says no.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature
  2019-08-08  0:18   ` Eric Blake
@ 2019-08-08 12:50     ` Max Reitz
  2019-08-08 14:07       ` Kevin Wolf
  0 siblings, 1 reply; 19+ messages in thread
From: Max Reitz @ 2019-08-08 12:50 UTC (permalink / raw)
  To: Eric Blake, Denis Plotnikov, kwolf, armbru
  Cc: vsementsov, den, qemu-block, qemu-devel


[-- Attachment #1.1: Type: text/plain, Size: 2343 bytes --]

On 08.08.19 02:18, Eric Blake wrote:
> On 7/4/19 8:09 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 2x faster than ZLIB providing a comparable compression ratio
>> and therefore provide a performance advantage in backup scenarios.
> 
> provides
> 
>>
>> The default compression is ZLIB. Images created with ZLIB compression type
>> are backward compatible with older qemu versions.
>>
>> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> 
>> +++ 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.
> 
> I'd word this 'from the default of zlib.'
> 
>> +                                If the compression type is default the bit must
>> +                                be unset.
> 
> Why? I see no reason to forbid a qcow2 image that has the incompatible
> bit set but still uses zlib compression.  True, such an image is not as
> friendly to older clients, but it is not technically wrong, and newer
> clients would still be able to use the image if not for this sentence
> telling them they must not.

Just because an image doesn’t adhere to the specification doesn’t mean
you have to reject it, if the intention is clear.

> I'd drop this sentence.

I wouldn’t, I like it (in essence).  Though maybe the “must” is indeed
too strong and it should be a “should” instead.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/3] qcow2: rework the cluster compression routine
  2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 2/3] qcow2: rework the cluster compression routine Denis Plotnikov
@ 2019-08-08 13:30   ` Max Reitz
  2019-08-08 14:39   ` Max Reitz
  1 sibling, 0 replies; 19+ messages in thread
From: Max Reitz @ 2019-08-08 13:30 UTC (permalink / raw)
  To: Denis Plotnikov, kwolf, eblake, armbru
  Cc: vsementsov, den, qemu-block, qemu-devel


[-- Attachment #1.1: Type: text/plain, Size: 3554 bytes --]

On 04.07.19 15:09, Denis Plotnikov wrote:
> 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(-)

I tried my best to rebase this patch on top of the current master, I
hope I did it right.

(A couple of the hunks in this patch are already in master.)

> diff --git a/block/qcow2.c b/block/qcow2.c
> index 8fa932a349..a107f76e98 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c

[...]

> @@ -4059,8 +4062,8 @@ static ssize_t qcow2_compress(void *dest, size_t dest_size,
>   * Returns: 0 on success
>   *          -1 on fail

This should be fixed to reflect the new return values.

>   */
> -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;

[...]

> @@ -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;

A plain abort() would work, too.  (Your choice.)

> +    }
> +
> +    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;

Same here.

Max

> +    }
> +
> +    return qcow2_co_do_compress(bs, dest, dest_size, src, src_size, fn);
>  }


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature
  2019-08-08 12:50     ` Max Reitz
@ 2019-08-08 14:07       ` Kevin Wolf
  0 siblings, 0 replies; 19+ messages in thread
From: Kevin Wolf @ 2019-08-08 14:07 UTC (permalink / raw)
  To: Max Reitz
  Cc: vsementsov, den, qemu-block, armbru, qemu-devel, Denis Plotnikov

[-- Attachment #1: Type: text/plain, Size: 2693 bytes --]

Am 08.08.2019 um 14:50 hat Max Reitz geschrieben:
> On 08.08.19 02:18, Eric Blake wrote:
> > On 7/4/19 8:09 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 2x faster than ZLIB providing a comparable compression ratio
> >> and therefore provide a performance advantage in backup scenarios.
> > 
> > provides
> > 
> >>
> >> The default compression is ZLIB. Images created with ZLIB compression type
> >> are backward compatible with older qemu versions.
> >>
> >> Signed-off-by: Denis Plotnikov <dplotnikov@virtuozzo.com>
> > 
> >> +++ 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.
> > 
> > I'd word this 'from the default of zlib.'
> > 
> >> +                                If the compression type is default the bit must
> >> +                                be unset.
> > 
> > Why? I see no reason to forbid a qcow2 image that has the incompatible
> > bit set but still uses zlib compression.  True, such an image is not as
> > friendly to older clients, but it is not technically wrong, and newer
> > clients would still be able to use the image if not for this sentence
> > telling them they must not.
> 
> Just because an image doesn’t adhere to the specification doesn’t mean
> you have to reject it, if the intention is clear.
> 
> > I'd drop this sentence.
> 
> I wouldn’t, I like it (in essence).  Though maybe the “must” is indeed
> too strong and it should be a “should” instead.

I'd agree that "should" is the best way for the spec.

In the code, I'd insist that QEMU doesn't add any extra code to verify
that this is the case (which previous versions of the series did).

Kevin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v2 2/3] qcow2: rework the cluster compression routine
  2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 2/3] qcow2: rework the cluster compression routine Denis Plotnikov
  2019-08-08 13:30   ` Max Reitz
@ 2019-08-08 14:39   ` Max Reitz
  1 sibling, 0 replies; 19+ messages in thread
From: Max Reitz @ 2019-08-08 14:39 UTC (permalink / raw)
  To: Denis Plotnikov, kwolf, eblake, armbru
  Cc: vsementsov, den, qemu-block, qemu-devel


[-- Attachment #1.1: Type: text/plain, Size: 1097 bytes --]

On 04.07.19 15:09, Denis Plotnikov wrote:
> 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 8fa932a349..a107f76e98 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c

[...]

> @@ -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

Actually, it returns the number of compressed bytes.

Max

> + *          a negative error code on fail
> + */


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [Qemu-devel] [PATCH v2 3/3] qcow2: add zstd cluster compression
  2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 3/3] qcow2: add zstd cluster compression Denis Plotnikov
  2019-07-09  6:18   ` Markus Armbruster
  2019-08-08  0:24   ` Eric Blake
@ 2019-08-08 14:44   ` Max Reitz
  2 siblings, 0 replies; 19+ messages in thread
From: Max Reitz @ 2019-08-08 14:44 UTC (permalink / raw)
  To: Denis Plotnikov, kwolf, eblake, armbru
  Cc: vsementsov, den, qemu-block, qemu-devel


[-- Attachment #1.1: Type: text/plain, Size: 6357 bytes --]

On 04.07.19 15:09, 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.
> 
> 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          | 99 ++++++++++++++++++++++++++++++++++++++++++
>  configure              | 32 ++++++++++++++
>  docs/interop/qcow2.txt | 19 ++++++++
>  qapi/block-core.json   |  3 +-
>  4 files changed, 152 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index a107f76e98..252eba636f 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c

[...]

> @@ -4092,6 +4103,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) {

s/ret/ZSTD_getErrorCode(ret)/

> +            return -ENOMEM;
> +        } else {
> +            return -EIO;
> +        }
> +    }
> +
> +    /* store the compressed chunk size in the very beginning of the buffer */
> +    *c_size = ret;

I think this should be stored in big endian.

> +
> +    return ret + sizeof(ret);

s/sizeof(ret)/sizeof(*c_size)/

> +}
> +
> +/*
> + * 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

*length

> +     * 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);

If you want to be strict, s/(char *)/(const char *)/.

> +
> +    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,

[...]

> diff --git a/configure b/configure
> index 1c563a7027..57a80e38e7 100755
> --- a/configure
> +++ b/configure

[...]

> @@ -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."

(1) When the user doesn’t specify anything and doesn’t have zstd
installed, this will throw an error.  That shouldn’t be, it should just
set zstd to "no", unless zstd was explicitly "yes".

(2) All other places use feature_not_found.  I think this should, too.

> +        fi
> +    fi

You must set zstd to "yes" if it was found.

(So it is used when available, even if the user did not explicitly pass
--enable-zstd.)

> +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"

The status should be printed in the list of what features are enabled.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2019-08-08 14:44 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-04 13:09 [Qemu-devel] [PATCH v2 0/3] add zstd cluster compression Denis Plotnikov
2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 1/3] qcow2: introduce compression type feature Denis Plotnikov
2019-07-09  6:15   ` Markus Armbruster
2019-08-07 23:12   ` Max Reitz
2019-08-08  0:09     ` Eric Blake
2019-08-08 12:48       ` Max Reitz
2019-08-08  0:18   ` Eric Blake
2019-08-08 12:50     ` Max Reitz
2019-08-08 14:07       ` Kevin Wolf
2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 2/3] qcow2: rework the cluster compression routine Denis Plotnikov
2019-08-08 13:30   ` Max Reitz
2019-08-08 14:39   ` Max Reitz
2019-07-04 13:09 ` [Qemu-devel] [PATCH v2 3/3] qcow2: add zstd cluster compression Denis Plotnikov
2019-07-09  6:18   ` Markus Armbruster
2019-07-30 14:42     ` Denis Plotnikov
2019-08-08  0:24   ` Eric Blake
2019-08-08 14:44   ` Max Reitz
2019-07-30 14:45 ` [Qemu-devel] [PATCH v2 0/3] " Denis Plotnikov
2019-08-07  7:12   ` Denis Plotnikov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.