On Wed, May 14, 2025 at 11:37:54PM +0800, Sunny Zhu wrote:
> On Fri, 9 May 2025 15:40:28 -0500, Eric Blake wrote:
> > @@ -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);
>
> It's ok when ret > 0 is obtained through bdrv_co_is_all_zeroes(target_bs).
> However,
> if ret = 1 originates by target-is-zero == true from qmp_blockdev_mirror,
> this means
> that target-is-zero also takes effect under sync=TOP. I am uncertain whether
> this
> aligns with our intended behavior.
>
> Under sync=TOP, target-is-zero could carry two distinct meanings:
> [1] The top snapshot reads as fully zero.
> [2] The entire destination snapshot chain reads as fully zero.
>
> Currently, target-is-zero is designed to represent scenario [2] on sync=TOP.
As written, we are telling the user they should not set target-is-zero
unless the entire destination (including through any snapshot chain)
reads as zero. Offhand (but untested) I think you can get by with
passing target-is-zero:true even when the destination is not fully
zero, _IF_ all portions of the image that will be mirrored read as
zero (and by doing that, the portions that will not be mirrored are
not cleared as a result). If my assumption is plausible, that would
be a slick way to do a sync=TOP mirror to copy just the dirty portions
of an image into a destination that already contains all contents from
the clean portions of the image prior to the point in time of the
dirty bitmap, in order to rehydrate a full image from an incremental
mirror. But given that it is not tested, I wouldn't recommend trying
it in a production environment.
I doubt that sync=TOP users will ever want to manually set
target-is-zero:true, but don't see any reason to change the series as
reviewed, because the documentation for target-is-zero was clear that
that manually setting it when the image is not actually zero can
possibly cause corruption (even if it ends up working without
corruption in some corner cases).
Meanwhile, the only effect of having the zero bitmap set when
target-is-zero was passed to sync=TOP (even if that is not what most
users will do) is that _if_ the portion of the image that is dirty and
needs to be mirrored reads as zero on the source, it will not be
written to the destination. It doesn't punch holes in the portions of
the destionation that correspond to unallocated bits of the source.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
