From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 766141FF0E4 for ; Tue, 14 Jul 2026 16:26:49 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 5961E214E6; Tue, 14 Jul 2026 16:26:37 +0200 (CEST) From: Fiona Ebner To: pve-devel@lists.proxmox.com Subject: [PATCH qemu 1/5] async snapshot: improve disk consistency by only calling vm_start() in savevm-end handler Date: Tue, 14 Jul 2026 16:25:57 +0200 Message-ID: <20260714142629.277917-2-f.ebner@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260714142629.277917-1-f.ebner@proxmox.com> References: <20260714142629.277917-1-f.ebner@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784039175738 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.269 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: RKSRL63SACJYUF4R6THIFDDKL5DSAAY2 X-Message-ID-Hash: RKSRL63SACJYUF4R6THIFDDKL5DSAAY2 X-MailFrom: f.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: A snapshot with VM state is created as follows in Proxmox VE: 1. qemu-server calls savevm-start and waits for query-savevm to return status 'completed'. 2. process_savevm_co() runs until state saving can converge. 3. blockdevs in IO threads are flushed async as an optimization. 4. The process_savevm_finalize() bottom half is scheduled and runs. 5. The VM is stopped, which drains and flushes all IO. 6. The state is set to 'completed'. 7a. The VM is started if it was running before. 7b. qemu-server sees status 'completed'. 8. qemu-server creates all the disk snapshots. 9. qemu-server calls savevm-end. Since vm_start() is the very next command after setting the state to 'completed', in practice, step 7a will always happen before step 8, which is problematic for consistency. The VM should only be started again after the disk snapshots are taken. The functionality for restoring the run state is already in place in qmp_savevm_end(), so rely on that. Signed-off-by: Fiona Ebner --- ...olerate-known-seSparse-reserved1-bit.patch | 8 +++--- ...async-for-background-state-snapshots.patch | 27 ++++++++++++------- ...add-optional-buffer-size-to-QEMUFile.patch | 6 ++--- ...se-migration-blocker-check-for-snaps.patch | 8 +++--- 4 files changed, 30 insertions(+), 19 deletions(-) diff --git a/debian/patches/extra/0004-block-vmdk-tolerate-known-seSparse-reserved1-bit.patch b/debian/patches/extra/0004-block-vmdk-tolerate-known-seSparse-reserved1-bit.patch index bd04ec222e..1ebe382bd7 100644 --- a/debian/patches/extra/0004-block-vmdk-tolerate-known-seSparse-reserved1-bit.patch +++ b/debian/patches/extra/0004-block-vmdk-tolerate-known-seSparse-reserved1-bit.patch @@ -32,10 +32,12 @@ unknown reserved1 bit or a set reserved2 is still refused. Signed-off-by: Thomas Lamprecht --- - block/vmdk.c | 3 +- - .../tests/vmdk-sesparse-reserved | 105 +++++++++++++++++ - .../tests/vmdk-sesparse-reserved.out | 14 +++ + block/vmdk.c | 3 +- + .../qemu-iotests/tests/vmdk-sesparse-reserved | 105 ++++++++++++++++++ + .../tests/vmdk-sesparse-reserved.out | 14 +++ 3 files changed, 121 insertions(+), 1 deletion(-) + create mode 100755 tests/qemu-iotests/tests/vmdk-sesparse-reserved + create mode 100644 tests/qemu-iotests/tests/vmdk-sesparse-reserved.out diff --git a/block/vmdk.c b/block/vmdk.c index cd8b4ec7c8..7f8fdf0727 100644 diff --git a/debian/patches/pve/0017-PVE-add-savevm-async-for-background-state-snapshots.patch b/debian/patches/pve/0017-PVE-add-savevm-async-for-background-state-snapshots.patch index 192f3abf46..0e1be00f71 100644 --- a/debian/patches/pve/0017-PVE-add-savevm-async-for-background-state-snapshots.patch +++ b/debian/patches/pve/0017-PVE-add-savevm-async-for-background-state-snapshots.patch @@ -35,7 +35,8 @@ Signed-off-by: Stefan Reiter improve runstate preservation use dedicated iothread for state file to avoid deadlock, bug #6262 add parameter to skip vm start to be used for hibernation - rebase for 11.0.0] + rebase for 11.0.0 + defer vm_start() to qmp_savevm_end() in case of success] Signed-off-by: Fiona Ebner --- hmp-commands-info.hx | 13 + @@ -43,13 +44,13 @@ Signed-off-by: Fiona Ebner include/migration/snapshot.h | 2 + include/monitor/hmp.h | 3 + migration/meson.build | 1 + - migration/savevm-async.c | 597 +++++++++++++++++++++++++++++++++++ + migration/savevm-async.c | 605 +++++++++++++++++++++++++++++++++++ monitor/hmp-cmds.c | 38 +++ qapi/migration.json | 34 ++ qapi/misc.json | 25 ++ qemu-options.hx | 12 + system/vl.c | 10 + - 11 files changed, 752 insertions(+) + 11 files changed, 760 insertions(+) create mode 100644 migration/savevm-async.c diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx @@ -147,10 +148,10 @@ index 0222d5ea6e..90d62d5723 100644 ), gnutls, zlib) diff --git a/migration/savevm-async.c b/migration/savevm-async.c new file mode 100644 -index 0000000000..2a860f11a1 +index 0000000000..6356557e52 --- /dev/null +++ b/migration/savevm-async.c -@@ -0,0 +1,597 @@ +@@ -0,0 +1,605 @@ +#include "qemu/osdep.h" +#include "migration/channel-savevm-async.h" +#include "migration/migration.h" @@ -385,11 +386,19 @@ index 0000000000..2a860f11a1 + save_snapshot_error("process_savevm_cleanup: invalid state: %d", + snap_state.state); + } -+ if (snap_state.vm_needs_start) { -+ if (!should_skip_vm_start()) { -+ vm_start(); ++ ++ /* ++ * In case of success, qemu-server still needs to create the disk snapshots ++ * while the VM is stopped. qmp_savevm_end() will restore the run state ++ * later. ++ */ ++ if (snap_state.state != SAVE_STATE_COMPLETED) { ++ if (snap_state.vm_needs_start) { ++ if (!should_skip_vm_start()) { ++ vm_start(); ++ } ++ snap_state.vm_needs_start = false; + } -+ snap_state.vm_needs_start = false; + } + + DPRINTF("timing: process_savevm_finalize (full) took %ld ms\n", diff --git a/debian/patches/pve/0018-PVE-add-optional-buffer-size-to-QEMUFile.patch b/debian/patches/pve/0018-PVE-add-optional-buffer-size-to-QEMUFile.patch index ab8aac16a5..9b615b0752 100644 --- a/debian/patches/pve/0018-PVE-add-optional-buffer-size-to-QEMUFile.patch +++ b/debian/patches/pve/0018-PVE-add-optional-buffer-size-to-QEMUFile.patch @@ -184,10 +184,10 @@ index a390554208..eda093b16a 100644 G_DEFINE_AUTOPTR_CLEANUP_FUNC(QEMUFile, qemu_fclose) diff --git a/migration/savevm-async.c b/migration/savevm-async.c -index 2a860f11a1..f5a4819e27 100644 +index 6356557e52..1902a842c1 100644 --- a/migration/savevm-async.c +++ b/migration/savevm-async.c -@@ -418,7 +418,7 @@ void qmp_savevm_start(const char *statefile, bool has_skip_vm_start, +@@ -426,7 +426,7 @@ void qmp_savevm_start(const char *statefile, bool has_skip_vm_start, QIOChannel *ioc = QIO_CHANNEL(qio_channel_savevm_async_new(snap_state.target, &snap_state.bs_pos)); @@ -196,7 +196,7 @@ index 2a860f11a1..f5a4819e27 100644 if (!snap_state.file) { error_setg(errp, "failed to open '%s'", statefile); -@@ -555,7 +555,8 @@ int load_snapshot_from_blockdev(const char *filename, Error **errp) +@@ -563,7 +563,8 @@ int load_snapshot_from_blockdev(const char *filename, Error **errp) bdrv_op_block_all(bs, blocker); /* restore the VM state */ diff --git a/debian/patches/pve/0046-savevm-async-reuse-migration-blocker-check-for-snaps.patch b/debian/patches/pve/0046-savevm-async-reuse-migration-blocker-check-for-snaps.patch index 0c4cef7f09..30ed1eafdb 100644 --- a/debian/patches/pve/0046-savevm-async-reuse-migration-blocker-check-for-snaps.patch +++ b/debian/patches/pve/0046-savevm-async-reuse-migration-blocker-check-for-snaps.patch @@ -61,10 +61,10 @@ Message-ID: <20250618102531.57444-1-f.ebner@proxmox.com> 5 files changed, 29 insertions(+), 4 deletions(-) diff --git a/block/vmdk.c b/block/vmdk.c -index cd8b4ec7c8..1f64ab1766 100644 +index 7f8fdf0727..5385a09fe5 100644 --- a/block/vmdk.c +++ b/block/vmdk.c -@@ -1404,9 +1404,7 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags, +@@ -1405,9 +1405,7 @@ static int vmdk_open(BlockDriverState *bs, QDict *options, int flags, qemu_co_mutex_init(&s->lock); /* Disable migration when VMDK images are used */ @@ -136,10 +136,10 @@ index b6888daced..80eb0dcd1f 100644 bool migration_in_postcopy(void); bool migration_postcopy_is_alive(MigrationStatus state); diff --git a/migration/savevm-async.c b/migration/savevm-async.c -index f5a4819e27..41376406eb 100644 +index 1902a842c1..150b2dac15 100644 --- a/migration/savevm-async.c +++ b/migration/savevm-async.c -@@ -384,7 +384,7 @@ void qmp_savevm_start(const char *statefile, bool has_skip_vm_start, +@@ -392,7 +392,7 @@ void qmp_savevm_start(const char *statefile, bool has_skip_vm_start, return; } -- 2.47.3