[PATCH 0/3] qcow2: fix discard-no-unref=on data corruption

[PATCH 0/3] qcow2: fix discard-no-unref=on data corruption

[Andrey Drobyshev]

1st commit with tests is my own old commit from previously unmerged
subcluster series [1].
2nd is the actual bug fix, 3rd is the regression test.

[1] https://lore.kernel.org/qemu-devel/20240913163942.423050-1-andrey.drobyshev@virtuozzo.com

Andrey Drobyshev (3):
  iotests/290: add test case to check 'discard-no-unref' option behavior
  qcow2: fix snapshot data corruption with discard-no-unref=on
    #VSTOR-151885
  iotests/290: add 'discard' and 'write -z -u' case with a snapshot
    #VSTOR-151885

 block/qcow2-cluster.c      |  6 +++--
 tests/qemu-iotests/290     | 50 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/290.out | 33 +++++++++++++++++++++++++
 3 files changed, 87 insertions(+), 2 deletions(-)

-- 
2.47.1

[PATCH 1/3] iotests/290: add test case to check 'discard-no-unref' option behavior

[Andrey Drobyshev]

We basically fill 2 images with identical data and perform discard
operations with and without 'discard-no-unref' enabled.  Then we check
that images still read identically, that their disk usage is the same
(i.e. fallocate(FALLOC_FL_PUNCH_HOLE|FALLOC_FL_KEEP_SIZE) is called for
both) and that with the option enabled cluster is still marked as
allocated in "qemu-img map" output.  We also check that the option
doesn't work with qcow2 v2 images.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
Reviewed-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 tests/qemu-iotests/290     | 34 ++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/290.out | 28 ++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)

diff --git a/tests/qemu-iotests/290 b/tests/qemu-iotests/290
index 341a25f96b5..152fd7c5d1f 100755
--- a/tests/qemu-iotests/290
+++ b/tests/qemu-iotests/290
@@ -90,6 +90,40 @@ for qcow2_compat in 0.10 1.1; do
     $QEMU_IMG map "$TEST_IMG" | _filter_testdir
 done
 
+echo
+echo "### Test 'qemu-io -c discard' with 'discard-no-unref' option enabled"
+echo
+
+echo "# Check that qcow2 v2 images don't support 'discard-no-unref' option"
+NOUNREF_IMG="$TEST_IMG.nounref"
+TEST_IMG="$NOUNREF_IMG" _make_test_img -o "compat=0.10" 128k
+# This should immediately fail with an error
+$QEMU_IO -c 'reopen -o discard-no-unref=on' "$NOUNREF_IMG" | _filter_qemu_io
+
+echo "# Create two compat=1.1 images and fill them with identical data"
+_make_test_img -o "compat=1.1" 128k
+TEST_IMG="$NOUNREF_IMG" _make_test_img -o "compat=1.1" 128k
+$QEMU_IO -c 'write -P 0xaa 0 128k' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c 'write -P 0xaa 0 128k' "$NOUNREF_IMG" | _filter_qemu_io
+
+echo "# Enable 'discard-no-unref' in one of them, discard 2nd cluster in both"
+$QEMU_IO -c 'discard 64k 64k' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IO -c 'reopen -o discard-no-unref=on' \
+         -c 'discard 64k 64k' "$NOUNREF_IMG" | _filter_qemu_io
+
+echo "# Compare disk usage of the 2 images"
+# Don't check the exact disk usage values but rather that they're equal
+echo "disk_usage(`basename $TEST_IMG`) - disk_usage(`basename $NOUNREF_IMG`)" \
+     "= $(( `disk_usage $TEST_IMG` - `disk_usage $NOUNREF_IMG`))"
+
+echo "# Check that images are still identical"
+$QEMU_IMG compare "$TEST_IMG" "$NOUNREF_IMG"
+
+echo "# Output of qemu-img map for the image with dropped reference"
+$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
+echo "# Output of qemu-img map for the image with kept reference"
+$QEMU_IMG map --output=json "$NOUNREF_IMG" | _filter_qemu_img_map
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/290.out b/tests/qemu-iotests/290.out
index 22b476594fb..f790feae81a 100644
--- a/tests/qemu-iotests/290.out
+++ b/tests/qemu-iotests/290.out
@@ -58,4 +58,32 @@ read 131072/131072 bytes at offset 0
 128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 # Output of qemu-img map
 Offset          Length          Mapped to       File
+
+### Test 'qemu-io -c discard' with 'discard-no-unref' option enabled
+
+# Check that qcow2 v2 images don't support 'discard-no-unref' option
+Formatting 'TEST_DIR/t.IMGFMT.nounref', fmt=IMGFMT size=131072
+qemu-io: discard-no-unref is only supported since qcow2 version 3
+# Create two compat=1.1 images and fill them with identical data
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
+Formatting 'TEST_DIR/t.IMGFMT.nounref', fmt=IMGFMT size=131072
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+wrote 131072/131072 bytes at offset 0
+128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Enable 'discard-no-unref' in one of them, discard 2nd cluster in both
+discard 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+discard 65536/65536 bytes at offset 65536
+64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+# Compare disk usage of the 2 images
+disk_usage(t.qcow2) - disk_usage(t.qcow2.nounref) = 0
+# Check that images are still identical
+Images are identical.
+# Output of qemu-img map for the image with dropped reference
+[{ "start": 0, "length": 65536, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": OFFSET},
+{ "start": 65536, "length": 65536, "depth": 0, "present": true, "zero": true, "data": false, "compressed": false}]
+# Output of qemu-img map for the image with kept reference
+[{ "start": 0, "length": 65536, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": OFFSET},
+{ "start": 65536, "length": 65536, "depth": 0, "present": true, "zero": true, "data": false, "compressed": false, "offset": OFFSET}]
 *** done
-- 
2.47.1

Re: [PATCH 1/3] iotests/290: add test case to check 'discard-no-unref' option behavior

[Jean-Louis Dupond]

On 27/10/2026 11:44, Andrey Drobyshev wrote:
> We basically fill 2 images with identical data and perform discard
> operations with and without 'discard-no-unref' enabled.  Then we check
> that images still read identically, that their disk usage is the same
> (i.e. fallocate(FALLOC_FL_PUNCH_HOLE|FALLOC_FL_KEEP_SIZE) is called for
> both) and that with the option enabled cluster is still marked as
> allocated in "qemu-img map" output.  We also check that the option
> doesn't work with qcow2 v2 images.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> Reviewed-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>
> ---
>   tests/qemu-iotests/290     | 34 ++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/290.out | 28 ++++++++++++++++++++++++++++
>   2 files changed, 62 insertions(+)
>
> diff --git a/tests/qemu-iotests/290 b/tests/qemu-iotests/290
> index 341a25f96b5..152fd7c5d1f 100755
> --- a/tests/qemu-iotests/290
> +++ b/tests/qemu-iotests/290
> @@ -90,6 +90,40 @@ for qcow2_compat in 0.10 1.1; do
>       $QEMU_IMG map "$TEST_IMG" | _filter_testdir
>   done
>   
> +echo
> +echo "### Test 'qemu-io -c discard' with 'discard-no-unref' option enabled"
> +echo
> +
> +echo "# Check that qcow2 v2 images don't support 'discard-no-unref' option"
> +NOUNREF_IMG="$TEST_IMG.nounref"

This file should be cleaned in '_cleanup()'.
Happens in test 289 for example.

> +TEST_IMG="$NOUNREF_IMG" _make_test_img -o "compat=0.10" 128k
> +# This should immediately fail with an error
> +$QEMU_IO -c 'reopen -o discard-no-unref=on' "$NOUNREF_IMG" | _filter_qemu_io
> +
> +echo "# Create two compat=1.1 images and fill them with identical data"
> +_make_test_img -o "compat=1.1" 128k
> +TEST_IMG="$NOUNREF_IMG" _make_test_img -o "compat=1.1" 128k
> +$QEMU_IO -c 'write -P 0xaa 0 128k' "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IO -c 'write -P 0xaa 0 128k' "$NOUNREF_IMG" | _filter_qemu_io
> +
> +echo "# Enable 'discard-no-unref' in one of them, discard 2nd cluster in both"
> +$QEMU_IO -c 'discard 64k 64k' "$TEST_IMG" | _filter_qemu_io
> +$QEMU_IO -c 'reopen -o discard-no-unref=on' \
> +         -c 'discard 64k 64k' "$NOUNREF_IMG" | _filter_qemu_io
> +
> +echo "# Compare disk usage of the 2 images"
> +# Don't check the exact disk usage values but rather that they're equal
> +echo "disk_usage(`basename $TEST_IMG`) - disk_usage(`basename $NOUNREF_IMG`)" \
> +     "= $(( `disk_usage $TEST_IMG` - `disk_usage $NOUNREF_IMG`))"
Shouldn't we call _require_disk_usage in the test? Otherwise the test 
might be invalid.
Also something like the following would be cleaner:
img1=$(basename "$TEST_IMG")
img2=$(basename "$NOUNREF_IMG")
usage1=$(disk_usage "$TEST_IMG")
usage2=$(disk_usage "$NOUNREF_IMG")
echo "disk_usage($img1) - disk_usage($img2) = $((usage1 - usage2))"
> +
> +echo "# Check that images are still identical"
> +$QEMU_IMG compare "$TEST_IMG" "$NOUNREF_IMG"
> +
> +echo "# Output of qemu-img map for the image with dropped reference"
> +$QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
> +echo "# Output of qemu-img map for the image with kept reference"
> +$QEMU_IMG map --output=json "$NOUNREF_IMG" | _filter_qemu_img_map
> +
>   # success, all done
>   echo "*** done"
>   rm -f $seq.full
> diff --git a/tests/qemu-iotests/290.out b/tests/qemu-iotests/290.out
> index 22b476594fb..f790feae81a 100644
> --- a/tests/qemu-iotests/290.out
> +++ b/tests/qemu-iotests/290.out
> @@ -58,4 +58,32 @@ read 131072/131072 bytes at offset 0
>   128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>   # Output of qemu-img map
>   Offset          Length          Mapped to       File
> +
> +### Test 'qemu-io -c discard' with 'discard-no-unref' option enabled
> +
> +# Check that qcow2 v2 images don't support 'discard-no-unref' option
> +Formatting 'TEST_DIR/t.IMGFMT.nounref', fmt=IMGFMT size=131072
> +qemu-io: discard-no-unref is only supported since qcow2 version 3
> +# Create two compat=1.1 images and fill them with identical data
> +Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
> +Formatting 'TEST_DIR/t.IMGFMT.nounref', fmt=IMGFMT size=131072
> +wrote 131072/131072 bytes at offset 0
> +128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +wrote 131072/131072 bytes at offset 0
> +128 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +# Enable 'discard-no-unref' in one of them, discard 2nd cluster in both
> +discard 65536/65536 bytes at offset 65536
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +discard 65536/65536 bytes at offset 65536
> +64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> +# Compare disk usage of the 2 images
> +disk_usage(t.qcow2) - disk_usage(t.qcow2.nounref) = 0
> +# Check that images are still identical
> +Images are identical.
> +# Output of qemu-img map for the image with dropped reference
> +[{ "start": 0, "length": 65536, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": OFFSET},
> +{ "start": 65536, "length": 65536, "depth": 0, "present": true, "zero": true, "data": false, "compressed": false}]
> +# Output of qemu-img map for the image with kept reference
> +[{ "start": 0, "length": 65536, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": OFFSET},
> +{ "start": 65536, "length": 65536, "depth": 0, "present": true, "zero": true, "data": false, "compressed": false, "offset": OFFSET}]
>   *** done
Thanks
Jean-Louis

Re: [PATCH 1/3] iotests/290: add test case to check 'discard-no-unref' option behavior

[Andrey Drobyshev]

Hello Jean-Louis,

On 10/29/26 4:58 PM, Jean-Louis Dupond wrote:
> 
> On 27/10/2026 11:44, Andrey Drobyshev wrote:
>> We basically fill 2 images with identical data and perform discard
>> operations with and without 'discard-no-unref' enabled.  Then we check
>> that images still read identically, that their disk usage is the same
>> (i.e. fallocate(FALLOC_FL_PUNCH_HOLE|FALLOC_FL_KEEP_SIZE) is called for
>> both) and that with the option enabled cluster is still marked as
>> allocated in "qemu-img map" output.  We also check that the option
>> doesn't work with qcow2 v2 images.
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> Reviewed-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
>> ---
>>   tests/qemu-iotests/290     | 34 ++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/290.out | 28 ++++++++++++++++++++++++++++
>>   2 files changed, 62 insertions(+)
>>
>> diff --git a/tests/qemu-iotests/290 b/tests/qemu-iotests/290
>> index 341a25f96b5..152fd7c5d1f 100755
>> --- a/tests/qemu-iotests/290
>> +++ b/tests/qemu-iotests/290
>> @@ -90,6 +90,40 @@ for qcow2_compat in 0.10 1.1; do
>>       $QEMU_IMG map "$TEST_IMG" | _filter_testdir
>>   done
>>   
>> +echo
>> +echo "### Test 'qemu-io -c discard' with 'discard-no-unref' option enabled"
>> +echo
>> +
>> +echo "# Check that qcow2 v2 images don't support 'discard-no-unref' option"
>> +NOUNREF_IMG="$TEST_IMG.nounref"
> 
> This file should be cleaned in '_cleanup()'.
> Happens in test 289 for example.

Agreed.
>> +TEST_IMG="$NOUNREF_IMG" _make_test_img -o "compat=0.10" 128k
>> +# This should immediately fail with an error
>> +$QEMU_IO -c 'reopen -o discard-no-unref=on' "$NOUNREF_IMG" | _filter_qemu_io
>> +
>> +echo "# Create two compat=1.1 images and fill them with identical data"
>> +_make_test_img -o "compat=1.1" 128k
>> +TEST_IMG="$NOUNREF_IMG" _make_test_img -o "compat=1.1" 128k
>> +$QEMU_IO -c 'write -P 0xaa 0 128k' "$TEST_IMG" | _filter_qemu_io
>> +$QEMU_IO -c 'write -P 0xaa 0 128k' "$NOUNREF_IMG" | _filter_qemu_io
>> +
>> +echo "# Enable 'discard-no-unref' in one of them, discard 2nd cluster in both"
>> +$QEMU_IO -c 'discard 64k 64k' "$TEST_IMG" | _filter_qemu_io
>> +$QEMU_IO -c 'reopen -o discard-no-unref=on' \
>> +         -c 'discard 64k 64k' "$NOUNREF_IMG" | _filter_qemu_io
>> +
>> +echo "# Compare disk usage of the 2 images"
>> +# Don't check the exact disk usage values but rather that they're equal
>> +echo "disk_usage(`basename $TEST_IMG`) - disk_usage(`basename $NOUNREF_IMG`)" \
>> +     "= $(( `disk_usage $TEST_IMG` - `disk_usage $NOUNREF_IMG`))"
> Shouldn't we call _require_disk_usage in the test? Otherwise the test 
> might be invalid.
> Also something like the following would be cleaner:
> img1=$(basename "$TEST_IMG")
> img2=$(basename "$NOUNREF_IMG")
> usage1=$(disk_usage "$TEST_IMG")
> usage2=$(disk_usage "$NOUNREF_IMG")
> echo "disk_usage($img1) - disk_usage($img2) = $((usage1 - usage2))"

You're right, we should add the check.

Thanks,
Andrey

Re: [PATCH 1/3] iotests/290: add test case to check 'discard-no-unref' option behavior

[Kevin Wolf]

Am 27.10.2026 um 11:44 hat Andrey Drobyshev geschrieben:
> We basically fill 2 images with identical data and perform discard
> operations with and without 'discard-no-unref' enabled.  Then we check
> that images still read identically, that their disk usage is the same
> (i.e. fallocate(FALLOC_FL_PUNCH_HOLE|FALLOC_FL_KEEP_SIZE) is called for
> both) and that with the option enabled cluster is still marked as
> allocated in "qemu-img map" output.  We also check that the option
> doesn't work with qcow2 v2 images.
> 
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> Reviewed-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
> Reviewed-by: Alberto Garcia <berto@igalia.com>

This test fails with -fuse:

$ tests/qemu-iotests/check -fuse -qcow2 290
[...]
290   fail       [16:49:10] [16:49:16]   5.3s                 output mismatch (see /home/kwolf/source/qemu/build-clang/scratch/qcow2-fuse-290/290.out.bad)
--- /home/kwolf/source/qemu/tests/qemu-iotests/290.out
+++ /home/kwolf/source/qemu/build-clang/scratch/qcow2-fuse-290/290.out.bad
@@ -77,7 +77,7 @@
 discard 65536/65536 bytes at offset 65536
 64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 # Compare disk usage of the 2 images
-disk_usage(t.qcow2) - disk_usage(t.qcow2.nounref) = 0
+disk_usage(fuse-t.qcow2) - disk_usage(fuse-t.qcow2.nounref) = 0
 # Check that images are still identical
 Images are identical.
 # Output of qemu-img map for the image with dropped reference
Failures: 290
Failed 1 of 1 iotests

Kevin

Re: [PATCH 1/3] iotests/290: add test case to check 'discard-no-unref' option behavior

[Andrey Drobyshev]

On 11/3/26 5:53 PM, Kevin Wolf wrote:
> Am 27.10.2026 um 11:44 hat Andrey Drobyshev geschrieben:
>> We basically fill 2 images with identical data and perform discard
>> operations with and without 'discard-no-unref' enabled.  Then we check
>> that images still read identically, that their disk usage is the same
>> (i.e. fallocate(FALLOC_FL_PUNCH_HOLE|FALLOC_FL_KEEP_SIZE) is called for
>> both) and that with the option enabled cluster is still marked as
>> allocated in "qemu-img map" output.  We also check that the option
>> doesn't work with qcow2 v2 images.
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> Reviewed-by: Alexander Ivanov <alexander.ivanov@virtuozzo.com>
>> Reviewed-by: Alberto Garcia <berto@igalia.com>
> 
> This test fails with -fuse:
> 
> $ tests/qemu-iotests/check -fuse -qcow2 290
> [...]
> 290   fail       [16:49:10] [16:49:16]   5.3s                 output mismatch (see /home/kwolf/source/qemu/build-clang/scratch/qcow2-fuse-290/290.out.bad)
> --- /home/kwolf/source/qemu/tests/qemu-iotests/290.out
> +++ /home/kwolf/source/qemu/build-clang/scratch/qcow2-fuse-290/290.out.bad
> @@ -77,7 +77,7 @@
>  discard 65536/65536 bytes at offset 65536
>  64 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  # Compare disk usage of the 2 images
> -disk_usage(t.qcow2) - disk_usage(t.qcow2.nounref) = 0
> +disk_usage(fuse-t.qcow2) - disk_usage(fuse-t.qcow2.nounref) = 0
>  # Check that images are still identical
>  Images are identical.
>  # Output of qemu-img map for the image with dropped reference
> Failures: 290
> Failed 1 of 1 iotests
> 
> Kevin

Hello Kevin,

It fails indeed, will fix in v2.

Thanks,
Andrey

[PATCH 2/3] qcow2: fix snapshot data corruption with discard-no-unref=on

[Andrey Drobyshev]

Whenever option discard-no-unref=on is enabled, we bypass refcount
machinery and unconditionally punch hole in the data on 'discard' and
'write -z -u' paths.  However, when this same cluster is also referenced
by an internal snapshot, such operations lead to problems.  The active L2
table is copied before it is changed, so the snapshot metadata stays
correct, but the host range under it is punched out and the snapshot reads
zeroes there afterwards. qemu-img check finds nothing wrong.

Fix this by guarding the 'keep_reference' flag by the presence of the
QCOW_OFLAG_COPIED bit.  I.e. only proceed with unconditional data
punching if we own this cluster exclusively.

Fixes: 42a2890a76f4 ("qcow2: add discard-no-unref option")
Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 block/qcow2-cluster.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index e02fae6a0c6..cc473c7c796 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1953,6 +1953,7 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t offset, uint64_t nb_clusters,
         QCow2ClusterType cluster_type =
             qcow2_get_cluster_type(bs, old_l2_entry);
         bool keep_reference = (cluster_type != QCOW2_CLUSTER_COMPRESSED) &&
+                              (old_l2_entry & QCOW_OFLAG_COPIED) &&
                               !full_discard &&
                               (s->discard_no_unref &&
                                type == QCOW2_DISCARD_REQUEST);
@@ -2097,8 +2098,9 @@ 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));
-        bool keep_reference =
-            (s->discard_no_unref && type != QCOW2_CLUSTER_COMPRESSED);
+        bool keep_reference = s->discard_no_unref &&
+                              type != QCOW2_CLUSTER_COMPRESSED &&
+                              (old_l2_entry & QCOW_OFLAG_COPIED);
         uint64_t new_l2_entry = old_l2_entry;
         uint64_t new_l2_bitmap = old_l2_bitmap;
 
-- 
2.47.1

Re: [PATCH 2/3] qcow2: fix snapshot data corruption with discard-no-unref=on

[Jean-Louis Dupond]

On 27/10/2026 11:44, Andrey Drobyshev wrote:
> Whenever option discard-no-unref=on is enabled, we bypass refcount
> machinery and unconditionally punch hole in the data on 'discard' and
> 'write -z -u' paths.  However, when this same cluster is also referenced
> by an internal snapshot, such operations lead to problems.  The active L2
> table is copied before it is changed, so the snapshot metadata stays
> correct, but the host range under it is punched out and the snapshot reads
> zeroes there afterwards. qemu-img check finds nothing wrong.
>
> Fix this by guarding the 'keep_reference' flag by the presence of the
> QCOW_OFLAG_COPIED bit.  I.e. only proceed with unconditional data
> punching if we own this cluster exclusively.
>
> Fixes: 42a2890a76f4 ("qcow2: add discard-no-unref option")
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>   block/qcow2-cluster.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index e02fae6a0c6..cc473c7c796 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1953,6 +1953,7 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t offset, uint64_t nb_clusters,
>           QCow2ClusterType cluster_type =
>               qcow2_get_cluster_type(bs, old_l2_entry);
>           bool keep_reference = (cluster_type != QCOW2_CLUSTER_COMPRESSED) &&
> +                              (old_l2_entry & QCOW_OFLAG_COPIED) &&
>                                 !full_discard &&
>                                 (s->discard_no_unref &&
>                                  type == QCOW2_DISCARD_REQUEST);
Wouldn't it be better to gate only qcow2_discard_cluster when the 
cluster is QCOW_OFLAG_COPIED?
Cause with the above change, we fully clear/remove the offset of the 
cluster, causing fragmentation again in the image afterwards.
And that was just the goal for discard-no-unref :)
> @@ -2097,8 +2098,9 @@ 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));
> -        bool keep_reference =
> -            (s->discard_no_unref && type != QCOW2_CLUSTER_COMPRESSED);
> +        bool keep_reference = s->discard_no_unref &&
> +                              type != QCOW2_CLUSTER_COMPRESSED &&
> +                              (old_l2_entry & QCOW_OFLAG_COPIED);
>           uint64_t new_l2_entry = old_l2_entry;
>           uint64_t new_l2_bitmap = old_l2_bitmap;
>   

And same here then.

Thanks
Jean-Louis

Re: [PATCH 2/3] qcow2: fix snapshot data corruption with discard-no-unref=on

[Kevin Wolf]

Am 29.10.2026 um 15:58 hat Jean-Louis Dupond geschrieben:
> 
> On 27/10/2026 11:44, Andrey Drobyshev wrote:
> > Whenever option discard-no-unref=on is enabled, we bypass refcount
> > machinery and unconditionally punch hole in the data on 'discard' and
> > 'write -z -u' paths.  However, when this same cluster is also referenced
> > by an internal snapshot, such operations lead to problems.  The active L2
> > table is copied before it is changed, so the snapshot metadata stays
> > correct, but the host range under it is punched out and the snapshot reads
> > zeroes there afterwards. qemu-img check finds nothing wrong.
> > 
> > Fix this by guarding the 'keep_reference' flag by the presence of the
> > QCOW_OFLAG_COPIED bit.  I.e. only proceed with unconditional data
> > punching if we own this cluster exclusively.
> > 
> > Fixes: 42a2890a76f4 ("qcow2: add discard-no-unref option")
> > Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> > ---
> >   block/qcow2-cluster.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> > index e02fae6a0c6..cc473c7c796 100644
> > --- a/block/qcow2-cluster.c
> > +++ b/block/qcow2-cluster.c
> > @@ -1953,6 +1953,7 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t offset, uint64_t nb_clusters,
> >           QCow2ClusterType cluster_type =
> >               qcow2_get_cluster_type(bs, old_l2_entry);
> >           bool keep_reference = (cluster_type != QCOW2_CLUSTER_COMPRESSED) &&
> > +                              (old_l2_entry & QCOW_OFLAG_COPIED) &&
> >                                 !full_discard &&
> >                                 (s->discard_no_unref &&
> >                                  type == QCOW2_DISCARD_REQUEST);
> 
> Wouldn't it be better to gate only qcow2_discard_cluster when the cluster is
> QCOW_OFLAG_COPIED?
> Cause with the above change, we fully clear/remove the offset of the
> cluster, causing fragmentation again in the image afterwards.
> And that was just the goal for discard-no-unref :)

I thought of this, too, but merely because it seems to be a closer match
for the expectation the name "discard-no-unref" sets.

In practice, I don't expect that you'd see any advantage from it.
Writing again to this cluster will cause COW, so you'll get
fragmentation either way.

Kevin

Re: [PATCH 2/3] qcow2: fix snapshot data corruption with discard-no-unref=on

[Andrey Drobyshev]

On 11/3/26 6:06 PM, Kevin Wolf wrote:
> Am 29.10.2026 um 15:58 hat Jean-Louis Dupond geschrieben:
>>
>> On 27/10/2026 11:44, Andrey Drobyshev wrote:
>>> Whenever option discard-no-unref=on is enabled, we bypass refcount
>>> machinery and unconditionally punch hole in the data on 'discard' and
>>> 'write -z -u' paths.  However, when this same cluster is also referenced
>>> by an internal snapshot, such operations lead to problems.  The active L2
>>> table is copied before it is changed, so the snapshot metadata stays
>>> correct, but the host range under it is punched out and the snapshot reads
>>> zeroes there afterwards. qemu-img check finds nothing wrong.
>>>
>>> Fix this by guarding the 'keep_reference' flag by the presence of the
>>> QCOW_OFLAG_COPIED bit.  I.e. only proceed with unconditional data
>>> punching if we own this cluster exclusively.
>>>
>>> Fixes: 42a2890a76f4 ("qcow2: add discard-no-unref option")
>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>>> ---
>>>   block/qcow2-cluster.c | 6 ++++--
>>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>>> index e02fae6a0c6..cc473c7c796 100644
>>> --- a/block/qcow2-cluster.c
>>> +++ b/block/qcow2-cluster.c
>>> @@ -1953,6 +1953,7 @@ discard_in_l2_slice(BlockDriverState *bs, uint64_t offset, uint64_t nb_clusters,
>>>           QCow2ClusterType cluster_type =
>>>               qcow2_get_cluster_type(bs, old_l2_entry);
>>>           bool keep_reference = (cluster_type != QCOW2_CLUSTER_COMPRESSED) &&
>>> +                              (old_l2_entry & QCOW_OFLAG_COPIED) &&
>>>                                 !full_discard &&
>>>                                 (s->discard_no_unref &&
>>>                                  type == QCOW2_DISCARD_REQUEST);
>>
>> Wouldn't it be better to gate only qcow2_discard_cluster when the cluster is
>> QCOW_OFLAG_COPIED?
>> Cause with the above change, we fully clear/remove the offset of the
>> cluster, causing fragmentation again in the image afterwards.
>> And that was just the goal for discard-no-unref :)
> 
> I thought of this, too, but merely because it seems to be a closer match
> for the expectation the name "discard-no-unref" sets.
> 
> In practice, I don't expect that you'd see any advantage from it.
> Writing again to this cluster will cause COW, so you'll get
> fragmentation either way.
> 
> Kevin

I agree with Kevin here, because for clusters whose refcount > 1 keeping
the offset gains us nothing.  Any subsequent write to such cluster will
go through handle_alloc() and allocating a new one.

So I'll keep this patch as-is, if there're no further objections.

Thanks,
Andrey

[PATCH 3/3] iotests/290: add 'discard' and 'write -z -u' case with a snapshot

[Andrey Drobyshev]

Before the previous commit, issuing 'discard' and 'write -z -u'
operations with discard-no-unref=on option enabled led to data
corruption.  Add a test to cover this case.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 tests/qemu-iotests/290     | 16 ++++++++++++++++
 tests/qemu-iotests/290.out |  5 +++++
 2 files changed, 21 insertions(+)

diff --git a/tests/qemu-iotests/290 b/tests/qemu-iotests/290
index 152fd7c5d1f..049a0ef7f13 100755
--- a/tests/qemu-iotests/290
+++ b/tests/qemu-iotests/290
@@ -124,6 +124,22 @@ $QEMU_IMG map --output=json "$TEST_IMG" | _filter_qemu_img_map
 echo "# Output of qemu-img map for the image with kept reference"
 $QEMU_IMG map --output=json "$NOUNREF_IMG" | _filter_qemu_img_map
 
+echo
+echo "### Discard with 'discard-no-unref' on a cluster shared with a snapshot"
+echo
+
+_make_test_img 128k
+$QEMU_IO -c 'write -q -P 0xaa 0 128k' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG snapshot -c snap "$TEST_IMG"
+QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT \
+$QEMU_IO -c 'discard -q 0 64k' -c 'write -q -z -u 64k 64k' --image-opts \
+         "driver=$IMGFMT,file.filename=$TEST_IMG,discard-no-unref=on" \
+         | _filter_qemu_io
+$QEMU_IO -c 'read -q -P 0 0 128k' "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG snapshot -a snap "$TEST_IMG"
+$QEMU_IO -c 'read -q -P 0xaa 0 128k' "$TEST_IMG" | _filter_qemu_io
+_check_test_img
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/290.out b/tests/qemu-iotests/290.out
index f790feae81a..aa5d164fa9f 100644
--- a/tests/qemu-iotests/290.out
+++ b/tests/qemu-iotests/290.out
@@ -86,4 +86,9 @@ Images are identical.
 # Output of qemu-img map for the image with kept reference
 [{ "start": 0, "length": 65536, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": OFFSET},
 { "start": 65536, "length": 65536, "depth": 0, "present": true, "zero": true, "data": false, "compressed": false, "offset": OFFSET}]
+
+### Discard with 'discard-no-unref' on a cluster shared with a snapshot
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=131072
+No errors were found on the image.
 *** done
-- 
2.47.1

Re: [PATCH 0/3] qcow2: fix discard-no-unref=on data corruption

[Denis V. Lunev]

On 10/27/26 11:44, Andrey Drobyshev wrote:
> 1st commit with tests is my own old commit from previously unmerged
> subcluster series [1].
> 2nd is the actual bug fix, 3rd is the regression test.
>
> [1] https://lore.kernel.org/qemu-devel/20240913163942.423050-1-andrey.drobyshev@virtuozzo.com
>
> Andrey Drobyshev (3):
>   iotests/290: add test case to check 'discard-no-unref' option behavior
>   qcow2: fix snapshot data corruption with discard-no-unref=on
>     #VSTOR-151885
>   iotests/290: add 'discard' and 'write -z -u' case with a snapshot
>     #VSTOR-151885
>
>  block/qcow2-cluster.c      |  6 +++--
>  tests/qemu-iotests/290     | 50 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/290.out | 33 +++++++++++++++++++++++++
>  3 files changed, 87 insertions(+), 2 deletions(-)
>
Reviewed-by: Denis V. Lunev <den@openvz.org>

Re: [PATCH 0/3] qcow2: fix discard-no-unref=on data corruption

[Kevin Wolf]

Am 27.10.2026 um 11:44 hat Andrey Drobyshev geschrieben:
> 1st commit with tests is my own old commit from previously unmerged
> subcluster series [1].
> 2nd is the actual bug fix, 3rd is the regression test.
> 
> [1] https://lore.kernel.org/qemu-devel/20240913163942.423050-1-andrey.drobyshev@virtuozzo.com
> 
> Andrey Drobyshev (3):
>   iotests/290: add test case to check 'discard-no-unref' option behavior
>   qcow2: fix snapshot data corruption with discard-no-unref=on
>     #VSTOR-151885
>   iotests/290: add 'discard' and 'write -z -u' case with a snapshot
>     #VSTOR-151885
> 
>  block/qcow2-cluster.c      |  6 +++--
>  tests/qemu-iotests/290     | 50 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/290.out | 33 +++++++++++++++++++++++++
>  3 files changed, 87 insertions(+), 2 deletions(-)

Patches 2 and 3:
Reviewed-by: Kevin Wolf <kwolf@redhat.com>