| Message ID | 1498566850-7934-5-git-send-email-pl@kamp.de |
|---|---|
| State | New |
| Headers | show |
On 06/27/2017 07:34 AM, Peter Lieven wrote: > this adds support for optimized zlib settings which almost Start sentences with a capital. > tripples the compression speed while maintaining about s/tripples/triples/ > the same compressed size. > > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > block/qcow2-cluster.c | 3 ++- > block/qcow2.c | 11 +++++++++-- > block/qcow2.h | 1 + > qemu-img.texi | 1 + > 4 files changed, 13 insertions(+), 3 deletions(-) > > +++ b/block/qcow2.h > @@ -173,6 +173,7 @@ typedef struct Qcow2UnknownHeaderExtension { > enum { > QCOW2_COMPRESSION_ZLIB = 0xC0318301, > QCOW2_COMPRESSION_LZO = 0xC0318302, > + QCOW2_COMPRESSION_ZLIB_FAST = 0xC0318303, Back to my comments on 1/4 - we MUST first get the qcow2 specification right, rather than adding undocumented headers in the code. And I still think you only need one variable-length header extension for covering all possible algorithms, rather than one header per algorithm. Let's get the spec right first, before worrying about the code implementing the spec.
Am 27.06.2017 um 14:53 schrieb Eric Blake: > On 06/27/2017 07:34 AM, Peter Lieven wrote: >> this adds support for optimized zlib settings which almost > Start sentences with a capital. > >> tripples the compression speed while maintaining about > s/tripples/triples/ > >> the same compressed size. >> >> Signed-off-by: Peter Lieven <pl@kamp.de> >> --- >> block/qcow2-cluster.c | 3 ++- >> block/qcow2.c | 11 +++++++++-- >> block/qcow2.h | 1 + >> qemu-img.texi | 1 + >> 4 files changed, 13 insertions(+), 3 deletions(-) >> >> +++ b/block/qcow2.h >> @@ -173,6 +173,7 @@ typedef struct Qcow2UnknownHeaderExtension { >> enum { >> QCOW2_COMPRESSION_ZLIB = 0xC0318301, >> QCOW2_COMPRESSION_LZO = 0xC0318302, >> + QCOW2_COMPRESSION_ZLIB_FAST = 0xC0318303, > Back to my comments on 1/4 - we MUST first get the qcow2 specification > right, rather than adding undocumented headers in the code. And I still > think you only need one variable-length header extension for covering > all possible algorithms, rather than one header per algorithm. Let's > get the spec right first, before worrying about the code implementing > the spec. > Okay, I think someone came up with the idea to have an optional header per algorithm, but you are right one header with an optional parameter payload will also do. I will split the spec change to a separate patch in V2 to make it easier to respin. Thanks, Peter
On Tue, Jun 27, 2017 at 02:34:10PM +0200, Peter Lieven wrote: > this adds support for optimized zlib settings which almost > tripples the compression speed while maintaining about > the same compressed size. > > Signed-off-by: Peter Lieven <pl@kamp.de> > --- > block/qcow2-cluster.c | 3 ++- > block/qcow2.c | 11 +++++++++-- > block/qcow2.h | 1 + > qemu-img.texi | 1 + > 4 files changed, 13 insertions(+), 3 deletions(-) > diff --git a/qemu-img.texi b/qemu-img.texi > index 043c1ba..83a5db2 100644 > --- a/qemu-img.texi > +++ b/qemu-img.texi > @@ -627,6 +627,7 @@ The following options are available if support for the respective libraries > has been enabled at compile time: > > zlib Uses standard zlib compression (default) > + zlib-fast Uses zlib compression with optimized compression parameters This looks like a poor design from a future proofing POV. I would much rather see us introduce a more flexible modelling of compression at the QAPI layer which lets us have tunables for each algorith, in the same way that the qcow2 built-in encryption now has ability to set tunables for each algorithm. eg your "zlib-fast" impl which just uses zlib with a window size of -15 could be expressed as qemu-img create -o compress.format=zlib,compress.window=-15 > lzo Uses LZO1X compression Regards, Daniel
Am 27.06.2017 um 15:16 schrieb Daniel P. Berrange: > On Tue, Jun 27, 2017 at 02:34:10PM +0200, Peter Lieven wrote: >> this adds support for optimized zlib settings which almost >> tripples the compression speed while maintaining about >> the same compressed size. >> >> Signed-off-by: Peter Lieven <pl@kamp.de> >> --- >> block/qcow2-cluster.c | 3 ++- >> block/qcow2.c | 11 +++++++++-- >> block/qcow2.h | 1 + >> qemu-img.texi | 1 + >> 4 files changed, 13 insertions(+), 3 deletions(-) >> diff --git a/qemu-img.texi b/qemu-img.texi >> index 043c1ba..83a5db2 100644 >> --- a/qemu-img.texi >> +++ b/qemu-img.texi >> @@ -627,6 +627,7 @@ The following options are available if support for the respective libraries >> has been enabled at compile time: >> >> zlib Uses standard zlib compression (default) >> + zlib-fast Uses zlib compression with optimized compression parameters > This looks like a poor design from a future proofing POV. I would much > rather see us introduce a more flexible modelling of compression at the > QAPI layer which lets us have tunables for each algorith, in the same > way that the qcow2 built-in encryption now has ability to set tunables > for each algorithm. > > eg your "zlib-fast" impl which just uses zlib with a window size of -15 > could be expressed as > > qemu-img create -o compress.format=zlib,compress.window=-15 It would also need a compress.level, but okay. This will make things more complicated as one might not know what good values for the algoritm are. Wouldn't it be better to have sth like compress.level=1..9 and let the implementation decide which parameters it choose for the algorithm? Do you have a pointer where I can find out how to imlement that in QAPI? Maybe the patches that introduces the encryption settings? Thanks, Peter
On Tue, Jun 27, 2017 at 03:23:19PM +0200, Peter Lieven wrote: > Am 27.06.2017 um 15:16 schrieb Daniel P. Berrange: > > On Tue, Jun 27, 2017 at 02:34:10PM +0200, Peter Lieven wrote: > > > this adds support for optimized zlib settings which almost > > > tripples the compression speed while maintaining about > > > the same compressed size. > > > > > > Signed-off-by: Peter Lieven <pl@kamp.de> > > > --- > > > block/qcow2-cluster.c | 3 ++- > > > block/qcow2.c | 11 +++++++++-- > > > block/qcow2.h | 1 + > > > qemu-img.texi | 1 + > > > 4 files changed, 13 insertions(+), 3 deletions(-) > > > diff --git a/qemu-img.texi b/qemu-img.texi > > > index 043c1ba..83a5db2 100644 > > > --- a/qemu-img.texi > > > +++ b/qemu-img.texi > > > @@ -627,6 +627,7 @@ The following options are available if support for the respective libraries > > > has been enabled at compile time: > > > zlib Uses standard zlib compression (default) > > > + zlib-fast Uses zlib compression with optimized compression parameters > > This looks like a poor design from a future proofing POV. I would much > > rather see us introduce a more flexible modelling of compression at the > > QAPI layer which lets us have tunables for each algorith, in the same > > way that the qcow2 built-in encryption now has ability to set tunables > > for each algorithm. > > > > eg your "zlib-fast" impl which just uses zlib with a window size of -15 > > could be expressed as > > > > qemu-img create -o compress.format=zlib,compress.window=-15 > > It would also need a compress.level, but okay. This will make things > more complicated as one might not know what good values for > the algoritm are. > > Wouldn't it be better to have sth like compress.level=1..9 and let > the implementation decide which parameters it choose for the algorithm? Yep, there's definitely a choice of approaches - exposing low level details of each library as parameters, vs trying to come up with a more abstracted, simplified notation and having the driver pick some of the low level details implicitly from that. Both are valid, and I don't have a particular opinion either way. > Do you have a pointer where I can find out how to imlement that > in QAPI? Maybe the patches that introduces the encryption settings? It is a little messy since we don't fully use QAPI in the block create code path. If you want to look at what I did for encryption though, see the block/qcow2.c file. In the qcow2_create() method I grab the encrypt.format option. Later in the qcow2_set_up_encryption() method I then extract & parse the format specific options from the QemuOpts array. My code is in Max's block branch, or on block-luks-qcow2-10 branch at https://github.com/berrange/qemu Regards, Daniel
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index ecb059b..d8e2378 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1532,6 +1532,7 @@ static int decompress_buffer(uint8_t *out_buf, int out_buf_size, switch (compression_algorithm_id) { case QCOW2_COMPRESSION_ZLIB: + case QCOW2_COMPRESSION_ZLIB_FAST: memset(strm, 0, sizeof(*strm)); strm->next_in = (uint8_t *)buf; @@ -1539,7 +1540,7 @@ static int decompress_buffer(uint8_t *out_buf, int out_buf_size, strm->next_out = out_buf; strm->avail_out = out_buf_size; - ret = inflateInit2(strm, -12); + ret = inflateInit2(strm, -15); if (ret != Z_OK) { return -1; } diff --git a/block/qcow2.c b/block/qcow2.c index bd65582..f07d8f0 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -87,6 +87,8 @@ static uint32_t is_compression_algorithm_supported(char *algorithm) /* no algorithm means the old default of zlib compression * with 12 window bits */ return QCOW2_COMPRESSION_ZLIB; + } else if (!strcmp(algorithm, "zlib-fast")) { + return QCOW2_COMPRESSION_ZLIB_FAST; #ifdef CONFIG_LZO } else if (!strcmp(algorithm, "lzo")) { return QCOW2_COMPRESSION_LZO; @@ -2722,6 +2724,7 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, QEMUIOVector hd_qiov; struct iovec iov; z_stream strm; + int z_level = Z_DEFAULT_COMPRESSION, z_windowBits = -12; int ret, out_len = 0; uint8_t *buf, *out_buf = NULL, *local_buf = NULL, *work_buf = NULL; uint64_t cluster_offset; @@ -2749,13 +2752,17 @@ qcow2_co_pwritev_compressed(BlockDriverState *bs, uint64_t offset, } switch (s->compression_algorithm_id) { + case QCOW2_COMPRESSION_ZLIB_FAST: + z_level = Z_BEST_SPEED; + z_windowBits = -15; + /* fall-through */ case QCOW2_COMPRESSION_ZLIB: out_buf = g_malloc(s->cluster_size); /* best compression, small window, no zlib header */ memset(&strm, 0, sizeof(strm)); - ret = deflateInit2(&strm, Z_DEFAULT_COMPRESSION, - Z_DEFLATED, -12, + ret = deflateInit2(&strm, z_level, + Z_DEFLATED, z_windowBits, 9, Z_DEFAULT_STRATEGY); if (ret != 0) { ret = -EINVAL; diff --git a/block/qcow2.h b/block/qcow2.h index 716012c..a89f986 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -173,6 +173,7 @@ typedef struct Qcow2UnknownHeaderExtension { enum { QCOW2_COMPRESSION_ZLIB = 0xC0318301, QCOW2_COMPRESSION_LZO = 0xC0318302, + QCOW2_COMPRESSION_ZLIB_FAST = 0xC0318303, }; enum { diff --git a/qemu-img.texi b/qemu-img.texi index 043c1ba..83a5db2 100644 --- a/qemu-img.texi +++ b/qemu-img.texi @@ -627,6 +627,7 @@ The following options are available if support for the respective libraries has been enabled at compile time: zlib Uses standard zlib compression (default) + zlib-fast Uses zlib compression with optimized compression parameters lzo Uses LZO1X compression The compression algorithm can only be defined at image create time and cannot
this adds support for optimized zlib settings which almost tripples the compression speed while maintaining about the same compressed size. Signed-off-by: Peter Lieven <pl@kamp.de> --- block/qcow2-cluster.c | 3 ++- block/qcow2.c | 11 +++++++++-- block/qcow2.h | 1 + qemu-img.texi | 1 + 4 files changed, 13 insertions(+), 3 deletions(-)