[pve-devel] [PATCH pve-qemu 1/2] fix saving and loading dirty bitmaps in snapshots

[pve-devel] [PATCH pve-qemu 1/2] fix saving and loading dirty bitmaps in snapshots

[Stefan Reiter]

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 <s.reiter@proxmox.com>
---

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 <s.reiter@proxmox.com>
  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 <t.lamprecht@proxmox.com>
 [PVE: query-proxmox-support: include library version]
 Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
 ---
- 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 <s.reiter@proxmox.com>
  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 <s.reiter@proxmox.com>
  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

[pve-devel] [PATCH qemu-server 2/2] snapshot: set migration caps before savevm-start

[Stefan Reiter]

A "savevm" call (both our async variant and the upstream sync one) use
migration code internally. As such, they both expect migration
capabilities to be set.

This is usually not a problem, as the default set of capabilities is ok,
however, it leads to differing snapshot settings if one does a snapshot
after a machine has been live-migrated (as the capabilities will persist
from that), which could potentially lead to discrepencies in snapshots
(currently it seems to be fine, but it still makes sense to set them to
safeguard against future changes).

Note that we do set the "dirty-bitmaps" capability now (if
query-proxmox-support reports true), which has three effects:

1) PBS dirty-bitmaps are preserved in snapshots, enabling
   fast-incremental backups to work after rollback (as long as no newer
   backups exist), including for hibernate/resume
2) snapshots taken from now on, with a QEMU version supporting bitmap
   migration, *might* lead to incompatibility of these snapshots with
   QEMU versions that don't know about bitmaps at all (i.e. < 5.0 IIRC?)
   - forward compatibility is still given, and all other capabilities we
   set go back to very old versions
3) since we now explicitly disable bitmap saving if the version doesn't
   report support, we avoid crashes even with not-updated QEMU versions

Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
---
 PVE/QemuConfig.pm     | 1 +
 PVE/QemuServer.pm     | 8 ++++++--
 test/snapshot-test.pm | 2 ++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm
index c58ff19..7ee8876 100644
--- a/PVE/QemuConfig.pm
+++ b/PVE/QemuConfig.pm
@@ -281,6 +281,7 @@ sub __snapshot_create_vol_snapshots_hook {
 		PVE::Storage::activate_volumes($storecfg, [$snap->{vmstate}]);
 		my $state_storage_id = PVE::Storage::parse_volume_id($snap->{vmstate});
 
+		PVE::QemuServer::set_migration_caps($vmid, 1);
 		mon_cmd($vmid, "savevm-start", statefile => $path);
 		print "saving VM state and RAM using storage '$state_storage_id'\n";
 		my $render_state = sub {
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 15100ed..1c0b5c2 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -4330,10 +4330,13 @@ sub qemu_volume_snapshot_delete {
 }
 
 sub set_migration_caps {
-    my ($vmid) = @_;
+    my ($vmid, $savevm) = @_;
 
     my $qemu_support = eval { mon_cmd($vmid, "query-proxmox-support") };
 
+    my $bitmap_prop = $savevm ? 'pbs-dirty-bitmap-savevm' : 'pbs-dirty-bitmap-migration';
+    my $dirty_bitmaps = $qemu_support->{$bitmap_prop} ? 1 : 0;
+
     my $cap_ref = [];
 
     my $enabled_cap = {
@@ -4342,7 +4345,7 @@ sub set_migration_caps {
 	"x-rdma-pin-all" => 0,
 	"zero-blocks" => 0,
 	"compress" => 0,
-	"dirty-bitmaps" => $qemu_support->{'pbs-dirty-bitmap-migration'} ? 1 : 0,
+	"dirty-bitmaps" => $dirty_bitmaps,
     };
 
     my $supported_capabilities = mon_cmd($vmid, "query-migrate-capabilities");
@@ -5600,6 +5603,7 @@ sub vm_suspend {
 	PVE::Storage::activate_volumes($storecfg, [$vmstate]);
 
 	eval {
+	    set_migration_caps($vmid, 1);
 	    mon_cmd($vmid, "savevm-start", statefile => $path);
 	    for(;;) {
 		my $state = mon_cmd($vmid, "query-savevm");
diff --git a/test/snapshot-test.pm b/test/snapshot-test.pm
index f65a902..cc04f01 100644
--- a/test/snapshot-test.pm
+++ b/test/snapshot-test.pm
@@ -380,6 +380,8 @@ sub vm_stop {
     return;
 }
 
+sub set_migration_caps {} # ignored
+
 # END redefine PVE::QemuServer methods
 
 PVE::Tools::run_command("rm -rf snapshot-working");
-- 
2.20.1

[pve-devel] applied: [PATCH pve-qemu 1/2] fix saving and loading dirty bitmaps in snapshots

[Thomas Lamprecht]

On 16.03.21 17:30, Stefan Reiter wrote:
> 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 <s.reiter@proxmox.com>
> ---
> 
> 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(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH qemu-server 2/2] snapshot: set migration caps before savevm-start

[Thomas Lamprecht]

On 16.03.21 17:30, Stefan Reiter wrote:
> A "savevm" call (both our async variant and the upstream sync one) use
> migration code internally. As such, they both expect migration
> capabilities to be set.
> 
> This is usually not a problem, as the default set of capabilities is ok,
> however, it leads to differing snapshot settings if one does a snapshot
> after a machine has been live-migrated (as the capabilities will persist
> from that), which could potentially lead to discrepencies in snapshots
> (currently it seems to be fine, but it still makes sense to set them to
> safeguard against future changes).
> 
> Note that we do set the "dirty-bitmaps" capability now (if
> query-proxmox-support reports true), which has three effects:
> 
> 1) PBS dirty-bitmaps are preserved in snapshots, enabling
>    fast-incremental backups to work after rollback (as long as no newer
>    backups exist), including for hibernate/resume
> 2) snapshots taken from now on, with a QEMU version supporting bitmap
>    migration, *might* lead to incompatibility of these snapshots with
>    QEMU versions that don't know about bitmaps at all (i.e. < 5.0 IIRC?)
>    - forward compatibility is still given, and all other capabilities we
>    set go back to very old versions

not an issue, in practice starting a snapshot made with a newer QEMU with
and older one did not work often due to the running machine version not
being available anyway...

> 3) since we now explicitly disable bitmap saving if the version doesn't
>    report support, we avoid crashes even with not-updated QEMU versions
> 
> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
> ---
>  PVE/QemuConfig.pm     | 1 +
>  PVE/QemuServer.pm     | 8 ++++++--
>  test/snapshot-test.pm | 2 ++
>  3 files changed, 9 insertions(+), 2 deletions(-)
> 
>

applied, thanks!

Albeit it feels like this could have been two patches and a short comment
for the set_migration_caps calls, as they can be slightly unexpected for
someone not knowing that half the things QEMU can do base on the migrate
code.