Re: [PATCH v6 3/4] qcow2: add zstd cluster compression

[Eric Blake]

On 3/12/20 4:22 AM, Denis Plotnikov wrote:
> zstd significantly reduces cluster compression time.
> It provides better compression performance maintaining
> the same level of the compression ratio in comparison with
> zlib, which, at the moment, is the only compression
> method available.

> +++ b/docs/interop/qcow2.txt
> @@ -208,6 +208,7 @@ version 2.
>   
>                       Available compression type values:
>                           0: zlib <https://www.zlib.net/>
> +                        1: zstd <http://github.com/facebook/zstd>
>   
>   
>   === Header padding ===
> @@ -575,11 +576,30 @@ Compressed Clusters Descriptor (x = 62 - (cluster_bits - 
> 8)):
>                       Another compressed cluster may map to the tail of the 
> final
>                       sector used by this compressed cluster.
>   
> +                    The layout of the compressed data depends on the compression
> +                    type used for the image (see compressed cluster layout).
> +
>   If a cluster is unallocated, read requests shall read the data from the 
> backing
>   file (except if bit 0 in the Standard Cluster Descriptor is set). If there is
>   no backing file or the backing file is smaller than the image, they shall read
>   zeros for all parts that are not covered by the backing file.
>   
> +=== Compressed Cluster Layout ===
> +
> +The compressed cluster data has a layout depending on the compression
> +type used for the image, as follows:
> +
> +Compressed data layout for the available compression types:
> +data_space_lenght - data chunk length available to store a compressed cluster.

length

> +(for more details see "Compressed Clusters Descriptor")
> +x = data_space_length - 1

If I understand correctly, data_space_length is really an upper bounds 
on the length available, because it is computed by rounding UP to the 
next 512-byte boundary (that is, the L2 descriptor lists the number of 
additional sectors used in storing the compressed data).  Which really 
means that we have the following, where + is cluster boundaries, S and E 
are the start and end of the compressed data, and D is the offset 
determined by data_space_length:

+-------+-------+------+
      S============E...D

> +
> +    0:  (default)  zlib <http://zlib.net/>:
> +            Byte  0 -  x:     the compressed data content
> +                              all the space provided used for compressed data

For zlib, we have byte 0-E are compressed data, and bytes (E+1)-D (if 
any) are ignored.  There is no way to tell how many bytes between E and 
D exist, because zlib doesn't care (the compression stream itself 
ensures that decompression stops when input reaches E because the output 
reached a cluster boundary at that point).

> +    1:  zstd <http://github.com/facebook/zstd>:
> +            Byte  0 -  3:     the length of compressed data in bytes
> +                  4 -  x:     the compressed data content

Whereas for zstd, the decompression MUST know the actual location of E, 
rather than passing in the slop between E and D; bytes 0-3 give us that 
information.

But your description is not very accurate:  if 'x' is point E, then it 
is NOT data_space_length - 1, but rather data_space_length - slop, where 
slop can be up to 511 bytes (the number of bytes from (E+1) to D).  And 
if 'x' is point E, then the real layout for zlib is:

byte 0 - E: the compressed data content
byte E+1 - x: ignored slop (E is implied solely by the compressed data)

and for zstd is:

byte 0 - 3: the length of the compressed data
byte 4 - E: the compressed data (E computed from byte 0-3)
byte E+1 - x: ignored

I'm not sure what the best way is to document this.

> +++ b/block/qcow2-threads.c

> +static ssize_t qcow2_zstd_compress(void *dest, size_t dest_size,
> +                                   const void *src, size_t src_size)
> +{
> +    size_t ret;
> +
> +    /*
> +     * steal ZSTD_LEN_BUF bytes in the very beginning of the buffer
> +     * to store compressed chunk size
> +     */
> +    char *d_buf = ((char *) dest) + ZSTD_LEN_BUF;
> +
> +    /*
> +     * sanity check that we can store the compressed data length,
> +     * and there is some space left for the compressor buffer
> +     */
> +    if (dest_size <= ZSTD_LEN_BUF) {
> +        return -ENOMEM;
> +    }
> +
> +    dest_size -= ZSTD_LEN_BUF;
> +
> +    ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5);

Where does the magic number 5 come from?

> +
> +    if (ZSTD_isError(ret)) {
> +        if (ZSTD_getErrorCode(ret) == ZSTD_error_dstSize_tooSmall) {
> +            return -ENOMEM;
> +        } else {
> +            return -EIO;
> +        }
> +    }
> +
> +    /*
> +     * paranoid sanity check that we can store
> +     * the compressed size in the first 4 bytes
> +     */
> +    if (ret > UINT32_MAX) {
> +        return -ENOMEM;
> +    }

The if is awkward.  I'd prefer to change this to:

    /*
     * Our largest cluster is 2M, and we insist that compression
     * actually compressed things.
     */
    assert(ret < UINT32_MAX);

or even tighten to assert(ret <= dest_size)

> +
> +    /* store the compressed chunk size in the very beginning of the buffer */
> +    stl_be_p(dest, ret);
> +
> +    return ret + ZSTD_LEN_BUF;
> +}
> +
> +/*
> + * qcow2_zstd_decompress()
> + *
> + * Decompress some data (not more than @src_size bytes) to produce exactly
> + * @dest_size bytes using zstd compression method
> + *
> + * @dest - destination buffer, @dest_size bytes
> + * @src - source buffer, @src_size bytes
> + *
> + * Returns: 0 on success
> + *          -EIO on any error
> + */
> +static ssize_t qcow2_zstd_decompress(void *dest, size_t dest_size,
> +                                     const void *src, size_t src_size)
> +{
> +    /*
> +     * zstd decompress wants to know the exact length of the data.
> +     * For that purpose, on compression, the length is stored in
> +     * the very beginning of the compressed buffer
> +     */
> +    size_t s_size;
> +    const char *s_buf = ((const char *) src) + ZSTD_LEN_BUF;
> +
> +    /*
> +     * sanity check that we can read 4 byte the content length and
> +     * and there is some content to decompress
> +     */
> +    if (src_size <= ZSTD_LEN_BUF) {
> +        return -EIO;
> +    }
> +
> +    s_size = ldl_be_p(src);
> +
> +    /* sanity check that the buffer is big enough to read the content from */
> +    if (src_size - ZSTD_LEN_BUF < s_size) {
> +        return -EIO;
> +    }
> +
> +    if (ZSTD_isError(
> +            ZSTD_decompress(dest, dest_size, s_buf, s_size))) {

You are correct that ZSTD_decompress() is picky that it must be given 
the exact size of the compressed buffer it is decompressing.  But the 
ZSTD manual mentions that if an exact size is not known in advance, that 
the streaming API can be used instead:

https://facebook.github.io/zstd/zstd_manual.html#Chapter9

In other words, would it be possible to NOT have to prepend four bytes 
of exact size information, by instead setting up decompression via the 
streaming API where the input is (usually) oversized, but the output 
buffer limited to exactly one cluster is sufficient to consume the exact 
compressed data and ignore the slop, just as we do in zlib?

The rest of this patch looks okay.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org