On Mon, Nov 18, 2024 at 19:24:17 +0530, Harikumar R wrote:
> From: Chun Feng Wu <danielwuwy@xxxxxxx>
>
> ThrottleGroup lifecycle implementation, note, in QOM, throttlegroup name is prefixed with
> "throttle-" to clearly separate throttle group objects into their own namespace.
> * "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 when vm is active,
> throttle group feature requries such flag
> * "objectAddNoWrap"("props") check is done by reusing qemuMonitorAddObject in
> qemuDomainSetThrottleGroup
>
> Signed-off-by: Chun Feng Wu <danielwuwy@xxxxxxx>
> ---
> src/qemu/qemu_driver.c | 388 +++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 388 insertions(+)
>
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e80e5fc511..d65d5fd2ff 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -20111,6 +20111,390 @@ qemuDomainGraphicsReload(virDomainPtr domain,
> return ret;
> }
>
> +
> +/**
> + * qemuDomainCheckThrottleGroupAllZero:
> + * @newiotune: Pointer to iotune, which contains detailed items
> + *
> + * Check if params within @newiotune contain all zero values, if yes,
> + * return failure since it's meaningless to set all zero values in @newiotune
> + *
> + * Returns -1 on failure, or 0 on success.
> + */
> +static int
> +qemuDomainCheckThrottleGroupAllZero(virDomainBlockIoTuneInfo *newiotune)
> +{
> + if (virDomainBlockIoTuneInfoHasAny(newiotune))
> + return 0;
> +
> + VIR_FREE(newiotune->group_name);
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("creating a new group/updating existing with all parameters zero is not supported"));
> + return -1;
> +}
> +
> +
> +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;
> +
> + 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);
The code from qemuDomainCheckThrottleGroupAllZero should be inlined here
as it doesn't make to check the data twice.
> +
> + 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 (virDomainObjIsActive(vm)) {
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_JSON)) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("This QEMU doesn't support throttle group creation"));
> + return -1;
> + }
> + }
> +
> + if (def) {
> + if (qemuDomainCheckThrottleGroupAllZero(&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 {
> + /* prefix group name with "throttle-" in QOM */
> + g_autofree char *prefixed_group_name = g_strdup_printf("throttle-%s", groupname);
> +
> + if (qemuMonitorThrottleGroupLimits(limits, &info)<0)
> + goto endjob;
> + if (qemuMonitorCreateObjectProps(&props,
> + "throttle-group", prefixed_group_name,
> + "a:limits", &limits,
> + NULL) < 0)
> + goto endjob;
> + qemuDomainObjEnterMonitor(vm);
> + 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 (qemuDomainCheckThrottleGroupAllZero(&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)
As noted, this getter is mostly pointless as it just returns what should
have been recorded in the XML in the first place so I don't think this
makes too much sense to have.
> +{
> + 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, -1);
> +
> +
> + if (!(vm = qemuDomainObjFromDomain(dom)))
> + return -1;
> +
> + if (virDomainGetThrottleGroupEnsureACL(dom->conn, vm->def) < 0)
> + goto cleanup;
> +
> + if (virDomainObjBeginJob(vm, VIR_JOB_QUERY) < 0)
> + goto cleanup;
> +
> + if (virDomainObjGetDefs(vm, flags, &def, &persistentDef) < 0)
> + goto endjob;
> +
> + if (def) {
> + if (virDomainThrottleGroupByName(def, groupname) == NULL) {
> + virReportError(VIR_ERR_INVALID_ARG,
> + _("throttle group '%1$s' was not found in the domain config"),
> + groupname);
> + goto endjob;
> + }
> + qemuDomainObjEnterMonitor(vm);
> + rc = qemuMonitorGetThrottleGroup(qemuDomainGetMonitor(vm), groupname, reply);
> + 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);
> + }
As noted you can't really combide VIR_DOMAIN_AFFECT_LIVE
VIR_DOMAIN_AFFECT_CONFIG as it'll return garbage.
> +
> + *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);
The rest looks reasonable.
