[Qemu-devel] Re: [PATCH 1/3] qcow2: Factor next_refcount_table_size out

[Juan Quintela]

Kevin Wolf <address@hidden> wrote:
> When the refcount table grows, it doesn't only grow by one entry but reserves
> some space for future refcount blocks. The algorithm to calculate the number 
> of
> entries stays the same with the fixes, so factor it out before replacing the
> rest.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  block/qcow2-refcount.c |   34 +++++++++++++++++++++++-----------
>  1 files changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 2fdc26b..0e2ecd7 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -123,6 +123,28 @@ static int get_refcount(BlockDriverState *bs, int64_t 
> cluster_index)
>      return be16_to_cpu(s->refcount_block_cache[block_index]);
>  }
>  
> +/*
> + * Rounds the refcount table size up to avoid growing the table for each 
> single
> + * refcount block that is allocated.
> + */
> +static unsigned int next_refcount_table_size(BDRVQcowState *s,
> +    unsigned int min_size)
> +{
> +    unsigned int refcount_table_clusters = 0;
> +    unsigned int new_table_size = 1;
> +
> +    while (min_size > new_table_size) {
> +        if (refcount_table_clusters == 0) {
> +            refcount_table_clusters = 1;
> +        } else {
> +            refcount_table_clusters = (refcount_table_clusters * 3 + 1) / 2;
> +        }
> +        new_table_size = refcount_table_clusters << (s->cluster_bits - 3);
> +    }
> +
> +    return new_table_size;
> +}
> +


couldn't something like:

static unsigned int next_refcount_table_size(BDRVQcowState *s,
                                             unsigned int min_size)
{
    unsigned int refcount_table_clusters = 1;
    unsigned int min_clusters = (min_size >> (s->clusters_bits -3)) + 1;

    while (min_clusters > refcount_table_clusters) {
            refcount_table_clusters = (refcount_table_clusters * 3 + 1) / 2;
    }

    return refconut_table_clusters << (s->cluster_bits - 3);
}

That removes the if from the inside loop, and makes the comparison in
clusters not in size.  What do you think?

>  static int grow_refcount_table(BlockDriverState *bs, int min_size)
>  {
>      BDRVQcowState *s = bs->opaque;
> @@ -136,17 +158,7 @@ static int grow_refcount_table(BlockDriverState *bs, int 
> min_size)
>      if (min_size <= s->refcount_table_size)
>          return 0;
>      /* compute new table size */
> -    refcount_table_clusters = s->refcount_table_size >> (s->cluster_bits - 
> 3);
> -    for(;;) {
> -        if (refcount_table_clusters == 0) {
> -            refcount_table_clusters = 1;
> -        } else {
> -            refcount_table_clusters = (refcount_table_clusters * 3 + 1) / 2;
> -        }
> -        new_table_size = refcount_table_clusters << (s->cluster_bits - 3);
> -        if (min_size <= new_table_size)
> -            break;
> -    }
> +    new_table_size = next_refcount_table_size(s, min_size);
>  #ifdef DEBUG_ALLOC2
>      printf("grow_refcount_table from %d to %d\n",
>             s->refcount_table_size,