On 02/06/2015 07:39 AM, Max Reitz wrote: > qcow2_alloc_bytes() is a function with insufficient error handling and > an unnecessary goto. This patch rewrites it. > > Signed-off-by: Max Reitz <mre...@redhat.com> > --- > v3: > - Use alloc_clusters_noref() and update_refcount() [Kevin]
Ouch. Not done quite right. Kevin, you may want to remove this from
your staging area, while Max works on a v4.
> +
> + free_in_cluster = s->cluster_size - offset_into_cluster(s, offset);
> + if (!offset || free_in_cluster < size) {
Let's consider all four possibilities:
Case 1: !offset
we won't be modifying any existing clusters. When we are done, the new
cluster needs a refcount of 1
Case 2: free_in_cluster >= size
we will only be modifying an existing cluster, assume it starts with
refcount of 1. When we are done, it needs a refcount of 2
Case 3: free_in_cluster < size, allocation is not contiguous
we won't be modifying any existing clusters. When we are done, the new
cluster needs a refcount of 1
Case 4: free_in_cluster < size, allocation is contiguous
we will be placing data into both an existing cluster and a new one;
assume the existing cluster starts with a refcount of 1. When we are
done, the old cluster needs a refcount of 2, and the new cluster needs a
refcount of 1
> + int64_t new_cluster = alloc_clusters_noref(bs, s->cluster_size);
This says the new cluster has a refcount of 0.
> + if (new_cluster < 0) {
> + return new_cluster;
> + }
> +
> + if (!offset || ROUND_UP(offset, s->cluster_size) != new_cluster) {
> + offset = new_cluster;
> }
> }
>
> - /* The cluster refcount was incremented, either by qcow2_alloc_clusters()
> - * or explicitly by qcow2_update_cluster_refcount(). Refcount blocks
> must
> - * be flushed before the caller's L2 table updates.
> - */
> + assert(offset);
> + ret = update_refcount(bs, offset, size, 1, QCOW2_DISCARD_NEVER);
Case 1: This incremented the new cluster. Good
Case 2: This incremented the old cluster. Good
Case 3: This incremented the new cluster. Good
Case 4: This incremented the old cluster. But the new cluster remains at
refcount 0. BAD.
v2 got it right, because it always put the NEW cluster at refcount 1 (if
there was a new cluster), then incremented the old cluster when needed.
> + if (ret < 0) {
> + return ret;
> + }
> +
> + /* The cluster refcount was incremented; refcount blocks must be flushed
> + * before the caller's L2 table updates. */
> qcow2_cache_set_dependency(bs, s->l2_table_cache,
> s->refcount_block_cache);
> +
> + s->free_byte_offset = offset + size;
> + if (!offset_into_cluster(s, s->free_byte_offset)) {
> + s->free_byte_offset = 0;
> + }
> +
> return offset;
> }
>
>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
