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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

and bumped, good catch!

On July 28, 2023 11:44 am, 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@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
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>