On Thu, Sep 24, 2026 at 01:56:10PM +0200, Denis V. Lunev wrote:
> From: Denis V. Lunev <[email protected]>
>
> With dirty bitmaps on more than one node, the source switches nodes
> during the postcopy bulk phase and sends the node name again. The
> destination reads those chunks in the postcopy listen thread, which does
> not hold the BQL, and resolves the name through bdrv_lookup_bs():
>
> qemu-system-x86_64: ../block.c:6400: bdrv_lookup_bs: Assertion
> `qemu_in_main_thread()' failed.
>
> The bdrv_find_dirty_bitmap() call right after it needs the BQL or the
> dirty bitmap lock as well. Taking the BQL there would deadlock: the
> listen thread holds s->lock, and dirty_bitmap_mig_before_vm_start()
> takes s->lock with the BQL held.
>
> Every node and bitmap that a non-START chunk refers to was set up by a
> START chunk, and those are loaded under the BQL during setup. Record the
> node alias there, and resolve all other chunks against s->bitmaps
> instead of the block graph.
>
> Bitmaps on a single node never resend the node name after setup, which
> is why migrate-bitmaps-postcopy-test does not catch this.
>
> Fixes: f791bf7f93f2 ("assertions for block global state API")
> Signed-off-by: Denis V. Lunev <[email protected]>
> Cc: Peter Xu <[email protected]>
> Cc: Fabiano Rosas <[email protected]>
> Cc: Eric Blake <[email protected]>
> Cc: Vladimir Sementsov-Ogievskiy <[email protected]>
> Cc: [email protected]
> ---
> migration/block-dirty-bitmap.c | 59 +++++++++++++++++++++++++++++++---
> 1 file changed, 55 insertions(+), 4 deletions(-)
>
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 1b8f39c12b..61691ee4cc 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -136,6 +136,7 @@ typedef struct DBMSaveState {
> } DBMSaveState;
>
> typedef struct LoadBitmapState {
> + char node_alias[256];
Is making this fixed-length rather than malloc'd char* correct?
/me checks
migration/block-dirty-bitmap.c: char node_alias[256];
Yep. I'm a bit surprised we did it that way earlier, but this is
correct. That said, if enough structs reference the same size, we may
want a #define or enum named value in a header, rather than a magic
number at each use site.
> +/*
> + * Only START chunks are guaranteed to be loaded under the BQL; the rest may
> + * come from the postcopy listen thread and must not touch the block graph.
> + */
> +static BlockDriverState *dirty_bitmap_load_find_bs(DBMLoadState *s,
> + const char *device,
> + const char *node_name,
> + Error **errp)
> +{
> + GSList *item;
> +
> + if (s->flags & DIRTY_BITMAP_MIG_FLAG_START) {
> + return bdrv_lookup_bs(device, node_name, errp);
> + }
> +
> + for (item = s->bitmaps; item; item = g_slist_next(item)) {
> + LoadBitmapState *b = item->data;
> +
> + if (!strcmp(b->node_alias, s->node_alias)) {
> + return b->bs;
> + }
> + }
> +
> + error_setg(errp, "Error: no migrated bitmaps on node alias '%s'",
> + s->node_alias);
Drop the "Error: " prefix; it will be redundant if the caller reports
rather than ignores the error.
Otherwise, this makes sense.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
