diff mbox series

[RFC,v2,01/26] qcow2: Add calculate_l2_meta()

Message ID 22820af5867124e4fdf3d0cb74b99f31edc1b37f.1572125022.git.berto@igalia.com (mailing list archive)
State New, archived
Headers show
Series Add subcluster allocation to qcow2 | expand

Commit Message

Alberto Garcia Oct. 26, 2019, 9:25 p.m. UTC
handle_alloc() creates a QCowL2Meta structure in order to update the
image metadata and perform the necessary copy-on-write operations.

This patch moves that code to a separate function so it can be used
from other places.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/qcow2-cluster.c | 76 +++++++++++++++++++++++++++++--------------
 1 file changed, 52 insertions(+), 24 deletions(-)

Comments

Vladimir Sementsov-Ogievskiy Oct. 28, 2019, 12:50 p.m. UTC | #1
27.10.2019 0:25, Alberto Garcia wrote:
> handle_alloc() creates a QCowL2Meta structure in order to update the
> image metadata and perform the necessary copy-on-write operations.
> 
> This patch moves that code to a separate function so it can be used
> from other places.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>   block/qcow2-cluster.c | 76 +++++++++++++++++++++++++++++--------------
>   1 file changed, 52 insertions(+), 24 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 8982b7b762..6c1dcdc781 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1019,6 +1019,55 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m)
>                           QCOW2_DISCARD_NEVER);
>   }
>   
> +/*
> + * For a given write request, create a new QCowL2Meta structure and
> + * add it to @m.
> + *
> + * @host_offset points to the beginning of the first cluster.
> + *
> + * @guest_offset and @bytes indicate the offset and length of the
> + * request.
> + *
> + * If @keep_old is true it means that the clusters were already
> + * allocated and will be overwritten. If false then the clusters are
> + * new and we have to decrease the reference count of the old ones.
> + */
> +static void calculate_l2_meta(BlockDriverState *bs, uint64_t host_offset,
> +                              uint64_t guest_offset, uint64_t bytes,
> +                              QCowL2Meta **m, bool keep_old)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    unsigned cow_start_from = 0;
> +    unsigned cow_start_to = offset_into_cluster(s, guest_offset);
> +    unsigned cow_end_from = cow_start_to + bytes;
> +    unsigned cow_end_to = ROUND_UP(cow_end_from, s->cluster_size);

New code is easier to read!

Interesting, seems QEMU_ALIGN_UP is more popular.. But comment above ROUND_UP
makes sense in using it here..

> +    unsigned nb_clusters = size_to_clusters(s, cow_end_from);
> +    QCowL2Meta *old_m = *m;
> +
> +    *m = g_malloc0(sizeof(**m));
> +    **m = (QCowL2Meta) {
> +        .next           = old_m,
> +
> +        .alloc_offset   = host_offset,
> +        .offset         = start_of_cluster(s, guest_offset),
> +        .nb_clusters    = nb_clusters,
> +
> +        .keep_old_clusters = keep_old,
> +
> +        .cow_start = {
> +            .offset     = cow_start_from,
> +            .nb_bytes   = cow_start_to - cow_start_from,
> +        },
> +        .cow_end = {
> +            .offset     = cow_end_from,
> +            .nb_bytes   = cow_end_to - cow_end_from,
> +        },
> +    };
> +
> +    qemu_co_queue_init(&(*m)->dependent_requests);
> +    QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight);
> +}
> +
>   /*
>    * Returns the number of contiguous clusters that can be used for an allocating
>    * write, but require COW to be performed (this includes yet unallocated space,
> @@ -1417,35 +1466,14 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset,
>       uint64_t requested_bytes = *bytes + offset_into_cluster(s, guest_offset);
>       int avail_bytes = nb_clusters << s->cluster_bits;
>       int nb_bytes = MIN(requested_bytes, avail_bytes);
> -    QCowL2Meta *old_m = *m;
> -
> -    *m = g_malloc0(sizeof(**m));
> -
> -    **m = (QCowL2Meta) {
> -        .next           = old_m,
> -
> -        .alloc_offset   = alloc_cluster_offset,
> -        .offset         = start_of_cluster(s, guest_offset),
> -        .nb_clusters    = nb_clusters,
> -
> -        .keep_old_clusters  = keep_old_clusters,
> -
> -        .cow_start = {
> -            .offset     = 0,
> -            .nb_bytes   = offset_into_cluster(s, guest_offset),
> -        },
> -        .cow_end = {
> -            .offset     = nb_bytes,

hmm this logic is changed.. after the patch, it would be not nb_bytes, but

offset_into_cluster(s, guest_offset) + MIN(*bytes, nb_bytes - offset_into_cluster(s, guest_offset))

> -            .nb_bytes   = avail_bytes - nb_bytes,
> -        },
> -    };
> -    qemu_co_queue_init(&(*m)->dependent_requests);
> -    QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight);
>   
>       *host_offset = alloc_cluster_offset + offset_into_cluster(s, guest_offset);
>       *bytes = MIN(*bytes, nb_bytes - offset_into_cluster(s, guest_offset));
>       assert(*bytes != 0);
>   
> +    calculate_l2_meta(bs, alloc_cluster_offset, guest_offset, *bytes,
> +                      m, keep_old_clusters);
> +
>       return 1;
>   
>   fail:
>
Max Reitz Oct. 30, 2019, 12:04 p.m. UTC | #2
On 26.10.19 23:25, Alberto Garcia wrote:
> handle_alloc() creates a QCowL2Meta structure in order to update the
> image metadata and perform the necessary copy-on-write operations.
> 
> This patch moves that code to a separate function so it can be used
> from other places.
> 
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
>  block/qcow2-cluster.c | 76 +++++++++++++++++++++++++++++--------------
>  1 file changed, 52 insertions(+), 24 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 8982b7b762..6c1dcdc781 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1019,6 +1019,55 @@ void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m)
>                          QCOW2_DISCARD_NEVER);
>  }
>  
> +/*
> + * For a given write request, create a new QCowL2Meta structure and
> + * add it to @m.
> + *
> + * @host_offset points to the beginning of the first cluster.

(I intended not to comment on such things on an RFC, but here I am...)

I’d call it host_cluster_offset to make clear that it points to a
cluster and isn’t the host offset for @guest_offset.

And now that I’ve gone this far already, I might as well say that I’d
like if it the comment noted that this function not only creates the
L2Meta structure but also adds it to the cluster_allocs list.

> + * @guest_offset and @bytes indicate the offset and length of the
> + * request.
> + *
> + * If @keep_old is true it means that the clusters were already
> + * allocated and will be overwritten. If false then the clusters are
> + * new and we have to decrease the reference count of the old ones.
> + */
> +static void calculate_l2_meta(BlockDriverState *bs, uint64_t host_offset,
> +                              uint64_t guest_offset, uint64_t bytes,

And now I’m so deep into non-RFC-level review territory, I might as well
say I’d prefer @bytes to be an unsigned (or maybe even a plain int),
because anything more wouldn’t work.  (Not least because cow_end_to is
an unsigned).

Sorry...

Max

> +                              QCowL2Meta **m, bool keep_old)
> +{
> +    BDRVQcow2State *s = bs->opaque;
> +    unsigned cow_start_from = 0;
> +    unsigned cow_start_to = offset_into_cluster(s, guest_offset);
> +    unsigned cow_end_from = cow_start_to + bytes;
> +    unsigned cow_end_to = ROUND_UP(cow_end_from, s->cluster_size);
> +    unsigned nb_clusters = size_to_clusters(s, cow_end_from);
> +    QCowL2Meta *old_m = *m;
> +
> +    *m = g_malloc0(sizeof(**m));
> +    **m = (QCowL2Meta) {
> +        .next           = old_m,
> +
> +        .alloc_offset   = host_offset,
> +        .offset         = start_of_cluster(s, guest_offset),
> +        .nb_clusters    = nb_clusters,
> +
> +        .keep_old_clusters = keep_old,
> +
> +        .cow_start = {
> +            .offset     = cow_start_from,
> +            .nb_bytes   = cow_start_to - cow_start_from,
> +        },
> +        .cow_end = {
> +            .offset     = cow_end_from,
> +            .nb_bytes   = cow_end_to - cow_end_from,
> +        },
> +    };
> +
> +    qemu_co_queue_init(&(*m)->dependent_requests);
> +    QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight);
> +}
> +
>  /*
>   * Returns the number of contiguous clusters that can be used for an allocating
>   * write, but require COW to be performed (this includes yet unallocated space,
> @@ -1417,35 +1466,14 @@ static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset,
>      uint64_t requested_bytes = *bytes + offset_into_cluster(s, guest_offset);
>      int avail_bytes = nb_clusters << s->cluster_bits;
>      int nb_bytes = MIN(requested_bytes, avail_bytes);
> -    QCowL2Meta *old_m = *m;
> -
> -    *m = g_malloc0(sizeof(**m));
> -
> -    **m = (QCowL2Meta) {
> -        .next           = old_m,
> -
> -        .alloc_offset   = alloc_cluster_offset,
> -        .offset         = start_of_cluster(s, guest_offset),
> -        .nb_clusters    = nb_clusters,
> -
> -        .keep_old_clusters  = keep_old_clusters,
> -
> -        .cow_start = {
> -            .offset     = 0,
> -            .nb_bytes   = offset_into_cluster(s, guest_offset),
> -        },
> -        .cow_end = {
> -            .offset     = nb_bytes,
> -            .nb_bytes   = avail_bytes - nb_bytes,
> -        },
> -    };
> -    qemu_co_queue_init(&(*m)->dependent_requests);
> -    QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight);
>  
>      *host_offset = alloc_cluster_offset + offset_into_cluster(s, guest_offset);
>      *bytes = MIN(*bytes, nb_bytes - offset_into_cluster(s, guest_offset));
>      assert(*bytes != 0);
>  
> +    calculate_l2_meta(bs, alloc_cluster_offset, guest_offset, *bytes,
> +                      m, keep_old_clusters);
> +
>      return 1;
>  
>  fail:
>
Alberto Garcia Oct. 30, 2019, 3:56 p.m. UTC | #3
On Mon 28 Oct 2019 01:50:54 PM CET, Vladimir Sementsov-Ogievskiy wrote:
>> -        .cow_end = {
>> -            .offset     = nb_bytes,
>
> hmm this logic is changed.. after the patch, it would be not nb_bytes, but
>
> offset_into_cluster(s, guest_offset) + MIN(*bytes, nb_bytes - offset_into_cluster(s, guest_offset))

It hasn't changed. The value of .cow_end.offset before the patch is
nb_bytes (these two lines are equivalent):

  nb_bytes = MIN(requested_bytes, avail_bytes);
  nb_bytes = MIN(*bytes + offset_into_cluster, avail_bytes);

After the patch we update *bytes to pass it to calculate_l2_meta(). Here
is the value (again, these are all equivalent):

  *bytes = MIN(*bytes, nb_bytes - offset_into_cluster)
  *bytes = MIN(*bytes, MIN(*bytes + offset_into_cluster, avail_bytes) -
           offset_into_cluster)
  *bytes = MIN(*bytes, MIN(*bytes, avail_bytes - offset_into_cluster))
  *bytes = MIN(*bytes, avail_bytes - offset_into_cluster)

And here's the value of .cow_end.offset set in calculate_l2_meta():

  cow_end_from = *bytes + offset_into_cluster
  cow_end_from = MIN(*bytes, avail_bytes - offset_into_cluster) + 
                 offset_into_cluster
  cow_end_from = MIN(*bytes + offset_into_cluster, avail_bytes)

This last definition of cow_end_from is the same as nb_bytes as used
before the patch.

Berto
Alberto Garcia Oct. 30, 2019, 4:02 p.m. UTC | #4
On Wed 30 Oct 2019 01:04:02 PM CET, Max Reitz wrote:
> (I intended not to comment on such things on an RFC, but here I am...)

No problem with that :-)

> I’d call it host_cluster_offset to make clear that it points to a
> cluster and isn’t the host offset for @guest_offset.

Sure, why not. We can also accept the exact offset within the cluster
and then use start_of_cluster(), but I prefer this one.

> And now that I’ve gone this far already, I might as well say that I’d
> like if it the comment noted that this function not only creates the
> L2Meta structure but also adds it to the cluster_allocs list.

I'll update the comment.

>> + * @guest_offset and @bytes indicate the offset and length of the
>> + * request.
>> + *
>> + * If @keep_old is true it means that the clusters were already
>> + * allocated and will be overwritten. If false then the clusters are
>> + * new and we have to decrease the reference count of the old ones.
>> + */
>> +static void calculate_l2_meta(BlockDriverState *bs, uint64_t host_offset,
>> +                              uint64_t guest_offset, uint64_t bytes,
>
> And now I’m so deep into non-RFC-level review territory, I might as
> well say I’d prefer @bytes to be an unsigned (or maybe even a plain
> int), because anything more wouldn’t work.  (Not least because
> cow_end_to is an unsigned).

True, I'll correct that too.

Thanks!

Berto
diff mbox series

Patch

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 8982b7b762..6c1dcdc781 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1019,6 +1019,55 @@  void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m)
                         QCOW2_DISCARD_NEVER);
 }
 
+/*
+ * For a given write request, create a new QCowL2Meta structure and
+ * add it to @m.
+ *
+ * @host_offset points to the beginning of the first cluster.
+ *
+ * @guest_offset and @bytes indicate the offset and length of the
+ * request.
+ *
+ * If @keep_old is true it means that the clusters were already
+ * allocated and will be overwritten. If false then the clusters are
+ * new and we have to decrease the reference count of the old ones.
+ */
+static void calculate_l2_meta(BlockDriverState *bs, uint64_t host_offset,
+                              uint64_t guest_offset, uint64_t bytes,
+                              QCowL2Meta **m, bool keep_old)
+{
+    BDRVQcow2State *s = bs->opaque;
+    unsigned cow_start_from = 0;
+    unsigned cow_start_to = offset_into_cluster(s, guest_offset);
+    unsigned cow_end_from = cow_start_to + bytes;
+    unsigned cow_end_to = ROUND_UP(cow_end_from, s->cluster_size);
+    unsigned nb_clusters = size_to_clusters(s, cow_end_from);
+    QCowL2Meta *old_m = *m;
+
+    *m = g_malloc0(sizeof(**m));
+    **m = (QCowL2Meta) {
+        .next           = old_m,
+
+        .alloc_offset   = host_offset,
+        .offset         = start_of_cluster(s, guest_offset),
+        .nb_clusters    = nb_clusters,
+
+        .keep_old_clusters = keep_old,
+
+        .cow_start = {
+            .offset     = cow_start_from,
+            .nb_bytes   = cow_start_to - cow_start_from,
+        },
+        .cow_end = {
+            .offset     = cow_end_from,
+            .nb_bytes   = cow_end_to - cow_end_from,
+        },
+    };
+
+    qemu_co_queue_init(&(*m)->dependent_requests);
+    QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight);
+}
+
 /*
  * Returns the number of contiguous clusters that can be used for an allocating
  * write, but require COW to be performed (this includes yet unallocated space,
@@ -1417,35 +1466,14 @@  static int handle_alloc(BlockDriverState *bs, uint64_t guest_offset,
     uint64_t requested_bytes = *bytes + offset_into_cluster(s, guest_offset);
     int avail_bytes = nb_clusters << s->cluster_bits;
     int nb_bytes = MIN(requested_bytes, avail_bytes);
-    QCowL2Meta *old_m = *m;
-
-    *m = g_malloc0(sizeof(**m));
-
-    **m = (QCowL2Meta) {
-        .next           = old_m,
-
-        .alloc_offset   = alloc_cluster_offset,
-        .offset         = start_of_cluster(s, guest_offset),
-        .nb_clusters    = nb_clusters,
-
-        .keep_old_clusters  = keep_old_clusters,
-
-        .cow_start = {
-            .offset     = 0,
-            .nb_bytes   = offset_into_cluster(s, guest_offset),
-        },
-        .cow_end = {
-            .offset     = nb_bytes,
-            .nb_bytes   = avail_bytes - nb_bytes,
-        },
-    };
-    qemu_co_queue_init(&(*m)->dependent_requests);
-    QLIST_INSERT_HEAD(&s->cluster_allocs, *m, next_in_flight);
 
     *host_offset = alloc_cluster_offset + offset_into_cluster(s, guest_offset);
     *bytes = MIN(*bytes, nb_bytes - offset_into_cluster(s, guest_offset));
     assert(*bytes != 0);
 
+    calculate_l2_meta(bs, alloc_cluster_offset, guest_offset, *bytes,
+                      m, keep_old_clusters);
+
     return 1;
 
 fail: