[libvirt] [PATCH 1/8] Add new API virDomainSetBlockIoThrottle
Lei Li
lilei at linux.vnet.ibm.com
Fri Oct 28 09:00:49 UTC 2011
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 at linux.vnet.ibm.com>
>> Signed-off-by: Lei Li<lilei at linux.vnet.ibm.com>
>> ---
>> 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
More information about the libvir-list
mailing list