[libvirt] [PATCHv6 0/7] block io throttle via per-device block IO tuning

Here's the latest state of Lei's patch series with all my comments folded in. I may have a few more tweaks to make now that I'm at the point of testing things with both old qemu (graceful rejection) and new qemu (sensical return values), so it may be a day or two (or even a weekend, since this is a holiday weekend for me) before I actually push this, so it wouldn't hurt if anyone else wants to review in the meantime. Lei Li (7): Add new API virDomain{Set, Get}BlockIoTune Add virDomain{Set, Get}BlockIoTune support to the remote driver Support block I/O throttle in XML Implement virDomain{Set, Get}BlockIoTune for the qemu driver Enable the blkdeviotune command in virsh Support virDomain{Set, Get}BlockIoTune in the python API Add tests for blkdeviotune daemon/remote.c | 64 ++++ docs/formatdomain.html.in | 39 +++ docs/schemas/domaincommon.rng | 122 +++++-- include/libvirt/libvirt.h.in | 63 ++++ python/generator.py | 2 + python/libvirt-override-api.xml | 16 + python/libvirt-override.c | 178 ++++++++++ src/conf/domain_conf.c | 90 +++++- src/conf/domain_conf.h | 14 + src/driver.h | 20 ++ src/libvirt.c | 148 +++++++++ src/libvirt_public.syms | 6 + src/qemu/qemu_command.c | 31 ++ src/qemu/qemu_driver.c | 340 ++++++++++++++++++++ src/qemu/qemu_monitor.c | 33 ++ src/qemu/qemu_monitor.h | 8 + src/qemu/qemu_monitor_json.c | 176 ++++++++++ src/qemu/qemu_monitor_json.h | 8 + src/qemu/qemu_monitor_text.c | 151 +++++++++- src/qemu/qemu_monitor_text.h | 8 + src/remote/remote_driver.c | 57 ++++ src/remote/remote_protocol.x | 27 ++- src/remote_protocol-structs | 24 ++ .../qemuxml2argv-blkdeviotune.args | 7 + .../qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml | 30 ++ tests/qemuxml2argvtest.c | 2 + tests/qemuxml2xmltest.c | 1 + tools/virsh.c | 244 ++++++++++++++ tools/virsh.pod | 30 ++ 29 files changed, 1900 insertions(+), 39 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml -- 1.7.7.3

From: Lei Li <lilei@linux.vnet.ibm.com> This patch add new pulic API virDomainSetBlockIoTune and virDomainGetBlockIoTune. Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt.h.in | 63 ++++++++++++++++++ python/generator.py | 3 + src/driver.h | 20 ++++++ src/libvirt.c | 149 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 6 ++ 5 files changed, 241 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 66c2a0f..3204e7c 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1671,6 +1671,69 @@ int virDomainBlockPull(virDomainPtr dom, const char *disk, unsigned long bandwidth, unsigned int flags); +/* Block I/O throttling support */ + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC: + * + * Macro for the BlockIoTune tunable weight: it represents the total + * bytes per second permitted through a block device, as a ullong. + */ +#define VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC "total_bytes_sec" + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC: + * + * Macro for the BlockIoTune tunable weight: it repersents the read + * bytes per second permitted through a block device, as a ullong. + */ +#define VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC "read_bytes_sec" + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC: + * + * Macro for the BlockIoTune tunable weight: it repersents the write + * bytes per second permitted through a block device, as a ullong. + */ +#define VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC "write_bytes_sec" + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC: + * + * Macro for the BlockIoTune tunable weight: it repersents the total + * I/O operations per second permitted through a block device, as a ullong. + */ +#define VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC "total_iops_sec" + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC: + * + * Macro for the BlockIoTune tunable weight: it repersents the read + * I/O operations per second permitted through a block device, as a ullong. + */ +#define VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC "read_iops_sec" + +/** + * VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC: + * Macro for the BlockIoTune tunable weight: it repersents the write + * I/O operations per second permitted through a block device, as a ullong. + */ +#define VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC "write_iops_sec" + +int +virDomainSetBlockIoTune(virDomainPtr dom, + const char *disk, + virTypedParameterPtr params, + int nparams, + unsigned int flags); +int +virDomainGetBlockIoTune(virDomainPtr dom, + const char *disk, + virTypedParameterPtr params, + int *nparams, + unsigned int flags); + + /* * NUMA support */ diff --git a/python/generator.py b/python/generator.py index 71afdb7..00bd94f 100755 --- a/python/generator.py +++ b/python/generator.py @@ -470,6 +470,9 @@ skip_function = ( "virNWFilterGetConnect", "virStoragePoolGetConnect", "virStorageVolGetConnect", + + "virDomainGetBlockIoTune", # not implemented yet + "virDomainSetBlockIoTune", # not implemented yet ) qemu_skip_function = ( diff --git a/src/driver.h b/src/driver.h index 4c14aaa..23e96d9 100644 --- a/src/driver.h +++ b/src/driver.h @@ -740,6 +740,24 @@ typedef int (*virDrvDomainBlockPull)(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags); +/* + * Block I/O throttling support + */ + +typedef int + (*virDrvDomainSetBlockIoTune)(virDomainPtr dom, + const char *disk, + virTypedParameterPtr params, + int nparams, + unsigned int flags); + +typedef int + (*virDrvDomainGetBlockIoTune)(virDomainPtr dom, + const char *disk, + virTypedParameterPtr params, + int *nparams, + unsigned int flags); + /** * _virDriver: @@ -899,6 +917,8 @@ struct _virDriver { virDrvDomainGetBlockJobInfo domainGetBlockJobInfo; virDrvDomainBlockJobSetSpeed domainBlockJobSetSpeed; virDrvDomainBlockPull domainBlockPull; + virDrvDomainSetBlockIoTune domainSetBlockIoTune; + virDrvDomainGetBlockIoTune domainGetBlockIoTune; }; typedef int diff --git a/src/libvirt.c b/src/libvirt.c index 87107e5..a64bc07 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17188,3 +17188,152 @@ error: virDispatchError(dom->conn); return -1; } + +/** + * virDomainSetBlockIoTune: + * @dom: pointer to domain object + * @disk: path to the block device, or device shorthand + * @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: An OR'ed set of virDomainModificationImpact + * + * Change all or a subset of the per-device block IO tunables. + * + * The @disk parameter is either an unambiguous source name of the + * block device (the <source file='...'/> sub-element, such as + * "/path/to/image"), or the device target shorthand (the <target + * dev='...'/> sub-element, such as "xvda"). Valid names can be found + * by calling virDomainGetXMLDesc() and inspecting elements + * within //domain/devices/disk. + * + * Returns -1 in case of error, 0 in case of success. + */ +int virDomainSetBlockIoTune(virDomainPtr dom, + const char *disk, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(dom, "disk=%p, params=%p, nparams=%d, flags=%x", + disk, params, nparams, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN(dom)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + if (dom->conn->flags & VIR_CONNECT_RO) { + virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (!disk || (nparams <= 0) || (params == NULL)) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + + if (virTypedParameterValidateSet(dom, params, nparams) < 0) + return -1; + + conn = dom->conn; + + if (conn->driver->domainSetBlockIoTune) { + int ret; + ret = conn->driver->domainSetBlockIoTune(dom, disk, params, nparams, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibDomainError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(dom->conn); + return -1; +} + +/** + * virDomainGetBlockIoTune: + * @dom: pointer to domain object + * @disk: path to the block device, or device shorthand + * @params: Pointer to blkio parameter object + * (return value, allocated by the caller) + * @nparams: Pointer to number of blkio parameters + * @flags: An OR'ed set of virDomainModificationImpact + * + * Get all block IO tunable parameters for a given device. On input, + * @nparams gives the size of the @params array; on output, @nparams + * gives how many slots were filled with parameter information, which + * might be less but will not exceed the input value. + * + * As a special case, calling with @params as NULL and @nparams as 0 + * on input will cause @nparams on output to contain the number of + * parameters supported by the hypervisor, either for the given @disk + * (note that block devices of different types might support different + * parameters), or if @disk is NULL, for all possible disks. The + * caller should then allocate @params array, + * i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API + * again. See virDomainGetMemoryParameters() for more details. + * + * The @disk parameter is either an unambiguous source name of the + * block device (the <source file='...'/> sub-element, such as + * "/path/to/image"), or the device target shorthand (the <target + * dev='...'/> sub-element, such as "xvda"). Valid names can be found + * by calling virDomainGetXMLDesc() and inspecting elements + * within //domain/devices/disk. This parameter cannot be NULL + * unless @nparams is 0 on input. + * + * Returns -1 in case of error, 0 in case of success. + */ +int virDomainGetBlockIoTune(virDomainPtr dom, + const char *disk, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(dom, "disk=%p, params=%p, nparams=%d, flags=%x", + NULLSTR(disk), params, (nparams) ? *nparams : -1, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN (dom)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + + if (nparams == NULL || *nparams < 0 || + ((params == NULL || disk == NULL) && *nparams != 0)) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + + if (VIR_DRV_SUPPORTS_FEATURE(dom->conn->driver, dom->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + flags |= VIR_TYPED_PARAM_STRING_OKAY; + + conn = dom->conn; + + if (conn->driver->domainGetBlockIoTune) { + int ret; + ret = conn->driver->domainGetBlockIoTune(dom, disk, params, nparams, flags); + if (ret < 0) + goto error; + return ret; + } + + virLibDomainError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(dom->conn); + return -1; +} + diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index bcefb10..e62387c 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -498,4 +498,10 @@ LIBVIRT_0.9.7 { virDomainSnapshotNumChildren; } LIBVIRT_0.9.5; +LIBVIRT_0.9.8 { + global: + virDomainGetBlockIoTune; + virDomainSetBlockIoTune; +} LIBVIRT_0.9.7; + # .... define new API here using predicted next version number .... -- 1.7.7.3

On 11/23/2011 02:44 PM, Eric Blake wrote:
From: Lei Li <lilei@linux.vnet.ibm.com>
This patch add new pulic API virDomainSetBlockIoTune and virDomainGetBlockIoTune.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- include/libvirt/libvirt.h.in | 63 ++++++++++++++++++ python/generator.py | 3 + src/driver.h | 20 ++++++ src/libvirt.c | 149 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 6 ++ 5 files changed, 241 insertions(+), 0 deletions(-)
I finally got around to testing this series. It looks like the errors with older qemu are decent enough without needing major tweaks in the code: # tools/virsh blkdeviotune dom vda --total_bytes_sec 10000000 error: Unable to change block I/O throttle error: Requested operation is not valid: Command 'block_set_io_throttle' is not found # tools/virsh blkdeviotune dom vda error: Unable to get block I/O throttle parameters error: invalid argument: No info for device 'drive-virtio-disk0' We may find further things to tweak. Off-hand, I know of my desire to fix things so that bytes_sec and iops_sec are independent, where setting one does not clear the other; also, I did not get around to testing hot-plug behavior with qemu that supports the new monitor command; my guess is that we probably forgot to propagate a hotplug with iotuning into qemu properly. But what we have in the series so far is good enough to start pushing, so that we beat the rc1 freeze; touchups can stretch out past the freeze as needed. After rebasing this to latest, I'm squashing this in before pushing, to pick up on the ideas of commit 4199f3de that went in during the meantime: diff --git i/src/libvirt.c w/src/libvirt.c index 8a5361e..5c09591 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -17574,6 +17574,12 @@ int virDomainGetBlockIoTune(virDomainPtr dom, VIR_DRV_FEATURE_TYPED_PARAM_STRING)) flags |= VIR_TYPED_PARAM_STRING_OKAY; + /* At most one of these two flags should be set. */ + if ((flags & VIR_DOMAIN_AFFECT_LIVE) && + (flags & VIR_DOMAIN_AFFECT_CONFIG)) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } conn = dom->conn; if (conn->driver->domainGetBlockIoTune) { -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: Lei Li <lilei@linux.vnet.ibm.com> Support Block I/O Throttle setting and query to remote driver. Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- daemon/remote.c | 64 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt.c | 1 - src/remote/remote_driver.c | 57 +++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 27 +++++++++++++++++- src/remote_protocol-structs | 24 +++++++++++++++ 5 files changed, 171 insertions(+), 2 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index 97c9538..8b2da0d 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1885,6 +1885,70 @@ cleanup: return rv; } +static int +remoteDispatchDomainGetBlockIoTune(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr hdr ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_get_block_io_tune_args *args, + remote_domain_get_block_io_tune_ret *ret) +{ + virDomainPtr dom = NULL; + int rv = -1; + virTypedParameterPtr params; + int nparams = args->nparams; + struct daemonClientPrivate *priv = + virNetServerClientGetPrivateData(client); + + if (!priv->conn) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("connection not open")); + goto cleanup; + } + + if (nparams > REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; + } + + if (VIR_ALLOC_N(params, nparams) < 0) { + virReportOOMError(); + goto cleanup; + } + + if (!(dom = get_nonnull_domain(priv->conn, args->dom))) + goto cleanup; + + if (virDomainGetBlockIoTune(dom, args->disk ? *args->disk : NULL, + params, &nparams, args->flags) < 0) + goto cleanup; + + /* In this case, we need to send back the number of parameters + * supported + */ + if (args->nparams == 0) { + ret->nparams = nparams; + goto success; + } + + /* Serialise the block I/O tuning parameters. */ + if (remoteSerializeTypedParameters(params, nparams, + &ret->params.params_val, + &ret->params.params_len, + args->flags) < 0) + goto cleanup; + +success: + rv = 0; + +cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + virTypedParameterArrayClear(params, nparams); + VIR_FREE(params); + if (dom) + virDomainFree(dom); + return rv; +} /*-------------------------------------------------------------*/ diff --git a/src/libvirt.c b/src/libvirt.c index a64bc07..074bc22 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17336,4 +17336,3 @@ error: virDispatchError(dom->conn); return -1; } - diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 94fd3e7..c37954e 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2178,6 +2178,61 @@ done: return rv; } +static int remoteDomainGetBlockIoTune(virDomainPtr domain, + const char *disk, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) +{ + int rv = -1; + remote_domain_get_block_io_tune_args args; + remote_domain_get_block_io_tune_ret ret; + struct private_data *priv = domain->conn->privateData; + + remoteDriverLock(priv); + + make_nonnull_domain(&args.dom, domain); + args.disk = disk ? (char **)&disk : NULL; + args.nparams = *nparams; + args.flags = flags; + + memset(&ret, 0, sizeof(ret)); + + + if (call(domain->conn, priv, 0, REMOTE_PROC_DOMAIN_GET_BLOCK_IO_TUNE, + (xdrproc_t) xdr_remote_domain_get_block_io_tune_args, + (char *) &args, + (xdrproc_t) xdr_remote_domain_get_block_io_tune_ret, + (char *) &ret) == -1) { + goto done; + } + + /* Handle the case when the caller does not know the number of parameters + * and is asking for the number of parameters supported + */ + if (*nparams == 0) { + *nparams = ret.nparams; + rv = 0; + goto cleanup; + } + + if (remoteDeserializeTypedParameters(ret.params.params_val, + ret.params.params_len, + REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX, + params, + nparams) < 0) + goto cleanup; + + rv = 0; + +cleanup: + xdr_free ((xdrproc_t) xdr_remote_domain_get_block_io_tune_ret, + (char *) &ret); +done: + remoteDriverUnlock(priv); + return rv; +} + /*----------------------------------------------------------------------*/ static virDrvOpenStatus ATTRIBUTE_NONNULL (1) @@ -4550,6 +4605,8 @@ static virDriver remote_driver = { .domainGetBlockJobInfo = remoteDomainGetBlockJobInfo, /* 0.9.4 */ .domainBlockJobSetSpeed = remoteDomainBlockJobSetSpeed, /* 0.9.4 */ .domainBlockPull = remoteDomainBlockPull, /* 0.9.4 */ + .domainSetBlockIoTune = remoteDomainSetBlockIoTune, /* 0.9.8 */ + .domainGetBlockIoTune = remoteDomainGetBlockIoTune, /* 0.9.8 */ }; static virNetworkDriver network_driver = { diff --git a/src/remote/remote_protocol.x b/src/remote/remote_protocol.x index 5ea1114..106a982 100644 --- a/src/remote/remote_protocol.x +++ b/src/remote/remote_protocol.x @@ -125,6 +125,9 @@ const REMOTE_DOMAIN_BLKIO_PARAMETERS_MAX = 16; /* Upper limit on list of memory parameters. */ const REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX = 16; +/* Upper limit on list of blockio tuning parameters. */ +const REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX = 16; + /* Upper limit on list of node cpu stats. */ const REMOTE_NODE_CPU_STATS_MAX = 16; @@ -1075,6 +1078,25 @@ struct remote_domain_block_pull_args { unsigned int flags; }; +struct remote_domain_set_block_io_tune_args { + remote_nonnull_domain dom; + remote_nonnull_string disk; + remote_typed_param params<REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX>; + unsigned int flags; +}; + +struct remote_domain_get_block_io_tune_args { + remote_nonnull_domain dom; + remote_string disk; + int nparams; + unsigned int flags; +}; + +struct remote_domain_get_block_io_tune_ret { + remote_typed_param params<REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX>; + int nparams; +}; + /* Network calls: */ struct remote_num_of_networks_ret { @@ -2564,7 +2586,10 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SNAPSHOT_NUM_CHILDREN = 246, /* autogen autogen priority:high */ REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_CHILDREN_NAMES = 247, /* autogen autogen priority:high */ REMOTE_PROC_DOMAIN_EVENT_DISK_CHANGE = 248, /* skipgen skipgen */ - REMOTE_PROC_DOMAIN_OPEN_GRAPHICS = 249 /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_OPEN_GRAPHICS = 249, /* skipgen skipgen */ + REMOTE_PROC_DOMAIN_SET_BLOCK_IO_TUNE = 250, /* autogen autogen */ + + REMOTE_PROC_DOMAIN_GET_BLOCK_IO_TUNE = 251 /* skipgen skipgen */ /* * Notice how the entries are grouped in sets of 10 ? diff --git a/src/remote_protocol-structs b/src/remote_protocol-structs index b460b77..853eac4 100644 --- a/src/remote_protocol-structs +++ b/src/remote_protocol-structs @@ -759,6 +759,28 @@ struct remote_domain_block_pull_args { uint64_t bandwidth; u_int flags; }; +struct remote_domain_set_block_io_tune_args { + remote_nonnull_domain dom; + remote_nonnull_string disk; + struct { + u_int params_len; + remote_typed_param * params_val; + } params; + u_int flags; +}; +struct remote_domain_get_block_io_tune_args { + remote_nonnull_domain dom; + remote_string disk; + int nparams; + u_int flags; +}; +struct remote_domain_get_block_io_tune_ret { + struct { + u_int params_len; + remote_typed_param * params_val; + } params; + int nparams; +}; struct remote_num_of_networks_ret { int num; }; @@ -2007,4 +2029,6 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_SNAPSHOT_LIST_CHILDREN_NAMES = 247, REMOTE_PROC_DOMAIN_EVENT_DISK_CHANGE = 248, REMOTE_PROC_DOMAIN_OPEN_GRAPHICS = 249, + REMOTE_PROC_DOMAIN_SET_BLOCK_IO_TUNE = 250, + REMOTE_PROC_DOMAIN_GET_BLOCK_IO_TUNE = 251, }; -- 1.7.7.3

On Wed, Nov 23, 2011 at 02:44:44PM -0700, Eric Blake wrote:
From: Lei Li <lilei@linux.vnet.ibm.com>
Support Block I/O Throttle setting and query to remote driver. [...] diff --git a/src/libvirt.c b/src/libvirt.c index a64bc07..074bc22 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17336,4 +17336,3 @@ error: virDispatchError(dom->conn); return -1; } -
Can we avoid this ? possly folding it in previous patch if that comes from it ?
diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c
ACK with this fixed. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On 11/24/2011 12:40 AM, Daniel Veillard wrote:
On Wed, Nov 23, 2011 at 02:44:44PM -0700, Eric Blake wrote:
From: Lei Li <lilei@linux.vnet.ibm.com>
Support Block I/O Throttle setting and query to remote driver. [...] diff --git a/src/libvirt.c b/src/libvirt.c index a64bc07..074bc22 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -17336,4 +17336,3 @@ error: virDispatchError(dom->conn); return -1; } -
Can we avoid this ? possly folding it in previous patch if that comes from it ?
Yep, previous patch failed 'make syntax-check' because of an empty blank line at the end, so it looks like I just squashed my fix into the wrong patch. And in the process of fixing this hunk, I had to deal with merge conflicts with Jirka's keepalive stuff, where I noticed he used 0.9.7 instead of 0.9.8 for all the new callbacks he added in the same places that this series touches. We'll get that fixed shortly. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 11/23/2011 02:44 PM, Eric Blake wrote:
From: Lei Li <lilei@linux.vnet.ibm.com>
Support Block I/O Throttle setting and query to remote driver.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- daemon/remote.c | 64 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt.c | 1 - src/remote/remote_driver.c | 57 +++++++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 27 +++++++++++++++++- src/remote_protocol-structs | 24 +++++++++++++++ 5 files changed, 171 insertions(+), 2 deletions(-)
diff --git a/daemon/remote.c b/daemon/remote.c index 97c9538..8b2da0d 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -1885,6 +1885,70 @@ cleanup: return rv; }
+static int +remoteDispatchDomainGetBlockIoTune(virNetServerPtr server ATTRIBUTE_UNUSED, + virNetServerClientPtr client ATTRIBUTE_UNUSED, + virNetMessagePtr hdr ATTRIBUTE_UNUSED, + virNetMessageErrorPtr rerr, + remote_domain_get_block_io_tune_args *args, + remote_domain_get_block_io_tune_ret *ret) +{ + virDomainPtr dom = NULL; + int rv = -1; + virTypedParameterPtr params;
Uninit...
+ if (nparams > REMOTE_DOMAIN_BLOCK_IO_TUNE_PARAMETERS_MAX) { + virNetError(VIR_ERR_INTERNAL_ERROR, "%s", _("nparams too large")); + goto cleanup; + } + + if (VIR_ALLOC_N(params, nparams) < 0) {
...and can get to cleanup before allocation...
+cleanup: + if (rv < 0) + virNetMessageSaveError(rerr); + virTypedParameterArrayClear(params, nparams); + VIR_FREE(params);
...which means death inside free(). Oops. I'm squashing this in: diff --git i/daemon/remote.c w/daemon/remote.c index 12ac6c2..e1d208c 100644 --- i/daemon/remote.c +++ w/daemon/remote.c @@ -1901,7 +1901,7 @@ remoteDispatchDomainGetBlockIoTune(virNetServerPtr server ATTRIBUTE_UNUSED, { virDomainPtr dom = NULL; int rv = -1; - virTypedParameterPtr params; + virTypedParameterPtr params = NULL; int nparams = args->nparams; struct daemonClientPrivate *priv = virNetServerClientGetPrivateData(client); -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: Lei Li <lilei@linux.vnet.ibm.com> Enable block I/O throttle for per-disk in XML, as the first per-disk IO tuning parameter. Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- docs/formatdomain.html.in | 39 +++++++++++++ docs/schemas/domaincommon.rng | 122 +++++++++++++++++++++++++++++------------ src/conf/domain_conf.c | 90 ++++++++++++++++++++++++++++++- src/conf/domain_conf.h | 14 +++++ 4 files changed, 228 insertions(+), 37 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 61123ac..f45d0ce 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -922,6 +922,11 @@ <driver name="tap" type="aio" cache="default"/> <source file='/var/lib/xen/images/fv0'/ startupPolicy='optional'> <target dev='hda' bus='ide'/> + <iotune> + <total_bytes_sec>10000000</total_bytes_sec> + <read_iops_sec>400000</read_iops_sec> + <write_iops_sec>100000</write_iops_sec> + </iotune> <boot order='2'/> <encryption type='...'> ... @@ -1039,6 +1044,40 @@ <span class="since">Since 0.0.3; <code>bus</code> attribute since 0.4.3; "usb" attribute value since after 0.4.4; "sata" attribute value since 0.9.7</span></dd> + <dt><code>iotune</code></dt> + <dd>The optional <code>iotune</code> element provides the + ability to provide additional per-device I/O tuning, with + values that can vary for each device (contrast this to + the <a href="#elementsBlockTuning"><code><blkiotune></code></a> + element, which applies globally to the domain). Currently, + the only tuning available is Block I/O throttling for qemu. + This element has optional sub-elements; any sub-element not + specified or given with a value of 0 implies no + limit. <span class="since">Since 0.9.8</span> + <dl> + <dt><code>total_bytes_sec</code></dt> + <dd>The optional <code>total_bytes_sec</code> element is the + total throughput limit in bytes per second. This cannot + appear with <code>read_bytes_sec</code> + or <code>write_bytes_sec</code>.</dd> + <dt><code>read_bytes_sec</code></dt> + <dd>The optional <code>read_bytes_sec</code> element is the + read throughput limit in bytes per second.</dd> + <dt><code>write_bytes_sec</code></dt> + <dd>The optional <code>write_bytes_sec</code> element is the + write throughput limit in bytes per second.</dd> + <dt><code>total_iops_sec</code></dt> + <dd>The optional <code>total_iops_sec</code> element is the + total I/O operations per second. This cannot + appear with <code>read_iops_sec</code> + or <code>write_iops_sec</code>.</dd> + <dt><code>read_iops_sec</code></dt> + <dd>The optional <code>read_iops_sec</code> element is the + read I/O operations per second.</dd> + <dt><code>write_iops_sec</code></dt> + <dd>The optional <code>write_iops_sec</code> element is the + write I/O operations per second.</dd> + </dl> <dt><code>driver</code></dt> <dd> The optional driver element allows specifying further details diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 8968ee6..bb6d94d 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -599,42 +599,47 @@ </element> </define> <define name="diskspec"> - <optional> - <ref name="driver"/> - </optional> - <optional> - <ref name="diskAuth"/> - </optional> - <ref name="target"/> - <optional> - <ref name="deviceBoot"/> - </optional> - <optional> - <element name="readonly"> - <empty/> - </element> - </optional> - <optional> - <element name="shareable"> - <empty/> - </element> - </optional> - <optional> - <element name="transient"> - <empty/> - </element> - </optional> - <optional> - <element name="serial"> - <ref name="diskSerial"/> - </element> - </optional> - <optional> - <ref name="encryption"/> - </optional> - <optional> - <ref name="address"/> - </optional> + <interleave> + <optional> + <ref name="driver"/> + </optional> + <optional> + <ref name="diskAuth"/> + </optional> + <ref name="target"/> + <optional> + <ref name="deviceBoot"/> + </optional> + <optional> + <element name="readonly"> + <empty/> + </element> + </optional> + <optional> + <element name="shareable"> + <empty/> + </element> + </optional> + <optional> + <element name="transient"> + <empty/> + </element> + </optional> + <optional> + <element name="serial"> + <ref name="diskSerial"/> + </element> + </optional> + <optional> + <ref name="encryption"/> + </optional> + <optional> + <ref name="diskIoTune"/> + </optional> + <optional> + <ref name="address"/> + </optional> + </interleave> </define> <define name="snapshot"> <attribute name="snapshot"> @@ -2596,6 +2601,51 @@ </element> </define> + <define name='diskIoTune'> + <element name="iotune"> + <interleave> + <choice> + <element name="total_bytes_sec"> + <data type="unsignedLong"/> + </element> + <group> + <interleave> + <optional> + <element name="read_bytes_sec"> + <data type="unsignedLong"/> + </element> + </optional> + <optional> + <element name="write_bytes_sec"> + <data type="unsignedLong"/> + </element> + </optional> + </interleave> + </group> + </choice> + <choice> + <element name="total_iops_sec"> + <data type="unsignedLong"/> + </element> + <group> + <interleave> + <optional> + <element name="read_iops_sec"> + <data type="unsignedLong"/> + </element> + </optional> + <optional> + <element name="write_iops_sec"> + <data type="unsignedLong"/> + </element> + </optional> + </interleave> + </group> + </choice> + </interleave> + </element> + </define> + <!-- Optional hypervisor extensions in their own namespace: QEmu diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index d365cee..a2702c5 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2394,6 +2394,7 @@ cleanup: static virDomainDiskDefPtr virDomainDiskDefParseXML(virCapsPtr caps, xmlNodePtr node, + xmlXPathContextPtr ctxt, virBitmapPtr bootMap, unsigned int flags) { @@ -2594,6 +2595,50 @@ virDomainDiskDefParseXML(virCapsPtr caps, } child = child->next; } + } else if (xmlStrEqual(cur->name, BAD_CAST "iotune")) { + if (virXPathULongLong("string(./devices/disk/iotune/total_bytes_sec)", + ctxt, &def->blkdeviotune.total_bytes_sec) < 0) { + def->blkdeviotune.total_bytes_sec = 0; + } + + if (virXPathULongLong("string(./devices/disk/iotune/read_bytes_sec)", + ctxt, &def->blkdeviotune.read_bytes_sec) < 0) { + def->blkdeviotune.read_bytes_sec = 0; + } + + if (virXPathULongLong("string(./devices/disk/iotune/write_bytes_sec)", + ctxt, &def->blkdeviotune.write_bytes_sec) < 0) { + def->blkdeviotune.write_bytes_sec = 0; + } + + if (virXPathULongLong("string(./devices/disk/iotune/total_iops_sec)", + ctxt, &def->blkdeviotune.total_iops_sec) < 0) { + def->blkdeviotune.total_iops_sec = 0; + } + + if (virXPathULongLong("string(./devices/disk/iotune/read_iops_sec)", + ctxt, &def->blkdeviotune.read_iops_sec) < 0) { + def->blkdeviotune.read_iops_sec = 0; + } + + if (virXPathULongLong("string(./devices/disk/iotune/write_iops_sec)", + ctxt, &def->blkdeviotune.write_iops_sec) < 0) { + def->blkdeviotune.write_iops_sec = 0; + } + + if ((def->blkdeviotune.total_bytes_sec && def->blkdeviotune.read_bytes_sec) + || (def->blkdeviotune.total_bytes_sec && def->blkdeviotune.write_bytes_sec)) { + virDomainReportError(VIR_ERR_XML_ERROR, + _("total and read/write bytes_sec cannot be set at the same time")); + goto error; + } + + if ((def->blkdeviotune.total_iops_sec && def->blkdeviotune.read_iops_sec) + || (def->blkdeviotune.total_iops_sec && def->blkdeviotune.write_iops_sec)) { + virDomainReportError(VIR_ERR_XML_ERROR, + _("total and read/write iops_sec cannot be set at the same time")); + goto error; + } } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) { def->readonly = 1; } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { @@ -6078,7 +6123,7 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps, if (xmlStrEqual(node->name, BAD_CAST "disk")) { dev->type = VIR_DOMAIN_DEVICE_DISK; - if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node, + if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node, ctxt, NULL, flags))) goto error; } else if (xmlStrEqual(node->name, BAD_CAST "lease")) { @@ -7148,6 +7193,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, for (i = 0 ; i < n ; i++) { virDomainDiskDefPtr disk = virDomainDiskDefParseXML(caps, nodes[i], + ctxt, bootMap, flags); if (!disk) @@ -9589,6 +9635,48 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " <target dev='%s' bus='%s'/>\n", def->dst, bus); + /*disk I/O throttling*/ + if (def->blkdeviotune.total_bytes_sec || + def->blkdeviotune.read_bytes_sec || + def->blkdeviotune.write_bytes_sec || + def->blkdeviotune.total_iops_sec || + def->blkdeviotune.read_iops_sec || + def->blkdeviotune.write_iops_sec) { + virBufferAddLit(buf, " <iotune>\n"); + if (def->blkdeviotune.total_bytes_sec) { + virBufferAsprintf(buf, " <total_bytes_sec>%llu</total_bytes_sec>\n", + def->blkdeviotune.total_bytes_sec); + } + + if (def->blkdeviotune.read_bytes_sec) { + virBufferAsprintf(buf, " <read_bytes_sec>%llu</read_bytes_sec>\n", + def->blkdeviotune.read_bytes_sec); + + } + + if (def->blkdeviotune.write_bytes_sec) { + virBufferAsprintf(buf, " <write_bytes_sec>%llu</write_bytes_sec>\n", + def->blkdeviotune.write_bytes_sec); + } + + if (def->blkdeviotune.total_iops_sec) { + virBufferAsprintf(buf, " <total_iops_sec>%llu</total_iops_sec>\n", + def->blkdeviotune.total_iops_sec); + } + + if (def->blkdeviotune.read_iops_sec) { + virBufferAsprintf(buf, " <read_iops_sec>%llu</read_iops_sec>", + def->blkdeviotune.read_iops_sec); + } + + if (def->blkdeviotune.write_iops_sec) { + virBufferAsprintf(buf, " <write_iops_sec>%llu</write_iops_sec>", + def->blkdeviotune.write_iops_sec); + } + + virBufferAddLit(buf, " </iotune>\n"); + } + if (def->bootIndex) virBufferAsprintf(buf, " <boot order='%d'/>\n", def->bootIndex); if (def->readonly) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 7511178..ff6921a 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -313,6 +313,17 @@ enum virDomainDiskSecretType { VIR_DOMAIN_DISK_SECRET_TYPE_LAST }; +typedef struct _virDomainBlockIoTuneInfo virDomainBlockIoTuneInfo; +struct _virDomainBlockIoTuneInfo { + unsigned long long total_bytes_sec; + unsigned long long read_bytes_sec; + unsigned long long write_bytes_sec; + unsigned long long total_iops_sec; + unsigned long long read_iops_sec; + unsigned long long write_iops_sec; +}; +typedef virDomainBlockIoTuneInfo *virDomainBlockIoTuneInfoPtr; + /* Stores the virtual disk configuration */ typedef struct _virDomainDiskDef virDomainDiskDef; typedef virDomainDiskDef *virDomainDiskDefPtr; @@ -335,6 +346,9 @@ struct _virDomainDiskDef { } auth; char *driverName; char *driverType; + + virDomainBlockIoTuneInfo blkdeviotune; + char *serial; int cachemode; int error_policy; /* enum virDomainDiskErrorPolicy */ -- 1.7.7.3

On Wed, Nov 23, 2011 at 02:44:45PM -0700, Eric Blake wrote:
From: Lei Li <lilei@linux.vnet.ibm.com>
Enable block I/O throttle for per-disk in XML, as the first per-disk IO tuning parameter.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com> [...] static virDomainDiskDefPtr virDomainDiskDefParseXML(virCapsPtr caps, xmlNodePtr node, + xmlXPathContextPtr ctxt, virBitmapPtr bootMap, unsigned int flags) { @@ -2594,6 +2595,50 @@ virDomainDiskDefParseXML(virCapsPtr caps, } child = child->next; } + } else if (xmlStrEqual(cur->name, BAD_CAST "iotune")) { + if (virXPathULongLong("string(./devices/disk/iotune/total_bytes_sec)", + ctxt, &def->blkdeviotune.total_bytes_sec) < 0) { + def->blkdeviotune.total_bytes_sec = 0; + }
This code is buggy I'm afraid. It's supposed to apply to virDomainDiskDefParseXML() which will go down in the tree searching for the informations on a given disk. By using an XPath query on the passed context without updating it with the current node, you go back to the domain root search and scan the fulls set of devices for iotune parameter, then if there is more than one disk with such param you will *concatenate* the string and get an erroneous value. 2 ways to fix this: - reset the node from ctxt to the current node of the disk parsed and use "iotune/total_bytes_sec" expressions (may also fit in the 80 char line ...) - just get the value directly not using XPath, but then get rid of the extra ctxt first is probably simpler w.r.t. current patch,
+ if (virXPathULongLong("string(./devices/disk/iotune/read_bytes_sec)", + ctxt, &def->blkdeviotune.read_bytes_sec) < 0) { + def->blkdeviotune.read_bytes_sec = 0; + } + + if (virXPathULongLong("string(./devices/disk/iotune/write_bytes_sec)", + ctxt, &def->blkdeviotune.write_bytes_sec) < 0) { + def->blkdeviotune.write_bytes_sec = 0; + } + + if (virXPathULongLong("string(./devices/disk/iotune/total_iops_sec)", + ctxt, &def->blkdeviotune.total_iops_sec) < 0) { + def->blkdeviotune.total_iops_sec = 0; + } + + if (virXPathULongLong("string(./devices/disk/iotune/read_iops_sec)", + ctxt, &def->blkdeviotune.read_iops_sec) < 0) { + def->blkdeviotune.read_iops_sec = 0; + } + + if (virXPathULongLong("string(./devices/disk/iotune/write_iops_sec)", + ctxt, &def->blkdeviotune.write_iops_sec) < 0) { + def->blkdeviotune.write_iops_sec = 0; + }
same applies to all those XPath queries.
+ if ((def->blkdeviotune.total_bytes_sec && def->blkdeviotune.read_bytes_sec) + || (def->blkdeviotune.total_bytes_sec && def->blkdeviotune.write_bytes_sec)) { + virDomainReportError(VIR_ERR_XML_ERROR, + _("total and read/write bytes_sec cannot be set at the same time")); + goto error; + } + + if ((def->blkdeviotune.total_iops_sec && def->blkdeviotune.read_iops_sec) + || (def->blkdeviotune.total_iops_sec && def->blkdeviotune.write_iops_sec)) { + virDomainReportError(VIR_ERR_XML_ERROR, + _("total and read/write iops_sec cannot be set at the same time")); + goto error; + } } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) { def->readonly = 1; } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { @@ -6078,7 +6123,7 @@ virDomainDeviceDefPtr virDomainDeviceDefParse(virCapsPtr caps,
if (xmlStrEqual(node->name, BAD_CAST "disk")) { dev->type = VIR_DOMAIN_DEVICE_DISK; - if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node, + if (!(dev->data.disk = virDomainDiskDefParseXML(caps, node, ctxt, NULL, flags))) goto error; } else if (xmlStrEqual(node->name, BAD_CAST "lease")) { @@ -7148,6 +7193,7 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps, for (i = 0 ; i < n ; i++) { virDomainDiskDefPtr disk = virDomainDiskDefParseXML(caps, nodes[i], + ctxt, bootMap, flags); if (!disk) @@ -9589,6 +9635,48 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAsprintf(buf, " <target dev='%s' bus='%s'/>\n", def->dst, bus);
+ /*disk I/O throttling*/ + if (def->blkdeviotune.total_bytes_sec || + def->blkdeviotune.read_bytes_sec || + def->blkdeviotune.write_bytes_sec || + def->blkdeviotune.total_iops_sec || + def->blkdeviotune.read_iops_sec || + def->blkdeviotune.write_iops_sec) { + virBufferAddLit(buf, " <iotune>\n"); + if (def->blkdeviotune.total_bytes_sec) { + virBufferAsprintf(buf, " <total_bytes_sec>%llu</total_bytes_sec>\n", + def->blkdeviotune.total_bytes_sec); + } + + if (def->blkdeviotune.read_bytes_sec) { + virBufferAsprintf(buf, " <read_bytes_sec>%llu</read_bytes_sec>\n", + def->blkdeviotune.read_bytes_sec); + + } + + if (def->blkdeviotune.write_bytes_sec) { + virBufferAsprintf(buf, " <write_bytes_sec>%llu</write_bytes_sec>\n", + def->blkdeviotune.write_bytes_sec); + } + + if (def->blkdeviotune.total_iops_sec) { + virBufferAsprintf(buf, " <total_iops_sec>%llu</total_iops_sec>\n", + def->blkdeviotune.total_iops_sec); + } + + if (def->blkdeviotune.read_iops_sec) { + virBufferAsprintf(buf, " <read_iops_sec>%llu</read_iops_sec>", + def->blkdeviotune.read_iops_sec); + } + + if (def->blkdeviotune.write_iops_sec) { + virBufferAsprintf(buf, " <write_iops_sec>%llu</write_iops_sec>", + def->blkdeviotune.write_iops_sec); + } + + virBufferAddLit(buf, " </iotune>\n"); + } +
Don't forget to reset back the ctxt->node on exit from the parsing routine if picking option 1
if (def->bootIndex) virBufferAsprintf(buf, " <boot order='%d'/>\n", def->bootIndex); if (def->readonly) diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
ACK, once fixed. Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On 11/24/2011 12:54 AM, Daniel Veillard wrote:
On Wed, Nov 23, 2011 at 02:44:45PM -0700, Eric Blake wrote:
From: Lei Li <lilei@linux.vnet.ibm.com>
Enable block I/O throttle for per-disk in XML, as the first per-disk IO tuning parameter.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com> [...]
This code is buggy I'm afraid. It's supposed to apply to virDomainDiskDefParseXML() which will go down in the tree searching for the informations on a given disk. By using an XPath query on the passed context without updating it with the current node, you go back to the domain root search and scan the fulls set of devices for iotune parameter, then if there is more than one disk with such param you will *concatenate* the string and get an erroneous value. 2 ways to fix this: - reset the node from ctxt to the current node of the disk parsed and use "iotune/total_bytes_sec" expressions (may also fit in the 80 char line ...)
We've done this elsewhere, so it's pretty easy to copy.
ACK, once fixed.
Here's what I'm squashing in. I still have to actually test response on qemu, though, before I'm comfortable pushing (it may be that the code as-is, while passing its self-test on xml handling, fails to gracefully handle old qemu that lacks support for this feature). diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index a2702c5..caf4cf5 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -2400,6 +2400,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, { virDomainDiskDefPtr def; xmlNodePtr cur, child; + xmlNodePtr save_ctxt = ctxt->node; char *type = NULL; char *device = NULL; char *snapshot = NULL; @@ -2431,6 +2432,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, return NULL; } + ctxt->node = node; + type = virXMLPropString(node, "type"); if (type) { if ((def->type = virDomainDiskTypeFromString(type)) < 0) { @@ -2596,47 +2599,59 @@ virDomainDiskDefParseXML(virCapsPtr caps, child = child->next; } } else if (xmlStrEqual(cur->name, BAD_CAST "iotune")) { - if (virXPathULongLong("string(./devices/disk/iotune/total_bytes_sec)", - ctxt, &def->blkdeviotune.total_bytes_sec) < 0) { + if (virXPathULongLong("string(./iotune/total_bytes_sec)", + ctxt, + &def->blkdeviotune.total_bytes_sec) < 0) { def->blkdeviotune.total_bytes_sec = 0; } - if (virXPathULongLong("string(./devices/disk/iotune/read_bytes_sec)", - ctxt, &def->blkdeviotune.read_bytes_sec) < 0) { + if (virXPathULongLong("string(./iotune/read_bytes_sec)", + ctxt, + &def->blkdeviotune.read_bytes_sec) < 0) { def->blkdeviotune.read_bytes_sec = 0; } - if (virXPathULongLong("string(./devices/disk/iotune/write_bytes_sec)", - ctxt, &def->blkdeviotune.write_bytes_sec) < 0) { + if (virXPathULongLong("string(./iotune/write_bytes_sec)", + ctxt, + &def->blkdeviotune.write_bytes_sec) < 0) { def->blkdeviotune.write_bytes_sec = 0; } - if (virXPathULongLong("string(./devices/disk/iotune/total_iops_sec)", - ctxt, &def->blkdeviotune.total_iops_sec) < 0) { + if (virXPathULongLong("string(./iotune/total_iops_sec)", + ctxt, + &def->blkdeviotune.total_iops_sec) < 0) { def->blkdeviotune.total_iops_sec = 0; } - if (virXPathULongLong("string(./devices/disk/iotune/read_iops_sec)", - ctxt, &def->blkdeviotune.read_iops_sec) < 0) { + if (virXPathULongLong("string(./iotune/read_iops_sec)", + ctxt, + &def->blkdeviotune.read_iops_sec) < 0) { def->blkdeviotune.read_iops_sec = 0; } - if (virXPathULongLong("string(./devices/disk/iotune/write_iops_sec)", - ctxt, &def->blkdeviotune.write_iops_sec) < 0) { + if (virXPathULongLong("string(./iotune/write_iops_sec)", + ctxt, + &def->blkdeviotune.write_iops_sec) < 0) { def->blkdeviotune.write_iops_sec = 0; } - if ((def->blkdeviotune.total_bytes_sec && def->blkdeviotune.read_bytes_sec) - || (def->blkdeviotune.total_bytes_sec && def->blkdeviotune.write_bytes_sec)) { + if ((def->blkdeviotune.total_bytes_sec && + def->blkdeviotune.read_bytes_sec) || + (def->blkdeviotune.total_bytes_sec && + def->blkdeviotune.write_bytes_sec)) { virDomainReportError(VIR_ERR_XML_ERROR, - _("total and read/write bytes_sec cannot be set at the same time")); + _("total and read/write bytes_sec " + "cannot be set at the same time")); goto error; } - if ((def->blkdeviotune.total_iops_sec && def->blkdeviotune.read_iops_sec) - || (def->blkdeviotune.total_iops_sec && def->blkdeviotune.write_iops_sec)) { + if ((def->blkdeviotune.total_iops_sec && + def->blkdeviotune.read_iops_sec) || + (def->blkdeviotune.total_iops_sec && + def->blkdeviotune.write_iops_sec)) { virDomainReportError(VIR_ERR_XML_ERROR, - _("total and read/write iops_sec cannot be set at the same time")); + _("total and read/write iops_sec " + "cannot be set at the same time")); goto error; } } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) { @@ -2933,6 +2948,7 @@ cleanup: virStorageEncryptionFree(encryption); VIR_FREE(startupPolicy); + ctxt->node = save_ctxt; return def; no_memory: -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 11/24/2011 11:01 PM, Eric Blake wrote:
On 11/24/2011 12:54 AM, Daniel Veillard wrote:
From: Lei Li<lilei@linux.vnet.ibm.com>
Enable block I/O throttle for per-disk in XML, as the first per-disk IO tuning parameter.
Signed-off-by: Lei Li<lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu<wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake<eblake@redhat.com> [...] This code is buggy I'm afraid. It's supposed to apply to virDomainDiskDefParseXML() which will go down in the tree searching for the informations on a given disk. By using an XPath query on the
On Wed, Nov 23, 2011 at 02:44:45PM -0700, Eric Blake wrote: passed context without updating it with the current node, you go back to the domain root search and scan the fulls set of devices for iotune parameter, then if there is more than one disk with such param you will *concatenate* the string and get an erroneous value. 2 ways to fix this: - reset the node from ctxt to the current node of the disk parsed and use "iotune/total_bytes_sec" expressions (may also fit in the 80 char line ...) We've done this elsewhere, so it's pretty easy to copy.
Thank you so much for your kind help! :-)
ACK, once fixed. Here's what I'm squashing in. I still have to actually test response on qemu, though, before I'm comfortable pushing (it may be that the code as-is, while passing its self-test on xml handling, fails to gracefully handle old qemu that lacks support for this feature).
diff --git i/src/conf/domain_conf.c w/src/conf/domain_conf.c index a2702c5..caf4cf5 100644 --- i/src/conf/domain_conf.c +++ w/src/conf/domain_conf.c @@ -2400,6 +2400,7 @@ virDomainDiskDefParseXML(virCapsPtr caps, { virDomainDiskDefPtr def; xmlNodePtr cur, child; + xmlNodePtr save_ctxt = ctxt->node; char *type = NULL; char *device = NULL; char *snapshot = NULL; @@ -2431,6 +2432,8 @@ virDomainDiskDefParseXML(virCapsPtr caps, return NULL; }
+ ctxt->node = node; + type = virXMLPropString(node, "type"); if (type) { if ((def->type = virDomainDiskTypeFromString(type))< 0) { @@ -2596,47 +2599,59 @@ virDomainDiskDefParseXML(virCapsPtr caps, child = child->next; } } else if (xmlStrEqual(cur->name, BAD_CAST "iotune")) { - if (virXPathULongLong("string(./devices/disk/iotune/total_bytes_sec)", - ctxt, &def->blkdeviotune.total_bytes_sec)< 0) { + if (virXPathULongLong("string(./iotune/total_bytes_sec)", + ctxt, + &def->blkdeviotune.total_bytes_sec)< 0) { def->blkdeviotune.total_bytes_sec = 0; }
- if (virXPathULongLong("string(./devices/disk/iotune/read_bytes_sec)", - ctxt, &def->blkdeviotune.read_bytes_sec)< 0) { + if (virXPathULongLong("string(./iotune/read_bytes_sec)", + ctxt, + &def->blkdeviotune.read_bytes_sec)< 0) { def->blkdeviotune.read_bytes_sec = 0; }
- if (virXPathULongLong("string(./devices/disk/iotune/write_bytes_sec)", - ctxt, &def->blkdeviotune.write_bytes_sec)< 0) { + if (virXPathULongLong("string(./iotune/write_bytes_sec)", + ctxt, + &def->blkdeviotune.write_bytes_sec)< 0) { def->blkdeviotune.write_bytes_sec = 0; }
- if (virXPathULongLong("string(./devices/disk/iotune/total_iops_sec)", - ctxt, &def->blkdeviotune.total_iops_sec)< 0) { + if (virXPathULongLong("string(./iotune/total_iops_sec)", + ctxt, + &def->blkdeviotune.total_iops_sec)< 0) { def->blkdeviotune.total_iops_sec = 0; }
- if (virXPathULongLong("string(./devices/disk/iotune/read_iops_sec)", - ctxt, &def->blkdeviotune.read_iops_sec)< 0) { + if (virXPathULongLong("string(./iotune/read_iops_sec)", + ctxt, +&def->blkdeviotune.read_iops_sec) < 0) { def->blkdeviotune.read_iops_sec = 0; }
- if (virXPathULongLong("string(./devices/disk/iotune/write_iops_sec)", - ctxt, &def->blkdeviotune.write_iops_sec)< 0) { + if (virXPathULongLong("string(./iotune/write_iops_sec)", + ctxt, + &def->blkdeviotune.write_iops_sec)< 0) { def->blkdeviotune.write_iops_sec = 0; }
- if ((def->blkdeviotune.total_bytes_sec&& def->blkdeviotune.read_bytes_sec) - || (def->blkdeviotune.total_bytes_sec&& def->blkdeviotune.write_bytes_sec)) { + if ((def->blkdeviotune.total_bytes_sec&& + def->blkdeviotune.read_bytes_sec) || + (def->blkdeviotune.total_bytes_sec&& + def->blkdeviotune.write_bytes_sec)) { virDomainReportError(VIR_ERR_XML_ERROR, - _("total and read/write bytes_sec cannot be set at the same time")); + _("total and read/write bytes_sec " + "cannot be set at the same time")); goto error; }
- if ((def->blkdeviotune.total_iops_sec&& def->blkdeviotune.read_iops_sec) - || (def->blkdeviotune.total_iops_sec&& def->blkdeviotune.write_iops_sec)) { + if ((def->blkdeviotune.total_iops_sec&& + def->blkdeviotune.read_iops_sec) || + (def->blkdeviotune.total_iops_sec&& + def->blkdeviotune.write_iops_sec)) { virDomainReportError(VIR_ERR_XML_ERROR, - _("total and read/write iops_sec cannot be set at the same time")); + _("total and read/write iops_sec " + "cannot be set at the same time")); goto error; } } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) { @@ -2933,6 +2948,7 @@ cleanup: virStorageEncryptionFree(encryption); VIR_FREE(startupPolicy);
+ ctxt->node = save_ctxt; return def;
no_memory:
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Lynn

On 11/24/2011 08:01 AM, Eric Blake wrote:
ACK, once fixed.
Here's what I'm squashing in. I still have to actually test response on qemu, though, before I'm comfortable pushing (it may be that the code as-is, while passing its self-test on xml handling, fails to gracefully handle old qemu that lacks support for this feature).
As I said in another mail, testing looks good. I ran out of time today; I'll finish pushing the other 6 in this series tomorrow. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 11/23/2011 02:44 PM, Eric Blake wrote:
From: Lei Li <lilei@linux.vnet.ibm.com>
Enable block I/O throttle for per-disk in XML, as the first per-disk IO tuning parameter.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com>
@@ -1039,6 +1044,40 @@ <span class="since">Since 0.0.3; <code>bus</code> attribute since 0.4.3; "usb" attribute value since after 0.4.4; "sata" attribute value since 0.9.7</span></dd> + <dt><code>iotune</code></dt> + <dd>The optional <code>iotune</code> element provides the + ability to provide additional per-device I/O tuning, with + values that can vary for each device (contrast this to + the <a href="#elementsBlockTuning"><code><blkiotune></code></a> + element, which applies globally to the domain).
I added a cross-reference in one direction, but not the other, so I'll squash this in: diff --git i/docs/formatdomain.html.in w/docs/formatdomain.html.in index 933072e..f08b948 100644 --- i/docs/formatdomain.html.in +++ w/docs/formatdomain.html.in @@ -536,7 +536,11 @@ single host block device, if they are backed by files within the same host file system, which is why this tuning parameter is at the global domain level rather than associated with each - guest disk device. Each <code>device</code> element has two + guest disk device (contrast this to + the <a href="#elementsDisks"><code><iotune></code></a> + element which can apply to an + individual <code><disk></code>). + Each <code>device</code> element has two mandatory sub-elements, <code>path</code> describing the absolute path of the device, and <code>weight</code> giving the relative weight of that device, in the range [100, -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: Lei Li <lilei@linux.vnet.ibm.com> Implement the block I/O throttle setting and getting support to qemu driver. Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- src/qemu/qemu_command.c | 31 ++++ src/qemu/qemu_driver.c | 340 ++++++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_monitor.c | 33 ++++ src/qemu/qemu_monitor.h | 8 + src/qemu/qemu_monitor_json.c | 176 ++++++++++++++++++++++ src/qemu/qemu_monitor_json.h | 8 + src/qemu/qemu_monitor_text.c | 151 +++++++++++++++++++- src/qemu/qemu_monitor_text.h | 8 + 8 files changed, 754 insertions(+), 1 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 85b34bb..905afa6 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -1866,6 +1866,37 @@ qemuBuildDriveStr(virConnectPtr conn ATTRIBUTE_UNUSED, } } + /*block I/O throttling*/ + if (disk->blkdeviotune.total_bytes_sec) { + virBufferAsprintf(&opt, ",bps=%llu", + disk->blkdeviotune.total_bytes_sec); + } + + if (disk->blkdeviotune.read_bytes_sec) { + virBufferAsprintf(&opt, ",bps_rd=%llu", + disk->blkdeviotune.read_bytes_sec); + } + + if (disk->blkdeviotune.write_bytes_sec) { + virBufferAsprintf(&opt, ",bps_wr=%llu", + disk->blkdeviotune.write_bytes_sec); + } + + if (disk->blkdeviotune.total_iops_sec) { + virBufferAsprintf(&opt, ",iops=%llu", + disk->blkdeviotune.total_iops_sec); + } + + if (disk->blkdeviotune.read_iops_sec) { + virBufferAsprintf(&opt, ",iops_rd=%llu", + disk->blkdeviotune.read_iops_sec); + } + + if (disk->blkdeviotune.write_iops_sec) { + virBufferAsprintf(&opt, ",iops_wr=%llu", + disk->blkdeviotune.write_iops_sec); + } + if (virBufferError(&opt)) { virReportOOMError(); goto error; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 94fbe94..0308a41 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -93,6 +93,8 @@ #define QEMU_NB_MEM_PARAM 3 +#define QEMU_NB_BLOCK_IO_TUNE_PARAM 6 + #if HAVE_LINUX_KVM_H # include <linux/kvm.h> #endif @@ -10763,6 +10765,342 @@ cleanup: return ret; } +static int +qemuDomainSetBlockIoTune(virDomainPtr dom, + const char *disk, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + qemuDomainObjPrivatePtr priv; + virDomainDefPtr persistentDef = NULL; + virDomainBlockIoTuneInfo info; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + const char *device = NULL; + int ret = -1; + int i; + bool isActive; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + memset(&info, 0, sizeof(info)); + + qemuDriverLock(driver); + virUUIDFormat(dom->uuid, uuidstr); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + device = qemuDiskPathToAlias(vm, disk); + if (!device) { + goto cleanup; + } + + if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + isActive = virDomainObjIsActive(vm); + + if (flags == VIR_DOMAIN_AFFECT_CURRENT) { + if (isActive) + flags = VIR_DOMAIN_AFFECT_LIVE; + else + flags = VIR_DOMAIN_AFFECT_CONFIG; + } + + if (!isActive && (flags & VIR_DOMAIN_AFFECT_LIVE)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (!vm->persistent) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot change persistent config of a transient domain")); + goto endjob; + } + if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) + goto endjob; + } + + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + + if (param->type != VIR_TYPED_PARAM_ULLONG) { + qemuReportError(VIR_ERR_INVALID_ARG, + _("expected unsigned long long for parameter %s"), + param->field); + goto endjob; + } + + if (STREQ(param->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC)) { + info.total_bytes_sec = param->value.ul; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC)) { + info.read_bytes_sec = param->value.ul; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC)) { + info.write_bytes_sec = param->value.ul; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC)) { + info.total_iops_sec = param->value.ul; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC)) { + info.read_iops_sec = param->value.ul; + } else if (STREQ(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC)) { + info.write_iops_sec = param->value.ul; + } else { + qemuReportError(VIR_ERR_INVALID_ARG, + _("Unrecognized parameter %s"), + param->field); + goto endjob; + } + } + + if ((info.total_bytes_sec && info.read_bytes_sec) || + (info.total_bytes_sec && info.write_bytes_sec)) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("total and read/write of bytes_sec cannot be set at the same time")); + goto endjob; + } + + if ((info.total_iops_sec && info.read_iops_sec) || + (info.total_iops_sec && info.write_iops_sec)) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("total and read/write of iops_sec cannot be set at the same time")); + goto endjob; + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + sa_assert(persistentDef); + int idx = virDomainDiskIndexByName(vm->def, disk, true); + if (i < 0) + goto endjob; + persistentDef->disks[idx]->blkdeviotune = info; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + priv = vm->privateData; + qemuDomainObjEnterMonitorWithDriver(driver, vm); + ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info); + qemuDomainObjExitMonitorWithDriver(driver, vm); + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + ret = virDomainSaveConfig(driver->configDir, persistentDef); + if (ret < 0) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("Write to config file failed")); + goto endjob; + } + } + +endjob: + if (qemuDomainObjEndJob(driver, vm) == 0) + vm = NULL; + +cleanup: + VIR_FREE(device); + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + +static int +qemuDomainGetBlockIoTune(virDomainPtr dom, + const char *disk, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) +{ + struct qemud_driver *driver = dom->conn->privateData; + virDomainObjPtr vm = NULL; + qemuDomainObjPrivatePtr priv; + virDomainDefPtr persistentDef = NULL; + virDomainBlockIoTuneInfo reply; + char uuidstr[VIR_UUID_STRING_BUFLEN]; + const char *device = NULL; + int ret = -1; + int i; + bool isActive; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG | + VIR_TYPED_PARAM_STRING_OKAY, -1); + + /* We don't return strings, and thus trivially support this flag. */ + flags &= ~VIR_TYPED_PARAM_STRING_OKAY; + + qemuDriverLock(driver); + virUUIDFormat(dom->uuid, uuidstr); + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + if (!vm) { + qemuReportError(VIR_ERR_NO_DOMAIN, + _("no domain with matching uuid '%s'"), uuidstr); + goto cleanup; + } + + if ((*nparams) == 0) { + /* Current number of parameters supported by QEMU Block I/O Throttling */ + *nparams = QEMU_NB_BLOCK_IO_TUNE_PARAM; + ret = 0; + goto cleanup; + } + + device = qemuDiskPathToAlias(vm, disk); + + if (!device) { + goto cleanup; + } + + if (qemuDomainObjBeginJobWithDriver(driver, vm, QEMU_JOB_MODIFY) < 0) + goto cleanup; + + isActive = virDomainObjIsActive(vm); + + if (flags == VIR_DOMAIN_AFFECT_CURRENT) { + if (isActive) + flags = VIR_DOMAIN_AFFECT_LIVE; + else + flags = VIR_DOMAIN_AFFECT_CONFIG; + } + + if ((flags & VIR_DOMAIN_AFFECT_LIVE) && (flags & VIR_DOMAIN_AFFECT_CONFIG)) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("Cannot query with --live and --config together")); + goto endjob; + } + + if (!isActive && (flags & VIR_DOMAIN_AFFECT_LIVE)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is not running")); + goto endjob; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + priv = vm->privateData; + qemuDomainObjEnterMonitorWithDriver(driver, vm); + ret = qemuMonitorGetBlockIoThrottle(priv->mon, device, &reply); + qemuDomainObjExitMonitorWithDriver(driver, vm); + if (ret < 0) + goto endjob; + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (!vm->persistent) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("domain is transient")); + goto endjob; + } + if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) + goto endjob; + + int idx = virDomainDiskIndexByName(vm->def, disk, true); + if (idx < 0) + goto endjob; + reply = persistentDef->disks[idx]->blkdeviotune; + } + + for (i = 0; i < QEMU_NB_BLOCK_IO_TUNE_PARAM && i < *nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + param->value.ul = 0; + param->type = VIR_TYPED_PARAM_ULLONG; + + switch(i) { + case 0: + if (virStrcpyStatic(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC); + goto endjob; + } + param->value.ul = reply.total_bytes_sec; + break; + + case 1: + if (virStrcpyStatic(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC); + goto endjob; + } + param->value.ul = reply.read_bytes_sec; + break; + + case 2: + if (virStrcpyStatic(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC); + goto endjob; + } + param->value.ul = reply.write_bytes_sec; + break; + + case 3: + if (virStrcpyStatic(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC); + goto endjob; + } + param->value.ul = reply.total_iops_sec; + break; + + case 4: + if (virStrcpyStatic(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC); + goto endjob; + } + param->value.ul = reply.read_iops_sec; + break; + + case 5: + if (virStrcpyStatic(param->field, + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field name '%s' too long"), + VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC); + goto endjob; + } + param->value.ul = reply.write_iops_sec; + break; + default: + break; + } + } + + if (*nparams > QEMU_NB_BLOCK_IO_TUNE_PARAM) + *nparams = QEMU_NB_BLOCK_IO_TUNE_PARAM; + ret = 0; + +endjob: + if (qemuDomainObjEndJob(driver, vm) == 0) + vm = NULL; + +cleanup: + VIR_FREE(device); + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} static virDriver qemuDriver = { .no = VIR_DRV_QEMU, @@ -10907,6 +11245,8 @@ static virDriver qemuDriver = { .domainGetBlockJobInfo = qemuDomainGetBlockJobInfo, /* 0.9.4 */ .domainBlockJobSetSpeed = qemuDomainBlockJobSetSpeed, /* 0.9.4 */ .domainBlockPull = qemuDomainBlockPull, /* 0.9.4 */ + .domainSetBlockIoTune = qemuDomainSetBlockIoTune, /* 0.9.8 */ + .domainGetBlockIoTune = qemuDomainGetBlockIoTune, /* 0.9.8 */ }; diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index 73e5ea9..660aa11 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -2567,6 +2567,39 @@ int qemuMonitorBlockJob(qemuMonitorPtr mon, return ret; } +int qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoTuneInfoPtr info) +{ + int ret; + + VIR_DEBUG("mon=%p, device=%p, info=%p", mon, device, info); + + if (mon->json) { + ret = qemuMonitorJSONSetBlockIoThrottle(mon, device, info); + } else { + ret = qemuMonitorTextSetBlockIoThrottle(mon, device, info); + } + return ret; +} + +int qemuMonitorGetBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoTuneInfoPtr reply) +{ + int ret; + + VIR_DEBUG("mon=%p, device=%p, reply=%p", mon, device, reply); + + if (mon->json) { + ret = qemuMonitorJSONGetBlockIoThrottle(mon, device, reply); + } else { + ret = qemuMonitorTextGetBlockIoThrottle(mon, device, reply); + } + return ret; +} + + int qemuMonitorVMStatusToPausedReason(const char *status) { int st; diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 883e0aa..0b14e0c 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -521,6 +521,14 @@ int qemuMonitorOpenGraphics(qemuMonitorPtr mon, const char *fdname, bool skipauth); +int qemuMonitorSetBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoTuneInfoPtr info); + +int qemuMonitorGetBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoTuneInfoPtr reply); + /** * When running two dd process and using <> redirection, we need a * shell that will not truncate files. These two strings serve that diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c index 56a62db..fb2a1fd 100644 --- a/src/qemu/qemu_monitor_json.c +++ b/src/qemu/qemu_monitor_json.c @@ -3276,3 +3276,179 @@ int qemuMonitorJSONOpenGraphics(qemuMonitorPtr mon, virJSONValueFree(reply); return ret; } + + +static int +qemuMonitorJSONBlockIoThrottleInfo(virJSONValuePtr result, + const char *device, + virDomainBlockIoTuneInfoPtr reply) +{ + virJSONValuePtr io_throttle; + int ret = -1; + int i; + int found = 0; + + io_throttle = virJSONValueObjectGet(result, "return"); + + if (!io_throttle || io_throttle->type != VIR_JSON_TYPE_ARRAY) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _(" block_io_throttle reply was missing device list")); + goto cleanup; + } + + for (i = 0; i < virJSONValueArraySize(io_throttle); i++) { + virJSONValuePtr temp_dev = virJSONValueArrayGet(io_throttle, i); + virJSONValuePtr inserted; + const char *current_dev; + + if (!temp_dev || temp_dev->type != VIR_JSON_TYPE_OBJECT) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block io throttle device entry was not in expected format")); + goto cleanup; + } + + if ((current_dev = virJSONValueObjectGetString(temp_dev, "device")) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block io throttle device entry was not in expected format")); + goto cleanup; + } + + if(STRPREFIX(current_dev, QEMU_DRIVE_HOST_PREFIX)) + current_dev += strlen(QEMU_DRIVE_HOST_PREFIX); + + if (STREQ(current_dev, device)) + continue; + + found = 1; + if ((inserted = virJSONValueObjectGet(temp_dev, "inserted")) == NULL || + inserted->type != VIR_JSON_TYPE_OBJECT) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("block io throttle inserted entry was not in expected format")); + goto cleanup; + } + + if (virJSONValueObjectGetNumberUlong(inserted, "bps", &reply->total_bytes_sec) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot read total_bytes_sec")); + goto cleanup; + } + + if (virJSONValueObjectGetNumberUlong(inserted, "bps_rd", &reply->read_bytes_sec) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot read read_bytes_sec")); + goto cleanup; + } + + if (virJSONValueObjectGetNumberUlong(inserted, "bps_wr", &reply->write_bytes_sec) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot read write_bytes_sec")); + goto cleanup; + } + + if (virJSONValueObjectGetNumberUlong(inserted, "iops", &reply->total_iops_sec) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot read total_iops_sec")); + goto cleanup; + } + + if (virJSONValueObjectGetNumberUlong(inserted, "iops_rd", &reply->read_iops_sec) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot read read_iops_sec")); + goto cleanup; + } + + if (virJSONValueObjectGetNumberUlong(inserted, "iops_wr", &reply->write_iops_sec) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("cannot read write_iops_sec")); + goto cleanup; + } + break; + } + + if (!found) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find throttling info for device '%s'"), + device); + goto cleanup; + } + ret = 0; + +cleanup: + return ret; +} + +int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoTuneInfoPtr info) +{ + int ret = -1; + virJSONValuePtr cmd = NULL; + virJSONValuePtr result = NULL; + + cmd = qemuMonitorJSONMakeCommand("block_set_io_throttle", + "s:device", device, + "U:bps", info->total_bytes_sec, + "U:bps_rd", info->read_bytes_sec, + "U:bps_wr", info->write_bytes_sec, + "U:iops", info->total_iops_sec, + "U:iops_rd", info->read_iops_sec, + "U:iops_wr", info->write_iops_sec, + NULL); + if (!cmd) + return -1; + + ret = qemuMonitorJSONCommand(mon, cmd, &result); + + if (ret == 0 && virJSONValueObjectHasKey(result, "error")) { + if (qemuMonitorJSONHasError(result, "DeviceNotActive")) + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("No active operation on device: %s"), device); + else if (qemuMonitorJSONHasError(result, "NotSupported")) + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("Operation is not supported for device: %s"), device); + else + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected error")); + ret = -1; + } + + virJSONValueFree(cmd); + virJSONValueFree(result); + return ret; +} + +int qemuMonitorJSONGetBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoTuneInfoPtr reply) +{ + int ret = -1; + virJSONValuePtr cmd = NULL; + virJSONValuePtr result = NULL; + + cmd = qemuMonitorJSONMakeCommand("query-block", NULL); + if (!cmd) { + return -1; + } + + ret = qemuMonitorJSONCommand(mon, cmd, &result); + + if (ret == 0 && virJSONValueObjectHasKey(result, "error")) { + if (qemuMonitorJSONHasError(result, "DeviceNotActive")) + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("No active operation on device: %s"), device); + else if (qemuMonitorJSONHasError(result, "NotSupported")) + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("Operation is not supported for device: %s"), device); + else + qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Unexpected error")); + ret = -1; + } + + if (ret == 0) + ret = qemuMonitorJSONBlockIoThrottleInfo(result, device, reply); + + virJSONValueFree(cmd); + virJSONValueFree(result); + return ret; +} diff --git a/src/qemu/qemu_monitor_json.h b/src/qemu/qemu_monitor_json.h index f10d7d2..1b8625f 100644 --- a/src/qemu/qemu_monitor_json.h +++ b/src/qemu/qemu_monitor_json.h @@ -255,4 +255,12 @@ int qemuMonitorJSONOpenGraphics(qemuMonitorPtr mon, const char *fdname, bool skipauth); +int qemuMonitorJSONSetBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoTuneInfoPtr info); + +int qemuMonitorJSONGetBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoTuneInfoPtr reply); + #endif /* QEMU_MONITOR_JSON_H */ diff --git a/src/qemu/qemu_monitor_text.c b/src/qemu/qemu_monitor_text.c index 5de4d24..c734892 100644 --- a/src/qemu/qemu_monitor_text.c +++ b/src/qemu/qemu_monitor_text.c @@ -812,7 +812,7 @@ int qemuMonitorTextGetBlockInfo(qemuMonitorPtr mon, if (!eol) eol = p + strlen(p); - p += devnamelen + 2; /*Skip to first label. */ + p += devnamelen + 2; /* Skip to first label. */ while (*p) { if (STRPREFIX(p, "removable=")) { @@ -3429,3 +3429,152 @@ cleanup: VIR_FREE(cmd); return ret; } + + +int qemuMonitorTextSetBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoTuneInfoPtr info) +{ + char *cmd = NULL; + char *result = NULL; + int ret = 0; + const char *cmd_name = NULL; + + /* For the not specified fields, 0 by default */ + cmd_name = "block_set_io_throttle"; + ret = virAsprintf(&cmd, "%s %s %llu %llu %llu %llu %llu %llu", cmd_name, + device, info->total_bytes_sec, info->read_bytes_sec, + info->write_bytes_sec, info->total_iops_sec, + info->read_iops_sec, info->write_iops_sec); + + if (ret < 0) { + virReportOOMError(); + return -1; + } + + if (qemuMonitorHMPCommand(mon, cmd, &result) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot run monitor command")); + ret = -1; + goto cleanup; + } + + if (qemuMonitorTextCommandNotFound(cmd_name, result)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("Command '%s' is not found"), cmd_name); + ret = -1; + goto cleanup; + } + +cleanup: + VIR_FREE(cmd); + VIR_FREE(result); + return ret; +} + +static int +qemuMonitorTextParseBlockIoThrottle(const char *result, + const char *device, + virDomainBlockIoTuneInfoPtr reply) +{ + char *dummy = NULL; + int ret = -1; + const char *p, *eol; + int devnamelen = strlen(device); + + p = result; + + while (*p) { + if (STRPREFIX(p, QEMU_DRIVE_HOST_PREFIX)) + p += strlen(QEMU_DRIVE_HOST_PREFIX); + + if (STREQLEN(p, device, devnamelen) && + p[devnamelen] == ':' && p[devnamelen+1] == ' ') { + + eol = strchr(p, '\n'); + if (!eol) + eol = p + strlen(p); + + p += devnamelen + 2; /* Skip to first label. */ + + while (*p) { + if (STRPREFIX(p, "bps=")) { + p += strlen("bps="); + if (virStrToLong_ull(p, &dummy, 10, &reply->total_bytes_sec) == -1) + VIR_DEBUG("error reading total_bytes_sec: %s", p); + } else if (STRPREFIX(p, "bps_rd=")) { + p += strlen("bps_rd="); + if (virStrToLong_ull(p, &dummy, 10, &reply->read_bytes_sec) == -1) + VIR_DEBUG("error reading read_bytes_sec: %s", p); + } else if (STRPREFIX(p, "bps_wr=")) { + p += strlen("bps_wr="); + if (virStrToLong_ull(p, &dummy, 10, &reply->write_bytes_sec) == -1) + VIR_DEBUG("error reading write_bytes_sec: %s", p); + } else if (STRPREFIX(p, "iops=")) { + p += strlen("iops="); + if (virStrToLong_ull(p, &dummy, 10, &reply->total_iops_sec) == -1) + VIR_DEBUG("error reading total_iops_sec: %s", p); + } else if (STRPREFIX(p, "iops_rd=")) { + p += strlen("iops_rd="); + if (virStrToLong_ull(p, &dummy, 10, &reply->read_iops_sec) == -1) + VIR_DEBUG("error reading read_iops_sec: %s", p); + } else if (STRPREFIX(p, "iops_wr=")) { + p += strlen("iops_wr="); + if (virStrToLong_ull(p, &dummy, 10, &reply->write_iops_sec) == -1) + VIR_DEBUG("error reading write_iops_sec: %s", p); + } else { + VIR_DEBUG(" unknown block info %s", p); + } + + /* Skip to next label. */ + p = strchr (p, ' '); + if (!p || p >= eol) + break; + p++; + } + ret = 0; + goto cleanup; + } + + /* Skip to next line. */ + p = strchr (p, '\n'); + if (!p) + break; + p++; + } + + qemuReportError(VIR_ERR_INVALID_ARG, + _("No info for device '%s'"), device); + +cleanup: + return ret; +} + +int qemuMonitorTextGetBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoTuneInfoPtr reply) +{ + char *result = NULL; + int ret = 0; + const char *cmd_name = "info block"; + + if (qemuMonitorHMPCommand(mon, cmd_name, &result) < 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("cannot run monitor command")); + ret = -1; + goto cleanup; + } + + if (qemuMonitorTextCommandNotFound(cmd_name, result)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + _("Command '%s' is not found"), cmd_name); + ret = -1; + goto cleanup; + } + + ret = qemuMonitorTextParseBlockIoThrottle(result, device, reply); + +cleanup: + VIR_FREE(result); + return ret; +} diff --git a/src/qemu/qemu_monitor_text.h b/src/qemu/qemu_monitor_text.h index f32fce0..c3e97e0 100644 --- a/src/qemu/qemu_monitor_text.h +++ b/src/qemu/qemu_monitor_text.h @@ -248,4 +248,12 @@ int qemuMonitorTextOpenGraphics(qemuMonitorPtr mon, const char *fdname, bool skipauth); +int qemuMonitorTextSetBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoTuneInfoPtr info); + +int qemuMonitorTextGetBlockIoThrottle(qemuMonitorPtr mon, + const char *device, + virDomainBlockIoTuneInfoPtr reply); + #endif /* QEMU_MONITOR_TEXT_H */ -- 1.7.7.3

On 11/23/2011 02:44 PM, Eric Blake wrote:
From: Lei Li <lilei@linux.vnet.ibm.com>
Implement the block I/O throttle setting and getting support to qemu driver.
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com>
+qemuDomainSetBlockIoTune(virDomainPtr dom, + const char *disk, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{
+ + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (!vm->persistent) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot change persistent config of a transient domain")); + goto endjob; + } + if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) + goto endjob; + } +
+ + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + sa_assert(persistentDef); + int idx = virDomainDiskIndexByName(vm->def, disk, true);
Oops - this should be on persistentDef, not vm->def.
+ if (i < 0) + goto endjob; + persistentDef->disks[idx]->blkdeviotune = info;
And this assignment should be delayed...
+ } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + priv = vm->privateData; + qemuDomainObjEnterMonitorWithDriver(driver, vm); + ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info); + qemuDomainObjExitMonitorWithDriver(driver, vm); + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) {
to here, after we know the live change (if any) took place. Not to mention that we must not get here if the live change failed. Here's what I'm squashing in: diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 698a961..ce4cba1 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -11080,6 +11080,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, int ret = -1; int i; bool isActive; + int idx = -1; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -11126,6 +11127,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, } if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) goto endjob; + idx = virDomainDiskIndexByName(persistentDef, disk, true); + if (i < 0) + goto endjob; } for (i = 0; i < nparams; i++) { @@ -11177,22 +11181,18 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; } - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - sa_assert(persistentDef); - int idx = virDomainDiskIndexByName(vm->def, disk, true); - if (i < 0) - goto endjob; - persistentDef->disks[idx]->blkdeviotune = info; - } - if (flags & VIR_DOMAIN_AFFECT_LIVE) { priv = vm->privateData; qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device, &info); qemuDomainObjExitMonitorWithDriver(driver, vm); } + if (ret < 0) + goto endjob; if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + sa_assert(persistentDef && idx >= 0); + persistentDef->disks[idx]->blkdeviotune = info; ret = virDomainSaveConfig(driver->configDir, persistentDef); if (ret < 0) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 12/01/2011 02:15 AM, Eric Blake wrote:
On 11/23/2011 02:44 PM, Eric Blake wrote:
From: Lei Li<lilei@linux.vnet.ibm.com>
Implement the block I/O throttle setting and getting support to qemu driver.
Signed-off-by: Lei Li<lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu<wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake<eblake@redhat.com> +qemuDomainSetBlockIoTune(virDomainPtr dom, + const char *disk, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + + if (flags& VIR_DOMAIN_AFFECT_CONFIG) { + if (!vm->persistent) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot change persistent config of a transient domain")); + goto endjob; + } + if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) + goto endjob; + } + + + if (flags& VIR_DOMAIN_AFFECT_CONFIG) { + sa_assert(persistentDef); + int idx = virDomainDiskIndexByName(vm->def, disk, true); Oops - this should be on persistentDef, not vm->def.
+ if (i< 0) + goto endjob; + persistentDef->disks[idx]->blkdeviotune = info; And this assignment should be delayed...
+ } + + if (flags& VIR_DOMAIN_AFFECT_LIVE) { + priv = vm->privateData; + qemuDomainObjEnterMonitorWithDriver(driver, vm); + ret = qemuMonitorSetBlockIoThrottle(priv->mon, device,&info); + qemuDomainObjExitMonitorWithDriver(driver, vm); + } + + if (flags& VIR_DOMAIN_AFFECT_CONFIG) { to here, after we know the live change (if any) took place. Not to mention that we must not get here if the live change failed. Here's what I'm squashing in:
diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 698a961..ce4cba1 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -11080,6 +11080,7 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, int ret = -1; int i; bool isActive; + int idx = -1;
virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -11126,6 +11127,9 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, } if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) goto endjob; + idx = virDomainDiskIndexByName(persistentDef, disk, true); + if (i< 0) + goto endjob; }
for (i = 0; i< nparams; i++) { @@ -11177,22 +11181,18 @@ qemuDomainSetBlockIoTune(virDomainPtr dom, goto endjob; }
- if (flags& VIR_DOMAIN_AFFECT_CONFIG) { - sa_assert(persistentDef); - int idx = virDomainDiskIndexByName(vm->def, disk, true); - if (i< 0) - goto endjob; - persistentDef->disks[idx]->blkdeviotune = info; - } - if (flags& VIR_DOMAIN_AFFECT_LIVE) { priv = vm->privateData; qemuDomainObjEnterMonitorWithDriver(driver, vm); ret = qemuMonitorSetBlockIoThrottle(priv->mon, device,&info); qemuDomainObjExitMonitorWithDriver(driver, vm); } + if (ret< 0) + goto endjob; Hi Eric,
Thanks again for your kind help! An error occurred when I test it. virsh #blkdeviotune kvm vda --total_bytes_sec 80000 --config error: Unable to change block I/O throttle error: An error occurred, but the cause is unknown I dig into the code, here is a logic error. The initial value of ret = -1, if just set --config, it will goto endjob directly without doing its really job here. I guess the purpose you add these two lines is to check if live option succeeded, how about just get rid of these two lines. since the relevant check has already done in qemu monitor code. If live option failed it will return ret = -1 with error report, and if ret< 0, src/libvirt.c will goto error. I submit a patch based on above to avoid this error.
if (flags& VIR_DOMAIN_AFFECT_CONFIG) { + sa_assert(persistentDef&& idx>= 0); + persistentDef->disks[idx]->blkdeviotune = info; ret = virDomainSaveConfig(driver->configDir, persistentDef); if (ret< 0) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s",
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Lei

From: Lei Li <lilei@linux.vnet.ibm.com> Support virsh command blkdeviotune. Can set or query a block disk I/O throttle setting. Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- tools/virsh.c | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 30 +++++++ 2 files changed, 274 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 50fc98d..ea5a267 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -6076,6 +6076,249 @@ cmdNetworkAutostart(vshControl *ctl, const vshCmd *cmd) } /* + * "blkdeviotune" command + */ +static const vshCmdInfo info_blkdeviotune[] = { + {"help", N_("Set or query a block device I/O tuning parameters.")}, + {"desc", N_("Set or query disk I/O parameters such as block throttling.")}, + {NULL, NULL} +}; + +static const vshCmdOptDef opts_blkdeviotune[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"device", VSH_OT_DATA, VSH_OFLAG_REQ, N_("block device")}, + {"total_bytes_sec", VSH_OT_INT, VSH_OFLAG_NONE, + N_("total throughput limit in bytes per second")}, + {"read_bytes_sec", VSH_OT_INT, VSH_OFLAG_NONE, + N_("read throughput limit in bytes per second")}, + {"write_bytes_sec", VSH_OT_INT, VSH_OFLAG_NONE, + N_("write throughput limit in bytes per second")}, + {"total_iops_sec", VSH_OT_INT, VSH_OFLAG_NONE, + N_("total I/O operations limit per second")}, + {"read_iops_sec", VSH_OT_INT, VSH_OFLAG_NONE, + N_("read I/O operations limit per second")}, + {"write_iops_sec", VSH_OT_INT, VSH_OFLAG_NONE, + N_("write I/O operations limit per second")}, + {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, + {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, + {NULL, 0, 0, NULL} +}; + +static bool +cmdBlkdeviotune(vshControl *ctl, const vshCmd *cmd) +{ + virDomainPtr dom = NULL; + const char *name, *disk; + unsigned long long total_bytes_sec = 0, read_bytes_sec = 0, write_bytes_sec = 0; + unsigned long long total_iops_sec = 0, read_iops_sec = 0, write_iops_sec = 0; + int nparams = 0; + virTypedParameterPtr params = NULL, temp = NULL; + unsigned int flags = 0, i = 0; + int rv = 0; + int current = vshCommandOptBool(cmd, "current"); + int config = vshCommandOptBool(cmd, "config"); + int live = vshCommandOptBool(cmd, "live"); + bool ret = false; + + if (current) { + if (live || config) { + vshError(ctl, "%s", _("--current must be specified exclusively")); + return false; + } + flags = VIR_DOMAIN_AFFECT_CURRENT; + } else { + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + } + + if (!vshConnectionUsability(ctl, ctl->conn)) + goto cleanup; + + if (!(dom = vshCommandOptDomain(ctl, cmd, &name))) + goto cleanup; + + if (vshCommandOptString(cmd, "device", &disk) < 0) + goto cleanup; + + if ((rv = vshCommandOptULongLong(cmd, "total_bytes_sec", &total_bytes_sec)) < 0) { + vshError(ctl, "%s", + _("Unable to parse integer parameter")); + goto cleanup; + } else if (rv > 0) { + nparams++; + } + + if ((rv = vshCommandOptULongLong(cmd, "read_bytes_sec", &read_bytes_sec)) < 0) { + vshError(ctl, "%s", + _("Unable to parse integer parameter")); + goto cleanup; + } else if (rv > 0) { + nparams++; + } + + if ((rv = vshCommandOptULongLong(cmd, "write_bytes_sec", &write_bytes_sec)) < 0) { + vshError(ctl, "%s", + _("Unable to parse integer parameter")); + goto cleanup; + } else if (rv > 0) { + nparams++; + } + + if ((rv = vshCommandOptULongLong(cmd, "total_iops_sec", &total_iops_sec)) < 0) { + vshError(ctl, "%s", + _("Unable to parse integer parameter")); + goto cleanup; + } else if (rv > 0) { + nparams++; + } + + if ((rv = vshCommandOptULongLong(cmd, "read_iops_sec", &read_iops_sec)) < 0) { + vshError(ctl, "%s", + _("Unable to parse integer parameter")); + goto cleanup; + } else if (rv > 0) { + nparams++; + } + + if ((rv = vshCommandOptULongLong(cmd, "write_iops_sec", &write_iops_sec)) < 0) { + vshError(ctl, "%s", + _("Unable to parse integer parameter")); + goto cleanup; + } else if (rv > 0) { + nparams++; + } + + if (nparams == 0) { + + if ((virDomainGetBlockIoTune(dom, NULL, NULL, &nparams, flags)) != 0) { + vshError(ctl, "%s", + _("Unable to get number of block I/O throttle parameters")); + goto cleanup; + } + + if (nparams == 0) { + ret = true; + goto cleanup; + } + + params = vshCalloc(ctl, nparams, sizeof(*params)); + + if ((virDomainGetBlockIoTune(dom, disk, params, &nparams, flags)) != 0) { + vshError(ctl, "%s", + _("Unable to get block I/O throttle parameters")); + goto cleanup; + } + + for (i = 0; i < nparams; i++) { + switch(params[i].type) { + case VIR_TYPED_PARAM_INT: + vshPrint(ctl, "%-15s: %d\n", params[i].field, + params[i].value.i); + break; + case VIR_TYPED_PARAM_UINT: + vshPrint(ctl, "%-15s: %u\n", params[i].field, + params[i].value.ui); + break; + case VIR_TYPED_PARAM_LLONG: + vshPrint(ctl, "%-15s: %lld\n", params[i].field, + params[i].value.l); + break; + case VIR_TYPED_PARAM_ULLONG: + vshPrint(ctl, "%-15s: %llu\n", params[i].field, + params[i].value.ul); + break; + case VIR_TYPED_PARAM_DOUBLE: + vshPrint(ctl, "%-15s: %f\n", params[i].field, + params[i].value.d); + break; + case VIR_TYPED_PARAM_BOOLEAN: + vshPrint(ctl, "%-15s: %d\n", params[i].field, + params[i].value.b); + break; + default: + vshPrint(ctl, "unimplemented block I/O throttle parameter type\n"); + } + } + + virDomainFree(dom); + return true; + } else { + /* Set the block I/O throttle, match by opt since parameters can be 0 */ + params = vshCalloc(ctl, nparams, sizeof(*params)); + i = 0; + + if ((i < nparams) && (vshCommandOptBool(cmd, "total_bytes_sec"))) { + temp = ¶ms[i]; + temp->type = VIR_TYPED_PARAM_ULLONG; + strncpy(temp->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_BYTES_SEC, + sizeof(temp->field)); + temp->value.ul = total_bytes_sec; + i++; + } + + if ((i < nparams) && (vshCommandOptBool(cmd, "read_bytes_sec"))) { + temp = ¶ms[i]; + temp->type = VIR_TYPED_PARAM_ULLONG; + strncpy(temp->field, VIR_DOMAIN_BLOCK_IOTUNE_READ_BYTES_SEC, + sizeof(temp->field)); + temp->value.ul = read_bytes_sec; + i++; + } + + if ((i < nparams) && (vshCommandOptBool(cmd, "write_bytes_sec"))) { + temp = ¶ms[i]; + temp->type = VIR_TYPED_PARAM_ULLONG; + strncpy(temp->field, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_BYTES_SEC, + sizeof(temp->field)); + temp->value.ul = write_bytes_sec; + i++; + } + + if ((i < nparams) && (vshCommandOptBool(cmd, "total_iops_sec"))) { + temp = ¶ms[i]; + temp->type = VIR_TYPED_PARAM_ULLONG; + strncpy(temp->field, VIR_DOMAIN_BLOCK_IOTUNE_TOTAL_IOPS_SEC, + sizeof(temp->field)); + temp->value.ul = total_iops_sec; + i++; + } + + if ((i < nparams) && (vshCommandOptBool(cmd, "read_iops_sec"))) { + temp = ¶ms[i]; + temp->type = VIR_TYPED_PARAM_ULLONG; + strncpy(temp->field, VIR_DOMAIN_BLOCK_IOTUNE_READ_IOPS_SEC, + sizeof(temp->field)); + temp->value.ul = read_iops_sec; + i++; + } + + if ((i < nparams) && (vshCommandOptBool(cmd, "write_iops_sec"))) { + temp = ¶ms[i]; + temp->type = VIR_TYPED_PARAM_ULLONG; + strncpy(temp->field, VIR_DOMAIN_BLOCK_IOTUNE_WRITE_IOPS_SEC, + sizeof(temp->field)); + temp->value.ul = write_iops_sec; + } + + if ((virDomainSetBlockIoTune(dom, disk, params, nparams, flags)) < 0) { + vshError(ctl, "%s", + _("Unable to change block I/O throttle")); + goto cleanup; + } + } + + ret = true; + +cleanup: + VIR_FREE(params); + virDomainFree(dom); + return ret; +} + +/* * "net-create" command */ static const vshCmdInfo info_network_create[] = { @@ -14689,6 +14932,7 @@ static const vshCmdDef domManagementCmds[] = { {"attach-interface", cmdAttachInterface, opts_attach_interface, info_attach_interface, 0}, {"autostart", cmdAutostart, opts_autostart, info_autostart, 0}, + {"blkdeviotune", cmdBlkdeviotune, opts_blkdeviotune, info_blkdeviotune, 0}, {"blkiotune", cmdBlkiotune, opts_blkiotune, info_blkiotune, 0}, {"blockpull", cmdBlockPull, opts_block_pull, info_block_pull, 0}, {"blockjob", cmdBlockJob, opts_block_job, info_block_job, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index db872dd..8737cac 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -572,6 +572,36 @@ operation can be checked with B<blockjob>. I<path> specifies fully-qualified path of the disk. I<bandwidth> specifies copying bandwidth limit in Mbps. +=item B<blkdeviotune> I<domain> I<device> +[[I<--config>] [I<--live>] | [I<--current>]] +[[I<total_bytes_sec>] | [I<read_bytes_sec>] [I<write_bytes_sec>]] +[[I<total_iops_sec>] | [I<read_iops_sec>] [I<write_iops_sec>]] + +Set or query the block disk io parameters for a block device of I<domain>. +I<device> specifies a unique target name (<target dev='name'/>) or source +file (<source file='name'/>) for one of the disk devices attached to +I<domain> (see also B<domblklist> for listing these names). + +If no limit is specified, it will query current I/O limits setting. +Otherwise, alter the limits with these flags: +I<--total_bytes_sec> specifies total throughput limit in bytes per second. +I<--read_bytes_sec> specifies read throughput limit in bytes per second. +I<--write_bytes_sec> specifies write throughput limit in bytes per second. +I<--total_iops_sec> specifies total I/O operations limit per second. +I<--read_iops_sec> specifies read I/O operations limit per second. +I<--write_iops_sec> specifies write I/O operations limit per second. + +When setting any value, all remaining values are reset to unlimited, +an explicit 0 also clears any limit. A non-zero value for a given total +cannot be mixed with non-zero values for read or write. + +If I<--live> is specified, affect a running guest. +If I<--config> is specified, affect the next boot of a persistent guest. +If I<--current> is specified, affect the current guest state. +Both I<--live> and I<--current> flags may be given, but I<--current> is +exclusive. If no flag is specified, behavior is different depending +on hypervisor. + =item B<blockjob> I<domain> I<path> [I<--abort>] [I<--info>] [I<bandwidth>] Manage active block operations. -- 1.7.7.3

From: Lei Li <lilei@linux.vnet.ibm.com> Python support for both setting and getting block I/O throttle. Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- python/generator.py | 5 +- python/libvirt-override-api.xml | 16 ++++ python/libvirt-override.c | 178 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 196 insertions(+), 3 deletions(-) diff --git a/python/generator.py b/python/generator.py index 00bd94f..88c52b9 100755 --- a/python/generator.py +++ b/python/generator.py @@ -414,6 +414,8 @@ skip_impl = ( 'virDomainGetBlockJobInfo', 'virDomainMigrateGetMaxSpeed', 'virDomainBlockStatsFlags', + 'virDomainSetBlockIoTune', + 'virDomainGetBlockIoTune', ) qemu_skip_impl = ( @@ -470,9 +472,6 @@ skip_function = ( "virNWFilterGetConnect", "virStoragePoolGetConnect", "virStorageVolGetConnect", - - "virDomainGetBlockIoTune", # not implemented yet - "virDomainSetBlockIoTune", # not implemented yet ) qemu_skip_function = ( diff --git a/python/libvirt-override-api.xml b/python/libvirt-override-api.xml index ef02f34..6aad49c 100644 --- a/python/libvirt-override-api.xml +++ b/python/libvirt-override-api.xml @@ -375,5 +375,21 @@ <arg name='flags' type='unsigned int' info='flags, currently unused, pass 0.'/> <return type='unsigned long' info='current max migration speed, or None in case of error'/> </function> + <function name='virDomainSetBlockIoTune' file='python'> + <info>Change the I/O tunables for a block device</info> + <arg name='dom' type='virDomainPtr' info='pointer to the domain'/> + <arg name='disk' type='const char *' info='disk name'/> + <arg name='params' type='virTypedParameterPtr' info='Pointer to blkio tuning params object'/> + <arg name='flags' type='unsigned int' info='an OR'ed set of virDomainModificationImpact'/> + <return type='int' info='0 in case of success, -1 in case of failure'/> + </function> + <function name='virDomainGetBlockIoTune' file='python'> + <info>Get the I/O tunables for a block device</info> + <arg name='dom' type='virDomainPtr' info='pointer to the domain'/> + <arg name='disk' type='const char *' info='disk name'/> + <arg name='params' type='virTypedParameterPtr' info='Pointer to blkio tuning params object'/> + <arg name='flags' type='unsigned int' info='an OR'ed set of virDomainModificationImpact'/> + <return type='int' info='0 in case of success, -1 in case of failure'/> + </function> </symbols> </api> diff --git a/python/libvirt-override.c b/python/libvirt-override.c index 1759bae..cfb9a31 100644 --- a/python/libvirt-override.c +++ b/python/libvirt-override.c @@ -3195,6 +3195,182 @@ LIBVIRT_END_ALLOW_THREADS; return ret; } +static PyObject * +libvirt_virDomainSetBlockIoTune(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) +{ + virDomainPtr domain; + PyObject *pyobj_domain, *pyinfo; + const char *disk; + unsigned int flags; + virTypedParameterPtr params; + int nparams = 0, i; + int c_ret; + + if (!PyArg_ParseTuple(args, (char *)"Ozi:virDomainSetBlockIoTune", + &pyobj_domain, &disk, &pyinfo, &flags)) + return(NULL); + domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_ret = virDomainGetBlockIoTune(domain, disk, NULL, &nparams, flags); + LIBVIRT_END_ALLOW_THREADS; + + if (c_ret < 0) + return VIR_PY_INT_FAIL; + + if ((params = malloc(sizeof(*params)*nparams)) == NULL) + return VIR_PY_INT_FAIL; + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_ret = virDomainGetBlockIoTune(domain, disk, params, &nparams, flags); + LIBVIRT_END_ALLOW_THREADS; + + if (c_ret < 0) { + free(params); + return VIR_PY_INT_FAIL; + } + + /* convert to a Python tuple of long objects */ + for (i = 0; i < nparams; i++) { + PyObject *key, *val; + key = libvirt_constcharPtrWrap(params[i].field); + val = PyDict_GetItem(pyinfo, key); + Py_DECREF(key); + + if (val == NULL) + continue; + + switch (params[i].type) { + case VIR_TYPED_PARAM_INT: + params[i].value.i = (int)PyInt_AS_LONG(val); + break; + + case VIR_TYPED_PARAM_UINT: + params[i].value.ui = (unsigned int)PyInt_AS_LONG(val); + break; + + case VIR_TYPED_PARAM_LLONG: + params[i].value.l = (long long)PyLong_AsLongLong(val); + break; + + case VIR_TYPED_PARAM_ULLONG: + params[i].value.ul = (unsigned long long)PyLong_AsLongLong(val); + break; + + case VIR_TYPED_PARAM_DOUBLE: + params[i].value.d = (double)PyFloat_AsDouble(val); + break; + + case VIR_TYPED_PARAM_BOOLEAN: + { + PyObject *hacktrue = PyBool_FromLong(1); + params[i].value.b = hacktrue == val ? 1: 0; + Py_DECREF(hacktrue); + } + break; + + default: + free(params); + return VIR_PY_INT_FAIL; + } + } + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_ret = virDomainSetBlockIoTune(domain, disk, params, nparams, flags); + LIBVIRT_END_ALLOW_THREADS; + + if (c_ret < 0) { + free(params); + return VIR_PY_INT_FAIL; + } + + free(params); + return VIR_PY_INT_SUCCESS; +} + +static PyObject * +libvirt_virDomainGetBlockIoTune(PyObject *self ATTRIBUTE_UNUSED, + PyObject *args) +{ + virDomainPtr domain; + PyObject *pyobj_domain, *pyreply; + const char *disk; + int nparams = 0, i; + unsigned int flags; + virTypedParameterPtr params; + int c_ret; + + if (!PyArg_ParseTuple(args, (char *)"Oi:virDomainGetBlockIoTune", + &pyobj_domain, &disk, &flags)) + return(NULL); + domain = (virDomainPtr) PyvirDomain_Get(pyobj_domain); + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_ret = virDomainGetBlockIoTune(domain, disk, NULL, &nparams, flags); + LIBVIRT_END_ALLOW_THREADS; + + if (c_ret < 0) + return VIR_PY_NONE; + + if ((params = malloc(sizeof(*params)*nparams)) == NULL) + return VIR_PY_NONE; + + LIBVIRT_BEGIN_ALLOW_THREADS; + c_ret = virDomainGetBlockIoTune(domain, disk, params, &nparams, flags); + LIBVIRT_END_ALLOW_THREADS; + + if (c_ret < 0) { + free(params); + return VIR_PY_NONE; + } + + /* convert to a Python tuple of long objects */ + if ((pyreply = PyDict_New()) == NULL) { + free(params); + return VIR_PY_NONE; + } + for (i = 0 ; i < nparams ; i++) { + PyObject *key, *val; + + switch (params[i].type) { + case VIR_TYPED_PARAM_INT: + val = PyInt_FromLong((long)params[i].value.i); + break; + + case VIR_TYPED_PARAM_UINT: + val = PyInt_FromLong((long)params[i].value.ui); + break; + + case VIR_TYPED_PARAM_LLONG: + val = PyLong_FromLongLong((long long)params[i].value.l); + break; + + case VIR_TYPED_PARAM_ULLONG: + val = PyLong_FromLongLong((unsigned long long)params[i].value.ul); + break; + + case VIR_TYPED_PARAM_DOUBLE: + val = PyFloat_FromDouble((double)params[i].value.d); + break; + + case VIR_TYPED_PARAM_BOOLEAN: + val = PyBool_FromLong((long)params[i].value.b); + break; + + default: + free(params); + Py_DECREF(pyreply); + return VIR_PY_NONE; + } + + key = libvirt_constcharPtrWrap(params[i].field); + PyDict_SetItem(pyreply, key, val); + } + free(params); + return(pyreply); +} + /******************************************* * Helper functions to avoid importing modules * for every callback @@ -4837,6 +5013,8 @@ static PyMethodDef libvirtMethods[] = { {(char *) "virDomainSnapshotListNames", libvirt_virDomainSnapshotListNames, METH_VARARGS, NULL}, {(char *) "virDomainRevertToSnapshot", libvirt_virDomainRevertToSnapshot, METH_VARARGS, NULL}, {(char *) "virDomainGetBlockJobInfo", libvirt_virDomainGetBlockJobInfo, METH_VARARGS, NULL}, + {(char *) "virDomainSetBlockIoTune", libvirt_virDomainSetBlockIoTune, METH_VARARGS, NULL}, + {(char *) "virDomainGetBlockIoTune", libvirt_virDomainGetBlockIoTune, METH_VARARGS, NULL}, {(char *) "virDomainSendKey", libvirt_virDomainSendKey, METH_VARARGS, NULL}, {(char *) "virDomainMigrateGetMaxSpeed", libvirt_virDomainMigrateGetMaxSpeed, METH_VARARGS, NULL}, {NULL, NULL, 0, NULL} -- 1.7.7.3

From: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- .../qemuxml2argv-blkdeviotune.args | 7 ++++ .../qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml | 30 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 + tests/qemuxml2xmltest.c | 1 + 4 files changed, 40 insertions(+), 0 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args new file mode 100644 index 0000000..9615290 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args @@ -0,0 +1,7 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -name QEMUGuest1 -nographic -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-ide0-0-0,cache=off,\ +bps=5000,iops=6000 -device \ +ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -usb \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml new file mode 100644 index 0000000..48553e4 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <iotune> + <total_bytes_sec>5000</total_bytes_sec> + <total_iops_sec>6000</total_iops_sec> + </iotune> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index fe24354..40ce00c 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -645,6 +645,8 @@ mymain(void) DO_TEST("blkiotune", false, QEMU_CAPS_NAME); DO_TEST("cputune", false, QEMU_CAPS_NAME); DO_TEST("numatune-memory", false, NONE); + DO_TEST("blkdeviotune", false, QEMU_CAPS_NAME, QEMU_CAPS_DEVICE, + QEMU_CAPS_DRIVE); DO_TEST("multifunction-pci-device", false, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 3f37520..2e6b5c7 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -191,6 +191,7 @@ mymain(void) DO_TEST("event_idx"); DO_TEST("usb-redir"); + DO_TEST("blkdeviotune"); /* These tests generate different XML */ DO_TEST_DIFFERENT("balloon-device-auto"); -- 1.7.7.3

On Wed, Nov 23, 2011 at 02:44:49PM -0700, Eric Blake wrote:
From: Lei Li <lilei@linux.vnet.ibm.com>
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> Signed-off-by: Eric Blake <eblake@redhat.com> --- .../qemuxml2argv-blkdeviotune.args | 7 ++++ .../qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml | 30 ++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 + tests/qemuxml2xmltest.c | 1 + 4 files changed, 40 insertions(+), 0 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args new file mode 100644 index 0000000..9615290 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args @@ -0,0 +1,7 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test /usr/bin/qemu -S -M \ +pc -m 214 -smp 1 -name QEMUGuest1 -nographic -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-ide0-0-0,cache=off,\ +bps=5000,iops=6000 -device \ +ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -usb \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml new file mode 100644 index 0000000..48553e4 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml @@ -0,0 +1,30 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory>219100</memory> + <currentMemory>219100</currentMemory> + <vcpu>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <iotune> + <total_bytes_sec>5000</total_bytes_sec> + <total_iops_sec>6000</total_iops_sec> + </iotune> + <address type='drive' controller='0' bus='0' unit='0'/> + </disk> + <controller type='ide' index='0'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index fe24354..40ce00c 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -645,6 +645,8 @@ mymain(void) DO_TEST("blkiotune", false, QEMU_CAPS_NAME); DO_TEST("cputune", false, QEMU_CAPS_NAME); DO_TEST("numatune-memory", false, NONE); + DO_TEST("blkdeviotune", false, QEMU_CAPS_NAME, QEMU_CAPS_DEVICE, + QEMU_CAPS_DRIVE);
DO_TEST("multifunction-pci-device", false, QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 3f37520..2e6b5c7 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -191,6 +191,7 @@ mymain(void) DO_TEST("event_idx");
DO_TEST("usb-redir"); + DO_TEST("blkdeviotune");
/* These tests generate different XML */ DO_TEST_DIFFERENT("balloon-device-auto");
ACK, but I would suggest to extend this (or augment the test) to list 2 <disk> with iotune, Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On 11/24/2011 01:31 AM, Daniel Veillard wrote:
DO_TEST("usb-redir"); + DO_TEST("blkdeviotune");
/* These tests generate different XML */ DO_TEST_DIFFERENT("balloon-device-auto");
ACK, but I would suggest to extend this (or augment the test) to list 2 <disk> with iotune,
Good idea, especially since it probably would have let me catch the ctxt handling bug in 3/7. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 11/24/2011 06:37 AM, Eric Blake wrote:
On 11/24/2011 01:31 AM, Daniel Veillard wrote:
DO_TEST("usb-redir"); + DO_TEST("blkdeviotune");
/* These tests generate different XML */ DO_TEST_DIFFERENT("balloon-device-auto");
ACK, but I would suggest to extend this (or augment the test) to list 2 <disk> with iotune,
Good idea, especially since it probably would have let me catch the ctxt handling bug in 3/7.
Yep - squashing this in will expose the flaw in 3/7. diff --git i/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args w/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args index 9615290..314c1ac 100644 --- i/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args +++ w/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args @@ -3,5 +3,8 @@ pc -m 214 -smp 1 -name QEMUGuest1 -nographic -nodefaults \ -monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ -drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-ide0-0-0,cache=off,\ bps=5000,iops=6000 -device \ -ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 -usb \ --device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 +ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-drive file=/dev/HostVG/QEMUGuest2,if=none,id=drive-ide0-0-1,cache=off,\ +bps_rd=5000,bps_wr=5000,iops=7000 -device \ +ide-drive,bus=ide.0,unit=1,drive=drive-ide0-0-1,id=ide0-0-1 \ +-usb -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git i/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml w/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml index 48553e4..7b6ec8f 100644 --- i/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml +++ w/tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml @@ -24,6 +24,17 @@ </iotune> <address type='drive' controller='0' bus='0' unit='0'/> </disk> + <disk type='block' device='disk'> + <driver name='qemu' type='qcow2' cache='none'/> + <source dev='/dev/HostVG/QEMUGuest2'/> + <target dev='hdb' bus='ide'/> + <iotune> + <read_bytes_sec>5000</read_bytes_sec> + <write_bytes_sec>5000</write_bytes_sec> + <total_iops_sec>7000</total_iops_sec> + </iotune> + <address type='drive' controller='0' bus='0' unit='1'/> + </disk> <controller type='ide' index='0'/> <memballoon model='virtio'/> </devices> -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Wed, Nov 23, 2011 at 02:44:42PM -0700, Eric Blake wrote:
Here's the latest state of Lei's patch series with all my comments folded in. I may have a few more tweaks to make now that I'm at the point of testing things with both old qemu (graceful rejection) and new qemu (sensical return values), so it may be a day or two (or even a weekend, since this is a holiday weekend for me) before I actually push this, so it wouldn't hurt if anyone else wants to review in the meantime.
Lei Li (7): Add new API virDomain{Set, Get}BlockIoTune Add virDomain{Set, Get}BlockIoTune support to the remote driver Support block I/O throttle in XML Implement virDomain{Set, Get}BlockIoTune for the qemu driver Enable the blkdeviotune command in virsh Support virDomain{Set, Get}BlockIoTune in the python API Add tests for blkdeviotune
daemon/remote.c | 64 ++++ docs/formatdomain.html.in | 39 +++ docs/schemas/domaincommon.rng | 122 +++++-- include/libvirt/libvirt.h.in | 63 ++++ python/generator.py | 2 + python/libvirt-override-api.xml | 16 + python/libvirt-override.c | 178 ++++++++++ src/conf/domain_conf.c | 90 +++++- src/conf/domain_conf.h | 14 + src/driver.h | 20 ++ src/libvirt.c | 148 +++++++++ src/libvirt_public.syms | 6 + src/qemu/qemu_command.c | 31 ++ src/qemu/qemu_driver.c | 340 ++++++++++++++++++++ src/qemu/qemu_monitor.c | 33 ++ src/qemu/qemu_monitor.h | 8 + src/qemu/qemu_monitor_json.c | 176 ++++++++++ src/qemu/qemu_monitor_json.h | 8 + src/qemu/qemu_monitor_text.c | 151 +++++++++- src/qemu/qemu_monitor_text.h | 8 + src/remote/remote_driver.c | 57 ++++ src/remote/remote_protocol.x | 27 ++- src/remote_protocol-structs | 24 ++ .../qemuxml2argv-blkdeviotune.args | 7 + .../qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml | 30 ++ tests/qemuxml2argvtest.c | 2 + tests/qemuxml2xmltest.c | 1 + tools/virsh.c | 244 ++++++++++++++ tools/virsh.pod | 30 ++ 29 files changed, 1900 insertions(+), 39 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-blkdeviotune.xml
ACK to the serie once the bug on 3/7 is fixed, if possible fix the couple of other small nits :-) thanks ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/

On 11/24/2011 01:32 AM, Daniel Veillard wrote:
On Wed, Nov 23, 2011 at 02:44:42PM -0700, Eric Blake wrote:
Here's the latest state of Lei's patch series with all my comments folded in. I may have a few more tweaks to make now that I'm at the point of testing things with both old qemu (graceful rejection) and new qemu (sensical return values), so it may be a day or two (or even a weekend, since this is a holiday weekend for me) before I actually push this, so it wouldn't hurt if anyone else wants to review in the meantime.
ACK to the serie once the bug on 3/7 is fixed, if possible fix the couple of other small nits :-)
Nits fixed and complete series now pushed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Eric, You're very nice, and thanks a lot in my heart. ;) On Thu, Dec 1, 2011 at 3:03 AM, Eric Blake <eblake@redhat.com> wrote:
On 11/24/2011 01:32 AM, Daniel Veillard wrote:
On Wed, Nov 23, 2011 at 02:44:42PM -0700, Eric Blake wrote:
Here's the latest state of Lei's patch series with all my comments folded in. I may have a few more tweaks to make now that I'm at the point of testing things with both old qemu (graceful rejection) and new qemu (sensical return values), so it may be a day or two (or even a weekend, since this is a holiday weekend for me) before I actually push this, so it wouldn't hurt if anyone else wants to review in the meantime.
ACK to the serie once the bug on 3/7 is fixed, if possible fix the couple of other small nits :-)
Nits fixed and complete series now pushed.
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
-- Regards, Zhi Yong Wu
participants (5)
-
Daniel Veillard
-
Eric Blake
-
Lei Li
-
Lynn
-
Zhi Yong Wu