Re: [Qemu-devel] [PATCH v0 1/3] qcow2: introduce compression type feature

[Markus Armbruster]

Doc & QAPI schema review only.

Denis Plotnikov <address@hidden> writes:

> The patch adds some preparation parts for incompatible compression type
> feature to QCOW2 header that indicates that *all* compressed clusters
> must be (de)compressed using a certain compression type.
>
> It is implied that the compression type is set on the image creation and
> can be changed only later by image conversion, thus compression type
> defines the only compression algorithm used for the image.
>
> The goal of the feature is to add support of other compression algorithms
> to qcow2. For example, ZSTD which is more effective on compression than ZLIB.
> It works roughly x2 faster than ZLIB providing a comparable compression ratio
> and therefore provide a performance advantage in backup scenarios.
>
> The default compression is ZLIB. Images created with ZLIB compression type
> is backward compatible with older qemu versions.
>
> Signed-off-by: Denis Plotnikov <address@hidden>
> ---
>  block/qcow2.c             | 61 +++++++++++++++++++++++++++++++++++++++
>  block/qcow2.h             | 29 ++++++++++++++-----
>  docs/interop/qcow2.txt    | 37 +++++++++++++++++++++++-
>  include/block/block_int.h |  1 +
>  qapi/block-core.json      | 34 ++++++++++++++++++++--
>  5 files changed, 151 insertions(+), 11 deletions(-)
[...]
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index af5711e533..cebcbc4f2f 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -109,7 +109,14 @@ in the description of a field.
                       Bit 0:      Dirty bit.  If this bit is set then refcounts
                                   may be inconsistent, make sure to scan L1/L2
                                   tables to repair refcounts before accessing 
the
                                   image.

                       Bit 1:      Corrupt bit.  If this bit is set then any 
data
                                   structure may be corrupt and the image must 
not
                                   be written to (unless for regaining
                                   consistency).

                       Bit 2:      External data file bit.  If this bit is set, 
an
                                   external data file is used. Guest clusters 
are
                                   then stored in the external data file. For 
such
                                   images, clusters in the external data file 
are
                                   not refcounted. The offset field in the
                                   Standard Cluster Descriptor must match the
                                   guest offset and neither compressed clusters
                                   nor internal snapshots are supported.

>                                  An External Data File Name header extension 
> may
>                                  be present if this bit is set.
>  
> -                    Bits 3-63:  Reserved (set to 0)
> +                    Bit 3:      Compression type. If the bit is set, then the

"Compression type bit", for consistency with the other three.

> +                                type of compression the image uses is set in 
> the
> +                                header extension. When the bit is set the
> +                                compression type extension header must be 
> present.
> +                                When the bit is not set the compression type
> +                                header must absent.
> +
> +                    Bits 4-63:  Reserved (set to 0)
>  
>           80 -  87:  compatible_features
>                      Bitmask of compatible features. An implementation can
> @@ -175,6 +182,7 @@ be stored. Each extension has a structure like the 
> following:
>                          0x23852875 - Bitmaps extension
>                          0x0537be77 - Full disk encryption header pointer
>                          0x44415441 - External data file name string
> +                        0x434D5052 - Compression type extension
>                          other      - Unknown header extension, can be safely
>                                       ignored
>  
> @@ -771,3 +779,30 @@ In the image file the 'enabled' state is reflected by 
> the 'auto' flag. If this
>  flag is set, the software must consider the bitmap as 'enabled' and start
>  tracking virtual disk changes to this bitmap from the first write to the
>  virtual disk. If this flag is not set then the bitmap is disabled.
> +
> +
> +== Compression type extension ==
> +
> +The compression type extension is an optional header extension. It stores the
> +compression type used for disk clusters (de)compression.

s/clusters/cluster/

> +A single compression type is applied to all compressed disk clusters,
> +with no way to change compression types per cluster. Two clusters of the 
> image
> +couldn't be compressed with different compression types.

Is the text after the first comma useful?

> +
> +The compression type is set on image creation. The only way to change
> +the compression type is to convert the image explicitly.

Suggest to scratch "explicitly".

> +
> +The compression type extension is present if and only if the incompatible
> +compression type bit is set.

Suggest "if the compression type bit (incompatible feature bit 3) is
set", for consistency with existing references to incompatible feature
bits.

>                               When the bit is not set the compression type
> +header must be absent.

This sentence is redundant with "if and only if".  Suggest to drop it.

> +
> +When the compression type bit is not set and the compression type header

Suggest "not set, and".

> +extension is absent, ZLIB compression is used for compressed clusters.
> +This defines default image compression type: ZLIB.

I find this sentence confusing.  Can we drop it?

> +Qemu < 4.1 can use images created with compression type ZLIB without any
> +additional preparations and cannot use images created with compression
> +types != ZLIB.

Suggest "QEMU versions older than 4.1 can only use images created with
compression type ZLIB".

> +
> +Available compression types:
> +    0: ZLIB
> +    1: ZSTD

Please add brief explanations with pointers to additional information
for both.  Something like

       0: zlib compression, see <http://zlib.net/>
       1: zstd compression, see FIXME

Mention RFC 1950, 1951 and 1952 for zlib if you like.

> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 01e855a066..814917baec 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -58,6 +58,7 @@
>  #define BLOCK_OPT_REFCOUNT_BITS     "refcount_bits"
>  #define BLOCK_OPT_DATA_FILE         "data_file"
>  #define BLOCK_OPT_DATA_FILE_RAW     "data_file_raw"
> +#define BLOCK_OPT_COMPRESSION_TYPE  "compression_type"
>  
>  #define BLOCK_PROBE_BUF_SIZE        512
>  
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 7ccbfff9d0..59610153fd 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -78,6 +78,9 @@
>  #
>  # @bitmaps: A list of qcow2 bitmap details (since 4.0)
>  #
> +# @compression-type: the compression method used for image clusters
> +#                    compression (since 4.1)

s/clusters/cluster/, I think.

More laconic: the image cluster compression method.

> +#
>  # Since: 1.7
>  ##
>  { 'struct': 'ImageInfoSpecificQCow2',
> @@ -89,7 +92,8 @@
>        '*corrupt': 'bool',
>        'refcount-bits': 'int',
>        '*encrypt': 'ImageInfoSpecificQCow2Encryption',
> -      '*bitmaps': ['Qcow2BitmapInfo']
> +      '*bitmaps': ['Qcow2BitmapInfo'],
> +      '*compression-type': 'Qcow2CompressionType'
>    } }
>  
>  ##
> @@ -3119,6 +3123,10 @@
>  #                         an image, the data file name is loaded from the 
> image
>  #                         file. (since 4.0)
>  #
> +# @compression-type:      compression method to use for image clusters 
> compression

Likewise.

> +#                         The comression method is set on image creation and 
> can

Typo: s/comression/compression/

> +#                         be changed via image converting only. (since 4.1)

I.e. it can't be changed.  Perhaps: "The compression method is fixed at
image creation time.  To change it, you have to use qemu-img convert."

Hmm.  BlockdevOptionsQcow2 has the qcow2-specific options for -blockdev.
What does passing @compression-type to -blockdev mean?  The actual
compression type is determined by the qcow2 image...  Am I confused?

> +#
>  # Since: 2.9
>  ##
>  { 'struct': 'BlockdevOptionsQcow2',
> @@ -3134,7 +3142,8 @@
>              '*refcount-cache-size': 'int',
>              '*cache-clean-interval': 'int',
>              '*encrypt': 'BlockdevQcow2Encryption',
> -            '*data-file': 'BlockdevRef' } }
> +            '*data-file': 'BlockdevRef',
> +            '*compression-type': 'Qcow2CompressionType' } }
>  
>  ##
>  # @SshHostKeyCheckMode:
> @@ -4206,6 +4215,19 @@
>    'data': [ 'v2', 'v3' ] }
>  
>  
> +##
> +# @Qcow2CompressionType:
> +#
> +# Compression type used in qcow2 image file
> +#
> +# @zlib     : gzip compressor
> +# @zstd     : zstd compression

Two nits.  One, we don't normally put spaces before the colon.  Two,
compressor vs. compression.

But I'd suggest something like

    @zlib: zlib compression, see <http://zlib.net/>

Mention RFC 1950, 1951 and 1952 if you like.

Same for @zstd.

> +#
> +# Since: 4.1
> +##
> +{ 'enum': 'Qcow2CompressionType',
> +  'data': [ 'zlib', 'zstd' ] }
> +
>  ##
>  # @BlockdevCreateOptionsQcow2:
>  #
> @@ -4228,6 +4250,11 @@
>  # @preallocation    Preallocation mode for the new image (default: off)
>  # @lazy-refcounts   True if refcounts may be updated lazily (default: off)
>  # @refcount-bits    Width of reference counts in bits (default: 16)
> +# @compression-type Compression method used for image compressed clusters

More laconic: the image cluster compression method.

> +#                   (default: zlib(gzip), since 4.1).

The default is "zlib", not "zlib(gzip)".  If you want to explain what
zlib is, do it in Qcow2CompressionType's doc string.

> +#                   Available types:
> +#                       zlib
> +#                       zstd

Isn't this redundant?

>  #
>  # Since: 2.12
>  ##
> @@ -4243,7 +4270,8 @@
>              '*cluster-size':    'size',
>              '*preallocation':   'PreallocMode',
>              '*lazy-refcounts':  'bool',
> -            '*refcount-bits':   'int' } }
> +            '*refcount-bits':   'int',
> +            '*compression-type': 'Qcow2CompressionType' } }
>  
>  ##
>  # @BlockdevCreateOptionsQed: