On Mon, Nov 18, 2024 at 19:24:19 +0530, Harikumar R wrote:
> From: Chun Feng Wu <danielwuwy@xxxxxxx>
>
> For hot attaching/detaching
> * Leverage qemuBlockThrottleFiltersData to prepare attaching/detaching
> throttle filter data for qemuMonitorBlockdevAdd and qemuMonitorBlockdevDel
> * For hot attaching, within qemuDomainAttachDiskGeneric,prepare throttle
> filters json data, and create corresponding blockdev for QMP request
> ("blockdev-add" with "driver":"throttle")
> * Each filter has a nodename, and those filters are chained up,
> create them in sequence, and delete them reversely
> * Delete filters by "qemuBlockThrottleFiltersDetach"("blockdev-del")
> when detaching device
>
> For throttle group commandline
> * Add qemuBuildThrottleGroupCommandLine in qemuBuildCommandLine to add
> "object" of throttle-group
> * Verify throttle group definition when lauching vm
> * Check QEMU_CAPS_OBJECT_JSON before "qemuBuildObjectCommandlineFromJSON",
> which is to build "-object" option
>
> For throttle filter commandline
> * Add qemuBuildDiskThrottleFiltersCommandLine in qemuBuildDiskCommandLine
> to add "blockdev"
>
> Signed-off-by: Chun Feng Wu <danielwuwy@xxxxxxx>
> ---
> src/qemu/qemu_command.c | 99 +++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_hotplug.c | 25 +++++++++++
> 2 files changed, 124 insertions(+)
>
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 0c119c8827..19c8ab1b7f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2022,6 +2022,99 @@ qemuBuildBlockStorageSourceAttachDataCommandline(virCommand *cmd,
> }
>
>
> +static bool
> +qemuDiskConfigThrottleGroupEnabled(const virDomainThrottleGroupDef *group)
This helper doesn't seem to be used elsewhere; inline it.
> +{
> + return !!group->group_name &&
> + virDomainBlockIoTuneInfoHasAny(group);
> +}
> +
> +
> +/**
> + * qemuBuildThrottleGroupCommandLine:
> + * @cmd: the command to modify
> + * @def: domain definition
> + * @qemuCaps: qemu capabilities object
> + *
> + * build throttle group object in json format
> + */
> +static int
> +qemuBuildThrottleGroupCommandLine(virCommand *cmd,
> + const virDomainDef *def,
> + virQEMUCaps *qemuCaps)
> +{
> + size_t i;
> +
> + for (i = 0; i < def->nthrottlegroups; i++) {
> + g_autoptr(virJSONValue) props = NULL;
> + g_autoptr(virJSONValue) limits = virJSONValueNewObject();
> + virDomainThrottleGroupDef *group = def->throttlegroups[i];
> + /* prefix group name with "throttle-" in QOM */
> + g_autofree char *prefixed_group_name = g_strdup_printf("throttle-%s", group->group_name);
> +
> + if (!qemuDiskConfigThrottleGroupEnabled(group)) {
> + continue;
> + }
... here.
> +
> + if (qemuMonitorThrottleGroupLimits(limits, group) < 0)
> + return -1;
> +
> + if (qemuMonitorCreateObjectProps(&props, "throttle-group", prefixed_group_name,
> + "a:limits", &limits,
> + NULL) < 0)
> + return -1;
> +
> + /* this throttle group feature requires "QEMU_CAPS_OBJECT_JSON" */
So by itself this comment is useless. It doesn't say *why* this is in
fact needed. Can you elaborate? Specifically the
qemuBuildObjectCommandlineFromJSON helper should be able to conver the
above to the dotted syntax, so I'm guessing the error is on qemu's side.
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_JSON)) {
> + virReportError(VIR_ERR_OPERATION_UNSUPPORTED, "%s",
> + _("This QEMU doesn't support throttle group creation"));
> + return -1;
> + }
Add empty line.
> + if (qemuBuildObjectCommandlineFromJSON(cmd, props, qemuCaps) < 0)
> + return -1;
> + }
> +
> + return 0;
> +
> +
> +static int
> +qemuBuildBlockThrottleFilterCommandline(virCommand *cmd,
> + qemuBlockThrottleFilterAttachData *data)
> +{
> + if (data->filterProps) {
> + g_autofree char *tmp = NULL;
> + if (!(tmp = virJSONValueToString(data->filterProps, false)))
> + return -1;
> +
> + virCommandAddArgList(cmd, "-blockdev", tmp, NULL);
> + }
> +
> + return 0;
> +}
> +
> +
> +static int
> +qemuBuildDiskThrottleFiltersCommandLine(virCommand *cmd,
> + virDomainDiskDef *disk)
> +{
> + g_autoptr(qemuBlockThrottleFiltersData) data = NULL;
> + size_t i;
> +
> + data = qemuBuildThrottleFiltersAttachPrepareBlockdev(disk);
> + if (!data)
> + return -1;
> +
> + for (i = 0; i < data->nfilterdata; i++) {
> + if (qemuBuildBlockThrottleFilterCommandline(cmd,
> + data->filterdata[i]) < 0)
> + return -1;
This patch seems to be split questionably. The previous patch added this
infrastructure so it doesn't make too much sense to be here.
For now I think we should leave it as-is to not create any additional
churn in this series, but for the next time please split them more
logically.
> + }
> +
> + return 0;
> +}
> +
> +
> static int
> qemuBuildDiskSourceCommandLine(virCommand *cmd,
> virDomainDiskDef *disk,
> @@ -2079,6 +2172,9 @@ qemuBuildDiskCommandLine(virCommand *cmd,
> if (qemuBuildDiskSourceCommandLine(cmd, disk, qemuCaps) < 0)
> return -1;
>
> + if (qemuBuildDiskThrottleFiltersCommandLine(cmd, disk) < 0)
> + return -1;
> +
> /* SD cards are currently instantiated via -drive if=sd, so the -device
> * part must be skipped */
> if (qemuDiskBusIsSD(disk->bus))
> @@ -10435,6 +10531,9 @@ qemuBuildCommandLine(virDomainObj *vm,
> if (qemuBuildIOThreadCommandLine(cmd, def, qemuCaps) < 0)
> return NULL;
>
> + if (qemuBuildThrottleGroupCommandLine(cmd, def, qemuCaps) < 0)
> + return NULL;
> +
> if (virDomainNumaGetNodeCount(def->numa) &&
> qemuBuildNumaCommandLine(cfg, def, cmd, priv) < 0)
> return NULL;
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index bddd553c88..bf001aa902 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -708,6 +708,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;
> @@ -746,6 +747,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 the comment.
> + if ((filterData = qemuBuildThrottleFiltersAttachPrepareBlockdev(disk))) {
> + if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0)
> + return -1;
> + /* QMP requests("blockdev-add" with "driver":"throttle") to QEMU */
pointless comment
> + rc = qemuBlockThrottleFiltersAttach(priv->mon, filterData);
> + qemuDomainObjExitMonitor(vm);
> + if (rc < 0)
> + goto rollback;
> + }
> +
> if (disk->transient) {
> g_autoptr(qemuBlockStorageSourceAttachData) backend = NULL;
> g_autoptr(GHashTable) blockNamedNodeData = NULL;
> @@ -817,6 +831,8 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
> if (extensionDeviceAttached)
> ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &disk->info));
>
> + qemuBlockThrottleFiltersDetach(priv->mon, filterData);
> +
> qemuBlockStorageSourceChainDetach(priv->mon, data);
>
> qemuDomainObjExitMonitor(vm);
> @@ -4642,6 +4658,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;
> @@ -4680,6 +4697,14 @@ qemuDomainRemoveDiskDevice(virQEMUDriver *driver,
> }
> }
>
> + qemuDomainObjEnterMonitor(vm);
> + /* QMP request("blockdev-del") to QEMU to delete throttle filter*/
pointless comment
> + if ((filterData = qemuBuildThrottleFiltersDetachPrepareBlockdev(disk))) {
> + /* "qemuBlockThrottleFiltersDetach" is used in rollback within "qemuDomainAttachDiskGeneric" as well */
pointless comment
> + qemuBlockThrottleFiltersDetach(priv->mon, filterData);
> + }
> + qemuDomainObjExitMonitor(vm);
> +
> qemuDomainObjEnterMonitor(vm);
>
> if (diskBackend)
> --
> 2.39.5 (Apple Git-154)
> a
With the changes above:
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
