On Mon, Nov 18, 2024 at 19:24:18 +0530, Harikumar R wrote:
> From: Chun Feng Wu <danielwuwy@xxxxxxx>
>
> It contains throttle filter nodename processing(new nodename,
> topnodename, parse and format nodename), throttle filter
> attaching/detaching preparation and implementation.
>
> * Updated "qemuDomainDiskGetTopNodename", so if throttlefilter is used
> together with copyOnRead, top node is throttle filter node, e.g.
> device -> throttle -> copyOnRead Layer-> image chain
> * In qemuBuildThrottleFiltersAttachPrepareBlockdev, if copy_on_read
> is on, build throttle nodename chain on top of copy_on_read nodename
> * In status xml, throttle filter nodename(virDomainDiskDef.nodename) is
> saved at disk/privateData/nodenames/nodename(type='throttle-filter'),
> corresponding parse/format sits in qemuDomainDiskPrivateParse and
> qemuDomainDiskPrivateFormat
> * If filter nodename hasn't been set by qemuDomainDiskPrivateParse,
> in qemuDomainPrepareThrottleFilterBlockdev, filter nodename index
> can be generated by reusing qemuDomainStorageIDNew and current
> global sequence number is persistented in virDomainObj-
> >privateData(qemuDomainObjPrivate)->nodenameindex.
> qemuDomainPrepareThrottleFilterBlockdev is called by
> qemuDomainPrepareDiskSourceBlockdev, which in turn used by both
> hotplug and qemuProcessStart to prepare throttle filter node name
> * Define method qemuBlockThrottleFilterGetProps, which is used by
> both hotplug and command to build throttle object for QEMU
> * Define methods for throttle filter attach/detach/rollback
>
> Signed-off-by: Chun Feng Wu <danielwuwy@xxxxxxx>
> ---
> src/qemu/qemu_block.c | 136 ++++++++++++++++++++++++++++++++++++++++
> src/qemu/qemu_block.h | 49 +++++++++++++++
> src/qemu/qemu_command.c | 81 ++++++++++++++++++++++++
> src/qemu/qemu_command.h | 6 ++
> src/qemu/qemu_domain.c | 73 +++++++++++++++++++--
> 5 files changed, 341 insertions(+), 4 deletions(-)
>
> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
> index 692b4d350e..5ff7319ceb 100644
> --- a/src/qemu/qemu_block.c
> +++ b/src/qemu/qemu_block.c
> @@ -2755,6 +2755,142 @@ 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.
> + */
> +static virJSONValue *
> +qemuBlockThrottleFilterGetProps(virDomainThrottleFilterDef *filter,
> + const char *parentNodeName)
> +{
> + g_autoptr(virJSONValue) props = NULL;
> + /* prefix group name with "throttle-" in QOM */
> + g_autofree char *prefixed_group_name = g_strdup_printf("throttle-%s", filter->group_name);
> + if (virJSONValueObjectAdd(&props,
> + "s:driver", "throttle",
> + "s:node-name", qemuBlockThrottleFilterGetNodename(filter),
> + "s:throttle-group", prefixed_group_name,
> + "s:file", parentNodeName,
> + NULL) < 0)
> + return NULL;
> +
> + 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;
The above call can be inlined, there's a bit too much indirection going
on here.
> +
> + data->filterNodeName = qemuBlockThrottleFilterGetNodename(filter);
> +
> + 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));
> +
> + 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);
> +}
> +
> +
> +/**
> + * qemuBlockThrottleFiltersAttach:
> + * @mon: monitor object
> + * @data: filter chain data
> + *
> + * Attach throttle filters.
> + * Caller must enter @mon prior calling this function.
> + */
> +int
> +qemuBlockThrottleFiltersAttach(qemuMonitor *mon,
> + qemuBlockThrottleFiltersData *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
> +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 f13a4a4a9a..b9e950e494 100644
> --- a/src/qemu/qemu_block.h
> +++ b/src/qemu/qemu_block.h
> @@ -215,6 +215,55 @@ qemuBlockStorageSourceCreateDetectSize(GHashTable *blockNamedNodeData,
> virStorageSource *src,
> virStorageSource *templ);
>
> +void
> +qemuBlockThrottleFilterSetNodename(virDomainThrottleFilterDef *filter,
> + char *nodename);
> +
> +const char *
> +qemuBlockThrottleFilterGetNodename(virDomainThrottleFilterDef *filter);
> +
> +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 696f891b47..0c119c8827 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -10996,6 +10996,87 @@ 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
> + *
> + * 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;
coding style:
const char *parent...
> + qemuDomainDiskPrivate *priv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> +
> + data = g_new0(qemuBlockThrottleFiltersData, 1);
> + /* if copy_on_read is enabled, put throttle chain on top of it */
> + if (disk->copy_on_read == VIR_TRISTATE_SWITCH_ON) {
> + parentNodeName = priv->nodeCopyOnRead;
> + } else {
> + /* get starting parentNodename, e.g. libvirt-1-format */
pointless comment
> + parentNodeName = qemuBlockStorageSourceGetEffectiveNodename(disk->src);
> + }
Add empty line.
> + /* build filterdata, which contains all filters info and sequence info through parentNodeName */
> + for (i = 0; i < disk->nthrottlefilters; i++) {
> + if (qemuBuildThrottleFiltersAttachPrepareBlockdevOne(data, disk->throttlefilters[i], parentNodeName) < 0)
> + return NULL;
> + 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]);
> + 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_command.h b/src/qemu/qemu_command.h
> index 76c514b5f7..1b0296ea42 100644
> --- a/src/qemu/qemu_command.h
> +++ b/src/qemu/qemu_command.h
> @@ -116,6 +116,12 @@ qemuBlockStorageSourceChainData *
> qemuBuildStorageSourceChainAttachPrepareBlockdevTop(virStorageSource *top,
> virStorageSource *backingStore);
>
> +qemuBlockThrottleFiltersData *
> +qemuBuildThrottleFiltersAttachPrepareBlockdev(virDomainDiskDef *disk);
> +
> +qemuBlockThrottleFiltersData *
> +qemuBuildThrottleFiltersDetachPrepareBlockdev(virDomainDiskDef *disk);
> +
> virJSONValue *
> qemuBuildDiskDeviceProps(const virDomainDef *def,
> virDomainDiskDef *disk,
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index f3b810a564..b6acf895c3 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -2235,6 +2235,34 @@ qemuStorageSourcePrivateDataFormat(virStorageSource *src,
> }
>
>
> +static int
> +virDomainDiskThrottleFilterNodeNamesParse(xmlXPathContextPtr ctxt,
> + virDomainDiskDef *def)
> +{
> + size_t i;
> + int n = 0;
> + g_autofree xmlNodePtr *nodes = NULL;
> + g_autoptr(GHashTable) throttleFiltersMap = virHashNew(g_free);
> +
> + if ((n = virXPathNodeSet("./nodenames/nodename[@type='throttle-filter']", ctxt, &nodes)) < 0)
> + return -1;
> +
> + for (i = 0; i < n; i++) {
> + g_hash_table_insert(throttleFiltersMap, virXMLPropString(nodes[i], "group"), virXMLPropString(nodes[i], "name"));
> + }
> +
> + for (i = 0; i < def->nthrottlefilters; i++) {
> + char* nodename = g_hash_table_lookup(throttleFiltersMap, def->throttlefilters[i]->group_name);
Coding style dictates: 'char *nodename';
> + if (nodename) {
> + /* first time to set nodename in filter */
pointless comment
> + def->throttlefilters[i]->nodename = g_strdup(nodename);
use the accessor defined before.
> + }
> + }
> +
> + return 0;
> +}
> +
> +
> static int
> qemuDomainDiskPrivateParse(xmlXPathContextPtr ctxt,
> virDomainDiskDef *disk)
> @@ -2244,6 +2272,9 @@ qemuDomainDiskPrivateParse(xmlXPathContextPtr ctxt,
> priv->qomName = virXPathString("string(./qom/@name)", ctxt);
> priv->nodeCopyOnRead = virXPathString("string(./nodenames/nodename[@type='copyOnRead']/@name)", ctxt);
>
> + if (virDomainDiskThrottleFilterNodeNamesParse(ctxt, disk) < 0)
> + return -1;
> +
> return 0;
> }
>
> @@ -2253,14 +2284,22 @@ qemuDomainDiskPrivateFormat(virDomainDiskDef *disk,
> virBuffer *buf)
> {
> qemuDomainDiskPrivate *priv = QEMU_DOMAIN_DISK_PRIVATE(disk);
> + size_t i;
>
> virBufferEscapeString(buf, "<qom name='%s'/>\n", priv->qomName);
>
> - if (priv->nodeCopyOnRead) {
> + if (priv->nodeCopyOnRead || disk->nthrottlefilters > 0) {
> virBufferAddLit(buf, "<nodenames>\n");
> virBufferAdjustIndent(buf, 2);
> - virBufferEscapeString(buf, "<nodename type='copyOnRead' name='%s'/>\n",
> - priv->nodeCopyOnRead);
> + if (priv->nodeCopyOnRead)
> + virBufferEscapeString(buf, "<nodename type='copyOnRead' name='%s'/>\n",
> + priv->nodeCopyOnRead);
> + if (disk->nthrottlefilters > 0) {
> + for (i = 0; i < disk->nthrottlefilters; i++) {
> + virBufferEscapeString(buf, "<nodename type='throttle-filter' name='%s' ", disk->throttlefilters[i]->nodename);
> + virBufferEscapeString(buf, "group='%s'/>\n", disk->throttlefilters[i]->group_name);
virBufferEscapeString skips the formatting of the complete format string
in case when the 3rd argument is NULL. This has the potential of
breaking the XML if the nodename or group_name are missing. Please check
them, as it's better to have one broken feature than lose the whole VM
when restartingf the daemon/.
> + }
> + }
> virBufferAdjustIndent(buf, -2);
> virBufferAddLit(buf, "</nodenames>\n");
> }
> @@ -6348,7 +6387,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).
> @@ -6361,6 +6401,10 @@ qemuDomainDiskGetTopNodename(virDomainDiskDef *disk)
> if (virStorageSourceIsEmpty(disk->src))
> return NULL;
>
> + /* If disk has throttles, take top throttle node name */
> + if (disk->nthrottlefilters > 0)
> + return disk->throttlefilters[disk->nthrottlefilters - 1]->nodename;
> +
> if (disk->copy_on_read == VIR_TRISTATE_SWITCH_ON)
> return priv->nodeCopyOnRead;
>
> @@ -9724,6 +9768,22 @@ qemuDomainPrepareStorageSourceBlockdevNodename(virDomainDiskDef *disk,
> }
>
>
> +static void
> +qemuDomainPrepareThrottleFilterBlockdev(virDomainThrottleFilterDef *filter,
> + qemuDomainObjPrivate *priv)
> +{
> + g_autofree char *nodenameprefix = NULL;
> +
> + /* skip setting throttle filter nodename if it's set by parsing statusxml */
> + if (filter->nodename) {
> + return;
> + }
> + nodenameprefix = g_strdup_printf("libvirt-%u", qemuDomainStorageIDNew(priv));
> + /* generate and set nodename into filter for later QEMU cmd preparation */
pointless comment
> + qemuBlockThrottleFilterSetNodename(filter, g_strdup_printf("%s-filter", nodenameprefix));
> +}
> +
> +
> int
> qemuDomainPrepareStorageSourceBlockdev(virDomainDiskDef *disk,
> virStorageSource *src,
> @@ -9747,6 +9807,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)
> @@ -9757,6 +9818,10 @@ qemuDomainPrepareDiskSourceBlockdev(virDomainDiskDef *disk,
> return -1;
> }
>
> + for (i = 0; i < disk->nthrottlefilters; i++) {
> + qemuDomainPrepareThrottleFilterBlockdev(disk->throttlefilters[i], priv);
> + }
> +
> return 0;
> }
The rest looks reasonable so if you do all the changes:
Reviewed-by: Peter Krempa <pkrempa@xxxxxxxxxx>
