diff mbox series

[v2,02/11] qcow2: simplify L2 entries accounting for discard-no-unref

Message ID 20240513063203.113911-3-andrey.drobyshev@virtuozzo.com (mailing list archive)
State New, archived
Headers show
Series qcow2: make subclusters discardable | expand

Commit Message

Andrey Drobyshev May 13, 2024, 6:31 a.m. UTC
Commits 42a2890a and b2b10904 introduce handling of discard-no-unref
option in discard_in_l2_slice() and zero_in_l2_slice().  They add even
more if's when chosing the right l2 entry.  What we really need for this
option is the new entry simply to contain the same host cluster offset,
no matter whether we unmap or zeroize the cluster.  For that OR'ing with
the old entry is enough.

This patch doesn't change the logic and is pure refactoring.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 block/qcow2-cluster.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

Comments

Alberto Garcia May 15, 2024, 10:28 p.m. UTC | #1
On Mon 13 May 2024 09:31:54 AM +03, Andrey Drobyshev wrote:
> Commits 42a2890a and b2b10904 introduce handling of discard-no-unref
> option in discard_in_l2_slice() and zero_in_l2_slice().  They add even
> more if's when chosing the right l2 entry.  What we really need for this
> option is the new entry simply to contain the same host cluster offset,
> no matter whether we unmap or zeroize the cluster.  For that OR'ing with
> the old entry is enough.
>
> This patch doesn't change the logic and is pure refactoring.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto
Alexander Ivanov May 21, 2024, 8:32 a.m. UTC | #2
On 5/13/24 08:31, Andrey Drobyshev wrote:
> Commits 42a2890a and b2b10904 introduce handling of discard-no-unref
> option in discard_in_l2_slice() and zero_in_l2_slice().  They add even
> more if's when chosing the right l2 entry.  What we really need for this
> option is the new entry simply to contain the same host cluster offset,
> no matter whether we unmap or zeroize the cluster.  For that OR'ing with
> the old entry is enough.
>
> This patch doesn't change the logic and is pure refactoring.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   block/qcow2-cluster.c | 34 +++++++++++++++-------------------
>   1 file changed, 15 insertions(+), 19 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index ce8c0076b3..5f057ba2fd 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1946,25 +1946,21 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t offset, uint64_t nb_clusters,
>               new_l2_entry = new_l2_bitmap = 0;
>           } else if (bs->backing || qcow2_cluster_is_allocated(cluster_type)) {
>               if (has_subclusters(s)) {
> -                if (keep_reference) {
> -                    new_l2_entry = old_l2_entry;
> -                } else {
> -                    new_l2_entry = 0;
> -                }
> +                new_l2_entry = 0;
>                   new_l2_bitmap = QCOW_L2_BITMAP_ALL_ZEROES;
>               } else {
> -                if (s->qcow_version >= 3) {
> -                    if (keep_reference) {
> -                        new_l2_entry |= QCOW_OFLAG_ZERO;
> -                    } else {
> -                        new_l2_entry = QCOW_OFLAG_ZERO;
> -                    }
> -                } else {
> -                    new_l2_entry = 0;
> -                }
> +                new_l2_entry = s->qcow_version >= 3 ? QCOW_OFLAG_ZERO : 0;
>               }
>           }
>   
> +        /*
> +         * No need to check for the QCOW version since discard-no-unref is
> +         * only allowed since version 3.
> +         */
> +        if (keep_reference) {
> +            new_l2_entry |= old_l2_entry;
> +        }
> +
>           if (old_l2_entry == new_l2_entry && old_l2_bitmap == new_l2_bitmap) {
>               continue;
>           }
> @@ -2064,19 +2060,19 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
>               ((flags & BDRV_REQ_MAY_UNMAP) && qcow2_cluster_is_allocated(type));
>           bool keep_reference =
>               (s->discard_no_unref && type != QCOW2_CLUSTER_COMPRESSED);
> -        uint64_t new_l2_entry = old_l2_entry;
> +        uint64_t new_l2_entry = unmap ? 0 : old_l2_entry;
>           uint64_t new_l2_bitmap = old_l2_bitmap;
>   
> -        if (unmap && !keep_reference) {
> -            new_l2_entry = 0;
> -        }
> -
>           if (has_subclusters(s)) {
>               new_l2_bitmap = QCOW_L2_BITMAP_ALL_ZEROES;
>           } else {
>               new_l2_entry |= QCOW_OFLAG_ZERO;
>           }
>   
> +        if (keep_reference) {
> +            new_l2_entry |= old_l2_entry;
> +        }
> +
>           if (old_l2_entry == new_l2_entry && old_l2_bitmap == new_l2_bitmap) {
>               continue;
>           }
Reviewed-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
diff mbox series

Patch

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index ce8c0076b3..5f057ba2fd 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1946,25 +1946,21 @@  discard_in_l2_slice(BlockDriverState *bs, uint64_t offset, uint64_t nb_clusters,
             new_l2_entry = new_l2_bitmap = 0;
         } else if (bs->backing || qcow2_cluster_is_allocated(cluster_type)) {
             if (has_subclusters(s)) {
-                if (keep_reference) {
-                    new_l2_entry = old_l2_entry;
-                } else {
-                    new_l2_entry = 0;
-                }
+                new_l2_entry = 0;
                 new_l2_bitmap = QCOW_L2_BITMAP_ALL_ZEROES;
             } else {
-                if (s->qcow_version >= 3) {
-                    if (keep_reference) {
-                        new_l2_entry |= QCOW_OFLAG_ZERO;
-                    } else {
-                        new_l2_entry = QCOW_OFLAG_ZERO;
-                    }
-                } else {
-                    new_l2_entry = 0;
-                }
+                new_l2_entry = s->qcow_version >= 3 ? QCOW_OFLAG_ZERO : 0;
             }
         }
 
+        /*
+         * No need to check for the QCOW version since discard-no-unref is
+         * only allowed since version 3.
+         */
+        if (keep_reference) {
+            new_l2_entry |= old_l2_entry;
+        }
+
         if (old_l2_entry == new_l2_entry && old_l2_bitmap == new_l2_bitmap) {
             continue;
         }
@@ -2064,19 +2060,19 @@  zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
             ((flags & BDRV_REQ_MAY_UNMAP) && qcow2_cluster_is_allocated(type));
         bool keep_reference =
             (s->discard_no_unref && type != QCOW2_CLUSTER_COMPRESSED);
-        uint64_t new_l2_entry = old_l2_entry;
+        uint64_t new_l2_entry = unmap ? 0 : old_l2_entry;
         uint64_t new_l2_bitmap = old_l2_bitmap;
 
-        if (unmap && !keep_reference) {
-            new_l2_entry = 0;
-        }
-
         if (has_subclusters(s)) {
             new_l2_bitmap = QCOW_L2_BITMAP_ALL_ZEROES;
         } else {
             new_l2_entry |= QCOW_OFLAG_ZERO;
         }
 
+        if (keep_reference) {
+            new_l2_entry |= old_l2_entry;
+        }
+
         if (old_l2_entry == new_l2_entry && old_l2_bitmap == new_l2_bitmap) {
             continue;
         }