block/mirror.c | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-)
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 |
--
2.47.3Am 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) {
By the way, can it ever happen that s->job && !s->job->dirty_bitmap or
is that second part of the condition redundant?
Kevin
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?
> By the way, can it ever happen that s->job && !s->job->dirty_bitmap or
> is that second part of the condition redundant?
In mirror_exit_common(), we do call bdrv_release_dirty_bitmap() before
the drained section replacing the node and before setting
bs_opaque->job = NULL; But AFAICS, we do not actually set
bs_opaque->job->dirty_bitmap = NULL anywhere. So yes, the second part of
the condition seems redundant to me too. I think the fact that we don't
set bs_opaque->job->dirty_bitmap = NULL can't lead to a use-after-free
either, because mirror_exit_common() happens while drained, because
mirror_run() drains the source (and thus also the filter on top) before
returning.
Best Regards,
Fiona
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 :)
>
>> By the way, can it ever happen that s->job && !s->job->dirty_bitmap or
>> is that second part of the condition redundant?
>
> In mirror_exit_common(), we do call bdrv_release_dirty_bitmap() before
> the drained section replacing the node and before setting
> bs_opaque->job = NULL; But AFAICS, we do not actually set
> bs_opaque->job->dirty_bitmap = NULL anywhere. So yes, the second part of
> the condition seems redundant to me too. I think the fact that we don't
> set bs_opaque->job->dirty_bitmap = NULL can't lead to a use-after-free
> either, because mirror_exit_common() happens while drained, because
> mirror_run() drains the source (and thus also the filter on top) before
> returning.
>
> Best Regards,
> Fiona
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;
Am 16.02.26 um 4:31 PM schrieb Kevin Wolf:
> 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.
You beat me to it :) Was thinking about this too, inspired by your
initial suggestion for the alternate approach to avoid relying on the
block layer for dirty tracking, but mine ended up needlessly involved,
because I removed the dirty_bitmap from the MirrorBlockJob object and
had to add a helper function for getting the dirty bitmap back from the
job, which is less than ideal :P
>
> 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) {
I put an assert(s->dirty_bitmap) here, but maybe it's not worth it?
> 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(
I created and disabled the dirty bitmap here already before the drained
section. It seems slightly more readable. Do you see any downside to that?
>
> 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.
> + */
This comment could become:
+ /*
+ * Disabling the dirty bitmap is safe, because:
+ *
+ * 1. If should_copy_to_target() == true, writes will be done
synchronously
+ * to the target, no need to set the bitmap.
+ *
+ * 2. If should_copy_to_target() == false, the dirty bitmap will be
set by
+ * bdrv_mirror_top_do_write().
+ */
> + 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;
>
>
Best Regards,
Fiona
Am 16.02.2026 um 17:12 hat Fiona Ebner geschrieben:
> Am 16.02.26 um 4:31 PM schrieb Kevin Wolf:
> > 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.
>
> You beat me to it :) Was thinking about this too, inspired by your
> initial suggestion for the alternate approach to avoid relying on the
> block layer for dirty tracking, but mine ended up needlessly involved,
> because I removed the dirty_bitmap from the MirrorBlockJob object and
> had to add a helper function for getting the dirty bitmap back from the
> job, which is less than ideal :P
> >
> > 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) {
>
> I put an assert(s->dirty_bitmap) here, but maybe it's not worth it?
The difference is SIGABRT here vs. SIGSEGV two lines later. I don't think
it matters much, either way is fine with me.
> > 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(
>
> I created and disabled the dirty bitmap here already before the drained
> section. It seems slightly more readable. Do you see any downside to that?
Hm... I think it's okay in practice because a drain comes after it, so
while there may be a race initially, we don't care when exactly we start
tracking as long as it's before checking the block status. But that's a
bit subtle.
We could keep creating the bitmap outside of the drain if that improves
things, but I would leave bdrv_disable_dirty_bitmap() inside the drained
section just to be very clear that there can't be a race with a request.
> >
> > 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.
> > + */
>
> This comment could become:
>
> + /*
> + * Disabling the dirty bitmap is safe, because:
> + *
> + * 1. If should_copy_to_target() == true, writes will be done
> synchronously
> + * to the target, no need to set the bitmap.
> + *
> + * 2. If should_copy_to_target() == false, the dirty bitmap will be
> set by
> + * bdrv_mirror_top_do_write().
> + */
Feels verbose to me because the implications of the existing comment
seem obvious to me, but if you think it's useful, it probably isn't as
obvious as I thought.
> > + 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;
Kevin
Am 16.02.26 um 5:38 PM schrieb Kevin Wolf:
> Am 16.02.2026 um 17:12 hat Fiona Ebner geschrieben:
>> Am 16.02.26 um 4:31 PM schrieb Kevin Wolf:
>>> 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.
>>
>> You beat me to it :) Was thinking about this too, inspired by your
>> initial suggestion for the alternate approach to avoid relying on the
>> block layer for dirty tracking, but mine ended up needlessly involved,
>> because I removed the dirty_bitmap from the MirrorBlockJob object and
>> had to add a helper function for getting the dirty bitmap back from the
>> job, which is less than ideal :P
>>>
>>> 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) {
>>
>> I put an assert(s->dirty_bitmap) here, but maybe it's not worth it?
>
> The difference is SIGABRT here vs. SIGSEGV two lines later. I don't think
> it matters much, either way is fine with me.
Right.
>>> 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(
>>
>> I created and disabled the dirty bitmap here already before the drained
>> section. It seems slightly more readable. Do you see any downside to that?
>
> Hm... I think it's okay in practice because a drain comes after it, so
> while there may be a race initially, we don't care when exactly we start
> tracking as long as it's before checking the block status. But that's a
> bit subtle.
>
> We could keep creating the bitmap outside of the drain if that improves
> things, but I would leave bdrv_disable_dirty_bitmap() inside the drained
> section just to be very clear that there can't be a race with a request.
Okay, agreed.
>>>
>>> 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.
>>> + */
>>
>> This comment could become:
>>
>> + /*
>> + * Disabling the dirty bitmap is safe, because:
>> + *
>> + * 1. If should_copy_to_target() == true, writes will be done
>> synchronously
>> + * to the target, no need to set the bitmap.
>> + *
>> + * 2. If should_copy_to_target() == false, the dirty bitmap will be
>> set by
>> + * bdrv_mirror_top_do_write().
>> + */
>
> Feels verbose to me because the implications of the existing comment
> seem obvious to me, but if you think it's useful, it probably isn't as
> obvious as I thought.
The thing that might not be immediately obvious is that "active mode"
only starts once the job reference is set in the opaque object.
>>> + 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;
Are you going to send this as a proper patch?
Best Regards,
Fiona
Am 12.02.26 um 1:07 PM schrieb Fiona Ebner:
> Cc: qemu-stable@nongnu.org
> Closes: https://gitlab.com/qemu-project/qemu/-/issues/3273
Sorry, forgot to add
Fixes: 058cfca564 ("block/mirror: move dirty bitmap to filter")
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
© 2016 - 2026 Red Hat, Inc.