Re: [Qemu-devel] [PATCH v2 1/4] qcow2: Constant cache size in bytes

[Kevin Wolf]

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 <address@hidden>
> ---
>  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
>