On Fri, May 09, 2025 at 03:40:28PM -0500, Eric Blake wrote:
> When mirroring, the goal is to ensure that the destination reads the
> same as the source; this goal is met whether the destination is sparse
> or fully-allocated (except when explicitly punching holes, then merely
> reading zero is not enough to know if it is sparse, so we still want
> to punch the hole). Avoiding a redundant write to zero (whether in
> the background because the zero cluster was marked in the dirty
> bitmap, or in the foreground because the guest is writing zeroes) when
> the destination already reads as zero makes mirroring faster, and
> avoids allocating the destination merely because the source reports as
> allocated.
I just realized: when we are trying to punch holes, a pre-zeroing pass
still does double I/O on portions of the source that are allocated
(once to punch the hole, another to write it back to non-zero). That
was the best earlier patches could do, without fine-grained zero
tracking. But with this patch, we can do better:
> @@ -847,8 +887,10 @@ static int coroutine_fn GRAPH_UNLOCKED
> mirror_dirty_init(MirrorBlockJob *s)
> bool punch_holes =
> target_bs->detect_zeroes == BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP &&
> bdrv_can_write_zeroes_with_unmap(target_bs);
> + int64_t bitmap_length = DIV_ROUND_UP(s->bdev_length, s->granularity);
>
> /* Determine if the image is already zero, regardless of sync mode. */
> + s->zero_bitmap = bitmap_new(bitmap_length);
> bdrv_graph_co_rdlock();
> bs = s->mirror_top_bs->backing->bs;
> if (s->target_is_zero) {
> @@ -862,7 +904,14 @@ static int coroutine_fn GRAPH_UNLOCKED
> mirror_dirty_init(MirrorBlockJob *s)
> if (ret < 0) {
> return ret;
> } else if (s->sync_mode == MIRROR_SYNC_MODE_TOP) {
> - /* In TOP mode, there is no benefit to a pre-zeroing pass. */
> + /*
> + * In TOP mode, there is no benefit to a pre-zeroing pass, but
> + * the zero bitmap can be set if the destination already reads
> + * as zero and we are not punching holes.
> + */
> + if (ret > 0 && !punch_holes) {
> + bitmap_set(s->zero_bitmap, 0, bitmap_length);
> + }
> } else if (ret == 0 || punch_holes) {
> /*
> * Here, we are in FULL mode; our goal is to avoid writing
> @@ -871,8 +920,9 @@ static int coroutine_fn GRAPH_UNLOCKED
> mirror_dirty_init(MirrorBlockJob *s)
> * zeroing happened externally (ret > 0) or if we have a fast
> * way to pre-zero the image (the dirty bitmap will be
> * populated later by the non-zero portions, the same as for
> - * TOP mode). If pre-zeroing is not fast, or we need to punch
> - * holes, then our only recourse is to write the entire image.
> + * TOP mode). If pre-zeroing is not fast, then our only
> + * recourse is to mark the entire image dirty. The act of
> + * pre-zeroing will populate the zero bitmap.
> */
> if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
> bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length);
If we are punching holes, leaving the zero bitmap clear (at this
stage) and setting the entire dirty bitmap will cause the entire image
to be visited, and punch holes only where they are needed, without a
pre-zeroing pass.
I will add that as a followup patch (no need to invalidate the reviews
on this one).
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
