[pve-devel] [PATCH qemu] add patch fixing resume for snapshot and hibernate with drive with iothread and a dirty bitmap
Friedrich Weber
f.weber at proxmox.com
Fri Jul 28 12:59:23 CEST 2023
As described, make a PBS backup of an existing VM and then took a
snapshot. Rolling back to the snapshot failed with "qemu:
qemu_mutex_unlock_impl: Operation not permitted". With this patch
applied, rolling back worked.
Tested-by: Friedrich Weber <f.weber at proxmox.com>
On 28/07/2023 11:44, Fiona Ebner wrote:
> 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 at 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 at 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 at 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 at 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
More information about the pve-devel
mailing list