| Message ID | 20170821135530.32344-1-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/21/2017 08:55 AM, Stefan Hajnoczi wrote: > Most qcow2 files are uncompressed so it is wasteful to allocate (32 + 1) > * cluster_size + 512 bytes upfront. Allocate s->cluster_cache and > s->cluster_data when the first read operation is performance on a > compressed cluster. > > The buffers are freed in .bdrv_close(). .bdrv_open() no longer has any > code paths that can allocate these buffers, so remove the free functions > in the error code path. > > This patch can result in significant memory savings when many qcow2 > disks are attached or backing file chains are long: > > Before 12.81% (1,023,193,088B) > After 5.36% (393,893,888B) > > Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru> > Tested-by: Alexey Kardashevskiy <aik@ozlabs.ru> > Cc: Kevin Wolf <kwolf@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- Reviewed-by: Eric Blake <eblake@redhat.com>
On Mon, Aug 21, 2017 at 02:55:30PM +0100, Stefan Hajnoczi wrote: > Most qcow2 files are uncompressed so it is wasteful to allocate (32 + 1) > * cluster_size + 512 bytes upfront. Allocate s->cluster_cache and > s->cluster_data when the first read operation is performance on a > compressed cluster. > > The buffers are freed in .bdrv_close(). .bdrv_open() no longer has any > code paths that can allocate these buffers, so remove the free functions > in the error code path. > > This patch can result in significant memory savings when many qcow2 > disks are attached or backing file chains are long: > > Before 12.81% (1,023,193,088B) > After 5.36% (393,893,888B) > > Reported-by: Alexey Kardashevskiy <aik@ozlabs.ru> > Tested-by: Alexey Kardashevskiy <aik@ozlabs.ru> > Cc: Kevin Wolf <kwolf@redhat.com> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com> > --- > v2: > * Changed EIO to ENOMEM [Eric] > * Added Alexey's Tested-by > --- > block/qcow2-cluster.c | 17 +++++++++++++++++ > block/qcow2.c | 12 ------------ > 2 files changed, 17 insertions(+), 12 deletions(-) Thanks, applied to my block-next tree: https://github.com/stefanha/qemu/commits/block-next Stefan
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c index f06c08f64c..8538533102 100644 --- a/block/qcow2-cluster.c +++ b/block/qcow2-cluster.c @@ -1516,6 +1516,23 @@ int qcow2_decompress_cluster(BlockDriverState *bs, uint64_t cluster_offset) nb_csectors = ((cluster_offset >> s->csize_shift) & s->csize_mask) + 1; 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(). + */ + 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); + if (!s->cluster_data) { + return -ENOMEM; + } + } + if (!s->cluster_cache) { + s->cluster_cache = g_malloc(s->cluster_size); + } + BLKDBG_EVENT(bs->file, BLKDBG_READ_COMPRESSED); ret = bdrv_read(bs->file, coffset >> 9, s->cluster_data, nb_csectors); diff --git a/block/qcow2.c b/block/qcow2.c index 40ba26c111..0ac201910a 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1360,16 +1360,6 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } - s->cluster_cache = g_malloc(s->cluster_size); - /* one more sector for decompressed data alignment */ - s->cluster_data = qemu_try_blockalign(bs->file->bs, QCOW_MAX_CRYPT_CLUSTERS - * s->cluster_size + 512); - if (s->cluster_data == NULL) { - error_setg(errp, "Could not allocate temporary cluster buffer"); - ret = -ENOMEM; - goto fail; - } - s->cluster_cache_offset = -1; s->flags = flags; @@ -1507,8 +1497,6 @@ static int qcow2_do_open(BlockDriverState *bs, QDict *options, int flags, if (s->refcount_block_cache) { qcow2_cache_destroy(bs, s->refcount_block_cache); } - g_free(s->cluster_cache); - qemu_vfree(s->cluster_data); qcrypto_block_free(s->crypto); qapi_free_QCryptoBlockOpenOptions(s->crypto_opts); return ret;