On Wed, May 14, 2025 at 05:09:14PM -0500, Eric Blake wrote:
> On Fri, May 09, 2025 at 03:40:26PM -0500, Eric Blake wrote:
> > The two callers to a mirror job (drive-mirror and blockdev-mirror) set
> > zero_target precisely when sync mode == FULL, with the one exception
> > that drive-mirror skips zeroing the target if it was newly created and
> > reads as zero. But given the previous patch, that exception is
> > equally captured by target_is_zero. And since we recently updated
> > things to pass the sync mode all the way through to
> > mirror_dirty_init(), we can now reconstruct the same conditionals
> > without the redundant parameter.
> >
> > Signed-off-by: Eric Blake <ebl...@redhat.com>
> >
> > ---
> >
> > v4: new patch
>
> I was about to send the pull request, and noticed that this patch
> reliably makes iotest 185 fail:
>
> {"return": {}}
> -{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event":
> "BLOCK_JOB_READY", "data": {"device": "mirror", "len": 0, "offset": 0,
> "speed": 0, "type": "mirror"}}
> ---- Writing data to the virtio-blk device ---
> ...
> -*** done
> +Timeout waiting for event BLOCK_JOB_READY on handle 5
> Failures: 185
>
>
> Investigating now...
Oh, that was extremely subtle.
Pre-patch, zero_target is set to false while sync == SYNC_TOP prior to
calling blockdev.c:blockdev_mirror_common(). But in that function, we
were forcefully slamming sync to MIRROR_SYNC_MODE_FULL if
bdrv_backing_chain_next(bs) == NULL. Once zero_target is no longer
available, that means any mirror job using "sync":"top" but with no
backing image (which is effectively syncing the entire chain) now
defaults to pre-zeroing - and since 185 intentionally throttles
things, the newly-triggered pre-zeroing takes so long that the test
times out.
Fortunately, an examination of all places in mirror.c that care about
sync_mode TOP vs FULL shows that prior to this series, they were
mostly treated identically everywhere (only zero_target mattered)
thanks to the is_none_mode bool. The lone exception was in
mirror_start, with this statement:
base = mode == MIRROR_SYNC_MODE_TOP ? bdrv_backing_chain_next(bs) : NULL;
but we know that the caller slammed mode to MODE_FULL precisely if it
used to be TOP and bdrv_backing_chain_next(bs) == NULL. So if the
caller doesn't slam sync to FULL, this ternary still sets base to the
same value. And we are back to skipping the pre-zeroing pass for
"sync":"top", the way test 185 wants.
Hence, I'm squashing this in, and preparing the pull request.
diff --git i/blockdev.c w/blockdev.c
index 93d403d8210..21443b45144 100644
--- i/blockdev.c
+++ w/blockdev.c
@@ -2871,10 +2871,6 @@ static void blockdev_mirror_common(const char *job_id,
BlockDriverState *bs,
return;
}
- if (!bdrv_backing_chain_next(bs) && sync == MIRROR_SYNC_MODE_TOP) {
- sync = MIRROR_SYNC_MODE_FULL;
- }
-
if (!replaces) {
/* We want to mirror from @bs, but keep implicit filters on top */
unfiltered_bs = bdrv_skip_implicit_filters(bs);
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
