[Qemu-devel] [PATCH 0/3] qcow2: Fix cluster leaks on write error

[Qemu-devel] [PATCH 0/3] qcow2: Fix cluster leaks on write error

[Kevin Wolf]

Kevin Wolf (3):
  qemu-iotests: Update 026.out.nocache reference output
  qcow2: Free allocated clusters on write error
  qemu-iotests: Test qcow2 not leaking clusters on write error

 block/qcow2.h                      |  1 +
 block/qcow2-cluster.c              | 11 +++++++++++
 block/qcow2.c                      |  2 ++
 tests/qemu-iotests/026             | 17 +++++++++++++++++
 tests/qemu-iotests/026.out         |  8 ++++++++
 tests/qemu-iotests/026.out.nocache | 14 +++++++++++---
 6 files changed, 50 insertions(+), 3 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH 1/3] qemu-iotests: Update 026.out.nocache reference output

[Kevin Wolf]

Commit abf754fe406 updated 026.out, but forgot to also update
026.out.nocache.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/026.out.nocache | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/026.out.nocache b/tests/qemu-iotests/026.out.nocache
index ea2e166a4e..650ccd8b01 100644
--- a/tests/qemu-iotests/026.out.nocache
+++ b/tests/qemu-iotests/026.out.nocache
@@ -541,7 +541,7 @@ Failed to flush the L2 table cache: No space left on device
 Failed to flush the refcount block cache: No space left on device
 write failed: No space left on device
 
-11 leaked clusters were found on the image.
+10 leaked clusters were found on the image.
 This means waste of disk space, but no harm to data.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824 
 
@@ -569,7 +569,7 @@ Failed to flush the L2 table cache: No space left on device
 Failed to flush the refcount block cache: No space left on device
 write failed: No space left on device
 
-11 leaked clusters were found on the image.
+10 leaked clusters were found on the image.
 This means waste of disk space, but no harm to data.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824 
 
@@ -597,7 +597,7 @@ Failed to flush the L2 table cache: No space left on device
 Failed to flush the refcount block cache: No space left on device
 write failed: No space left on device
 
-11 leaked clusters were found on the image.
+10 leaked clusters were found on the image.
 This means waste of disk space, but no harm to data.
 Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824 
 
-- 
2.13.6

[Qemu-devel] [PATCH 2/3] qcow2: Free allocated clusters on write error

[Kevin Wolf]

If we managed to allocate the clusters, but then failed to write the
data, there's a good chance that we'll still be able to free the
clusters again in order to avoid cluster leaks (the refcounts are
cached, so even if we can't write them out right now, we may be able to
do so when the VM is resumed after a werror=stop/enospc pause).

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.h         |  1 +
 block/qcow2-cluster.c | 11 +++++++++++
 block/qcow2.c         |  2 ++
 3 files changed, 14 insertions(+)

diff --git a/block/qcow2.h b/block/qcow2.h
index 01b5250415..1c9c0d3631 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -614,6 +614,7 @@ uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
                                          int compressed_size);
 
 int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
+void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m);
 int qcow2_cluster_discard(BlockDriverState *bs, uint64_t offset,
                           uint64_t bytes, enum qcow2_discard_type type,
                           bool full_discard);
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 0d74584c9b..d37fe08b3d 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -994,6 +994,17 @@ err:
     return ret;
  }
 
+/**
+ * Frees the allocated clusters because the request failed and they won't
+ * actually be linked.
+ */
+void qcow2_alloc_cluster_abort(BlockDriverState *bs, QCowL2Meta *m)
+{
+    BDRVQcow2State *s = bs->opaque;
+    qcow2_free_clusters(bs, m->alloc_offset, m->nb_clusters << s->cluster_bits,
+                        QCOW2_DISCARD_NEVER);
+}
+
 /*
  * 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,
diff --git a/block/qcow2.c b/block/qcow2.c
index da54b2c8f8..7a4db12afd 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1777,6 +1777,8 @@ static coroutine_fn int qcow2_handle_l2meta(BlockDriverState *bs,
             if (ret) {
                 goto out;
             }
+        } else {
+            qcow2_alloc_cluster_abort(bs, l2meta);
         }
 
         /* Take the request off the list of running requests */
-- 
2.13.6

[Qemu-devel] [PATCH 3/3] qemu-iotests: Test qcow2 not leaking clusters on write error

[Kevin Wolf]

This adds a test for a temporary write failure, which simulates the
situation after werror=stop/enospc has stopped the VM. We shouldn't
leave leaked clusters behind in such cases.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 tests/qemu-iotests/026             | 17 +++++++++++++++++
 tests/qemu-iotests/026.out         |  8 ++++++++
 tests/qemu-iotests/026.out.nocache |  8 ++++++++
 3 files changed, 33 insertions(+)

diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026
index 7fadfbace5..582d254195 100755
--- a/tests/qemu-iotests/026
+++ b/tests/qemu-iotests/026
@@ -200,6 +200,23 @@ done
 done
 done
 
+echo
+echo === Avoid cluster leaks after temporary failure ===
+echo
+
+cat > "$TEST_DIR/blkdebug.conf" <<EOF
+[inject-error]
+event = "write_aio"
+errno = "5"
+once = "on"
+EOF
+
+# After the failed first write, do a second write so that the updated refcount
+# block is actually written back
+_make_test_img 64M
+$QEMU_IO -c "write 0 1M" -c "write 0 1M" "$BLKDBG_TEST_IMG" | _filter_qemu_io
+_check_test_img
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/026.out b/tests/qemu-iotests/026.out
index 8e89416a86..dd10a82b51 100644
--- a/tests/qemu-iotests/026.out
+++ b/tests/qemu-iotests/026.out
@@ -675,4 +675,12 @@ write failed: No space left on device
 
 96 leaked clusters were found on the image.
 This means waste of disk space, but no harm to data.
+
+=== Avoid cluster leaks after temporary failure ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+write failed: Input/output error
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+No errors were found on the image.
 *** done
diff --git a/tests/qemu-iotests/026.out.nocache b/tests/qemu-iotests/026.out.nocache
index 650ccd8b01..1ca6cda15c 100644
--- a/tests/qemu-iotests/026.out.nocache
+++ b/tests/qemu-iotests/026.out.nocache
@@ -683,4 +683,12 @@ write failed: No space left on device
 
 96 leaked clusters were found on the image.
 This means waste of disk space, but no harm to data.
+
+=== Avoid cluster leaks after temporary failure ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+write failed: Input/output error
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+No errors were found on the image.
 *** done
-- 
2.13.6

Re: [Qemu-devel] [PATCH 2/3] qcow2: Free allocated clusters on write error

[Eric Blake]

On 06/28/2018 10:39 AM, Kevin Wolf wrote:
> If we managed to allocate the clusters, but then failed to write the
> data, there's a good chance that we'll still be able to free the
> clusters again in order to avoid cluster leaks (the refcounts are
> cached, so even if we can't write them out right now, we may be able to
> do so when the VM is resumed after a werror=stop/enospc pause).
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>   block/qcow2.h         |  1 +
>   block/qcow2-cluster.c | 11 +++++++++++
>   block/qcow2.c         |  2 ++
>   3 files changed, 14 insertions(+)

Hmm, I wonder if this interacts poorly with my proposed test addition 
for HUGE images, which is still pending review:
https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg05488.html
https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg04542.html

(time for me to go testing...)

> +++ b/block/qcow2.c
> @@ -1777,6 +1777,8 @@ static coroutine_fn int qcow2_handle_l2meta(BlockDriverState *bs,
>               if (ret) {
>                   goto out;
>               }
> +        } else {
> +            qcow2_alloc_cluster_abort(bs, l2meta);
>           }

None of our existing tests caught this? But it looks right to me.
Reviewed-by: Eric Blake <eblake@redhat.com>


-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 0/3] qcow2: Fix cluster leaks on write error

[Max Reitz]

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

On 2018-06-28 17:39, Kevin Wolf wrote:
> Kevin Wolf (3):
>   qemu-iotests: Update 026.out.nocache reference output
>   qcow2: Free allocated clusters on write error
>   qemu-iotests: Test qcow2 not leaking clusters on write error
> 
>  block/qcow2.h                      |  1 +
>  block/qcow2-cluster.c              | 11 +++++++++++
>  block/qcow2.c                      |  2 ++
>  tests/qemu-iotests/026             | 17 +++++++++++++++++
>  tests/qemu-iotests/026.out         |  8 ++++++++
>  tests/qemu-iotests/026.out.nocache | 14 +++++++++++---
>  6 files changed, 50 insertions(+), 3 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [PATCH 2/3] qcow2: Free allocated clusters on write error

[Eric Blake]

On 06/28/2018 10:49 AM, Eric Blake wrote:
> On 06/28/2018 10:39 AM, Kevin Wolf wrote:
>> If we managed to allocate the clusters, but then failed to write the
>> data, there's a good chance that we'll still be able to free the
>> clusters again in order to avoid cluster leaks (the refcounts are
>> cached, so even if we can't write them out right now, we may be able to
>> do so when the VM is resumed after a werror=stop/enospc pause).
>>
>> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
>> ---
>>   block/qcow2.h         |  1 +
>>   block/qcow2-cluster.c | 11 +++++++++++
>>   block/qcow2.c         |  2 ++
>>   3 files changed, 14 insertions(+)
> 
> Hmm, I wonder if this interacts poorly with my proposed test addition 
> for HUGE images, which is still pending review:
> https://lists.gnu.org/archive/html/qemu-devel/2018-06/msg05488.html
> https://lists.gnu.org/archive/html/qemu-devel/2018-04/msg04542.html
> 
> (time for me to go testing...)

Phew, my test didn't change (but I will go ahead and repost a v7 to fix 
the minor conflict in iotest numbering).

And that means I can also add for this series:

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH 0/3] qcow2: Fix cluster leaks on write error

[Kevin Wolf]

Am 28.06.2018 um 17:39 hat Kevin Wolf geschrieben:
> Kevin Wolf (3):
>   qemu-iotests: Update 026.out.nocache reference output
>   qcow2: Free allocated clusters on write error
>   qemu-iotests: Test qcow2 not leaking clusters on write error

Thanks for the review, applied to the block branch.

Kevin