Am 04.03.26 um 2:02 PM schrieb Kevin Wolf:
> Am 02.03.2026 um 13:54 hat Fiona Ebner geschrieben:
>> 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:
>
> No, I don't remember. Maybe some mailing list archaeology could help,
> but I'm not planning to do that right now.
>
>>> 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.
>
> Yes, I think this would be the expected behaviour.
>
>> 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?
>
> What would the surprise be? That the filters are kept instead of being
> replaced? But wouldn't it be inconsistent then that the same setup works
> if it's a root node?
>
> I suppose what would have been consistent is if BlockBackend names kept
> implicit filters, but node names didn't. It's too late for that, though.
I was thinking along those lines, but agreed, it is too late for that.
>> 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 :)
>
> That sounds promising.
>
> Given that the 11.0 freeze is on Tuesday, do we still want to move
> forward with a v2 of this patch, or do you think we should rather focus
> on making throttle nodes usable in all of the cases we care about, even
> if that means delaying it to 11.1?
I'd like to do the latter. The users of qmp_get_root_bs() where having a
throttle node on top might potentially be an issue are:
qmp_blockdev_snapshot_delete_internal_sync() and
internal_snapshot_action() - these are using bdrv_snapshot_fallback() to
skip filters on top.
qmp_block_commit() - here it's already possible to specific a dedicated
top node for the operation and not use the filter.
qmp_{blockdev,drive}_mirror() - which is the case I'm already looking into.
qmp_change_backing_file() - operates on the specified image-node-name.
So it seems mirror is the only place where a change is actually needed?
Best Regards,
Fiona
