* [PATCH] hw/block/block: add 'throttle-group' property
@ 2026-01-16 14:39 Fiona Ebner
2026-02-10 9:22 ` Fiona Ebner
2026-02-24 14:34 ` Kevin Wolf
0 siblings, 2 replies; 8+ messages in thread
From: Fiona Ebner @ 2026-01-16 14:39 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, hreitz, kwolf, jsnow
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.
There already is a 'block_set_io_throttle' QMP command, but it's nicer
to be able to do it via the commandline too.
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
Hope I didn't miss a way to do this already.
Should changing via qom-set be supported? Currently, an attempt fails:
> Error: Attempt to set property 'throttle-group' on device 'scsi0'
> (type 'scsi-hd') after it was realized
but there already is the 'block_set_io_throttle' QMP command.
hw/block/block.c | 15 +++++++++++++++
include/hw/block/block.h | 4 +++-
tests/qemu-iotests/172.out | 38 ++++++++++++++++++++++++++++++++++++++
3 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/hw/block/block.c b/hw/block/block.c
index f187fa025d..1d70acdb76 100644
--- a/hw/block/block.c
+++ b/hw/block/block.c
@@ -254,6 +254,21 @@ bool blkconf_apply_backend_options(BlockConf *conf, bool readonly,
conf->num_stats_intervals, errp)) {
return false;
}
+
+ if (conf->throttle_group) {
+ if (!throttle_group_exists(conf->throttle_group)) {
+ error_setg(errp, "Throttle group '%s' not found",
+ conf->throttle_group);
+ return false;
+ }
+ if (blk_get_public(blk)->throttle_group_member.throttle_state) {
+ error_setg(errp, "Cannot set throttle group, because there already"
+ " is a throttle configuration (via '-drive'?)");
+ return false;
+ }
+ blk_io_limits_enable(blk, conf->throttle_group);
+ }
+
return true;
}
diff --git a/include/hw/block/block.h b/include/hw/block/block.h
index 7dc19d8a45..5565b32e62 100644
--- a/include/hw/block/block.h
+++ b/include/hw/block/block.h
@@ -36,6 +36,7 @@ typedef struct BlockConf {
BlockdevOnError werror;
uint32_t num_stats_intervals;
uint32_t *stats_intervals;
+ char *throttle_group;
} BlockConf;
static inline unsigned int get_physical_block_exp(BlockConf *conf)
@@ -71,7 +72,8 @@ static inline unsigned int get_physical_block_exp(BlockConf *conf)
_conf.account_failed, ON_OFF_AUTO_AUTO), \
DEFINE_PROP_ARRAY("stats-intervals", _state, \
_conf.num_stats_intervals, _conf.stats_intervals, \
- qdev_prop_uint32, uint32_t)
+ qdev_prop_uint32, uint32_t), \
+ DEFINE_PROP_STRING("throttle-group", _state, _conf.throttle_group)
#define DEFINE_BLOCK_PROPERTIES(_state, _conf) \
DEFINE_PROP_DRIVE("drive", _state, _conf.blk), \
diff --git a/tests/qemu-iotests/172.out b/tests/qemu-iotests/172.out
index a023cd407d..368c411dcf 100644
--- a/tests/qemu-iotests/172.out
+++ b/tests/qemu-iotests/172.out
@@ -31,6 +31,7 @@ Testing:
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "288"
@@ -61,6 +62,7 @@ Testing: -fda TEST_DIR/t.qcow2
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/unattached/device[N]
@@ -98,6 +100,7 @@ Testing: -fdb TEST_DIR/t.qcow2
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
dev: floppy, id ""
unit = 0 (0x0)
@@ -113,6 +116,7 @@ Testing: -fdb TEST_DIR/t.qcow2
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "288"
floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/unattached/device[N]
@@ -154,6 +158,7 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
dev: floppy, id ""
unit = 0 (0x0)
@@ -169,6 +174,7 @@ Testing: -fda TEST_DIR/t.qcow2 -fdb TEST_DIR/t.qcow2.2
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/unattached/device[N]
@@ -211,6 +217,7 @@ Testing: -fdb
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "288"
dev: floppy, id ""
unit = 0 (0x0)
@@ -226,6 +233,7 @@ Testing: -fdb
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "288"
@@ -256,6 +264,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/unattached/device[N]
@@ -293,6 +302,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
dev: floppy, id ""
unit = 0 (0x0)
@@ -308,6 +318,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2,index=1
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "288"
floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/unattached/device[N]
@@ -349,6 +360,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
dev: floppy, id ""
unit = 0 (0x0)
@@ -364,6 +376,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=floppy,file=TEST_DIR/t
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/unattached/device[N]
@@ -409,6 +422,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/peripheral-anon/device[N]
@@ -446,6 +460,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,unit=1
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/peripheral-anon/device[N]
@@ -483,6 +498,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
dev: floppy, id ""
unit = 0 (0x0)
@@ -498,6 +514,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qco
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/peripheral-anon/device[N]
@@ -549,6 +566,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
dev: floppy, id ""
unit = 0 (0x0)
@@ -564,6 +582,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/unattached/device[N]
@@ -606,6 +625,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
dev: floppy, id ""
unit = 0 (0x0)
@@ -621,6 +641,7 @@ Testing: -fda TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/unattached/device[N]
@@ -663,6 +684,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
dev: floppy, id ""
unit = 1 (0x1)
@@ -678,6 +700,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/unattached/device[N]
@@ -720,6 +743,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
dev: floppy, id ""
unit = 1 (0x1)
@@ -735,6 +759,7 @@ Testing: -fdb TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.qcow2.2 -device fl
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
floppy1 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/unattached/device[N]
@@ -786,6 +811,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
dev: floppy, id ""
unit = 0 (0x0)
@@ -801,6 +827,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/unattached/device[N]
@@ -843,6 +870,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
dev: floppy, id ""
unit = 0 (0x0)
@@ -858,6 +886,7 @@ Testing: -drive if=floppy,file=TEST_DIR/t.qcow2 -drive if=none,file=TEST_DIR/t.q
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
floppy0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/unattached/device[N]
@@ -906,6 +935,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -global floppy.drive=none0 -device
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/peripheral-anon/device[N]
@@ -973,6 +1003,7 @@ Testing: -device floppy
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "288"
Testing: -device floppy,drive-type=120
@@ -1000,6 +1031,7 @@ Testing: -device floppy,drive-type=120
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "120"
Testing: -device floppy,drive-type=144
@@ -1027,6 +1059,7 @@ Testing: -device floppy,drive-type=144
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
Testing: -device floppy,drive-type=288
@@ -1054,6 +1087,7 @@ Testing: -device floppy,drive-type=288
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "288"
@@ -1084,6 +1118,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "120"
none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/peripheral-anon/device[N]
@@ -1121,6 +1156,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,drive-t
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "288"
none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/peripheral-anon/device[N]
@@ -1161,6 +1197,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,logical
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/peripheral-anon/device[N]
@@ -1198,6 +1235,7 @@ Testing: -drive if=none,file=TEST_DIR/t.qcow2 -device floppy,drive=none0,physica
account-invalid = "auto"
account-failed = "auto"
stats-intervals = <null>
+ throttle-group = ""
drive-type = "144"
none0 (NODE_NAME): TEST_DIR/t.qcow2 (qcow2)
Attached to: /machine/peripheral-anon/device[N]
--
2.47.3
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/block/block: add 'throttle-group' property
2026-01-16 14:39 [PATCH] hw/block/block: add 'throttle-group' property Fiona Ebner
@ 2026-02-10 9:22 ` Fiona Ebner
2026-02-24 14:34 ` Kevin Wolf
1 sibling, 0 replies; 8+ messages in thread
From: Fiona Ebner @ 2026-02-10 9:22 UTC (permalink / raw)
To: qemu-devel; +Cc: qemu-block, hreitz, kwolf, jsnow
Am 16.01.26 um 5:41 PM schrieb Fiona Ebner:
> 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.
>
> There already is a 'block_set_io_throttle' QMP command, but it's nicer
> to be able to do it via the commandline too.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> Hope I didn't miss a way to do this already.
>
> Should changing via qom-set be supported? Currently, an attempt fails:
>> Error: Attempt to set property 'throttle-group' on device 'scsi0'
>> (type 'scsi-hd') after it was realized
> but there already is the 'block_set_io_throttle' QMP command.
Ping
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/block/block: add 'throttle-group' property
2026-01-16 14:39 [PATCH] hw/block/block: add 'throttle-group' property Fiona Ebner
2026-02-10 9:22 ` Fiona Ebner
@ 2026-02-24 14:34 ` Kevin Wolf
2026-02-26 15:20 ` Fiona Ebner
1 sibling, 1 reply; 8+ messages in thread
From: Kevin Wolf @ 2026-02-24 14:34 UTC (permalink / raw)
To: Fiona Ebner; +Cc: qemu-devel, qemu-block, hreitz, jsnow
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?
> There already is a 'block_set_io_throttle' QMP command, but it's nicer
> to be able to do it via the commandline too.
>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> Hope I didn't miss a way to do this already.
>
> Should changing via qom-set be supported? Currently, an attempt fails:
> > Error: Attempt to set property 'throttle-group' on device 'scsi0'
> > (type 'scsi-hd') after it was realized
> but there already is the 'block_set_io_throttle' QMP command.
It would be nice, but I don't think runtime writable properties are used
much in devices because I believe this would have to be done with plain
QOM properties, i.e. outside of qdev.
Though if it's actually easy, we could consider implementing it.
> hw/block/block.c | 15 +++++++++++++++
> include/hw/block/block.h | 4 +++-
> tests/qemu-iotests/172.out | 38 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 56 insertions(+), 1 deletion(-)
Looks good to me, though maybe we could also use a qemu-iotests case
that actually tries to set the property instead of only updating the
output everywhere for having no throttle group.
Kevin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/block/block: add 'throttle-group' property
2026-02-24 14:34 ` Kevin Wolf
@ 2026-02-26 15:20 ` Fiona Ebner
2026-02-26 16:31 ` Kevin Wolf
0 siblings, 1 reply; 8+ messages in thread
From: Fiona Ebner @ 2026-02-26 15:20 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, hreitz, jsnow
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?
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?
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.
>> There already is a 'block_set_io_throttle' QMP command, but it's nicer
>> to be able to do it via the commandline too.
>>
>> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
>> ---
>>
>> Hope I didn't miss a way to do this already.
>>
>> Should changing via qom-set be supported? Currently, an attempt fails:
>>> Error: Attempt to set property 'throttle-group' on device 'scsi0'
>>> (type 'scsi-hd') after it was realized
>> but there already is the 'block_set_io_throttle' QMP command.
>
> It would be nice, but I don't think runtime writable properties are used
> much in devices because I believe this would have to be done with plain
> QOM properties, i.e. outside of qdev.
>
> Though if it's actually easy, we could consider implementing it.
>
>> hw/block/block.c | 15 +++++++++++++++
>> include/hw/block/block.h | 4 +++-
>> tests/qemu-iotests/172.out | 38 ++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 56 insertions(+), 1 deletion(-)
>
> Looks good to me, though maybe we could also use a qemu-iotests case
> that actually tries to set the property instead of only updating the
> output everywhere for having no throttle group.
Ack, I'll send a v2 with a test.
Best Regards,
Fiona
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/block/block: add 'throttle-group' property
2026-02-26 15:20 ` Fiona Ebner
@ 2026-02-26 16:31 ` Kevin Wolf
2026-03-02 12:54 ` Fiona Ebner
0 siblings, 1 reply; 8+ messages in thread
From: Kevin Wolf @ 2026-02-26 16:31 UTC (permalink / raw)
To: Fiona Ebner; +Cc: qemu-devel, qemu-block, hreitz, jsnow
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?
>
> 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?
Kevin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/block/block: add 'throttle-group' property
2026-02-26 16:31 ` Kevin Wolf
@ 2026-03-02 12:54 ` Fiona Ebner
2026-03-04 13:02 ` Kevin Wolf
0 siblings, 1 reply; 8+ messages in thread
From: Fiona Ebner @ 2026-03-02 12:54 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, hreitz, jsnow
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 <kwolf@redhat.com>
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/block/block: add 'throttle-group' property
2026-03-02 12:54 ` Fiona Ebner
@ 2026-03-04 13:02 ` Kevin Wolf
2026-03-04 14:08 ` Fiona Ebner
0 siblings, 1 reply; 8+ messages in thread
From: Kevin Wolf @ 2026-03-04 13:02 UTC (permalink / raw)
To: Fiona Ebner; +Cc: qemu-devel, qemu-block, hreitz, jsnow
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 <kwolf@redhat.com>
> > 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.
> 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?
Kevin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/block/block: add 'throttle-group' property
2026-03-04 13:02 ` Kevin Wolf
@ 2026-03-04 14:08 ` Fiona Ebner
0 siblings, 0 replies; 8+ messages in thread
From: Fiona Ebner @ 2026-03-04 14:08 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block, hreitz, jsnow
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 <kwolf@redhat.com>
>>> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2026-03-04 14:09 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-01-16 14:39 [PATCH] hw/block/block: add 'throttle-group' property Fiona Ebner
2026-02-10 9:22 ` Fiona Ebner
2026-02-24 14:34 ` Kevin Wolf
2026-02-26 15:20 ` Fiona Ebner
2026-02-26 16:31 ` Kevin Wolf
2026-03-02 12:54 ` Fiona Ebner
2026-03-04 13:02 ` Kevin Wolf
2026-03-04 14:08 ` Fiona Ebner
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.