Re: [Qemu-devel] [PATCH v3] qcow2: Rewrite qcow2_alloc_bytes()

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 3017 bytes --]

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 <mreitz@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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]