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

[Kevin Wolf]

Am 29.05.2019 um 13:40 hat Vladimir Sementsov-Ogievskiy geschrieben:
> 28.05.2019 17:37, Denis Plotnikov wrote:
> > The patch adds some preparation parts for incompatible compression type
> > feature to QCOW2 header that indicates that *all* compressed clusters
> > must be (de)compressed using a certain compression type.
> > 
> > It is implied that the compression type is set on the image creation and
> > can be changed only later by image conversion, thus compression type
> > defines the only compression algorithm used for the image.
> > 
> > The goal of the feature is to add support of other compression algorithms
> > to qcow2. For example, ZSTD which is more effective on compression than 
> > ZLIB.
> > It works roughly 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>
> > ---
> 
> [...]
> 
> > 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.
> >                                   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
> > +                                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.
> > +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.
> > +
> > +The compression type is set on image creation. The only way to change
> > +the compression type is to convert the image explicitly.
> > +
> > +The compression type extension is present if and only if the incompatible
> > +compression type bit is set. When the bit is not set the compression type
> > +header must be absent.
> 
> Hmm, not the first time we introduce incompatible bit to make incompatible
> header extension, as all header extensions are compatible by default, as for
> unknown header extension we have:
> 
>                          other      - Unknown header extension, can be safely
>                                       ignored
> 
> Hm. Should we instead define one incompatible bit for all such future cases,
> i.e. add incompatible bit HEADER_EXTENSION_FLAGS, add flag field to header
> extension declaration, and define one flag in it: COMPATIBLE, which will mean,
> that this extension may be safely ignored (old default behavior).

Would this actually make the implementation simpler, though?

I mean, the original idea was anyway that we'd just add new fields to
the header instead of using header extensions. Header extensions were
only meant for dynamically sized things like strings. That we would add
new fields to the header is why the header_length field even exists.

So in this case, we could consider using bytes 104-107 of the header for
compression_type instead of adding a new header extension.

Kevin