Am 26.02.26 um 5:31 PM schrieb Kevin Wolf: > Am 26.02.2026 um 16:20 hat Fiona Ebner geschrieben: >> Am 24.02.26 um 3:34 PM schrieb Kevin Wolf: >>> Am 16.01.2026 um 15:39 hat Fiona Ebner geschrieben: >>>> With '-drive', it is possible to specify the throttle configuration >>>> directly on the commandline. Add the possibility to do the same when >>>> using the modern way with '-blockdev' and a front-end device. Using a >>>> throttle filter block node is not always an option: in particular, the >>>> mirror block job operates on a root block node and it might be desired >>>> to throttle only the guest IO, but not to the block job. >>> >>> Hm, is there still a reason why we require a root node for the source?
Do you happen to know what the original reason for the restriction was? >From git history I cannot really see anything special, just: > commit 07eec652722f3d12b07c5b28d0671ddfc22fe6a5 > Author: Kevin Wolf <[email protected]> > Date: Thu Jun 23 14:20:24 2016 +0200 > > block: Accept node-name for blockdev-mirror > > In order to remove the necessity to use BlockBackend names in the > external API, we want to allow node-names everywhere. This converts > blockdev-mirror to accept a node-name without lifting the restriction > that we're operating at a root node. For operations like backup and stream, the same restriction was lifted: 930fe17f99 ("blockdev: enable non-root nodes for backup source") 554b614765 ("block: Add QMP support for streaming to an intermediate layer") We have the following code snippet in blockdev_mirror_common(): > if (!replaces) { > /* We want to mirror from @bs, but keep implicit filters on top */ > unfiltered_bs = bdrv_skip_implicit_filters(bs); > if (unfiltered_bs != bs) { > replaces = unfiltered_bs->node_name; > } > } Doing this as well for non-root nodes would still match the documentation: > # @replaces: with sync=full graph node name to be replaced by the new > # image when a whole image copy is done. This can be used to > # repair broken Quorum files. By default, @device is replaced, > # although implicitly created filters on it are kept. But I guess we could also do something like > diff --git a/blockdev.c b/blockdev.c > index 6e6932ddea..0ff058bed0 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2931,7 +2931,14 @@ static void blockdev_mirror_common(const char *job_id, > BlockDriverState *bs, > /* We want to mirror from @bs, but keep implicit filters on top */ > unfiltered_bs = bdrv_skip_implicit_filters(bs); > if (unfiltered_bs != bs) { > - replaces = unfiltered_bs->node_name; > + if (bdrv_is_root_node(bs)) { > + replaces = unfiltered_bs->node_name; > + } else { > + error_setg(errp, "Detected non-root block node which is an" > + " implicit filter, please specify @replaces" > + " parameter explicitly"); > + return; > + } > } > } > to avoid any surprises for people who explicitly specify a non-root implicit filter as the source? Or what do you think? Reading through the code, I don't see any other issues with using a non-root node as the source and an initial test case for a node below a throttle node seems to work fine too :) >> >> I'm not sure if the restriction could be lifted. But AFAICS, that >> doesn't help in my case with a throttle node as the root node: >> >> Say I'm mirroring the node below throttle and the job is ready to be >> completed. Further, assume that requests pile up for the root node, >> while the node below is mostly idle, which can easily happen with low >> throttle limits: >> >> At some moment, there might be no IO in-flight for the node below >> throttle and thus the mirror can complete, while all the in-flight >> requests for the throttle node are currently being intercepted by the >> throttle group and waiting for the timer to wake them. Because, the call >> to bdrv_inc_in_flight() for the node below throttle only happens after >> the intercepted requests are woken. Mirror and the node below do not >> know about the parent's in flight requests. Is this interpretation correct? > > Well, mirror doesn't just complete by itself, but let's assume it > already was ready and you told it to complete. Then yes, if there is no > more activity and both sides are in sync, the mirror job can complete. > > However, is this any different from a mirror job completing when the > guest has already put a request in the virtio-blk virtqueue, but the > device model hasn't processed it yet? From the guest's perspective, the > request is in flight in both cases, and from the mirror job's > perspective it hasn't started yet in both cases. > > I think the only difference is that with throttling you might hit this > case more often, but it's not a case that is fundamentally impossible > without throttling. > >> There are scenarios where we finish the job via block-job-cancel after >> freezing the guest filesystem to ensure consistency, so having >> intercepted requests as described above would mess it up. > > Why? You got a copy that was in sync at the time that the mirror job > completed. The job doesn't promise the exact point of time that the copy > matches, it's just some arbitrary time after you request completion when > both sides are in sync. This implies that requests after this arbitrary > point in time will not be included in the copy. > > Though I'm also wondering, if you successfully froze the file system (as > opposed to still trying to freeze it), why are we getting write > requests? Shouldn't freezing involve waiting for completion of all > in-flight requests? Ah, yes, you're right! There won't be any in-flight requests anymore once it's frozen. The throttling just means that the guest has to wait longer for completion first. Also, the parent throttle node is drained too when the mirror finishes :) Best Regards, Fiona
