Re: [PATCH] migration/block-dirty-bitmap: don't hold the load lock across stream reads

[Eric Blake <eblake@redhat.com>]

On Tue, Nov 10, 2026 at 07:24:08PM +0100, Denis V. Lunev wrote:
> From: Denis V. Lunev <den@openvz.org>
> 
> dirty_bitmap_load() takes dbm_state.load.lock for each chunk and holds
> it while it reads the chunk from the migration stream.
> 
> In precopy the load runs in the incoming migration coroutine, which
> yields in qemu_fill_buffer() while it waits for data, still holding
> the lock. A quit on the destination at that point never completes:
> migration_shutdown() calls dirty_bitmap_mig_cancel_incoming(), which
> takes the same lock in the same main thread.
> 
> In postcopy the load runs in the listen thread, and the main thread
> takes the lock in dirty_bitmap_mig_before_vm_start(), called from
> loadvm_postcopy_handle_run_bh(), and in migration_shutdown(). The main
> loop then waits until the source sends the next chunk.
> 
> Read each chunk into a DBMLoadChunk first and take the lock only to
> resolve the names and apply the chunk. The lock protects the bitmaps
> list and the resolved node and bitmap, which the main thread uses. The
> flags and alias strings read from the stream are only touched by the
> code doing the load.

I'm guessing this should be considered for 11.2, and necessary as a
followup for the recent commit 4d11088a "migration/block-dirty-bitmap:
fix postcopy abort...")?

> 
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> Cc: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> Cc: Eric Blake <eblake@redhat.com>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Peter Xu <peterx@redhat.com>
> Cc: Fabiano Rosas <farosas@suse.de>
> Cc: qemu-block@nongnu.org
> ---
>  migration/block-dirty-bitmap.c | 222 +++++++++++++++++++--------------
>  1 file changed, 130 insertions(+), 92 deletions(-)
> 
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 35623379eb..14e5a984e4 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c

> -static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
> +static int dirty_bitmap_load_bits(DBMLoadState *s, DBMLoadChunk *c)
>  {
...
> -        needed_size = bdrv_dirty_bitmap_serialization_size(s->bitmap,
> -                                                           first_byte,
> -                                                           nr_bytes);
> -
> -        if (needed_size > buf_size ||
> -            buf_size > QEMU_ALIGN_UP(needed_size, 4 * sizeof(long))
> -             /* Here used same alignment as in send_bitmap_bits */
> -        ) {

It looks odd to see a comment appear mid-argument, which results in
unusual formatting...

> -            error_report("Migrated bitmap granularity doesn't "
> -                         "match the destination bitmap '%s' granularity",
> -                         bdrv_dirty_bitmap_name(s->bitmap));
> -            cancel_incoming_locked(s);
> -            return 0;
> -        }
> +    if (s->cancelled) {
> +        return 0;
> +    }
>  
> -        bdrv_dirty_bitmap_deserialize_part(s->bitmap, buf, first_byte, nr_bytes,
> -                                           false);
> +    needed_size = bdrv_dirty_bitmap_serialization_size(s->bitmap,
> +                                                       c->first_byte,
> +                                                       c->nr_bytes);
> +
> +    if (needed_size > c->buf_size ||
> +        c->buf_size > QEMU_ALIGN_UP(needed_size, 4 * sizeof(long))
> +         /* Here used same alignment as in send_bitmap_bits */
> +    ) {

...which then got preserved with an unindent. Not worth cleaning up,
but it did make me do a double-take.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org