Re: [PATCH] block/mirror: disable dirty bitmap only after job init

[Kevin Wolf <kwolf@redhat.com>]

Am 16.02.2026 um 14:01 hat Fiona Ebner geschrieben:
> Am 16.02.26 um 1:48 PM schrieb Fiona Ebner:
> > Am 16.02.26 um 11:25 AM schrieb Kevin Wolf:
> >> Am 12.02.2026 um 13:02 hat Fiona Ebner geschrieben:
> >>> Currently, the dirty bitmap is disabled too early and the following
> >>> bad scenario is possible:
> >>>
> >>> 1. Dirty bitmap is disabled in mirror_start_job()
> >>> 2. Some request are started in mirror_top_bs while s->job == NULL
> >>> 3. mirror_dirty_init() -> bdrv_co_is_allocated_above() runs and because
> >>>    the request hasn't completed yet, the block isn't allocated
> >>> 4. The request completes, still sees s->job == NULL and skips the
> >>>    bitmap, and nothing else will mark it dirty either
> >>>
> >>> One ingredient is that mirror_top_opaque->job is only set after the
> >>> job is fully initialized. For the rationale, see commit 32125b1460
> >>> ("mirror: Fix access of uninitialised fields during start").
> >>>
> >>> Disabling the dirty bitmap is safe once bdrv_mirror_top_do_write()
> >>> sees that the job is set, because then:
> >>>
> >>> 1. When not using MIRROR_COPY_MODE_WRITE_BLOCKING, the dirty bitmap
> >>>    will be set by bdrv_mirror_top_do_write().
> >>>
> >>> 2. When using MIRROR_COPY_MODE_WRITE_BLOCKING, writes will be done
> >>>    synchronously to the target.
> >>>
> >>> At least with virtio-blk using iothread-vq-mapping, mirror_run() and
> >>> bdrv_mirror_top_do_write() might be called in different threads.
> >>> bdrv_disable_dirty_bitmap() acquires and releases the dirty bitmap
> >>> mutex, so the memory is synchronized between threads.
> >>>
> >>> Many thanks to Kevin Wolf for discussing the issue and solutions with
> >>> me! [0]
> >>>
> >>> [0]: https://lore.kernel.org/qemu-devel/4853b0e5-8ec3-41e9-9a53-b1912b8e4449@dupond.be/T/
> >>>
> >>> Cc: qemu-stable@nongnu.org
> >>> Closes: https://gitlab.com/qemu-project/qemu/-/issues/3273
> >>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> >>> ---
> >>>  block/mirror.c | 21 +++++++++++++++------
> >>>  1 file changed, 15 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/block/mirror.c b/block/mirror.c
> >>> index b344182c74..eadd4501e8 100644
> >>> --- a/block/mirror.c
> >>> +++ b/block/mirror.c
> >>> @@ -1123,6 +1123,21 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
> >>>       */
> >>>      mirror_top_opaque->job = s;
> >>>  
> >>> +    /*
> >>> +     * Disabling the dirty bitmap is safe once bdrv_mirror_top_do_write() sees
> >>> +     * that the job is set, because then:
> >>> +     *
> >>> +     * 1. When not using MIRROR_COPY_MODE_WRITE_BLOCKING, the dirty bitmap will
> >>> +     * be set by bdrv_mirror_top_do_write().
> >>> +     *
> >>> +     * 2. When using MIRROR_COPY_MODE_WRITE_BLOCKING, writes will be done
> >>> +     * synchronously to the target.
> >>> +     *
> >>> +     * bdrv_disable_dirty_bitmap() acquires and releases the dirty bitmap mutex,
> >>> +     * so the memory is synchronized between threads.
> >>> +     */
> >>> +    bdrv_disable_dirty_bitmap(s->dirty_bitmap);
> >>> +
> >>>      assert(!s->dbi);
> >>>      s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap);
> >>>      for (;;) {
> >>> @@ -2014,12 +2029,6 @@ static BlockJob *mirror_start_job(
> >>>          goto fail;
> >>>      }
> >>>  
> >>> -    /*
> >>> -     * The dirty bitmap is set by bdrv_mirror_top_do_write() when not in active
> >>> -     * mode.
> >>> -     */
> >>> -    bdrv_disable_dirty_bitmap(s->dirty_bitmap);
> >>> -
> >>>      bdrv_graph_wrlock_drained();
> >>>      ret = block_job_add_bdrv(&s->common, "source", bs, 0,
> >>>                               BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE |
> >>
> >> The thing I meant in the other thread is if we don't need something like
> >> this additionally:
> >>
> >> diff --git a/block/mirror.c b/block/mirror.c
> >> index b344182c747..159954158ba 100644
> >> --- a/block/mirror.c
> >> +++ b/block/mirror.c
> >> @@ -1672,9 +1672,17 @@ bdrv_mirror_top_do_write(BlockDriverState *bs, MirrorMethod method,
> >>          abort();
> >>      }
> >>  
> >> -    if (!copy_to_target && s->job && s->job->dirty_bitmap) {
> >> +    if (!copy_to_target) {
> >>          qatomic_set(&s->job->actively_synced, false);
> >> -        bdrv_set_dirty_bitmap(s->job->dirty_bitmap, offset, bytes);
> >> +        if (s->job && s->job->dirty_bitmap) {
> >> +            bdrv_set_dirty_bitmap(s->job->dirty_bitmap, offset, bytes);
> >> +        } else {
> >> +            /*
> >> +             * Avoid race in the case that mirror_run() disables the bitmap
> >> +             * between here and bdrv_co_write_req_finish().
> >> +             */
> >> +            bdrv_set_dirty(bs, offset, bytes);
> >> +        }
> >>      }
> >>  
> >>      if (ret < 0) {
> > 
> > You're right! It's not enough to ensure that the job is set before the
> > bitmap is disabled, because both could happen after the check for job here.
> > 
> > Isn't this change enough by itself? After the change,
> > bdrv_mirror_top_do_write() ensures that:
> > 
> > 1. When copy_to_target == true, the write is done synchronously to the
> > target, no need to set the dirty bitmap.
> > 
> > 2. When copy_to_target == false, the dirty bitmap is set.
> > 
> > So it doesn't really matter at which point the dirty bitmap is disabled?
> > Or am I missing something again?
> 
> Ah right, bdrv_set_dirty() only sets it if still enabled :)

Yes, exactly.

If that's still too confusing, how about just giving mirror_top_bs
access to its dirty bitmap right from the start while it's drained? I
didn't try to reproduce the bug with it yet, though.

Kevin

diff --git a/block/mirror.c b/block/mirror.c
index b344182c747..c11aca1366c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -99,6 +99,7 @@ typedef struct MirrorBlockJob {
 
 typedef struct MirrorBDSOpaque {
     MirrorBlockJob *job;
+    BdrvDirtyBitmap *dirty_bitmap;
     bool stop;
     bool is_commit;
 } MirrorBDSOpaque;
@@ -1672,9 +1673,9 @@ bdrv_mirror_top_do_write(BlockDriverState *bs, MirrorMethod method,
         abort();
     }
 
-    if (!copy_to_target && s->job && s->job->dirty_bitmap) {
+    if (!copy_to_target) {
         qatomic_set(&s->job->actively_synced, false);
-        bdrv_set_dirty_bitmap(s->job->dirty_bitmap, offset, bytes);
+        bdrv_set_dirty_bitmap(s->dirty_bitmap, offset, bytes);
     }
 
     if (ret < 0) {
@@ -1901,13 +1902,28 @@ static BlockJob *mirror_start_job(
 
     bdrv_drained_begin(bs);
     ret = bdrv_append(mirror_top_bs, bs, errp);
-    bdrv_drained_end(bs);
-
     if (ret < 0) {
+        bdrv_drained_end(bs);
         bdrv_unref(mirror_top_bs);
         return NULL;
     }
 
+    bs_opaque->dirty_bitmap = bdrv_create_dirty_bitmap(mirror_top_bs,
+                                                       granularity,
+                                                       NULL, errp);
+    if (!bs_opaque->dirty_bitmap) {
+        bdrv_drained_end(bs);
+        bdrv_unref(mirror_top_bs);
+        return NULL;
+    }
+
+    /*
+     * The dirty bitmap is set by bdrv_mirror_top_do_write() when not in active
+     * mode.
+     */
+    bdrv_disable_dirty_bitmap(bs_opaque->dirty_bitmap);
+    bdrv_drained_end(bs);
+
     /* Make sure that the source is not resized while the job is running */
     s = block_job_create(job_id, driver, NULL, mirror_top_bs,
                          BLK_PERM_CONSISTENT_READ,
@@ -2002,24 +2018,13 @@ static BlockJob *mirror_start_job(
     s->base_overlay = bdrv_find_overlay(bs, base);
     s->granularity = granularity;
     s->buf_size = ROUND_UP(buf_size, granularity);
+    s->dirty_bitmap = bs_opaque->dirty_bitmap;
     s->unmap = unmap;
     if (auto_complete) {
         s->should_complete = true;
     }
     bdrv_graph_rdunlock_main_loop();
 
-    s->dirty_bitmap = bdrv_create_dirty_bitmap(s->mirror_top_bs, granularity,
-                                               NULL, errp);
-    if (!s->dirty_bitmap) {
-        goto fail;
-    }
-
-    /*
-     * The dirty bitmap is set by bdrv_mirror_top_do_write() when not in active
-     * mode.
-     */
-    bdrv_disable_dirty_bitmap(s->dirty_bitmap);
-
     bdrv_graph_wrlock_drained();
     ret = block_job_add_bdrv(&s->common, "source", bs, 0,
                              BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE |
@@ -2099,9 +2104,6 @@ fail:
         g_free(s->replaces);
         blk_unref(s->target);
         bs_opaque->job = NULL;
-        if (s->dirty_bitmap) {
-            bdrv_release_dirty_bitmap(s->dirty_bitmap);
-        }
         job_early_fail(&s->common.job);
     }
 
@@ -2115,6 +2117,7 @@ fail:
     bdrv_graph_wrunlock();
     bdrv_drained_end(bs);
 
+    bdrv_release_dirty_bitmap(bs_opaque->dirty_bitmap);
     bdrv_unref(mirror_top_bs);
 
     return NULL;