From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Ni55y-00035w-1e for qemu-devel@nongnu.org; Thu, 18 Feb 2010 07:03:02 -0500 Received: from [199.232.76.173] (port=56311 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ni55v-000358-SQ for qemu-devel@nongnu.org; Thu, 18 Feb 2010 07:02:59 -0500 Received: from Debian-exim by monty-python.gnu.org with spam-scanned (Exim 4.60) (envelope-from ) id 1Ni55u-0001mL-3d for qemu-devel@nongnu.org; Thu, 18 Feb 2010 07:02:59 -0500 Received: from mx1.redhat.com ([209.132.183.28]:21479) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Ni55t-0001mF-Me for qemu-devel@nongnu.org; Thu, 18 Feb 2010 07:02:57 -0500 From: Juan Quintela In-Reply-To: <1266250769-5816-3-git-send-email-kwolf@redhat.com> (Kevin Wolf's message of "Mon, 15 Feb 2010 17:19:28 +0100") References: <1266250769-5816-1-git-send-email-kwolf@redhat.com> <1266250769-5816-3-git-send-email-kwolf@redhat.com> Date: Thu, 18 Feb 2010 13:02:18 +0100 Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Subject: [Qemu-devel] Re: [PATCH 2/3] qcow2: Rewrite alloc_refcount_block/grow_refcount_table List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Kevin Wolf Cc: hch@lst.de, qemu-devel@nongnu.org, gleb@redhat.com, armbru@redhat.com Kevin Wolf 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.