On Thu, May 01, 2025 at 12:38:30AM +0800, Sunny Zhu wrote:
> on Thu 24 Apr 2025 19:52:08 -0500, Eric wrote:
> > if (s->zero_target) {
> > + int64_t bitmap_length = DIV_ROUND_UP(s->bdev_length,
> > s->granularity);
> > +
> > offset = 0;
> > bdrv_graph_co_rdlock();
> > ret = bdrv_co_is_all_zeroes(target_bs);
> > @@ -856,6 +898,7 @@ static int coroutine_fn GRAPH_UNLOCKED
> > mirror_dirty_init(MirrorBlockJob *s)
> > if (ret < 0) {
> > return ret;
> > }
> > + s->zero_bitmap = bitmap_new(bitmap_length);
> > /*
> > * If the destination already reads as zero, and we are not
> > * requested to punch holes into existing zeroes, then we can
> > @@ -864,6 +907,7 @@ static int coroutine_fn GRAPH_UNLOCKED
> > mirror_dirty_init(MirrorBlockJob *s)
> > if (ret > 0 &&
> > (target_bs->detect_zeroes !=
> > BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP ||
> > !bdrv_can_write_zeroes_with_unmap(target_bs))) {
> > + bitmap_set(s->zero_bitmap, 0, bitmap_length);
>
> when arg->mode != NEW_IMAGE_MODE_EXISTING && bdrv_has_zero_init(target_bs) is
> true
> in drive_mirror (This means the target image is newly created), in which case
> s->zero_target == false, we still need to execute bitmap_set(s->zero_bitmap,
> 0, bitmap_length)
Good catch. I will fix that in v4.
Now that I'm thinking a bit more about it, I wonder if s->zero_target
is the right name. We have several pieces of information feeding the
potential optimizations: are we mirroring the full image or just a
portion of it (if just a portion, pre-zeroing is wrong because we
shouldn't touch the part of the image not being mirrored), did we just
create the image (in which case it reads as zero and we can skip
pre-zeroing), did the user pass target-is-zero (later in this series,
same effect as if we just created the image), is punching zeroes
allowed (not all setups allow it; and even when it is allowed, there
are tradeoffs on whether to punch one big hole and then fill back up
or to only punch small holes as zeroes are encountered).
It's a big mess of conditionals, so I'm still trying to figure out if
there is a more sensible way to arrange the various logic bits. With
the addition of target-is-zero, maybe it makes more sense to rename
s->zero_target to s->full_image, and to set s->full_image=true
s->target_is_zero in the arg->mode != NEW_IMAGE_MODE_EXISTING &&
bdrv_has_zero_init(target) case.
>
> > offset = s->bdev_length;
> > }
> > if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
> > @@ -875,6 +919,7 @@ static int coroutine_fn GRAPH_UNLOCKED
> > mirror_dirty_init(MirrorBlockJob *s)
> > while (offset < s->bdev_length) {
> > int bytes = MIN(s->bdev_length - offset,
> > QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
