Re: [PATCH] mirror: Fix missed dirty bitmap writes during startup

[Kevin Wolf <kwolf@redhat.com>]

Am 05.03.2026 um 19:34 hat Vladimir Sementsov-Ogievskiy geschrieben:
> On 19.02.26 23:24, Kevin Wolf wrote:
> > Currently, mirror disables the block layer's dirty bitmap before its own
> > replacement is working. This means that during startup, there is a
> > window in which the allocation status of blocks in the source has
> > already been checked, but new writes coming in aren't tracked yet,
> > resulting in a corrupted copy:
> > 
> > 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").
> > 
> > Fix this by giving mirror_top_bs access to dirty_bitmap and enabling it
> > to track writes from the beginning. Disabling the block layer's tracking
> > and enabling the mirror_top_bs one happens in a drained section, so
> > there is no danger of races with in-flight requests any more. All of
> > this happens well before the block allocation status is checked, so we
> > can be sure that no writes will be missed.
> > 
> > Cc: qemu-stable@nongnu.org
> > Closes: https://gitlab.com/qemu-project/qemu/-/issues/3273
> > Fixes: 32125b14606a ('mirror: Fix access of uninitialised fields during start')
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> > Supersedes: <20260212120411.369498-1-f.ebner@proxmox.com>
> > ---
> >   block/mirror.c | 48 +++++++++++++++++++++++++++++-------------------
> >   1 file changed, 29 insertions(+), 19 deletions(-)
> > 
> > diff --git a/block/mirror.c b/block/mirror.c
> > index b344182c747..f38636e7457 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,35 @@ 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 mirror job doesn't use the block layer's dirty tracking because it
> > +     * needs to be able to switch seemlessly between background copy mode (which
> > +     * does need dirty tracking) and write blocking mode (which doesn't) and
> > +     * doing that would require draining the node. Instead, mirror_top_bs takes
> > +     * care of updating the dirty bitmap as appropriate.
> > +     *
> > +     * Note that write blocking mode only becomes effective after mirror_run()
> > +     * sets mirror_top_opaque->job (see should_copy_to_target()). Until then,
> > +     * we're still in background copy mode irrespective of @copy_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 +2025,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 +2111,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 +2124,7 @@ fail:
> >       bdrv_graph_wrunlock();
> >       bdrv_drained_end(bs);
> > +    bdrv_release_dirty_bitmap(bs_opaque->dirty_bitmap);
> 
> 
> Hmm. Shouldn't we change position of _release_ in mirror_exit_common() too?
> 
> Now the sequence is:
> 
> bdrv_release_dirty_bitmap(s->dirty_bitmap);
> 
> 
> < could mirror_top_bs access dirty_bitmap here, before drained begin? >
> 
> ...
> 
> drained begin
> 
> .. a lot of logic, including actual removing of the mirror_top_bs from the chain ..
> 
> drained end
> 
> bdrv_unref(mirror_top_bs)

I think you're right, but isn't this already a preexisting bug in
master? After releasing, we don't set s->dirty_bitmap = NULL, which
could have prevented the access in the code before this patch. So this
should probably be a separate patch.

mirror_exit_common() runs in the main loop, so I assume you can
hit this when using an iothread.

It seems that initially the release was later, but commit 2119882 moved
it earlier, without saying why it did that. Paolo, do you remember?

Kevin