From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:39783) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJjS4-00067S-1G for qemu-devel@nongnu.org; Tue, 19 Aug 2014 09:27:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XJjRx-00010K-My for qemu-devel@nongnu.org; Tue, 19 Aug 2014 09:27:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:4464) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XJjRx-00010G-Fq for qemu-devel@nongnu.org; Tue, 19 Aug 2014 09:27:45 -0400 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id s7JDRiKd001752 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK) for ; Tue, 19 Aug 2014 09:27:45 -0400 Date: Tue, 19 Aug 2014 15:27:41 +0200 From: Kevin Wolf Message-ID: <20140819132741.GF4638@noname.redhat.com> References: <1408392454-22044-1-git-send-email-mreitz@redhat.com> <1408392454-22044-2-git-send-email-mreitz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1408392454-22044-2-git-send-email-mreitz@redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 1/4] qcow2: Constant cache size in bytes List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, Stefan Hajnoczi Am 18.08.2014 um 22:07 hat Max Reitz geschrieben: > Specifying the metadata cache sizes in clusters results in less clusters > (and much less bytes) covered for small cluster sizes and vice versa. > Using a constant byte size reduces this difference, and makes it > possible to manually specify the cache size in an easily comprehensible > unit. > > Signed-off-by: Max Reitz > --- > block/qcow2.c | 16 ++++++++++++++-- > block/qcow2.h | 10 ++++++++-- > 2 files changed, 22 insertions(+), 4 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 435e0e1..910d9cf 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -470,6 +470,7 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, > uint64_t l1_vm_state_index; > const char *opt_overlap_check; > int overlap_check_template = 0; > + uint64_t l2_cache_size, refcount_cache_size; > > ret = bdrv_pread(bs->file, 0, &header, sizeof(header)); > if (ret < 0) { > @@ -707,8 +708,19 @@ static int qcow2_open(BlockDriverState *bs, QDict *options, int flags, > } > > /* alloc L2 table/refcount block cache */ > - s->l2_table_cache = qcow2_cache_create(bs, L2_CACHE_SIZE); > - s->refcount_block_cache = qcow2_cache_create(bs, REFCOUNT_CACHE_SIZE); > + l2_cache_size = DEFAULT_L2_CACHE_BYTE_SIZE / s->cluster_size; > + if (l2_cache_size < MIN_L2_CACHE_SIZE) { > + l2_cache_size = MIN_L2_CACHE_SIZE; > + } > + > + refcount_cache_size = l2_cache_size > + / (DEFAULT_L2_REFCOUNT_SIZE_RATIO * s->cluster_size); The factor s->cluster_size is too much, l2_cache_size is already in clusters. I think one of the other patches in the series fixes this again, at least I can't see the bug in the final applied version. If you agree, I'll change this patch while applying to make the series bisectable. Kevin > + if (refcount_cache_size < MIN_REFCOUNT_CACHE_SIZE) { > + refcount_cache_size = MIN_REFCOUNT_CACHE_SIZE; > + } > + > + s->l2_table_cache = qcow2_cache_create(bs, l2_cache_size); > + s->refcount_block_cache = qcow2_cache_create(bs, refcount_cache_size); > if (s->l2_table_cache == NULL || s->refcount_block_cache == NULL) { > error_setg(errp, "Could not allocate metadata caches"); > ret = -ENOMEM; > diff --git a/block/qcow2.h b/block/qcow2.h > index b49424b..671783d 100644 > --- a/block/qcow2.h > +++ b/block/qcow2.h > @@ -64,10 +64,16 @@ > #define MIN_CLUSTER_BITS 9 > #define MAX_CLUSTER_BITS 21 > > -#define L2_CACHE_SIZE 16 > +#define MIN_L2_CACHE_SIZE 1 /* cluster */ > > /* Must be at least 4 to cover all cases of refcount table growth */ > -#define REFCOUNT_CACHE_SIZE 4 > +#define MIN_REFCOUNT_CACHE_SIZE 4 /* clusters */ > + > +#define DEFAULT_L2_CACHE_BYTE_SIZE 1048576 /* bytes */ > + > +/* The refblock cache needs only a fourth of the L2 cache size to cover as many > + * clusters */ > +#define DEFAULT_L2_REFCOUNT_SIZE_RATIO 4 > > #define DEFAULT_CLUSTER_SIZE 65536 > > -- > 2.0.4 >