[Qemu-devel] [PATCH V5 00/10] add Qcow2 compress format extension

[Qemu-devel] [PATCH V5 00/10] add Qcow2 compress format extension

[Peter Lieven]

this adds a create option for Qcow2 images to specify the compression format
and level for compressed clusters. The series adds 2 algorithms to choose from:
zlib and lzo. zlib is the current default, but with unoptimal settings.
If no compress.format option is specified the old zlib with the old parameters
is used and the created images are backwards compatible with older QEMU version.
As soon as a compression format is specified a new compress format header extension
is written and the Qcow2 images are incompatible with older QEMU versions.

Some numbers for an uncompressed Debian 9 QCOW2 image (size 1148MB):

compress.format     compress time    compressed size   decompress time
none                35.7s            339MB             3.4s
zlib (default)      30.5s            320MB             3.2s
zlib (level 1)      12.8s            348MB             3.2s
lzo                  4.2s            429MB             1.6s

Changes V4->V5: (thanks to Eric for his various comments)
  patch 1: rename format from zlib -> deflate
           add windows size to header
           define default values (deflate, level 0, window_size 12)
  patch 2: move level param to Qcow2CompressDeflate
           add window-size to Qcow2CompressDeflate
  patch 3: set default values in the parameter check function, pass Qcow2Compress struct
  patch 4: mention compress.window-size and the old default values
  patch 5: use Qcow2Compress struct inside BDRVQcow2State
           set default values (deflate/0/12) in qcow2_do_open
  patch 9: fix docs/interop/qcow2.txt

Changes V3->V4:
  - reduce the compress format name to 15 bytes [Eric]
  - explain the default compression level for zlib [Eric]
  - explain the difference between zlib with default compression
    and the old default [Eric]
  - added Patch 10

Changes V2->V3:
  - rebase to current master (qcow2 crypto extension)
  - patch 1: explicitly list valid format names, list valid compression levels,
             remove extra_data and extra_data_size for now [Kevin]
  - patch 2: don't hook the compression options in the blockdev-add options [Kevin, Daniel]
  - patch 3: parse compress settings from the qapi struct [Daniel]
  - patch 4: mention valid values for zlib, mention incompatiblity for QEMU < 2.10 [Kevin]
  - patch 5: use qapi to parse the compress format from header [Daniel]
  - added patch 6

Changes V1->V2:
  - split the series into more patches
  - added an qapi scheme for the compression settings
  - renamed compression_algorithm to compress.format and added compress.level+
  - updated the header extension to carry a variable extra payload and compress
    level.
  - removed extra reservations for header extensions
  - added missing lzo_init and fixed compress overhead for lzo

Peter Lieven (10):
  specs/qcow2: add compress format extension
  qapi/block-core: add Qcow2Compress parameters
  block/qcow2: parse compress create options
  qemu-img: add documentation for compress settings
  block/qcow2: read and write the compress format extension
  block/qcow2: simplify ret usage in qcow2_create
  block/qcow2: optimize qcow2_co_pwritev_compressed
  block/qcow2: start using the compress format extension
  block/qcow2: add lzo compress format
  block/qcow2: add compress info to image specific info

 block/qcow2-cluster.c     |  73 ++++++++----
 block/qcow2.c             | 282 +++++++++++++++++++++++++++++++++++++++-------
 block/qcow2.h             |  21 +++-
 configure                 |   2 +-
 docs/interop/qcow2.txt    |  53 ++++++++-
 include/block/block_int.h |  39 ++++---
 qapi/block-core.json      |  55 ++++++++-
 qemu-img.texi             |  27 +++++
 roms/ipxe                 |   2 +-
 9 files changed, 461 insertions(+), 93 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH V5 01/10] specs/qcow2: add compress format extension

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 docs/interop/qcow2.txt | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
 roms/ipxe              |  2 +-
 2 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index d7fdb1f..d0d2a8f 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -86,7 +86,12 @@ in the description of a field.
                                 be written to (unless for regaining
                                 consistency).
 
-                    Bits 2-63:  Reserved (set to 0)
+                    Bit 2:      Compress format bit.  If and only if this bit
+                                is set then the compress format extension
+                                MUST be present and MUST be parsed and checked
+                                for compatibility.
+
+                    Bits 3-63:  Reserved (set to 0)
 
          80 -  87:  compatible_features
                     Bitmask of compatible features. An implementation can
@@ -137,6 +142,7 @@ be stored. Each extension has a structure like the following:
                         0x6803f857 - Feature name table
                         0x23852875 - Bitmaps extension
                         0x0537be77 - Full disk encryption header pointer
+                        0xC03183A3 - Compress format extension
                         other      - Unknown header extension, can be safely
                                      ignored
 
@@ -311,6 +317,49 @@ The algorithms used for encryption vary depending on the method
    in the LUKS header, with the physical disk sector as the
    input tweak.
 
+
+== Compress format extension ==
+
+The compress format extension is an optional header extension. It provides
+the ability to specify the compress algorithm and compress parameters
+that are used for compressed clusters. This new header MUST be present if
+the incompatible-feature bit "compress format bit" is set and MUST be absent
+otherwise.
+
+The fields of the compress format extension are:
+
+    Byte  0 - 13:  compress_format_name (padded with zeros, but not
+                   necessarily null terminated if it has full length).
+                   Valid compression format names currently are:
+
+                   deflate: Standard zlib deflate compression without
+                            compression header
+
+              14:  compress_level (uint8_t)
+
+                   0 = default compress level (valid for all formats, default)
+
+                   Additional valid compression levels for deflate compression:
+
+                   All values between 1 and 9. 1 gives best speed, 9 gives best
+                   compression. The default compression level is defined by zlib
+                   and currently defaults to 6.
+
+              15:  compress_window_size (uint8_t)
+
+                   Window or dictionary size used by the compression format.
+                   Currently only used by the deflate compression algorithm.
+
+                   Valid window sizes for deflate compression range from 8 to
+                   15 inclusively.
+
+Note: Omitting the incompatible "Compress format bit" results in the usage
+of deflate compression with default compression level and a window size of 12
+(which was default before QEMU 2.11). If exactly these parameters are choosen
+it is free to the implementation to omit the "Compress format bit" and the
+compress format extension when updating the QCOW2 header.
+
+
 == Host cluster management ==
 
 qcow2 manages the allocation of host clusters by maintaining a reference count
diff --git a/roms/ipxe b/roms/ipxe
index 0600d3a..b991c67 160000
--- a/roms/ipxe
+++ b/roms/ipxe
@@ -1 +1 @@
-Subproject commit 0600d3ae94f93efd10fc6b3c7420a9557a3a1670
+Subproject commit b991c67c1d91574ef22336cc3a5944d1e63230c9
-- 
1.9.1

[Qemu-devel] [PATCH V5 02/10] qapi/block-core: add Qcow2Compress parameters

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 qapi/block-core.json | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 833c602..f652206 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2455,6 +2455,46 @@
             '*encrypt': 'BlockdevQcow2Encryption' } }
 
 ##
+# @Qcow2CompressFormat:
+#
+# @deflate: standard zlib deflate compression
+#
+# Since: 2.11
+##
+{ 'enum': 'Qcow2CompressFormat',
+  'data': [ 'deflate' ] }
+
+##
+# @Qcow2CompressDeflate:
+#
+# @level: specifies the compression level. 0 = default compression,
+#         1 = fastest compression, 9 = best compresion
+# @window-size: specifies the window size used for deflate compression.
+#               8...15 = window size of 2^8 to 2^15 byte (default)
+#
+# Since: 2.11
+##
+{ 'struct': 'Qcow2CompressDeflate',
+  'data': { '*level': 'uint8',
+            '*window-size': 'uint8' } }
+
+##
+# @Qcow2Compress:
+#
+# Specifies the compression format and compression level that should
+# be used for compressed Qcow2 clusters.
+#
+# @format: specifies the compression format to use. (defaults to zlib)
+#
+# Since: 2.11
+##
+{ 'union': 'Qcow2Compress',
+  'base': { 'format': 'Qcow2CompressFormat' },
+  'discriminator': 'format',
+  'data': { 'deflate': 'Qcow2CompressDeflate' } }
+
+
+##
 # @BlockdevOptionsSsh:
 #
 # @server:              host address
-- 
1.9.1

[Qemu-devel] [PATCH V5 03/10] block/qcow2: parse compress create options

[Peter Lieven]

this adds parsing and validation for the compress create
options. They are only validated but not yet used.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/qcow2.c             | 86 +++++++++++++++++++++++++++++++++++++++++++++--
 include/block/block_int.h | 39 +++++++++++----------
 2 files changed, 105 insertions(+), 20 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 90efa44..a5270a6 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -31,6 +31,8 @@
 #include "qapi/qmp/qerror.h"
 #include "qapi/qmp/qbool.h"
 #include "qapi/util.h"
+#include "qapi-visit.h"
+#include "qapi/qobject-input-visitor.h"
 #include "qapi/qmp/types.h"
 #include "qapi-event.h"
 #include "trace.h"
@@ -161,6 +163,34 @@ static ssize_t qcow2_crypto_hdr_write_func(QCryptoBlock *block, size_t offset,
     return ret;
 }
 
+static void
+qcow2_check_compress_settings(Qcow2Compress *compress, Error **errp)
+{
+    if (compress->format == QCOW2_COMPRESS_FORMAT_DEFLATE) {
+        if (!compress->u.deflate.has_level) {
+            compress->u.deflate.has_level = true;
+            compress->u.deflate.level = 0;
+        }
+        if (compress->u.deflate.level > 9) {
+            error_setg(errp, "Compress level %" PRIu8 " is not supported for"
+                       " format '%s'", compress->u.deflate.level,
+                       Qcow2CompressFormat_lookup[compress->format]);
+            return;
+        }
+        if (!compress->u.deflate.has_window_size) {
+            compress->u.deflate.has_window_size = true;
+            compress->u.deflate.window_size = 15;
+        }
+        if (compress->u.deflate.window_size < 8 ||
+            compress->u.deflate.window_size > 15) {
+            error_setg(errp, "Compress window size %" PRIu8 " is not supported"
+                       " for format '%s'", compress->u.deflate.window_size,
+                       Qcow2CompressFormat_lookup[compress->format]);
+            return;
+        }
+    }
+}
+
 
 /* 
  * read qcow2 extension and fill bs
@@ -2706,7 +2736,8 @@ static int qcow2_create2(const char *filename, int64_t total_size,
                          const char *backing_file, const char *backing_format,
                          int flags, size_t cluster_size, PreallocMode prealloc,
                          QemuOpts *opts, int version, int refcount_order,
-                         const char *encryptfmt, Error **errp)
+                         const char *encryptfmt, Qcow2Compress *compress,
+                         Error **errp)
 {
     QDict *options;
 
@@ -2898,6 +2929,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     char *backing_file = NULL;
     char *backing_fmt = NULL;
     char *buf = NULL;
+    Qcow2Compress compress, *compress_ptr = NULL;
     uint64_t size = 0;
     int flags = 0;
     size_t cluster_size = DEFAULT_CLUSTER_SIZE;
@@ -2975,9 +3007,42 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 
     refcount_order = ctz32(refcount_bits);
 
+    if (qemu_opt_get(opts, BLOCK_OPT_COMPRESS_FORMAT) ||
+        qemu_opt_get(opts, BLOCK_OPT_COMPRESS_LEVEL) ||
+        qemu_opt_get(opts, BLOCK_OPT_COMPRESS_WINDOW_SIZE)) {
+        QDict *options, *compressopts = NULL;
+        Visitor *v;
+        options = qemu_opts_to_qdict(opts, NULL);
+        qdict_extract_subqdict(options, &compressopts, "compress.");
+        v = qobject_input_visitor_new_keyval(QOBJECT(compressopts));
+        visit_start_struct(v, NULL, NULL, 0, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            ret = -EINVAL;
+            goto finish;
+        }
+        visit_type_Qcow2Compress_members(v, &compress, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            ret = -EINVAL;
+            goto finish;
+        }
+        visit_end_struct(v, NULL);
+        visit_free(v);
+        QDECREF(compressopts);
+        QDECREF(options);
+        qcow2_check_compress_settings(&compress, &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
+            ret = -EINVAL;
+            goto finish;
+        }
+        compress_ptr = &compress;
+    }
+
     ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
                         cluster_size, prealloc, opts, version, refcount_order,
-                        encryptfmt, &local_err);
+                        encryptfmt, compress_ptr, &local_err);
     error_propagate(errp, local_err);
 
 finish:
@@ -4287,6 +4352,23 @@ static QemuOptsList qcow2_create_opts = {
             .help = "Width of a reference count entry in bits",
             .def_value_str = "16"
         },
+        {
+            .name = BLOCK_OPT_COMPRESS_FORMAT,
+            .type = QEMU_OPT_STRING,
+            .help = "Compress format used for compressed clusters (deflate)",
+        },
+        {
+            .name = BLOCK_OPT_COMPRESS_LEVEL,
+            .type = QEMU_OPT_NUMBER,
+            .help = "Compress level used for compressed clusters (0 = default,"
+                    " further valid options for deflate: 1=fastest ... 9=best)",
+        },
+        {
+            .name = BLOCK_OPT_COMPRESS_WINDOW_SIZE,
+            .type = QEMU_OPT_NUMBER,
+            .help = "Compress windows size used for compression (valid for"
+                    " deflate compression with values from 8 to 15)",
+        },
         { /* end of list */ }
     }
 };
diff --git a/include/block/block_int.h b/include/block/block_int.h
index d4f4ea7..91428d8 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -39,24 +39,27 @@
 
 #define BLOCK_FLAG_LAZY_REFCOUNTS   8
 
-#define BLOCK_OPT_SIZE              "size"
-#define BLOCK_OPT_ENCRYPT           "encryption"
-#define BLOCK_OPT_ENCRYPT_FORMAT    "encrypt.format"
-#define BLOCK_OPT_COMPAT6           "compat6"
-#define BLOCK_OPT_HWVERSION         "hwversion"
-#define BLOCK_OPT_BACKING_FILE      "backing_file"
-#define BLOCK_OPT_BACKING_FMT       "backing_fmt"
-#define BLOCK_OPT_CLUSTER_SIZE      "cluster_size"
-#define BLOCK_OPT_TABLE_SIZE        "table_size"
-#define BLOCK_OPT_PREALLOC          "preallocation"
-#define BLOCK_OPT_SUBFMT            "subformat"
-#define BLOCK_OPT_COMPAT_LEVEL      "compat"
-#define BLOCK_OPT_LAZY_REFCOUNTS    "lazy_refcounts"
-#define BLOCK_OPT_ADAPTER_TYPE      "adapter_type"
-#define BLOCK_OPT_REDUNDANCY        "redundancy"
-#define BLOCK_OPT_NOCOW             "nocow"
-#define BLOCK_OPT_OBJECT_SIZE       "object_size"
-#define BLOCK_OPT_REFCOUNT_BITS     "refcount_bits"
+#define BLOCK_OPT_SIZE                 "size"
+#define BLOCK_OPT_ENCRYPT              "encryption"
+#define BLOCK_OPT_ENCRYPT_FORMAT       "encrypt.format"
+#define BLOCK_OPT_COMPAT6              "compat6"
+#define BLOCK_OPT_HWVERSION            "hwversion"
+#define BLOCK_OPT_BACKING_FILE         "backing_file"
+#define BLOCK_OPT_BACKING_FMT          "backing_fmt"
+#define BLOCK_OPT_CLUSTER_SIZE         "cluster_size"
+#define BLOCK_OPT_TABLE_SIZE           "table_size"
+#define BLOCK_OPT_PREALLOC             "preallocation"
+#define BLOCK_OPT_SUBFMT               "subformat"
+#define BLOCK_OPT_COMPAT_LEVEL         "compat"
+#define BLOCK_OPT_LAZY_REFCOUNTS       "lazy_refcounts"
+#define BLOCK_OPT_ADAPTER_TYPE         "adapter_type"
+#define BLOCK_OPT_REDUNDANCY           "redundancy"
+#define BLOCK_OPT_NOCOW                "nocow"
+#define BLOCK_OPT_OBJECT_SIZE          "object_size"
+#define BLOCK_OPT_REFCOUNT_BITS        "refcount_bits"
+#define BLOCK_OPT_COMPRESS_FORMAT      "compress.format"
+#define BLOCK_OPT_COMPRESS_LEVEL       "compress.level"
+#define BLOCK_OPT_COMPRESS_WINDOW_SIZE "compress.window-size"
 
 #define BLOCK_PROBE_BUF_SIZE        512
 
-- 
1.9.1

[Qemu-devel] [PATCH V5 04/10] qemu-img: add documentation for compress settings

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 qemu-img.texi | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/qemu-img.texi b/qemu-img.texi
index 72dabd6..3612c59 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -676,6 +676,32 @@ file which is COW and has data blocks already, it couldn't be changed to NOCOW
 by setting @code{nocow=on}. One can issue @code{lsattr filename} to check if
 the NOCOW flag is set or not (Capital 'C' is NOCOW flag).
 
+@item compress.format
+Defines which compression algorithm is should be used for compressed clusters.
+The following options are available if support for the respective libraries
+has been enabled at compile time:
+
+   deflate        Uses standard zlib defalte compression
+
+The compression algorithm can only be defined at image create time and cannot
+be changed later.
+
+Note: defining compression format settings which are different from the old
+      default (format=deflate, level=0, window-size=12) will result in the
+      compression format extension being written to the Qcow2 image. Versions
+      of QEMU before 2.11 will not be able to open images with this extension.
+
+@item compress.level
+Valid for compress.format=deflate defines the compression level to use for
+selected compression format. The default of @code{compress.level=0} will use
+the default compression level for the format. Alternate values range from 1 for
+fastest compression to 9 for the best compression.
+
+@item compress.window-size
+Valid for compress.format=deflate defines the compression window size used
+during compression. Valid window sizes for deflate compression range from 8 to
+15 inclusively. The default is @code{compress.window-size=15}.
+
 @end table
 
 @item Other
-- 
1.9.1

[Qemu-devel] [PATCH V5 05/10] block/qcow2: read and write the compress format extension

[Peter Lieven]

we now read the extension on open and write it on update, but
do not yet use it.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/qcow2.c | 92 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 block/qcow2.h | 21 ++++++++++----
 2 files changed, 108 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index a5270a6..7fd52e1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -70,6 +70,7 @@ typedef struct {
 #define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
 #define  QCOW2_EXT_MAGIC_CRYPTO_HEADER 0x0537be77
 #define  QCOW2_EXT_MAGIC_BITMAPS 0x23852875
+#define  QCOW2_EXT_MAGIC_COMPRESS_FORMAT 0xC03183A3
 
 static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
@@ -269,6 +270,57 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
 #endif
             break;
 
+        case QCOW2_EXT_MAGIC_COMPRESS_FORMAT:
+        {
+            Qcow2CompressFormatExt compress_ext;
+            Error *local_err = NULL;
+            if (ext.len != sizeof(compress_ext)) {
+                error_setg(errp, "ERROR: ext_compress_format: len=%"
+                           PRIu32 " invalid (!=%zu)", ext.len,
+                           sizeof(compress_ext));
+                return 2;
+            }
+            ret = bdrv_pread(bs->file, offset, &compress_ext,
+                             ext.len);
+            if (ret < 0) {
+                error_setg_errno(errp, -ret, "ERROR: ext_compress_fromat:"
+                                 " Could not read extension");
+                return 3;
+            }
+
+            s->compress.format =
+                qapi_enum_parse(Qcow2CompressFormat_lookup,
+                                compress_ext.name, QCOW2_COMPRESS_FORMAT__MAX,
+                                -1, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return 4;
+            }
+
+            if (s->compress.format == QCOW2_COMPRESS_FORMAT_DEFLATE) {
+                s->compress.u.deflate.has_level = true;
+                s->compress.u.deflate.level = compress_ext.level;
+                s->compress.u.deflate.has_window_size = true;
+                s->compress.u.deflate.window_size = compress_ext.window_size;
+            }
+
+            qcow2_check_compress_settings(&s->compress, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return 5;
+            }
+
+#ifdef DEBUG_EXT
+            if (s->compress.format == QCOW2_COMPRESS_FORMAT_DEFLATE) {
+                printf("Qcow2: Got compress format %s with level %" PRIu8
+                       " window_size %" PRIu8 "\n",
+                       Qcow2CompressFormat_lookup[s->compress.format],
+                       s->compress.u.deflate.level,
+                       s->compress.u.deflate.window_size);
+            }
+#endif
+            break;
+        }
         case QCOW2_EXT_MAGIC_FEATURE_TABLE:
             if (p_feature_table != NULL) {
                 void* feature_table = g_malloc0(ext.len + 2 * sizeof(Qcow2Feature));
@@ -1403,6 +1455,12 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
     s->cluster_cache_offset = -1;
     s->flags = flags;
 
+    s->compress.format = QCOW2_COMPRESS_FORMAT_DEFLATE;
+    s->compress.u.deflate.has_level = true;
+    s->compress.u.deflate.level = 0;
+    s->compress.u.deflate.has_window_size = true;
+    s->compress.u.deflate.window_size = 12;
+
     ret = qcow2_refcount_init(bs);
     if (ret != 0) {
         error_setg_errno(errp, -ret, "Could not initialize refcount handling");
@@ -2320,6 +2378,30 @@ int qcow2_update_header(BlockDriverState *bs)
         buflen -= ret;
     }
 
+    /* Compress Format header extension */
+    if (s->compress.format != QCOW2_COMPRESS_FORMAT_DEFLATE ||
+        s->compress.u.deflate.level != 0 ||
+        s->compress.u.deflate.window_size != 12) {
+        Qcow2CompressFormatExt ext = {};
+        assert(s->compress.format < QCOW2_COMPRESS_FORMAT__MAX);
+        strncpy((char *) &ext.name,
+                Qcow2CompressFormat_lookup[s->compress.format],
+                sizeof(ext.name));
+        if (s->compress.format == QCOW2_COMPRESS_FORMAT_DEFLATE) {
+            ext.level = s->compress.u.deflate.level;
+            ext.window_size = s->compress.u.deflate.window_size;
+        }
+        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_COMPRESS_FORMAT,
+                             &ext, sizeof(ext),
+                             buflen);
+        if (ret < 0) {
+            goto fail;
+        }
+        buf += ret;
+        buflen -= ret;
+        header->incompatible_features |= cpu_to_be64(QCOW2_INCOMPAT_COMPRESS);
+    }
+
     /* Feature table */
     if (s->qcow_version >= 3) {
         Qcow2Feature features[] = {
@@ -2334,6 +2416,11 @@ int qcow2_update_header(BlockDriverState *bs)
                 .name = "corrupt bit",
             },
             {
+                .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
+                .bit  = QCOW2_INCOMPAT_COMPRESS_BITNR,
+                .name = "compress format bit",
+            },
+            {
                 .type = QCOW2_FEAT_TYPE_COMPATIBLE,
                 .bit  = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
                 .name = "lazy refcounts",
@@ -2855,6 +2942,11 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         abort();
     }
 
+    if (compress) {
+        BDRVQcow2State *s = blk_bs(blk)->opaque;
+        memcpy(&s->compress, compress, sizeof(Qcow2Compress));
+    }
+
     /* Create a full header (including things like feature table) */
     ret = qcow2_update_header(blk_bs(blk));
     if (ret < 0) {
diff --git a/block/qcow2.h b/block/qcow2.h
index 96a8d43..abd35dd 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -188,13 +188,16 @@ enum {
 
 /* Incompatible feature bits */
 enum {
-    QCOW2_INCOMPAT_DIRTY_BITNR   = 0,
-    QCOW2_INCOMPAT_CORRUPT_BITNR = 1,
-    QCOW2_INCOMPAT_DIRTY         = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
-    QCOW2_INCOMPAT_CORRUPT       = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
+    QCOW2_INCOMPAT_DIRTY_BITNR    = 0,
+    QCOW2_INCOMPAT_CORRUPT_BITNR  = 1,
+    QCOW2_INCOMPAT_COMPRESS_BITNR = 2,
+    QCOW2_INCOMPAT_DIRTY          = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
+    QCOW2_INCOMPAT_CORRUPT        = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
+    QCOW2_INCOMPAT_COMPRESS       = 1 << QCOW2_INCOMPAT_COMPRESS_BITNR,
 
     QCOW2_INCOMPAT_MASK          = QCOW2_INCOMPAT_DIRTY
-                                 | QCOW2_INCOMPAT_CORRUPT,
+                                 | QCOW2_INCOMPAT_CORRUPT
+                                 | QCOW2_INCOMPAT_COMPRESS,
 };
 
 /* Compatible feature bits */
@@ -228,6 +231,12 @@ typedef struct Qcow2Feature {
     char    name[46];
 } QEMU_PACKED Qcow2Feature;
 
+typedef struct Qcow2CompressFormatExt {
+    char name[14];
+    uint8_t level;
+    uint8_t window_size;
+} QEMU_PACKED Qcow2CompressFormatExt;
+
 typedef struct Qcow2DiscardRegion {
     BlockDriverState *bs;
     uint64_t offset;
@@ -307,6 +316,8 @@ typedef struct BDRVQcow2State {
     Qcow2GetRefcountFunc *get_refcount;
     Qcow2SetRefcountFunc *set_refcount;
 
+    Qcow2Compress compress;
+
     bool discard_passthrough[QCOW2_DISCARD_MAX];
 
     int overlap_check; /* bitmask of Qcow2MetadataOverlap values */
-- 
1.9.1

[Qemu-devel] [PATCH V5 06/10] block/qcow2: simplify ret usage in qcow2_create

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/qcow2.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 7fd52e1..ffe609d 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3031,7 +3031,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     int refcount_order;
     const char *encryptfmt = NULL;
     Error *local_err = NULL;
-    int ret;
+    int ret = -EINVAL;
 
     /* Read out options */
     size = ROUND_UP(qemu_opt_get_size_del(opts, BLOCK_OPT_SIZE, 0),
@@ -3043,7 +3043,6 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
         if (qemu_opt_get_del(opts, BLOCK_OPT_ENCRYPT)) {
             error_setg(errp, "Options " BLOCK_OPT_ENCRYPT " and "
                        BLOCK_OPT_ENCRYPT_FORMAT " are mutually exclusive");
-            ret = -EINVAL;
             goto finish;
         }
     } else if (qemu_opt_get_bool_del(opts, BLOCK_OPT_ENCRYPT, false)) {
@@ -3052,7 +3051,6 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     cluster_size = qcow2_opt_get_cluster_size_del(opts, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        ret = -EINVAL;
         goto finish;
     }
     buf = qemu_opt_get_del(opts, BLOCK_OPT_PREALLOC);
@@ -3061,14 +3059,12 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
                                &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        ret = -EINVAL;
         goto finish;
     }
 
     version = qcow2_opt_get_version_del(opts, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        ret = -EINVAL;
         goto finish;
     }
 
@@ -3079,21 +3075,18 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     if (backing_file && prealloc != PREALLOC_MODE_OFF) {
         error_setg(errp, "Backing file and preallocation cannot be used at "
                    "the same time");
-        ret = -EINVAL;
         goto finish;
     }
 
     if (version < 3 && (flags & BLOCK_FLAG_LAZY_REFCOUNTS)) {
         error_setg(errp, "Lazy refcounts only supported with compatibility "
                    "level 1.1 and above (use compat=1.1 or greater)");
-        ret = -EINVAL;
         goto finish;
     }
 
     refcount_bits = qcow2_opt_get_refcount_bits_del(opts, version, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        ret = -EINVAL;
         goto finish;
     }
 
@@ -3110,13 +3103,11 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
         visit_start_struct(v, NULL, NULL, 0, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
-            ret = -EINVAL;
             goto finish;
         }
         visit_type_Qcow2Compress_members(v, &compress, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
-            ret = -EINVAL;
             goto finish;
         }
         visit_end_struct(v, NULL);
@@ -3126,12 +3117,12 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
         qcow2_check_compress_settings(&compress, &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
-            ret = -EINVAL;
             goto finish;
         }
         compress_ptr = &compress;
     }
 
+    /* ret is still -EINVAL until here */
     ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
                         cluster_size, prealloc, opts, version, refcount_order,
                         encryptfmt, compress_ptr, &local_err);
-- 
1.9.1

[Qemu-devel] [PATCH V5 07/10] block/qcow2: optimize qcow2_co_pwritev_compressed

[Peter Lieven]

if we specify exactly one iov of s->cluster_size bytes we can avoid
the bounce buffer.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/qcow2.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index ffe609d..a12b3d7 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3428,7 +3428,7 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
     struct iovec iov;
     z_stream strm;
     int ret, out_len;
-    uint8_t *buf, *out_buf;
+    uint8_t *buf, *out_buf, *local_buf = NULL;
     uint64_t cluster_offset;
 
     if (bytes == 0) {
@@ -3438,8 +3438,8 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
         return bdrv_truncate(bs->file, cluster_offset, PREALLOC_MODE_OFF, NULL);
     }
 
-    buf = qemu_blockalign(bs, s->cluster_size);
-    if (bytes != s->cluster_size) {
+    if (bytes != s->cluster_size || qiov->niov != 1) {
+        buf = local_buf = qemu_blockalign(bs, s->cluster_size);
         if (bytes > s->cluster_size ||
             offset + bytes != bs->total_sectors << BDRV_SECTOR_BITS)
         {
@@ -3448,8 +3448,10 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
         }
         /* Zero-pad last write if image size is not cluster aligned */
         memset(buf + bytes, 0, s->cluster_size - bytes);
+        qemu_iovec_to_buf(qiov, 0, buf, bytes);
+    } else {
+        buf = qiov->iov[0].iov_base;
     }
-    qemu_iovec_to_buf(qiov, 0, buf, bytes);
 
     out_buf = g_malloc(s->cluster_size);
 
@@ -3517,7 +3519,7 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
 success:
     ret = 0;
 fail:
-    qemu_vfree(buf);
+    qemu_vfree(local_buf);
     g_free(out_buf);
     return ret;
 }
-- 
1.9.1

[Qemu-devel] [PATCH V5 08/10] block/qcow2: start using the compress format extension

[Peter Lieven]

we now pass the parameters to the zlib compressor if the
extension is present and use the old default values if
the extension is absent.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/qcow2-cluster.c | 58 ++++++++++++++++++++++++++++++---------------------
 block/qcow2.c         | 53 +++++++++++++++++++++++-----------------------
 2 files changed, 61 insertions(+), 50 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f06c08f..345feec 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1479,30 +1479,39 @@ again:
 }
 
 static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
-                             const uint8_t *buf, int buf_size)
+                             const uint8_t *buf, int buf_size,
+                             Qcow2Compress *compress)
 {
-    z_stream strm1, *strm = &strm1;
-    int ret, out_len;
-
-    memset(strm, 0, sizeof(*strm));
-
-    strm->next_in = (uint8_t *)buf;
-    strm->avail_in = buf_size;
-    strm->next_out = out_buf;
-    strm->avail_out = out_buf_size;
-
-    ret = inflateInit2(strm, -12);
-    if (ret != Z_OK)
-        return -1;
-    ret = inflate(strm, Z_FINISH);
-    out_len = strm->next_out - out_buf;
-    if ((ret != Z_STREAM_END && ret != Z_BUF_ERROR) ||
-        out_len != out_buf_size) {
-        inflateEnd(strm);
-        return -1;
-    }
-    inflateEnd(strm);
-    return 0;
+    int ret = 0, out_len;
+
+    switch (compress->format) {
+    case QCOW2_COMPRESS_FORMAT_DEFLATE:
+    {
+        z_stream z_strm = {};
+
+        z_strm.next_in = (uint8_t *)buf;
+        z_strm.avail_in = buf_size;
+        z_strm.next_out = out_buf;
+        z_strm.avail_out = out_buf_size;
+
+        ret = inflateInit2(&z_strm, -compress->u.deflate.window_size);
+        if (ret != Z_OK) {
+            return -1;
+        }
+        ret = inflate(&z_strm, Z_FINISH);
+        out_len = z_strm.next_out - out_buf;
+        ret = -(ret != Z_STREAM_END);
+        inflateEnd(&z_strm);
+        break;
+    }
+    default:
+        abort(); /* should never reach this point */
+    }
+
+    if (out_len != out_buf_size) {
+        ret = -1;
+    }
+    return ret;
 }
 
 int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
@@ -1523,7 +1532,8 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
             return ret;
         }
         if (decompress_buffer(s->cluster_cache, s->cluster_size,
-                              s->cluster_data + sector_offset, csize) < 0) {
+                              s->cluster_data + sector_offset, csize,
+                              &s->compress) < 0) {
             return -EIO;
         }
         s->cluster_cache_offset = coffset;
diff --git a/block/qcow2.c b/block/qcow2.c
index a12b3d7..6031963 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3426,9 +3426,9 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
     BDRVQcow2State *s = bs->opaque;
     QEMUIOVector hd_qiov;
     struct iovec iov;
-    z_stream strm;
-    int ret, out_len;
-    uint8_t *buf, *out_buf, *local_buf = NULL;
+    z_stream z_strm = {};
+    int ret, out_len = 0;
+    uint8_t *buf, *out_buf = NULL, *local_buf = NULL;
     uint64_t cluster_offset;
 
     if (bytes == 0) {
@@ -3453,34 +3453,35 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
         buf = qiov->iov[0].iov_base;
     }
 
-    out_buf = g_malloc(s->cluster_size);
+    switch (s->compress.format) {
+    case QCOW2_COMPRESS_FORMAT_DEFLATE:
+        out_buf = g_malloc(s->cluster_size);
+        ret = deflateInit2(
+            &z_strm, s->compress.u.deflate.level ?
+                     Z_DEFAULT_COMPRESSION : s->compress.u.deflate.level,
+                     Z_DEFLATED, -s->compress.u.deflate.window_size,
+                     9, Z_DEFAULT_STRATEGY);
+        if (ret != Z_OK) {
+            ret = -EINVAL;
+            goto fail;
+        }
 
-    /* best compression, small window, no zlib header */
-    memset(&strm, 0, sizeof(strm));
-    ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION,
-                       Z_DEFLATED, -12,
-                       9, Z_DEFAULT_STRATEGY);
-    if (ret != 0) {
-        ret = -EINVAL;
-        goto fail;
-    }
+        z_strm.avail_in = s->cluster_size;
+        z_strm.next_in = (uint8_t *)buf;
+        z_strm.avail_out = s->cluster_size;
+        z_strm.next_out = out_buf;
 
-    strm.avail_in = s->cluster_size;
-    strm.next_in = (uint8_t *)buf;
-    strm.avail_out = s->cluster_size;
-    strm.next_out = out_buf;
+        ret = deflate(&z_strm, Z_FINISH);
+        out_len = z_strm.next_out - out_buf;
+        deflateEnd(&z_strm);
 
-    ret = deflate(&strm, Z_FINISH);
-    if (ret != Z_STREAM_END && ret != Z_OK) {
-        deflateEnd(&strm);
-        ret = -EINVAL;
-        goto fail;
+        ret = ret != Z_STREAM_END;
+        break;
+    default:
+        abort(); /* should never reach this point */
     }
-    out_len = strm.next_out - out_buf;
-
-    deflateEnd(&strm);
 
-    if (ret != Z_STREAM_END || out_len >= s->cluster_size) {
+    if (ret || out_len >= s->cluster_size) {
         /* could not compress: write normal cluster */
         ret = qcow2_co_pwritev(bs, offset, bytes, qiov, 0);
         if (ret < 0) {
-- 
1.9.1

[Qemu-devel] [PATCH V5 09/10] block/qcow2: add lzo compress format

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/qcow2-cluster.c  | 15 +++++++++++++++
 block/qcow2.c          | 25 ++++++++++++++++++++++++-
 configure              |  2 +-
 docs/interop/qcow2.txt |  2 ++
 qapi/block-core.json   | 15 ++++++++++++---
 qemu-img.texi          |  1 +
 6 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 345feec..f48cded 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -24,6 +24,9 @@
 
 #include "qemu/osdep.h"
 #include <zlib.h>
+#ifdef CONFIG_LZO
+#include <lzo/lzo1x.h>
+#endif
 
 #include "qapi/error.h"
 #include "qemu-common.h"
@@ -1504,6 +1507,18 @@ static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
         inflateEnd(&z_strm);
         break;
     }
+#ifdef CONFIG_LZO
+    case QCOW2_COMPRESS_FORMAT_LZO:
+        out_len = out_buf_size;
+        ret = lzo1x_decompress_safe(buf, buf_size, out_buf,
+                                    (lzo_uint *) &out_len, NULL);
+        if (ret == LZO_E_INPUT_NOT_CONSUMED) {
+            /* We always read up to the next sector boundary. Thus
+             * buf_size may be larger than the original compressed size. */
+            ret = 0;
+        }
+        break;
+#endif
     default:
         abort(); /* should never reach this point */
     }
diff --git a/block/qcow2.c b/block/qcow2.c
index 6031963..0e9c2b8 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -26,6 +26,9 @@
 #include "sysemu/block-backend.h"
 #include "qemu/module.h"
 #include <zlib.h>
+#ifdef CONFIG_LZO
+#include <lzo/lzo1x.h>
+#endif
 #include "block/qcow2.h"
 #include "qemu/error-report.h"
 #include "qapi/qmp/qerror.h"
@@ -310,6 +313,14 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
                 return 5;
             }
 
+#ifdef CONFIG_LZO
+            if (s->compress.format == QCOW2_COMPRESS_FORMAT_LZO &&
+                lzo_init() != LZO_E_OK) {
+                error_setg(errp, "ERROR: internal error - lzo_init() failed");
+                return 6;
+            }
+#endif
+
 #ifdef DEBUG_EXT
             if (s->compress.format == QCOW2_COMPRESS_FORMAT_DEFLATE) {
                 printf("Qcow2: Got compress format %s with level %" PRIu8
@@ -317,6 +328,9 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
                        Qcow2CompressFormat_lookup[s->compress.format],
                        s->compress.u.deflate.level,
                        s->compress.u.deflate.window_size);
+            } else {
+                printf("Qcow2: Got compress format %s\n",
+                       Qcow2CompressFormat_lookup[s->compress.format]);
             }
 #endif
             break;
@@ -3428,7 +3442,7 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
     struct iovec iov;
     z_stream z_strm = {};
     int ret, out_len = 0;
-    uint8_t *buf, *out_buf = NULL, *local_buf = NULL;
+    uint8_t *buf, *out_buf = NULL, *local_buf = NULL, *work_buf = NULL;
     uint64_t cluster_offset;
 
     if (bytes == 0) {
@@ -3477,6 +3491,14 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
 
         ret = ret != Z_STREAM_END;
         break;
+#ifdef CONFIG_LZO
+    case QCOW2_COMPRESS_FORMAT_LZO:
+        out_buf = g_malloc(s->cluster_size + s->cluster_size / 16 + 64 + 3);
+        work_buf = g_malloc(LZO1X_1_MEM_COMPRESS);
+        ret = lzo1x_1_compress(buf, s->cluster_size, out_buf,
+                               (lzo_uint *) &out_len, work_buf);
+        break;
+#endif
     default:
         abort(); /* should never reach this point */
     }
@@ -3522,6 +3544,7 @@ success:
 fail:
     qemu_vfree(local_buf);
     g_free(out_buf);
+    g_free(work_buf);
     return ret;
 }
 
diff --git a/configure b/configure
index 987f59b..c1cee88 100755
--- a/configure
+++ b/configure
@@ -1958,7 +1958,7 @@ if test "$lzo" != "no" ; then
 int main(void) { lzo_version(); return 0; }
 EOF
     if compile_prog "" "-llzo2" ; then
-        libs_softmmu="$libs_softmmu -llzo2"
+        LIBS="$LIBS -llzo2"
         lzo="yes"
     else
         if test "$lzo" = "yes"; then
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index d0d2a8f..3262667 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -335,6 +335,8 @@ The fields of the compress format extension are:
                    deflate: Standard zlib deflate compression without
                             compression header
 
+                   lzo:     LZO1X compression without additional header
+
               14:  compress_level (uint8_t)
 
                    0 = default compress level (valid for all formats, default)
diff --git a/qapi/block-core.json b/qapi/block-core.json
index f652206..d655bb1 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2458,11 +2458,12 @@
 # @Qcow2CompressFormat:
 #
 # @deflate: standard zlib deflate compression
+# @lzo: lzo1x compression
 #
 # Since: 2.11
 ##
 { 'enum': 'Qcow2CompressFormat',
-  'data': [ 'deflate' ] }
+  'data': [ 'deflate', 'lzo' ] }
 
 ##
 # @Qcow2CompressDeflate:
@@ -2479,6 +2480,14 @@
             '*window-size': 'uint8' } }
 
 ##
+# @Qcow2CompressLZO:
+#
+# Since: 2.10
+##
+{ 'struct': 'Qcow2CompressLZO',
+  'data': { } }
+
+##
 # @Qcow2Compress:
 #
 # Specifies the compression format and compression level that should
@@ -2491,8 +2500,8 @@
 { 'union': 'Qcow2Compress',
   'base': { 'format': 'Qcow2CompressFormat' },
   'discriminator': 'format',
-  'data': { 'deflate': 'Qcow2CompressDeflate' } }
-
+  'data': { 'deflate': 'Qcow2CompressDeflate',
+            'lzo': 'Qcow2CompressLZO' } }
 
 ##
 # @BlockdevOptionsSsh:
diff --git a/qemu-img.texi b/qemu-img.texi
index 3612c59..5b0bdbe 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -682,6 +682,7 @@ The following options are available if support for the respective libraries
 has been enabled at compile time:
 
    deflate        Uses standard zlib defalte compression
+   lzo            Uses LZO1X compression
 
 The compression algorithm can only be defined at image create time and cannot
 be changed later.
-- 
1.9.1

[Qemu-devel] [PATCH V5 10/10] block/qcow2: add compress info to image specific info

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/qcow2.c        | 7 +++++++
 qapi/block-core.json | 6 +++++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 0e9c2b8..7e03877 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3968,6 +3968,13 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
         spec_info->u.qcow2.data->encrypt = qencrypt;
     }
 
+    if (s->incompatible_features & QCOW2_INCOMPAT_COMPRESS) {
+        spec_info->u.qcow2.data->compress = g_new0(Qcow2Compress, 1);
+        memcpy(spec_info->u.qcow2.data->compress, &s->compress,
+               sizeof(Qcow2Compress));
+        spec_info->u.qcow2.data->has_compress = true;
+    }
+
     return spec_info;
 }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index d655bb1..3b338d5 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -68,6 +68,9 @@
 # @encrypt: details about encryption parameters; only set if image
 #           is encrypted (since 2.10)
 #
+# @compress: details about parameters for compressed clusters; only set if
+#            the compress format header extension is present (since 2.11)
+#
 # Since: 1.7
 ##
 { 'struct': 'ImageInfoSpecificQCow2',
@@ -76,7 +79,8 @@
       '*lazy-refcounts': 'bool',
       '*corrupt': 'bool',
       'refcount-bits': 'int',
-      '*encrypt': 'ImageInfoSpecificQCow2Encryption'
+      '*encrypt': 'ImageInfoSpecificQCow2Encryption',
+      '*compress': 'Qcow2Compress'
   } }
 
 ##
-- 
1.9.1

Re: [Qemu-devel] [PATCH V5 01/10] specs/qcow2: add compress format extension

[Eric Blake]

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

On 07/25/2017 09:41 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  docs/interop/qcow2.txt | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  roms/ipxe              |  2 +-
>  2 files changed, 51 insertions(+), 2 deletions(-)
> 

> +
> +== Compress format extension ==
> +
> +The compress format extension is an optional header extension. It provides
> +the ability to specify the compress algorithm and compress parameters

s/the compress algorithm/the compression algorithm/

> +that are used for compressed clusters. This new header MUST be present if
> +the incompatible-feature bit "compress format bit" is set and MUST be absent
> +otherwise.
> +
> +The fields of the compress format extension are:
> +
> +    Byte  0 - 13:  compress_format_name (padded with zeros, but not
> +                   necessarily null terminated if it has full length).
> +                   Valid compression format names currently are:
> +
> +                   deflate: Standard zlib deflate compression without
> +                            compression header

Why did you name it "deflate" instead of "zlib" again?

> +
> +              14:  compress_level (uint8_t)
> +
> +                   0 = default compress level (valid for all formats, default)
> +
> +                   Additional valid compression levels for deflate compression:
> +
> +                   All values between 1 and 9. 1 gives best speed, 9 gives best
> +                   compression. The default compression level is defined by zlib
> +                   and currently defaults to 6.
> +
> +              15:  compress_window_size (uint8_t)
> +
> +                   Window or dictionary size used by the compression format.
> +                   Currently only used by the deflate compression algorithm.

What must this be set to for other algorithms?  I guess we get to that
in later patches.

> +
> +                   Valid window sizes for deflate compression range from 8 to
> +                   15 inclusively.
> +
> +Note: Omitting the incompatible "Compress format bit" results in the usage
> +of deflate compression with default compression level and a window size of 12
> +(which was default before QEMU 2.11). If exactly these parameters are choosen

s/choosen/chosen,/

> +it is free to the implementation to omit the "Compress format bit" and the

s/it is free to the implementation to omit/the implementation may omit/

> +compress format extension when updating the QCOW2 header.
> +
> +
>  == Host cluster management ==
>  
>  qcow2 manages the allocation of host clusters by maintaining a reference count
> diff --git a/roms/ipxe b/roms/ipxe
> index 0600d3a..b991c67 160000
> --- a/roms/ipxe
> +++ b/roms/ipxe
> @@ -1 +1 @@
> -Subproject commit 0600d3ae94f93efd10fc6b3c7420a9557a3a1670
> +Subproject commit b991c67c1d91574ef22336cc3a5944d1e63230c9

Oops.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH V5 01/10] specs/qcow2: add compress format extension

[Peter Lieven]

Am 25.07.2017 um 17:03 schrieb Eric Blake:
> On 07/25/2017 09:41 AM, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>  docs/interop/qcow2.txt | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  roms/ipxe              |  2 +-
>>  2 files changed, 51 insertions(+), 2 deletions(-)
>>
>> +
>> +== Compress format extension ==
>> +
>> +The compress format extension is an optional header extension. It provides
>> +the ability to specify the compress algorithm and compress parameters
> s/the compress algorithm/the compression algorithm/
>
>> +that are used for compressed clusters. This new header MUST be present if
>> +the incompatible-feature bit "compress format bit" is set and MUST be absent
>> +otherwise.
>> +
>> +The fields of the compress format extension are:
>> +
>> +    Byte  0 - 13:  compress_format_name (padded with zeros, but not
>> +                   necessarily null terminated if it has full length).
>> +                   Valid compression format names currently are:
>> +
>> +                   deflate: Standard zlib deflate compression without
>> +                            compression header
> Why did you name it "deflate" instead of "zlib" again?

zlib provides raw deflate encoding and gzip encoding both with and without header afaik.
We use raw deflate encoding without header in QEMU. And the name of the algorithm is deflate.
I found it more appropriate than zlib.

>
>> +
>> +              14:  compress_level (uint8_t)
>> +
>> +                   0 = default compress level (valid for all formats, default)
>> +
>> +                   Additional valid compression levels for deflate compression:
>> +
>> +                   All values between 1 and 9. 1 gives best speed, 9 gives best
>> +                   compression. The default compression level is defined by zlib
>> +                   and currently defaults to 6.
>> +
>> +              15:  compress_window_size (uint8_t)
>> +
>> +                   Window or dictionary size used by the compression format.
>> +                   Currently only used by the deflate compression algorithm.
> What must this be set to for other algorithms?  I guess we get to that
> in later patches.

Its only used by the deflate algorithm. At this point we only have that algorithm.
lzo has no such encoding. If we add e.g. lzma it has a dictionary size which is
a power of two (typcial value would be 2^20).

>
>> +
>> +                   Valid window sizes for deflate compression range from 8 to
>> +                   15 inclusively.
>> +
>> +Note: Omitting the incompatible "Compress format bit" results in the usage
>> +of deflate compression with default compression level and a window size of 12
>> +(which was default before QEMU 2.11). If exactly these parameters are choosen
> s/choosen/chosen,/
>
>> +it is free to the implementation to omit the "Compress format bit" and the
> s/it is free to the implementation to omit/the implementation may omit/
>
>> +compress format extension when updating the QCOW2 header.
>> +
>> +
>>  == Host cluster management ==
>>  
>>  qcow2 manages the allocation of host clusters by maintaining a reference count
>> diff --git a/roms/ipxe b/roms/ipxe
>> index 0600d3a..b991c67 160000
>> --- a/roms/ipxe
>> +++ b/roms/ipxe
>> @@ -1 +1 @@
>> -Subproject commit 0600d3ae94f93efd10fc6b3c7420a9557a3a1670
>> +Subproject commit b991c67c1d91574ef22336cc3a5944d1e63230c9
> Oops.

Yes, oops. But I guess I have to respin anyway ;-)

Can you also have a look at the remainder of the series?

Thanks,
Peter

Re: [Qemu-devel] [PATCH V5 01/10] specs/qcow2: add compress format extension

[Eric Blake]

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

On 07/25/2017 03:29 PM, Peter Lieven wrote:

>>> +                   deflate: Standard zlib deflate compression without
>>> +                            compression header
>> Why did you name it "deflate" instead of "zlib" again?
> 
> zlib provides raw deflate encoding and gzip encoding both with and without header afaik.
> We use raw deflate encoding without header in QEMU. And the name of the algorithm is deflate.
> I found it more appropriate than zlib.

Okay, I can live with that.

> 
>>
>>> +
>>> +              14:  compress_level (uint8_t)
>>> +
>>> +                   0 = default compress level (valid for all formats, default)
>>> +
>>> +                   Additional valid compression levels for deflate compression:
>>> +
>>> +                   All values between 1 and 9. 1 gives best speed, 9 gives best
>>> +                   compression. The default compression level is defined by zlib
>>> +                   and currently defaults to 6.
>>> +
>>> +              15:  compress_window_size (uint8_t)
>>> +
>>> +                   Window or dictionary size used by the compression format.
>>> +                   Currently only used by the deflate compression algorithm.
>> What must this be set to for other algorithms?  I guess we get to that
>> in later patches.
> 
> Its only used by the deflate algorithm. At this point we only have that algorithm.
> lzo has no such encoding. If we add e.g. lzma it has a dictionary size which is
> a power of two (typcial value would be 2^20).

So for lzo, we should document that the field must be 0 (when we get to
adding it).

> 
> Can you also have a look at the remainder of the series?

It's on my list for 2.11 reviews, although 2.10 freeze stuff is taking
priority, so it may be a few days yet.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH V5 02/10] qapi/block-core: add Qcow2Compress parameters

[Eric Blake]

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

On 07/25/2017 09:41 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qapi/block-core.json | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 833c602..f652206 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -2455,6 +2455,46 @@
>              '*encrypt': 'BlockdevQcow2Encryption' } }
>  
>  ##
> +# @Qcow2CompressFormat:
> +#
> +# @deflate: standard zlib deflate compression
> +#
> +# Since: 2.11
> +##
> +{ 'enum': 'Qcow2CompressFormat',
> +  'data': [ 'deflate' ] }
> +
> +##
> +# @Qcow2CompressDeflate:
> +#
> +# @level: specifies the compression level. 0 = default compression,
> +#         1 = fastest compression, 9 = best compresion

By putting level here instead of in the common base type, you'll have to
re-implement it for each compression type - but that's also okay since
they (might) have different ranges of valid levels.

s/compresion/compression/

maybe: 0 = default compression (same as level 6), 1=...


> +# @window-size: specifies the window size used for deflate compression.
> +#               8...15 = window size of 2^8 to 2^15 byte (default)

15 is the new default, but it might be worth an explicit mention that a
window of 12 is required for back-compat to older images.

> +#
> +# Since: 2.11
> +##
> +{ 'struct': 'Qcow2CompressDeflate',
> +  'data': { '*level': 'uint8',
> +            '*window-size': 'uint8' } }
> +
> +##
> +# @Qcow2Compress:
> +#
> +# Specifies the compression format and compression level that should
> +# be used for compressed Qcow2 clusters.
> +#
> +# @format: specifies the compression format to use. (defaults to zlib)

s/zlib/deflate/

> +#
> +# Since: 2.11
> +##
> +{ 'union': 'Qcow2Compress',
> +  'base': { 'format': 'Qcow2CompressFormat' },
> +  'discriminator': 'format',
> +  'data': { 'deflate': 'Qcow2CompressDeflate' } }
> +
> +
> +##
>  # @BlockdevOptionsSsh:
>  #
>  # @server:              host address
> 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH V5 03/10] block/qcow2: parse compress create options

[Eric Blake]

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

On 07/25/2017 09:41 AM, Peter Lieven wrote:
> this adds parsing and validation for the compress create
> options. They are only validated but not yet used.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/qcow2.c             | 86 +++++++++++++++++++++++++++++++++++++++++++++--
>  include/block/block_int.h | 39 +++++++++++----------
>  2 files changed, 105 insertions(+), 20 deletions(-)
> 

> +static void
> +qcow2_check_compress_settings(Qcow2Compress *compress, Error **errp)
> +{
> +    if (compress->format == QCOW2_COMPRESS_FORMAT_DEFLATE) {
> +        if (!compress->u.deflate.has_level) {
> +            compress->u.deflate.has_level = true;
> +            compress->u.deflate.level = 0;
> +        }
> +        if (compress->u.deflate.level > 9) {
> +            error_setg(errp, "Compress level %" PRIu8 " is not supported for"
> +                       " format '%s'", compress->u.deflate.level,
> +                       Qcow2CompressFormat_lookup[compress->format]);
> +            return;
> +        }
> +        if (!compress->u.deflate.has_window_size) {
> +            compress->u.deflate.has_window_size = true;
> +            compress->u.deflate.window_size = 15;

Maybe for 2.11 we should finally get around to adding parameter defaults
directly in QMP (you would then not need a has_FOO for any parameter FOO
that can specify its own default).  Not your problem to solve, though.

> +        }
> +        if (compress->u.deflate.window_size < 8 ||
> +            compress->u.deflate.window_size > 15) {
> +            error_setg(errp, "Compress window size %" PRIu8 " is not supported"
> +                       " for format '%s'", compress->u.deflate.window_size,
> +                       Qcow2CompressFormat_lookup[compress->format]);

Should we allow a window_size of 0 in the UI (meaning pick a sane
non-zero default)?  It's odd that you allow 0 for level but not for
window-size.

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

Dead return statement, since the end of the function occurs anyways.
But arguably it helps future maintenance if more tests were to be added
later?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH V5 04/10] qemu-img: add documentation for compress settings

[Eric Blake]

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

On 07/25/2017 09:41 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  qemu-img.texi | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/qemu-img.texi b/qemu-img.texi
> index 72dabd6..3612c59 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -676,6 +676,32 @@ file which is COW and has data blocks already, it couldn't be changed to NOCOW
>  by setting @code{nocow=on}. One can issue @code{lsattr filename} to check if
>  the NOCOW flag is set or not (Capital 'C' is NOCOW flag).
>  
> +@item compress.format
> +Defines which compression algorithm is should be used for compressed clusters.
> +The following options are available if support for the respective libraries
> +has been enabled at compile time:
> +
> +   deflate        Uses standard zlib defalte compression
> +
> +The compression algorithm can only be defined at image create time and cannot
> +be changed later.

We can't change it via 'qemu-img amend'?  Might be an interesting
feature to add later: in-place re-compression to a new algorithm - but
since compression is global to the file, you have to be careful that
aborting the operation in the middle doesn't mix the two compressions.
Or put another way, we have to use at least twice the disk space for all
compressed clusters, and only at the end of things update the headers to
point to the new clusters and away from the old (so that an early abort
just treats all the new clusters as leaked); possibly by amending from
compressed to uncompressed back to compressed.

Oh, and why is 'qemu-img --help' mostly alphabetical, except that
'amend' comes after 'resize'?  Guess I'll do a quickie patch for that.


> +
> +Note: defining compression format settings which are different from the old
> +      default (format=deflate, level=0, window-size=12) will result in the
> +      compression format extension being written to the Qcow2 image. Versions
> +      of QEMU before 2.11 will not be able to open images with this extension.
> +
> +@item compress.level
> +Valid for compress.format=deflate defines the compression level to use for
> +selected compression format. The default of @code{compress.level=0} will use
> +the default compression level for the format. Alternate values range from 1 for
> +fastest compression to 9 for the best compression.
> +
> +@item compress.window-size
> +Valid for compress.format=deflate defines the compression window size used
> +during compression. Valid window sizes for deflate compression range from 8 to
> +15 inclusively. The default is @code{compress.window-size=15}.

Maybe emphasize that larger is generally better (smaller exists for the
long-gone days of small machines, and for back-compat usage of 12 in
older qemu).  In fact, if we wanted, we could declare that qcow2's use
of deflate can ONLY use 12 or 15, instead of anything in the range 8-15,
if we though that would be easier to maintain.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH V5 09/10] block/qcow2: add lzo compress format

[Eric Blake]

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

On 07/25/2017 09:41 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/qcow2-cluster.c  | 15 +++++++++++++++
>  block/qcow2.c          | 25 ++++++++++++++++++++++++-
>  configure              |  2 +-
>  docs/interop/qcow2.txt |  2 ++
>  qapi/block-core.json   | 15 ++++++++++++---
>  qemu-img.texi          |  1 +
>  6 files changed, 55 insertions(+), 5 deletions(-)
> 

> +++ b/docs/interop/qcow2.txt
> @@ -335,6 +335,8 @@ The fields of the compress format extension are:
>                     deflate: Standard zlib deflate compression without
>                              compression header
>  
> +                   lzo:     LZO1X compression without additional header
> +
>                14:  compress_level (uint8_t)
>  
>                     0 = default compress level (valid for all formats, default)

What do compress_level and compress_window have to be for lzo?  (If they
are not useful, document that it must be 0 - which you've already done
for compress_level).


> +# @Qcow2CompressLZO:
> +#
> +# Since: 2.10

2.11

> +##
> +{ 'struct': 'Qcow2CompressLZO',
> +  'data': { } }
> +
> +##
>  # @Qcow2Compress:
>  #
>  # Specifies the compression format and compression level that should
> @@ -2491,8 +2500,8 @@
>  { 'union': 'Qcow2Compress',
>    'base': { 'format': 'Qcow2CompressFormat' },
>    'discriminator': 'format',
> -  'data': { 'deflate': 'Qcow2CompressDeflate' } }
> -
> +  'data': { 'deflate': 'Qcow2CompressDeflate',
> +            'lzo': 'Qcow2CompressLZO' } }
>  

Was the loss of the blank line intentional?

> +++ b/qemu-img.texi
> @@ -682,6 +682,7 @@ The following options are available if support for the respective libraries
>  has been enabled at compile time:
>  
>     deflate        Uses standard zlib defalte compression
> +   lzo            Uses LZO1X compression

The .json file spelled it lzo1x; is there a difference implied by the
choice of capitalization?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH V5 10/10] block/qcow2: add compress info to image specific info

[Eric Blake]

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

On 07/25/2017 09:41 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/qcow2.c        | 7 +++++++
>  qapi/block-core.json | 6 +++++-
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> +++ b/qapi/block-core.json
> @@ -68,6 +68,9 @@
>  # @encrypt: details about encryption parameters; only set if image
>  #           is encrypted (since 2.10)
>  #
> +# @compress: details about parameters for compressed clusters; only set if
> +#            the compress format header extension is present (since 2.11)

Thinking aloud:

I still think this is better as "only set if the image uses compressed
headers" (similar to encrypt).  That is, I would like this output to be
present even when opening legacy deflate/0/12 images.

But thinking more, what I am REALLY asking for is "if any cluster is
compressed, then this information is present; if nothing is compressed,
the choice of compression header doesn't matter".  But we don't have a
quick-and-dirty way to tell if an image has any compressed clusters,
short of examining every L2 map (including maps inside internal snapshots).

Hmm - we already have 'Dirty bit' and 'Corrupt bit' as
incompatible_features that can quickly identify certain image
properties; do we want another bit set to 1 for images known to use
compressed clusters?  Or even make it an auto-clear bit (or even a pair
of auto-clear bits: one to designate that this image was written by a
new-enough qemu that promises to mark the image if any compressed
clusters exist, and the other is set only if such clusters do exist;
that way, if both bits are clear, we know we have to walk L2 tables to
look for compressed clusters, but if the first bit is set, then the
second bit is reliable)?

So maybe this means we are still debating spec ideas.  Kevin, do you
want to chime in?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH V5 10/10] block/qcow2: add compress info to image specific info

[Peter Lieven]

Am 25.07.2017 um 23:55 schrieb Eric Blake:
> On 07/25/2017 09:41 AM, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>  block/qcow2.c        | 7 +++++++
>>  qapi/block-core.json | 6 +++++-
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> +++ b/qapi/block-core.json
>> @@ -68,6 +68,9 @@
>>  # @encrypt: details about encryption parameters; only set if image
>>  #           is encrypted (since 2.10)
>>  #
>> +# @compress: details about parameters for compressed clusters; only set if
>> +#            the compress format header extension is present (since 2.11)
> Thinking aloud:
>
> I still think this is better as "only set if the image uses compressed
> headers" (similar to encrypt).  That is, I would like this output to be
> present even when opening legacy deflate/0/12 images.
>
> But thinking more, what I am REALLY asking for is "if any cluster is
> compressed, then this information is present; if nothing is compressed,
> the choice of compression header doesn't matter".  But we don't have a
> quick-and-dirty way to tell if an image has any compressed clusters,
> short of examining every L2 map (including maps inside internal snapshots).
>
> Hmm - we already have 'Dirty bit' and 'Corrupt bit' as
> incompatible_features that can quickly identify certain image
> properties; do we want another bit set to 1 for images known to use
> compressed clusters?  Or even make it an auto-clear bit (or even a pair
> of auto-clear bits: one to designate that this image was written by a
> new-enough qemu that promises to mark the image if any compressed
> clusters exist, and the other is set only if such clusters do exist;
> that way, if both bits are clear, we know we have to walk L2 tables to
> look for compressed clusters, but if the first bit is set, then the
> second bit is reliable)?
>
> So maybe this means we are still debating spec ideas.  Kevin, do you
> want to chime in?
>

I would also vote for adding an information to the image if it contains
compressed clusters. 2 bits sounds reasonable. One to promise we set
the compressed bit if we write any compressed cluster and one to tell
that there are actually compressed cluster. If we set the "promise" bit
at create time we can simply hook up in qcow2_co_pwritev_comperssed
and set the bit there as soon as we write any compressed cluster.

The compression info I would display unconditionally. Because the compress
settings are made at create time and are there even if we don't have compressed
clusters yet.

Peter

Re: [Qemu-devel] [PATCH V5 10/10] block/qcow2: add compress info to image specific info

[Peter Lieven]

Am 25.07.2017 um 23:55 schrieb Eric Blake:
> On 07/25/2017 09:41 AM, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>  block/qcow2.c        | 7 +++++++
>>  qapi/block-core.json | 6 +++++-
>>  2 files changed, 12 insertions(+), 1 deletion(-)
>>
>> +++ b/qapi/block-core.json
>> @@ -68,6 +68,9 @@
>>  # @encrypt: details about encryption parameters; only set if image
>>  #           is encrypted (since 2.10)
>>  #
>> +# @compress: details about parameters for compressed clusters; only set if
>> +#            the compress format header extension is present (since 2.11)
> Thinking aloud:
>
> I still think this is better as "only set if the image uses compressed
> headers" (similar to encrypt).  That is, I would like this output to be
> present even when opening legacy deflate/0/12 images.
>
> But thinking more, what I am REALLY asking for is "if any cluster is
> compressed, then this information is present; if nothing is compressed,
> the choice of compression header doesn't matter".  But we don't have a
> quick-and-dirty way to tell if an image has any compressed clusters,
> short of examining every L2 map (including maps inside internal snapshots).
>
> Hmm - we already have 'Dirty bit' and 'Corrupt bit' as
> incompatible_features that can quickly identify certain image
> properties; do we want another bit set to 1 for images known to use
> compressed clusters?  Or even make it an auto-clear bit (or even a pair
> of auto-clear bits: one to designate that this image was written by a
> new-enough qemu that promises to mark the image if any compressed
> clusters exist, and the other is set only if such clusters do exist;
> that way, if both bits are clear, we know we have to walk L2 tables to
> look for compressed clusters, but if the first bit is set, then the
> second bit is reliable)?
>
> So maybe this means we are still debating spec ideas.  Kevin, do you
> want to chime in?
>
Hi all,


I would really like to move this forward and get at least the spec for

the QCOW2 Extension Header fixed. I internally need this feature and would

like not to produce incompatible images.

Maybe we can even decide later if we add further bits for tracking if

there are compressed cluster and keep this indepented of this series.


Thanks,
Peter

Re: [Qemu-devel] [PATCH V5 10/10] block/qcow2: add compress info to image specific info

[Kevin Wolf]

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

Am 25.07.2017 um 23:55 hat Eric Blake geschrieben:
> On 07/25/2017 09:41 AM, Peter Lieven wrote:
> > Signed-off-by: Peter Lieven <pl@kamp.de>
> > ---
> >  block/qcow2.c        | 7 +++++++
> >  qapi/block-core.json | 6 +++++-
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> > 
> > +++ b/qapi/block-core.json
> > @@ -68,6 +68,9 @@
> >  # @encrypt: details about encryption parameters; only set if image
> >  #           is encrypted (since 2.10)
> >  #
> > +# @compress: details about parameters for compressed clusters; only set if
> > +#            the compress format header extension is present (since 2.11)
> 
> Thinking aloud:
> 
> I still think this is better as "only set if the image uses compressed
> headers" (similar to encrypt).  That is, I would like this output to be
> present even when opening legacy deflate/0/12 images.
> 
> But thinking more, what I am REALLY asking for is "if any cluster is
> compressed, then this information is present; if nothing is compressed,
> the choice of compression header doesn't matter".  But we don't have a
> quick-and-dirty way to tell if an image has any compressed clusters,
> short of examining every L2 map (including maps inside internal snapshots).
> 
> Hmm - we already have 'Dirty bit' and 'Corrupt bit' as
> incompatible_features that can quickly identify certain image
> properties; do we want another bit set to 1 for images known to use
> compressed clusters?  Or even make it an auto-clear bit (or even a pair
> of auto-clear bits: one to designate that this image was written by a
> new-enough qemu that promises to mark the image if any compressed
> clusters exist, and the other is set only if such clusters do exist;
> that way, if both bits are clear, we know we have to walk L2 tables to
> look for compressed clusters, but if the first bit is set, then the
> second bit is reliable)?
> 
> So maybe this means we are still debating spec ideas.  Kevin, do you
> want to chime in?

I'm not sure if this would be an useful extension. Adding feature bits
is a relatively big change and I don't see what it actually buys us
here.

First of all, what use is the information that an image contains
compressed clusters? For any practical use that I could imagine, there
is a big difference between an image that contains one compressed
cluster and an image that has 100% compressed clusters - much bigger
than between the image with one compressed cluster and an image that is
fully uncompressed.

But if we assume for a moment that the information is useful: For old
images, we would still be in the same situation as today and couldn't
tell whether an image contains compressed clusters or not. For new
images that are meant for new qemu versions, we can have the compression
header, which is probably as close as we can get (at least I don't think
you suggested something that would unset the compressed bit as soon as
the last compressed cluster gets uncompressed through copy-on-write).

That leaves us with images created by new qemu versions that are meant
to be consumed by old versions so that we can't set the incompatible
feature bit. If we are concerned about these, couldn't we simply specify
that a compression header that contains the current defaults MAY exist
even without the incompatible feature bit?

Kevin

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

Re: [Qemu-devel] [PATCH V5 01/10] specs/qcow2: add compress format extension

[Kevin Wolf]

Am 25.07.2017 um 16:41 hat Peter Lieven geschrieben:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  docs/interop/qcow2.txt | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  roms/ipxe              |  2 +-
>  2 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index d7fdb1f..d0d2a8f 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -86,7 +86,12 @@ in the description of a field.
>                                  be written to (unless for regaining
>                                  consistency).
>  
> -                    Bits 2-63:  Reserved (set to 0)
> +                    Bit 2:      Compress format bit.  If and only if this bit
> +                                is set then the compress format extension
> +                                MUST be present and MUST be parsed and checked
> +                                for compatibility.
> +
> +                    Bits 3-63:  Reserved (set to 0)
>  
>           80 -  87:  compatible_features
>                      Bitmask of compatible features. An implementation can
> @@ -137,6 +142,7 @@ be stored. Each extension has a structure like the following:
>                          0x6803f857 - Feature name table
>                          0x23852875 - Bitmaps extension
>                          0x0537be77 - Full disk encryption header pointer
> +                        0xC03183A3 - Compress format extension
>                          other      - Unknown header extension, can be safely
>                                       ignored
>  
> @@ -311,6 +317,49 @@ The algorithms used for encryption vary depending on the method
>     in the LUKS header, with the physical disk sector as the
>     input tweak.
>  
> +
> +== Compress format extension ==
> +
> +The compress format extension is an optional header extension. It provides
> +the ability to specify the compress algorithm and compress parameters
> +that are used for compressed clusters. This new header MUST be present if
> +the incompatible-feature bit "compress format bit" is set and MUST be absent
> +otherwise.
> +
> +The fields of the compress format extension are:
> +
> +    Byte  0 - 13:  compress_format_name (padded with zeros, but not
> +                   necessarily null terminated if it has full length).
> +                   Valid compression format names currently are:
> +
> +                   deflate: Standard zlib deflate compression without
> +                            compression header
> +
> +              14:  compress_level (uint8_t)
> +
> +                   0 = default compress level (valid for all formats, default)
> +
> +                   Additional valid compression levels for deflate compression:
> +
> +                   All values between 1 and 9. 1 gives best speed, 9 gives best
> +                   compression. The default compression level is defined by zlib
> +                   and currently defaults to 6.
> +
> +              15:  compress_window_size (uint8_t)
> +
> +                   Window or dictionary size used by the compression format.
> +                   Currently only used by the deflate compression algorithm.
> +
> +                   Valid window sizes for deflate compression range from 8 to
> +                   15 inclusively.

So what is the plan with respect to adding new compression algorithms?

If I understand correctly, we'll use the same extension type
(0xC03183A3) and a different compress_format_name. However, the other
algorithm will likely have different options and also a different number
of options. Will the meaning of bytes 14-end then depend on the specific
algorithm?

Part of why I'm asking this is because I wonder why compress_format_name
is 14 characters. It looks to me as if that is intended to make the
header a round 16 bytes for the deflate algorithm. But unless we say
"16 bits ought to be enough for every algorithm", this is just
optimising a special case. (Or actually not optimising, but just moving
the padding from the end of the header extension to padding inside the
compress_format_name field.)

Wouldn't 8 bytes still be plenty of space for a format name, and look
more natural? Then any format that requires options would have a little
more space without immediately going to a 24 byte header extension.

Kevin

Re: [Qemu-devel] [PATCH V5 01/10] specs/qcow2: add compress format extension

[Peter Lieven]

Am 11.09.2017 um 16:22 schrieb Kevin Wolf:
> Am 25.07.2017 um 16:41 hat Peter Lieven geschrieben:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   docs/interop/qcow2.txt | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>   roms/ipxe              |  2 +-
>>   2 files changed, 51 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index d7fdb1f..d0d2a8f 100644
>> --- a/docs/interop/qcow2.txt
>> +++ b/docs/interop/qcow2.txt
>> @@ -86,7 +86,12 @@ in the description of a field.
>>                                   be written to (unless for regaining
>>                                   consistency).
>>   
>> -                    Bits 2-63:  Reserved (set to 0)
>> +                    Bit 2:      Compress format bit.  If and only if this bit
>> +                                is set then the compress format extension
>> +                                MUST be present and MUST be parsed and checked
>> +                                for compatibility.
>> +
>> +                    Bits 3-63:  Reserved (set to 0)
>>   
>>            80 -  87:  compatible_features
>>                       Bitmask of compatible features. An implementation can
>> @@ -137,6 +142,7 @@ be stored. Each extension has a structure like the following:
>>                           0x6803f857 - Feature name table
>>                           0x23852875 - Bitmaps extension
>>                           0x0537be77 - Full disk encryption header pointer
>> +                        0xC03183A3 - Compress format extension
>>                           other      - Unknown header extension, can be safely
>>                                        ignored
>>   
>> @@ -311,6 +317,49 @@ The algorithms used for encryption vary depending on the method
>>      in the LUKS header, with the physical disk sector as the
>>      input tweak.
>>   
>> +
>> +== Compress format extension ==
>> +
>> +The compress format extension is an optional header extension. It provides
>> +the ability to specify the compress algorithm and compress parameters
>> +that are used for compressed clusters. This new header MUST be present if
>> +the incompatible-feature bit "compress format bit" is set and MUST be absent
>> +otherwise.
>> +
>> +The fields of the compress format extension are:
>> +
>> +    Byte  0 - 13:  compress_format_name (padded with zeros, but not
>> +                   necessarily null terminated if it has full length).
>> +                   Valid compression format names currently are:
>> +
>> +                   deflate: Standard zlib deflate compression without
>> +                            compression header
>> +
>> +              14:  compress_level (uint8_t)
>> +
>> +                   0 = default compress level (valid for all formats, default)
>> +
>> +                   Additional valid compression levels for deflate compression:
>> +
>> +                   All values between 1 and 9. 1 gives best speed, 9 gives best
>> +                   compression. The default compression level is defined by zlib
>> +                   and currently defaults to 6.
>> +
>> +              15:  compress_window_size (uint8_t)
>> +
>> +                   Window or dictionary size used by the compression format.
>> +                   Currently only used by the deflate compression algorithm.
>> +
>> +                   Valid window sizes for deflate compression range from 8 to
>> +                   15 inclusively.
> So what is the plan with respect to adding new compression algorithms?
>
> If I understand correctly, we'll use the same extension type
> (0xC03183A3) and a different compress_format_name. However, the other
> algorithm will likely have different options and also a different number
> of options. Will the meaning of bytes 14-end then depend on the specific
> algorithm?

The idea of the current options is that almost every algorithm will have
a compression level setting and most have a dictionary or window
size. This is why I added them to the common header.

>
> Part of why I'm asking this is because I wonder why compress_format_name
> is 14 characters. It looks to me as if that is intended to make the
> header a round 16 bytes for the deflate algorithm. But unless we say
> "16 bits ought to be enough for every algorithm", this is just
> optimising a special case. (Or actually not optimising, but just moving
> the padding from the end of the header extension to padding inside the
> compress_format_name field.)
>
> Wouldn't 8 bytes still be plenty of space for a format name, and look
> more natural? Then any format that requires options would have a little
> more space without immediately going to a 24 byte header extension.

Sure 8 characters will still be enough. I can respin the series with
an updated header if you like.

Thanks,
Peter

Re: [Qemu-devel] [PATCH V5 01/10] specs/qcow2: add compress format extension

[Kevin Wolf]

Am 18.09.2017 um 12:09 hat Peter Lieven geschrieben:
> Am 11.09.2017 um 16:22 schrieb Kevin Wolf:
> > Am 25.07.2017 um 16:41 hat Peter Lieven geschrieben:
> > > Signed-off-by: Peter Lieven <pl@kamp.de>
> > > ---
> > >   docs/interop/qcow2.txt | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
> > >   roms/ipxe              |  2 +-
> > >   2 files changed, 51 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> > > index d7fdb1f..d0d2a8f 100644
> > > --- a/docs/interop/qcow2.txt
> > > +++ b/docs/interop/qcow2.txt
> > > @@ -86,7 +86,12 @@ in the description of a field.
> > >                                   be written to (unless for regaining
> > >                                   consistency).
> > > -                    Bits 2-63:  Reserved (set to 0)
> > > +                    Bit 2:      Compress format bit.  If and only if this bit
> > > +                                is set then the compress format extension
> > > +                                MUST be present and MUST be parsed and checked
> > > +                                for compatibility.
> > > +
> > > +                    Bits 3-63:  Reserved (set to 0)
> > >            80 -  87:  compatible_features
> > >                       Bitmask of compatible features. An implementation can
> > > @@ -137,6 +142,7 @@ be stored. Each extension has a structure like the following:
> > >                           0x6803f857 - Feature name table
> > >                           0x23852875 - Bitmaps extension
> > >                           0x0537be77 - Full disk encryption header pointer
> > > +                        0xC03183A3 - Compress format extension
> > >                           other      - Unknown header extension, can be safely
> > >                                        ignored
> > > @@ -311,6 +317,49 @@ The algorithms used for encryption vary depending on the method
> > >      in the LUKS header, with the physical disk sector as the
> > >      input tweak.
> > > +
> > > +== Compress format extension ==
> > > +
> > > +The compress format extension is an optional header extension. It provides
> > > +the ability to specify the compress algorithm and compress parameters
> > > +that are used for compressed clusters. This new header MUST be present if
> > > +the incompatible-feature bit "compress format bit" is set and MUST be absent
> > > +otherwise.
> > > +
> > > +The fields of the compress format extension are:
> > > +
> > > +    Byte  0 - 13:  compress_format_name (padded with zeros, but not
> > > +                   necessarily null terminated if it has full length).
> > > +                   Valid compression format names currently are:
> > > +
> > > +                   deflate: Standard zlib deflate compression without
> > > +                            compression header
> > > +
> > > +              14:  compress_level (uint8_t)
> > > +
> > > +                   0 = default compress level (valid for all formats, default)
> > > +
> > > +                   Additional valid compression levels for deflate compression:
> > > +
> > > +                   All values between 1 and 9. 1 gives best speed, 9 gives best
> > > +                   compression. The default compression level is defined by zlib
> > > +                   and currently defaults to 6.
> > > +
> > > +              15:  compress_window_size (uint8_t)
> > > +
> > > +                   Window or dictionary size used by the compression format.
> > > +                   Currently only used by the deflate compression algorithm.
> > > +
> > > +                   Valid window sizes for deflate compression range from 8 to
> > > +                   15 inclusively.
> > So what is the plan with respect to adding new compression algorithms?
> > 
> > If I understand correctly, we'll use the same extension type
> > (0xC03183A3) and a different compress_format_name. However, the other
> > algorithm will likely have different options and also a different number
> > of options. Will the meaning of bytes 14-end then depend on the specific
> > algorithm?
> 
> The idea of the current options is that almost every algorithm will have
> a compression level setting and most have a dictionary or window
> size. This is why I added them to the common header.

It's this kind of assumptions that lead to awkward interfaces in the
long run, because if you say "almost every" case looks like this, you
can be sure that one day you'll get one of the remaining cases where the
field becomes useless.

Also, while most formats may support a compress_level, it is also likely
that each of them uses a different range of values and a different
default. So they look very similar, but are in fact different.

I think this is best dealt with by treating these options as specific to
the format, and if many formats coincide to have a field with the same
name at the same place, so be it.

If one day we finally get to a state where 'qemu-img create' options are
expressed in the QAPI schema, I would include the fields in the
individual branches of the union type (with a documentation what the
exact semantics are for the specific format) rather than in the base
type where you have to explain the semantics without actually referring
to the branches.

> > Part of why I'm asking this is because I wonder why compress_format_name
> > is 14 characters. It looks to me as if that is intended to make the
> > header a round 16 bytes for the deflate algorithm. But unless we say
> > "16 bits ought to be enough for every algorithm", this is just
> > optimising a special case. (Or actually not optimising, but just moving
> > the padding from the end of the header extension to padding inside the
> > compress_format_name field.)
> > 
> > Wouldn't 8 bytes still be plenty of space for a format name, and look
> > more natural? Then any format that requires options would have a little
> > more space without immediately going to a 24 byte header extension.
> 
> Sure 8 characters will still be enough. I can respin the series with
> an updated header if you like.

Yes, I would prefer this.

Kevin

Re: [Qemu-devel] [PATCH V5 01/10] specs/qcow2: add compress format extension

[Peter Lieven]

Am 18.09.2017 um 12:50 schrieb Kevin Wolf:
> Am 18.09.2017 um 12:09 hat Peter Lieven geschrieben:
>> Am 11.09.2017 um 16:22 schrieb Kevin Wolf:
>>> Am 25.07.2017 um 16:41 hat Peter Lieven geschrieben:
>>>> Signed-off-by: Peter Lieven <pl@kamp.de>
>>>> ---
>>>>   docs/interop/qcow2.txt | 51 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>>>   roms/ipxe              |  2 +-
>>>>   2 files changed, 51 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>>>> index d7fdb1f..d0d2a8f 100644
>>>> --- a/docs/interop/qcow2.txt
>>>> +++ b/docs/interop/qcow2.txt
>>>> @@ -86,7 +86,12 @@ in the description of a field.
>>>>                                   be written to (unless for regaining
>>>>                                   consistency).
>>>> -                    Bits 2-63:  Reserved (set to 0)
>>>> +                    Bit 2:      Compress format bit.  If and only if this bit
>>>> +                                is set then the compress format extension
>>>> +                                MUST be present and MUST be parsed and checked
>>>> +                                for compatibility.
>>>> +
>>>> +                    Bits 3-63:  Reserved (set to 0)
>>>>            80 -  87:  compatible_features
>>>>                       Bitmask of compatible features. An implementation can
>>>> @@ -137,6 +142,7 @@ be stored. Each extension has a structure like the following:
>>>>                           0x6803f857 - Feature name table
>>>>                           0x23852875 - Bitmaps extension
>>>>                           0x0537be77 - Full disk encryption header pointer
>>>> +                        0xC03183A3 - Compress format extension
>>>>                           other      - Unknown header extension, can be safely
>>>>                                        ignored
>>>> @@ -311,6 +317,49 @@ The algorithms used for encryption vary depending on the method
>>>>      in the LUKS header, with the physical disk sector as the
>>>>      input tweak.
>>>> +
>>>> +== Compress format extension ==
>>>> +
>>>> +The compress format extension is an optional header extension. It provides
>>>> +the ability to specify the compress algorithm and compress parameters
>>>> +that are used for compressed clusters. This new header MUST be present if
>>>> +the incompatible-feature bit "compress format bit" is set and MUST be absent
>>>> +otherwise.
>>>> +
>>>> +The fields of the compress format extension are:
>>>> +
>>>> +    Byte  0 - 13:  compress_format_name (padded with zeros, but not
>>>> +                   necessarily null terminated if it has full length).
>>>> +                   Valid compression format names currently are:
>>>> +
>>>> +                   deflate: Standard zlib deflate compression without
>>>> +                            compression header
>>>> +
>>>> +              14:  compress_level (uint8_t)
>>>> +
>>>> +                   0 = default compress level (valid for all formats, default)
>>>> +
>>>> +                   Additional valid compression levels for deflate compression:
>>>> +
>>>> +                   All values between 1 and 9. 1 gives best speed, 9 gives best
>>>> +                   compression. The default compression level is defined by zlib
>>>> +                   and currently defaults to 6.
>>>> +
>>>> +              15:  compress_window_size (uint8_t)
>>>> +
>>>> +                   Window or dictionary size used by the compression format.
>>>> +                   Currently only used by the deflate compression algorithm.
>>>> +
>>>> +                   Valid window sizes for deflate compression range from 8 to
>>>> +                   15 inclusively.
>>> So what is the plan with respect to adding new compression algorithms?
>>>
>>> If I understand correctly, we'll use the same extension type
>>> (0xC03183A3) and a different compress_format_name. However, the other
>>> algorithm will likely have different options and also a different number
>>> of options. Will the meaning of bytes 14-end then depend on the specific
>>> algorithm?
>> The idea of the current options is that almost every algorithm will have
>> a compression level setting and most have a dictionary or window
>> size. This is why I added them to the common header.
> It's this kind of assumptions that lead to awkward interfaces in the
> long run, because if you say "almost every" case looks like this, you
> can be sure that one day you'll get one of the remaining cases where the
> field becomes useless.
>
> Also, while most formats may support a compress_level, it is also likely
> that each of them uses a different range of values and a different
> default. So they look very similar, but are in fact different.
>
> I think this is best dealt with by treating these options as specific to
> the format, and if many formats coincide to have a field with the same
> name at the same place, so be it.
>
> If one day we finally get to a state where 'qemu-img create' options are
> expressed in the QAPI schema, I would include the fields in the
> individual branches of the union type (with a documentation what the
> exact semantics are for the specific format) rather than in the base
> type where you have to explain the semantics without actually referring
> to the branches.
>
>>> Part of why I'm asking this is because I wonder why compress_format_name
>>> is 14 characters. It looks to me as if that is intended to make the
>>> header a round 16 bytes for the deflate algorithm. But unless we say
>>> "16 bits ought to be enough for every algorithm", this is just
>>> optimising a special case. (Or actually not optimising, but just moving
>>> the padding from the end of the header extension to padding inside the
>>> compress_format_name field.)
>>>
>>> Wouldn't 8 bytes still be plenty of space for a format name, and look
>>> more natural? Then any format that requires options would have a little
>>> more space without immediately going to a 24 byte header extension.
>> Sure 8 characters will still be enough. I can respin the series with
>> an updated header if you like.
> Yes, I would prefer this.

Somehow, I forgot to respin this series. I will send a new version for 2.12

Peter

>
> Kevin