[Qemu-devel] [PATCH v2 0/2] qcow2: Undo leaked allocations in co_writev

[Qemu-devel] [PATCH v2 0/2] qcow2: Undo leaked allocations in co_writev

[Max Reitz]

If a write request on a qcow2 image spans more than one L2 table,
qcow2_alloc_cluster_offset cannot allocate the required clusters in a
single operation. This results in leaks, if a subsequent (atomic)
allocation in that function fails, because qcow2_co_writev does not undo
unused cluster allocations.

This series implements that deallocation and provides a test for it.

v2:
 - patch 1: added a comment about the significance of future changes at
   a certain point in qcow2_co_writev
 - patch 1: changed QCOW2_DISCARD_ALWAYS to QCOW2_DISCARD_OTHER

Max Reitz (2):
  qcow2: Undo leaked allocations in co_writev
  qemu-iotests: Extend test 026

 block/qcow2.c                      | 15 +++++++++++++++
 tests/qemu-iotests/026             | 31 +++++++++++++++++++++++++++++++
 tests/qemu-iotests/026.out         |  8 ++++++++
 tests/qemu-iotests/026.out.nocache |  8 ++++++++
 4 files changed, 62 insertions(+)

-- 
1.8.3.1

[Qemu-devel] [PATCH v2 1/2] qcow2: Undo leaked allocations in co_writev

[Max Reitz]

If the write request spans more than one L2 table,
qcow2_alloc_cluster_offset cannot handle the required allocations
atomically. This results in leaks if it allocated new clusters in any
but the last L2 table touched and an error occurs in qcow2_co_writev
before having established the L2 link. These non-atomic allocations
were, however, indeed successful and are therefore given to the caller
in the L2Meta list.

If an error occurs in qcow2_co_writev and the L2Meta list is unwound,
all its remaining entries are clusters whose L2 links were not yet
established. Thus, all allocations in that list should be undone.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/block/qcow2.c b/block/qcow2.c
index 13e34f0..69d8270 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1078,6 +1078,14 @@ static coroutine_fn int qcow2_co_writev(BlockDriverState *bs,
                 goto fail;
             }
 
+            /* WARNING: If adding a "goto fail" anywhere from here until the
+             * current QCowL2Meta element is removed from the l2meta list (that
+             * is, until the end of this loop), mind the qcow2_free_clusters()
+             * call after the fail label. Always make sure that every list
+             * element with a non-zero nb_clusters value remaining when reaching
+             * the while loop after that label indeed corresponds to a leaked
+             * allocation which has to be freed. */
+
             /* Take the request off the list of running requests */
             if (l2meta->nb_clusters != 0) {
                 QLIST_REMOVE(l2meta, next_in_flight);
@@ -1103,6 +1111,13 @@ fail:
     while (l2meta != NULL) {
         QCowL2Meta *next;
 
+        /* Undo all leaked allocations */
+        if (l2meta->nb_clusters != 0) {
+            qcow2_free_clusters(bs, l2meta->alloc_offset,
+                                l2meta->nb_clusters << s->cluster_bits,
+                                QCOW2_DISCARD_OTHER);
+        }
+
         if (l2meta->nb_clusters != 0) {
             QLIST_REMOVE(l2meta, next_in_flight);
         }
-- 
1.8.3.1

[Qemu-devel] [PATCH v2 2/2] qemu-iotests: Extend test 026

[Max Reitz]

Extend test case 026 by an aio_write fail test, which should not result
in any leaked clusters.

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/026             | 31 +++++++++++++++++++++++++++++++
 tests/qemu-iotests/026.out         |  8 ++++++++
 tests/qemu-iotests/026.out.nocache |  8 ++++++++
 3 files changed, 47 insertions(+)

diff --git a/tests/qemu-iotests/026 b/tests/qemu-iotests/026
index ebe29d0..a9dfe36 100755
--- a/tests/qemu-iotests/026
+++ b/tests/qemu-iotests/026
@@ -193,6 +193,37 @@ done
 done
 done
 
+echo
+echo === Write leak test ===
+echo
+CLUSTER_SIZE=512
+
+for event in write_aio; do
+for errno in 28; do
+for imm in off; do
+for once in on; do
+
+cat > "$TEST_DIR/blkdebug.conf" <<EOF
+[inject-error]
+event = "$event"
+errno = "$errno"
+immediately = "$imm"
+once = "$once"
+EOF
+
+_make_test_img 1G
+
+echo
+echo "Event: $event; errno: $errno; imm: $imm; once: $once"
+$QEMU_IO -c "write 0 128k" "$BLKDBG_TEST_IMG" | _filter_qemu_io
+
+_check_test_img 2>&1 | grep -v "refcount=1 reference=0"
+
+done
+done
+done
+done
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/026.out b/tests/qemu-iotests/026.out
index 1504579..c94daca 100644
--- a/tests/qemu-iotests/026.out
+++ b/tests/qemu-iotests/026.out
@@ -599,4 +599,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.
+
+=== Write leak test ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824 
+
+Event: write_aio; errno: 28; imm: off; once: on
+write failed: No space left on device
+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 c9d242e..962bb71 100644
--- a/tests/qemu-iotests/026.out.nocache
+++ b/tests/qemu-iotests/026.out.nocache
@@ -607,4 +607,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.
+
+=== Write leak test ===
+
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=1073741824 
+
+Event: write_aio; errno: 28; imm: off; once: on
+write failed: No space left on device
+No errors were found on the image.
 *** done
-- 
1.8.3.1