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.
>
> 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 :)
> + 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
>
--
Adam Litke <agl@xxxxxxxxxx>
IBM Linux Technology Center
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list