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

[Qemu-devel] [PATCH V4 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 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             | 248 ++++++++++++++++++++++++++++++++++++++--------
 block/qcow2.h             |  21 +++-
 configure                 |   2 +-
 docs/interop/qcow2.txt    |  43 +++++++-
 include/block/block_int.h |   2 +
 qapi/block-core.json      |  54 +++++++++-
 qemu-img.texi             |  21 ++++
 8 files changed, 392 insertions(+), 72 deletions(-)

-- 
1.9.1

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

[Peter Lieven]

Signed-off-by: Peter Lieven <pl@kamp.de>
---
 docs/interop/qcow2.txt | 43 ++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
index d7fdb1f..ef5abb2 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,41 @@ 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.
+
+Note: Ommitting the incompatible "Compress format bit" results in the usage
+of zlib compression with default compression level (default before QEMU 2.10).
+However, this old default has a smaller compression window size which results in
+lower compression ratio and slightly worse compression speed compared to
+explicity specifying zlib compression with default compression level in the
+compress format extension.
+
+The fields of the compress format extension are:
+
+    Byte  0 - 14:  compress_format_name (padded with zeros, but not
+                   necessarily null terminated if it has full length).
+                   Valid compression format names currently are:
+
+                   zlib: Standard zlib deflate compression
+
+              15:  compress_level (uint8_t)
+
+                   0 = default compress level (valid for all formats, default)
+
+                   Additional valid compression levels for zlib 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.
+
+
 == Host cluster management ==
 
 qcow2 manages the allocation of host clusters by maintaining a reference count
-- 
1.9.1

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

[Eric Blake]

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

On 07/20/2017 09:20 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  docs/interop/qcow2.txt | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 

> +== 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.
> +
> +Note: Ommitting the incompatible "Compress format bit" results in the usage

s/Ommitting/Omitting/

> +of zlib compression with default compression level (default before QEMU 2.10).
> +However, this old default has a smaller compression window size which results in
> +lower compression ratio and slightly worse compression speed compared to
> +explicity specifying zlib compression with default compression level in the

s/explicity/explicitly/

> +compress format extension.

If window size affects decompression, then we absolutely must specify
what window size is in use, both for the old-style compression (bug that
we haven't mentioned window size in the past), and for the new-style.
That is, if you have to tell zlib at decompression time what size window
was used at compression, then our choices for window size should be
mentioned in this spec.  Furthermore, if window size matters, it sounds
like it should be a zlib-specific tunable.  I really would like to be
able to document that having the extension header present with
parameters XYZ is the same as omitting the extension header (but that is
only doable if window size is a tunable parameter).

I haven't yet checked your code implementation to see where you are
setting window sizes, to know if window size is something that should be
a tunable in the file format.

-- 
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 V4 01/10] specs/qcow2: add compress format extension

[Peter Lieven]

Am 20.07.2017 um 17:52 schrieb Eric Blake:
> On 07/20/2017 09:20 AM, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>  docs/interop/qcow2.txt | 43 ++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 42 insertions(+), 1 deletion(-)
>>
>> +== 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.
>> +
>> +Note: Ommitting the incompatible "Compress format bit" results in the usage
> s/Ommitting/Omitting/
>
>> +of zlib compression with default compression level (default before QEMU 2.10).
>> +However, this old default has a smaller compression window size which results in
>> +lower compression ratio and slightly worse compression speed compared to
>> +explicity specifying zlib compression with default compression level in the
> s/explicity/explicitly/
>
>> +compress format extension.
> If window size affects decompression, then we absolutely must specify
> what window size is in use, both for the old-style compression (bug that
> we haven't mentioned window size in the past), and for the new-style.
> That is, if you have to tell zlib at decompression time what size window
> was used at compression, then our choices for window size should be
> mentioned in this spec.  Furthermore, if window size matters, it sounds
> like it should be a zlib-specific tunable.  I really would like to be
> able to document that having the extension header present with
> parameters XYZ is the same as omitting the extension header (but that is
> only doable if window size is a tunable parameter).
>
> I haven't yet checked your code implementation to see where you are
> setting window sizes, to know if window size is something that should be
> a tunable in the file format.
>
You don't have to know the window size that was used. The docs of zlib

just say, that the windowSize specified at inflate must be greater or equal

to the one used at deflate. So if we change the code to use 15 we can safely

decompress old clusters that where compressed with 12.


Peter

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

[Eric Blake]

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

On 07/20/2017 11:26 AM, Peter Lieven wrote:
>> I haven't yet checked your code implementation to see where you are
>> setting window sizes, to know if window size is something that should be
>> a tunable in the file format.
>>
> You don't have to know the window size that was used. The docs of zlib
> 
> just say, that the windowSize specified at inflate must be greater or equal
> 
> to the one used at deflate. So if we change the code to use 15 we can safely
> 
> decompress old clusters that where compressed with 12.

Then you DO have to know the window size.  If I write my own qcow2
parser, and don't know what size you picked on compression, then how do
I know that the size I pick is >= your size during my decompression?

Even if you DON'T want size to be a tunable (and I'm on the fence on
that one), you STILL need the spec to state that for maximum
compatibility, when the incompatible feature is off, implementations
MUST use window size 12 for compression; and that when the compression
extension is used, implementations MUST use window size 15.

Or, by having window size be a tunable in the extension header allows
implementations to record window size, then you allow implemenations to
explicitly KNOW what window size they must meet or exceed.

-- 
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 V4 01/10] specs/qcow2: add compress format extension

[Peter Lieven]

Am 20.07.2017 um 20:31 schrieb Eric Blake:
> On 07/20/2017 11:26 AM, Peter Lieven wrote:
>>> I haven't yet checked your code implementation to see where you are
>>> setting window sizes, to know if window size is something that should be
>>> a tunable in the file format.
>>>
>> You don't have to know the window size that was used. The docs of zlib
>>
>> just say, that the windowSize specified at inflate must be greater or equal
>>
>> to the one used at deflate. So if we change the code to use 15 we can safely
>>
>> decompress old clusters that where compressed with 12.
> Then you DO have to know the window size.  If I write my own qcow2
> parser, and don't know what size you picked on compression, then how do
> I know that the size I pick is >= your size during my decompression?
>
> Even if you DON'T want size to be a tunable (and I'm on the fence on
> that one), you STILL need the spec to state that for maximum
> compatibility, when the incompatible feature is off, implementations
> MUST use window size 12 for compression; and that when the compression
> extension is used, implementations MUST use window size 15.
>
> Or, by having window size be a tunable in the extension header allows
> implementations to record window size, then you allow implemenations to
> explicitly KNOW what window size they must meet or exceed.
>

But they still have to now what values to use if the header is absent.


Bottom line, you are right. Its cleaner to have that value in the header and

default it to zlib, level 0, windowBits 12 if the header is absent and on the other

side don't write the header if these values are used.


I checked the spec again expect for the windowBits nothing else is passed as

a parameter to inflateinit2.


Here is what is written about the windowBits provided to inflateInit2:

" The windowBits parameter is the base two logarithm of the maximum window size (the size of the history buffer). It should be in the range 8..15 for this version of the library. The default value is 15 if inflateInit is used instead. windowBits must be greater than or equal to the windowBitsvalue provided to deflateInit2() while compressing, or it must be equal to 15 if deflateInit2() was not used. If a compressed stream with a larger window size is given as input, inflate() will return with the error code Z_DATA_ERROR instead of trying to allocate a larger window."


I checked through the parameters of some compression algorithms. Most obviously have a positive interger for a compression level. And at least some have not a window as zlib, but a dictionary size which is also a power of two. So it might be reasonable to have that second

uint8_t also in the default header for a window/dictionary size exponent.


So it would propose to update the header like this:


    Byte  0 - 13:  compress_format_name
              14:  compress_level (uint8_t)
              15:  compress_window_exp (uint8_t)


Peter

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

[Peter Lieven]

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

diff --git a/qapi/block-core.json b/qapi/block-core.json
index ff8e2ba..95e5393 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2454,6 +2454,44 @@
             '*encrypt': 'BlockdevQcow2Encryption' } }
 
 ##
+# @Qcow2CompressFormat:
+# @zlib: standard zlib deflate compression
+#
+# Since: 2.10
+##
+{ 'enum': 'Qcow2CompressFormat',
+  'data': [ 'zlib' ] }
+
+##
+# @Qcow2CompressZLib:
+#
+# Since: 2.10
+##
+{ 'struct': 'Qcow2CompressZLib',
+  'data': { } }
+
+##
+# @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)
+#
+# @level: specifies the compression level. 0 = default compression,
+#         1 = fastest compression, x = highest compresion (x may very between
+#         different compression formats)
+#
+# Since: 2.10
+##
+{ 'union': 'Qcow2Compress',
+  'base': { 'format': 'Qcow2CompressFormat',
+            '*level': 'uint8' },
+  'discriminator': 'format',
+  'data': { 'zlib': 'Qcow2CompressZLib' } }
+
+
+##
 # @BlockdevOptionsSsh:
 #
 # @server:              host address
-- 
1.9.1

[Qemu-devel] [PATCH V4 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             | 53 +++++++++++++++++++++++++++++++++++++++++++++--
 include/block/block_int.h |  2 ++
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index d5790af..073d50f 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"
@@ -2706,7 +2708,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 +2901,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 +2979,43 @@ 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)) {
+        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);
+        if (compress.format == QCOW2_COMPRESS_FORMAT_ZLIB &&
+            compress.level > 9) {
+            error_setg(errp, "Compress level %" PRIu8 " is not supported for"
+                       " format '%s'", compress.level,
+                       Qcow2CompressFormat_lookup[compress.format]);
+            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 +4325,17 @@ 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 (zlib)",
+        },
+        {
+            .name = BLOCK_OPT_COMPRESS_LEVEL,
+            .type = QEMU_OPT_NUMBER,
+            .help = "Compress level used for compressed clusters (0 = default,"
+                    " 1=fastest, x=best; x varies depending on compress.format)",
+        },
         { /* end of list */ }
     }
 };
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 5c6b761..8245963 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -57,6 +57,8 @@
 #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_PROBE_BUF_SIZE        512
 
-- 
1.9.1

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

[Peter Lieven]

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

diff --git a/qemu-img.texi b/qemu-img.texi
index 72dabd6..fcc4c1d 100644
--- a/qemu-img.texi
+++ b/qemu-img.texi
@@ -676,6 +676,26 @@ 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:
+
+   zlib            Uses standard zlib compression
+
+The compression algorithm can only be defined at image create time and cannot
+be changed later.
+
+Note: defining a compression format will result in the compression format
+      extension being written to the Qcow2 image. Versions of QEMU before 2.10
+      will not be able to open images with this extension.
+
+@item compress.level
+Valid for compress.format=zlib 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.
+
 @end table
 
 @item Other
-- 
1.9.1

[Qemu-devel] [PATCH V4 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 | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 block/qcow2.h | 21 +++++++++----
 2 files changed, 105 insertions(+), 10 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 073d50f..1dff87e 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)
 {
@@ -164,6 +165,17 @@ static ssize_t qcow2_crypto_hdr_write_func(QCryptoBlock *block, size_t offset,
 }
 
 
+static void qcow2_compress_level_supported(int format, uint8_t level,
+                                           Error **errp)
+{
+    if (format == QCOW2_COMPRESS_FORMAT_ZLIB && level > 9) {
+        error_setg(errp, "ERROR: compress level %" PRIu8 " is not"
+                         " supported for format '%s'", level,
+                         Qcow2CompressFormat_lookup[format]);
+    }
+}
+
+
 /* 
  * read qcow2 extension and fill bs
  * start reading from start_offset
@@ -241,6 +253,48 @@ 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;
+            }
+
+            qcow2_compress_level_supported(s->compress_format,
+                                           compress_ext.level, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                return 5;
+            }
+            s->compress_level = compress_ext.level;
+
+#ifdef DEBUG_EXT
+            printf("Qcow2: Got compress format %s with compress level %"
+                   PRIu8 "\n", Qcow2CompressFormat_lookup[s->compress_format],
+                   s->compress_level);
+#endif
+            break;
+        }
         case QCOW2_EXT_MAGIC_FEATURE_TABLE:
             if (p_feature_table != NULL) {
                 void* feature_table = g_malloc0(ext.len + 2 * sizeof(Qcow2Feature));
@@ -1374,6 +1428,7 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags,
 
     s->cluster_cache_offset = -1;
     s->flags = flags;
+    s->compress_format = -1;
 
     ret = qcow2_refcount_init(bs);
     if (ret != 0) {
@@ -2292,6 +2347,25 @@ int qcow2_update_header(BlockDriverState *bs)
         buflen -= ret;
     }
 
+    /* Compress Format header extension */
+    if (s->compress_format >= 0) {
+        Qcow2CompressFormatExt ext;
+        assert(s->compress_format < QCOW2_COMPRESS_FORMAT__MAX);
+        strncpy((char *) &ext.name,
+                Qcow2CompressFormat_lookup[s->compress_format],
+                sizeof(ext.name));
+        ext.level = s->compress_level;
+        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[] = {
@@ -2306,6 +2380,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",
@@ -2827,6 +2906,12 @@ static int qcow2_create2(const char *filename, int64_t total_size,
         abort();
     }
 
+    if (compress) {
+        BDRVQcow2State *s = blk_bs(blk)->opaque;
+        s->compress_format = compress->format;
+        s->compress_level = compress->level;
+    }
+
     /* Create a full header (including things like feature table) */
     ret = qcow2_update_header(blk_bs(blk));
     if (ret < 0) {
@@ -3002,11 +3087,10 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
         visit_free(v);
         QDECREF(compressopts);
         QDECREF(options);
-        if (compress.format == QCOW2_COMPRESS_FORMAT_ZLIB &&
-            compress.level > 9) {
-            error_setg(errp, "Compress level %" PRIu8 " is not supported for"
-                       " format '%s'", compress.level,
-                       Qcow2CompressFormat_lookup[compress.format]);
+        qcow2_compress_level_supported(compress.format, compress.level,
+                                       &local_err);
+        if (local_err) {
+            error_propagate(errp, local_err);
             ret = -EINVAL;
             goto finish;
         }
diff --git a/block/qcow2.h b/block/qcow2.h
index 96a8d43..9a564d4 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,11 @@ typedef struct Qcow2Feature {
     char    name[46];
 } QEMU_PACKED Qcow2Feature;
 
+typedef struct Qcow2CompressFormatExt {
+    char name[15];
+    uint8_t level;
+} QEMU_PACKED Qcow2CompressFormatExt;
+
 typedef struct Qcow2DiscardRegion {
     BlockDriverState *bs;
     uint64_t offset;
@@ -304,6 +312,9 @@ typedef struct BDRVQcow2State {
     int refcount_bits;
     uint64_t refcount_max;
 
+    int compress_format;
+    uint8_t compress_level;
+
     Qcow2GetRefcountFunc *get_refcount;
     Qcow2SetRefcountFunc *set_refcount;
 
-- 
1.9.1

[Qemu-devel] [PATCH V4 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 1dff87e..67e48e1 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -2996,7 +2996,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),
@@ -3008,7 +3008,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)) {
@@ -3017,7 +3016,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);
@@ -3026,14 +3024,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;
     }
 
@@ -3044,21 +3040,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;
     }
 
@@ -3074,13 +3067,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);
@@ -3091,12 +3082,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;
         }
         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 V4 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 67e48e1..978e8d2 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3393,7 +3393,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) {
@@ -3403,8 +3403,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)
         {
@@ -3413,8 +3413,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);
 
@@ -3482,7 +3484,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 V4 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         | 57 +++++++++++++++++++++++++++-----------------------
 2 files changed, 65 insertions(+), 50 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f06c08f..6c14d59 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,
+                             int compress_format)
 {
-    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_ZLIB:
+    case -1: {
+        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, -15);
+        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_format) < 0) {
             return -EIO;
         }
         s->cluster_cache_offset = coffset;
diff --git a/block/qcow2.c b/block/qcow2.c
index 978e8d2..57249cc 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3391,9 +3391,10 @@ 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 z_windowBits = -15, z_level = Z_DEFAULT_COMPRESSION;
+    int ret, out_len = 0;
+    uint8_t *buf, *out_buf = NULL, *local_buf = NULL;
     uint64_t cluster_offset;
 
     if (bytes == 0) {
@@ -3418,34 +3419,38 @@ 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 -1:
+        z_windowBits = -12;
+    case QCOW2_COMPRESS_FORMAT_ZLIB:
+        out_buf = g_malloc(s->cluster_size);
+        if (s->compress_level > 0) {
+            z_level = s->compress_level;
+        }
 
-    /* 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;
-    }
+        ret = deflateInit2(&z_strm, z_level, Z_DEFLATED, z_windowBits, 9,
+                           Z_DEFAULT_STRATEGY);
+        if (ret != Z_OK) {
+            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;
+        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;
 
-    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(&z_strm, Z_FINISH);
+        out_len = z_strm.next_out - out_buf;
+        deflateEnd(&z_strm);
 
-    deflateEnd(&strm);
+        ret = ret != Z_STREAM_END;
+        break;
+    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) {
-- 
1.9.1

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

[Eric Blake]

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

On 07/20/2017 09:20 AM, Peter Lieven wrote:
> 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         | 57 +++++++++++++++++++++++++++-----------------------
>  2 files changed, 65 insertions(+), 50 deletions(-)

>  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,
> +                             int compress_format)
>  {

> -
> -    ret = inflateInit2(strm, -12);

The old code initializes with 12,


> +    switch (compress_format) {
> +    case QCOW2_COMPRESS_FORMAT_ZLIB:
> +    case -1: {

What is case -1 supposed to be? When the compression format was not
specified?  Again, I'd really like there a way to encode the default
(when no format is encoded in the qcow2 file) to be representable
alongside the new code, and then we just special-case writing the
compression format to the file to omit the extension header if the user
requested parameters that match the old default.

> +        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, -15);

The new code is now unconditionally initializing with -15 instead of
-12.  Does that matter, or does decompression work regardless of window
size used at creation, as long as the initialized size at decompression
is at least as large?  On the other hand, I guess that means if someone
compresses with a large window, and then I initialize the decompressor
with a small window, my decompression will fail?  That's why knowing the
minimum window size should be part of the spec, whether or not we make
it a tunable.

> +++ b/block/qcow2.c
> @@ -3391,9 +3391,10 @@ 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 z_windowBits = -15, z_level = Z_DEFAULT_COMPRESSION;
> +    int ret, out_len = 0;
> +    uint8_t *buf, *out_buf = NULL, *local_buf = NULL;
>      uint64_t cluster_offset;
>  
>      if (bytes == 0) {
> @@ -3418,34 +3419,38 @@ 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 -1:
> +        z_windowBits = -12;
> +    case QCOW2_COMPRESS_FORMAT_ZLIB:
> +        out_buf = g_malloc(s->cluster_size);
> +        if (s->compress_level > 0) {
> +            z_level = s->compress_level;
> +        }

And this is where I wonder if z_windowBits should be explicitly encoded
in the qcow2 format, rather than just magic numbers we use under the
hood, so that someone else trying to reimplement things will create
compatible files.

-- 
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 V4 08/10] block/qcow2: start using the compress format extension

[Peter Lieven]

Am 20.07.2017 um 18:00 schrieb Eric Blake:
> On 07/20/2017 09:20 AM, Peter Lieven wrote:
>> 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         | 57 +++++++++++++++++++++++++++-----------------------
>>  2 files changed, 65 insertions(+), 50 deletions(-)
>>  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,
>> +                             int compress_format)
>>  {
>> -
>> -    ret = inflateInit2(strm, -12);
> The old code initializes with 12,
>
>
>> +    switch (compress_format) {
>> +    case QCOW2_COMPRESS_FORMAT_ZLIB:
>> +    case -1: {
> What is case -1 supposed to be? When the compression format was not
> specified?  Again, I'd really like there a way to encode the default
> (when no format is encoded in the qcow2 file) to be representable
> alongside the new code, and then we just special-case writing the
> compression format to the file to omit the extension header if the user
> requested parameters that match the old default.
>
>> +        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, -15);
> The new code is now unconditionally initializing with -15 instead of
> -12.  Does that matter, or does decompression work regardless of window
> size used at creation, as long as the initialized size at decompression
> is at least as large?  On the other hand, I guess that means if someone
> compresses with a large window, and then I initialize the decompressor
> with a small window, my decompression will fail?  That's why knowing the
> minimum window size should be part of the spec, whether or not we make
> it a tunable.

The decompression is supposed to fail if you compress with 15 and
decompress with 12. In fact it doesn't.

>
>> +++ b/block/qcow2.c
>> @@ -3391,9 +3391,10 @@ 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 z_windowBits = -15, z_level = Z_DEFAULT_COMPRESSION;
>> +    int ret, out_len = 0;
>> +    uint8_t *buf, *out_buf = NULL, *local_buf = NULL;
>>      uint64_t cluster_offset;
>>  
>>      if (bytes == 0) {
>> @@ -3418,34 +3419,38 @@ 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 -1:
>> +        z_windowBits = -12;
>> +    case QCOW2_COMPRESS_FORMAT_ZLIB:
>> +        out_buf = g_malloc(s->cluster_size);
>> +        if (s->compress_level > 0) {
>> +            z_level = s->compress_level;
>> +        }
> And this is where I wonder if z_windowBits should be explicitly encoded
> in the qcow2 format, rather than just magic numbers we use under the
> hood, so that someone else trying to reimplement things will create
> compatible files.
>
I would like to avoid the windowBits in the qcow2 header as it makes

the code to read and write it more complicated. If you don't like the change

of the windowBits we can even stick with 12. If someone wants fast compression

he will likely not use zlib at all and use lzo.

I just changed the windowBits to 15 as it increases speed and improves compression.

(likely at the cost of memory during compression/decompression)


Peter

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

[Eric Blake]

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

On 07/20/2017 11:30 AM, Peter Lieven wrote:

>> The new code is now unconditionally initializing with -15 instead of
>> -12.  Does that matter, or does decompression work regardless of window
>> size used at creation, as long as the initialized size at decompression
>> is at least as large?  On the other hand, I guess that means if someone
>> compresses with a large window, and then I initialize the decompressor
>> with a small window, my decompression will fail?  That's why knowing the
>> minimum window size should be part of the spec, whether or not we make
>> it a tunable.
> 
> The decompression is supposed to fail if you compress with 15 and
> decompress with 12. In fact it doesn't.

Actually, I think (this is my guess here, not actual researched fact)
that the decompression error is possible ONLY if compression produced a
symbol that actually required more than 12 bits of memory - to get that
large of a symbol, you need to compress a lot of bits.  For our default
cluster of 64k, it might very well be that you rarely, if ever,
encounter a single cluster that compresses differently under window size
15 than it did under window size 12 (other than perhaps the speed at
which compression took place), because there simply wasn't enough
content to reach the point where you needed a symbol in the compression
stream using more than 12 bits.  So in that case, compressing under 15
and decompressing under 12 doesn't hit the error.  But as you get larger
cluster sizes (2M clusters), or perhaps if you pass particularly nasty
sequences of input to compression (I'm not sure what sequences would
have the right properties), then you do indeed result in a compression
stream that starts to encounter symbols exceeding the window size.

But if my guess is right, then don't read the docs as "decompression
will fail", but rather as "decompression may fail" if you set the
decompress window smaller than the compression window.

> I would like to avoid the windowBits in the qcow2 header as it makes
> 
> the code to read and write it more complicated. If you don't like the change
> 
> of the windowBits we can even stick with 12. If someone wants fast compression
> 
> he will likely not use zlib at all and use lzo.

Also, note that historically, 'compress -b N' has allowed tuning the
window size; current POSIX states that compress only has to support
windows from 9 to 14, but permits implementations to use up to 16 (and
future POSIX is considering improving the compress utility to require
support for 16 as the window size, https://posix.rhansen.org/p/bug1041).
 I don't know why gzip didn't expose a '-b N' windowsize parameter the
way compress did, but it sounds like the same thing.

> 
> I just changed the windowBits to 15 as it increases speed and improves compression.

Does windowBits 16 make any difference?

> 
> (likely at the cost of memory during compression/decompression)
> 
> 
> Peter
> 
> 

-- 
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 V4 08/10] block/qcow2: start using the compress format extension

[Peter Lieven]

Am 20.07.2017 um 21:19 schrieb Eric Blake:
> On 07/20/2017 11:30 AM, Peter Lieven wrote:
>
>>> The new code is now unconditionally initializing with -15 instead of
>>> -12.  Does that matter, or does decompression work regardless of window
>>> size used at creation, as long as the initialized size at decompression
>>> is at least as large?  On the other hand, I guess that means if someone
>>> compresses with a large window, and then I initialize the decompressor
>>> with a small window, my decompression will fail?  That's why knowing the
>>> minimum window size should be part of the spec, whether or not we make
>>> it a tunable.
>> The decompression is supposed to fail if you compress with 15 and
>> decompress with 12. In fact it doesn't.
> Actually, I think (this is my guess here, not actual researched fact)
> that the decompression error is possible ONLY if compression produced a
> symbol that actually required more than 12 bits of memory - to get that
> large of a symbol, you need to compress a lot of bits.  For our default
> cluster of 64k, it might very well be that you rarely, if ever,
> encounter a single cluster that compresses differently under window size
> 15 than it did under window size 12 (other than perhaps the speed at
> which compression took place), because there simply wasn't enough
> content to reach the point where you needed a symbol in the compression
> stream using more than 12 bits.  So in that case, compressing under 15
> and decompressing under 12 doesn't hit the error.  But as you get larger
> cluster sizes (2M clusters), or perhaps if you pass particularly nasty
> sequences of input to compression (I'm not sure what sequences would
> have the right properties), then you do indeed result in a compression
> stream that starts to encounter symbols exceeding the window size.
>
> But if my guess is right, then don't read the docs as "decompression
> will fail", but rather as "decompression may fail" if you set the
> decompress window smaller than the compression window.
>
>> I would like to avoid the windowBits in the qcow2 header as it makes
>>
>> the code to read and write it more complicated. If you don't like the change
>>
>> of the windowBits we can even stick with 12. If someone wants fast compression
>>
>> he will likely not use zlib at all and use lzo.
> Also, note that historically, 'compress -b N' has allowed tuning the
> window size; current POSIX states that compress only has to support
> windows from 9 to 14, but permits implementations to use up to 16 (and
> future POSIX is considering improving the compress utility to require
> support for 16 as the window size, https://posix.rhansen.org/p/bug1041).
>  I don't know why gzip didn't expose a '-b N' windowsize parameter the
> way compress did, but it sounds like the same thing.
>
>> I just changed the windowBits to 15 as it increases speed and improves compression.
> Does windowBits 16 make any difference?

16 is not allowed. Deflate allows only 2^8 through 2^15 window size.
While we are talking, I thing we should choose not zlib but deflate for the
compress format name.

Peter

[Qemu-devel] [PATCH V4 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         | 42 +++++++++++++++++++++++++++++++++++-------
 configure             |  2 +-
 qapi/block-core.json  | 14 ++++++++++++--
 qemu-img.texi         |  1 +
 5 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 6c14d59..7d47dde 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 57249cc..0ba5977 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"
@@ -165,10 +168,18 @@ static ssize_t qcow2_crypto_hdr_write_func(QCryptoBlock *block, size_t offset,
 }
 
 
-static void qcow2_compress_level_supported(int format, uint8_t level,
+static void qcow2_compress_settings_supported(int format, uint8_t level,
                                            Error **errp)
 {
-    if (format == QCOW2_COMPRESS_FORMAT_ZLIB && level > 9) {
+#ifndef CONFIG_LZO
+    if (format == QCOW2_COMPRESS_FORMAT_LZO) {
+        error_setg(errp, "ERROR: compress format '%s' is not supported",
+                         Qcow2CompressFormat_lookup[format]);
+        return;
+    }
+#endif
+    if ((format == QCOW2_COMPRESS_FORMAT_ZLIB && level > 9) ||
+        (format == QCOW2_COMPRESS_FORMAT_LZO && level > 0)) {
         error_setg(errp, "ERROR: compress level %" PRIu8 " is not"
                          " supported for format '%s'", level,
                          Qcow2CompressFormat_lookup[format]);
@@ -280,14 +291,22 @@ static int qcow2_read_extensions(BlockDriverState *bs, uint64_t start_offset,
                 return 4;
             }
 
-            qcow2_compress_level_supported(s->compress_format,
-                                           compress_ext.level, &local_err);
+            qcow2_compress_settings_supported(s->compress_format,
+                                              compress_ext.level, &local_err);
             if (local_err) {
                 error_propagate(errp, local_err);
                 return 5;
             }
             s->compress_level = compress_ext.level;
 
+#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
             printf("Qcow2: Got compress format %s with compress level %"
                    PRIu8 "\n", Qcow2CompressFormat_lookup[s->compress_format],
@@ -3078,8 +3097,8 @@ static int qcow2_create(const char *filename, QemuOpts *opts, Error **errp)
         visit_free(v);
         QDECREF(compressopts);
         QDECREF(options);
-        qcow2_compress_level_supported(compress.format, compress.level,
-                                       &local_err);
+        qcow2_compress_settings_supported(compress.format, compress.level,
+                                          &local_err);
         if (local_err) {
             error_propagate(errp, local_err);
             goto finish;
@@ -3394,7 +3413,7 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset,
     z_stream z_strm = {};
     int z_windowBits = -15, z_level = Z_DEFAULT_COMPRESSION;
     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) {
@@ -3446,6 +3465,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 */
     }
@@ -3491,6 +3518,7 @@ success:
 fail:
     qemu_vfree(local_buf);
     g_free(out_buf);
+    g_free(work_buf);
     return ret;
 }
 
diff --git a/configure b/configure
index befebe2..c28a45d 100755
--- a/configure
+++ b/configure
@@ -1982,7 +1982,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/qapi/block-core.json b/qapi/block-core.json
index 95e5393..9eb76df 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2456,11 +2456,12 @@
 ##
 # @Qcow2CompressFormat:
 # @zlib: standard zlib deflate compression
+# @lzo: lzo1x compression
 #
 # Since: 2.10
 ##
 { 'enum': 'Qcow2CompressFormat',
-  'data': [ 'zlib' ] }
+  'data': [ 'zlib', 'lzo' ] }
 
 ##
 # @Qcow2CompressZLib:
@@ -2471,6 +2472,14 @@
   'data': { } }
 
 ##
+# @Qcow2CompressLZO:
+#
+# Since: 2.10
+##
+{ 'struct': 'Qcow2CompressLZO',
+  'data': { } }
+
+##
 # @Qcow2Compress:
 #
 # Specifies the compression format and compression level that should
@@ -2488,7 +2497,8 @@
   'base': { 'format': 'Qcow2CompressFormat',
             '*level': 'uint8' },
   'discriminator': 'format',
-  'data': { 'zlib': 'Qcow2CompressZLib' } }
+  'data': { 'zlib': 'Qcow2CompressZLib',
+            'lzo': 'Qcow2CompressLZO' } }
 
 
 ##
diff --git a/qemu-img.texi b/qemu-img.texi
index fcc4c1d..17a17e5 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:
 
    zlib            Uses standard zlib compression
+   lzo             Uses LZO1X compression
 
 The compression algorithm can only be defined at image create time and cannot
 be changed later.
-- 
1.9.1

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

[Eric Blake]

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

On 07/20/2017 09:20 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/qcow2-cluster.c | 15 +++++++++++++++
>  block/qcow2.c         | 42 +++++++++++++++++++++++++++++++++++-------
>  configure             |  2 +-
>  qapi/block-core.json  | 14 ++++++++++++--
>  qemu-img.texi         |  1 +
>  5 files changed, 64 insertions(+), 10 deletions(-)

Where is the change to docs/ for updating the qcow2 spec to document the
new format?

-- 
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 V4 09/10] block/qcow2: add lzo compress format

[Peter Lieven]

Am 20.07.2017 um 18:03 schrieb Eric Blake:
> On 07/20/2017 09:20 AM, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>  block/qcow2-cluster.c | 15 +++++++++++++++
>>  block/qcow2.c         | 42 +++++++++++++++++++++++++++++++++++-------
>>  configure             |  2 +-
>>  qapi/block-core.json  | 14 ++++++++++++--
>>  qemu-img.texi         |  1 +
>>  5 files changed, 64 insertions(+), 10 deletions(-)
> Where is the change to docs/ for updating the qcow2 spec to document the
> new format?
>

was lost during rebase. Will fix this.

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

[Peter Lieven]

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

diff --git a/block/qcow2.c b/block/qcow2.c
index 0ba5977..59cf3b3 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -3942,6 +3942,15 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs)
         spec_info->u.qcow2.data->encrypt = qencrypt;
     }
 
+    if (s->compress_format != -1) {
+        Qcow2Compress *qcompress = g_new0(Qcow2Compress, 1);
+        qcompress->format = s->compress_format;
+        qcompress->level = s->compress_level;
+        qcompress->has_level = true;
+        spec_info->u.qcow2.data->compress = qcompress;
+        spec_info->u.qcow2.data->has_compress = true;
+    }
+
     return spec_info;
 }
 
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 9eb76df..9310715 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.10)
+#
 # 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 V4 10/10] block/qcow2: add compress info to image specific info

[Eric Blake]

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

On 07/20/2017 09:20 AM, Peter Lieven wrote:
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
>  block/qcow2.c        | 9 +++++++++
>  qapi/block-core.json | 6 +++++-
>  2 files changed, 14 insertions(+), 1 deletion(-)
> 

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 9eb76df..9310715 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.10)

Would it be possible/wise to ALWAYS output compression information, even
if it is the older style zlib with small window and default level?  Made
easier if you can represent all tunables of zlib (where you special-case
writing the qcow2 header to omit the extension header when the user has
requested the defaults).

This is a new feature, and unfortunately we've missed soft freeze for
2.10.  You'll want to update your series to uniformly state since 2.11.

-- 
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 V4 10/10] block/qcow2: add compress info to image specific info

[Peter Lieven]

Am 20.07.2017 um 18:05 schrieb Eric Blake:
> On 07/20/2017 09:20 AM, Peter Lieven wrote:
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>>  block/qcow2.c        | 9 +++++++++
>>  qapi/block-core.json | 6 +++++-
>>  2 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/qapi/block-core.json b/qapi/block-core.json
>> index 9eb76df..9310715 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.10)
> Would it be possible/wise to ALWAYS output compression information, even
> if it is the older style zlib with small window and default level?  Made
> easier if you can represent all tunables of zlib (where you special-case
> writing the qcow2 header to omit the extension header when the user has
> requested the defaults).

If we would store it in the header it will make the read and write of
the header much more complicated if we don't add the window size
to the default header (e.g. shrinking the format name to 14 and adding
to uint8_t for level and window size)

>
> This is a new feature, and unfortunately we've missed soft freeze for
> 2.10.  You'll want to update your series to uniformly state since 2.11.
>
Okay.