Re: [PATCH 1/2] migration/block-dirty-bitmap: fix postcopy abort with bitmaps on several nodes

[Eric Blake <eblake@redhat.com>]

On Thu, Sep 24, 2026 at 01:56:10PM +0200, Denis V. Lunev wrote:
> From: Denis V. Lunev <den@openvz.org>
> 
> 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 <den@openvz.org>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Fabiano Rosas <farosas@suse.de>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Cc: qemu-stable@nongnu.org
> ---
>  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