[pve-devel] [PATCH qemu] add patch fixing resume for snapshot and hibernate with drive with iothread and a dirty bitmap

[Fiona Ebner <f.ebner@proxmox.com>]

Not difficult to run into, just have a drive with iothread, take a PBS
backup and then take a snapshot or hibernate. Resuming will fail with
> qemu: qemu_mutex_unlock_impl: Operation not permitted
because of not acquiring the correct AioContext first.

Migration is not affected, because it runs in coroutine context.

Reported in the community forum:
https://forum.proxmox.com/threads/129899/

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Surprised there were not more reports, but it could also be that
people are now sitting on some snapshots which can't be rolled back
without this fix.

Will try to reproduce the issue with upstream QEMU (don't see why they
wouldn't be affected) and upstream the fix if they are affected too.

 ...dirty-bitmap-fix-loading-bitmap-when.patch | 48 +++++++++++++++++++
 ...dirty-bitmap-migrate-other-bitmaps-e.patch |  2 +-
 ...apshots-hold-the-BQL-during-setup-ca.patch |  6 +--
 debian/patches/series                         |  1 +
 4 files changed, 53 insertions(+), 4 deletions(-)
 create mode 100644 debian/patches/extra/0010-migration-block-dirty-bitmap-fix-loading-bitmap-when.patch

diff --git a/debian/patches/extra/0010-migration-block-dirty-bitmap-fix-loading-bitmap-when.patch b/debian/patches/extra/0010-migration-block-dirty-bitmap-fix-loading-bitmap-when.patch
new file mode 100644
index 0000000..bb01ced
--- /dev/null
+++ b/debian/patches/extra/0010-migration-block-dirty-bitmap-fix-loading-bitmap-when.patch
@@ -0,0 +1,48 @@
+From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
+From: Fiona Ebner <f.ebner@proxmox.com>
+Date: Fri, 28 Jul 2023 10:47:48 +0200
+Subject: [PATCH] migration/block-dirty-bitmap: fix loading bitmap when there
+ is an iothread
+
+The bdrv_create_dirty_bitmap() function (which is also called by
+bdrv_dirty_bitmap_create_successor()) uses bdrv_getlength(bs). This is
+a wrapper around a coroutine, and thus uses bdrv_poll_co(). Polling
+tries to release the AioContext which will trigger an assert() if it
+hasn't been acquired before.
+
+The issue does not happen for migration, because there we are in a
+coroutine already, so the wrapper will just call bdrv_co_getlength()
+directly without polling.
+
+Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
+---
+ migration/block-dirty-bitmap.c | 6 ++++++
+ 1 file changed, 6 insertions(+)
+
+diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
+index fe73aa94b1..7eaf498439 100644
+--- a/migration/block-dirty-bitmap.c
++++ b/migration/block-dirty-bitmap.c
+@@ -805,8 +805,11 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
+                      "destination", bdrv_dirty_bitmap_name(s->bitmap));
+         return -EINVAL;
+     } else {
++        AioContext *ctx = bdrv_get_aio_context(s->bs);
++        aio_context_acquire(ctx);
+         s->bitmap = bdrv_create_dirty_bitmap(s->bs, granularity,
+                                              s->bitmap_name, &local_err);
++        aio_context_release(ctx);
+         if (!s->bitmap) {
+             error_report_err(local_err);
+             return -EINVAL;
+@@ -833,7 +836,10 @@ static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
+ 
+     bdrv_disable_dirty_bitmap(s->bitmap);
+     if (flags & DIRTY_BITMAP_MIG_START_FLAG_ENABLED) {
++        AioContext *ctx = bdrv_get_aio_context(s->bs);
++        aio_context_acquire(ctx);
+         bdrv_dirty_bitmap_create_successor(s->bitmap, &local_err);
++        aio_context_release(ctx);
+         if (local_err) {
+             error_report_err(local_err);
+             return -EINVAL;
diff --git a/debian/patches/pve/0035-migration-block-dirty-bitmap-migrate-other-bitmaps-e.patch b/debian/patches/pve/0035-migration-block-dirty-bitmap-migrate-other-bitmaps-e.patch
index 0e3f38d..bd721fc 100644
--- a/debian/patches/pve/0035-migration-block-dirty-bitmap-migrate-other-bitmaps-e.patch
+++ b/debian/patches/pve/0035-migration-block-dirty-bitmap-migrate-other-bitmaps-e.patch
@@ -19,7 +19,7 @@ Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
-index fe73aa94b1..a6440929fa 100644
+index 7eaf498439..509f3df0a6 100644
 --- a/migration/block-dirty-bitmap.c
 +++ b/migration/block-dirty-bitmap.c
 @@ -539,7 +539,7 @@ static int add_bitmaps_to_list(DBMSaveState *s, BlockDriverState *bs,
diff --git a/debian/patches/pve/0044-migration-for-snapshots-hold-the-BQL-during-setup-ca.patch b/debian/patches/pve/0044-migration-for-snapshots-hold-the-BQL-during-setup-ca.patch
index cbc39cc..04ef6cb 100644
--- a/debian/patches/pve/0044-migration-for-snapshots-hold-the-BQL-during-setup-ca.patch
+++ b/debian/patches/pve/0044-migration-for-snapshots-hold-the-BQL-during-setup-ca.patch
@@ -67,10 +67,10 @@ index a8dfd8fefd..fa9b0b0f10 100644
       * must_precopy:
       * - must be migrated in precopy or in stopped state
 diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
-index a6440929fa..69fab3275c 100644
+index 509f3df0a6..42dc4a8d61 100644
 --- a/migration/block-dirty-bitmap.c
 +++ b/migration/block-dirty-bitmap.c
-@@ -1214,10 +1214,17 @@ static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
+@@ -1220,10 +1220,17 @@ static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
  {
      DBMSaveState *s = &((DBMState *)opaque)->save;
      SaveBitmapState *dbms = NULL;
@@ -90,7 +90,7 @@ index a6440929fa..69fab3275c 100644
          return -1;
      }
  
-@@ -1225,7 +1232,9 @@ static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
+@@ -1231,7 +1238,9 @@ static int dirty_bitmap_save_setup(QEMUFile *f, void *opaque)
          send_bitmap_start(f, s, dbms);
      }
      qemu_put_bitmap_flags(f, DIRTY_BITMAP_MIG_FLAG_EOS);
diff --git a/debian/patches/series b/debian/patches/series
index c9c96d7..a4dd4c2 100644
--- a/debian/patches/series
+++ b/debian/patches/series
@@ -7,6 +7,7 @@ extra/0006-lsi53c895a-disable-reentrancy-detection-for-script-R.patch
 extra/0007-bcm2835_property-disable-reentrancy-detection-for-io.patch
 extra/0008-raven-disable-reentrancy-detection-for-iomem.patch
 extra/0009-apic-disable-reentrancy-detection-for-apic-msi.patch
+extra/0010-migration-block-dirty-bitmap-fix-loading-bitmap-when.patch
 bitmap-mirror/0001-drive-mirror-add-support-for-sync-bitmap-mode-never.patch
 bitmap-mirror/0002-drive-mirror-add-support-for-conditional-and-always-.patch
 bitmap-mirror/0003-mirror-add-check-for-bitmap-mode-without-bitmap.patch
-- 
2.39.2