From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id EF4156B481 for ; Tue, 16 Mar 2021 17:31:04 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E606124F94 for ; Tue, 16 Mar 2021 17:30:34 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 480B724F8C for ; Tue, 16 Mar 2021 17:30:33 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 13AD742654 for ; Tue, 16 Mar 2021 17:30:33 +0100 (CET) From: Stefan Reiter To: pve-devel@lists.proxmox.com Date: Tue, 16 Mar 2021 17:30:22 +0100 Message-Id: <20210316163023.24534-1-s.reiter@proxmox.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.022 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [meson.build, proxmox.com] Subject: [pve-devel] [PATCH pve-qemu 1/2] fix saving and loading dirty bitmaps in snapshots X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 16 Mar 2021 16:31:05 -0000 Saving dirty bitmaps from our savevm-async code didn't work, since we use a coroutine which holds the iothread mutex already (upstream savevm is sync, migration uses a thread). Release the mutex before calling the one function that (according to it's documentation) requires the lock to *not* be held: qemu_savevm_state_pending. Additionally, loading dirty bitmaps requires a call to dirty_bitmap_mig_before_vm_start after "loadvm", which the upstream savevm does explicitly afterwards - do that too. This is exposed via the query-proxmox-support property "pbs-dirty-bitmap-savevm". Signed-off-by: Stefan Reiter --- This fixes a bug reported in the forum[0] as well as enterprise support. This patch contains the actual fix, i.e. what makes it not crash, but we can actually go one step further, and by adding the query-proxmox-support info we can avoid crashes even with older versions (see patch 2/2). As a neat side-effect, this also allows us to fix dirty bitmaps for snapshots and hibernate/resume, so that's a plus :) [0] https://forum.proxmox.com/threads/query-savevm.85719/ ...async-for-background-state-snapshots.patch | 15 +++++++++++---- ...add-optional-buffer-size-to-QEMUFile.patch | 6 +++--- ...dd-query_proxmox_support-QMP-command.patch | 19 ++++++++++++------- ...E-add-query-pbs-bitmap-info-QMP-call.patch | 17 ++++++++++------- ...-transaction-to-synchronize-job-stat.patch | 2 +- ...-block-on-finishing-and-cleanup-crea.patch | 4 ++-- ...igrate-dirty-bitmap-state-via-savevm.patch | 18 ++++++++++-------- ...routine-QMP-for-backup-cancel_backup.patch | 4 ++-- .../pve/0043-PBS-add-master-key-support.patch | 16 ++++++++-------- 9 files changed, 59 insertions(+), 42 deletions(-) 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 e70e69a..3a58139 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 @@ -29,13 +29,13 @@ Signed-off-by: Stefan Reiter include/migration/snapshot.h | 1 + include/monitor/hmp.h | 5 + migration/meson.build | 1 + - migration/savevm-async.c | 591 +++++++++++++++++++++++++++++++++++ + migration/savevm-async.c | 598 +++++++++++++++++++++++++++++++++++ monitor/hmp-cmds.c | 57 ++++ qapi/migration.json | 34 ++ qapi/misc.json | 32 ++ qemu-options.hx | 12 + softmmu/vl.c | 10 + - 11 files changed, 789 insertions(+) + 11 files changed, 796 insertions(+) create mode 100644 migration/savevm-async.c diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx @@ -151,10 +151,10 @@ index 980e37865c..e62b79b60f 100644 )) diff --git a/migration/savevm-async.c b/migration/savevm-async.c new file mode 100644 -index 0000000000..4e345c1a7d +index 0000000000..593a619088 --- /dev/null +++ b/migration/savevm-async.c -@@ -0,0 +1,591 @@ +@@ -0,0 +1,598 @@ +#include "qemu/osdep.h" +#include "migration/migration.h" +#include "migration/savevm.h" @@ -457,7 +457,11 @@ index 0000000000..4e345c1a7d + while (snap_state.state == SAVE_STATE_ACTIVE) { + uint64_t pending_size, pend_precopy, pend_compatible, pend_postcopy; + ++ /* pending is expected to be called without iothread lock */ ++ qemu_mutex_unlock_iothread(); + qemu_savevm_state_pending(snap_state.file, 0, &pend_precopy, &pend_compatible, &pend_postcopy); ++ qemu_mutex_lock_iothread(); ++ + pending_size = pend_precopy + pend_compatible + pend_postcopy; + + maxlen = blk_getlength(snap_state.target) - 30*1024*1024; @@ -729,6 +733,9 @@ index 0000000000..4e345c1a7d + qemu_system_reset(SHUTDOWN_CAUSE_NONE); + ret = qemu_loadvm_state(f); + ++ /* dirty bitmap migration has a special case we need to trigger manually */ ++ dirty_bitmap_mig_before_vm_start(); ++ + qemu_fclose(f); + migration_incoming_state_destroy(); + if (ret < 0) { 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 d3e7b73..d730a72 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 @@ -164,10 +164,10 @@ index a9b6d6ccb7..8752d27c74 100644 int qemu_get_fd(QEMUFile *f); int qemu_fclose(QEMUFile *f); diff --git a/migration/savevm-async.c b/migration/savevm-async.c -index 4e345c1a7d..8a17ec1f74 100644 +index 593a619088..cc2552d977 100644 --- a/migration/savevm-async.c +++ b/migration/savevm-async.c -@@ -414,7 +414,7 @@ void qmp_savevm_start(bool has_statefile, const char *statefile, Error **errp) +@@ -418,7 +418,7 @@ void qmp_savevm_start(bool has_statefile, const char *statefile, Error **errp) goto restart; } @@ -176,7 +176,7 @@ index 4e345c1a7d..8a17ec1f74 100644 if (!snap_state.file) { error_set(errp, ERROR_CLASS_GENERIC_ERROR, "failed to open '%s'", statefile); -@@ -563,7 +563,7 @@ int load_snapshot_from_blockdev(const char *filename, Error **errp) +@@ -567,7 +567,7 @@ int load_snapshot_from_blockdev(const char *filename, Error **errp) blk_op_block_all(be, blocker); /* restore the VM state */ diff --git a/debian/patches/pve/0033-PVE-add-query_proxmox_support-QMP-command.patch b/debian/patches/pve/0033-PVE-add-query_proxmox_support-QMP-command.patch index 5697adf..18cc56e 100644 --- a/debian/patches/pve/0033-PVE-add-query_proxmox_support-QMP-command.patch +++ b/debian/patches/pve/0033-PVE-add-query_proxmox_support-QMP-command.patch @@ -11,15 +11,15 @@ Signed-off-by: Thomas Lamprecht [PVE: query-proxmox-support: include library version] Signed-off-by: Stefan Reiter --- - pve-backup.c | 8 ++++++++ - qapi/block-core.json | 25 +++++++++++++++++++++++++ - 2 files changed, 33 insertions(+) + pve-backup.c | 9 +++++++++ + qapi/block-core.json | 29 +++++++++++++++++++++++++++++ + 2 files changed, 38 insertions(+) diff --git a/pve-backup.c b/pve-backup.c -index b8182aaf89..40c2697b37 100644 +index b8182aaf89..98e79552ef 100644 --- a/pve-backup.c +++ b/pve-backup.c -@@ -1073,3 +1073,11 @@ BackupStatus *qmp_query_backup(Error **errp) +@@ -1073,3 +1073,12 @@ BackupStatus *qmp_query_backup(Error **errp) return info; } @@ -29,13 +29,14 @@ index b8182aaf89..40c2697b37 100644 + ProxmoxSupportStatus *ret = g_malloc0(sizeof(*ret)); + ret->pbs_library_version = g_strdup(proxmox_backup_qemu_version()); + ret->pbs_dirty_bitmap = true; ++ ret->pbs_dirty_bitmap_savevm = true; + return ret; +} diff --git a/qapi/block-core.json b/qapi/block-core.json -index 553112d998..e0a0a60354 100644 +index 553112d998..f3608390c4 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json -@@ -868,6 +868,31 @@ +@@ -868,6 +868,35 @@ ## { 'command': 'backup-cancel' } @@ -47,11 +48,15 @@ index 553112d998..e0a0a60354 100644 +# @pbs-dirty-bitmap: True if dirty-bitmap-incremental backups to PBS are +# supported. +# ++# @pbs-dirty-bitmap-savevm: True if 'dirty-bitmaps' migration capability can ++# safely be set for savevm-async. ++# +# @pbs-library-version: Running version of libproxmox-backup-qemu0 library. +# +## +{ 'struct': 'ProxmoxSupportStatus', + 'data': { 'pbs-dirty-bitmap': 'bool', ++ 'pbs-dirty-bitmap-savevm': 'bool', + 'pbs-library-version': 'str' } } + +## diff --git a/debian/patches/pve/0034-PVE-add-query-pbs-bitmap-info-QMP-call.patch b/debian/patches/pve/0034-PVE-add-query-pbs-bitmap-info-QMP-call.patch index 828bf4e..f0ca311 100644 --- a/debian/patches/pve/0034-PVE-add-query-pbs-bitmap-info-QMP-call.patch +++ b/debian/patches/pve/0034-PVE-add-query-pbs-bitmap-info-QMP-call.patch @@ -68,7 +68,7 @@ index 604026bb37..95f4e7f5c1 100644 info->zero_bytes, zero_per); diff --git a/pve-backup.c b/pve-backup.c -index 40c2697b37..1e7b92a950 100644 +index 98e79552ef..8305105fd5 100644 --- a/pve-backup.c +++ b/pve-backup.c @@ -46,6 +46,7 @@ static struct PVEBackupState { @@ -314,7 +314,7 @@ index 40c2697b37..1e7b92a950 100644 err: l = di_list; -@@ -1074,10 +1100,41 @@ BackupStatus *qmp_query_backup(Error **errp) +@@ -1074,11 +1100,42 @@ BackupStatus *qmp_query_backup(Error **errp) return info; } @@ -353,29 +353,32 @@ index 40c2697b37..1e7b92a950 100644 ProxmoxSupportStatus *ret = g_malloc0(sizeof(*ret)); ret->pbs_library_version = g_strdup(proxmox_backup_qemu_version()); ret->pbs_dirty_bitmap = true; + ret->pbs_dirty_bitmap_savevm = true; + ret->query_bitmap_info = true; return ret; } diff --git a/qapi/block-core.json b/qapi/block-core.json -index e0a0a60354..b368371e8e 100644 +index f3608390c4..f57fda122c 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json -@@ -876,11 +876,14 @@ +@@ -876,6 +876,8 @@ # @pbs-dirty-bitmap: True if dirty-bitmap-incremental backups to PBS are # supported. # +# @query-bitmap-info: True if the 'query-pbs-bitmap-info' QMP call is supported. +# - # @pbs-library-version: Running version of libproxmox-backup-qemu0 library. + # @pbs-dirty-bitmap-savevm: True if 'dirty-bitmaps' migration capability can + # safely be set for savevm-async. # +@@ -884,6 +886,7 @@ ## { 'struct': 'ProxmoxSupportStatus', 'data': { 'pbs-dirty-bitmap': 'bool', + 'query-bitmap-info': 'bool', + 'pbs-dirty-bitmap-savevm': 'bool', 'pbs-library-version': 'str' } } - ## -@@ -893,6 +896,59 @@ +@@ -897,6 +900,59 @@ ## { 'command': 'query-proxmox-support', 'returns': 'ProxmoxSupportStatus' } diff --git a/debian/patches/pve/0037-PVE-Backup-Use-a-transaction-to-synchronize-job-stat.patch b/debian/patches/pve/0037-PVE-Backup-Use-a-transaction-to-synchronize-job-stat.patch index bb3b026..29f8050 100644 --- a/debian/patches/pve/0037-PVE-Backup-Use-a-transaction-to-synchronize-job-stat.patch +++ b/debian/patches/pve/0037-PVE-Backup-Use-a-transaction-to-synchronize-job-stat.patch @@ -16,7 +16,7 @@ Signed-off-by: Stefan Reiter 1 file changed, 49 insertions(+), 118 deletions(-) diff --git a/pve-backup.c b/pve-backup.c -index 1e7b92a950..f3df43eb8c 100644 +index 8305105fd5..d7f2b2206f 100644 --- a/pve-backup.c +++ b/pve-backup.c @@ -52,6 +52,7 @@ static struct PVEBackupState { diff --git a/debian/patches/pve/0038-PVE-Backup-Don-t-block-on-finishing-and-cleanup-crea.patch b/debian/patches/pve/0038-PVE-Backup-Don-t-block-on-finishing-and-cleanup-crea.patch index d15dda1..66023d6 100644 --- a/debian/patches/pve/0038-PVE-Backup-Don-t-block-on-finishing-and-cleanup-crea.patch +++ b/debian/patches/pve/0038-PVE-Backup-Don-t-block-on-finishing-and-cleanup-crea.patch @@ -54,7 +54,7 @@ Signed-off-by: Stefan Reiter 2 files changed, 144 insertions(+), 78 deletions(-) diff --git a/pve-backup.c b/pve-backup.c -index f3df43eb8c..ded90cb822 100644 +index d7f2b2206f..e671ed8d48 100644 --- a/pve-backup.c +++ b/pve-backup.c @@ -33,7 +33,9 @@ const char *PBS_BITMAP_NAME = "pbs-incremental-dirty-bitmap"; @@ -478,7 +478,7 @@ index f3df43eb8c..ded90cb822 100644 qemu_mutex_unlock(&backup_state.stat.lock); diff --git a/qapi/block-core.json b/qapi/block-core.json -index b368371e8e..b90d6abe64 100644 +index f57fda122c..9b827cbe43 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -775,12 +775,15 @@ diff --git a/debian/patches/pve/0039-PVE-Migrate-dirty-bitmap-state-via-savevm.patch b/debian/patches/pve/0039-PVE-Migrate-dirty-bitmap-state-via-savevm.patch index a4ee6bc..b4a7d5a 100644 --- a/debian/patches/pve/0039-PVE-Migrate-dirty-bitmap-state-via-savevm.patch +++ b/debian/patches/pve/0039-PVE-Migrate-dirty-bitmap-state-via-savevm.patch @@ -162,23 +162,24 @@ index 0000000000..29f2b3860d + NULL); +} diff --git a/pve-backup.c b/pve-backup.c -index ded90cb822..6b25292ba1 100644 +index e671ed8d48..bd2647e5f3 100644 --- a/pve-backup.c +++ b/pve-backup.c -@@ -1130,5 +1130,6 @@ ProxmoxSupportStatus *qmp_query_proxmox_support(Error **errp) +@@ -1130,6 +1130,7 @@ ProxmoxSupportStatus *qmp_query_proxmox_support(Error **errp) ret->pbs_library_version = g_strdup(proxmox_backup_qemu_version()); ret->pbs_dirty_bitmap = true; - ret->query_bitmap_info = true; + ret->pbs_dirty_bitmap_savevm = true; + ret->pbs_dirty_bitmap_migration = true; + ret->query_bitmap_info = true; return ret; } diff --git a/qapi/block-core.json b/qapi/block-core.json -index b90d6abe64..dee3d87efe 100644 +index 9b827cbe43..30eb1262ff 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json -@@ -881,12 +881,18 @@ - # - # @query-bitmap-info: True if the 'query-pbs-bitmap-info' QMP call is supported. +@@ -884,6 +884,11 @@ + # @pbs-dirty-bitmap-savevm: True if 'dirty-bitmaps' migration capability can + # safely be set for savevm-async. # +# @pbs-dirty-bitmap-migration: True if safe migration of dirty-bitmaps including +# PBS state is supported. Enabling 'dirty-bitmaps' @@ -188,9 +189,10 @@ index b90d6abe64..dee3d87efe 100644 # @pbs-library-version: Running version of libproxmox-backup-qemu0 library. # ## - { 'struct': 'ProxmoxSupportStatus', +@@ -891,6 +896,7 @@ 'data': { 'pbs-dirty-bitmap': 'bool', 'query-bitmap-info': 'bool', + 'pbs-dirty-bitmap-savevm': 'bool', + 'pbs-dirty-bitmap-migration': 'bool', 'pbs-library-version': 'str' } } diff --git a/debian/patches/pve/0042-PVE-Use-coroutine-QMP-for-backup-cancel_backup.patch b/debian/patches/pve/0042-PVE-Use-coroutine-QMP-for-backup-cancel_backup.patch index a18d478..ad86b55 100644 --- a/debian/patches/pve/0042-PVE-Use-coroutine-QMP-for-backup-cancel_backup.patch +++ b/debian/patches/pve/0042-PVE-Use-coroutine-QMP-for-backup-cancel_backup.patch @@ -115,7 +115,7 @@ index 4ce7bc0b5e..0923037dec 100644 static void proxmox_backup_schedule_wake(void *data) { CoCtxData *waker = (CoCtxData *)data; diff --git a/pve-backup.c b/pve-backup.c -index 6b25292ba1..f7597ae55c 100644 +index bd2647e5f3..dec9c0d188 100644 --- a/pve-backup.c +++ b/pve-backup.c @@ -357,7 +357,7 @@ static void job_cancel_bh(void *opaque) { @@ -574,7 +574,7 @@ index 6b25292ba1..f7597ae55c 100644 BackupStatus *qmp_query_backup(Error **errp) diff --git a/qapi/block-core.json b/qapi/block-core.json -index dee3d87efe..82133e2bee 100644 +index 30eb1262ff..6ff5367383 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -847,7 +847,7 @@ diff --git a/debian/patches/pve/0043-PBS-add-master-key-support.patch b/debian/patches/pve/0043-PBS-add-master-key-support.patch index 52e600c..c207ce5 100644 --- a/debian/patches/pve/0043-PBS-add-master-key-support.patch +++ b/debian/patches/pve/0043-PBS-add-master-key-support.patch @@ -30,7 +30,7 @@ index 11c84d5508..0932deb28c 100644 false, NULL, // PBS backup-id false, 0, // PBS backup-time diff --git a/pve-backup.c b/pve-backup.c -index f7597ae55c..0ecadf6ce6 100644 +index dec9c0d188..076146cc1e 100644 --- a/pve-backup.c +++ b/pve-backup.c @@ -531,6 +531,7 @@ UuidInfo coroutine_fn *qmp_backup( @@ -49,15 +49,15 @@ index f7597ae55c..0ecadf6ce6 100644 has_compress ? compress : true, has_encrypt ? encrypt : has_keyfile, has_fingerprint ? fingerprint : NULL, -@@ -1041,5 +1043,6 @@ ProxmoxSupportStatus *qmp_query_proxmox_support(Error **errp) - ret->pbs_dirty_bitmap = true; - ret->query_bitmap_info = true; +@@ -1042,5 +1044,6 @@ ProxmoxSupportStatus *qmp_query_proxmox_support(Error **errp) + ret->pbs_dirty_bitmap_savevm = true; ret->pbs_dirty_bitmap_migration = true; + ret->query_bitmap_info = true; + ret->pbs_masterkey = true; return ret; } diff --git a/qapi/block-core.json b/qapi/block-core.json -index 82133e2bee..be3d6a0d37 100644 +index 6ff5367383..bef9b65fec 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -818,6 +818,8 @@ @@ -77,7 +77,7 @@ index 82133e2bee..be3d6a0d37 100644 '*fingerprint': 'str', '*backup-id': 'str', '*backup-time': 'int', -@@ -886,6 +889,9 @@ +@@ -889,6 +892,9 @@ # migration cap if this is false/unset may lead # to crashes on migration! # @@ -87,9 +87,9 @@ index 82133e2bee..be3d6a0d37 100644 # @pbs-library-version: Running version of libproxmox-backup-qemu0 library. # ## -@@ -893,6 +899,7 @@ - 'data': { 'pbs-dirty-bitmap': 'bool', +@@ -897,6 +903,7 @@ 'query-bitmap-info': 'bool', + 'pbs-dirty-bitmap-savevm': 'bool', 'pbs-dirty-bitmap-migration': 'bool', + 'pbs-masterkey': 'bool', 'pbs-library-version': 'str' } } -- 2.20.1