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

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

[Denis V. Lunev]

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.

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
@@ -796,12 +796,22 @@ static void dirty_bitmap_state_pending(void *opaque, MigPendingData *data,
     data->postcopy_bytes += pending;
 }
 
+/* Chunk payload, read from the stream without holding DBMLoadState.lock */
+typedef struct DBMLoadChunk {
+    uint32_t granularity;
+    uint8_t start_flags;
+    uint64_t first_byte;
+    uint64_t nr_bytes;
+    uint64_t buf_size;
+    uint8_t *buf;
+} DBMLoadChunk;
+
 /* First occurrence of this bitmap. It should be created if doesn't exist */
-static int dirty_bitmap_load_start(QEMUFile *f, DBMLoadState *s)
+static int dirty_bitmap_load_start(DBMLoadState *s, DBMLoadChunk *c)
 {
     Error *local_err = NULL;
-    uint32_t granularity = qemu_get_be32(f);
-    uint8_t flags = qemu_get_byte(f);
+    uint32_t granularity = c->granularity;
+    uint8_t flags = c->start_flags;
     LoadBitmapState *b;
     bool persistent;
 
@@ -959,7 +969,7 @@ void dirty_bitmap_mig_cancel_incoming(void)
     qemu_mutex_unlock(&s->lock);
 }
 
-static void dirty_bitmap_load_complete(QEMUFile *f, DBMLoadState *s)
+static void dirty_bitmap_load_complete(DBMLoadState *s)
 {
     GSList *item;
     trace_dirty_bitmap_load_complete();
@@ -990,71 +1000,43 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DBMLoadState *s)
     }
 }
 
-static int dirty_bitmap_load_bits(QEMUFile *f, DBMLoadState *s)
+static int dirty_bitmap_load_bits(DBMLoadState *s, DBMLoadChunk *c)
 {
-    uint64_t first_byte = qemu_get_be64(f) << BDRV_SECTOR_BITS;
-    uint64_t nr_bytes = (uint64_t)qemu_get_be32(f) << BDRV_SECTOR_BITS;
-    trace_dirty_bitmap_load_bits_enter(first_byte >> BDRV_SECTOR_BITS,
-                                       nr_bytes >> BDRV_SECTOR_BITS);
+    uint64_t needed_size;
+
+    trace_dirty_bitmap_load_bits_enter(c->first_byte >> BDRV_SECTOR_BITS,
+                                       c->nr_bytes >> BDRV_SECTOR_BITS);
 
     if (s->flags & DIRTY_BITMAP_MIG_FLAG_ZEROES) {
         trace_dirty_bitmap_load_bits_zeroes();
         if (!s->cancelled) {
-            bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, first_byte,
-                                                 nr_bytes, false);
-        }
-    } else {
-        size_t ret;
-        g_autofree uint8_t *buf = NULL;
-        uint64_t buf_size = qemu_get_be64(f);
-        uint64_t needed_size;
-
-        /*
-         * The actual check for buf_size is done a bit later. We can't do it in
-         * cancelled mode as we don't have the bitmap to check the constraints
-         * (so, we allocate a buffer and read prior to the check). On the other
-         * hand, we shouldn't blindly g_malloc the number from the stream.
-         * Actually one chunk should not be larger than CHUNK_SIZE. Let's allow
-         * a bit larger (which means that bitmap migration will fail anyway and
-         * the whole migration will most probably fail soon due to broken
-         * stream).
-         */
-        if (buf_size > 10 * CHUNK_SIZE) {
-            error_report("Bitmap migration stream buffer allocation request "
-                         "is too large");
-            return -EIO;
-        }
-
-        buf = g_malloc(buf_size);
-        ret = qemu_get_buffer(f, buf, buf_size);
-        if (ret != buf_size) {
-            error_report("Failed to read bitmap bits");
-            return -EIO;
-        }
-
-        if (s->cancelled) {
-            return 0;
+            bdrv_dirty_bitmap_deserialize_zeroes(s->bitmap, c->first_byte,
+                                                 c->nr_bytes, false);
         }
+        return 0;
+    }
 
-        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 */
-        ) {
-            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 */
+    ) {
+        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;
     }
 
+    bdrv_dirty_bitmap_deserialize_part(s->bitmap, c->buf, c->first_byte,
+                                       c->nr_bytes, false);
     return 0;
 }
 
@@ -1105,23 +1087,73 @@ static BdrvDirtyBitmap *dirty_bitmap_load_find_bitmap(DBMLoadState *s)
     return NULL;
 }
 
-static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
-                                    GHashTable *alias_map)
+static int dirty_bitmap_read_chunk(QEMUFile *f, DBMLoadState *s,
+                                   DBMLoadChunk *c)
+{
+    s->flags = qemu_get_bitmap_flags(f);
+    trace_dirty_bitmap_load_header(s->flags);
+
+    if ((s->flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) &&
+        !qemu_get_counted_string(f, s->node_alias)) {
+        error_report("Unable to read node alias string");
+        return -EINVAL;
+    }
+
+    if ((s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) &&
+        !qemu_get_counted_string(f, s->bitmap_alias)) {
+        error_report("Unable to read bitmap alias string");
+        return -EINVAL;
+    }
+
+    if (s->flags & DIRTY_BITMAP_MIG_FLAG_START) {
+        c->granularity = qemu_get_be32(f);
+        c->start_flags = qemu_get_byte(f);
+    } else if (s->flags & DIRTY_BITMAP_MIG_FLAG_COMPLETE) {
+        /* no payload */
+    } else if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITS) {
+        c->first_byte = qemu_get_be64(f) << BDRV_SECTOR_BITS;
+        c->nr_bytes = (uint64_t)qemu_get_be32(f) << BDRV_SECTOR_BITS;
+
+        if (!(s->flags & DIRTY_BITMAP_MIG_FLAG_ZEROES)) {
+            c->buf_size = qemu_get_be64(f);
+
+            /*
+             * The actual check for buf_size is done when the chunk is
+             * applied. We can't do it in cancelled mode as we don't have the
+             * bitmap to check the constraints (so, we allocate a buffer and
+             * read prior to the check). On the other hand, we shouldn't
+             * blindly g_malloc the number from the stream. Actually one chunk
+             * should not be larger than CHUNK_SIZE. Let's allow a bit larger
+             * (which means that bitmap migration will fail anyway and the
+             * whole migration will most probably fail soon due to broken
+             * stream).
+             */
+            if (c->buf_size > 10 * CHUNK_SIZE) {
+                error_report("Bitmap migration stream buffer allocation "
+                             "request is too large");
+                return -EIO;
+            }
+
+            c->buf = g_malloc(c->buf_size);
+            if (qemu_get_buffer(f, c->buf, c->buf_size) != c->buf_size) {
+                error_report("Failed to read bitmap bits");
+                return -EIO;
+            }
+        }
+    }
+
+    return qemu_file_get_error(f);
+}
+
+static void dirty_bitmap_load_header(DBMLoadState *s, GHashTable *alias_map)
 {
     GHashTable *bitmap_alias_map = NULL;
     Error *local_err = NULL;
     bool nothing;
-    s->flags = qemu_get_bitmap_flags(f);
-    trace_dirty_bitmap_load_header(s->flags);
 
     nothing = s->flags == (s->flags & DIRTY_BITMAP_MIG_FLAG_EOS);
 
     if (s->flags & DIRTY_BITMAP_MIG_FLAG_DEVICE_NAME) {
-        if (!qemu_get_counted_string(f, s->node_alias)) {
-            error_report("Unable to read node alias string");
-            return -EINVAL;
-        }
-
         if (!s->cancelled) {
             if (alias_map) {
                 const AliasMapInnerNode *amin;
@@ -1165,11 +1197,6 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
     if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITMAP_NAME) {
         const char *bitmap_name;
 
-        if (!qemu_get_counted_string(f, s->bitmap_alias)) {
-            error_report("Unable to read bitmap alias string");
-            return -EINVAL;
-        }
-
         bitmap_name = s->bitmap_alias;
         if (!s->cancelled && bitmap_alias_map) {
             BitmapMigrationBitmapAlias *bmap_inner;
@@ -1206,7 +1233,23 @@ static int dirty_bitmap_load_header(QEMUFile *f, DBMLoadState *s,
         error_report("Error: block device name is not set");
         cancel_incoming_locked(s);
     }
+}
 
+static int dirty_bitmap_apply_chunk(DBMLoadState *s, DBMLoadChunk *c,
+                                    GHashTable *alias_map)
+{
+    dirty_bitmap_load_header(s, alias_map);
+
+    if (s->flags & DIRTY_BITMAP_MIG_FLAG_START) {
+        return dirty_bitmap_load_start(s, c);
+    }
+    if (s->flags & DIRTY_BITMAP_MIG_FLAG_COMPLETE) {
+        dirty_bitmap_load_complete(s);
+        return 0;
+    }
+    if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITS) {
+        return dirty_bitmap_load_bits(s, c);
+    }
     return 0;
 }
 
@@ -1240,28 +1283,23 @@ static int dirty_bitmap_load(QEMUFile *f, void *opaque, int version_id)
     }
 
     do {
-        QEMU_LOCK_GUARD(&s->lock);
-
-        ret = dirty_bitmap_load_header(f, s, alias_map);
-        if (ret < 0) {
-            cancel_incoming_locked(s);
-            goto fail;
-        }
-
-        if (s->flags & DIRTY_BITMAP_MIG_FLAG_START) {
-            ret = dirty_bitmap_load_start(f, s);
-        } else if (s->flags & DIRTY_BITMAP_MIG_FLAG_COMPLETE) {
-            dirty_bitmap_load_complete(f, s);
-        } else if (s->flags & DIRTY_BITMAP_MIG_FLAG_BITS) {
-            ret = dirty_bitmap_load_bits(f, s);
-        }
+        DBMLoadChunk c = {};
 
-        if (!ret) {
-            ret = qemu_file_get_error(f);
+        /*
+         * dirty_bitmap_mig_before_vm_start() takes the lock from the main
+         * thread, so never hold it while waiting for the stream.
+         */
+        ret = dirty_bitmap_read_chunk(f, s, &c);
+        WITH_QEMU_LOCK_GUARD(&s->lock) {
+            if (!ret) {
+                ret = dirty_bitmap_apply_chunk(s, &c, alias_map);
+            }
+            if (ret) {
+                cancel_incoming_locked(s);
+            }
         }
-
+        g_free(c.buf);
         if (ret) {
-            cancel_incoming_locked(s);
             goto fail;
         }
     } while (!(s->flags & DIRTY_BITMAP_MIG_FLAG_EOS));

base-commit: 723622bedb0b423de7d1c61ebbe6e7a71934949c
-- 
2.53.0

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

[Eric Blake]

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