On 03.07.2019 17:36, Eric Blake wrote: > 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? > ok >> >> 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]). Is it really worth to do that? I liked Kevin's comment that the size should complain with some variable size > >> + if (dest_size < sizeof(*c_size)) { >> + return -ENOMEM; >> + } >> + >> + dest_size -= sizeof(*c_size); >> + >> + ret = ZSTD_compress(d_buf, dest_size, src, src_size, 5); > > The fact that you are storing the size separate from the data passed to > zstd MUST be documented in the qcow2 spec, for the next person to > produce/consume the same data. Should I add a separate chapter there? > > >> +++ b/qapi/block-core.json >> @@ -4215,11 +4215,12 @@ >> # Compression type used in qcow2 image file >> # >> # @zlib: zlib compression, see <http://zlib.net/> >> +# @zstd: zstd compression, see <http://github.com/facebook/zstd> >> # >> # Since: 4.1 >> ## >> { 'enum': 'Qcow2CompressionType', >> - 'data': [ 'zlib' ] } >> + 'data': [ 'zlib', 'zstd' ] } > > Since you patched configure so that linking against zstd is optional, > this should use { 'name':'zstd', 'if':'CONDITIONAL' } so that during > introspection, the enum only advertises zstd on a build that linked > against the library. Sure, will change it >
-- Best, Denis