[PATCH 0/3] block: Make snapshots fail gracefully on inactive images

[PATCH 0/3] block: Make snapshots fail gracefully on inactive images

[Kevin Wolf]

Kevin Wolf (3):
  block: Fix crash when loading snapshot on inactive node
  vl: Improve error message for conflicting -incoming and -loadvm
  iotests: Basic tests for internal snapshots

 block/snapshot.c                              |   4 +-
 system/vl.c                                   |   4 +
 .../tests/qcow2-internal-snapshots            | 170 ++++++++++++++++++
 .../tests/qcow2-internal-snapshots.out        | 107 +++++++++++
 4 files changed, 284 insertions(+), 1 deletion(-)
 create mode 100755 tests/qemu-iotests/tests/qcow2-internal-snapshots
 create mode 100644 tests/qemu-iotests/tests/qcow2-internal-snapshots.out

-- 
2.43.0

[PATCH 1/3] block: Fix crash when loading snapshot on inactive node

[Kevin Wolf]

bdrv_is_read_only() only checks if the node is configured to be
read-only eventually, but even if it returns false, writing to the node
may not be permitted at the moment (because it's inactive).

bdrv_is_writable() checks that the node can be written to right now, and
this is what the snapshot operations really need.

Change bdrv_can_snapshot() to use bdrv_is_writable() to fix crashes like
the following:

$ ./qemu-system-x86_64 -hda /tmp/test.qcow2 -loadvm foo -incoming defer
qemu-system-x86_64: ../block/io.c:1990: int bdrv_co_write_req_prepare(BdrvChild *, int64_t, int64_t, BdrvTrackedRequest *, int): Assertion `!(bs->open_flags & BDRV_O_INACTIVE)' failed.

The resulting error message after this patch isn't perfect yet, but at
least it doesn't crash any more:

$ ./qemu-system-x86_64 -hda /tmp/test.qcow2 -loadvm foo -incoming defer
qemu-system-x86_64: Device 'ide0-hd0' is writable but does not support snapshots

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/snapshot.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/block/snapshot.c b/block/snapshot.c
index ec8cf4810b..c4d40e80dd 100644
--- a/block/snapshot.c
+++ b/block/snapshot.c
@@ -196,8 +196,10 @@ bdrv_snapshot_fallback(BlockDriverState *bs)
 int bdrv_can_snapshot(BlockDriverState *bs)
 {
     BlockDriver *drv = bs->drv;
+
     GLOBAL_STATE_CODE();
-    if (!drv || !bdrv_is_inserted(bs) || bdrv_is_read_only(bs)) {
+
+    if (!drv || !bdrv_is_inserted(bs) || !bdrv_is_writable(bs)) {
         return 0;
     }
 
-- 
2.43.0

[PATCH 2/3] vl: Improve error message for conflicting -incoming and -loadvm

[Kevin Wolf]

Currently, the conflict between -incoming and -loadvm is only detected
when loading the snapshot fails because the image is still inactive for
the incoming migration. This results in a suboptimal error message:

$ ./qemu-system-x86_64 -hda /tmp/test.qcow2 -loadvm foo -incoming defer
qemu-system-x86_64: Device 'ide0-hd0' is writable but does not support snapshots

Catch the situation already in qemu_validate_options() to improve the
message:

$ ./qemu-system-x86_64 -hda /tmp/test.qcow2 -loadvm foo -incoming defer
qemu-system-x86_64: 'incoming' and 'loadvm' options are mutually exclusive

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 system/vl.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/system/vl.c b/system/vl.c
index 2bcd9efb9a..6b87bfa32c 100644
--- a/system/vl.c
+++ b/system/vl.c
@@ -2426,6 +2426,10 @@ static void qemu_validate_options(const QDict *machine_opts)
           }
     }
 
+    if (loadvm && incoming) {
+        error_report("'incoming' and 'loadvm' options are mutually exclusive");
+        exit(EXIT_FAILURE);
+    }
     if (loadvm && preconfig_requested) {
         error_report("'preconfig' and 'loadvm' options are "
                      "mutually exclusive");
-- 
2.43.0

[PATCH 3/3] iotests: Basic tests for internal snapshots

[Kevin Wolf]

We have a few test cases that include tests for corner case aspects of
internal snapshots, but nothing that tests that they actually function
as snapshots or that involves deleting a snapshot. Add a test for this
kind of basic internal snapshot functionality.

The error cases include a regression test for the crash we just fixed
with snapshot operations on inactive images.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 .../tests/qcow2-internal-snapshots            | 170 ++++++++++++++++++
 .../tests/qcow2-internal-snapshots.out        | 107 +++++++++++
 2 files changed, 277 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/qcow2-internal-snapshots
 create mode 100644 tests/qemu-iotests/tests/qcow2-internal-snapshots.out

diff --git a/tests/qemu-iotests/tests/qcow2-internal-snapshots b/tests/qemu-iotests/tests/qcow2-internal-snapshots
new file mode 100755
index 0000000000..36523aba06
--- /dev/null
+++ b/tests/qemu-iotests/tests/qcow2-internal-snapshots
@@ -0,0 +1,170 @@
+#!/usr/bin/env bash
+# group: rw quick
+#
+# Test case for internal snapshots in qcow2
+#
+# Copyright (C) 2023 Red Hat, Inc.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+# creator
+owner=kwolf@redhat.com
+
+seq="$(basename $0)"
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+
+_cleanup()
+{
+	_cleanup_test_img
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ../common.rc
+. ../common.filter
+
+# This tests qcow2-specific low-level functionality
+_supported_fmt qcow2
+_supported_proto generic
+# Internal snapshots are (currently) impossible with refcount_bits=1,
+# and generally impossible with external data files
+_unsupported_imgopts 'compat=0.10' 'refcount_bits=1[^0-9]' data_file
+
+IMG_SIZE=64M
+
+_qemu()
+{
+    $QEMU -no-shutdown -nographic -monitor stdio -serial none \
+          -blockdev file,filename="$TEST_IMG",node-name=disk0-file \
+          -blockdev "$IMGFMT",file=disk0-file,node-name=disk0 \
+          -object iothread,id=iothread0 \
+          -device virtio-scsi,iothread=iothread0 \
+          -device scsi-hd,drive=disk0,share-rw=on \
+          "$@" 2>&1 |\
+    _filter_qemu | _filter_hmp | _filter_qemu_io
+}
+
+_make_test_img $IMG_SIZE
+
+echo
+echo "=== Write some data, take a snapshot and overwrite part of it ==="
+echo
+
+{
+    echo 'qemu-io disk0 "write -P0x11 0 1M"'
+    # Give qemu some time to boot before saving the VM state
+    sleep 0.5
+    echo "savevm snap0"
+    echo 'qemu-io disk0 "write -P0x22 0 512k"'
+    echo "quit"
+} | _qemu
+
+echo
+$QEMU_IMG snapshot -l "$TEST_IMG" | _filter_date | _filter_vmstate_size
+_check_test_img
+
+echo
+echo "=== Verify that loading the snapshot reverts to the old content ==="
+echo
+
+{
+    # -loadvm reverted the write from the previous QEMU instance
+    echo 'qemu-io disk0 "read -P0x11 0 1M"'
+
+    # Verify that it works without restarting QEMU, too
+    echo 'qemu-io disk0 "write -P0x33 512k 512k"'
+    echo "loadvm snap0"
+    echo 'qemu-io disk0 "read -P0x11 0 1M"'
+
+    # Verify COW by writing a partial cluster
+    echo 'qemu-io disk0 "write -P0x33 63k 2k"'
+    echo 'qemu-io disk0 "read -P0x11 0 63k"'
+    echo 'qemu-io disk0 "read -P0x33 63k 2k"'
+    echo 'qemu-io disk0 "read -P0x11 65k 63k"'
+
+    # Take a second snapshot
+    echo "savevm snap1"
+
+    echo "quit"
+} | _qemu -loadvm snap0
+
+echo
+$QEMU_IMG snapshot -l "$TEST_IMG" | _filter_date | _filter_vmstate_size
+_check_test_img
+
+echo
+echo "=== qemu-img snapshot can revert to snapshots ==="
+echo
+
+$QEMU_IMG snapshot -a snap0 "$TEST_IMG"
+$QEMU_IO -c "read -P0x11 0 1M" "$TEST_IMG" | _filter_qemu_io
+$QEMU_IMG snapshot -a snap1 "$TEST_IMG"
+$QEMU_IO \
+    -c "read -P0x11 0 63k" \
+    -c "read -P0x33 63k 2k" \
+    -c "read -P0x11 65k 63k" \
+    "$TEST_IMG" | _filter_qemu_io
+
+echo
+echo "=== Deleting snapshots ==="
+echo
+{
+    # The active layer stays unaffected by deleting the snapshot
+    echo "delvm snap1"
+    echo 'qemu-io disk0 "read -P0x11 0 63k"'
+    echo 'qemu-io disk0 "read -P0x33 63k 2k"'
+    echo 'qemu-io disk0 "read -P0x11 65k 63k"'
+
+    echo "quit"
+} | _qemu
+
+
+echo
+$QEMU_IMG snapshot -l "$TEST_IMG" | _filter_date | _filter_vmstate_size
+_check_test_img
+
+echo
+echo "=== Error cases ==="
+echo
+
+# snap1 should not exist any more
+_qemu -loadvm snap1
+
+echo
+{
+    echo "loadvm snap1"
+    echo "quit"
+} | _qemu
+
+# Snapshot operations and inactive images are incompatible
+echo
+_qemu -loadvm snap0 -incoming defer
+{
+    echo "loadvm snap0"
+    echo "delvm snap0"
+    echo "savevm snap1"
+    echo "quit"
+} | _qemu -incoming defer
+
+# -loadvm and -preconfig are incompatible
+echo
+_qemu -loadvm snap0 -preconfig
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/tests/qcow2-internal-snapshots.out b/tests/qemu-iotests/tests/qcow2-internal-snapshots.out
new file mode 100644
index 0000000000..438f535e6a
--- /dev/null
+++ b/tests/qemu-iotests/tests/qcow2-internal-snapshots.out
@@ -0,0 +1,107 @@
+QA output created by qcow2-internal-snapshots
+Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=67108864
+
+=== Write some data, take a snapshot and overwrite part of it ===
+
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) qemu-io disk0 "write -P0x11 0 1M"
+wrote 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+(qemu) savevm snap0
+(qemu) qemu-io disk0 "write -P0x22 0 512k"
+wrote 524288/524288 bytes at offset 0
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+(qemu) quit
+
+Snapshot list:
+ID        TAG               VM SIZE                DATE     VM CLOCK     ICOUNT
+1         snap0                SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
+No errors were found on the image.
+
+=== Verify that loading the snapshot reverts to the old content ===
+
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) qemu-io disk0 "read -P0x11 0 1M"
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+(qemu) qemu-io disk0 "write -P0x33 512k 512k"
+wrote 524288/524288 bytes at offset 524288
+512 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+(qemu) loadvm snap0
+(qemu) qemu-io disk0 "read -P0x11 0 1M"
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+(qemu) qemu-io disk0 "write -P0x33 63k 2k"
+wrote 2048/2048 bytes at offset 64512
+2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+(qemu) qemu-io disk0 "read -P0x11 0 63k"
+read 64512/64512 bytes at offset 0
+63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+(qemu) qemu-io disk0 "read -P0x33 63k 2k"
+read 2048/2048 bytes at offset 64512
+2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+(qemu) qemu-io disk0 "read -P0x11 65k 63k"
+read 64512/64512 bytes at offset 66560
+63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+(qemu) savevm snap1
+(qemu) quit
+
+Snapshot list:
+ID        TAG               VM SIZE                DATE     VM CLOCK     ICOUNT
+1         snap0                SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
+2         snap1                SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
+No errors were found on the image.
+
+=== qemu-img snapshot can revert to snapshots ===
+
+read 1048576/1048576 bytes at offset 0
+1 MiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 64512/64512 bytes at offset 0
+63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 2048/2048 bytes at offset 64512
+2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+read 64512/64512 bytes at offset 66560
+63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+
+=== Deleting snapshots ===
+
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) delvm snap1
+(qemu) qemu-io disk0 "read -P0x11 0 63k"
+read 64512/64512 bytes at offset 0
+63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+(qemu) qemu-io disk0 "read -P0x33 63k 2k"
+read 2048/2048 bytes at offset 64512
+2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+(qemu) qemu-io disk0 "read -P0x11 65k 63k"
+read 64512/64512 bytes at offset 66560
+63 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
+(qemu) quit
+
+Snapshot list:
+ID        TAG               VM SIZE                DATE     VM CLOCK     ICOUNT
+1         snap0                SIZE yyyy-mm-dd hh:mm:ss 00:00:00.000
+No errors were found on the image.
+
+=== Error cases ===
+
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) QEMU_PROG: Snapshot 'snap1' does not exist in one or more devices
+
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) loadvm snap1
+Error: Snapshot 'snap1' does not exist in one or more devices
+(qemu) quit
+
+QEMU_PROG: 'incoming' and 'loadvm' options are mutually exclusive
+QEMU X.Y.Z monitor - type 'help' for more information
+(qemu) loadvm snap0
+Error: Device 'disk0' is writable but does not support snapshots
+(qemu) delvm snap0
+Error: Device 'disk0' is writable but does not support snapshots
+(qemu) savevm snap1
+Error: Device 'disk0' is writable but does not support snapshots
+(qemu) quit
+
+QEMU_PROG: 'preconfig' and 'loadvm' options are mutually exclusive
+*** done
-- 
2.43.0

Re: [PATCH 0/3] block: Make snapshots fail gracefully on inactive images

[Michael Tokarev]

01.12.2023 17:25, Kevin Wolf :
> Kevin Wolf (3):
>    block: Fix crash when loading snapshot on inactive node
>    vl: Improve error message for conflicting -incoming and -loadvm
>    iotests: Basic tests for internal snapshots

This smells like a stable-8.2 material, at least the first change
(tho I'd pick all 3).  Maybe stable-8.1 too.

On the other hand, this is a rarely-used code path.

Thanks,

/mjt

Re: [PATCH 0/3] block: Make snapshots fail gracefully on inactive images

[Kevin Wolf]

Am 22.12.2023 um 20:24 hat Michael Tokarev geschrieben:
> 01.12.2023 17:25, Kevin Wolf :
> > Kevin Wolf (3):
> >    block: Fix crash when loading snapshot on inactive node
> >    vl: Improve error message for conflicting -incoming and -loadvm
> >    iotests: Basic tests for internal snapshots
> 
> This smells like a stable-8.2 material, at least the first change
> (tho I'd pick all 3).  Maybe stable-8.1 too.

Yes, makes sense to me. It was a bit late for 8.2.0, but adding it to a
future stable release is reasonable.

> On the other hand, this is a rarely-used code path.

It is, but if you do hit it, it's still a crash.

Kevin