| Message ID | 1500560441-5670-9-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
On 07/20/2017 09:20 AM, Peter Lieven wrote: > we now pass the parameters to the zlib compressor if the > extension is present and use the old default values if > the extension is absent. > > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > block/qcow2-cluster.c | 58 ++++++++++++++++++++++++++++++--------------------- > block/qcow2.c | 57 +++++++++++++++++++++++++++----------------------- > 2 files changed, 65 insertions(+), 50 deletions(-) > static int decompress_buffer(uint8_t *out_buf, int out_buf_size, > - const uint8_t *buf, int buf_size) > + const uint8_t *buf, int buf_size, > + int compress_format) > { > - > - ret = inflateInit2(strm, -12); The old code initializes with 12, > + switch (compress_format) { > + case QCOW2_COMPRESS_FORMAT_ZLIB: > + case -1: { What is case -1 supposed to be? When the compression format was not specified? Again, I'd really like there a way to encode the default (when no format is encoded in the qcow2 file) to be representable alongside the new code, and then we just special-case writing the compression format to the file to omit the extension header if the user requested parameters that match the old default. > + z_stream z_strm = {}; > + > + z_strm.next_in = (uint8_t *)buf; > + z_strm.avail_in = buf_size; > + z_strm.next_out = out_buf; > + z_strm.avail_out = out_buf_size; > + > + ret = inflateInit2(&z_strm, -15); The new code is now unconditionally initializing with -15 instead of -12. Does that matter, or does decompression work regardless of window size used at creation, as long as the initialized size at decompression is at least as large? On the other hand, I guess that means if someone compresses with a large window, and then I initialize the decompressor with a small window, my decompression will fail? That's why knowing the minimum window size should be part of the spec, whether or not we make it a tunable. > +++ b/block/qcow2.c > @@ -3391,9 +3391,10 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, > BDRVQcow2State *s = bs->opaque; > QEMUIOVector hd_qiov; > struct iovec iov; > - z_stream strm; > - int ret, out_len; > - uint8_t *buf, *out_buf, *local_buf = NULL; > + z_stream z_strm = {}; > + int z_windowBits = -15, z_level = Z_DEFAULT_COMPRESSION; > + int ret, out_len = 0; > + uint8_t *buf, *out_buf = NULL, *local_buf = NULL; > uint64_t cluster_offset; > > if (bytes == 0) { > @@ -3418,34 +3419,38 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, > buf = qiov->iov[0].iov_base; > } > > - out_buf = g_malloc(s->cluster_size); > + switch (s->compress_format) { > + case -1: > + z_windowBits = -12; > + case QCOW2_COMPRESS_FORMAT_ZLIB: > + out_buf = g_malloc(s->cluster_size); > + if (s->compress_level > 0) { > + z_level = s->compress_level; > + } And this is where I wonder if z_windowBits should be explicitly encoded in the qcow2 format, rather than just magic numbers we use under the hood, so that someone else trying to reimplement things will create compatible files.
Am 20.07.2017 um 18:00 schrieb Eric Blake: > On 07/20/2017 09:20 AM, Peter Lieven wrote: >> we now pass the parameters to the zlib compressor if the >> extension is present and use the old default values if >> the extension is absent. >> >> Signed-off-by: Peter Lieven <pl@kamp.de> >> --- >> block/qcow2-cluster.c | 58 ++++++++++++++++++++++++++++++--------------------- >> block/qcow2.c | 57 +++++++++++++++++++++++++++----------------------- >> 2 files changed, 65 insertions(+), 50 deletions(-) >> static int decompress_buffer(uint8_t *out_buf, int out_buf_size, >> - const uint8_t *buf, int buf_size) >> + const uint8_t *buf, int buf_size, >> + int compress_format) >> { >> - >> - ret = inflateInit2(strm, -12); > The old code initializes with 12, > > >> + switch (compress_format) { >> + case QCOW2_COMPRESS_FORMAT_ZLIB: >> + case -1: { > What is case -1 supposed to be? When the compression format was not > specified? Again, I'd really like there a way to encode the default > (when no format is encoded in the qcow2 file) to be representable > alongside the new code, and then we just special-case writing the > compression format to the file to omit the extension header if the user > requested parameters that match the old default. > >> + z_stream z_strm = {}; >> + >> + z_strm.next_in = (uint8_t *)buf; >> + z_strm.avail_in = buf_size; >> + z_strm.next_out = out_buf; >> + z_strm.avail_out = out_buf_size; >> + >> + ret = inflateInit2(&z_strm, -15); > The new code is now unconditionally initializing with -15 instead of > -12. Does that matter, or does decompression work regardless of window > size used at creation, as long as the initialized size at decompression > is at least as large? On the other hand, I guess that means if someone > compresses with a large window, and then I initialize the decompressor > with a small window, my decompression will fail? That's why knowing the > minimum window size should be part of the spec, whether or not we make > it a tunable. The decompression is supposed to fail if you compress with 15 and decompress with 12. In fact it doesn't. > >> +++ b/block/qcow2.c >> @@ -3391,9 +3391,10 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, >> BDRVQcow2State *s = bs->opaque; >> QEMUIOVector hd_qiov; >> struct iovec iov; >> - z_stream strm; >> - int ret, out_len; >> - uint8_t *buf, *out_buf, *local_buf = NULL; >> + z_stream z_strm = {}; >> + int z_windowBits = -15, z_level = Z_DEFAULT_COMPRESSION; >> + int ret, out_len = 0; >> + uint8_t *buf, *out_buf = NULL, *local_buf = NULL; >> uint64_t cluster_offset; >> >> if (bytes == 0) { >> @@ -3418,34 +3419,38 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, >> buf = qiov->iov[0].iov_base; >> } >> >> - out_buf = g_malloc(s->cluster_size); >> + switch (s->compress_format) { >> + case -1: >> + z_windowBits = -12; >> + case QCOW2_COMPRESS_FORMAT_ZLIB: >> + out_buf = g_malloc(s->cluster_size); >> + if (s->compress_level > 0) { >> + z_level = s->compress_level; >> + } > And this is where I wonder if z_windowBits should be explicitly encoded > in the qcow2 format, rather than just magic numbers we use under the > hood, so that someone else trying to reimplement things will create > compatible files. > I would like to avoid the windowBits in the qcow2 header as it makes the code to read and write it more complicated. If you don't like the change of the windowBits we can even stick with 12. If someone wants fast compression he will likely not use zlib at all and use lzo. I just changed the windowBits to 15 as it increases speed and improves compression. (likely at the cost of memory during compression/decompression) Peter
On 07/20/2017 11:30 AM, Peter Lieven wrote: >> The new code is now unconditionally initializing with -15 instead of >> -12. Does that matter, or does decompression work regardless of window >> size used at creation, as long as the initialized size at decompression >> is at least as large? On the other hand, I guess that means if someone >> compresses with a large window, and then I initialize the decompressor >> with a small window, my decompression will fail? That's why knowing the >> minimum window size should be part of the spec, whether or not we make >> it a tunable. > > The decompression is supposed to fail if you compress with 15 and > decompress with 12. In fact it doesn't. Actually, I think (this is my guess here, not actual researched fact) that the decompression error is possible ONLY if compression produced a symbol that actually required more than 12 bits of memory - to get that large of a symbol, you need to compress a lot of bits. For our default cluster of 64k, it might very well be that you rarely, if ever, encounter a single cluster that compresses differently under window size 15 than it did under window size 12 (other than perhaps the speed at which compression took place), because there simply wasn't enough content to reach the point where you needed a symbol in the compression stream using more than 12 bits. So in that case, compressing under 15 and decompressing under 12 doesn't hit the error. But as you get larger cluster sizes (2M clusters), or perhaps if you pass particularly nasty sequences of input to compression (I'm not sure what sequences would have the right properties), then you do indeed result in a compression stream that starts to encounter symbols exceeding the window size. But if my guess is right, then don't read the docs as "decompression will fail", but rather as "decompression may fail" if you set the decompress window smaller than the compression window. > I would like to avoid the windowBits in the qcow2 header as it makes > > the code to read and write it more complicated. If you don't like the change > > of the windowBits we can even stick with 12. If someone wants fast compression > > he will likely not use zlib at all and use lzo. Also, note that historically, 'compress -b N' has allowed tuning the window size; current POSIX states that compress only has to support windows from 9 to 14, but permits implementations to use up to 16 (and future POSIX is considering improving the compress utility to require support for 16 as the window size, https://posix.rhansen.org/p/bug1041). I don't know why gzip didn't expose a '-b N' windowsize parameter the way compress did, but it sounds like the same thing. > > I just changed the windowBits to 15 as it increases speed and improves compression. Does windowBits 16 make any difference? > > (likely at the cost of memory during compression/decompression) > > > Peter > >
Am 20.07.2017 um 21:19 schrieb Eric Blake: > On 07/20/2017 11:30 AM, Peter Lieven wrote: > >>> The new code is now unconditionally initializing with -15 instead of >>> -12. Does that matter, or does decompression work regardless of window >>> size used at creation, as long as the initialized size at decompression >>> is at least as large? On the other hand, I guess that means if someone >>> compresses with a large window, and then I initialize the decompressor >>> with a small window, my decompression will fail? That's why knowing the >>> minimum window size should be part of the spec, whether or not we make >>> it a tunable. >> The decompression is supposed to fail if you compress with 15 and >> decompress with 12. In fact it doesn't. > Actually, I think (this is my guess here, not actual researched fact) > that the decompression error is possible ONLY if compression produced a > symbol that actually required more than 12 bits of memory - to get that > large of a symbol, you need to compress a lot of bits. For our default > cluster of 64k, it might very well be that you rarely, if ever, > encounter a single cluster that compresses differently under window size > 15 than it did under window size 12 (other than perhaps the speed at > which compression took place), because there simply wasn't enough > content to reach the point where you needed a symbol in the compression > stream using more than 12 bits. So in that case, compressing under 15 > and decompressing under 12 doesn't hit the error. But as you get larger > cluster sizes (2M clusters), or perhaps if you pass particularly nasty > sequences of input to compression (I'm not sure what sequences would > have the right properties), then you do indeed result in a compression > stream that starts to encounter symbols exceeding the window size. > > But if my guess is right, then don't read the docs as "decompression > will fail", but rather as "decompression may fail" if you set the > decompress window smaller than the compression window. > >> I would like to avoid the windowBits in the qcow2 header as it makes >> >> the code to read and write it more complicated. If you don't like the change >> >> of the windowBits we can even stick with 12. If someone wants fast compression >> >> he will likely not use zlib at all and use lzo. > Also, note that historically, 'compress -b N' has allowed tuning the > window size; current POSIX states that compress only has to support > windows from 9 to 14, but permits implementations to use up to 16 (and > future POSIX is considering improving the compress utility to require > support for 16 as the window size, https://posix.rhansen.org/p/bug1041). > I don't know why gzip didn't expose a '-b N' windowsize parameter the > way compress did, but it sounds like the same thing. > >> I just changed the windowBits to 15 as it increases speed and improves compression. > Does windowBits 16 make any difference? 16 is not allowed. Deflate allows only 2^8 through 2^15 window size. While we are talking, I thing we should choose not zlib but deflate for the compress format name. Peter
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index f06c08f..6c14d59 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1479,30 +1479,39 @@ again: } static int decompress_buffer(uint8_t *out_buf, int out_buf_size, - const uint8_t *buf, int buf_size) + const uint8_t *buf, int buf_size, + int compress_format) { - z_stream strm1, *strm = &strm1; - int ret, out_len; - - memset(strm, 0, sizeof(*strm)); - - strm->next_in = (uint8_t *)buf; - strm->avail_in = buf_size; - strm->next_out = out_buf; - strm->avail_out = out_buf_size; - - ret = inflateInit2(strm, -12); - if (ret != Z_OK) - return -1; - ret = inflate(strm, Z_FINISH); - out_len = strm->next_out - out_buf; - if ((ret != Z_STREAM_END && ret != Z_BUF_ERROR) || - out_len != out_buf_size) { - inflateEnd(strm); - return -1; - } - inflateEnd(strm); - return 0; + int ret = 0, out_len; + + switch (compress_format) { + case QCOW2_COMPRESS_FORMAT_ZLIB: + case -1: { + z_stream z_strm = {}; + + z_strm.next_in = (uint8_t *)buf; + z_strm.avail_in = buf_size; + z_strm.next_out = out_buf; + z_strm.avail_out = out_buf_size; + + ret = inflateInit2(&z_strm, -15); + if (ret != Z_OK) { + return -1; + } + ret = inflate(&z_strm, Z_FINISH); + out_len = z_strm.next_out - out_buf; + ret = -(ret != Z_STREAM_END); + inflateEnd(&z_strm); + break; + } + default: + abort(); /* should never reach this point */ + } + + if (out_len != out_buf_size) { + ret = -1; + } + return ret; } int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset) @@ -1523,7 +1532,8 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset) return ret; } if (decompress_buffer(s->cluster_cache, s->cluster_size, - s->cluster_data + sector_offset, csize) < 0) { + s->cluster_data + sector_offset, csize, + s->compress_format) < 0) { return -EIO; } s->cluster_cache_offset = coffset; diff --git a/block/qcow2.c b/block/qcow2.c index 978e8d2..57249cc 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3391,9 +3391,10 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, BDRVQcow2State *s = bs->opaque; QEMUIOVector hd_qiov; struct iovec iov; - z_stream strm; - int ret, out_len; - uint8_t *buf, *out_buf, *local_buf = NULL; + z_stream z_strm = {}; + int z_windowBits = -15, z_level = Z_DEFAULT_COMPRESSION; + int ret, out_len = 0; + uint8_t *buf, *out_buf = NULL, *local_buf = NULL; uint64_t cluster_offset; if (bytes == 0) { @@ -3418,34 +3419,38 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, buf = qiov->iov[0].iov_base; } - out_buf = g_malloc(s->cluster_size); + switch (s->compress_format) { + case -1: + z_windowBits = -12; + case QCOW2_COMPRESS_FORMAT_ZLIB: + out_buf = g_malloc(s->cluster_size); + if (s->compress_level > 0) { + z_level = s->compress_level; + } - /* best compression, small window, no zlib header */ - memset(&strm, 0, sizeof(strm)); - ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION, - Z_DEFLATED, -12, - 9, Z_DEFAULT_STRATEGY); - if (ret != 0) { - ret = -EINVAL; - goto fail; - } + ret = deflateInit2(&z_strm, z_level, Z_DEFLATED, z_windowBits, 9, + Z_DEFAULT_STRATEGY); + if (ret != Z_OK) { + ret = -EINVAL; + goto fail; + } - strm.avail_in = s->cluster_size; - strm.next_in = (uint8_t *)buf; - strm.avail_out = s->cluster_size; - strm.next_out = out_buf; + z_strm.avail_in = s->cluster_size; + z_strm.next_in = (uint8_t *)buf; + z_strm.avail_out = s->cluster_size; + z_strm.next_out = out_buf; - ret = deflate(&strm, Z_FINISH); - if (ret != Z_STREAM_END && ret != Z_OK) { - deflateEnd(&strm); - ret = -EINVAL; - goto fail; - } - out_len = strm.next_out - out_buf; + ret = deflate(&z_strm, Z_FINISH); + out_len = z_strm.next_out - out_buf; + deflateEnd(&z_strm); - deflateEnd(&strm); + ret = ret != Z_STREAM_END; + break; + default: + abort(); /* should never reach this point */ + } - if (ret != Z_STREAM_END || out_len >= s->cluster_size) { + if (ret || out_len >= s->cluster_size) { /* could not compress: write normal cluster */ ret = qcow2_co_pwritev(bs, offset, bytes, qiov, 0); if (ret < 0) {
we now pass the parameters to the zlib compressor if the extension is present and use the old default values if the extension is absent. Signed-off-by: Peter Lieven <pl@kamp.de> --- block/qcow2-cluster.c | 58 ++++++++++++++++++++++++++++++--------------------- block/qcow2.c | 57 +++++++++++++++++++++++++++----------------------- 2 files changed, 65 insertions(+), 50 deletions(-)