Re: [Qemu-devel] [Qemu-block] [PATCH] qcow2: do not allocate extra memory

[John Snow <jsnow@redhat.com>]

On 07/12/2016 01:43 PM, Vladimir Sementsov-Ogievskiy wrote:
> There are no needs to allocate more than one cluster, as we set
> avail_out for deflate to one cluster.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
> 
> Hi all!
> 
> Please, can anybody say me what I'm missing?
> 

Not the first mystery padding I've seen from a Blue Swirl checkin.

I can't figure out the purpose here either.

> I've looked through deflate documentation at
> http://www.zlib.net/manual.html, and I didn't find anything about
> allocating more memory for out buffer than specified in avail_out
> field.. What is this magic formula?
> 

Here's my guess. the qcow2 write compressed function *tries* to compress
a sector. Sometimes it isn't able to and the output data may be larger
than the input data.

In these cases, perhaps we were trying to allow enough buffer room for
the worst cast *expansion*. We check the length of the output data after
calling deflate to see if it actually increased.

Notably, since we only ever give it s->cluster_size, though, the out_len
can only ever be as big as s->cluster_size, which makes the >=
comparison a little misleading later.

Even if we could store something compressed if it was precisely 64KB as
an example, it's probably the right answer to store such things
uncompressed, so "==" (or >=) is probably an OK condition ... even if we
limit the buffer to exactly 64KB.

Where did s->cluster_size/1000 + 128 come from? No idea: AFAICT zlib
advertises a maximum overhead of 5 bytes per 16KB with a one-time
overhead of 6 bytes, so it should look something more like:

DIV_ROUND_UP(s->cluster_size, 16384) * 5 + 6

Even so, maybe we don't need it and we can just trash the result soundly
if we fill up a single cluster_size buffer.

> ========
> 
> All uses of out_buf in the function:
> 
> uint8_t *out_buf;
> ...
> out_buf = g_malloc(s->cluster_size + (s->cluster_size / 1000) + 128);
> ...
> strm.avail_out = s->cluster_size;
> strm.next_out = out_buf;
> 
> ret = deflate(&strm, Z_FINISH);
> ...
> out_len = strm.next_out - out_buf;
> ...
> ret = bdrv_pwrite(bs->file, cluster_offset, out_buf, out_len);
> ...
> g_free(out_buf);
> 
>  block/qcow.c  | 2 +-
>  block/qcow2.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/block/qcow.c b/block/qcow.c
> index ac849bd..d8826f3 100644
> --- a/block/qcow.c
> +++ b/block/qcow.c
> @@ -983,7 +983,7 @@ static int qcow_write_compressed(BlockDriverState *bs, int64_t sector_num,
>          return ret;
>      }
>  
> -    out_buf = g_malloc(s->cluster_size + (s->cluster_size / 1000) + 128);
> +    out_buf = g_malloc(s->cluster_size);
>  
>      /* best compression, small window, no zlib header */
>      memset(&strm, 0, sizeof(strm));
> diff --git a/block/qcow2.c b/block/qcow2.c
> index a5ea19b..b1c90ae 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -2612,7 +2612,7 @@ static int qcow2_write_compressed(BlockDriverState *bs, int64_t sector_num,
>          return ret;
>      }
>  
> -    out_buf = g_malloc(s->cluster_size + (s->cluster_size / 1000) + 128);
> +    out_buf = g_malloc(s->cluster_size);
>  
>      /* best compression, small window, no zlib header */
>      memset(&strm, 0, sizeof(strm));
>