On Thu, 1 May 2025 12:58:42 -0500, Eric wrote:
> 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.
Correct. The pre-zero and skip-zero optimizations are only applicable
when sync == MIRROR_SYNC_MODE_FULL. In MIRROR_SYNC_MODE_TOP scenarios,
where the source snapshot chain (excluding the top snapshot) is pre-copied
to the target, the destination image is not an all-zero image. Consequently,
we can neither perform pre-zeroing nor skip-zero operations in this case.
if (sync == MIRROR_SYNC_MODE_TOP) {
/* No optimization is needed. */
} else {
/* sync == MIRROR_SYNC_MODE_FULL */
if (mode == NEW_IMAGE_MODE_EXISTING) {
if (target_is_zero) {
bitmap_set(zero_bitmap)
} else {
pre_zeroing;
bitmap_set(zero_bitmap);
}
} else {
if (bdrv_has_zero_init(target_bs)) {
bitmap_set(zero_bitmap);
} else {
pre_zeroing;
bitmap_set(zero_bitmap);
}
}
}
We list all possible scenarios, where some code branches can be merged
during implementation.
