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

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

[Max Reitz]

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]
- Only modify s->free_byte_offset if the function is successful; this is
  now necessary because update_refcount() is called unconditionally and
  thus, if it failed and alloc_clusters_noref() had been called and
  had returned a non-contiguous offset, s->free_byte_offset would point
  to an unallocated cluster's head, which is both wrong in itself and
  would also violate the assertion at the beginning of the function
---
 block/qcow2-refcount.c | 78 +++++++++++++++++++++++++-------------------------
 1 file changed, 39 insertions(+), 39 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 9afdb40..9b80ca7 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -759,54 +759,54 @@ int qcow2_alloc_clusters_at(BlockDriverState *bs, uint64_t offset,
 int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
 {
     BDRVQcowState *s = bs->opaque;
-    int64_t offset, cluster_offset;
-    int free_in_cluster;
+    int64_t offset;
+    size_t free_in_cluster;
+    int ret;
 
     BLKDBG_EVENT(bs->file, BLKDBG_CLUSTER_ALLOC_BYTES);
     assert(size > 0 && size <= s->cluster_size);
-    if (s->free_byte_offset == 0) {
-        offset = qcow2_alloc_clusters(bs, s->cluster_size);
-        if (offset < 0) {
-            return offset;
+    assert(!s->free_byte_offset || offset_into_cluster(s, s->free_byte_offset));
+
+    offset = s->free_byte_offset;
+
+    if (offset) {
+        int refcount = qcow2_get_refcount(bs, offset >> s->cluster_bits);
+        if (refcount < 0) {
+            return refcount;
         }
-        s->free_byte_offset = offset;
-    }
- redo:
-    free_in_cluster = s->cluster_size -
-        offset_into_cluster(s, s->free_byte_offset);
-    if (size <= free_in_cluster) {
-        /* enough space in current cluster */
-        offset = s->free_byte_offset;
-        s->free_byte_offset += size;
-        free_in_cluster -= size;
-        if (free_in_cluster == 0)
-            s->free_byte_offset = 0;
-        if (offset_into_cluster(s, offset) != 0)
-            qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, 1,
-                                          QCOW2_DISCARD_NEVER);
-    } else {
-        offset = qcow2_alloc_clusters(bs, s->cluster_size);
-        if (offset < 0) {
-            return offset;
+
+        if (refcount == 0xffff) {
+            offset = 0;
         }
-        cluster_offset = start_of_cluster(s, s->free_byte_offset);
-        if ((cluster_offset + s->cluster_size) == offset) {
-            /* we are lucky: contiguous data */
-            offset = s->free_byte_offset;
-            qcow2_update_cluster_refcount(bs, offset >> s->cluster_bits, 1,
-                                          QCOW2_DISCARD_NEVER);
-            s->free_byte_offset += size;
-        } else {
-            s->free_byte_offset = offset;
-            goto redo;
+    }
+
+    free_in_cluster = s->cluster_size - offset_into_cluster(s, offset);
+    if (!offset || free_in_cluster < size) {
+        int64_t new_cluster = alloc_clusters_noref(bs, s->cluster_size);
+        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);
+    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;
 }
 
-- 
2.1.0

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

[Kevin Wolf]

Am 06.02.2015 um 15:39 hat Max Reitz geschrieben:
> 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]
> - Only modify s->free_byte_offset if the function is successful; this is
>   now necessary because update_refcount() is called unconditionally and
>   thus, if it failed and alloc_clusters_noref() had been called and
>   had returned a non-contiguous offset, s->free_byte_offset would point
>   to an unallocated cluster's head, which is both wrong in itself and
>   would also violate the assertion at the beginning of the function

Looks much nicer, I think. :-)

Thanks, applied to the block branch.

Kevin

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

[Eric Blake]

[-- 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 --]

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

[Eric Blake]

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

On 02/06/2015 08:27 AM, Eric Blake wrote:

>>  
>> -    /* 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.

Wait. Maybe I'm confused.  You are requesting an update_refcount()
across size bytes, and given the offset, that means that the code will
round up to cover BOTH clusters in one call.  Does update_refcount()
properly increment from  [ 1, 0 ] to [ 2, 1 ] when given a 2-cluster
size (when offset, size is rounded up to cluster boundaries)?  If so,
then there is no bug after all.

-- 
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 --]

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

[Kevin Wolf]

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

Am 06.02.2015 um 16:31 hat Eric Blake geschrieben:
> On 02/06/2015 08:27 AM, Eric Blake wrote:
> 
> >>  
> >> -    /* 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.
> 
> Wait. Maybe I'm confused.  You are requesting an update_refcount()
> across size bytes, and given the offset, that means that the code will
> round up to cover BOTH clusters in one call.  Does update_refcount()
> properly increment from  [ 1, 0 ] to [ 2, 1 ] when given a 2-cluster
> size (when offset, size is rounded up to cluster boundaries)?  If so,
> then there is no bug after all.

Yes, this is what it should be doing. Letting one update_refcount() call
(more or less) atomically increment both refcounts was the whole point
of my suggestion anyway because that is what enables the simplification.

Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]