Kevin Wolf <kw...@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.
