[libvirt] [PATCHv6 3/7] Support block I/O throttle in XML
Lynn
matrixs.zero at gmail.com
Fri Nov 25 09:45:40 UTC 2011
On 11/24/2011 11:01 PM, Eric Blake wrote:
> 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 at 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 at linux.vnet.ibm.com>
>>> Signed-off-by: Zhi Yong Wu<wuzhy at linux.vnet.ibm.com>
>>> Signed-off-by: Eric Blake<eblake at 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.
>
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 at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
Lynn
More information about the libvir-list
mailing list