On Tue, Jul 02, 2024 at 16:11:03 +0200, Peter Krempa wrote:

On Wed, Jun 12, 2024 at 03:02:10 -0700, wucf@linux.ibm.com wrote:

...

From: Chun Feng Wu <wucf@linux.ibm.com>

* Add new elements '<throttlefilters>' * <ThrottleFilters> can include multiple throttlegroup references to form filter chain in qemu * Chained throttle filters feature in qemu is described at https://github.com/qemu/qemu/blob/master/docs/throttle.txt

Signed-off-by: Chun Feng Wu <wucf@linux.ibm.com> --- docs/formatdomain.rst | 22 ++++++++++++++++++++++ src/conf/schemas/domaincommon.rng | 19 ++++++++++++++++++- 2 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/docs/formatdomain.rst b/docs/formatdomain.rst index b7e1f9cc83..0fa8f1267c 100644 --- a/docs/formatdomain.rst +++ b/docs/formatdomain.rst @@ -2736,6 +2736,15 @@ paravirtualized driver is specified via the ``disk`` element. <source dev='/dev/vhost-vdpa-0' /> <target dev='vdg' bus='virtio'/> </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='qcow2' /> + <source file='/var/lib/libvirt/images/disk.qcow2'/> + <target dev='vdh' bus='virtio'/> + <throttlefilters> + <throttlefilter group='limit2'/> + <throttlefilter group='limit012'/> + </throttlefilters> + </disk> </devices> ...

@@ -3217,6 +3226,19 @@ paravirtualized driver is specified via the ``disk`` element. :since:`since after 0.4.4`; "sata" attribute value :since:`since 0.9.7`; "removable" attribute value :since:`since 1.1.3`; "rotation_rate" attribute value :since:`since 7.3.0` +``throttlefilters`` + The optional ``throttlefilters`` element provides the ability to provide additional + per-device throttle chain :since:`Since 10.5.0` + For example, if we have four different disks and we want to limit I/O for each one + and we also want to limit combined I/O of all four disks, we can leverage + ``throttlefilters`` to achieve this goal by setting two ``throttlefilter`` for + each disk: disk's own filter(e.g. limit2) and combined filter(e.g. limit012).

...

+ The nodes in qemu shape a chain like libvirt-4-filter(node name of "limit012") -> + libvirt-3-filter(node name of "limit2") -> libvirt-2-format -> libvirt-1-storage. + ``throttlefilters`` and ``iotune`` should be used exclusively.

Node names are a qemu driver internal implementation detail and thus must not be noted in documentation.

I'm not exactly sure how the internals in qemu work here, but you also might want to document how the order of the filters impacts things (or that it does not impact things).

On Wed, Jun 12, 2024 at 03:02:13 -0700, wucf@linux.ibm.com wrote:

From: Chun Feng Wu <wucf@linux.ibm.com>

* ThrottleGroup is updated through "qemuMonitorJSONUpdateThrottleGroup" * ThrottleGroup is retrieved through "qemuMonitorJSONGetThrottleGroup" * ThrottleGroup is deleted by reusing "qemuMonitorDelObject" * ThrottleGroup is added by reusing "qemuMonitorAddObject" * "qemuMonitorMakeThrottleGroupLimits" will be used by building qemu cmd as well

Signed-off-by: Chun Feng Wu <wucf@linux.ibm.com> --- src/qemu/qemu_monitor.c | 34 ++++++++ src/qemu/qemu_monitor.h | 14 ++++ src/qemu/qemu_monitor_json.c | 150 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 14 ++++ 4 files changed, 212 insertions(+)

[...]

diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index c5e758e7f8..462b40cb6b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4633,6 +4633,156 @@ int qemuMonitorJSONGetBlockIoThrottle(qemuMonitor *mon, return qemuMonitorJSONBlockIoThrottleInfo(devices, qdevid, reply); }

+ +int +qemuMonitorMakeThrottleGroupLimits(virJSONValue *limits, + const virDomainThrottleGroupDef *group) +{ + if (virJSONValueObjectAdd(&limits, + "P:bps-total", + group->total_bytes_sec,

Please format both the string and the value on a single line (disregarding any line length "suggestions"):

+ "P:bps-read", group->read_bytes_sec, + "P:bps-write", group->write_bytes_sec,

like that

+ "P:iops-total", + group->total_iops_sec, + "P:iops-read", + group->read_iops_sec, + "P:iops-write", + group->write_iops_sec, + "P:bps-total-max", + group->total_bytes_sec_max, + "P:bps-read-max", + group->read_bytes_sec_max,

Please note that I'll be on hollidays, so the rest of the review will be delayed.

On Wed, Jun 12, 2024 at 03:02:13 -0700, wucf@linux.ibm.com wrote:

From: Chun Feng Wu <wucf@linux.ibm.com>

* ThrottleGroup is updated through "qemuMonitorJSONUpdateThrottleGroup" * ThrottleGroup is retrieved through "qemuMonitorJSONGetThrottleGroup" * ThrottleGroup is deleted by reusing "qemuMonitorDelObject" * ThrottleGroup is added by reusing "qemuMonitorAddObject" * "qemuMonitorMakeThrottleGroupLimits" will be used by building qemu cmd as well

Signed-off-by: Chun Feng Wu <wucf@linux.ibm.com> --- src/qemu/qemu_monitor.c | 34 ++++++++ src/qemu/qemu_monitor.h | 14 ++++ src/qemu/qemu_monitor_json.c | 150 +++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 14 ++++ 4 files changed, 212 insertions(+)

diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 34e2ccab97..2f067ab5d6 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2996,6 +2996,40 @@ qemuMonitorGetBlockIoThrottle(qemuMonitor *mon, }

+int +qemuMonitorThrottleGroupLimits(virJSONValue *limits, + const virDomainThrottleGroupDef *group) +{ + return qemuMonitorMakeThrottleGroupLimits(limits, group); +} + + +int +qemuMonitorUpdateThrottleGroup(qemuMonitor *mon, + const char *qomid, + virDomainBlockIoTuneInfo *info) +{ + VIR_DEBUG("qomid=%s, info=%p", qomid, info); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONUpdateThrottleGroup(mon, qomid, info); +} + + +int +qemuMonitorGetThrottleGroup(qemuMonitor *mon, + const char *groupname, + virDomainBlockIoTuneInfo *reply) +{ + VIR_DEBUG("throttle-group=%s, reply=%p", groupname, reply); + + QEMU_CHECK_MONITOR(mon); + + return qemuMonitorJSONGetThrottleGroup(mon, groupname, reply); +} + + int qemuMonitorVMStatusToPausedReason(const char *status) { diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index b78f539c85..6474ce124c 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -1062,6 +1062,20 @@ int qemuMonitorGetBlockIoThrottle(qemuMonitor *mon, const char *qdevid, virDomainBlockIoTuneInfo *reply);

+int +qemuMonitorThrottleGroupLimits(virJSONValue *limits, + const virDomainThrottleGroupDef *group); + +int +qemuMonitorUpdateThrottleGroup(qemuMonitor *mon, + const char *qomid, + virDomainBlockIoTuneInfo *info); + +int +qemuMonitorGetThrottleGroup(qemuMonitor *mon, + const char *groupname, + virDomainBlockIoTuneInfo *reply); + int qemuMonitorSystemWakeup(qemuMonitor *mon);

int qemuMonitorGetVersion(qemuMonitor *mon, diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index c5e758e7f8..462b40cb6b 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -4633,6 +4633,156 @@ int qemuMonitorJSONGetBlockIoThrottle(qemuMonitor *mon, return qemuMonitorJSONBlockIoThrottleInfo(devices, qdevid, reply); }

+ +int +qemuMonitorMakeThrottleGroupLimits(virJSONValue *limits, + const virDomainThrottleGroupDef *group) +{ + if (virJSONValueObjectAdd(&limits, + "P:bps-total", + group->total_bytes_sec, + "P:bps-read", + group->read_bytes_sec, + "P:bps-write", + group->write_bytes_sec, + "P:iops-total", + group->total_iops_sec, + "P:iops-read", + group->read_iops_sec, + "P:iops-write", + group->write_iops_sec, + "P:bps-total-max", + group->total_bytes_sec_max, + "P:bps-read-max", + group->read_bytes_sec_max, + "P:bps-write-max", + group->write_bytes_sec_max, + "P:iops-total-max", + group->total_iops_sec_max, + "P:iops-read-max", + group->read_iops_sec_max, + "P:iops-write-max", + group->write_iops_sec_max, + "P:iops-size", + group->size_iops_sec, + /* avoid error from QEMU: "the burst length cannot be 0" for throttlelimits + * when setting max-length + */ + "P:bps-total-max-length", + group->total_bytes_sec_max_length, + "P:bps-read-max-length", + group->read_bytes_sec_max_length, + "P:bps-write-max-length", + group->write_bytes_sec_max_length, + "P:iops-total-max-length", + group->total_iops_sec_max_length, + "P:iops-read-max-length", + group->read_iops_sec_max_length, + "P:iops-write-max-length", + group->write_iops_sec_max_length, + NULL) < 0) + return -1; + + return 0; +} + + +int +qemuMonitorJSONUpdateThrottleGroup(qemuMonitor *mon, + const char *qomid, + virDomainBlockIoTuneInfo *info) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) result = NULL; + g_autoptr(virJSONValue) limits = virJSONValueNewObject(); + + if (qemuMonitorMakeThrottleGroupLimits(limits, info)<0)

Formatting.

+ return -1; + + if (!(cmd = qemuMonitorJSONMakeCommand("qom-set", + "s:property", "limits", + "s:path", qomid, + "a:value", &limits, + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &result) < 0) + return -1; + + if (qemuMonitorJSONCheckError(cmd, result) < 0) + return -1; + + return 0; +} + + +#define GET_THROTTLE_GROUP_VALUE(FIELD, STORE) \ + if (virJSONValueObjectHasKey(ret, FIELD)) { \ + if (virJSONValueObjectGetNumberUlong(ret, FIELD, &reply->STORE) < 0) { \ + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, \ + _("throttle group field '%1$s' missing in qemu's output"), \

The error string is incorrect, because you check first whether the field exists, thus at this point it did exist but is malformed.

+ #STORE); \ + return -1; \ + } \ + } + + +int +qemuMonitorJSONGetThrottleGroup(qemuMonitor *mon, + const char *gname, + virDomainBlockIoTuneInfo *reply) +{ + g_autoptr(virJSONValue) cmd = NULL; + g_autoptr(virJSONValue) result = NULL; + g_autofree char *groupCopy = NULL; + virJSONValue *ret; + + g_autofree char *path = g_strdup_printf("/objects/%s", gname);

Note that since all objects live in one namespace in qemu you'll have to add a prefix to the group name so that the user will not be able to accidentaly pick a group name (which would be equivalent with the object 'id') which we might either be using or introduce in the future. Add a 'throttle-' prefix so that we clearly separate the throttle group objects into their own namespace. This will need to be done everywhere where you pass the throttle group name as object name to qemu.

+ if (!(cmd = qemuMonitorJSONMakeCommand("qom-get", + "s:path", path, + "s:property", "limits", + NULL))) + return -1; + + if (qemuMonitorJSONCommand(mon, cmd, &result) < 0) + return -1; + + if (qemuMonitorJSONCheckError(cmd, result) < 0) + return -1; + + if (!(ret = qemuMonitorJSONGetReply(cmd, result, VIR_JSON_TYPE_OBJECT))) + return -1; + + GET_THROTTLE_GROUP_VALUE("bps-total", total_bytes_sec); + GET_THROTTLE_GROUP_VALUE("bps-read", read_bytes_sec); + GET_THROTTLE_GROUP_VALUE("bps-write", write_bytes_sec); + GET_THROTTLE_GROUP_VALUE("iops-total", total_iops_sec); + GET_THROTTLE_GROUP_VALUE("iops-read", read_iops_sec); + GET_THROTTLE_GROUP_VALUE("iops-write", write_iops_sec); + + GET_THROTTLE_GROUP_VALUE("bps-total-max", total_bytes_sec_max); + GET_THROTTLE_GROUP_VALUE("bps-read-max", read_bytes_sec_max); + GET_THROTTLE_GROUP_VALUE("bps-write-max", write_bytes_sec_max); + GET_THROTTLE_GROUP_VALUE("iops-total-max", total_iops_sec_max); + GET_THROTTLE_GROUP_VALUE("iops-read-max", read_iops_sec_max); + GET_THROTTLE_GROUP_VALUE("iops-write-max", write_iops_sec_max); + GET_THROTTLE_GROUP_VALUE("iops-size", size_iops_sec); + + GET_THROTTLE_GROUP_VALUE("bps-total-max-length", total_bytes_sec_max_length); + GET_THROTTLE_GROUP_VALUE("bps-read-max-length", read_bytes_sec_max_length); + GET_THROTTLE_GROUP_VALUE("bps-write-max-length", write_bytes_sec_max_length); + GET_THROTTLE_GROUP_VALUE("iops-total-max-length", total_iops_sec_max_length); + GET_THROTTLE_GROUP_VALUE("iops-read-max-length", read_iops_sec_max_length); + GET_THROTTLE_GROUP_VALUE("iops-write-max-length", write_iops_sec_max_length); + + groupCopy = g_strdup(gname); + reply->group_name = g_steal_pointer(&groupCopy); + + return 0; +} +#undef GET_THROTTLE_GROUP_VALUE + + int qemuMonitorJSONSystemWakeup(qemuMonitor *mon) { g_autoptr(virJSONValue) cmd = NULL;

On Wed, Jun 12, 2024 at 03:02:15 -0700, wucf@linux.ibm.com wrote:

From: Chun Feng Wu <wucf@linux.ibm.com>

Defined new public APIs: * virDomainSetThrottleGroup to add or update throttlegroup within specific domain, it will be referenced by throttlefilter later in disk to do limits * virDomainGetThrottleGroup to get throttlegroup info, old-style is discarded(APIs to query first for the number of parameters and then give it a reasonably-sized pointer), instead, the new approach is adopted that API returns allocated array of fields and number of fileds that are in it. * virDomainDelThrottleGroup to delete throttlegroup, it fails if this throttlegroup is still referenced by some throttlefilter

Please use shorter lines in commit message.

Signed-off-by: Chun Feng Wu <wucf@linux.ibm.com> --- include/libvirt/libvirt-domain.h | 21 +++ src/driver-hypervisor.h | 22 ++++ src/libvirt-domain.c | 196 ++++++++++++++++++++++++++++ src/libvirt_private.syms | 9 ++ src/libvirt_public.syms | 7 + src/remote/remote_daemon_dispatch.c | 44 +++++++ src/remote/remote_driver.c | 40 ++++++ src/remote/remote_protocol.x | 48 ++++++- src/remote_protocol-structs | 28 ++++ 9 files changed, 414 insertions(+), 1 deletion(-)

[...]

diff --git a/src/libvirt-domain.c b/src/libvirt-domain.c index 7c6b93963c..37ba587b57 100644 --- a/src/libvirt-domain.c +++ b/src/libvirt-domain.c @@ -14162,3 +14162,199 @@ virDomainGraphicsReload(virDomainPtr domain, virDispatchError(domain->conn); return -1; } + + +/** + * virDomainSetThrottleGroup: + * @dom: pointer to domain object + * @group: throttle group name + * @params: Pointer to blkio parameter objects + * @nparams: Number of blkio parameters (this value can be the same or + * less than the number of parameters supported) + * @flags: bitwise-OR of virDomainModificationImpact + * + * Add throttlegroup or change all or a subset of the throttlegroup options + * within specific domain + * + * The @group parameter is the name for new or existing throttlegroup, + * it cannot be NULL, detailed throttlegroup info is included in @params, + * it either creates new throttlegroup with @params or updates existing + * throttlegroup with @params, throttlegroup can be referenced by throttle + * filter in attached disk to do limits, the difference from iotune is that + * multiple throttlegroups can be referenced within attached disk + * + * Returns -1 in case of error, 0 in case of success. + * + * Since: 10.5.0 + */ +int +virDomainSetThrottleGroup(virDomainPtr dom, + const char *group, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(dom, "params=%p, nparams=%d, flags=0x%x", + params, nparams, flags); + VIR_TYPED_PARAMS_DEBUG(params, nparams); + + virResetLastError(); + + virCheckDomainReturn(dom, -1); + conn = dom->conn; + + virCheckReadOnlyGoto(conn->flags, error); + virCheckNonNullArgGoto(group, error); + virCheckPositiveArgGoto(nparams, error); + virCheckNonNullArgGoto(params, error); + + if (virTypedParameterValidateSet(dom->conn, params, nparams) < 0) + goto error; + + if (conn->driver->domainSetThrottleGroup) { + int ret; + ret = conn->driver->domainSetThrottleGroup(dom, group, params, nparams, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(dom->conn); + return -1; +} + + +/** + * virDomainGetThrottleGroup: + * @dom: pointer to domain object + * @group: throttle group name + * @params: pointer that will be filled with an array of typed parameters + * @nparams: pointer filled with number of elements in @params + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags

Typed parameter flags are not to be passed by user.

+ * + * Get all block IO tunable parameters for specific throttle group. @group cannot be NULL. + * @nparams gives how many slots were filled with parameter information + * + * + * Returns -1 in case of error, 0 in case of success. + * + * Since: 10.5.0 + */ +int +virDomainGetThrottleGroup(virDomainPtr dom, + const char *group, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags) +{ + virConnectPtr conn; + int rc; + + VIR_DOMAIN_DEBUG(dom, "params=%p, nparams=%d, flags=0x%x", + params, (nparams) ? *nparams : -1, flags); + + virResetLastError(); + + virCheckDomainReturn(dom, -1); + virCheckNonNullArgGoto(group, error); + virCheckNonNullArgGoto(nparams, error); + virCheckNonNullArgGoto(params, error); + + rc = VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING);

All libvirt versions supporting this API support also string typed params, so this code is not needed and you can assume support.

+ if (rc < 0) + goto error; + if (rc) + flags |= VIR_TYPED_PARAM_STRING_OKAY; + + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE, + VIR_DOMAIN_AFFECT_CONFIG, + error);

You explicitly document that both can be used.

+ + conn = dom->conn; + + if (conn->driver->domainGetThrottleGroup) { + int ret; + ret = conn->driver->domainGetThrottleGroup(dom, group, params, nparams, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(dom->conn); + return -1; +} + + +/** + * virDomainDelThrottleGroup: + * @dom: pointer to domain object + * @group: throttle group name + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags

This takes not typed params.

+ * + * Delete an throttlegroup from the domain. @group cannot be NULL, + * and the @group to be deleted must not have a throttlefilter associated with + * it and can be any of the current valid group. + * + * @flags may include VIR_DOMAIN_AFFECT_LIVE or VIR_DOMAIN_AFFECT_CONFIG. + * Both flags may be set.

[1].

+ * If VIR_DOMAIN_AFFECT_LIVE is set, the change affects a running domain + * and may fail if domain is not alive. + * If VIR_DOMAIN_AFFECT_CONFIG is set, the change affects persistent state, + * and will fail for transient domains. If neither flag is specified (that is, + * @flags is VIR_DOMAIN_AFFECT_CURRENT), then an inactive domain modifies + * persistent setup, while an active domain is hypervisor-dependent on whether + * just live or both live and persistent state is changed. + * + * Returns -1 in case of error, 0 in case of success. + * + * Since: 10.5.0 + */ +int +virDomainDelThrottleGroup(virDomainPtr dom, + const char *group, + unsigned int flags) +{ + virConnectPtr conn; + int rc; + + virResetLastError(); + + virCheckDomainReturn(dom, -1); + virCheckNonNullArgGoto(group, error); + + rc = VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING);

This API is not passing any typed parameters so all of this code is not actually used.

+ if (rc < 0) + goto error; + if (rc) + flags |= VIR_TYPED_PARAM_STRING_OKAY; + + VIR_EXCLUSIVE_FLAGS_GOTO(VIR_DOMAIN_AFFECT_LIVE, + VIR_DOMAIN_AFFECT_CONFIG, + error);

This API is updating state of a VM thus must not be allowed on read-only connections. You'll need to also use proper flags in the .x file (which I've deleted while replying). Also note that many of existing APIs allow dual use of _LIVE and _CONFIG flags. You even document it [1].

+ + conn = dom->conn; + + if (conn->driver->domainDelThrottleGroup) { + int ret; + ret = conn->driver->domainDelThrottleGroup(dom, group, flags); + if (ret < 0) + goto error; + return ret; + } + + virReportUnsupportedError(); + + error: + virDispatchError(dom->conn); + return -1; +}

diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 7a3492d9d7..2b37f29a28 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -948,4 +948,11 @@ LIBVIRT_10.2.0 { virDomainGraphicsReload; } LIBVIRT_10.1.0;

+LIBVIRT_10.5.0 {

10.7.0

+ global: + virDomainSetThrottleGroup; + virDomainGetThrottleGroup; + virDomainDelThrottleGroup; +} LIBVIRT_10.2.0; + # .... define new API here using predicted next version number .... diff --git a/src/remote/remote_daemon_dispatch.c b/src/remote/remote_daemon_dispatch.c index cfc1067e40..f5a9e45303 100644 --- a/src/remote/remote_daemon_dispatch.c +++ b/src/remote/remote_daemon_dispatch.c @@ -3614,6 +3614,50 @@ remoteDispatchDomainGetBlockIoTune(virNetServer *server G_GNUC_UNUSED, return rv; }

+ +static int +remoteDispatchDomainGetThrottleGroup(virNetServer *server G_GNUC_UNUSED, + virNetServerClient *client, + virNetMessage *hdr G_GNUC_UNUSED, + struct virNetMessageError *rerr, + remote_domain_get_throttle_group_args *args, + remote_domain_get_throttle_group_ret *ret) +{ + virDomainPtr dom = NULL; + int rv = -1; + virTypedParameterPtr params = NULL; + int nparams = 0; + virConnectPtr conn = remoteGetHypervisorConn(client); + + if (!conn) + goto cleanup; + + if (!(dom = get_nonnull_domain(conn, args->dom))) + goto cleanup; + + if (virDomainGetThrottleGroup(dom, args->group ? *args->group : NULL, + ¶ms, &nparams, args->flags) < 0) + goto cleanup; + + /* Serialize the ThrottleGroup parameters. */ + if (virTypedParamsSerialize(params, nparams, + REMOTE_DOMAIN_THROTTLE_GROUP_PARAMETERS_MAX, + (struct _virTypedParameterRemote **) &ret->params.params_val, + &ret->params.params_len, + args->flags) < 0) + goto cleanup; + + rv = 0; + + cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + virTypedParamsFree(params, nparams); + virObjectUnref(dom); + return rv; +} + + /*-------------------------------------------------------------*/

static int diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index e76d9e9ba4..4f2b28e662 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2583,6 +2583,43 @@ static int remoteDomainGetBlockIoTune(virDomainPtr domain, return 0; }

+ +static int +remoteDomainGetThrottleGroup(virDomainPtr domain, + const char *group, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags) +{ + remote_domain_get_throttle_group_args args = {0}; + g_auto(remote_domain_get_throttle_group_ret) ret = {0}; + struct private_data *priv = domain->conn->privateData; + VIR_LOCK_GUARD lock = remoteDriverLock(priv); + + make_nonnull_domain(&args.dom, domain); + args.group = group ? (char **)&group : NULL; + args.flags = flags; + + if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_THROTTLE_GROUP, + (xdrproc_t) xdr_remote_domain_get_throttle_group_args, + (char *) &args, + (xdrproc_t) xdr_remote_domain_get_throttle_group_ret, + (char *) &ret) == -1) { + return -1; + } + + if (virTypedParamsDeserialize((struct _virTypedParameterRemote *) ret.params.params_val, + ret.params.params_len, + REMOTE_DOMAIN_THROTTLE_GROUP_PARAMETERS_MAX, + params, + nparams) < 0) + return -1; + + return 0; + +} + + static int remoteDomainGetCPUStats(virDomainPtr domain, virTypedParameterPtr params, unsigned int nparams, @@ -7842,6 +7879,9 @@ static virHypervisorDriver hypervisor_driver = { .domainSetLaunchSecurityState = remoteDomainSetLaunchSecurityState, /* 8.0.0 */ .domainFDAssociate = remoteDomainFDAssociate, /* 9.0.0 */ .domainGraphicsReload = remoteDomainGraphicsReload, /* 10.2.0 */ + .domainSetThrottleGroup = remoteDomainSetThrottleGroup, /* 10.5.0 */ + .domainGetThrottleGroup = remoteDomainGetThrottleGroup, /* 10.5.0 */ + .domainDelThrottleGroup = remoteDomainDelThrottleGroup, /* 10.5.0 */

Don't forget to update these to 10.7.0

};

On Wed, Jun 12, 2024 at 03:02:16 -0700, wucf@linux.ibm.com wrote:

From: Chun Feng Wu <wucf@linux.ibm.com>

Implement the following methods in qemu driver: * Extract common methods for "qemuDomainSetBlockIoTune" and "qemuDomainSetThrottleGroup": qemuDomainValidateBlockIoTune, qemuDomainSetBlockIoTuneFields, qemuDomainCheckBlockIoTuneMutualExclusion, qemuDomainCheckBlockIoTuneMax. * "qemuDomainSetThrottleGroup", this method is to add("object-add") or update("qom-set") throttlegroup in QOM and update corresponding objects in DOM * "qemuDomainGetThrottleGroup", this method queries throttlegroup info by groupname * "qemuDomainDelThrottleGroup", this method checks if group is referenced by any throttle in disks and delete it if it's not used anymore * Check flag "QEMU_CAPS_OBJECT_JSON" during qemuDomainSetThrottleGroup, throttle group feature requries such flag * "objectAddNoWrap"("props") check is done by reusing qemuMonitorAddObject in qemuDomainSetThrottleGroup

Same comment as before. Try to keep the lines shorter and preferrably do a high level description rather than repeating what the commit does.

Signed-off-by: Chun Feng Wu <wucf@linux.ibm.com> --- src/qemu/qemu_driver.c | 611 +++++++++++++++++++++++++++++++++++------ 1 file changed, 528 insertions(+), 83 deletions(-)

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index e2698c7924..f7e435d6d5 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14955,35 +14955,8 @@ qemuDomainCheckBlockIoTuneReset(virDomainDiskDef *disk,

static int -qemuDomainSetBlockIoTune(virDomainPtr dom, - const char *path, - virTypedParameterPtr params, - int nparams, - unsigned int flags) +qemuDomainValidateBlockIoTune(virTypedParameterPtr params, int nparams)

This refactor should be separated to a separate commit as this commit is getting a bit too complex.

{ - virQEMUDriver *driver = dom->conn->privateData; - virDomainObj *vm = NULL; - qemuDomainObjPrivate *priv; - virDomainDef *def = NULL; - virDomainDef *persistentDef = NULL; - virDomainBlockIoTuneInfo info = { 0 }; - virDomainBlockIoTuneInfo conf_info = { 0 }; - int ret = -1; - size_t i; - virDomainDiskDef *conf_disk = NULL; - virDomainDiskDef *disk; - qemuBlockIoTuneSetFlags set_fields = 0; - g_autoptr(virQEMUDriverConfig) cfg = NULL; - virObjectEvent *event = NULL; - virTypedParameterPtr eventParams = NULL; - int eventNparams = 0; - int eventMaxparams = 0; - virDomainBlockIoTuneInfo *cur_info; - virDomainBlockIoTuneInfo *conf_cur_info; - - - virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | - VIR_DOMAIN_AFFECT_CONFIG, -1); if (virTypedParamsValidate(params, nparams, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, VIR_TYPED_PARAM_ULLONG, @@ -15028,32 +15001,28 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, NULL) < 0) return -1;

- if (!(vm = qemuDomainObjFromDomain(dom))) - return -1; - - if (virDomainSetBlockIoTuneEnsureACL(dom->conn, vm->def, flags) < 0) - goto cleanup; - - cfg = virQEMUDriverGetConfig(driver); - - if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) - goto cleanup; - - priv = vm->privateData; + return 0; +}

- if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) - goto endjob;

- if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, - VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) - goto endjob; +static int +qemuDomainSetBlockIoTuneFields(virDomainBlockIoTuneInfo *info, + virTypedParameterPtr params, + int nparams, + qemuBlockIoTuneSetFlags *set_fields, + virTypedParameterPtr *eventParams, + int *eventNparams, + int *eventMaxparams) +{ + size_t i; + int ret = -1;

#define SET_IOTUNE_FIELD(FIELD, BOOL, CONST) \ if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_##CONST)) { \ - info.FIELD = param->value.ul; \ - set_fields |= QEMU_BLOCK_IOTUNE_SET_##BOOL; \ - if (virTypedParamsAddULLong(&eventParams, &eventNparams, \ - &eventMaxparams, \ + info->FIELD = param->value.ul; \ + *set_fields |= QEMU_BLOCK_IOTUNE_SET_##BOOL; \ + if (virTypedParamsAddULLong(eventParams, eventNparams, \ + eventMaxparams, \ VIR_DOMAIN_TUNABLE_BLKDEV_##CONST, \ param->value.ul) < 0) \ goto endjob; \ @@ -15093,10 +15062,10 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,

/* NB: Cannot use macro since this is a value.s not a value.ul */ if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME)) { - info.group_name = g_strdup(param->value.s); - set_fields |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME; - if (virTypedParamsAddString(&eventParams, &eventNparams, - &eventMaxparams, + info->group_name = g_strdup(param->value.s); + *set_fields |= QEMU_BLOCK_IOTUNE_SET_GROUP_NAME; + if (virTypedParamsAddString(eventParams, eventNparams, + eventMaxparams, VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, param->value.s) < 0) goto endjob; @@ -15119,56 +15088,56 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,

#undef SET_IOTUNE_FIELD

- if ((info.total_bytes_sec && info.read_bytes_sec) || - (info.total_bytes_sec && info.write_bytes_sec)) { + ret = 0; + endjob: + return ret; +} + + +static int +qemuDomainCheckBlockIoTuneMutualExclusion(virDomainBlockIoTuneInfo *info) +{ + if ((info->total_bytes_sec && info->read_bytes_sec) || + (info->total_bytes_sec && info->write_bytes_sec)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("total and read/write of bytes_sec cannot be set at the same time")); - goto endjob; + return -1; }

- if ((info.total_iops_sec && info.read_iops_sec) || - (info.total_iops_sec && info.write_iops_sec)) { + if ((info->total_iops_sec && info->read_iops_sec) || + (info->total_iops_sec && info->write_iops_sec)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("total and read/write of iops_sec cannot be set at the same time")); - goto endjob; + return -1; }

- if ((info.total_bytes_sec_max && info.read_bytes_sec_max) || - (info.total_bytes_sec_max && info.write_bytes_sec_max)) { + if ((info->total_bytes_sec_max && info->read_bytes_sec_max) || + (info->total_bytes_sec_max && info->write_bytes_sec_max)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("total and read/write of bytes_sec_max cannot be set at the same time")); - goto endjob; + return -1; }

- if ((info.total_iops_sec_max && info.read_iops_sec_max) || - (info.total_iops_sec_max && info.write_iops_sec_max)) { + if ((info->total_iops_sec_max && info->read_iops_sec_max) || + (info->total_iops_sec_max && info->write_iops_sec_max)) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("total and read/write of iops_sec_max cannot be set at the same time")); - goto endjob; + return -1; }

- virDomainBlockIoTuneInfoCopy(&info, &conf_info); - - if (def) { - if (!(disk = qemuDomainDiskByName(def, path))) - goto endjob; - - if (!qemuDomainDiskBlockIoTuneIsSupported(disk)) - goto endjob; + return 0; +}

- cur_info = qemuDomainFindGroupBlockIoTune(def, disk, &info);

- if (qemuDomainSetBlockIoTuneDefaults(&info, cur_info, - set_fields) < 0) - goto endjob; - - if (qemuDomainCheckBlockIoTuneReset(disk, &info) < 0) - goto endjob; +static int +qemuDomainCheckBlockIoTuneMax(virDomainBlockIoTuneInfo *info) +{ + int ret = -1;

#define CHECK_MAX(val, _bool) \ do { \ - if (info.val##_max) { \ - if (!info.val) { \ + if (info->val##_max) { \ + if (!info->val) { \ if (QEMU_BLOCK_IOTUNE_SET_##_bool) { \ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ _("cannot reset '%1$s' when '%2$s' is set"), \ @@ -15180,7 +15149,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, } \ goto endjob; \ } \ - if (info.val##_max < info.val) { \ + if (info->val##_max < info->val) { \ virReportError(VIR_ERR_CONFIG_UNSUPPORTED, \ _("value '%1$s' cannot be smaller than '%2$s'"), \ #val "_max", #val); \ @@ -15198,6 +15167,96 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,

#undef CHECK_MAX

+ ret = 0; + endjob: + return ret; +} + + +static int +qemuDomainSetBlockIoTune(virDomainPtr dom, + const char *path, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + virQEMUDriver *driver = dom->conn->privateData; + virDomainObj *vm = NULL; + qemuDomainObjPrivate *priv; + virDomainDef *def = NULL; + virDomainDef *persistentDef = NULL; + virDomainBlockIoTuneInfo info = { 0 }; + virDomainBlockIoTuneInfo conf_info = { 0 }; + int ret = -1; + virDomainDiskDef *conf_disk = NULL; + virDomainDiskDef *disk; + qemuBlockIoTuneSetFlags set_fields = 0; + g_autoptr(virQEMUDriverConfig) cfg = NULL; + virObjectEvent *event = NULL; + virTypedParameterPtr eventParams = NULL; + int eventNparams = 0; + int eventMaxparams = 0; + virDomainBlockIoTuneInfo *cur_info; + virDomainBlockIoTuneInfo *conf_cur_info; + + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + if (qemuDomainValidateBlockIoTune(params, nparams) < 0) + return -1; + + if (!(vm = qemuDomainObjFromDomain(dom))) + return -1; + + if (virDomainSetBlockIoTuneEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + cfg = virQEMUDriverGetConfig(driver); + + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) + goto cleanup; + + priv = vm->privateData; + + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) + goto endjob; + + if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_DISK, path) < 0) + goto endjob; + + if (qemuDomainSetBlockIoTuneFields(&info, + params, + nparams, + &set_fields, + &eventParams, + &eventNparams, + &eventMaxparams) < 0) + goto endjob; + + if (qemuDomainCheckBlockIoTuneMutualExclusion(&info) < 0) + goto endjob; + + virDomainBlockIoTuneInfoCopy(&info, &conf_info); + + if (def) { + if (!(disk = qemuDomainDiskByName(def, path))) + goto endjob; + + if (!qemuDomainDiskBlockIoTuneIsSupported(disk)) + goto endjob; + + cur_info = qemuDomainFindGroupBlockIoTune(def, disk, &info); + + if (qemuDomainSetBlockIoTuneDefaults(&info, cur_info, set_fields) < 0) + goto endjob; + + if (qemuDomainCheckBlockIoTuneReset(disk, &info) < 0) + goto endjob; + + if (qemuDomainCheckBlockIoTuneMax(&info) < 0) + goto endjob; + /* blockdev-based qemu doesn't want to set the throttling when a cdrom * is empty. Skip the monitor call here since we will set the throttling * once new media is inserted */

All of the above belongs to a separate or two separate commits.

@@ -19995,6 +20054,389 @@ qemuDomainGraphicsReload(virDomainPtr domain, return ret; }

+ +/** + * qemuDomainCheckThrottleGroupReset: + * @groupname: group to create or update + * @newiotune: Pointer to iotune, which contains detailed items + * + * Check if params within @newiotune contain all zero values, if yes + * and @newiotune specifies the same group name as @groupname, return + * failure since it's meaningless to set all zero values in @newiotune + * + * Returns -1 on failure, or 0 on success. + */ +static int +qemuDomainCheckThrottleGroupReset(const char *groupname, + virDomainBlockIoTuneInfo *newiotune) +{ + if (virDomainBlockIoTuneInfoHasAny(newiotune)) + return 0; + + if (newiotune->group_name && + STRNEQ_NULLABLE(newiotune->group_name, groupname)) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("creating a new group/updating existing with all parameters zero is not supported")); + return -1; + } + + /* all zero means remove any throttling and remove from group for qemu */ + VIR_FREE(newiotune->group_name); + + return 0; +} + + +static int +qemuDomainSetThrottleGroup(virDomainPtr dom, + const char *groupname, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + virQEMUDriver *driver = dom->conn->privateData; + virDomainObj *vm = NULL; + virDomainDef *def = NULL; + virDomainDef *persistentDef = NULL; + virDomainThrottleGroupDef info = { 0 }; + virDomainThrottleGroupDef conf_info = { 0 }; + int ret = -1; + qemuBlockIoTuneSetFlags set_fields = 0; + g_autoptr(virQEMUDriverConfig) cfg = NULL; + virObjectEvent *event = NULL; + virTypedParameterPtr eventParams = NULL; + int eventNparams = 0; + int eventMaxparams = 0; + virDomainThrottleGroupDef *cur_info; + virDomainThrottleGroupDef *conf_cur_info; + int rc = 0; + g_autoptr(virJSONValue) props = NULL; + g_autoptr(virJSONValue) limits = virJSONValueNewObject(); + qemuDomainObjPrivate *priv = NULL; + virQEMUCaps *qemuCaps = NULL; + + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + if (qemuDomainValidateBlockIoTune(params, nparams) < 0) + return -1;

Spacing.

+ + if (!(vm = qemuDomainObjFromDomain(dom))) + return -1; + + if (virDomainSetThrottleGroupEnsureACL(dom->conn, vm->def, flags) < 0) + goto cleanup; + + cfg = virQEMUDriverGetConfig(driver); + + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) + goto endjob; + + if (virTypedParamsAddString(&eventParams, &eventNparams, &eventMaxparams, + VIR_DOMAIN_TUNABLE_BLKDEV_GROUP_NAME, groupname) < 0) + goto endjob; + + if (qemuDomainSetBlockIoTuneFields(&info, + params, + nparams, + &set_fields, + &eventParams, + &eventNparams, + &eventMaxparams) < 0) + goto endjob; + + if (qemuDomainCheckBlockIoTuneMutualExclusion(&info) < 0) + goto endjob; + + virDomainThrottleGroupDefCopy(&info, &conf_info); + + priv = vm->privateData; + qemuCaps = priv->qemuCaps; + /* this throttle group feature requires "QEMU_CAPS_OBJECT_JSON" + * when starting domain later, so check such flag here as well */ + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_JSON)) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("QEMU_CAPS_OBJECT_JSON support is required for throttle group creation")); + return -1; + }

If the VM is not alive the above check will not work. If the VM is not alive this must not be checked. Also _("QEMU_CAPS_OBJECT_JSON is an internal name and VIR_ERR_INTERNAL_ERROR is not appropriate.

+ + if (def) { + if (qemuDomainCheckThrottleGroupReset(groupname, &info) < 0) + goto endjob; + + if (qemuDomainCheckBlockIoTuneMax(&info) < 0) + goto endjob; + + cur_info = virDomainThrottleGroupByName(def, groupname); + /* Update existing group. */ + if (cur_info != NULL) { + if (qemuDomainSetBlockIoTuneDefaults(&info, cur_info, set_fields) < 0) + goto endjob; + qemuDomainObjEnterMonitor(vm); + rc = qemuMonitorUpdateThrottleGroup(qemuDomainGetMonitor(vm), + groupname, + &info); + qemuDomainObjExitMonitor(vm); + if (rc < 0) + goto endjob; + virDomainThrottleGroupUpdate(def, &info); + } else { + if (qemuMonitorThrottleGroupLimits(limits, &info)<0) + goto endjob; + if (qemuMonitorCreateObjectProps(&props, + "throttle-group", groupname,

As noted groupname must be prefixed when used with qemu to avoid namespacing issues by users being able to choose a name.

+ "a:limits", &limits, + NULL) < 0) + goto endjob; + qemuDomainObjEnterMonitor(vm); + /* different QMP version has different format for "object-add", reuse + * "qemuMonitorAddObject" to check if "objectAddNoWrap"("props") is + * requried */

Regardless of this comment all object creation must use the appropriate API so this comment seems pointless.

+ rc = qemuMonitorAddObject(qemuDomainGetMonitor(vm), &props, NULL); + qemuDomainObjExitMonitor(vm); + if (rc < 0) + goto endjob; + virDomainThrottleGroupAdd(def, &info); + } + + qemuDomainSaveStatus(vm); + + if (eventNparams) { + event = virDomainEventTunableNewFromDom(dom, &eventParams, eventNparams); + virObjectEventStateQueue(driver->domainEventState, event); + } + } + + if (persistentDef) { + conf_cur_info = virDomainThrottleGroupByName(persistentDef, groupname); + + if (qemuDomainCheckThrottleGroupReset(groupname, &conf_info) < 0) + goto endjob; + + if (conf_cur_info != NULL) { + if (qemuDomainSetBlockIoTuneDefaults(&conf_info, conf_cur_info, + set_fields) < 0) + goto endjob; + virDomainThrottleGroupUpdate(persistentDef, &conf_info); + } else { + virDomainThrottleGroupAdd(persistentDef, &conf_info); + } + + + if (virDomainDefSave(persistentDef, driver->xmlopt, + cfg->configDir) < 0) + goto endjob; + } + + ret = 0; + endjob: + virDomainObjEndJob(vm); + + cleanup: + virDomainObjEndAPI(&vm); + virTypedParamsFree(eventParams, eventNparams); + return ret; +} + + +static int +qemuDomainGetThrottleGroup(virDomainPtr dom, + const char *groupname, + virTypedParameterPtr *params, + int *nparams, + unsigned int flags) +{ + virDomainObj *vm = NULL; + virDomainDef *def = NULL; + virDomainDef *persistentDef = NULL; + virDomainThrottleGroupDef groupDef = { 0 }; + virDomainThrottleGroupDef *reply = &groupDef; + int ret = -1; + int maxparams = 0; + int rc = 0; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG | + VIR_TYPED_PARAM_STRING_OKAY, -1);

As noted all callers which know this API are new enough to support typed param string so this flag doesn't make sense.

+ + + if (!(vm = qemuDomainObjFromDomain(dom))) + return -1; + + if (virDomainGetThrottleGroupEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (virDomainObjBeginJob(vm, VIR_JOB_QUERY) < 0) + goto cleanup; + + /* the API check guarantees that only one of the definitions will be set */ + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) + goto endjob;

I might have misplaced a comment about supporting mutual exclusivity in reply to previous patch. If that is so please ignore that comment.

+ + if (def) { + qemuDomainObjEnterMonitor(vm); + rc = qemuMonitorGetThrottleGroup(qemuDomainGetMonitor(vm), groupname, reply);

You don't validate that the given throttle group even exists. You shouldn't allow passing that to qemu if it's not in the definition. Also as noted before you'll have to prefix the group name to avoid namespace collisions.

+ qemuDomainObjExitMonitor(vm); + + if (rc < 0) + goto endjob; + } + + if (persistentDef) { + reply = virDomainThrottleGroupByName(persistentDef, groupname); + if (reply == NULL) { + virReportError(VIR_ERR_INVALID_ARG, + _("throttle group '%1$s' was not found in the domain config"), + groupname); + goto endjob; + } + reply->group_name = g_strdup(groupname); + } + + *nparams = 0; + +#define THROTTLE_GROUP_ASSIGN(name, var) \ + if (virTypedParamsAddULLong(params, \ + nparams, \ + &maxparams, \ + VIR_DOMAIN_BLOCK_IOTUNE_ ## name, \ + reply->var) < 0) \ + goto endjob; + + + if (virTypedParamsAddString(params, nparams, &maxparams, + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, + reply->group_name) < 0) + goto endjob; + + THROTTLE_GROUP_ASSIGN(TOTAL_BYTES_SEC, total_bytes_sec); + THROTTLE_GROUP_ASSIGN(READ_BYTES_SEC, read_bytes_sec); + THROTTLE_GROUP_ASSIGN(WRITE_BYTES_SEC, write_bytes_sec); + + THROTTLE_GROUP_ASSIGN(TOTAL_IOPS_SEC, total_iops_sec); + THROTTLE_GROUP_ASSIGN(READ_IOPS_SEC, read_iops_sec); + THROTTLE_GROUP_ASSIGN(WRITE_IOPS_SEC, write_iops_sec); + + THROTTLE_GROUP_ASSIGN(TOTAL_BYTES_SEC_MAX, total_bytes_sec_max); + THROTTLE_GROUP_ASSIGN(READ_BYTES_SEC_MAX, read_bytes_sec_max); + THROTTLE_GROUP_ASSIGN(WRITE_BYTES_SEC_MAX, write_bytes_sec_max); + + THROTTLE_GROUP_ASSIGN(TOTAL_IOPS_SEC_MAX, total_iops_sec_max); + THROTTLE_GROUP_ASSIGN(READ_IOPS_SEC_MAX, read_iops_sec_max); + THROTTLE_GROUP_ASSIGN(WRITE_IOPS_SEC_MAX, write_iops_sec_max); + + THROTTLE_GROUP_ASSIGN(SIZE_IOPS_SEC, size_iops_sec); + + THROTTLE_GROUP_ASSIGN(TOTAL_BYTES_SEC_MAX_LENGTH, total_bytes_sec_max_length); + THROTTLE_GROUP_ASSIGN(READ_BYTES_SEC_MAX_LENGTH, read_bytes_sec_max_length); + THROTTLE_GROUP_ASSIGN(WRITE_BYTES_SEC_MAX_LENGTH, write_bytes_sec_max_length); + + THROTTLE_GROUP_ASSIGN(TOTAL_IOPS_SEC_MAX_LENGTH, total_iops_sec_max_length); + THROTTLE_GROUP_ASSIGN(READ_IOPS_SEC_MAX_LENGTH, read_iops_sec_max_length); + THROTTLE_GROUP_ASSIGN(WRITE_IOPS_SEC_MAX_LENGTH, write_iops_sec_max_length); +#undef THROTTLE_GROUP_ASSIGN + + ret = 0; + + endjob: + virDomainObjEndJob(vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +} + + +static int +qemuDomainCheckThrottleGroupRef(virDomainDef *def, + const char *group_name) +{ + size_t i; + for (i = 0; i < def->ndisks; i++) { + virDomainDiskDef *disk = def->disks[i]; + if (virDomainThrottleFilterFind(disk, group_name)) { + virReportError(VIR_ERR_INTERNAL_ERROR,

This is not an internal error but invalid argument from the user.

+ _("throttle group '%1$s' is still being used by disk %2$s"), + group_name, disk->dst); + return -1; + } + } + return 0; +} + + +static int +qemuDomainDelThrottleGroup(virDomainPtr dom, + const char *groupname, + unsigned int flags) +{ + virQEMUDriver *driver = dom->conn->privateData; + virDomainObj *vm = NULL; + virDomainDef *def = NULL; + virDomainDef *persistentDef = NULL; + g_autoptr(virQEMUDriverConfig) cfg = NULL; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG | + VIR_TYPED_PARAM_STRING_OKAY, -1);

This API is not taking typed params.

+ + if (!(vm = qemuDomainObjFromDomain(dom))) + return -1; + + if (virDomainDelThrottleGroupEnsureACL(dom->conn, vm->def) < 0) + goto cleanup; + + if (virDomainObjBeginJob(vm, VIR_JOB_MODIFY) < 0) + goto cleanup; + + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0) + goto endjob; + + if (def) { + int rc = 0; + + /* check if this group is still being used by disks */ + if (qemuDomainCheckThrottleGroupRef(def, groupname) < 0) + goto endjob; + + qemuDomainObjEnterMonitor(vm); + rc = qemuMonitorDelObject(qemuDomainGetMonitor(vm), groupname, true); + qemuDomainObjExitMonitor(vm); + + if (rc < 0) + goto endjob; + + virDomainThrottleGroupDel(def, groupname); + qemuDomainSaveStatus(vm); + } + + if (persistentDef) { + /* check if this group is still being used by disks */ + if (qemuDomainCheckThrottleGroupRef(persistentDef, groupname) < 0) + goto endjob;

This should be done before the step modifying the live config so that you catch errors upfront and avoid a partial success with error reported.

+ + cfg = virQEMUDriverGetConfig(driver); + virDomainThrottleGroupDel(persistentDef, groupname); + if (virDomainDefSave(persistentDef, driver->xmlopt, + cfg->configDir) < 0) + goto endjob; + } + + ret = 0; + + endjob: + virDomainObjEndJob(vm); + + cleanup: + virDomainObjEndAPI(&vm); + return ret; +}

On Wed, Jun 12, 2024 at 03:02:17 -0700, wucf@linux.ibm.com wrote:

From: Chun Feng Wu <wucf@linux.ibm.com>

When attaching disk along with specified throttle groups, those groups will be chained up by parent node name, this change includes service side codes: * Each filter references one throttle group by group name * Update "qemuDomainDiskGetTopNodename" to take top throttle node name if disk has throttles * Each filter has a nodename, and those filters are chained up in sequence * Filter nodename index is generated by reusing qemuDomainStorageIDNew and current global sequence number is persistented in virDomainObj->privateData(qemuDomainObjPrivate)->nodenameindex * During hotplug, filter is created through QMP request("blockdev-add" with "driver":"throttle") to QEMU * Delete filters by "qemuBlockThrottleFiltersDetach"("blockdev-del") when detaching device * Use "iotune" and "throttlefilters" exclusively for specific disk

Signed-off-by: Chun Feng Wu <wucf@linux.ibm.com> --- src/conf/domain_validate.c | 8 +++ src/qemu/qemu_block.c | 131 +++++++++++++++++++++++++++++++++++++ src/qemu/qemu_block.h | 53 +++++++++++++++ src/qemu/qemu_command.c | 84 ++++++++++++++++++++++++ src/qemu/qemu_command.h | 9 +++ src/qemu/qemu_domain.c | 39 ++++++++++- src/qemu/qemu_domain.h | 8 +++ src/qemu/qemu_driver.c | 6 ++ src/qemu/qemu_hotplug.c | 33 ++++++++++ 9 files changed, 370 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 395e036e8f..4cc5ed7577 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -942,6 +942,14 @@ virDomainDiskDefValidate(const virDomainDef *def, return -1; }

+ if (disk->throttlefilters && (disk->blkdeviotune.group_name || + virDomainBlockIoTuneInfoHasAny(&disk->blkdeviotune))) {

This is hard to read as you've broken up the line within a nested brace. It may confuse readers. Rewrite as: if (disk->throttlefilters && (disk->blkdeviotune.group_name || virDomainBlockIoTuneInfoHasAny(&disk->blkdeviotune))) {

+ virReportError(VIR_ERR_XML_ERROR,

Operation unsupported.

+ _("block 'throttlefilters' can't be used together with 'iotune' for disk '%1$s'"), + disk->dst); + return -1; + } + return 0; }

diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 738b72d7ea..9b8ff65887 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -2739,6 +2739,137 @@ qemuBlockStorageSourceCreateDetectSize(GHashTable *blockNamedNodeData, }

+void +qemuBlockThrottleFilterSetNodename(virDomainThrottleFilterDef *filter, + char *nodename) +{ + g_free(filter->nodename); + filter->nodename = nodename; +} + + +const char * +qemuBlockThrottleFilterGetNodename(virDomainThrottleFilterDef *filter) +{ + return filter->nodename; +}

All of this helper infrastructure doesn't belong to a patch that is declaring that it's dealing with the setup of qemu. (I also wrote that below as I've noticed it there). It makes this patch too complex and thus hard to review. It's also the reason it takes me so long. I'm demotivated on such commits as it takes way too long to go through. Any setup of nodenames and other libvirt-internal stuff, such as validatio of config etc should be separated.

+/** + * qemuBlockThrottleFilterGetProps: + * @filter: throttle filter + * @parentNodeName: parent nodename of @filter + * + * Build "arguments" part of "blockdev-add" QMP cmd. + * e.g. {"execute":"blockdev-add", "arguments":{"driver":"throttle", + * "node-name":"libvirt-2-filter", "throttle-group":"limits0", + * "file": "libvirt-1-format"}}

Avoid this JSON in comments, as it's obvious from the code. Rather describe any special handling if needed.

+ */ +virJSONValue * +qemuBlockThrottleFilterGetProps(virDomainThrottleFilterDef *filter, + const char *parentNodeName) +{ + g_autoptr(virJSONValue) props = NULL; + + if (virJSONValueObjectAdd(&props, + "s:driver", "throttle", + "s:node-name", qemuBlockThrottleFilterGetNodename(filter), + "s:throttle-group", filter->group_name, + "s:file", parentNodeName, + NULL) < 0) + return 0;

return NULL; It looks confusing because 0 is success in our code.

+ + return g_steal_pointer(&props); +}

[...]

+qemuBlockThrottleFilterAttachData * +qemuBlockThrottleFilterAttachPrepareBlockdev(virDomainThrottleFilterDef *filter, + const char *parentNodeName) +{ + g_autoptr(qemuBlockThrottleFilterAttachData) data = NULL; + + data = g_new0(qemuBlockThrottleFilterAttachData, 1); + + if (!(data->filterProps = qemuBlockThrottleFilterGetProps(filter, parentNodeName))) + return NULL; + + data->filterNodeName = qemuBlockThrottleFilterGetNodename(filter); + data->filterAttached = true;

Well, if you set this to true right here, it's pointless to even have the variable. The code you've copied this from uses the 'Attached' variable so that it's set only after the object is created in qemu. This allows safe rollback. If you set it to true for everything you will attempt to potentially roll back stuff that was not yet set in qemu, making the variable itself pointless.

+ + return g_steal_pointer(&data); +} + + +void +qemuBlockThrottleFilterAttachRollback(qemuMonitor *mon, + qemuBlockThrottleFilterAttachData *data) +{ + virErrorPtr orig_err; + + virErrorPreserveLast(&orig_err); + + if (data->filterAttached) + ignore_value(qemuMonitorBlockdevDel(mon, data->filterNodeName));

... here ...

+ + virErrorRestore(&orig_err); +} + + +void +qemuBlockThrottleFiltersDataFree(qemuBlockThrottleFiltersData *data) +{ + size_t i; + + if (!data) + return; + + for (i = 0; i < data->nfilterdata; i++) + qemuBlockThrottleFilterAttachDataFree(data->filterdata[i]); + + g_free(data->filterdata); + g_free(data); +} + + +int +qemuBlockThrottleFiltersAttach(qemuMonitor *mon, + qemuBlockThrottleFiltersData *data) +{ + size_t i; + + for (i = 0; i < data->nfilterdata; i++) { + if (qemuMonitorBlockdevAdd(mon, &data->filterdata[i]->filterProps) < 0)

This function requires the monitor to be locked. If you want to have this as a separate function you must document it and state this fact.

+ return -1; + data->filterdata[i]->filterAttached = true; + } + + return 0; +} + + +void +qemuBlockThrottleFiltersDetach(qemuMonitor *mon, + qemuBlockThrottleFiltersData *data) +{ + size_t i; + + for (i = data->nfilterdata; i > 0; i--) + qemuBlockThrottleFilterAttachRollback(mon, data->filterdata[i-1]); +} + + int qemuBlockRemoveImageMetadata(virQEMUDriver *driver, virDomainObj *vm, diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index f9e961d85d..9888954ce4 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -214,6 +214,59 @@ qemuBlockStorageSourceCreateDetectSize(GHashTable *blockNamedNodeData, virStorageSource *src, virStorageSource *templ);

+void +qemuBlockThrottleFilterSetNodename(virDomainThrottleFilterDef *filter, + char *nodename); + +const char * +qemuBlockThrottleFilterGetNodename(virDomainThrottleFilterDef *filter); + +virJSONValue * +qemuBlockThrottleFilterGetProps(virDomainThrottleFilterDef *filter, + const char *parentNodeName);

This function isn't used outside of qemu_block.c so it doesn't need to be exported.

+ +typedef struct qemuBlockThrottleFilterAttachData qemuBlockThrottleFilterAttachData; +struct qemuBlockThrottleFilterAttachData { + virJSONValue *filterProps; + const char *filterNodeName; + bool filterAttached; +}; + +qemuBlockThrottleFilterAttachData * +qemuBlockThrottleFilterAttachPrepareBlockdev(virDomainThrottleFilterDef *throttlefilter, + const char *parentNodeName); + +void +qemuBlockThrottleFilterAttachDataFree(qemuBlockThrottleFilterAttachData *data); + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuBlockThrottleFilterAttachData, + qemuBlockThrottleFilterAttachDataFree); + +void +qemuBlockThrottleFilterAttachRollback(qemuMonitor *mon, + qemuBlockThrottleFilterAttachData *data); + +struct _qemuBlockThrottleFiltersData { + qemuBlockThrottleFilterAttachData **filterdata; + size_t nfilterdata; +}; + +typedef struct _qemuBlockThrottleFiltersData qemuBlockThrottleFiltersData; + +void +qemuBlockThrottleFiltersDataFree(qemuBlockThrottleFiltersData *data); + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuBlockThrottleFiltersData, + qemuBlockThrottleFiltersDataFree); + +int +qemuBlockThrottleFiltersAttach(qemuMonitor *mon, + qemuBlockThrottleFiltersData *data); + +void +qemuBlockThrottleFiltersDetach(qemuMonitor *mon, + qemuBlockThrottleFiltersData *data); + int qemuBlockRemoveImageMetadata(virQEMUDriver *driver, virDomainObj *vm, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2d0eddc79e..5ccae956d3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1582,6 +1582,13 @@ qemuDiskConfigBlkdeviotuneEnabled(const virDomainDiskDef *disk) }

+bool +qemuDiskConfigThrottleFiltersEnabled(const virDomainDiskDef *disk)

This function doesn't need to be exported.

+{ + return disk->nthrottlefilters > 0; +} + + /** * qemuDiskBusIsSD: * @bus: disk bus @@ -11055,6 +11062,83 @@ qemuBuildStorageSourceChainAttachPrepareBlockdevOne(qemuBlockStorageSourceChainD }

+/** + * qemuBuildThrottleFiltersAttachPrepareBlockdevOne: + * @data: filter chain data, which consists of array of filters and size of such array + * @throttlefilter: new filter to be added into filter array + * @parentNodeName: parent nodename for this new throttlefilter, which is used to build "blockdev-add" QMP request

the stuff after the comma can be dropped.

+ * + * Build filter node chain to provide more flexibility for block disk I/O limits + */ +static int +qemuBuildThrottleFiltersAttachPrepareBlockdevOne(qemuBlockThrottleFiltersData *data, + virDomainThrottleFilterDef *throttlefilter, + const char *parentNodeName) +{ + g_autoptr(qemuBlockThrottleFilterAttachData) elem = NULL; + + if (!(elem = qemuBlockThrottleFilterAttachPrepareBlockdev(throttlefilter, parentNodeName))) + return -1; + + VIR_APPEND_ELEMENT(data->filterdata, data->nfilterdata, elem); + return 0; +} + + +/** + * qemuBuildThrottleFiltersAttachPrepareBlockdev: + * @disk: domain disk + * + * Build filter node chain to provide more flexibility for block disk I/O limits + */ +qemuBlockThrottleFiltersData * +qemuBuildThrottleFiltersAttachPrepareBlockdev(virDomainDiskDef *disk) +{ + g_autoptr(qemuBlockThrottleFiltersData) data = NULL; + size_t i; + const char * parentNodeName = NULL; + g_autofree char *tmp_nodename = NULL; + + data = g_new0(qemuBlockThrottleFiltersData, 1); + /* get starting parentNodename, e.g. libvirt-1-format */ + parentNodeName = qemuBlockStorageSourceGetEffectiveNodename(disk->src); + /* build filterdata, which contains all filters info and sequence info through parentNodeName */ + for (i = 0; i < disk->nthrottlefilters; i++) { + tmp_nodename = g_strdup(parentNodeName); + if (qemuBuildThrottleFiltersAttachPrepareBlockdevOne(data, disk->throttlefilters[i], tmp_nodename) < 0) + return NULL;

The nodename copied into 'tmp_nodename' is leaked on every iteration. Also qemuBuildThrottleFiltersAttachPrepareBlockdevOne doesn't modify it so there's no point in copying it.

+ parentNodeName = disk->throttlefilters[i]->nodename; + } + + return g_steal_pointer(&data); +} + + +/** + * qemuBuildThrottleFiltersDetachPrepareBlockdev: + * @disk: domain disk + * + * Build filters data for later "blockdev-del" + */ +qemuBlockThrottleFiltersData * +qemuBuildThrottleFiltersDetachPrepareBlockdev(virDomainDiskDef *disk) +{ + g_autoptr(qemuBlockThrottleFiltersData) data = g_new0(qemuBlockThrottleFiltersData, 1); + size_t i; + + /* build filterdata, which contains filters info and sequence info */ + for (i = 0; i < disk->nthrottlefilters; i++) { + g_autoptr(qemuBlockThrottleFilterAttachData) elem = g_new0(qemuBlockThrottleFilterAttachData, 1); + /* ignore other fields since the following info are enough for "blockdev-del" */ + elem->filterNodeName = qemuBlockThrottleFilterGetNodename(disk->throttlefilters[i]);

So I didn't yet see any code that serializes any of this in the status XML, thus it seems that this will not work after you restart libvirtd/virtqemud if a VM with filters is running, and then try to detach disks. You'll need to add that, or base the filter nodename on something else. Note that presence of the node name is authoritative and we must not try to regenerate it. That would hinder changing the node names in the future. See how the copyOnRead layer node name is stored.

+ elem->filterAttached = true; + + VIR_APPEND_ELEMENT(data->filterdata, data->nfilterdata, elem); + } + return g_steal_pointer(&data); +} + + /** * qemuBuildStorageSourceChainAttachPrepareBlockdev: * @top: storage source chain

[...]

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7ba2ea4a5e..2831036e23 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7989,7 +7989,8 @@ qemuDomainDetermineDiskChain(virQEMUDriver *driver, * @disk: disk definition object * * Returns the pointer to the node-name of the topmost layer used by @disk as - * backend. Currently returns the nodename of the copy-on-read filter if enabled + * backend. Currently returns the nodename of top throttle filter if enabled + * or the nodename of the copy-on-read filter if enabled * or the nodename of the top image's format driver. Empty disks return NULL. * This must be used only with disks instantiated via -blockdev (thus not * for SD cards). @@ -8005,6 +8006,10 @@ qemuDomainDiskGetTopNodename(virDomainDiskDef *disk) if (disk->copy_on_read == VIR_TRISTATE_SWITCH_ON) return priv->nodeCopyOnRead;

+ /* If disk has throttles, take top throttle node name */ + if (disk->nthrottlefilters > 0) + return disk->throttlefilters[disk->nthrottlefilters-1]->nodename;

return disk->throttlefilters[disk->nthrottlefilters - 1]->nodename; (extra spaces around subtraction operator)

return qemuBlockStorageSourceGetEffectiveNodename(disk->src); }

@@ -11336,6 +11341,32 @@ qemuDomainPrepareStorageSourceBlockdevNodename(virDomainDiskDef *disk, }

+int +qemuDomainPrepareThrottleFilterBlockdevNodename(virDomainThrottleFilterDef *filter,

This function is used only in this file so it doesn't need to be exported. Also it's used only in one place so it can be inlined as it makes the code harder to follow than necessary.

+ const char *nodenameprefix) +{ + char *nodename = g_strdup_printf("%s-filter", nodenameprefix); + + qemuBlockThrottleFilterSetNodename(filter, nodename); + + return 0; +} + + +int +qemuDomainPrepareThrottleFilterBlockdev(virDomainThrottleFilterDef *filter, + qemuDomainObjPrivate *priv)

This function is used only in this module thus it doesn't need to be exported. Also it can't fail so there's no point in having a return value.

+{ + g_autofree char *nodenameprefix = NULL; + + filter->id = qemuDomainStorageIDNew(priv); + + nodenameprefix = g_strdup_printf("libvirt-%u", filter->id);

nodename = g_strdup_printf("libvirt-%u-filter" ... Instead of the convoluted mess and the below call.

+ + return qemuDomainPrepareThrottleFilterBlockdevNodename(filter, nodenameprefix);

[...]

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f7e435d6d5..60989ae693 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14828,6 +14828,12 @@ qemuDomainDiskBlockIoTuneIsSupported(virDomainDiskDef *disk) return false; }

+ if (disk->throttlefilters) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("block 'iotune' can't be used together with 'throttlefilters' for disk '%1$s'"), disk->dst); + return false; + }

Preferrably add all this infrastructure which doesn't deal with the setup of qemu in a separate patch. This patch is overly complex as it intermixes all of these validation bits with the actual qemu interaction.

+ return true; }

diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4a3f4f657e..103b9e9f00 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -657,6 +657,7 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm, virDomainAsyncJob asyncJob) { g_autoptr(qemuBlockStorageSourceChainData) data = NULL; + g_autoptr(qemuBlockThrottleFiltersData) filterData = NULL; qemuDomainObjPrivate *priv = vm->privateData; g_autoptr(virJSONValue) devprops = NULL; bool extensionDeviceAttached = false; @@ -695,6 +696,19 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm, if (rc < 0) goto rollback;

+ /* Setup throttling filters + * add additional "blockdev-add"(throttle filter) between "blockdev-add" (qemuBlockStorageSourceChainAttach) and "device_add" (qemuDomainAttachExtensionDevice)

Drop above line.

+ */ + if ((filterData = qemuBuildThrottleFiltersAttachPrepareBlockdev(disk))) { + if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) + return -1; + /* QMP requests("blockdev-add" with "driver":"throttle") to QEMU */ + rc = qemuBlockThrottleFiltersAttach(priv->mon, filterData); + qemuDomainObjExitMonitor(vm); + if (rc < 0) + goto rollback; + }

The ordering is wrong. In the prepare step you are ordering the node name dependencies such as device -> copyOnRead Layer -> throttle -> image chain OBviously to ensure hierarchy at hotplug they need to be plugged from the end. This block here is placed _AFTER_ copyOnRead layer instantiation, so that will not work with disks with copyOnRead.

+ if (disk->transient) { g_autoptr(qemuBlockStorageSourceAttachData) backend = NULL; g_autoptr(GHashTable) blockNamedNodeData = NULL; @@ -766,6 +780,8 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm, if (extensionDeviceAttached) ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &disk->info));

+ qemuBlockThrottleFiltersDetach(priv->mon, filterData); + qemuBlockStorageSourceChainDetach(priv->mon, data);

qemuDomainObjExitMonitor(vm); @@ -921,6 +937,14 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, bool releaseSeclabel = false; int ret = -1;

+ if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { + if (disk->nthrottlefilters > 0) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cdrom device with throttle filters isn't supported"));

So and now this is why I don't like the fact that you are doing this on a per-disk level. On a per-image level (if you'd instantiate 'throttle' layers when adding the image) this would not be a problem. I'd strongly prefer if you modify this such that the trottle layers will be instantiated at per-storage-source level both in XML and in the code.

+ return -1; + } + } + if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) { virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", _("floppy device hotplug isn't supported")); @@ -4499,6 +4523,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriver *driver, { qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk); g_autoptr(qemuBlockStorageSourceChainData) diskBackend = NULL; + g_autoptr(qemuBlockThrottleFiltersData) filterData = NULL; size_t i; qemuDomainObjPrivate *priv = vm->privateData; int ret = -1; @@ -4537,6 +4562,14 @@ qemuDomainRemoveDiskDevice(virQEMUDriver *driver, } }

+ qemuDomainObjEnterMonitor(vm); + /* QMP request("blockdev-del") to QEMU to delete throttle filter*/ + if ((filterData = qemuBuildThrottleFiltersDetachPrepareBlockdev(disk))) { + /* "qemuBlockThrottleFiltersDetach" is used in rollback within "qemuDomainAttachDiskGeneric" as well */ + qemuBlockThrottleFiltersDetach(priv->mon, filterData);

In which case this'd be automatic.

+ } + qemuDomainObjExitMonitor(vm); + qemuDomainObjEnterMonitor(vm);

if (diskBackend) -- 2.34.1

This patch should also contain the corresponding commandline infrastructure for the filter itself. The disk backend code is specifically unified by using the intermediate struct so that it's guaranteed that both hotplug and commandline work the same. Thus they need to be added simultaenously.

On 2024/8/15 13:04, Chun Feng Wu wrote:

On 2024/8/9 22:04, Peter Krempa wrote:

...

On Wed, Jun 12, 2024 at 03:02:17 -0700,wucf@linux.ibm.com  wrote:

...

From: Chun Feng Wu<wucf@linux.ibm.com>

When attaching disk along with specified throttle groups, those groups will be chained up by parent node name, this change includes service side codes: * Each filter references one throttle group by group name * Update "qemuDomainDiskGetTopNodename" to take top throttle node name if disk has throttles * Each filter has a nodename, and those filters are chained up in sequence * Filter nodename index is generated by reusing qemuDomainStorageIDNew and current global sequence number is persistented in virDomainObj->privateData(qemuDomainObjPrivate)->nodenameindex * During hotplug, filter is created through QMP request("blockdev-add" with "driver":"throttle") to QEMU * Delete filters by "qemuBlockThrottleFiltersDetach"("blockdev-del") when detaching device * Use "iotune" and "throttlefilters" exclusively for specific disk

Signed-off-by: Chun Feng Wu<wucf@linux.ibm.com> ---   src/conf/domain_validate.c |   8 +++   src/qemu/qemu_block.c      | 131 +++++++++++++++++++++++++++++++++++++   src/qemu/qemu_block.h      |  53 +++++++++++++++   src/qemu/qemu_command.c    |  84 ++++++++++++++++++++++++   src/qemu/qemu_command.h    |   9 +++   src/qemu/qemu_domain.c     |  39 ++++++++++-   src/qemu/qemu_domain.h     |   8 +++   src/qemu/qemu_driver.c     |   6 ++   src/qemu/qemu_hotplug.c    |  33 ++++++++++   9 files changed, 370 insertions(+), 1 deletion(-)

diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 395e036e8f..4cc5ed7577 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -942,6 +942,14 @@ virDomainDiskDefValidate(const virDomainDef *def,           return -1;       }   +    if (disk->throttlefilters && (disk->blkdeviotune.group_name || + virDomainBlockIoTuneInfoHasAny(&disk->blkdeviotune))) { This is hard to read as you've broken up the line within a nested brace. It may confuse readers.

Rewrite as:

     if (disk->throttlefilters &&          (disk->blkdeviotune.group_name || virDomainBlockIoTuneInfoHasAny(&disk->blkdeviotune))) {

...

+ virReportError(VIR_ERR_XML_ERROR, Operation unsupported.

...

+                       _("block 'throttlefilters' can't be used together with 'iotune' for disk '%1$s'"), +                       disk->dst); +        return -1; +    } +       return 0;   }   diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c index 738b72d7ea..9b8ff65887 100644 --- a/src/qemu/qemu_block.c +++ b/src/qemu/qemu_block.c @@ -2739,6 +2739,137 @@ qemuBlockStorageSourceCreateDetectSize(GHashTable *blockNamedNodeData,   }     +void +qemuBlockThrottleFilterSetNodename(virDomainThrottleFilterDef *filter, +                                   char *nodename) +{ +    g_free(filter->nodename); +    filter->nodename = nodename; +} + + +const char * +qemuBlockThrottleFilterGetNodename(virDomainThrottleFilterDef *filter) +{ +    return filter->nodename; +} All of this helper infrastructure doesn't belong to a patch that is declaring that it's dealing with the setup of qemu. (I also wrote that below as I've noticed it there).

It makes this patch too complex and thus hard to review. It's also the reason it takes me so long. I'm demotivated on such commits as it takes way too long to go through.

Any setup of nodenames and other libvirt-internal stuff, such as validatio of config etc should be separated.

...

+/** + * qemuBlockThrottleFilterGetProps: + * @filter: throttle filter + * @parentNodeName: parent nodename of @filter + * + * Build "arguments" part of "blockdev-add" QMP cmd. + * e.g. {"execute":"blockdev-add", "arguments":{"driver":"throttle", + * "node-name":"libvirt-2-filter", "throttle-group":"limits0", + * "file": "libvirt-1-format"}} Avoid this JSON in comments, as it's obvious from the code. Rather describe any special handling if needed.

...

+ */ +virJSONValue * +qemuBlockThrottleFilterGetProps(virDomainThrottleFilterDef *filter, +                                const char *parentNodeName) +{ +    g_autoptr(virJSONValue) props = NULL; + +    if (virJSONValueObjectAdd(&props, +                              "s:driver", "throttle", +                              "s:node-name", qemuBlockThrottleFilterGetNodename(filter), +                              "s:throttle-group", filter->group_name, +                              "s:file", parentNodeName, +                              NULL) < 0) +        return 0; return NULL; It looks confusing because 0 is success in our code.

...

+ +    return g_steal_pointer(&props); +} [...]

...

+qemuBlockThrottleFilterAttachData * +qemuBlockThrottleFilterAttachPrepareBlockdev(virDomainThrottleFilterDef *filter, +                                             const char *parentNodeName) +{ +    g_autoptr(qemuBlockThrottleFilterAttachData) data = NULL; + +    data = g_new0(qemuBlockThrottleFilterAttachData, 1); + +    if (!(data->filterProps = qemuBlockThrottleFilterGetProps(filter, parentNodeName))) +        return NULL; + +    data->filterNodeName = qemuBlockThrottleFilterGetNodename(filter); +    data->filterAttached = true; Well, if you set this to true right here, it's pointless to even have the variable.

The code you've copied this from uses the 'Attached' variable so that it's set only after the object is created in qemu. This allows safe rollback.

If you set it to true for everything you will attempt to potentially roll back stuff that was not yet set in qemu, making the variable itself pointless.

...

+ +    return g_steal_pointer(&data); +} + + +void +qemuBlockThrottleFilterAttachRollback(qemuMonitor *mon, + qemuBlockThrottleFilterAttachData *data) +{ +    virErrorPtr orig_err; + +    virErrorPreserveLast(&orig_err); + +    if (data->filterAttached) +        ignore_value(qemuMonitorBlockdevDel(mon, data->filterNodeName)); ... here ...

...

+ +    virErrorRestore(&orig_err); +} + + +void +qemuBlockThrottleFiltersDataFree(qemuBlockThrottleFiltersData *data) +{ +    size_t i; + +    if (!data) +        return; + +    for (i = 0; i < data->nfilterdata; i++) + qemuBlockThrottleFilterAttachDataFree(data->filterdata[i]); + +    g_free(data->filterdata); +    g_free(data); +} + + +int +qemuBlockThrottleFiltersAttach(qemuMonitor *mon, +                               qemuBlockThrottleFiltersData *data) +{ +    size_t i; + +    for (i = 0; i < data->nfilterdata; i++) { +        if (qemuMonitorBlockdevAdd(mon, &data->filterdata[i]->filterProps) < 0) This function requires the monitor to be locked. If you want to have this as a separate function you must document it and state this fact.

...

+            return -1; +        data->filterdata[i]->filterAttached = true; +    } + +    return 0; +} + + +void +qemuBlockThrottleFiltersDetach(qemuMonitor *mon, +                               qemuBlockThrottleFiltersData *data) +{ +    size_t i; + +    for (i = data->nfilterdata; i > 0; i--) +        qemuBlockThrottleFilterAttachRollback(mon, data->filterdata[i-1]); +} + +   int   qemuBlockRemoveImageMetadata(virQEMUDriver *driver,                                virDomainObj *vm, diff --git a/src/qemu/qemu_block.h b/src/qemu/qemu_block.h index f9e961d85d..9888954ce4 100644 --- a/src/qemu/qemu_block.h +++ b/src/qemu/qemu_block.h @@ -214,6 +214,59 @@ qemuBlockStorageSourceCreateDetectSize(GHashTable *blockNamedNodeData,                                          virStorageSource *src,                                          virStorageSource *templ);   +void +qemuBlockThrottleFilterSetNodename(virDomainThrottleFilterDef *filter, +                                   char *nodename); + +const char * +qemuBlockThrottleFilterGetNodename(virDomainThrottleFilterDef *filter); + +virJSONValue * +qemuBlockThrottleFilterGetProps(virDomainThrottleFilterDef *filter, +                                const char *parentNodeName); This function isn't used outside of qemu_block.c so it doesn't need to be exported.

...

+ +typedef struct qemuBlockThrottleFilterAttachData qemuBlockThrottleFilterAttachData; +struct qemuBlockThrottleFilterAttachData { +    virJSONValue *filterProps; +    const char *filterNodeName; +    bool filterAttached; +}; + +qemuBlockThrottleFilterAttachData * +qemuBlockThrottleFilterAttachPrepareBlockdev(virDomainThrottleFilterDef *throttlefilter, +                                             const char *parentNodeName); + +void +qemuBlockThrottleFilterAttachDataFree(qemuBlockThrottleFilterAttachData *data); + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuBlockThrottleFilterAttachData, + qemuBlockThrottleFilterAttachDataFree); + +void +qemuBlockThrottleFilterAttachRollback(qemuMonitor *mon, + qemuBlockThrottleFilterAttachData *data); + +struct _qemuBlockThrottleFiltersData { +    qemuBlockThrottleFilterAttachData **filterdata; +    size_t nfilterdata; +}; + +typedef struct _qemuBlockThrottleFiltersData qemuBlockThrottleFiltersData; + +void +qemuBlockThrottleFiltersDataFree(qemuBlockThrottleFiltersData *data); + +G_DEFINE_AUTOPTR_CLEANUP_FUNC(qemuBlockThrottleFiltersData, + qemuBlockThrottleFiltersDataFree); + +int +qemuBlockThrottleFiltersAttach(qemuMonitor *mon, +                               qemuBlockThrottleFiltersData *data); + +void +qemuBlockThrottleFiltersDetach(qemuMonitor *mon, +                               qemuBlockThrottleFiltersData *data); +   int   qemuBlockRemoveImageMetadata(virQEMUDriver *driver,                                virDomainObj *vm, diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 2d0eddc79e..5ccae956d3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1582,6 +1582,13 @@ qemuDiskConfigBlkdeviotuneEnabled(const virDomainDiskDef *disk)   }     +bool +qemuDiskConfigThrottleFiltersEnabled(const virDomainDiskDef *disk) This function doesn't need to be exported.

...

+{ +    return disk->nthrottlefilters > 0; +} + +   /**    * qemuDiskBusIsSD:    * @bus: disk bus @@ -11055,6 +11062,83 @@ qemuBuildStorageSourceChainAttachPrepareBlockdevOne(qemuBlockStorageSourceChainD   }     +/** + * qemuBuildThrottleFiltersAttachPrepareBlockdevOne: + * @data: filter chain data, which consists of array of filters and size of such array + * @throttlefilter: new filter to be added into filter array + * @parentNodeName: parent nodename for this new throttlefilter, which is used to build "blockdev-add" QMP request the stuff after the comma can be dropped.

...

+ * + * Build filter node chain to provide more flexibility for block disk I/O limits + */ +static int +qemuBuildThrottleFiltersAttachPrepareBlockdevOne(qemuBlockThrottleFiltersData *data, + virDomainThrottleFilterDef *throttlefilter, +                                                 const char *parentNodeName) +{ +    g_autoptr(qemuBlockThrottleFilterAttachData) elem = NULL; + +    if (!(elem = qemuBlockThrottleFilterAttachPrepareBlockdev(throttlefilter, parentNodeName))) +        return -1; + +    VIR_APPEND_ELEMENT(data->filterdata, data->nfilterdata, elem); +    return 0; +} + + +/** + * qemuBuildThrottleFiltersAttachPrepareBlockdev: + * @disk: domain disk + * + * Build filter node chain to provide more flexibility for block disk I/O limits + */ +qemuBlockThrottleFiltersData * +qemuBuildThrottleFiltersAttachPrepareBlockdev(virDomainDiskDef *disk) +{ +    g_autoptr(qemuBlockThrottleFiltersData) data = NULL; +    size_t i; +    const char * parentNodeName = NULL; +    g_autofree char *tmp_nodename = NULL; + +    data = g_new0(qemuBlockThrottleFiltersData, 1); +    /* get starting parentNodename, e.g. libvirt-1-format */ +    parentNodeName = qemuBlockStorageSourceGetEffectiveNodename(disk->src); +    /* build filterdata, which contains all filters info and sequence info through parentNodeName */ +    for (i = 0; i < disk->nthrottlefilters; i++) { +        tmp_nodename = g_strdup(parentNodeName); +        if (qemuBuildThrottleFiltersAttachPrepareBlockdevOne(data, disk->throttlefilters[i], tmp_nodename) < 0) +            return NULL; The nodename copied into 'tmp_nodename' is leaked on every iteration. Also qemuBuildThrottleFiltersAttachPrepareBlockdevOne doesn't modify it so there's no point in copying it.

...

+        parentNodeName = disk->throttlefilters[i]->nodename; +    } + +    return g_steal_pointer(&data); +} + + +/** + * qemuBuildThrottleFiltersDetachPrepareBlockdev: + * @disk: domain disk + * + * Build filters data for later "blockdev-del" + */ +qemuBlockThrottleFiltersData * +qemuBuildThrottleFiltersDetachPrepareBlockdev(virDomainDiskDef *disk) +{ +    g_autoptr(qemuBlockThrottleFiltersData) data = g_new0(qemuBlockThrottleFiltersData, 1); +    size_t i; + +    /* build filterdata, which contains filters info and sequence info */ +    for (i = 0; i < disk->nthrottlefilters; i++) { +        g_autoptr(qemuBlockThrottleFilterAttachData) elem = g_new0(qemuBlockThrottleFilterAttachData, 1); +        /* ignore other fields since the following info are enough for "blockdev-del" */ +        elem->filterNodeName = qemuBlockThrottleFilterGetNodename(disk->throttlefilters[i]); So I didn't yet see any code that serializes any of this in the status XML, thus it seems that this will not work after you restart libvirtd/virtqemud if a VM with filters is running, and then try to detach disks. You'll need to add that, or base the filter nodename on something else.

Note that presence of the node name is authoritative and we must not try to regenerate it. That would hinder changing the node names in the future.

See how the copyOnRead layer node name is stored.

Filter node name is generated by rule "libvirt-nodenameindex-filter" in method

"qemuDomainPrepareThrottleFilterBlockdev", which is called by

"qemuDomainPrepareDiskSourceBlockdev".

it follows the same way like "libvirt-nodenameindex-format" node.

And I tested restarting libvirtd, vm with throttle works well in this case.

...

...

+        elem->filterAttached = true; + +        VIR_APPEND_ELEMENT(data->filterdata, data->nfilterdata, elem); +    } +    return g_steal_pointer(&data); +} + +   /**    * qemuBuildStorageSourceChainAttachPrepareBlockdev:    * @top: storage source chain

[...]

...

diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 7ba2ea4a5e..2831036e23 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7989,7 +7989,8 @@ qemuDomainDetermineDiskChain(virQEMUDriver *driver,    * @disk: disk definition object    *    * Returns the pointer to the node-name of the topmost layer used by @disk as - * backend. Currently returns the nodename of the copy-on-read filter if enabled + * backend. Currently returns the nodename of top throttle filter if enabled + * or the nodename of the copy-on-read filter if enabled    * or the nodename of the top image's format driver. Empty disks return NULL.    * This must be used only with disks instantiated via -blockdev (thus not    * for SD cards). @@ -8005,6 +8006,10 @@ qemuDomainDiskGetTopNodename(virDomainDiskDef *disk)       if (disk->copy_on_read == VIR_TRISTATE_SWITCH_ON)           return priv->nodeCopyOnRead;   +    /* If disk has throttles, take top throttle node name */ +    if (disk->nthrottlefilters > 0) +        return disk->throttlefilters[disk->nthrottlefilters-1]->nodename;

        return disk->throttlefilters[disk->nthrottlefilters - 1]->nodename;

        (extra spaces around subtraction operator)

...

      return qemuBlockStorageSourceGetEffectiveNodename(disk->src);   }   @@ -11336,6 +11341,32 @@ qemuDomainPrepareStorageSourceBlockdevNodename(virDomainDiskDef *disk,   }     +int +qemuDomainPrepareThrottleFilterBlockdevNodename(virDomainThrottleFilterDef *filter, This function is used only in this file so it doesn't need to be exported. Also it's used only in one place so it can be inlined as it makes the code harder to follow than necessary.

...

+ const char *nodenameprefix) +{ +    char *nodename = g_strdup_printf("%s-filter", nodenameprefix); + +    qemuBlockThrottleFilterSetNodename(filter, nodename); + +    return 0; +} + + +int +qemuDomainPrepareThrottleFilterBlockdev(virDomainThrottleFilterDef *filter, +                                        qemuDomainObjPrivate *priv) This function is used only in this module thus it doesn't need to be exported. Also it can't fail so there's no point in having a return value.

...

+{ +    g_autofree char *nodenameprefix = NULL; + +    filter->id = qemuDomainStorageIDNew(priv); + +    nodenameprefix = g_strdup_printf("libvirt-%u", filter->id);         nodename = g_strdup_printf("libvirt-%u-filter" ...

Instead of the convoluted mess and the below call.

...

+ +    return qemuDomainPrepareThrottleFilterBlockdevNodename(filter, nodenameprefix); [...]

...

diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index f7e435d6d5..60989ae693 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -14828,6 +14828,12 @@ qemuDomainDiskBlockIoTuneIsSupported(virDomainDiskDef *disk)           return false;       }   +    if (disk->throttlefilters) { +        virReportError(VIR_ERR_CONFIG_UNSUPPORTED, +                       _("block 'iotune' can't be used together with 'throttlefilters' for disk '%1$s'"), disk->dst); +        return false; +    } Preferrably add all this infrastructure which doesn't deal with the setup of qemu in a separate patch.

This patch is overly complex as it intermixes all of these validation bits with the actual qemu interaction.

...

+       return true;   }   diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index 4a3f4f657e..103b9e9f00 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -657,6 +657,7 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,                               virDomainAsyncJob asyncJob)   {       g_autoptr(qemuBlockStorageSourceChainData) data = NULL; +    g_autoptr(qemuBlockThrottleFiltersData) filterData = NULL;       qemuDomainObjPrivate *priv = vm->privateData;       g_autoptr(virJSONValue) devprops = NULL;       bool extensionDeviceAttached = false; @@ -695,6 +696,19 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,           if (rc < 0)               goto rollback;   +        /* Setup throttling filters +        * add additional "blockdev-add"(throttle filter) between "blockdev-add" (qemuBlockStorageSourceChainAttach) and "device_add" (qemuDomainAttachExtensionDevice) Drop above line.

...

+        */ +        if ((filterData = qemuBuildThrottleFiltersAttachPrepareBlockdev(disk))) { +            if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0) +                return -1; +            /* QMP requests("blockdev-add" with "driver":"throttle") to QEMU */ +            rc = qemuBlockThrottleFiltersAttach(priv->mon, filterData); +            qemuDomainObjExitMonitor(vm); +            if (rc < 0) +                goto rollback; +        } The ordering is wrong. In the prepare step you are ordering the node name dependencies such as

  device -> copyOnRead Layer -> throttle -> image chain

OBviously to ensure hierarchy at hotplug they need to be plugged from the end.

This block here is placed _AFTER_ copyOnRead layer instantiation, so that will not work with disks with copyOnRead.

After updating qemuBuildThrottleFiltersAttachPrepareBlockdev and qemuDomainDiskGetTopNodename,

order can be adjusted to be "device -> throttle-> copyOnRead Layer-> image chain", and throttle

works with copyOnRead, but I am also considering your suggestion about per-storage-source

...

...

+           if (disk->transient) {               g_autoptr(qemuBlockStorageSourceAttachData) backend = NULL;               g_autoptr(GHashTable) blockNamedNodeData = NULL; @@ -766,6 +780,8 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,       if (extensionDeviceAttached) ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &disk->info));   +    qemuBlockThrottleFiltersDetach(priv->mon, filterData); +       qemuBlockStorageSourceChainDetach(priv->mon, data);         qemuDomainObjExitMonitor(vm); @@ -921,6 +937,14 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver,       bool releaseSeclabel = false;       int ret = -1;   +    if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { +        if (disk->nthrottlefilters > 0) { +            virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", +                        _("cdrom device with throttle filters isn't supported")); So and now this is why I don't like the fact that you are doing this on a per-disk level. On a per-image level (if you'd instantiate 'throttle' layers when adding the image) this would not be a problem.

I'd strongly prefer if you modify this such that the trottle layers will be instantiated at per-storage-source level both in XML and in the code.

regarding per-storage-source, do you mean xml like below? if so, when specifying "--throttle-groups" in "attach-disk"

it apply groups on top source(vm1_disk_2.qcow2) only? is there scenario to apply throttle-groups on backing store source?

  <disk type='file' device='disk'>       <driver name='qemu' type='qcow2'/>       <source file='/virt/disks/vm1_disk_2.qcow2' index='1'>           <throttlefilters>             <throttlefilter group='limit0'/>           </throttlefilters>       </source>       <backingStore type='file' index='4'>         <format type='qcow2'/>         <source file='/virt/disks/backing.qcow2'>           <throttlefilters>             <throttlefilter group='limit1'/>           </throttlefilters>         </source>         <backingStore/>       </backingStore>       <target dev='vdc' bus='virtio'/>       <alias name='virtio-disk2'/>       <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/>    </disk>

and regarding code part for per-storage-source, do you mean preparing throttle filter chain json within "qemuBuildStorageSourceChainAttachPrepareBlockdev(virStorageSource *top)"? if so, current "qemuBuildStorageSourceChainAttachPrepareBlockdev" doesn't include copy-on-read layer, however, throttlefilter chain depends on it for top source, in that case, should preparation of copy-on-read be moved into "qemuBuildStorageSourceChainAttachPrepareBlockdev" as well? if yes, current parameter "virStorageSource *top" is not enough, also, do you see any concern about updating "qemuBuildStorageSourceChainAttachPrepareBlockdev" for throttle chain, it seems it has a lot of callers

...

...

+            return -1; +        } +    } +       if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) {           virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",                          _("floppy device hotplug isn't supported")); @@ -4499,6 +4523,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriver *driver,   {       qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);       g_autoptr(qemuBlockStorageSourceChainData) diskBackend = NULL; +    g_autoptr(qemuBlockThrottleFiltersData) filterData = NULL;       size_t i;       qemuDomainObjPrivate *priv = vm->privateData;       int ret = -1; @@ -4537,6 +4562,14 @@ qemuDomainRemoveDiskDevice(virQEMUDriver *driver,           }       }   +    qemuDomainObjEnterMonitor(vm); +    /* QMP request("blockdev-del") to QEMU to delete throttle filter*/ +    if ((filterData = qemuBuildThrottleFiltersDetachPrepareBlockdev(disk))) { +        /* "qemuBlockThrottleFiltersDetach" is used in rollback within "qemuDomainAttachDiskGeneric" as well */ +        qemuBlockThrottleFiltersDetach(priv->mon, filterData); In which case this'd be automatic.

...

+    } +    qemuDomainObjExitMonitor(vm); +       qemuDomainObjEnterMonitor(vm);         if (diskBackend) -- 2.34.1

This patch should also contain the corresponding commandline infrastructure for the filter itself.

The disk backend code is specifically unified by using the intermediate struct so that it's guaranteed that both hotplug and commandline work the same. Thus they need to be added simultaenously.

-- Thanks and Regards,

Wu

On Thu, Aug 15, 2024 at 13:04:47 +0800, Chun Feng Wu wrote: (I'd suggest you trim irrelevant stuff when responding. It's hard to find what you've responded to in this massive message .

On 2024/8/9 22:04, Peter Krempa wrote:

...

On Wed, Jun 12, 2024 at 03:02:17 -0700,wucf@linux.ibm.com wrote:

...

From: Chun Feng Wu<wucf@linux.ibm.com>

[...]

...

...

+ * qemuBuildThrottleFiltersDetachPrepareBlockdev: + * @disk: domain disk + * + * Build filters data for later "blockdev-del" + */ +qemuBlockThrottleFiltersData * +qemuBuildThrottleFiltersDetachPrepareBlockdev(virDomainDiskDef *disk) +{ + g_autoptr(qemuBlockThrottleFiltersData) data = g_new0(qemuBlockThrottleFiltersData, 1); + size_t i; + + /* build filterdata, which contains filters info and sequence info */ + for (i = 0; i < disk->nthrottlefilters; i++) { + g_autoptr(qemuBlockThrottleFilterAttachData) elem = g_new0(qemuBlockThrottleFilterAttachData, 1); + /* ignore other fields since the following info are enough for "blockdev-del" */ + elem->filterNodeName = qemuBlockThrottleFilterGetNodename(disk->throttlefilters[i]); So I didn't yet see any code that serializes any of this in the status XML, thus it seems that this will not work after you restart libvirtd/virtqemud if a VM with filters is running, and then try to detach disks. You'll need to add that, or base the filter nodename on something else.

Note that presence of the node name is authoritative and we must not try to regenerate it. That would hinder changing the node names in the future.

See how the copyOnRead layer node name is stored.

Filter node name is generated by rule "libvirt-nodenameindex-filter" in method

"qemuDomainPrepareThrottleFilterBlockdev", which is called by

"qemuDomainPrepareDiskSourceBlockdev".

it follows the same way like "libvirt-nodenameindex-format" node.

And I tested restarting libvirtd, vm with throttle works well in this case.

The problem is that the actual error shows only in logs as the detaching of the backend-nodes is on a code path that can't meaningfully report errors. Did you check in the logs that: - the removal of the throttling node was actually requested - that it did have a proper node name That is on hot-unplug of the disk having such config after you've restarted libvirt. The code for other disks always stores the nodenames as-generated in the status XML (XML describing a running VM) which I don't see in this series thus I infer it doesn't/can't work. [...]

...

...

@@ -921,6 +937,14 @@ qemuDomainAttachDeviceDiskLiveInternal(virQEMUDriver *driver, bool releaseSeclabel = false; int ret = -1; + if (disk->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { + if (disk->nthrottlefilters > 0) { + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s", + _("cdrom device with throttle filters isn't supported")); So and now this is why I don't like the fact that you are doing this on a per-disk level. On a per-image level (if you'd instantiate 'throttle' layers when adding the image) this would not be a problem.

I'd strongly prefer if you modify this such that the trottle layers will be instantiated at per-storage-source level both in XML and in the code.

regarding per-storage-source, do you mean xml like below? if so, when specifying "--throttle-groups" in "attach-disk"

it apply groups on top source(vm1_disk_2.qcow2) only? is there scenario to apply throttle-groups on backing store source?

  <disk type='file' device='disk'>       <driver name='qemu' type='qcow2'/>       <source file='/virt/disks/vm1_disk_2.qcow2' index='1'>           <throttlefilters>             <throttlefilter group='limit0'/>           </throttlefilters>       </source>       <backingStore type='file' index='4'>         <format type='qcow2'/>         <source file='/virt/disks/backing.qcow2'>           <throttlefilters>             <throttlefilter group='limit1'/>           </throttlefilters>         </source>         <backingStore/>       </backingStore>       <target dev='vdc' bus='virtio'/>       <alias name='virtio-disk2'/>       <address type='pci' domain='0x0000' bus='0x00' slot='0x06' function='0x0'/>    </disk>

Yeah, something like this. That allows adding filters on other layers too. As I said it depends on how you expect to use this feature, because both make sense. I do reckon that in most cases this is an overkill though. If you decide that this is too complicated, you can use the approach you did, but then need to store the nodename of the throttle layer on the disk level in the status XML.

On Wed, Jun 12, 2024 at 03:02:23 -0700, wucf@linux.ibm.com wrote:

From: Chun Feng Wu <wucf@linux.ibm.com>

* Add new cmds: throttlegroupset, throttlegrouplist, throttlegroupinfo, throttlegroupdel

Signed-off-by: Chun Feng Wu <wucf@linux.ibm.com> --- tools/virsh-domain.c | 428 +++++++++++++++++++++++++++++++++++++++++++

This patch is missing addition to the manpage documenting the commands in docs/manpages/virsh.rst.

1 file changed, 428 insertions(+)

diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c index 50e80689a2..f84a65451a 100644 --- a/tools/virsh-domain.c +++ b/tools/virsh-domain.c @@ -1509,6 +1509,410 @@ cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) goto cleanup; }

+ +/* + * "throttlegrouplist" command + */ +static const vshCmdInfo info_throttlegrouplist = { + .help = N_("list all domain throttlegroups."), + .desc = N_("Get the summary of throttle groups for a domain."), +}; + + +static const vshCmdOptDef opts_throttlegrouplist[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + {.name = "inactive", + .type = VSH_OT_BOOL, + .help = N_("get inactive rather than running configuration") + }, + {.name = NULL} +}; + + +static bool +cmdThrottleGroupList(vshControl *ctl, + const vshCmd *cmd) +{ + unsigned int flags = 0; + size_t i; + g_autoptr(xmlDoc) xml = NULL; + g_autoptr(xmlXPathContext) ctxt = NULL; + g_autofree xmlNodePtr *groups = NULL; + ssize_t ngroups; + g_autoptr(vshTable) table = NULL; + + if (vshCommandOptBool(cmd, "inactive")) + flags |= VIR_DOMAIN_XML_INACTIVE; + + if (virshDomainGetXML(ctl, cmd, flags, &xml, &ctxt) < 0) + return false; + + ngroups = virXPathNodeSet("./throttlegroups/throttlegroup", ctxt, &groups); + if (ngroups < 0)

You need to report an error here, but on the other hand I don't think that having 0 groups is an error.

+ return false; + + table = vshTableNew(_("Name"), NULL); + + if (!table) + return false; + + for (i = 0; i < ngroups; i++) { + g_autofree char *name = NULL; + ctxt->node = groups[i]; + name = virXPathString("string(./group_name)", ctxt);

Here you don't handle NULL

+ if (vshTableRowAppend(table, name, NULL) < 0)

... which would make this fail use NULLSTR_EMPTY

+ return false; + } + + vshTablePrintToStdout(table, ctl); + + return true; +} + + +/* + * "throttlegroupset" command + */ +static const vshCmdInfo info_throttlegroupset = { + .help = N_("Add or update a throttling group."), + .desc = N_("Add or updte a throttling group."), +}; + + +static const vshCmdOptDef opts_throttlegroupset[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + {.name = "group-name", + .type = VSH_OT_STRING, + .positional = true, + .required = true, + .completer = virshCompleteEmpty,

It will be possible to auto-complete this argument so virshCompleteEmpty is not appropriate. That annotation must be used exclusively for fields which don't make sense tot be complted.

+ .help = N_("throttle group name") + }, + {.name = "total-bytes-sec", + .type = VSH_OT_INT, + .help = N_("total throughput limit, as scaled integer (default bytes)") + }, + {.name = "read-bytes-sec", + .type = VSH_OT_INT, + .help = N_("read throughput limit, as scaled integer (default bytes)") + }, + {.name = "write-bytes-sec", + .type = VSH_OT_INT, + .help = N_("write throughput limit, as scaled integer (default bytes)") + }, + {.name = "total-iops-sec", + .type = VSH_OT_INT, + .help = N_("total I/O operations limit per second") + }, + {.name = "read-iops-sec", + .type = VSH_OT_INT, + .help = N_("read I/O operations limit per second") + }, + {.name = "write-iops-sec", + .type = VSH_OT_INT, + .help = N_("write I/O operations limit per second") + }, + {.name = "total-bytes-sec-max", + .type = VSH_OT_INT, + .help = N_("total max, as scaled integer (default bytes)") + }, + {.name = "read-bytes-sec-max", + .type = VSH_OT_INT, + .help = N_("read max, as scaled integer (default bytes)") + }, + {.name = "write-bytes-sec-max", + .type = VSH_OT_INT, + .help = N_("write max, as scaled integer (default bytes)") + }, + {.name = "total-iops-sec-max", + .type = VSH_OT_INT, + .help = N_("total I/O operations max") + }, + {.name = "read-iops-sec-max", + .type = VSH_OT_INT, + .help = N_("read I/O operations max") + }, + {.name = "write-iops-sec-max", + .type = VSH_OT_INT, + .help = N_("write I/O operations max") + }, + {.name = "size-iops-sec", + .type = VSH_OT_INT, + .help = N_("I/O size in bytes") + }, + {.name = "total-bytes-sec-max-length", + .type = VSH_OT_INT, + .help = N_("duration in seconds to allow total max bytes") + }, + {.name = "read-bytes-sec-max-length", + .type = VSH_OT_INT, + .help = N_("duration in seconds to allow read max bytes") + }, + {.name = "write-bytes-sec-max-length", + .type = VSH_OT_INT, + .help = N_("duration in seconds to allow write max bytes") + }, + {.name = "total-iops-sec-max-length", + .type = VSH_OT_INT, + .help = N_("duration in seconds to allow total I/O operations max") + }, + {.name = "read-iops-sec-max-length", + .type = VSH_OT_INT, + .help = N_("duration in seconds to allow read I/O operations max") + }, + {.name = "write-iops-sec-max-length", + .type = VSH_OT_INT, + .help = N_("duration in seconds to allow write I/O operations max") + },

I guess that once we have two of the commands using these it'd make sense to have these as a macro to avoid having to update two places the next time another tunable is added.

+ VIRSH_COMMON_OPT_DOMAIN_CONFIG, + VIRSH_COMMON_OPT_DOMAIN_LIVE, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, + {.name = NULL} +}; + + +static bool +cmdThrottleGroupSet(vshControl *ctl, + const vshCmd *cmd) +{ + g_autoptr(virshDomain) dom = NULL; + const char *name; + const char *group_name = NULL; + unsigned long long value; + int nparams = 0; + int maxparams = 0; + virTypedParameterPtr params = NULL; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + int rv = 0; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool ret = false; + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(dom = virshCommandOptDomain(ctl, cmd, &name))) + goto cleanup;

This function doesn't use 'name' any more so there's no point in fetching it.

+ + +#define VSH_SET_THROTTLE_GROUP_SCALED(PARAM, CONST) \ + if ((rv = vshCommandOptScaledInt(ctl, cmd, #PARAM, &value, \ + 1, ULLONG_MAX)) < 0) { \ + goto interror; \ + } else if (rv > 0) { \ + if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, \ + VIR_DOMAIN_BLOCK_IOTUNE_##CONST, \ + value) < 0) \ + goto save_error; \ + } + + VSH_SET_THROTTLE_GROUP_SCALED(total-bytes-sec, TOTAL_BYTES_SEC); + VSH_SET_THROTTLE_GROUP_SCALED(read-bytes-sec, READ_BYTES_SEC); + VSH_SET_THROTTLE_GROUP_SCALED(write-bytes-sec, WRITE_BYTES_SEC); + VSH_SET_THROTTLE_GROUP_SCALED(total-bytes-sec-max, TOTAL_BYTES_SEC_MAX); + VSH_SET_THROTTLE_GROUP_SCALED(read-bytes-sec-max, READ_BYTES_SEC_MAX); + VSH_SET_THROTTLE_GROUP_SCALED(write-bytes-sec-max, WRITE_BYTES_SEC_MAX); +#undef VSH_SET_THROTTLE_GROUP_SCALED + +#define VSH_SET_THROTTLE_GROUP(PARAM, CONST) \ + if ((rv = vshCommandOptULongLong(ctl, cmd, #PARAM, &value)) < 0) { \ + goto interror; \ + } else if (rv > 0) { \ + if (virTypedParamsAddULLong(¶ms, &nparams, &maxparams, \ + VIR_DOMAIN_BLOCK_IOTUNE_##CONST, \ + value) < 0) \ + goto save_error; \ + } + + VSH_SET_THROTTLE_GROUP(total-iops-sec, TOTAL_IOPS_SEC); + VSH_SET_THROTTLE_GROUP(read-iops-sec, READ_IOPS_SEC); + VSH_SET_THROTTLE_GROUP(write-iops-sec, WRITE_IOPS_SEC); + VSH_SET_THROTTLE_GROUP(total-iops-sec-max, TOTAL_IOPS_SEC_MAX); + VSH_SET_THROTTLE_GROUP(read-iops-sec-max, READ_IOPS_SEC_MAX); + VSH_SET_THROTTLE_GROUP(write-iops-sec-max, WRITE_IOPS_SEC_MAX); + VSH_SET_THROTTLE_GROUP(size-iops-sec, SIZE_IOPS_SEC); + + VSH_SET_THROTTLE_GROUP(total-bytes-sec-max-length, TOTAL_BYTES_SEC_MAX_LENGTH); + VSH_SET_THROTTLE_GROUP(read-bytes-sec-max-length, READ_BYTES_SEC_MAX_LENGTH); + VSH_SET_THROTTLE_GROUP(write-bytes-sec-max-length, WRITE_BYTES_SEC_MAX_LENGTH); + VSH_SET_THROTTLE_GROUP(total-iops-sec-max-length, TOTAL_IOPS_SEC_MAX_LENGTH); + VSH_SET_THROTTLE_GROUP(read-iops-sec-max-length, READ_IOPS_SEC_MAX_LENGTH); + VSH_SET_THROTTLE_GROUP(write-iops-sec-max-length, WRITE_IOPS_SEC_MAX_LENGTH); +#undef VSH_SET_THROTTLE_GROUP + + if (vshCommandOptString(ctl, cmd, "group-name", &group_name) < 0) { + goto cleanup; + } + + if (group_name) {

gropu name is guaranteed to exist ...

+ if (virTypedParamsAddString(¶ms, &nparams, &maxparams, + VIR_DOMAIN_BLOCK_IOTUNE_GROUP_NAME, + group_name) < 0) + goto save_error; + } + + if (virDomainSetThrottleGroup(dom, group_name, params, nparams, flags) < 0) + goto error;

otherwis this'd fail anyways.

+ vshPrintExtra(ctl, "%s", _("Throttle group set successfully\n")); + + ret = true; + + cleanup: + virTypedParamsFree(params, nparams); + return ret; + + save_error: + vshSaveLibvirtError(); + error: + vshError(ctl, "%s", _("Unable to change throttle group")); + goto cleanup; + + interror: + vshError(ctl, "%s", _("Unable to parse integer parameter")); + goto cleanup; +} + + +/* + * "throttlegroupdel" command + */ +static const vshCmdInfo info_throttlegroupdel = { + .help = N_("Delete a throttling group."), + .desc = N_("Delete a throttling group."), +}; + + +static const vshCmdOptDef opts_throttlegroupdel[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + {.name = "group-name", + .type = VSH_OT_STRING, + .positional = true, + .required = true, + .completer = virshCompleteEmpty,

Same as above regarding the annotation.

+ .help = N_("throttle group name") + }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, + VIRSH_COMMON_OPT_DOMAIN_LIVE, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, + {.name = NULL} +}; + + +static bool +cmdThrottleGroupDel(vshControl *ctl, + const vshCmd *cmd) +{ + g_autoptr(virshDomain) dom = NULL; + const char *group_name = NULL; + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool current = vshCommandOptBool(cmd, "current"); + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(dom = virshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptString(ctl, cmd, "group-name", &group_name) < 0) { + return false; + } + + if (virDomainDelThrottleGroup(dom, group_name, flags) < 0) + return false; + vshPrintExtra(ctl, "%s", _("Throttle group deleted successfully\n")); + + return true; +} + + +/* + * "throttlegroupinfo" command + */ +static const vshCmdInfo info_throttlegroupinfo = { + .help = N_("Get a throttling group."), + .desc = N_("Get a throttling group."), +}; + + +static const vshCmdOptDef opts_throttlegroupinfo[] = { + VIRSH_COMMON_OPT_DOMAIN_FULL(0), + {.name = "group-name", + .type = VSH_OT_STRING, + .positional = true, + .required = true, + .completer = virshCompleteEmptya

Same as above. ,

+ .help = N_("throttle group name") + }, + VIRSH_COMMON_OPT_DOMAIN_CONFIG, + VIRSH_COMMON_OPT_DOMAIN_LIVE, + VIRSH_COMMON_OPT_DOMAIN_CURRENT, + {.name = NULL} +}; + + +static bool +cmdThrottleGroupInfo(vshControl *ctl, + const vshCmd *cmd) +{ + g_autoptr(virshDomain) dom = NULL; + const char *name; + const char *group_name = NULL; + int nparams = 0; + virTypedParameterPtr params = NULL; + unsigned int flags = VIR_DOMAIN_AFFECT_CURRENT; + size_t i; + bool current = vshCommandOptBool(cmd, "current"); + bool config = vshCommandOptBool(cmd, "config"); + bool live = vshCommandOptBool(cmd, "live"); + bool ret = false; + + VSH_EXCLUSIVE_OPTIONS_VAR(current, live); + VSH_EXCLUSIVE_OPTIONS_VAR(current, config); + + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + + if (!(dom = virshCommandOptDomain(ctl, cmd, &name))) + goto cleanup;

Name is not used.

+ + if (vshCommandOptString(ctl, cmd, "group-name", &group_name) < 0) { + goto cleanup; + } + + if (virDomainGetThrottleGroup(dom, group_name, ¶ms, &nparams, flags) != 0) { + vshError(ctl, "%s", + _("Unable to get throttle group parameters"));

Misaligned.

+ goto cleanup; + } + + for (i = 0; i < nparams; i++) { + g_autofree char *str = vshGetTypedParamValue(ctl, ¶ms[i]); + vshPrint(ctl, "%-15s: %s\n", params[i].field, str); + } + + ret = true; + + cleanup: + virTypedParamsFree(params, nparams); + return ret; +} + + /* * "blkiotune" command */ @@ -13378,6 +13782,30 @@ const vshCmdDef domManagementCmds[] = { .info = &info_blkdeviotune, .flags = 0 }, + {.name = "throttlegroupset", + .handler = cmdThrottleGroupSet, + .opts = opts_throttlegroupset, + .info = &info_throttlegroupset, + .flags = 0 + }, + {.name = "throttlegroupdel", + .handler = cmdThrottleGroupDel, + .opts = opts_throttlegroupdel, + .info = &info_throttlegroupdel, + .flags = 0 + }, + {.name = "throttlegroupinfo", + .handler = cmdThrottleGroupInfo, + .opts = opts_throttlegroupinfo, + .info = &info_throttlegroupinfo, + .flags = 0 + }, + {.name = "throttlegrouplist", + .handler = cmdThrottleGroupList, + .opts = opts_throttlegrouplist, + .info = &info_throttlegrouplist, + .flags = 0 + },

I suggest using the 'dom' prefix as all of the objects are per-domain.

{.name = "blkiotune", .handler = cmdBlkiotune, .opts = opts_blkiotune, -- 2.34.1

On Wed, Jun 12, 2024 at 03:02:24 -0700, wucf@linux.ibm.com wrote:

From: Chun Feng Wu <wucf@linux.ibm.com>

* Update "attach_disk" to support new option: throttle-groups to form filter chain in QEMU for specific disk

Signed-off-by: Chun Feng Wu <wucf@linux.ibm.com> --- tools/virsh-completer-domain.c | 64 ++++++++++++++++++++++++++++++++++ tools/virsh-completer-domain.h | 5 +++ tools/virsh-domain.c | 25 ++++++++++++- 3 files changed, 93 insertions(+), 1 deletion(-)

Missing corresponding manpage update.

diff --git a/tools/virsh-completer-domain.c b/tools/virsh-completer-domain.c index 61362224a3..000cf65c99 100644 --- a/tools/virsh-completer-domain.c +++ b/tools/virsh-completer-domain.c @@ -248,6 +248,70 @@ virshDomainMigrateDisksCompleter(vshControl *ctl, }

+static char ** +virshDomainThrottleGroupCompleter(vshControl *ctl, + const vshCmd *cmd, + unsigned int flags)

Well, you see that it's possible to complete these. Add this helper beforehand and use it approprately also when adding the helpers in previous patch.

+{ + virshControl *priv = ctl->privData; + g_autoptr(xmlDoc) xmldoc = NULL; + g_autoptr(xmlXPathContext) ctxt = NULL; + g_autofree xmlNodePtr *groups = NULL; + int ngroups; + size_t i; + g_auto(GStrv) tmp = NULL; + + virCheckFlags(0, NULL); + + if (!priv->conn || virConnectIsAlive(priv->conn) <= 0) + return NULL; + + if (virshDomainGetXML(ctl, cmd, 0, &xmldoc, &ctxt) < 0) + return NULL; + + ngroups = virXPathNodeSet("./throttlegroups/throttlegroup", ctxt, &groups); + if (ngroups < 0) + return NULL; + + tmp = g_new0(char *, ngroups + 1); + + for (i = 0; i < ngroups; i++) { + ctxt->node = groups[i]; + if (!(tmp[i] = virXPathString("string(./group_name)", ctxt))) + return NULL;

Since this also does everything that the 'throttlegrouplist' command does, you can theoretically even reuse this there and use it to fill the table.

+ } + + return g_steal_pointer(&tmp); +}

[...]