[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v7 6/6] qcow2: Avoid memory over-allocation on c
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v7 6/6] qcow2: Avoid memory over-allocation on compressed images |
Date: |
Fri, 29 Jun 2018 17:47:11 +0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
Am 29.06.2018 um 17:16 hat Eric Blake geschrieben:
> On 06/29/2018 04:03 AM, Kevin Wolf wrote:
> > Am 28.06.2018 um 21:07 hat Eric Blake geschrieben:
> > > When reading a compressed image, we were allocating s->cluster_data
> > > to 32*cluster_size + 512 (possibly over 64 megabytes, for an image
> > > with 2M clusters). Let's check out the history:
> > >
>
> > > However, the qcow2 spec permits an all-ones sector count, plus
> > > a full sector containing the initial offset, for a maximum read
> > > of 2 full clusters. Thanks to the way decompression works, even
> > > though such a value is too large for the actual compressed data
> > > (for all but 512-byte clusters), it really doesn't matter if we
> > > read too much (gzip ignores slop, once it has decoded a full
> > > cluster). So it's easier to just allocate cluster_data to be as
> > > large as we can ever possibly see; even if it still wastes up to
> > > 2M on any image created by qcow2, that's still an improvment of
>
> s/improvment/improvement/
>
> > > 60M less waste than pre-patch.
> > >
> > > Signed-off-by: Eric Blake <address@hidden>
> > > Reviewed-by: Alberto Garcia <address@hidden>
> > >
> > > ---
>
> > > +++ b/block/qcow2-cluster.c
> > > @@ -1599,20 +1599,29 @@ int qcow2_decompress_cluster(BlockDriverState
> > > *bs, uint64_t cluster_offset)
> > > sector_offset = coffset & 511;
> > > csize = nb_csectors * 512 - sector_offset;
> > >
> > > - /* Allocate buffers on first decompress operation, most images
> > > are
> > > - * uncompressed and the memory overhead can be avoided. The
> > > buffers
> > > - * are freed in .bdrv_close().
> > > + /* Allocate buffers on the first decompress operation; most
> > > + * images are uncompressed and the memory overhead can be
> > > + * avoided. The buffers are freed in .bdrv_close(). qemu
> > > + * never writes an inflated cluster, and gzip itself never
> > > + * inflates a problematic cluster by more than 0.015%, but the
> > > + * qcow2 format allows up to 1 byte short of 2 full clusters
> > > + * when including the sector containing offset. gzip ignores
> > > + * trailing slop, so just allocate that much up front rather
> > > + * than reject third-party images with overlarge csize.
> > > */
> > > + assert(!!s->cluster_data == !!s->cluster_cache);
> > > + assert(csize <= 2 * s->cluster_size);
> > > if (!s->cluster_data) {
> > > - /* one more sector for decompressed data alignment */
> > > - s->cluster_data = qemu_try_blockalign(bs->file->bs,
> > > - QCOW_MAX_CRYPT_CLUSTERS * s->cluster_size + 512);
> > > + s->cluster_data = g_try_malloc(2 * s->cluster_size);
> > > if (!s->cluster_data) {
> > > return -ENOMEM;
> > > }
> > > - }
> > > - if (!s->cluster_cache) {
> > > - s->cluster_cache = g_malloc(s->cluster_size);
> > > + s->cluster_cache = g_try_malloc(s->cluster_size);
> > > + if (!s->cluster_cache) {
> > > + g_free(s->cluster_data);
> > > + s->cluster_data = NULL;
> > > + return -ENOMEM;
> > > + }
> > > }
> >
> > I wonder how much of a difference s->cluster_cache really makes. I
> > wouldn't expect guests to access the same cluster twice in a row.
>
> I don't know if it makes a difference. But my patch is not even touching
> that - ALL I'm doing is changing a 64M allocation into a 4M allocation, with
> otherwise no change to the frequency of allocation (which is once per
> image).
>
> >
> > Maybe the easiest solution would be switching to temporary buffers that
> > would have the exact size we need and would be freed at the end of the
> > request?
>
> The exact size for a qemu-produced image would be at most 2M instead of 4M -
> but doing the change you propose WILL cause more frequent allocations (once
> per cluster, rather than once per image). We'd have to benchmark if it
> makes sense.
I wouldn't expect that another allocation makes a big difference when
you already have to decompress the whole cluster. In fact, it could
speed things up because we could then parallelise this.
Hmm... Wasn't there a series for using a worker thread for decompression
recently? It might actually already make that change, I don't remember.
> But that would be a separate patch from this one.
Yes, just a thought I had while reviewing your patch.
Kevin
- Re: [Qemu-devel] [PATCH v7 3/6] qcow2: Reduce REFT_OFFSET_MASK, (continued)
[Qemu-devel] [PATCH v7 4/6] qcow2: Don't allow overflow during cluster allocation, Eric Blake, 2018/06/28
[Qemu-devel] [PATCH v7 2/6] qcow2: Document some maximum size constraints, Eric Blake, 2018/06/28
[Qemu-devel] [PATCH v7 5/6] iotests: Add new test 220 for max compressed cluster offset, Eric Blake, 2018/06/28
[Qemu-devel] [PATCH v7 6/6] qcow2: Avoid memory over-allocation on compressed images, Eric Blake, 2018/06/28