On 10/28/2011 04:39 AM, Adam Litke wrote:
On Thu, Oct 27, 2011 at 05:20:03PM +0800, Lei HH Li wrote:This patch add new pulic API virDomainSetBlockIoThrottle to src/libvirt.c enable iotune setting for a device in domain XML.You should include the API definition for both virDomainSetBlockIoThrottle and virDomainGetBlockIoThrottle in the same patch. That way, reviewers can see the complete picture of all API changes in one place.
I split it since the feature 'block I/O setting was already reviewed at RFC V1',
and 'set' was implemented by supporting qmp command 'block_set_io_throttle'.
'get' was implemented by supporting qmp command 'query-block'.
I thought It will be much clearer to review patches that have been split.
OK, I will merge the Get method into this.
Signed-off-by: Zhi Yong Wu<wuzhy@xxxxxxxxxxxxxxxxxx> Signed-off-by: Lei Li<lilei@xxxxxxxxxxxxxxxxxx> --- include/libvirt/libvirt.h.in | 17 +++++++++ src/conf/domain_conf.c | 76 ++++++++++++++++++++++++++++++++++++++++++ src/conf/domain_conf.h | 11 ++++++ src/driver.h | 11 ++++++ src/libvirt.c | 62 ++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 1 + src/util/xml.h | 2 + 7 files changed, 180 insertions(+), 0 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 7102bce..ff2926e 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1575,6 +1575,23 @@ int virDomainBlockJobSetSpeed(virDomainPtr dom, const char *path, int virDomainBlockPull(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags); +typedef struct _virDomainBlockIoThrottleInfo virDomainBlockIoThrottleInfo; +struct _virDomainBlockIoThrottleInfo { + unsigned long long bps; + unsigned long long bps_rd; + unsigned long long bps_wr; + unsigned long long iops; + unsigned long long iops_rd; + unsigned long long iops_wr;In the last series, it was suggested that you change the names of these tuning parameters to make them more self-explanatory. I suggested: "bps" => "total_bytes_sec" : total throughput limit in bytes per second "bps_rd" => "read_bytes_sec" : read throughput limit in bytes per second "bps_wr" => "write_bytes_sec" : read throughput limit in bytes per second "iops" => "total_iops_sec" : total I/O operations per second "iops_rd" => "read_iops_sec" : read I/O operations per second "iops_wr" => "write_iops_sec" : write I/O operations per second This applies to the variable names in the above structure and to the attributes in the XML schema.+}; +typedef virDomainBlockIoThrottleInfo *virDomainBlockIoThrottleInfoPtr; + +int +virDomainSetBlockIoThrottle(virDomainPtr dom, + const char *disk, + virDomainBlockIoThrottleInfoPtr info, + unsigned int flags); + /* * NUMA support diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 7f9c542..8f1c65f 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -2444,6 +2444,41 @@ virDomainDiskDefParseXML(virCapsPtr caps, iotag = virXMLPropString(cur, "io"); ioeventfd = virXMLPropString(cur, "ioeventfd"); event_idx = virXMLPropString(cur, "event_idx"); + } else if (xmlStrEqual(cur->name, BAD_CAST "iotune")) { + char *io_throttle = NULL; + io_throttle = virXMLPropString(cur, "bps"); + if (io_throttle) { + def->blkiothrottle.bps = strtoull(io_throttle, NULL, 10); + VIR_FREE(io_throttle); + } + + io_throttle = virXMLPropString(cur, "bps_rd"); + if (io_throttle) { + def->blkiothrottle.bps_rd = strtoull(io_throttle, NULL, 10); + VIR_FREE(io_throttle); + } + + io_throttle = virXMLPropString(cur, "bps_wr"); + if (io_throttle) { + def->blkiothrottle.bps_wr = strtoull(io_throttle, NULL, 10); + VIR_FREE(io_throttle); + } + io_throttle = virXMLPropString(cur, "iops"); + if (io_throttle) { + def->blkiothrottle.iops = strtoull(io_throttle, NULL, 10); + VIR_FREE(io_throttle); + } + + io_throttle = virXMLPropString(cur, "iops_rd"); + if (io_throttle) { + def->blkiothrottle.iops_rd = strtoull(io_throttle, NULL, 10); + VIR_FREE(io_throttle); + } + + io_throttle = virXMLPropString(cur, "iops_wr"); + if (io_throttle) { + def->blkiothrottle.iops_wr = strtoull(io_throttle, NULL, 10); + }With so many attributes (bps, bps_rd, etc), I wonder if this schema should be changed to the following: <iotune> <total_bytes_sec>10000</total_bytes_sec> <total_iops_sec>1000</total_iops_sec> </iotune> Anyone else with more experience (or strong opinions) care to chime in here?} else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) { def->readonly = 1; } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) { @@ -9317,6 +9352,47 @@ virDomainDiskDefFormat(virBufferPtr buf, virBufferAsprintf(buf, "<target dev='%s' bus='%s'/>\n", def->dst, bus); + /*disk I/O throttling*/ + if (def->blkiothrottle.bps + || def->blkiothrottle.bps_rd + || def->blkiothrottle.bps_wr + || def->blkiothrottle.iops + || def->blkiothrottle.iops_rd + || def->blkiothrottle.iops_wr) {Ick. But you said in the summary that you will be fixing this in a future post. Please :)
Yes, I said in the summary that I will fix it when implement --current --live --config options support.
+ virBufferAsprintf(buf, "<iotune"); + if (def->blkiothrottle.bps) { + virBufferAsprintf(buf, " bps='%llu'", + def->blkiothrottle.bps); + } + + if (def->blkiothrottle.bps_rd) { + virBufferAsprintf(buf, " bps_rd='%llu'", + def->blkiothrottle.bps_rd); + } + + if (def->blkiothrottle.bps_wr) { + virBufferAsprintf(buf, " bps_wr='%llu'", + def->blkiothrottle.bps_wr); + } + + if (def->blkiothrottle.iops) { + virBufferAsprintf(buf, " iops='%llu'", + def->blkiothrottle.iops); + } + + if (def->blkiothrottle.iops_rd) { + virBufferAsprintf(buf, " iops_rd='%llu'", + def->blkiothrottle.iops_rd); + } + + if (def->blkiothrottle.iops_wr) { + virBufferAsprintf(buf, " iops_wr='%llu'", + def->blkiothrottle.iops_wr); + } + + virBufferAsprintf(buf, "/>\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 0b4d2c2..55c7493 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -292,6 +292,17 @@ struct _virDomainDiskDef { virDomainDiskHostDefPtr hosts; char *driverName; char *driverType; + + /*disk I/O throttling*/ + struct { + unsigned long long bps; + unsigned long long bps_rd; + unsigned long long bps_wr; + unsigned long long iops; + unsigned long long iops_rd; + unsigned long long iops_wr; + } blkiothrottle; + char *serial; int cachemode; int error_policy; /* enum virDomainDiskErrorPolicy */ diff --git a/src/driver.h b/src/driver.h index b899d0e..ffa71c8 100644 --- a/src/driver.h +++ b/src/driver.h @@ -735,6 +735,16 @@ typedef int (*virDrvDomainBlockPull)(virDomainPtr dom, const char *path, unsigned long bandwidth, unsigned int flags); +/* + * Block I/O throttling support + */ + +typedef int + (*virDrvDomainSetBlockIoThrottle)(virDomainPtr dom, + const char *disk, + virDomainBlockIoThrottleInfoPtr info, + unsigned int flags); + /** * _virDriver: @@ -893,6 +903,7 @@ struct _virDriver { virDrvDomainGetBlockJobInfo domainGetBlockJobInfo; virDrvDomainBlockJobSetSpeed domainBlockJobSetSpeed; virDrvDomainBlockPull domainBlockPull; + virDrvDomainSetBlockIoThrottle domainSetBlockIoThrottle; }; typedef int diff --git a/src/libvirt.c b/src/libvirt.c index a6bcee6..dfc74fb 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -16964,3 +16964,65 @@ error: virDispatchError(dom->conn); return -1; } + +/** + * virDomainSetBlockIoThrottle: + * @dom: pointer to domain object + * @disk: Fully-qualified disk name + * @info: specify block I/O limits in bytes + * @flags: indicate if it set or display block I/O limits info + * + * This function is mainly to enable Block I/O throttling function in libvirt. + * It is used to change the block I/O throttling setting for specified domain. + * + * Returns 0 if the operation has started, -1 on failure. + */ +int virDomainSetBlockIoThrottle(virDomainPtr dom, + const char *disk, + virDomainBlockIoThrottleInfoPtr info, + unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(dom, "disk=%p, info=%p, flags=%x", + disk, info, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN (dom)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + conn = dom->conn; + + if (dom->conn->flags& VIR_CONNECT_RO) { + virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + + if (!disk) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + + if (!info) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + + if (conn->driver->domainSetBlockIoThrottle) { + int ret; + ret = conn->driver->domainSetBlockIoThrottle(dom, disk, info, 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 9762fc4..ce29978 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -495,6 +495,7 @@ LIBVIRT_0.9.7 { virDomainSnapshotGetParent; virDomainSnapshotListChildrenNames; virDomainSnapshotNumChildren; + virDomainSetBlockIoThrottle; } LIBVIRT_0.9.5; # .... define new API here using predicted next version number .... diff --git a/src/util/xml.h b/src/util/xml.h index d30e066..cca3ea0 100644 --- a/src/util/xml.h +++ b/src/util/xml.h @@ -50,6 +50,8 @@ xmlNodePtr virXPathNode(const char *xpath, int virXPathNodeSet(const char *xpath, xmlXPathContextPtr ctxt, xmlNodePtr **list); +int virXMLStringToULongLong (const char *stringval, + unsigned long long *value); char * virXMLPropString(xmlNodePtr node, const char *name); -- 1.7.1
-- Lei -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list