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

[Qemu-devel] [PATCH v2] 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>
---
v2:
- s/free_cluster_index/free_byte_index/ [Eric]
- added an assertion at the start of the function that
  s->free_byte_offset is either 0 or points to the tail of a cluster
  (but never to the start)
- use ROUND_UP() instead of start_of_cluster() + cluster_size [Eric]
- added an assertion that s->free_byte_offset is set before using it
  [Eric]
---
 block/qcow2-refcount.c | 77 +++++++++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 32 deletions(-)

diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
index 9afdb40..eede60d 100644
--- a/block/qcow2-refcount.c
+++ b/block/qcow2-refcount.c
@@ -759,46 +759,51 @@ 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, new_cluster = 0, cluster_end;
+    size_t free_in_cluster;
 
     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));
+
+    if (s->free_byte_offset) {
+        int refcount = qcow2_get_refcount(bs,
+            s->free_byte_offset >> s->cluster_bits);
+        if (refcount < 0) {
+            return refcount;
+        }
+
+        if (refcount == 0xffff) {
+            s->free_byte_offset = 0;
         }
-        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 (!s->free_byte_offset || free_in_cluster < size) {
+        new_cluster = qcow2_alloc_clusters(bs, s->cluster_size);
+        if (new_cluster < 0) {
+            return new_cluster;
+        }
+
+        cluster_end = ROUND_UP(s->free_byte_offset, s->cluster_size);
+        if (!s->free_byte_offset || cluster_end != new_cluster) {
+            s->free_byte_offset = new_cluster;
         }
-        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;
+    }
+
+    assert(s->free_byte_offset);
+    if (offset_into_cluster(s, s->free_byte_offset)) {
+        int ret = qcow2_update_cluster_refcount(bs,
+                s->free_byte_offset >> s->cluster_bits, 1,
+                QCOW2_DISCARD_NEVER);
+        if (ret < 0) {
+            if (new_cluster > 0) {
+                qcow2_free_clusters(bs, new_cluster, s->cluster_size,
+                                    QCOW2_DISCARD_OTHER);
+            }
+            return ret;
         }
     }
 
@@ -807,6 +812,14 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
      * be flushed before the caller's L2 table updates.
      */
     qcow2_cache_set_dependency(bs, s->l2_table_cache, s->refcount_block_cache);
+
+    offset = s->free_byte_offset;
+
+    s->free_byte_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 v2] qcow2: Rewrite qcow2_alloc_bytes()

[Eric Blake]

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

On 02/05/2015 08:58 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>
> ---
> v2:
> - s/free_cluster_index/free_byte_index/ [Eric]
> - added an assertion at the start of the function that
>   s->free_byte_offset is either 0 or points to the tail of a cluster
>   (but never to the start)
> - use ROUND_UP() instead of start_of_cluster() + cluster_size [Eric]
> - added an assertion that s->free_byte_offset is set before using it
>   [Eric]

Looks nicer.

> +        cluster_end = ROUND_UP(s->free_byte_offset, s->cluster_size);
> +        if (!s->free_byte_offset || cluster_end != new_cluster) {

Here, I can prove that the left side of the || makes no difference to
the outcome (using just the right half is sufficient, since
!s->free_byte_offset implies cluster_end == 0, but new_cluster will be
non-zero because we never allocate the header cluster). I'd be fine if
you wanted to respin that and add the comment for the micro-optimized
shorter code; but for maintenance purposes, it's easier to rely on the
explicit condition (even if redundant) than to think about whether a
proof listed in a comment is correct, so I'm also fine leaving things as is.

> +            s->free_byte_offset = new_cluster;

At this point, s->free_byte_offset violates the precondition if we just
allocated a cluster, so we have to make sure that we restore the
precondition before exiting...

> +    if (offset_into_cluster(s, s->free_byte_offset)) {
> +        int ret = qcow2_update_cluster_refcount(bs,
> +                s->free_byte_offset >> s->cluster_bits, 1,
> +                QCOW2_DISCARD_NEVER);
> +        if (ret < 0) {
> +            if (new_cluster > 0) {
> +                qcow2_free_clusters(bs, new_cluster, s->cluster_size,
> +                                    QCOW2_DISCARD_OTHER);
> +            }
> +            return ret;

...this early exit only happens if s->free_byte_offset was pointing to a
tail, and there are no other early exits, with the final exit properly
restoring the precondition.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
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 v2] qcow2: Rewrite qcow2_alloc_bytes()

[Kevin Wolf]

Am 05.02.2015 um 16:58 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>
> ---
> v2:
> - s/free_cluster_index/free_byte_index/ [Eric]
> - added an assertion at the start of the function that
>   s->free_byte_offset is either 0 or points to the tail of a cluster
>   (but never to the start)
> - use ROUND_UP() instead of start_of_cluster() + cluster_size [Eric]
> - added an assertion that s->free_byte_offset is set before using it
>   [Eric]
> ---
>  block/qcow2-refcount.c | 77 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 45 insertions(+), 32 deletions(-)
> 
> diff --git a/block/qcow2-refcount.c b/block/qcow2-refcount.c
> index 9afdb40..eede60d 100644
> --- a/block/qcow2-refcount.c
> +++ b/block/qcow2-refcount.c
> @@ -759,46 +759,51 @@ 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, new_cluster = 0, cluster_end;
> +    size_t free_in_cluster;
>  
>      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));
> +
> +    if (s->free_byte_offset) {
> +        int refcount = qcow2_get_refcount(bs,
> +            s->free_byte_offset >> s->cluster_bits);
> +        if (refcount < 0) {
> +            return refcount;
> +        }
> +
> +        if (refcount == 0xffff) {
> +            s->free_byte_offset = 0;
>          }
> -        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 (!s->free_byte_offset || free_in_cluster < size) {
> +        new_cluster = qcow2_alloc_clusters(bs, s->cluster_size);

The code could perhaps become a bit nicer if you used
alloc_clusters_noref() here...

> +        if (new_cluster < 0) {
> +            return new_cluster;
> +        }
> +
> +        cluster_end = ROUND_UP(s->free_byte_offset, s->cluster_size);
> +        if (!s->free_byte_offset || cluster_end != new_cluster) {
> +            s->free_byte_offset = new_cluster;
>          }
> -        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;
> +    }
> +
> +    assert(s->free_byte_offset);
> +    if (offset_into_cluster(s, s->free_byte_offset)) {

...because this block could become unconditional then, ...

> +        int ret = qcow2_update_cluster_refcount(bs,
> +                s->free_byte_offset >> s->cluster_bits, 1,
> +                QCOW2_DISCARD_NEVER);

...here you could use update_refcount() with the actual byte count
(which also avoids two unnecessary shifts)...

> +        if (ret < 0) {
> +            if (new_cluster > 0) {
> +                qcow2_free_clusters(bs, new_cluster, s->cluster_size,
> +                                    QCOW2_DISCARD_OTHER);
> +            }

...and this part wouldn't be needed because update_refcount() already
tries to fail atomically.

> +            return ret;
>          }
>      }
>  
> @@ -807,6 +812,14 @@ int64_t qcow2_alloc_bytes(BlockDriverState *bs, int size)
>       * be flushed before the caller's L2 table updates.
>       */

It would also simplify the two lines of this comment that aren't in the
patch context any more. ;-)

>      qcow2_cache_set_dependency(bs, s->l2_table_cache, s->refcount_block_cache);
> +
> +    offset = s->free_byte_offset;
> +
> +    s->free_byte_offset += size;
> +    if (!offset_into_cluster(s, s->free_byte_offset)) {
> +        s->free_byte_offset = 0;
> +    }
> +
>      return offset;
>  }

The patch looks correct to me. Let me know if you'd like to address the
point I made above, or if I should apply it as it is.

Kevin

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

[Max Reitz]

On 2015-02-06 at 09:08, Kevin Wolf wrote:
> Am 05.02.2015 um 16:58 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>
>> ---
>> v2:
>> - s/free_cluster_index/free_byte_index/ [Eric]
>> - added an assertion at the start of the function that
>>    s->free_byte_offset is either 0 or points to the tail of a cluster
>>    (but never to the start)
>> - use ROUND_UP() instead of start_of_cluster() + cluster_size [Eric]
>> - added an assertion that s->free_byte_offset is set before using it
>>    [Eric]
>> ---
>>   block/qcow2-refcount.c | 77 +++++++++++++++++++++++++++++---------------------
>>   1 file changed, 45 insertions(+), 32 deletions(-)
>>

[snip]

> The patch looks correct to me. Let me know if you'd like to address the
> point I made above, or if I should apply it as it is.

Good question. On one hand, I like your suggestion because it would 
indeed make the code shorter. On the other hand, I somehow feel better 
using functions that are prefixed qcow2_* because I feel like it might 
make the code harder to read if sometimes we use qcow2_alloc_clusters() 
and alloc_clusters_noref() at other times; and sometimes we use 
qcow2_update_cluster_refcount() and update_refcount() at other times.

But then again, it might make sense to have this function mirror 
qcow2_alloc_clusters(), which does use alloc_clusters_noref() and 
update_refcount() (of course).

So I think I'll go with your suggestion, thanks!

Max