[PATCH] qcow2: keep reference on zeroize with discard-no-unref enabled

[PATCH] qcow2: keep reference on zeroize with discard-no-unref enabled

[Jean-Louis Dupond]

When the discard-no-unref flag is enabled, we keep the reference for
normal discard requests.
But when a discard is executed on a snapshot/qcow2 image with backing,
the discards are saved as zero clusters in the snapshot image.

When committing the snapshot to the backing file, not
discard_in_l2_slice is called but zero_in_l2_slice. Which did not had
any logic to keep the reference when discard-no-unref is enabled.

Therefor we add logic in the zero_in_l2_slice call to keep the reference
on commit.

Next to that we also revert some logic from 42a2890a and just call
qcow2_free_any_cluster. As this will just decrease the refcount but not
remove the reference itself. And it will also send the discard further
to the lower layer.

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1621
Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
---
 block/qcow2-cluster.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index f4f6cd6ad0..48532ca3c2 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1975,16 +1975,7 @@ static int discard_in_l2_slice(BlockDriverState *bs, uint64_t offset,
         if (has_subclusters(s)) {
             set_l2_bitmap(s, l2_slice, l2_index + i, new_l2_bitmap);
         }
-        if (!keep_reference) {
-            /* Then decrease the refcount */
-            qcow2_free_any_cluster(bs, old_l2_entry, type);
-        } else if (s->discard_passthrough[type] &&
-                   (cluster_type == QCOW2_CLUSTER_NORMAL ||
-                    cluster_type == QCOW2_CLUSTER_ZERO_ALLOC)) {
-            /* If we keep the reference, pass on the discard still */
-            bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK,
-                          s->cluster_size);
-       }
+        qcow2_free_any_cluster(bs, old_l2_entry, type);
     }
 
     qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice);
@@ -2062,9 +2053,14 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
         QCow2ClusterType type = qcow2_get_cluster_type(bs, old_l2_entry);
         bool unmap = (type == QCOW2_CLUSTER_COMPRESSED) ||
             ((flags & BDRV_REQ_MAY_UNMAP) && qcow2_cluster_is_allocated(type));
-        uint64_t new_l2_entry = unmap ? 0 : old_l2_entry;
+        uint64_t new_l2_entry = old_l2_entry;
         uint64_t new_l2_bitmap = old_l2_bitmap;
 
+        if (unmap &&
+            !(s->discard_no_unref && type != QCOW2_CLUSTER_COMPRESSED)) {
+            new_l2_entry = 0;
+        }
+
         if (has_subclusters(s)) {
             new_l2_bitmap = QCOW_L2_BITMAP_ALL_ZEROES;
         } else {
-- 
2.42.0

Re: [PATCH] qcow2: keep reference on zeroize with discard-no-unref enabled

[Jean-Louis Dupond]

Replaced by PATCH v2.

On 4/09/2023 17:09, Jean-Louis Dupond wrote:
> When the discard-no-unref flag is enabled, we keep the reference for
> normal discard requests.
> But when a discard is executed on a snapshot/qcow2 image with backing,
> the discards are saved as zero clusters in the snapshot image.
>
> When committing the snapshot to the backing file, not
> discard_in_l2_slice is called but zero_in_l2_slice. Which did not had
> any logic to keep the reference when discard-no-unref is enabled.
>
> Therefor we add logic in the zero_in_l2_slice call to keep the reference
> on commit.
>
> Next to that we also revert some logic from 42a2890a and just call
> qcow2_free_any_cluster. As this will just decrease the refcount but not
> remove the reference itself. And it will also send the discard further
> to the lower layer.
>
> Fixes: https://gitlab.com/qemu-project/qemu/-/issues/1621
> Signed-off-by: Jean-Louis Dupond <jean-louis@dupond.be>
> ---
>   block/qcow2-cluster.c | 18 +++++++-----------
>   1 file changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index f4f6cd6ad0..48532ca3c2 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1975,16 +1975,7 @@ static int discard_in_l2_slice(BlockDriverState *bs, uint64_t offset,
>           if (has_subclusters(s)) {
>               set_l2_bitmap(s, l2_slice, l2_index + i, new_l2_bitmap);
>           }
> -        if (!keep_reference) {
> -            /* Then decrease the refcount */
> -            qcow2_free_any_cluster(bs, old_l2_entry, type);
> -        } else if (s->discard_passthrough[type] &&
> -                   (cluster_type == QCOW2_CLUSTER_NORMAL ||
> -                    cluster_type == QCOW2_CLUSTER_ZERO_ALLOC)) {
> -            /* If we keep the reference, pass on the discard still */
> -            bdrv_pdiscard(s->data_file, old_l2_entry & L2E_OFFSET_MASK,
> -                          s->cluster_size);
> -       }
> +        qcow2_free_any_cluster(bs, old_l2_entry, type);
>       }
>   
>       qcow2_cache_put(s->l2_table_cache, (void **) &l2_slice);
> @@ -2062,9 +2053,14 @@ zero_in_l2_slice(BlockDriverState *bs, uint64_t offset,
>           QCow2ClusterType type = qcow2_get_cluster_type(bs, old_l2_entry);
>           bool unmap = (type == QCOW2_CLUSTER_COMPRESSED) ||
>               ((flags & BDRV_REQ_MAY_UNMAP) && qcow2_cluster_is_allocated(type));
> -        uint64_t new_l2_entry = unmap ? 0 : old_l2_entry;
> +        uint64_t new_l2_entry = old_l2_entry;
>           uint64_t new_l2_bitmap = old_l2_bitmap;
>   
> +        if (unmap &&
> +            !(s->discard_no_unref && type != QCOW2_CLUSTER_COMPRESSED)) {
> +            new_l2_entry = 0;
> +        }
> +
>           if (has_subclusters(s)) {
>               new_l2_bitmap = QCOW_L2_BITMAP_ALL_ZEROES;
>           } else {