[Qemu-devel] [PATCH 0/4] block/qcow2: add compression_algorithm create option

[Qemu-devel] [PATCH 0/4] block/qcow2: add compression_algorithm create option

[Peter Lieven]

this adds a create option for QCOW2 images to specify the compression algorithm
for compressed clusters. The series adds 3 algorithms to choose from:
zlib (default), zlib-fast and lzo. zlib-fast optimizes the zlib parameters
without the need for an additional compression library. lzo is choosen
as we already link against it and its widely available. Further
libraries like zstd are in preparation.

Compression time vs. size of an uncompressed Debian 9 QCOW2 image (size 1148MB):

zlib         35.7s     339MB
zlib-fast    12.8s     348MB
lzo          4.2s      429MB

Peter Lieven (4):
  block/qcow2: add compression_algorithm create option
  block/qcow2: optimize qcow2_co_pwritev_compressed
  block/qcow2: add lzo compression algorithm
  block/qcow2: add zlib-fast compression algorithm

 block/qcow2-cluster.c     |  66 ++++++++++------
 block/qcow2.c             | 186 +++++++++++++++++++++++++++++++++++++---------
 block/qcow2.h             |  22 ++++--
 configure                 |   2 +-
 docs/interop/qcow2.txt    |   8 +-
 include/block/block_int.h |  35 ++++-----
 qemu-img.texi             |  12 +++
 7 files changed, 251 insertions(+), 80 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/4] block/qcow2: add compression_algorithm create option

[Peter Lieven]

this patch adds a new compression_algorithm option when creating qcow2 images.
The current default for the compresison algorithm is zlib and zlib will be
used when this option is omitted (like before).

If the option is specified e.g. with:

 qemu-img create -f qcow2 -o compression_algorithm=zlib image.qcow2 1G

then a new compression algorithm header extension is added and an incompatible
feature bit is set. This means that if the header is present it must be parsed
by Qemu on qcow2_open and it must be validated if the specified compression
algorithm is supported by the current build of Qemu.

This means if the compression_algorithm option is specified Qemu prior to this
commit will not be able to open the created image.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/qcow2.c             | 93 ++++++++++++++++++++++++++++++++++++++++++++---
 block/qcow2.h             | 20 +++++++---
 docs/interop/qcow2.txt    |  8 +++-
 include/block/block_int.h | 35 +++++++++---------
 qemu-img.texi             | 10 +++++
 5 files changed, 138 insertions(+), 28 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2f94f03..893b145 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -60,9 +60,11 @@ typedef struct {
     uint32_t len;
 } QEMU_PACKED QCowExtension;
 
-#define  QCOW2_EXT_MAGIC_END 0
-#define  QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
-#define  QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
+#define QCOW2_EXT_MAGIC_END                   0
+#define QCOW2_EXT_MAGIC_BACKING_FORMAT        0xE2792ACA
+#define QCOW2_EXT_MAGIC_FEATURE_TABLE         0x6803f857
+#define QCOW2_EXT_MAGIC_COMPRESSION_ALGORITHM 0xc0318300
+/* 0xc03183xx reserved for further use of compression algorithm parameters */
 
 static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
 {
@@ -76,6 +78,15 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
         return 0;
 }
 
+static uint32_t is_compression_algorithm_supported(char *algorithm)
+{
+    if (!algorithm[0] || !strcmp(algorithm, "zlib")) {
+        /* no algorithm means the old default of zlib compression
+         * with 12 window bits */
+        return QCOW2_COMPRESSION_ZLIB;
+    }
+    return 0;
+}
 
 /* 
  * read qcow2 extension and fill bs
@@ -148,6 +159,34 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
 #endif
             break;
 
+        case QCOW2_EXT_MAGIC_COMPRESSION_ALGORITHM:
+            if (ext.len >= sizeof(s->compression_algorithm)) {
+                error_setg(errp, "ERROR: ext_compression_algorithm: len=%"
+                           PRIu32 " too large (>=%zu)", ext.len,
+                           sizeof(s->compression_algorithm));
+                return 2;
+            }
+            ret = bdrv_pread(bs->file, offset, s->compression_algorithm,
+                             ext.len);
+            if (ret < 0) {
+                error_setg_errno(errp, -ret, "ERROR: ext_compression_algorithm:"
+                                 " Could not read algorithm name");
+                return 3;
+            }
+            s->compression_algorithm[ext.len] = '\0';
+            s->compression_algorithm_id =
+                is_compression_algorithm_supported(s->compression_algorithm);
+            if (!s->compression_algorithm_id) {
+                error_setg(errp, "ERROR: compression algorithm '%s' is "
+                           " unsupported", s->compression_algorithm);
+                return 4;
+            }
+#ifdef DEBUG_EXT
+            printf("Qcow2: Got compression algorithm %s\n",
+                   s->compression_algorithm);
+#endif
+            break;
+
         case QCOW2_EXT_MAGIC_FEATURE_TABLE:
             if (p_feature_table != NULL) {
                 void* feature_table = g_malloc0(ext.len + 2 * sizeof(Qcow2Feature));
@@ -1104,6 +1143,7 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
 
     s->cluster_cache_offset = -1;
     s->flags = flags;
+    s->compression_algorithm_id = QCOW2_COMPRESSION_ZLIB;
 
     ret = qcow2_refcount_init(bs);
     if (ret != 0) {
@@ -1981,6 +2021,21 @@ int qcow2_update_header(BlockDriverState *bs)
         buflen -= ret;
     }
 
+    /* Compression Algorithm header extension */
+    if (s->compression_algorithm[0]) {
+        ret = header_ext_add(buf, QCOW2_EXT_MAGIC_COMPRESSION_ALGORITHM,
+                             s->compression_algorithm,
+                             strlen(s->compression_algorithm),
+                             buflen);
+        if (ret < 0) {
+            goto fail;
+        }
+        buf += ret;
+        buflen -= ret;
+        header->incompatible_features |=
+            cpu_to_be64(QCOW2_INCOMPAT_COMPRESSION);
+    }
+
     /* Feature table */
     if (s->qcow_version >= 3) {
         Qcow2Feature features[] = {
@@ -1995,6 +2050,11 @@ int qcow2_update_header(BlockDriverState *bs)
                 .name = "corrupt bit",
             },
             {
+                .type = QCOW2_FEAT_TYPE_INCOMPATIBLE,
+                .bit  = QCOW2_INCOMPAT_COMPRESSION_BITNR,
+                .name = "compression algorithm",
+            },
+            {
                 .type = QCOW2_FEAT_TYPE_COMPATIBLE,
                 .bit  = QCOW2_COMPAT_LAZY_REFCOUNTS_BITNR,
                 .name = "lazy refcounts",
@@ -2144,7 +2204,7 @@ 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,
-                         Error **errp)
+                         char *compression_algorithm, Error **errp)
 {
     int cluster_bits;
     QDict *options;
@@ -2332,6 +2392,12 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         abort();
     }
 
+    if (compression_algorithm[0]) {
+        BDRVQcow2State *s = blk_bs(blk)->opaque;
+        memcpy(s->compression_algorithm, compression_algorithm,
+               strlen(compression_algorithm));
+    }
+
     /* Create a full header (including things like feature table) */
     ret = qcow2_update_header(blk_bs(blk));
     if (ret < 0) {
@@ -2395,6 +2461,7 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
     char *backing_file = NULL;
     char *backing_fmt = NULL;
     char *buf = NULL;
+    char *compression_algorithm = NULL;
     uint64_t size = 0;
     int flags = 0;
     size_t cluster_size = DEFAULT_CLUSTER_SIZE;
@@ -2475,15 +2542,25 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
 
     refcount_order = ctz32(refcount_bits);
 
+    compression_algorithm = qemu_opt_get_del(opts,
+                                             BLOCK_OPT_COMPRESSION_ALGORITHM);
+    if (!is_compression_algorithm_supported(compression_algorithm)) {
+        error_setg(errp, "Compression algorithm '%s' is not supported",
+                   compression_algorithm);
+        ret = -EINVAL;
+        goto finish;
+    }
+
     ret = qcow2_create2(filename, size, backing_file, backing_fmt, flags,
                         cluster_size, prealloc, opts, version, refcount_order,
-                        &local_err);
+                        compression_algorithm, &local_err);
     error_propagate(errp, local_err);
 
 finish:
     g_free(backing_file);
     g_free(backing_fmt);
     g_free(buf);
+    g_free(compression_algorithm);
     return ret;
 }
 
@@ -3458,6 +3535,12 @@ static QemuOptsList qcow2_create_opts = {
             .help = "Width of a reference count entry in bits",
             .def_value_str = "16"
         },
+        {
+            .name = BLOCK_OPT_COMPRESSION_ALGORITHM,
+            .type = QEMU_OPT_STRING,
+            .help = "Compression algorithm used for compressed clusters",
+            .def_value_str = ""
+        },
         { /* end of list */ }
     }
 };
diff --git a/block/qcow2.h b/block/qcow2.h
index 87b15eb..1c9ba06 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -171,6 +171,10 @@ typedef struct Qcow2UnknownHeaderExtension {
 } Qcow2UnknownHeaderExtension;
 
 enum {
+    QCOW2_COMPRESSION_ZLIB          = 0xC0318301,
+};
+
+enum {
     QCOW2_FEAT_TYPE_INCOMPATIBLE    = 0,
     QCOW2_FEAT_TYPE_COMPATIBLE      = 1,
     QCOW2_FEAT_TYPE_AUTOCLEAR       = 2,
@@ -178,13 +182,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_COMPRESSION_BITNR  = 2,
+    QCOW2_INCOMPAT_DIRTY              = 1 << QCOW2_INCOMPAT_DIRTY_BITNR,
+    QCOW2_INCOMPAT_CORRUPT            = 1 << QCOW2_INCOMPAT_CORRUPT_BITNR,
+    QCOW2_INCOMPAT_COMPRESSION        = 1 << QCOW2_INCOMPAT_COMPRESSION_BITNR,
 
     QCOW2_INCOMPAT_MASK          = QCOW2_INCOMPAT_DIRTY
-                                 | QCOW2_INCOMPAT_CORRUPT,
+                                 | QCOW2_INCOMPAT_CORRUPT
+                                 | QCOW2_INCOMPAT_COMPRESSION,
 };
 
 /* Compatible feature bits */
@@ -294,6 +301,9 @@ typedef struct BDRVQcow2State {
      * override) */
     char *image_backing_file;
     char *image_backing_format;
+
+    char compression_algorithm[16];
+    uint32_t compression_algorithm_id;
 } BDRVQcow2State;
 
 typedef struct Qcow2COWRegion {
diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 80cdfd0..1f165d6 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -85,7 +85,11 @@ in the description of a field.
                                 be written to (unless for regaining
                                 consistency).
 
-                    Bits 2-63:  Reserved (set to 0)
+                    Bit 2:      Compress algorithm bit.  If this bit is set then
+                                the compress algorithm extension must be parsed
+                                and checked for compatiblity.
+
+                    Bits 3-63:  Reserved (set to 0)
 
          80 -  87:  compatible_features
                     Bitmask of compatible features. An implementation can
@@ -135,6 +139,8 @@ be stored. Each extension has a structure like the following:
                         0xE2792ACA - Backing file format name
                         0x6803f857 - Feature name table
                         0x23852875 - Bitmaps extension
+                        0xC0318300 - Compression Algorithm
+                        0xC03183xx - Reserved for compression algorithm params
                         other      - Unknown header extension, can be safely
                                      ignored
 
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 15fa602..03a4b8f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -40,23 +40,24 @@
 #define BLOCK_FLAG_ENCRYPT          1
 #define BLOCK_FLAG_LAZY_REFCOUNTS   8
 
-#define BLOCK_OPT_SIZE              "size"
-#define BLOCK_OPT_ENCRYPT           "encryption"
-#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_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_COMPRESSION_ALGORITHM "compression_algorithm"
 
 #define BLOCK_PROBE_BUF_SIZE        512
 
diff --git a/qemu-img.texi b/qemu-img.texi
index 5b925ec..c0d1bec 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -621,6 +621,16 @@ 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 compression_algorithm
+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:
+
+   zlib            Uses standard zlib compression (default)
+
+The compression algorithm can only be defined at image create time and cannot
+be changed later.
+
 @end table
 
 @item Other
-- 
1.9.1

[Qemu-devel] [PATCH 2/4] 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 893b145..c91eb1f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2716,7 +2716,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) {
@@ -2726,8 +2726,8 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
         return bdrv_truncate(bs->file, cluster_offset, 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)
         {
@@ -2736,8 +2736,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);
 
@@ -2805,7 +2807,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 3/4] block/qcow2: add lzo compression algorithm

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/qcow2-cluster.c | 65 +++++++++++++++++++++++++++++++---------------
 block/qcow2.c         | 72 ++++++++++++++++++++++++++++++++++-----------------
 block/qcow2.h         |  1 +
 configure             |  2 +-
 qemu-img.texi         |  1 +
 5 files changed, 95 insertions(+), 46 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 3d341fd..ecb059b 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"
@@ -1521,30 +1524,49 @@ 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,
+                             uint32_t compression_algorithm_id)
 {
     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) {
+    int ret = 0, out_len;
+
+    switch (compression_algorithm_id) {
+    case QCOW2_COMPRESSION_ZLIB:
+        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;
+        ret = -(ret != Z_STREAM_END);
         inflateEnd(strm);
-        return -1;
+        break;
+#ifdef CONFIG_LZO
+    case QCOW2_COMPRESSION_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 */
     }
-    inflateEnd(strm);
-    return 0;
+    if (out_len != out_buf_size) {
+        ret = -1;
+    }
+    return ret;
 }
 
 int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset)
@@ -1565,7 +1587,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->compression_algorithm_id) < 0) {
             return -EIO;
         }
         s->cluster_cache_offset = coffset;
diff --git a/block/qcow2.c b/block/qcow2.c
index c91eb1f..bd65582 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"
@@ -84,6 +87,10 @@ static uint32_t is_compression_algorithm_supported(char *algorithm)
         /* no algorithm means the old default of zlib compression
          * with 12 window bits */
         return QCOW2_COMPRESSION_ZLIB;
+#ifdef CONFIG_LZO
+    } else if (!strcmp(algorithm, "lzo")) {
+        return QCOW2_COMPRESSION_LZO;
+#endif
     }
     return 0;
 }
@@ -2715,8 +2722,8 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
     QEMUIOVector hd_qiov;
     struct iovec iov;
     z_stream strm;
-    int ret, out_len;
-    uint8_t *buf, *out_buf, *local_buf = NULL;
+    int ret, out_len = 0;
+    uint8_t *buf, *out_buf = NULL, *local_buf = NULL, *work_buf = NULL;
     uint64_t cluster_offset;
 
     if (bytes == 0) {
@@ -2741,34 +2748,50 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
         buf = qiov->iov[0].iov_base;
     }
 
-    out_buf = g_malloc(s->cluster_size);
+    switch (s->compression_algorithm_id) {
+    case QCOW2_COMPRESSION_ZLIB:
+        out_buf = g_malloc(s->cluster_size);
 
-    /* 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;
-    }
+        /* 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;
+        }
 
-    strm.avail_in = s->cluster_size;
-    strm.next_in = (uint8_t *)buf;
-    strm.avail_out = s->cluster_size;
-    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(&strm, Z_FINISH);
+        if (ret != Z_STREAM_END && ret != Z_OK) {
+            deflateEnd(&strm);
+            ret = -EINVAL;
+            goto fail;
+        }
+        out_len = strm.next_out - out_buf;
 
-    ret = deflate(&strm, Z_FINISH);
-    if (ret != Z_STREAM_END && ret != Z_OK) {
         deflateEnd(&strm);
-        ret = -EINVAL;
-        goto fail;
-    }
-    out_len = strm.next_out - out_buf;
 
-    deflateEnd(&strm);
+        ret = ret != Z_STREAM_END;
+        break;
+#ifdef CONFIG_LZO
+    case QCOW2_COMPRESSION_LZO:
+        out_buf = g_malloc(s->cluster_size + s->cluster_size / 64 + 16 + 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 */
+    }
 
-    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) {
@@ -2809,6 +2832,7 @@ success:
 fail:
     qemu_vfree(local_buf);
     g_free(out_buf);
+    g_free(work_buf);
     return ret;
 }
 
diff --git a/block/qcow2.h b/block/qcow2.h
index 1c9ba06..716012c 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -172,6 +172,7 @@ typedef struct Qcow2UnknownHeaderExtension {
 
 enum {
     QCOW2_COMPRESSION_ZLIB          = 0xC0318301,
+    QCOW2_COMPRESSION_LZO           = 0xC0318302,
 };
 
 enum {
diff --git a/configure b/configure
index c571ad1..81d3286 100755
--- a/configure
+++ b/configure
@@ -1890,7 +1890,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/qemu-img.texi b/qemu-img.texi
index c0d1bec..043c1ba 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -627,6 +627,7 @@ The following options are available if support for the respective libraries
 has been enabled at compile time:
 
    zlib            Uses standard zlib compression (default)
+   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 4/4] block/qcow2: add zlib-fast compression algorithm

[Peter Lieven]

this adds support for optimized zlib settings which almost
tripples the compression speed while maintaining about
the same compressed size.

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 block/qcow2-cluster.c |  3 ++-
 block/qcow2.c         | 11 +++++++++--
 block/qcow2.h         |  1 +
 qemu-img.texi         |  1 +
 4 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index ecb059b..d8e2378 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1532,6 +1532,7 @@ static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
 
     switch (compression_algorithm_id) {
     case QCOW2_COMPRESSION_ZLIB:
+    case QCOW2_COMPRESSION_ZLIB_FAST:
         memset(strm, 0, sizeof(*strm));
 
         strm->next_in = (uint8_t *)buf;
@@ -1539,7 +1540,7 @@ static int decompress_buffer(uint8_t *out_buf, int out_buf_size,
         strm->next_out = out_buf;
         strm->avail_out = out_buf_size;
 
-        ret = inflateInit2(strm, -12);
+        ret = inflateInit2(strm, -15);
         if (ret != Z_OK) {
             return -1;
         }
diff --git a/block/qcow2.c b/block/qcow2.c
index bd65582..f07d8f0 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -87,6 +87,8 @@ static uint32_t is_compression_algorithm_supported(char *algorithm)
         /* no algorithm means the old default of zlib compression
          * with 12 window bits */
         return QCOW2_COMPRESSION_ZLIB;
+    } else if (!strcmp(algorithm, "zlib-fast")) {
+        return QCOW2_COMPRESSION_ZLIB_FAST;
 #ifdef CONFIG_LZO
     } else if (!strcmp(algorithm, "lzo")) {
         return QCOW2_COMPRESSION_LZO;
@@ -2722,6 +2724,7 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
     QEMUIOVector hd_qiov;
     struct iovec iov;
     z_stream strm;
+    int z_level = Z_DEFAULT_COMPRESSION, z_windowBits = -12;
     int ret, out_len = 0;
     uint8_t *buf, *out_buf = NULL, *local_buf = NULL, *work_buf = NULL;
     uint64_t cluster_offset;
@@ -2749,13 +2752,17 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
     }
 
     switch (s->compression_algorithm_id) {
+    case QCOW2_COMPRESSION_ZLIB_FAST:
+        z_level = Z_BEST_SPEED;
+        z_windowBits = -15;
+        /* fall-through */
     case QCOW2_COMPRESSION_ZLIB:
         out_buf = g_malloc(s->cluster_size);
 
         /* best compression, small window, no zlib header */
         memset(&strm, 0, sizeof(strm));
-        ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION,
-                           Z_DEFLATED, -12,
+        ret = deflateInit2(&strm, z_level,
+                           Z_DEFLATED, z_windowBits,
                            9, Z_DEFAULT_STRATEGY);
         if (ret != 0) {
             ret = -EINVAL;
diff --git a/block/qcow2.h b/block/qcow2.h
index 716012c..a89f986 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -173,6 +173,7 @@ typedef struct Qcow2UnknownHeaderExtension {
 enum {
     QCOW2_COMPRESSION_ZLIB          = 0xC0318301,
     QCOW2_COMPRESSION_LZO           = 0xC0318302,
+    QCOW2_COMPRESSION_ZLIB_FAST     = 0xC0318303,
 };
 
 enum {
diff --git a/qemu-img.texi b/qemu-img.texi
index 043c1ba..83a5db2 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -627,6 +627,7 @@ The following options are available if support for the respective libraries
 has been enabled at compile time:
 
    zlib            Uses standard zlib compression (default)
+   zlib-fast       Uses zlib compression with optimized compression parameters
    lzo             Uses LZO1X compression
 
 The compression algorithm can only be defined at image create time and cannot
-- 
1.9.1

Re: [Qemu-devel] [PATCH 1/4] block/qcow2: add compression_algorithm create option

[Eric Blake]

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

On 06/27/2017 07:34 AM, Peter Lieven wrote:
> this patch adds a new compression_algorithm option when creating qcow2 images.
> The current default for the compresison algorithm is zlib and zlib will be

s/compresison/compression/

> used when this option is omitted (like before).
> 
> If the option is specified e.g. with:
> 
>  qemu-img create -f qcow2 -o compression_algorithm=zlib image.qcow2 1G
> 
> then a new compression algorithm header extension is added and an incompatible
> feature bit is set. This means that if the header is present it must be parsed
> by Qemu on qcow2_open and it must be validated if the specified compression
> algorithm is supported by the current build of Qemu.
> 
> This means if the compression_algorithm option is specified Qemu prior to this
> commit will not be able to open the created image.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/qcow2.c             | 93 ++++++++++++++++++++++++++++++++++++++++++++---
>  block/qcow2.h             | 20 +++++++---
>  docs/interop/qcow2.txt    |  8 +++-

Focusing on just the spec change first:

> +++ b/docs/interop/qcow2.txt
> @@ -85,7 +85,11 @@ in the description of a field.
>                                  be written to (unless for regaining
>                                  consistency).
>  
> -                    Bits 2-63:  Reserved (set to 0)
> +                    Bit 2:      Compress algorithm bit.  If this bit is set then
> +                                the compress algorithm extension must be parsed
> +                                and checked for compatiblity.

s/compatiblity/compatibility/

> +
> +                    Bits 3-63:  Reserved (set to 0)
>  
>           80 -  87:  compatible_features
>                      Bitmask of compatible features. An implementation can
> @@ -135,6 +139,8 @@ be stored. Each extension has a structure like the following:
>                          0xE2792ACA - Backing file format name
>                          0x6803f857 - Feature name table
>                          0x23852875 - Bitmaps extension
> +                        0xC0318300 - Compression Algorithm
> +                        0xC03183xx - Reserved for compression algorithm params

s/params/parameters/

You have now introduced 256 different reserved headers, without
documenting any of their formats.  You absolutely MUST include a
documentation of how the new 0xC0318300 header is laid out (see, for
example, our section on "Bitmaps extension"), along with text mentioning
that the new header MUST be present if incompatible-feature bit is set
and MUST be absent otherwise.  But I also think that with a bit of
proper design work, you only need ONE header for all possible algorithm
parameters, rather than burning an additional 255 unspecified
reservations.  That is, make sure your new header includes a common
prefix including a length field and the algorightm in use, and then the
length covers a variable-length suffix that can be parsed in a
per-algorithm-specific manner for whatever additional parameters are
needed for that algorithm.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH 4/4] block/qcow2: add zlib-fast compression algorithm

[Eric Blake]

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

On 06/27/2017 07:34 AM, Peter Lieven wrote:
> this adds support for optimized zlib settings which almost

Start sentences with a capital.

> tripples the compression speed while maintaining about

s/tripples/triples/

> the same compressed size.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/qcow2-cluster.c |  3 ++-
>  block/qcow2.c         | 11 +++++++++--
>  block/qcow2.h         |  1 +
>  qemu-img.texi         |  1 +
>  4 files changed, 13 insertions(+), 3 deletions(-)
> 

> +++ b/block/qcow2.h
> @@ -173,6 +173,7 @@ typedef struct Qcow2UnknownHeaderExtension {
>  enum {
>      QCOW2_COMPRESSION_ZLIB          = 0xC0318301,
>      QCOW2_COMPRESSION_LZO           = 0xC0318302,
> +    QCOW2_COMPRESSION_ZLIB_FAST     = 0xC0318303,

Back to my comments on 1/4 - we MUST first get the qcow2 specification
right, rather than adding undocumented headers in the code.  And I still
think you only need one variable-length header extension for covering
all possible algorithms, rather than one header per algorithm.  Let's
get the spec right first, before worrying about the code implementing
the spec.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH 4/4] block/qcow2: add zlib-fast compression algorithm

[Peter Lieven]

Am 27.06.2017 um 14:53 schrieb Eric Blake:
> On 06/27/2017 07:34 AM, Peter Lieven wrote:
>> this adds support for optimized zlib settings which almost
> Start sentences with a capital.
>
>> tripples the compression speed while maintaining about
> s/tripples/triples/
>
>> the same compressed size.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block/qcow2-cluster.c |  3 ++-
>>   block/qcow2.c         | 11 +++++++++--
>>   block/qcow2.h         |  1 +
>>   qemu-img.texi         |  1 +
>>   4 files changed, 13 insertions(+), 3 deletions(-)
>>
>> +++ b/block/qcow2.h
>> @@ -173,6 +173,7 @@ typedef struct Qcow2UnknownHeaderExtension {
>>   enum {
>>       QCOW2_COMPRESSION_ZLIB          = 0xC0318301,
>>       QCOW2_COMPRESSION_LZO           = 0xC0318302,
>> +    QCOW2_COMPRESSION_ZLIB_FAST     = 0xC0318303,
> Back to my comments on 1/4 - we MUST first get the qcow2 specification
> right, rather than adding undocumented headers in the code.  And I still
> think you only need one variable-length header extension for covering
> all possible algorithms, rather than one header per algorithm.  Let's
> get the spec right first, before worrying about the code implementing
> the spec.
>

Okay, I think someone came up with the idea to have an optional
header per algorithm, but you are right one header with an optional
parameter payload will also do.

I will split the spec change to a separate patch in V2 to make it easier
to respin.

Thanks,
Peter

Re: [Qemu-devel] [PATCH 4/4] block/qcow2: add zlib-fast compression algorithm

[Daniel P. Berrange]

On Tue, Jun 27, 2017 at 02:34:10PM +0200, Peter Lieven wrote:
> this adds support for optimized zlib settings which almost
> tripples the compression speed while maintaining about
> the same compressed size.
> 
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/qcow2-cluster.c |  3 ++-
>  block/qcow2.c         | 11 +++++++++--
>  block/qcow2.h         |  1 +
>  qemu-img.texi         |  1 +
>  4 files changed, 13 insertions(+), 3 deletions(-)
> diff --git a/qemu-img.texi b/qemu-img.texi
> index 043c1ba..83a5db2 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -627,6 +627,7 @@ The following options are available if support for the respective libraries
>  has been enabled at compile time:
>  
>     zlib            Uses standard zlib compression (default)
> +   zlib-fast       Uses zlib compression with optimized compression parameters

This looks like a poor design from a future proofing POV. I would much
rather see us introduce a more flexible modelling of compression at the
QAPI layer which lets us have tunables for each algorith, in the same
way that the qcow2 built-in encryption now has ability to set tunables
for each algorithm.

eg your "zlib-fast" impl which just uses zlib with a window size of -15
could be expressed as

   qemu-img create -o compress.format=zlib,compress.window=-15



>     lzo             Uses LZO1X compression


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/4] block/qcow2: add compression_algorithm create option

[Daniel P. Berrange]

On Tue, Jun 27, 2017 at 02:34:07PM +0200, Peter Lieven wrote:
> this patch adds a new compression_algorithm option when creating qcow2 images.
> The current default for the compresison algorithm is zlib and zlib will be
> used when this option is omitted (like before).
> 
> If the option is specified e.g. with:
> 
>  qemu-img create -f qcow2 -o compression_algorithm=zlib image.qcow2 1G

IMHO we should introduce a nested struct "compress" struct to hold the format
name, and any other format specific arguments, in a way that maps nicely to
any future QAPI representmatch of create options. eg

{ 'enum': 'BlockdevQcow2CompressFormat',
  'data': [ 'zlib', 'lzo' ] }

{ 'union': 'BlockdevQcow2Compress',
  'base': { 'format': 'BlockdevQcow2CompressFormat' },
  'discriminator': 'format',
  'data': { 'zlib': 'BlockdevQcow2CompressZLib',
            'lzo': 'BlockdevQcow2CompressLZO'} }

so it would map to

 qemu-img create -f qcow2 -o compress.format=zlib image.qcow2 1G

and let us have other compress.XXXX options specific to each format

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 4/4] block/qcow2: add zlib-fast compression algorithm

[Peter Lieven]

Am 27.06.2017 um 15:16 schrieb Daniel P. Berrange:
> On Tue, Jun 27, 2017 at 02:34:10PM +0200, Peter Lieven wrote:
>> this adds support for optimized zlib settings which almost
>> tripples the compression speed while maintaining about
>> the same compressed size.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block/qcow2-cluster.c |  3 ++-
>>   block/qcow2.c         | 11 +++++++++--
>>   block/qcow2.h         |  1 +
>>   qemu-img.texi         |  1 +
>>   4 files changed, 13 insertions(+), 3 deletions(-)
>> diff --git a/qemu-img.texi b/qemu-img.texi
>> index 043c1ba..83a5db2 100644
>> --- a/qemu-img.texi
>> +++ b/qemu-img.texi
>> @@ -627,6 +627,7 @@ The following options are available if support for the respective libraries
>>   has been enabled at compile time:
>>   
>>      zlib            Uses standard zlib compression (default)
>> +   zlib-fast       Uses zlib compression with optimized compression parameters
> This looks like a poor design from a future proofing POV. I would much
> rather see us introduce a more flexible modelling of compression at the
> QAPI layer which lets us have tunables for each algorith, in the same
> way that the qcow2 built-in encryption now has ability to set tunables
> for each algorithm.
>
> eg your "zlib-fast" impl which just uses zlib with a window size of -15
> could be expressed as
>
>     qemu-img create -o compress.format=zlib,compress.window=-15

It would also need a compress.level, but okay. This will make things
more complicated as one might not know what good values for
the algoritm are.

Wouldn't it be better to have sth like compress.level=1..9 and let
the implementation decide which parameters it choose for the algorithm?

Do you have a pointer where I can find out how to imlement that
in QAPI? Maybe the patches that introduces the encryption settings?

Thanks,
Peter

Re: [Qemu-devel] [PATCH 1/4] block/qcow2: add compression_algorithm create option

[Peter Lieven]

Am 27.06.2017 um 15:20 schrieb Daniel P. Berrange:
> On Tue, Jun 27, 2017 at 02:34:07PM +0200, Peter Lieven wrote:
>> this patch adds a new compression_algorithm option when creating qcow2 images.
>> The current default for the compresison algorithm is zlib and zlib will be
>> used when this option is omitted (like before).
>>
>> If the option is specified e.g. with:
>>
>>   qemu-img create -f qcow2 -o compression_algorithm=zlib image.qcow2 1G
> IMHO we should introduce a nested struct "compress" struct to hold the format
> name, and any other format specific arguments, in a way that maps nicely to
> any future QAPI representmatch of create options. eg
>
> { 'enum': 'BlockdevQcow2CompressFormat',
>    'data': [ 'zlib', 'lzo' ] }
>
> { 'union': 'BlockdevQcow2Compress',
>    'base': { 'format': 'BlockdevQcow2CompressFormat' },
>    'discriminator': 'format',
>    'data': { 'zlib': 'BlockdevQcow2CompressZLib',
>              'lzo': 'BlockdevQcow2CompressLZO'} }
>
> so it would map to
>
>   qemu-img create -f qcow2 -o compress.format=zlib image.qcow2 1G
>
> and let us have other compress.XXXX options specific to each format

Or would it be possible to start with just a compress.level (int) parameter.
In fact that would be sufficient for almost all formats (or better use algorithms?).
The windowBits can be default to -15 in the future. It seems the old choice of -12
was just not optimal. We just have to use it for backwards compatiblity if the compress
options are not specified.

Peter

Re: [Qemu-devel] [PATCH 4/4] block/qcow2: add zlib-fast compression algorithm

[Daniel P. Berrange]

On Tue, Jun 27, 2017 at 03:23:19PM +0200, Peter Lieven wrote:
> Am 27.06.2017 um 15:16 schrieb Daniel P. Berrange:
> > On Tue, Jun 27, 2017 at 02:34:10PM +0200, Peter Lieven wrote:
> > > this adds support for optimized zlib settings which almost
> > > tripples the compression speed while maintaining about
> > > the same compressed size.
> > > 
> > > Signed-off-by: Peter Lieven <pl@kamp.de>
> > > ---
> > >   block/qcow2-cluster.c |  3 ++-
> > >   block/qcow2.c         | 11 +++++++++--
> > >   block/qcow2.h         |  1 +
> > >   qemu-img.texi         |  1 +
> > >   4 files changed, 13 insertions(+), 3 deletions(-)
> > > diff --git a/qemu-img.texi b/qemu-img.texi
> > > index 043c1ba..83a5db2 100644
> > > --- a/qemu-img.texi
> > > +++ b/qemu-img.texi
> > > @@ -627,6 +627,7 @@ The following options are available if support for the respective libraries
> > >   has been enabled at compile time:
> > >      zlib            Uses standard zlib compression (default)
> > > +   zlib-fast       Uses zlib compression with optimized compression parameters
> > This looks like a poor design from a future proofing POV. I would much
> > rather see us introduce a more flexible modelling of compression at the
> > QAPI layer which lets us have tunables for each algorith, in the same
> > way that the qcow2 built-in encryption now has ability to set tunables
> > for each algorithm.
> > 
> > eg your "zlib-fast" impl which just uses zlib with a window size of -15
> > could be expressed as
> > 
> >     qemu-img create -o compress.format=zlib,compress.window=-15
> 
> It would also need a compress.level, but okay. This will make things
> more complicated as one might not know what good values for
> the algoritm are.
> 
> Wouldn't it be better to have sth like compress.level=1..9 and let
> the implementation decide which parameters it choose for the algorithm?

Yep, there's definitely a choice of approaches - exposing low level
details of each library as parameters, vs trying to come up with a
more abstracted, simplified notation and having the driver pick some
of the low level details implicitly from that. Both are valid, and I
don't have a particular opinion either way.

> Do you have a pointer where I can find out how to imlement that
> in QAPI? Maybe the patches that introduces the encryption settings?

It is a little messy since we don't fully use QAPI in the block create
code path. If you want to look at what I did for encryption though, see
the block/qcow2.c file. In the qcow2_create() method I grab the
encrypt.format option. Later in the qcow2_set_up_encryption() method
I then extract & parse the format specific options from the QemuOpts
array. My code is in Max's block branch, or on block-luks-qcow2-10 branch
at https://github.com/berrange/qemu

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH 1/4] block/qcow2: add compression_algorithm create option

[Peter Lieven]

Am 27.06.2017 um 14:49 schrieb Eric Blake:
> On 06/27/2017 07:34 AM, Peter Lieven wrote:
>> this patch adds a new compression_algorithm option when creating qcow2 images.
>> The current default for the compresison algorithm is zlib and zlib will be
> s/compresison/compression/
>
>> used when this option is omitted (like before).
>>
>> If the option is specified e.g. with:
>>
>>   qemu-img create -f qcow2 -o compression_algorithm=zlib image.qcow2 1G
>>
>> then a new compression algorithm header extension is added and an incompatible
>> feature bit is set. This means that if the header is present it must be parsed
>> by Qemu on qcow2_open and it must be validated if the specified compression
>> algorithm is supported by the current build of Qemu.
>>
>> This means if the compression_algorithm option is specified Qemu prior to this
>> commit will not be able to open the created image.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>   block/qcow2.c             | 93 ++++++++++++++++++++++++++++++++++++++++++++---
>>   block/qcow2.h             | 20 +++++++---
>>   docs/interop/qcow2.txt    |  8 +++-
> Focusing on just the spec change first:
>
>> +++ b/docs/interop/qcow2.txt
>> @@ -85,7 +85,11 @@ in the description of a field.
>>                                   be written to (unless for regaining
>>                                   consistency).
>>   
>> -                    Bits 2-63:  Reserved (set to 0)
>> +                    Bit 2:      Compress algorithm bit.  If this bit is set then
>> +                                the compress algorithm extension must be parsed
>> +                                and checked for compatiblity.
> s/compatiblity/compatibility/
>
>> +
>> +                    Bits 3-63:  Reserved (set to 0)
>>   
>>            80 -  87:  compatible_features
>>                       Bitmask of compatible features. An implementation can
>> @@ -135,6 +139,8 @@ be stored. Each extension has a structure like the following:
>>                           0xE2792ACA - Backing file format name
>>                           0x6803f857 - Feature name table
>>                           0x23852875 - Bitmaps extension
>> +                        0xC0318300 - Compression Algorithm
>> +                        0xC03183xx - Reserved for compression algorithm params
> s/params/parameters/
>
> You have now introduced 256 different reserved headers, without
> documenting any of their formats.  You absolutely MUST include a
> documentation of how the new 0xC0318300 header is laid out (see, for
> example, our section on "Bitmaps extension"), along with text mentioning
> that the new header MUST be present if incompatible-feature bit is set
> and MUST be absent otherwise.  But I also think that with a bit of
> proper design work, you only need ONE header for all possible algorithm
> parameters, rather than burning an additional 255 unspecified
> reservations.  That is, make sure your new header includes a common
> prefix including a length field and the algorightm in use, and then the
> length covers a variable-length suffix that can be parsed in a
> per-algorithm-specific manner for whatever additional parameters are
> needed for that algorithm.
>

Before I continue, can you please give feedback on the following spec change:

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index 80cdfd0..f1428e9 100644
--- a/docs/interop/qcow2.txt
+++ b/docs/interop/qcow2.txt
@@ -85,7 +85,11 @@ in the description of a field.
                                  be written to (unless for regaining
                                  consistency).

-                    Bits 2-63:  Reserved (set to 0)
+                    Bit 2:      Compression format bit.  Iff this bit is set then
+                                the compression 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
@@ -135,6 +139,7 @@ be stored. Each extension has a structure like the following:
                          0xE2792ACA - Backing file format name
                          0x6803f857 - Feature name table
                          0x23852875 - Bitmaps extension
+                        0xC0318300 - Compression format extension
                          other      - Unknown header extension, can be safely
                                       ignored

@@ -208,6 +213,28 @@ The fields of the bitmaps extension are:
                     starts. Must be aligned to a cluster boundary.


+== Compression format extension ==
+
+The compression format extension is an optional header extension. It provides
+the ability to specify the compression algorithm and compression parameters
+that are used for compressed clusters. This new header MUST be present if
+the incompatible-feature bit "compression format bit" is set and MUST be absent
+otherwise.
+
+The fields of the compression format extension are:
+
+    Byte  0 - 15:  compression_format_name (padded with zeros, but not
+                   necessarily null terminated if it has full length)
+
+              16:  compression_level (uint8_t)
+                   0 = default compression level
+                   1 = lowest compression level
+                   x = highest compression level (the highest compression
+                       level may vary for different compression formats)
+
+         17 - 23:  Reserved for future use, must be zero.
+
+
  == Host cluster management ==

  qcow2 manages the allocation of host clusters by maintaining a reference count

Thanks,
Peter

Re: [Qemu-devel] [PATCH 1/4] block/qcow2: add compression_algorithm create option

[Eric Blake]

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

On 06/27/2017 09:49 AM, Peter Lieven wrote:

>>
> 
> Before I continue, can you please give feedback on the following spec
> change:
> 
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index 80cdfd0..f1428e9 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -85,7 +85,11 @@ in the description of a field.
>                                  be written to (unless for regaining
>                                  consistency).
> 
> -                    Bits 2-63:  Reserved (set to 0)
> +                    Bit 2:      Compression format bit.  Iff this bit

I know what this means, but spelling it "If and only if" or "When" might
make more sense to other readers, as "Iff" is not common in English.

> is set then
> +                                the compression 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
> @@ -135,6 +139,7 @@ be stored. Each extension has a structure like the
> following:
>                          0xE2792ACA - Backing file format name
>                          0x6803f857 - Feature name table
>                          0x23852875 - Bitmaps extension
> +                        0xC0318300 - Compression format extension

Now that you aren't burning 256 magic numbers, it may make sense to have
the last two hex digits be non-zero.


> +== Compression format extension ==
> +
> +The compression format extension is an optional header extension. It
> provides

Inline pasting created interesting wrapping, but the actual patch will
obviously read better.

> +the ability to specify the compression algorithm and compression
> parameters
> +that are used for compressed clusters. This new header MUST be present if
> +the incompatible-feature bit "compression format bit" is set and MUST
> be absent
> +otherwise.
> +
> +The fields of the compression format extension are:
> +
> +    Byte  0 - 15:  compression_format_name (padded with zeros, but not
> +                   necessarily null terminated if it has full length)

Do we really want arbitrary names of formats, or do we want to specify
specific algorithms (gzip, lzo, zstd) as an enum?  Which way gives us
maximum likelihood of interoperability?

> +
> +              16:  compression_level (uint8_t)
> +                   0 = default compression level
> +                   1 = lowest compression level
> +                   x = highest compression level (the highest compression
> +                       level may vary for different compression formats)
> +
> +         17 - 23:  Reserved for future use, must be zero.

Feels pretty limited - you don't have a length field for variable-length
extension of additional parameters, but have to fit all additions in the
next 8 bytes.  Yes, all extension headers are already paired with a
length parameter outside of the struct, sent alongside the header magic
number, but embedding a length directly in the header (while redundant)
makes it easier to keep information local to the header.  See
extra_data_size under Bitmap directory, for example.  Of course, we may
turn those 8 bytes INTO a length field, that then describe the rest of
the variable length parameters, but why not do it up front?

If we go with an enum mapping of supported compression formats, then you
can go into further details on exactly what extra parameters are
supports for each algorithm; while leaving it as a free-form text string
makes it harder to interpret what any additional payload will represent.

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH 1/4] block/qcow2: add compression_algorithm create option

[Peter Lieven]

Am 27.06.2017 um 17:04 schrieb Eric Blake:
> On 06/27/2017 09:49 AM, Peter Lieven wrote:
>
>> Before I continue, can you please give feedback on the following spec
>> change:
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index 80cdfd0..f1428e9 100644
>> --- a/docs/interop/qcow2.txt
>> +++ b/docs/interop/qcow2.txt
>> @@ -85,7 +85,11 @@ in the description of a field.
>>                                   be written to (unless for regaining
>>                                   consistency).
>>
>> -                    Bits 2-63:  Reserved (set to 0)
>> +                    Bit 2:      Compression format bit.  Iff this bit
> I know what this means, but spelling it "If and only if" or "When" might
> make more sense to other readers, as "Iff" is not common in English.
>
>> is set then
>> +                                the compression 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
>> @@ -135,6 +139,7 @@ be stored. Each extension has a structure like the
>> following:
>>                           0xE2792ACA - Backing file format name
>>                           0x6803f857 - Feature name table
>>                           0x23852875 - Bitmaps extension
>> +                        0xC0318300 - Compression format extension
> Now that you aren't burning 256 magic numbers, it may make sense to have
> the last two hex digits be non-zero.
>
>
>> +== Compression format extension ==
>> +
>> +The compression format extension is an optional header extension. It
>> provides
> Inline pasting created interesting wrapping, but the actual patch will
> obviously read better.
>
>> +the ability to specify the compression algorithm and compression
>> parameters
>> +that are used for compressed clusters. This new header MUST be present if
>> +the incompatible-feature bit "compression format bit" is set and MUST
>> be absent
>> +otherwise.
>> +
>> +The fields of the compression format extension are:
>> +
>> +    Byte  0 - 15:  compression_format_name (padded with zeros, but not
>> +                   necessarily null terminated if it has full length)
> Do we really want arbitrary names of formats, or do we want to specify
> specific algorithms (gzip, lzo, zstd) as an enum?  Which way gives us
> maximum likelihood of interoperability?

At some point I have to parse the name and convert it into a number.
This could be the same routine for the parsing of the compress.format
parameter and the compression_format_name in the header field.
The idea was that an accidently change in the enum cannot break
anything as it is local to the qemu binary. But I am ok to have an enum
if the values for each format a fixed. In this case it might be sufficient
to have an uint8_t for the compression format + an uint8_t for the compression
level. Then 2 bytes reserved + an uint32_t for the extra data size. So currently (the extra
data is not necessary at the moment for any format) the header would
be limited to 8 bytes.

Anyway if we glue this into a QAPI scheme, I would appreciate to get
some help with it as I am not that familiar with it yet.

Thanks,
Peter

Re: [Qemu-devel] [PATCH 1/4] block/qcow2: add compression_algorithm create option

[Peter Lieven]

Am 27.06.2017 um 17:11 schrieb Peter Lieven:
> Am 27.06.2017 um 17:04 schrieb Eric Blake:
>> On 06/27/2017 09:49 AM, Peter Lieven wrote:
>>
>>> Before I continue, can you please give feedback on the following spec
>>> change:
>>>
>>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>>> index 80cdfd0..f1428e9 100644
>>> --- a/docs/interop/qcow2.txt
>>> +++ b/docs/interop/qcow2.txt
>>> @@ -85,7 +85,11 @@ in the description of a field.
>>>                                   be written to (unless for regaining
>>>                                   consistency).
>>>
>>> -                    Bits 2-63:  Reserved (set to 0)
>>> +                    Bit 2:      Compression format bit.  Iff this bit
>> I know what this means, but spelling it "If and only if" or "When" might
>> make more sense to other readers, as "Iff" is not common in English.
>>
>>> is set then
>>> +                                the compression 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
>>> @@ -135,6 +139,7 @@ be stored. Each extension has a structure like the
>>> following:
>>>                           0xE2792ACA - Backing file format name
>>>                           0x6803f857 - Feature name table
>>>                           0x23852875 - Bitmaps extension
>>> +                        0xC0318300 - Compression format extension
>> Now that you aren't burning 256 magic numbers, it may make sense to have
>> the last two hex digits be non-zero.
>>
>>
>>> +== Compression format extension ==
>>> +
>>> +The compression format extension is an optional header extension. It
>>> provides
>> Inline pasting created interesting wrapping, but the actual patch will
>> obviously read better.
>>
>>> +the ability to specify the compression algorithm and compression
>>> parameters
>>> +that are used for compressed clusters. This new header MUST be present if
>>> +the incompatible-feature bit "compression format bit" is set and MUST
>>> be absent
>>> +otherwise.
>>> +
>>> +The fields of the compression format extension are:
>>> +
>>> +    Byte  0 - 15:  compression_format_name (padded with zeros, but not
>>> +                   necessarily null terminated if it has full length)
>> Do we really want arbitrary names of formats, or do we want to specify
>> specific algorithms (gzip, lzo, zstd) as an enum?  Which way gives us
>> maximum likelihood of interoperability?
>
> At some point I have to parse the name and convert it into a number.
> This could be the same routine for the parsing of the compress.format
> parameter and the compression_format_name in the header field.
> The idea was that an accidently change in the enum cannot break
> anything as it is local to the qemu binary. But I am ok to have an enum
> if the values for each format a fixed. In this case it might be sufficient
> to have an uint8_t for the compression format + an uint8_t for the compression
> level. Then 2 bytes reserved + an uint32_t for the extra data size. So currently (the extra
> data is not necessary at the moment for any format) the header would
> be limited to 8 bytes.
>
> Anyway if we glue this into a QAPI scheme, I would appreciate to get
> some help with it as I am not that familiar with it yet.

Forgot to mention, the idea to store the name here was to be able to display
an appropiate error message if a format is not supported (like compression format xyz is not supported).

Peter

Re: [Qemu-devel] [PATCH 1/4] block/qcow2: add compression_algorithm create option

[Denis V. Lunev]

On 06/27/2017 04:27 PM, Peter Lieven wrote:
> Am 27.06.2017 um 15:20 schrieb Daniel P. Berrange:
>> On Tue, Jun 27, 2017 at 02:34:07PM +0200, Peter Lieven wrote:
>>> this patch adds a new compression_algorithm option when creating
>>> qcow2 images.
>>> The current default for the compresison algorithm is zlib and zlib
>>> will be
>>> used when this option is omitted (like before).
>>>
>>> If the option is specified e.g. with:
>>>
>>>   qemu-img create -f qcow2 -o compression_algorithm=zlib image.qcow2 1G
>> IMHO we should introduce a nested struct "compress" struct to hold
>> the format
>> name, and any other format specific arguments, in a way that maps
>> nicely to
>> any future QAPI representmatch of create options. eg
>>
>> { 'enum': 'BlockdevQcow2CompressFormat',
>>    'data': [ 'zlib', 'lzo' ] }
>>
>> { 'union': 'BlockdevQcow2Compress',
>>    'base': { 'format': 'BlockdevQcow2CompressFormat' },
>>    'discriminator': 'format',
>>    'data': { 'zlib': 'BlockdevQcow2CompressZLib',
>>              'lzo': 'BlockdevQcow2CompressLZO'} }
>>
>> so it would map to
>>
>>   qemu-img create -f qcow2 -o compress.format=zlib image.qcow2 1G
>>
>> and let us have other compress.XXXX options specific to each format
>
> Or would it be possible to start with just a compress.level (int)
> parameter.
> In fact that would be sufficient for almost all formats (or better use
> algorithms?).
> The windowBits can be default to -15 in the future. It seems the old
> choice of -12
> was just not optimal. We just have to use it for backwards
> compatiblity if the compress
> options are not specified.
>
> Peter
>
We can put generic parameters on top (in generic header) but
put algorithm-dependent container inside. This could be
viable for the future to avoid incompatible format changes.

Den

Re: [Qemu-devel] [PATCH 1/4] block/qcow2: add compression_algorithm create option

[Denis V. Lunev]

On 06/27/2017 03:34 PM, Peter Lieven wrote:
> this patch adds a new compression_algorithm option when creating qcow2 images.
> The current default for the compresison algorithm is zlib and zlib will be
> used when this option is omitted (like before).
>
> If the option is specified e.g. with:
>
>  qemu-img create -f qcow2 -o compression_algorithm=zlib image.qcow2 1G
>
> then a new compression algorithm header extension is added and an incompatible
> feature bit is set. This means that if the header is present it must be parsed
> by Qemu on qcow2_open and it must be validated if the specified compression
> algorithm is supported by the current build of Qemu.
>
> This means if the compression_algorithm option is specified Qemu prior to this
> commit will not be able to open the created image.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>

as general, it is weird to have formatting changes, spec changes and
real code changes in one patch.

[skipped]

> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index 80cdfd0..1f165d6 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -85,7 +85,11 @@ in the description of a field.
>                                  be written to (unless for regaining
>                                  consistency).
>  
> -                    Bits 2-63:  Reserved (set to 0)
> +                    Bit 2:      Compress algorithm bit.  If this bit is set then
> +                                the compress algorithm extension must be parsed
> +                                and checked for compatiblity.
Eric is correct here. We should add note that compressed algorithm extension
must present when the bit is sent and must be absent in the other case.

> +
> +                    Bits 3-63:  Reserved (set to 0)
>  
>           80 -  87:  compatible_features
>                      Bitmask of compatible features. An implementation can
> @@ -135,6 +139,8 @@ be stored. Each extension has a structure like the following:
>                          0xE2792ACA - Backing file format name
>                          0x6803f857 - Feature name table
>                          0x23852875 - Bitmaps extension
> +                        0xC0318300 - Compression Algorithm
> +                        0xC03183xx - Reserved for compression algorithm params
>                          other      - Unknown header extension, can be safely
>                                       ignored

I think that there is no need to reserve 255 magics once we will
add opaque container to the extension.

>  
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 15fa602..03a4b8f 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -40,23 +40,24 @@
>  #define BLOCK_FLAG_ENCRYPT          1
>  #define BLOCK_FLAG_LAZY_REFCOUNTS   8
>  
> -#define BLOCK_OPT_SIZE              "size"
> -#define BLOCK_OPT_ENCRYPT           "encryption"
> -#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_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_COMPRESSION_ALGORITHM "compression_algorithm"
>  
>  #define BLOCK_PROBE_BUF_SIZE        512
>  
> diff --git a/qemu-img.texi b/qemu-img.texi
> index 5b925ec..c0d1bec 100644
> --- a/qemu-img.texi
> +++ b/qemu-img.texi
> @@ -621,6 +621,16 @@ 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 compression_algorithm
> +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:
> +
> +   zlib            Uses standard zlib compression (default)
> +
> +The compression algorithm can only be defined at image create time and cannot
> +be changed later.
> +
>  @end table
>  
>  @item Other

Re: [Qemu-devel] [PATCH 1/4] block/qcow2: add compression_algorithm create option

[Peter Lieven]

Am 27.06.2017 um 17:04 schrieb Eric Blake:
> On 06/27/2017 09:49 AM, Peter Lieven wrote:
>
>> Before I continue, can you please give feedback on the following spec
>> change:
>>
>> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
>> index 80cdfd0..f1428e9 100644
>> --- a/docs/interop/qcow2.txt
>> +++ b/docs/interop/qcow2.txt
>> @@ -85,7 +85,11 @@ in the description of a field.
>>                                  be written to (unless for regaining
>>                                  consistency).
>>
>> -                    Bits 2-63:  Reserved (set to 0)
>> +                    Bit 2:      Compression format bit.  Iff this bit
> I know what this means, but spelling it "If and only if" or "When" might
> make more sense to other readers, as "Iff" is not common in English.
>
>> is set then
>> +                                the compression 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
>> @@ -135,6 +139,7 @@ be stored. Each extension has a structure like the
>> following:
>>                          0xE2792ACA - Backing file format name
>>                          0x6803f857 - Feature name table
>>                          0x23852875 - Bitmaps extension
>> +                        0xC0318300 - Compression format extension
> Now that you aren't burning 256 magic numbers, it may make sense to have
> the last two hex digits be non-zero.
>
>
>> +== Compression format extension ==
>> +
>> +The compression format extension is an optional header extension. It
>> provides
> Inline pasting created interesting wrapping, but the actual patch will
> obviously read better.
>
>> +the ability to specify the compression algorithm and compression
>> parameters
>> +that are used for compressed clusters. This new header MUST be present if
>> +the incompatible-feature bit "compression format bit" is set and MUST
>> be absent
>> +otherwise.
>> +
>> +The fields of the compression format extension are:
>> +
>> +    Byte  0 - 15:  compression_format_name (padded with zeros, but not
>> +                   necessarily null terminated if it has full length)
> Do we really want arbitrary names of formats, or do we want to specify
> specific algorithms (gzip, lzo, zstd) as an enum?  Which way gives us
> maximum likelihood of interoperability?
>
>> +
>> +              16:  compression_level (uint8_t)
>> +                   0 = default compression level
>> +                   1 = lowest compression level
>> +                   x = highest compression level (the highest compression
>> +                       level may vary for different compression formats)
>> +
>> +         17 - 23:  Reserved for future use, must be zero.
> Feels pretty limited - you don't have a length field for variable-length
> extension of additional parameters, but have to fit all additions in the
> next 8 bytes.  Yes, all extension headers are already paired with a
> length parameter outside of the struct, sent alongside the header magic
> number, but embedding a length directly in the header (while redundant)
> makes it easier to keep information local to the header.  See
> extra_data_size under Bitmap directory, for example.  Of course, we may
> turn those 8 bytes INTO a length field, that then describe the rest of
> the variable length parameters, but why not do it up front?
>
> If we go with an enum mapping of supported compression formats, then you
> can go into further details on exactly what extra parameters are
> supports for each algorithm; while leaving it as a free-form text string
> makes it harder to interpret what any additional payload will represent.
>

I send a V2 of the series including the update of the spec last week.

Maybe you can have a look if this version is better.


Thanks,

Peter