[Qemu-devel] Re: [PATCH 2/3] qcow2: Rewrite alloc_refcount_block/grow_refcount_table

[Juan Quintela <quintela@redhat.com>]

Kevin Wolf <kwolf@redhat.com> wrote:

> +    /* Find the refcount block for the given cluster */
> +    refcount_table_index = cluster_index >> (s->cluster_bits - REFCOUNT_SHIFT);
> +    if (refcount_table_index >= s->refcount_table_size) {
> +        refcount_block_offset = 0;
> +    } else {
> +        refcount_block_offset = s->refcount_table[refcount_table_index];
> +    }
> +
> +    /* If it's already there, we're done */
> +    if (refcount_block_offset) {
> +        if (refcount_block_offset != s->refcount_block_cache_offset) {
> +            ret = load_refcount_block(bs, refcount_block_offset);
> +            if (ret < 0) {
> +                return ret;
> +            }
> +        }
> +        return refcount_block_offset;
> +    }

I would merge  this if in the previous one.  as a bonus,
refcount_block_offset can be local to that if branch and no need of the
else one.

> +
> +    /*
> +     * If we came here, we need to allocate something. Something is at least
> +     * a cluster for the new refcount block. It may also include a new refcount
> +     * table if the old refcount table is too small.
> +     *
> +     * Note that allocating clusters here needs some special care:
> +     *
> +     * - We can't use the normal qcow2_alloc_clusters(), it would try to
> +     *   increase the refcount and very likely we would end up with an endless
> +     *   recursion. Instead we must place the refcount blocks in a way that
> +     *   they can describe them themselves.
> +     *
> +     * - We need to consider that at this point we are inside update_refcounts
> +     *   and doing the initial refcount increase. This means that some clusters
> +     *   have already been allocated by the caller, but their refcount isn't
> +     *   accurate yet. free_cluster_index tells us where this allocation ends
> +     *   as long as we don't overwrite it by freeing clusters.
> +     *
> +     * - alloc_clusters_noref and qcow2_free_clusters may load a different
> +     *   refcount block into the cache
> +     */
> +
> +    if (cache_refcount_updates) {
> +        write_refcount_block(s);

write_refconut_blocks() can return -EIO.  It is not handled anywhere
else either.


> +    /* Calculate the number of refcount blocks needed so far */
> +    uint64_t refcount_block_clusters = 1 << (s->cluster_bits - REFCOUNT_SHIFT);
> +    uint64_t blocks_used = (s->free_cluster_index +
> +        refcount_block_clusters - 1) / refcount_block_clusters;
> +
> +    /* And now we need at least one block more for the new metadata */
> +    uint64_t table_size = next_refcount_table_size(s, blocks_used + 1);
> +    uint64_t last_table_size = table_size;

only place where last_table_size is assigned to.

> +    uint64_t blocks_clusters;
> +    do {
> +        uint64_t table_clusters = size_to_clusters(s, table_size);
> +        blocks_clusters = 1 +
> +            ((table_clusters + refcount_block_clusters - 1)
> +            / refcount_block_clusters);
> +        uint64_t meta_clusters = table_clusters + blocks_clusters;
> +
> +        table_size = next_refcount_table_size(s, blocks_used +
> +            ((meta_clusters + refcount_block_clusters - 1)
> +            / refcount_block_clusters));

this value can be the same than previous next_refcount_table_size()
call or bigger.

> +
> +    } while (last_table_size != table_size);

how can this converge? (already discussed on irc with keving that a
last_table_size = table_size is missing somewhere in the loop.

Later, Juan.