[Qemu-devel] [PATCH] qcow2: Flush metadata during read-only reopen

[Qemu-devel] [PATCH] qcow2: Flush metadata during read-only reopen

[Kevin Wolf]

If lazy refcounts are enabled for a backing file, committing to this
backing file may leave it in a dirty state even if the commit succeeds.
The reason is that the bdrv_flush() call in bdrv_commit() doesn't flush
refcount updates with lazy refcounts enabled, and qcow2_reopen_prepare()
doesn't take care to flush metadata.

In order to fix this, this patch also fixes qcow2_mark_clean(), which
contains another ineffective bdrv_flush() call beause lazy refcounts are
disabled only afterwards. All existing callers of qcow2_mark_clean()
either don't modify refcounts or already flush manually, so that this
fixes only a latent, but not yet actually triggerable bug.

Another instance of the same problem is live snapshots. Again, a real
corruption is prevented by an explicit flush for non-read-only images in
external_snapshot_prepare(), but images using lazy refcounts stay dirty.

Cc: qemu-stable@nongnu.org
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/qcow2.c              | 25 +++++++++++++++++++++----
 tests/qemu-iotests/039     | 20 ++++++++++++++++++++
 tests/qemu-iotests/039.out | 11 +++++++++++
 3 files changed, 52 insertions(+), 4 deletions(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 2bb9980..9847f07 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -269,12 +269,15 @@ static int qcow2_mark_clean(BlockDriverState *bs)
     BDRVQcowState *s = bs->opaque;
 
     if (s->incompatible_features & QCOW2_INCOMPAT_DIRTY) {
-        int ret = bdrv_flush(bs);
+        int ret;
+
+        s->incompatible_features &= ~QCOW2_INCOMPAT_DIRTY;
+
+        ret = bdrv_flush(bs);
         if (ret < 0) {
             return ret;
         }
 
-        s->incompatible_features &= ~QCOW2_INCOMPAT_DIRTY;
         return qcow2_update_header(bs);
     }
     return 0;
@@ -900,11 +903,25 @@ static int qcow2_set_key(BlockDriverState *bs, const char *key)
     return 0;
 }
 
-/* We have nothing to do for QCOW2 reopen, stubs just return
- * success */
+/* We have no actual commit/abort logic for qcow2, but we need to write out any
+ * unwritten data if we reopen read-only. */
 static int qcow2_reopen_prepare(BDRVReopenState *state,
                                 BlockReopenQueue *queue, Error **errp)
 {
+    int ret;
+
+    if ((state->flags & BDRV_O_RDWR) == 0) {
+        ret = bdrv_flush(state->bs);
+        if (ret < 0) {
+            return ret;
+        }
+
+        ret = qcow2_mark_clean(state->bs);
+        if (ret < 0) {
+            return ret;
+        }
+    }
+
     return 0;
 }
 
diff --git a/tests/qemu-iotests/039 b/tests/qemu-iotests/039
index 9b355c0..b9cbe99 100755
--- a/tests/qemu-iotests/039
+++ b/tests/qemu-iotests/039
@@ -131,6 +131,26 @@ ulimit -c "$old_ulimit"
 ./qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
 _check_test_img
 
+echo
+echo "== Committing to a backing file with lazy_refcounts=on =="
+
+IMGOPTS="compat=1.1,lazy_refcounts=on"
+TEST_IMG="$TEST_IMG".base _make_test_img $size
+
+IMGOPTS="compat=1.1,lazy_refcounts=on,backing_file=$TEST_IMG.base"
+_make_test_img $size
+
+$QEMU_IO -c "write 0 512" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG commit "$TEST_IMG"
+
+# The dirty bit must not be set
+./qcow2.py "$TEST_IMG" dump-header | grep incompatible_features
+./qcow2.py "$TEST_IMG".base dump-header | grep incompatible_features
+
+_check_test_img
+TEST_IMG="$TEST_IMG".base _check_test_img
+
+
 # success, all done
 echo "*** done"
 rm -f $seq.full
diff --git a/tests/qemu-iotests/039.out b/tests/qemu-iotests/039.out
index 077fa64..fb31ae0 100644
--- a/tests/qemu-iotests/039.out
+++ b/tests/qemu-iotests/039.out
@@ -54,4 +54,15 @@ wrote 512/512 bytes at offset 0
 512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 incompatible_features     0x0
 No errors were found on the image.
+
+== Committing to a backing file with lazy_refcounts=on ==
+Formatting 'TEST_DIR/t.IMGFMT.base', fmt=IMGFMT size=134217728 
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 backing_file='TEST_DIR/t.IMGFMT.base' 
+wrote 512/512 bytes at offset 0
+512 bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+Image committed.
+incompatible_features     0x0
+incompatible_features     0x0
+No errors were found on the image.
+No errors were found on the image.
 *** done
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] qcow2: Flush metadata during read-only reopen

[Stefan Hajnoczi]

On Thu, Apr 03, 2014 at 03:46:32PM +0200, Kevin Wolf wrote:
> If lazy refcounts are enabled for a backing file, committing to this
> backing file may leave it in a dirty state even if the commit succeeds.
> The reason is that the bdrv_flush() call in bdrv_commit() doesn't flush
> refcount updates with lazy refcounts enabled, and qcow2_reopen_prepare()
> doesn't take care to flush metadata.
> 
> In order to fix this, this patch also fixes qcow2_mark_clean(), which
> contains another ineffective bdrv_flush() call beause lazy refcounts are
> disabled only afterwards. All existing callers of qcow2_mark_clean()
> either don't modify refcounts or already flush manually, so that this
> fixes only a latent, but not yet actually triggerable bug.
> 
> Another instance of the same problem is live snapshots. Again, a real
> corruption is prevented by an explicit flush for non-read-only images in
> external_snapshot_prepare(), but images using lazy refcounts stay dirty.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/qcow2.c              | 25 +++++++++++++++++++++----
>  tests/qemu-iotests/039     | 20 ++++++++++++++++++++
>  tests/qemu-iotests/039.out | 11 +++++++++++
>  3 files changed, 52 insertions(+), 4 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>