On Thu, 1 May 2025 12:33:14 -0500, Eric wrote:
> > > + * If the destination already reads as zero, and we are not
> > > + * requested to punch holes into existing zeroes, then we can
> > > + * skip pre-zeroing the destination.
> > > + */
> > > + if (ret > 0 &&
> > > + (target_bs->detect_zeroes !=
> > > BLOCKDEV_DETECT_ZEROES_OPTIONS_UNMAP ||
> > > + !bdrv_can_write_zeroes_with_unmap(target_bs))) {
> > > + offset = s->bdev_length;
> >
> > If when bdrv_can_write_zeroes_with_unmap(target_bs) == true, we prefer to
> > punch holes regardless of whether target_bs already reads as zero, then
> > execute bdrv_co_is_all_zeroes() in advance might be wasteful.
>
> Hmm. bdrv_co_is_all_zeroes() is supposed to be fast, but you're right
> that even faster than a syscall or two is no syscalls at all.
Indeed, bdrv_co_is_all_zeroes() is supposed to be fast, and since we are on the
management plane rather than the data plane, the impact here should be
negligible.
>
> >
> > if (bdrv_can_write_zeroes_with_unmap(target_bs)) {
> > initial_zeroing();
> > } else {
> > ret = bdrv_co_is_all_zeroes(target_bs);
> > ...
> > }
>
> That's a bigger refactoring; probably worth doing in a separate patch.
> It looks like I should probably do a v4 along those lines, to see how
> it compares.
>
> >
> > > + }
> > > if (!bdrv_can_write_zeroes_with_unmap(target_bs)) {
> > > bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length);
> > > return 0;
> >
> > When ret > 0, We should not return directly here.
>
> That's pre-existing, but correct. If we can't write zeroes with
> unmap, then we mark the entire image dirty (every zero in the source
> will result in zeroes in the dest - as it was before this series); and
> we have also marked the zero bitmap (writing zeroes will be a no-op if
> the zero bitmap says that is safe). The rest of this function has two
> purposes: finish pre-zeroing (well, there's nothing to pre-zero if we
> can't punch holes, and especially nothing to pre-zero if we already
> know the image reads as all zero), and populate the dirty bitmap (we
> just populated the entire map here, so it's not worth trying to
> populate the map with finer granularity later), so returning here is
> the right thing to do.
I apologize that my previous explanation missed the key point. What I meant is:
when bdrv_co_is_all_zeroes() > 0 &&
!bdrv_can_write_zeroes_with_unmap(target_bs),
we set offset = s->bdev_length;
However here, when !bdrv_can_write_zeroes_with_unmap(target_bs) is true, we
directly
return, which means that offset = s->bdev_length (i.e., our logic to skip
pre-zeroing
the destination) will never take effect.
The appropriate modification here should be:
if (!ret && !bdrv_can_write_zeroes_with_unmap(target_bs)) {
bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length);
return 0;
>
> >
> > > }
> > >
> > > s->initial_zeroing_ongoing = true;
> > > - for (offset = 0; offset < s->bdev_length; ) {
> > > + while (offset < s->bdev_length) {
> > > int bytes = MIN(s->bdev_length - offset,
> > > QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
> >
>
