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

Stefan Reiter s.reiter at proxmox.com
Tue Mar 16 17:30:22 CET 2021


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






More information about the pve-devel mailing list