From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58645) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bN369-00071A-K0 for qemu-devel@nongnu.org; Tue, 12 Jul 2016 15:12:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bN365-0004jb-D2 for qemu-devel@nongnu.org; Tue, 12 Jul 2016 15:12:00 -0400 References: <1468345431-106198-1-git-send-email-vsementsov@virtuozzo.com> <57853A35.4030501@redhat.com> From: Vladimir Sementsov-Ogievskiy Message-ID: <578540F4.2070309@virtuozzo.com> Date: Tue, 12 Jul 2016 22:11:48 +0300 MIME-Version: 1.0 In-Reply-To: <57853A35.4030501@redhat.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] qcow2: do not allocate extra memory List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Eric Blake , qemu-devel@nongnu.org, qemu-block@nongnu.org Cc: mreitz@redhat.com, kwolf@redhat.com, stefanha@redhat.com, den@openvz.org, fabrice@bellard.org On 12.07.2016 21:43, Eric Blake wrote: > On 07/12/2016 11:43 AM, 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 >> --- >> >> Hi all! >> >> Please, can anybody say me what I'm missing? >> > https://tools.ietf.org/html/rfc1951 states a simple fact about compression: > > A simple counting argument shows that no lossless compression > algorithm can compress every possible input data set. For the > format defined here, the worst case expansion is 5 bytes per 32K- > byte block, i.e., a size increase of 0.015% for large data sets. > > So overallocating the output buffer guarantees that you will get a valid > compression result via a single function call, even when the data is > incompressible (the zlib format specifically documents that if the > normal algorithm on the data does not reduce its size, then you merely > add a fixed-length marker that documents that fact, so you at least > avoid unbounded expansion when trying to compress pathological data). > > But since the qcow2 format already has a way of documenting whether a > cluster is compressed or not, we probably don't have to rely on zlib's > marker for uncompressible data, and could instead tweak the code to > specifically refuse to compress any cluster whose output would result in > more than a cluster's worth of bytes. I'm not familiar enough with > zlib's interface to know how easy or hard this is, and whether merely > checking error codes is sufficient, nor whether qemu's use of zlib would > behave correctly in the face of such an error when the output buffer is > undersized because the data was incompressible. zlib doc says: "deflate compresses as much data as possible, and stops when the input buffer becomes empty or the output buffer becomes full" It will not write more then avail_out bytes to out buffer. > > >> ... >> strm.avail_out = s->cluster_size; >> strm.next_out = out_buf; >> >> ret = deflate(&strm, Z_FINISH); >> ... >> out_len = strm.next_out - out_buf; > You've skipped what is done with ret, which will be different according > to whether the entire compressed stream fit in the buffer described by > strm, and that would have to be audited as part of your proposed patch. ret would be Z_STREAM_END if it fit in and Z_OK if not. (if there are no errors ofcourse). What I've skipped? I just say that nobody knows about this extra allocation - neither zlib nor other code in this function (except g_free=). > > >> - out_buf = g_malloc(s->cluster_size + (s->cluster_size / 1000) + 128); >> + out_buf = g_malloc(s->cluster_size); > Is avoiding the fudge factor really worth it? I don't know that we'll > get a noticeable performance gain with this patch, and it may be easier > to leave things alone than to audit that we are correctly handling cases > where the attempt at compression results in a zlib buffer larger than > the original data, even when the output buffer size is now constrained > differently. > I'm not insist on applying this patch, I am just trying to understand this. -- Best regards, Vladimir