On Thu, Apr 11, 2024 at 19:01:53 -0700, wucf@xxxxxxxxxxxxx wrote:
> From: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
>
> 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
> * Each filter has a nodename, and those filters are chained up in sequence
> * Filter nodename index is persistented in virDomainObj->privateData(qemuDomainObjPrivate)
> * During hotplug, filter is created through QMP request("blockdev-add" with "driver":"throttle") to QEMU
> * Finally, "device_add"(attach) QMP request is adjusted to set "drive" to be top filter nodename in chain.
>
> Signed-off-by: Chun Feng Wu <wucf@xxxxxxxxxxxxx>
> ---a
As noted before, the proper approach is to add the XML bits first so
some asumptions may be off.
Before next version please add implementation only after the XML and
docs is added.
> src/qemu/qemu_block.c | 131 ++++++++++++++++++
> src/qemu/qemu_block.h | 53 +++++++
> src/qemu/qemu_command.c | 64 +++++++++
> src/qemu/qemu_command.h | 6 +
> src/qemu/qemu_domain.c | 71 ++++++++++
> src/qemu/qemu_domain.h | 19 ++-
> src/qemu/qemu_hotplug.c | 24 ++++
> .../qemustatusxml2xmldata/backup-pull-in.xml | 1 +
> .../blockjob-blockdev-in.xml | 1 +
> .../blockjob-mirror-in.xml | 1 +
> .../migration-in-params-in.xml | 1 +
> .../migration-out-nbd-bitmaps-in.xml | 1 +
> .../migration-out-nbd-out.xml | 1 +
> .../migration-out-nbd-tls-out.xml | 1 +
> .../migration-out-params-in.xml | 1 +
> tests/qemustatusxml2xmldata/modern-in.xml | 1 +
> tests/qemustatusxml2xmldata/upgrade-out.xml | 1 +
> .../qemustatusxml2xmldata/vcpus-multi-in.xml | 1 +
> 18 files changed, 375 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 738b72d7ea..c53de2465e 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;
> +}
> +
> +
> +/**
> + * 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"}}
> + */
> +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 g_steal_pointer(&props);
> +}
> +
> +
> +void
> +qemuBlockThrottleFilterAttachDataFree(qemuBlockThrottleFilterAttachData *data)
> +{
> + if (!data)
> + return;
> +
> + virJSONValueFree(data->filterProps);
> + g_free(data);
> +}
> +
> +
> +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;
You can't claim that the filter node was attached at this point. The
rollback code won't work properly.
> +
> + 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));
See, this would attempt to do stuff even when you didn't 'blockdev-add' it
first.
> +
> + virErrorRestore(&orig_err);
> +}
> +
> +
> +void
> +qemuBlockThrottleFilterChainDataFree(qemuBlockThrottleFilterChainData *data)
Using 'Chain' is not appropriate here as you seem to be adding filters
only on the top/disk level.
> +{
> + 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
> +qemuBlockThrottleFilterChainAttach(qemuMonitor *mon,
> + qemuBlockThrottleFilterChainData *data)
> +{
> + size_t i;
> +
> + for (i = 0; i < data->nfilterdata; i++) {
> + if (qemuMonitorBlockdevAdd(mon, &data->filterdata[i]->filterProps) < 0)
> + return -1;
> + data->filterdata[i]->filterAttached = true;
> + }
> +
> + return 0;
> +}
> +
> +
> +void
> +qemuBlockThrottleFilterChainDetach(qemuMonitor *mon,
> + qemuBlockThrottleFilterChainData *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_command.c b/src/qemu/qemu_command.c
> index 9d4563861f..2d8036c3ae 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1577,6 +1577,13 @@ qemuDiskConfigBlkdeviotuneEnabled(const virDomainDiskDef *disk)
> }
>
>
> +bool
> +qemuDiskConfigThrottleFilterChainEnabled(const virDomainDiskDef *disk)
> +{
> + return disk->nthrottlefilters > 0;
> +}
> +
> +
> /**
> * qemuDiskBusIsSD:
> * @bus: disk bus
> @@ -1936,6 +1943,11 @@ qemuBuildDiskDeviceProps(const virDomainDef *def,
> } else {
> if (qemuDomainDiskGetBackendAlias(disk, &drive) < 0)
> return NULL;
Here we have this handy function which is supposed to fill/return the
node name for the whole disk backend ...
> +
> + /* make sure device drive points to top throttle filter */
> + if (qemuDiskConfigThrottleFilterChainEnabled(disk)) {
> + drive = g_strdup(disk->throttlefilters[disk->nthrottlefilters-1]->nodename);
... which is also the filter. Any reason you didn't put this code where
it belongs (qemuDomainDiskGetTopNodename, as you need to look one
deeper for the proper func.)
Additionally this leaks the previous pointer that qemuDomainDiskGetBackendAlias
sets rith before this.
> + }
> }
>
> /* bootindex for floppies is configured via the fdc controller */
> @@ -11026,6 +11038,58 @@ qemuBuildStorageSourceChainAttachPrepareBlockdevOne(qemuBlockStorageSourceChainD
> }
>
>
> +/**
> + * qemuBuildThrottleFilterChainAttachPrepareBlockdevOne:
> + * @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
> + *
> + * Build filter node chain to provide more flexibility for block disk I/O limits
> + */
> +static int
> +qemuBuildThrottleFilterChainAttachPrepareBlockdevOne(qemuBlockThrottleFilterChainData *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;
> +}
> +
> +
> +/**
> + * qemuBuildThrottleFilterChainAttachPrepareBlockdev:
> + * @disk: domain disk
> + *
> + * Build filter node chain to provide more flexibility for block disk I/O limits
> + */
> +qemuBlockThrottleFilterChainData *
> +qemuBuildThrottleFilterChainAttachPrepareBlockdev(virDomainDiskDef *disk)
> +{
> + g_autoptr(qemuBlockThrottleFilterChainData) data = NULL;
> + size_t i;
> + const char * parentNodeName = NULL;
> + g_autofree char *tmp_nodename = NULL;
> +
> + data = g_new0(qemuBlockThrottleFilterChainData, 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 (qemuBuildThrottleFilterChainAttachPrepareBlockdevOne(data, disk->throttlefilters[i], tmp_nodename) < 0)
> + return NULL;
> + parentNodeName = disk->throttlefilters[i]->nodename;
> + }
> +
> + return g_steal_pointer(&data);
> +}
> +
> +
> /**
> * qemuBuildStorageSourceChainAttachPrepareBlockdev:
> * @top: storage source chain
> diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h
> index 341ec43f9a..e2dee47906 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -116,6 +116,12 @@ qemuBlockStorageSourceChainData *
> qemuBuildStorageSourceChainAttachPrepareBlockdevTop(virStorageSource *top,
> virStorageSource *backingStore);
>
> +qemuBlockThrottleFilterChainData *
> +qemuBuildThrottleFilterChainAttachPrepareBlockdev(virDomainDiskDef *disk);
> +
> +bool
> +qemuDiskConfigThrottleFilterChainEnabled(const virDomainDiskDef *disk);
> +
> virJSONValue *
> qemuBuildDiskDeviceProps(const virDomainDef *def,
> virDomainDiskDef *disk,
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index b676f59c3a..0d80f15f05 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -684,6 +684,19 @@ qemuDomainStorageIDNew(qemuDomainObjPrivate *priv)
> }
>
>
> +/**
> + * qemuDomainThrottleFilterIDNew:
> + * @priv: qemu VM private data object.
> + *
> + * Generate a new unique id for throttle filter object. Useful for node name generation.
> + */
> +static unsigned int
> +qemuDomainThrottleFilterIDNew(qemuDomainObjPrivate *priv)
> +{
> + return ++priv->filternodenameindex;
> +}
> +
> +
> /**
> * qemuDomainStorageIDReset:
> * @priv: qemu VM private data object.
> @@ -698,6 +711,20 @@ qemuDomainStorageIDReset(qemuDomainObjPrivate *priv)
> }
>
>
> +/**
> + * qemuDomainThrottleFilterIDReset:
> + * @priv: qemu VM private data object.
> + *
> + * Resets the data for the node name generator. The node names need to be unique
> + * for a single instance, so can be reset on VM shutdown.
> + */
> +static void
> +qemuDomainThrottleFilterIDReset(qemuDomainObjPrivate *priv)
> +{
> + priv->filternodenameindex = 0;
> +}
As I've noted below, all node name assignment must use
priv->nodenameindex, so that there aren't any duplicate or confusing
code paths. Please remove all of this and anything that's related to
filternodenameindex.
This is also lacking any form of remembering the nodenames used for the
filter layer into the status XML, which means that you won't be able to
unplug any of such disks after restart of the daemon.
[...]
> +int
> +qemuDomainPrepareThrottleFilterBlockdev(virDomainThrottleFilterDef *filter,
> + qemuDomainObjPrivate *priv)
> +{
> + g_autofree char *nodenameprefix = NULL;
> +
> + filter->id = qemuDomainThrottleFilterIDNew(priv);
> +
> + nodenameprefix = g_strdup_printf("libvirt-%u", filter->id);
> +
> + return qemuDomainPrepareThrottleFilterBlockdevNodename(filter, nodenameprefix);
> +}
> +
> +
> int
> qemuDomainPrepareStorageSourceBlockdev(virDomainDiskDef *disk,
> virStorageSource *src,
> @@ -11371,6 +11436,7 @@ qemuDomainPrepareDiskSourceBlockdev(virDomainDiskDef *disk,
> {
> qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> virStorageSource *n;
> + size_t i;
>
> if (disk->copy_on_read == VIR_TRISTATE_SWITCH_ON &&
> !diskPriv->nodeCopyOnRead)
> @@ -11381,6 +11447,11 @@ qemuDomainPrepareDiskSourceBlockdev(virDomainDiskDef *disk,
> return -1;
> }
>
> + for (i = 0; i < disk->nthrottlefilters; i++) {
> + if (qemuDomainPrepareThrottleFilterBlockdev(disk->throttlefilters[i], priv) < 0)
> + return -1;
> + }
> +
> return 0;
> }
>
> diff --git a/src/qemu/qemu_domain.h b/src/qemu/qemu_domain.h
> index 49e4da435b..a6158b39a9 100644
> --- a/src/qemu/qemu_domain.h
> +++ b/src/qemu/qemu_domain.h
> @@ -204,6 +204,9 @@ struct _qemuDomainObjPrivate {
> /* counter for generating node names for qemu disks */
> unsigned long long nodenameindex;
>
> + /* counter for generating node names for throttle filters */
> + unsigned long long filternodenameindex;
All node names for the VM ought to be generated from the above
nodenameindex. Do not add this one.
[...]
> @@ -880,10 +895,6 @@ int qemuDomainSetPrivatePaths(virQEMUDriver *driver,
>
> virDomainDiskDef *qemuDomainDiskByName(virDomainDef *def, const char *name);
>
> -virDomainThrottleGroupDef *
> -qemuDomainThrottleGroupByName(virDomainDef *def,
> - const char *name);
This looks like it belongs to another patch in this series.
> -
> char *qemuDomainGetMasterKeyFilePath(const char *libDir);
>
> int qemuDomainMasterKeyReadFile(qemuDomainObjPrivate *priv);
> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
> index 62dc879ed4..37d766b707 100644
> --- a/src/qemu/qemu_hotplug.c
> +++ b/src/qemu/qemu_hotplug.c
> @@ -656,6 +656,7 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
> virDomainAsyncJob asyncJob)
> {
> g_autoptr(qemuBlockStorageSourceChainData) data = NULL;
> + g_autoptr(qemuBlockThrottleFilterChainData) filterData = NULL;
> qemuDomainObjPrivate *priv = vm->privateData;
> g_autoptr(virJSONValue) devprops = NULL;
> bool extensionDeviceAttached = false;
> @@ -694,6 +695,19 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
> if (rc < 0)
> goto rollback;
>
> + /* Setup throttling filter chain
> + * add additional "blockdev-add"(throttle filter) between "blockdev-add" (qemuBlockStorageSourceChainAttach) and "device_add" (qemuDomainAttachExtensionDevice)
> + */
> + if ((filterData = qemuBuildThrottleFilterChainAttachPrepareBlockdev(disk))) {
> + if (qemuDomainObjEnterMonitorAsync(vm, asyncJob) < 0)
> + return -1;
> + /* QMP requests("blockdev-add" with "driver":"throttle") to QEMU */
> + rc = qemuBlockThrottleFilterChainAttach(priv->mon, filterData);
> + qemuDomainObjExitMonitor(vm);
> + if (rc < 0)
> + goto rollback;
> + }
> +
> if (disk->transient) {
> g_autoptr(qemuBlockStorageSourceAttachData) backend = NULL;
> g_autoptr(GHashTable) blockNamedNodeData = NULL;
The hotplug code also contains code to change media in cdroms. You'll
either have to forbid use of throttling on cdroms or modify the media
change code too.
> @@ -765,6 +779,8 @@ qemuDomainAttachDiskGeneric(virDomainObj *vm,
> if (extensionDeviceAttached)
> ignore_value(qemuDomainDetachExtensionDevice(priv->mon, &disk->info));
>
> + qemuBlockThrottleFilterChainDetach(priv->mon, filterData);
> +
> qemuBlockStorageSourceChainDetach(priv->mon, data);
>
> qemuDomainObjExitMonitor(vm);
> @@ -4504,6 +4520,7 @@ qemuDomainRemoveDiskDevice(virQEMUDriver *driver,
> {
> qemuDomainDiskPrivate *diskPriv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> g_autoptr(qemuBlockStorageSourceChainData) diskBackend = NULL;
> + g_autoptr(qemuBlockThrottleFilterChainData) filterData = NULL;
> size_t i;
> qemuDomainObjPrivate *priv = vm->privateData;
> int ret = -1;
> @@ -4542,6 +4559,13 @@ qemuDomainRemoveDiskDevice(virQEMUDriver *driver,
> }
> }
>
> + qemuDomainObjEnterMonitor(vm);
> + /* QMP request("blockdev-del") to QEMU to delete throttle filter*/
> + if ((filterData = qemuBuildThrottleFilterChainAttachPrepareBlockdev(disk))) {
You don't really need to format the full JSON object here. Now I
understand the hack with setting that this was added at 'AttachPrepare'
time though.
> + qemuBlockThrottleFilterChainDetach(priv->mon, filterData);
> + }
> + qemuDomainObjExitMonitor(vm);
> +
> qemuDomainObjEnterMonitor(vm);
>
> if (diskBackend)
_______________________________________________
Devel mailing list -- devel@xxxxxxxxxxxxxxxxx
To unsubscribe send an email to devel-leave@xxxxxxxxxxxxxxxxx
