Re: [Qemu-devel] [PATCH v1 3/3] qcow2: add zstd cluster compression

[Kevin Wolf <kwolf@redhat.com>]

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

Am 03.07.2019 um 16:36 hat Eric Blake geschrieben:
> On 7/3/19 6:00 AM, Denis Plotnikov wrote:
> > zstd significantly reduces cluster compression time.
> > It provides better compression performance maintaining
> > the same level of compression ratio in comparison with
> > zlib, which, by the moment, has been the only compression
> > method available.
> > 
> 
> > ---
> >  block/qcow2.c        | 96 ++++++++++++++++++++++++++++++++++++++++++++
> >  configure            | 32 +++++++++++++++
> >  qapi/block-core.json |  3 +-
> >  3 files changed, 130 insertions(+), 1 deletion(-)
> 
> Where is the change to docs/interop/qcow2.txt to describe this new
> compression format?
> 
> > 
> > diff --git a/block/qcow2.c b/block/qcow2.c
> > index 37a563a671..caa04b0beb 100644
> > --- a/block/qcow2.c
> > +++ b/block/qcow2.c
> > @@ -27,6 +27,11 @@
> >  #define ZLIB_CONST
> >  #include <zlib.h>
> >  
> 
> > +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
> > +                                   const void *src, size_t src_size)
> > +{
> > +    ssize_t ret;
> > +    uint32_t *c_size = dest;
> > +    /* steal some bytes to store compressed chunk size */
> > +    char *d_buf = ((char *) dest) + sizeof(*c_size);
> > +
> 
> Do you always want exactly 4 bytes for the compressed size? Or is it
> worth some sort of variable-length encoding, since we're already dealing
> with non-cacheline-aligned data? You can represent all sizes up to 4M
> using a maximum of 3 bytes (set the high bit if the integer continues,
> then sizes 0-127 take 1 byte [7 bits], 128-32767 take 2 bytes [15 bits],
> and 32768-4194303 take 3 bytes [22 bits]).

I don't think the additional complexity is worth it. The combination of
endianness conversions (which are missing here, this is a bug!) and
variable lengths is almost guaranteed to not only give us headaches, but
also to result in corner case bugs.

Also, are we sure that we will always keep 2M as the maximum cluster
size?

Kevin

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