On Tue, Oct 11, 2011 at 10:59 PM, Adam Litke <agl@xxxxxxxxxx> wrote:
> On Mon, Oct 10, 2011 at 09:45:09PM +0800, Lei HH Li wrote:
>
> Hi Lei. You are missing a patch summary at the top of this email. In your
> summary you want to let reviewers know what the patch is doing. For example,
> this patch defines the new virDomainBlockIoThrottle API and specifies the XML
> schema. Also at the top of the patch you have an opportunity to explain why you
> made a particular design decision. For example, you could explain why you chose
I think so:). We should explain why we create one new libvirt
commands, not extending blkiotune.
BTW: Can we CCed these patches to those related developers to get
their comments? (e.g, Daniel, Gui JianFeng, etc)
> to represent the throttling inside the <disk> tag rather than alongside the
> <blkiotune> settings.
>
>> Signed-off-by: Zhi Yong Wu <wuzhy@xxxxxxxxxxxxxxxxxx>
>> ---
>> include/libvirt/libvirt.h.in | 22 ++++++++++++
>> src/conf/domain_conf.c | 77 ++++++++++++++++++++++++++++++++++++++++++
>> src/conf/domain_conf.h | 11 ++++++
>> src/driver.h | 11 ++++++
>> src/libvirt.c | 66 ++++++++++++++++++++++++++++++++++++
>> src/libvirt_public.syms | 1 +
>> src/util/xml.h | 3 ++
>> 7 files changed, 191 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in
>> index 07617be..f7b892d 100644
>> --- a/include/libvirt/libvirt.h.in
>> +++ b/include/libvirt/libvirt.h.in
>> @@ -1573,6 +1573,28 @@ int virDomainBlockJobSetSpeed(virDomainPtr dom, const char *path,
>> int virDomainBlockPull(virDomainPtr dom, const char *path,
>> unsigned long bandwidth, unsigned int flags);
>>
>> +/*
>> + * Block I/O throttling support
>> + */
>> +
>> +typedef unsigned long long virDomainBlockIoThrottleUnits;
>> +
>> +typedef struct _virDomainBlockIoThrottleInfo virDomainBlockIoThrottleInfo;
>> +struct _virDomainBlockIoThrottleInfo {
>> + virDomainBlockIoThrottleUnits bps;
>> + virDomainBlockIoThrottleUnits bps_rd;
>> + virDomainBlockIoThrottleUnits bps_wr;
>> + virDomainBlockIoThrottleUnits iops;
>> + virDomainBlockIoThrottleUnits iops_rd;
>> + virDomainBlockIoThrottleUnits iops_wr;
>> +};
>> +typedef virDomainBlockIoThrottleInfo *virDomainBlockIoThrottleInfoPtr;
>
> I don't think it is necessary to use a typedef for the unsigned long long values
> in the virDomainBlockIoThrottleInfo structure. Just use unsigned long long
> directly.
>
> You might also want to consider using virTypedParameter's for this structure.
> It would allow us to add additional fields in the future.
>
>> +
>> +int virDomainBlockIoThrottle(virDomainPtr dom,
>
> The libvirt project style is to place the function return value on its own line:
>
> int
> virDomainBlockIoThrottle(virDomainPtr dom,
> ...
>
>> + const char *disk,
>> + virDomainBlockIoThrottleInfoPtr info,
>> + virDomainBlockIoThrottleInfoPtr reply,
>> + unsigned int flags);
>>
>> /*
>> * NUMA support
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 944cfa9..d0ba07e 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -2422,6 +2422,42 @@ virDomainDiskDefParseXML(virCapsPtr caps,
>> iotag = virXMLPropString(cur, "io");
>> ioeventfd = virXMLPropString(cur, "ioeventfd");
>> event_idx = virXMLPropString(cur, "event_idx");
>> + } else if (xmlStrEqual(cur->name, BAD_CAST "iothrottle")) {
>> + 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);
>> + }
>> } else if (xmlStrEqual(cur->name, BAD_CAST "readonly")) {
>> def->readonly = 1;
>> } else if (xmlStrEqual(cur->name, BAD_CAST "shareable")) {
>> @@ -9249,6 +9285,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) {
>> + virBufferAsprintf(buf, " <iothrottle");
>> + 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 e07fd2f..b60a6de 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -283,6 +283,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 f85a1b1..741e196 100644
>> --- a/src/driver.h
>> +++ b/src/driver.h
>> @@ -725,6 +725,16 @@ typedef int
>> (*virDrvDomainBlockPull)(virDomainPtr dom, const char *path,
>> unsigned long bandwidth, unsigned int flags);
>>
>> +/*
>> + * Block I/O throttling support
>> + */
>> +
>> +typedef int
>> + (*virDrvDomainBlockIoThrottle)(virDomainPtr dom,
>> + const char *disk,
>> + virDomainBlockIoThrottleInfoPtr info,
>> + virDomainBlockIoThrottleInfoPtr reply,
>> + unsigned int flags);
>>
>> /**
>> * _virDriver:
>> @@ -881,6 +891,7 @@ struct _virDriver {
>> virDrvDomainGetBlockJobInfo domainGetBlockJobInfo;
>> virDrvDomainBlockJobSetSpeed domainBlockJobSetSpeed;
>> virDrvDomainBlockPull domainBlockPull;
>> + virDrvDomainBlockIoThrottle domainBlockIoThrottle;
>> };
>>
>> typedef int
>> diff --git a/src/libvirt.c b/src/libvirt.c
>> index 182e031..5f4514c 100644
>> --- a/src/libvirt.c
>> +++ b/src/libvirt.c
>> @@ -16706,3 +16706,69 @@ error:
>> virDispatchError(dom->conn);
>> return -1;
>> }
>> +
>> +/**
>> + * virDomainBlockIoThrottle:
>> + * @dom: pointer to domain object
>> + * @disk: Fully-qualified disk name
>> + * @info: specify block I/O limits in bytes
>> + * @reply: store block I/O limits setting
>
> This really needs to be split into two separate functions:
>
> virDomainSetBlockIoThrottle - Set block I/O limits for a device
> - Requires a connection in 'write' mode.
> - Limits (info) structure passed as an input parameter
> virDomainGetBlockIoThrottle - Get the current block I/O limits for a device
> - Works on a read-only connection.
> - Current limits are written to the output parameter (info)
>
> Consider using virTypedParameter to allow future extension:
>
> int
> virDomainSetBlockIoThrottle(virDomainPtr dom,
> const char *disk,
> virTypedParameterPtr params,
> int *nparams, unsigned int flags);
>
> int
> virDomainGetBlockIoThrottle(virDomainPtr dom,
> const char *disk,
> virTypedParameterPtr params,
> int *nparams, unsigned int flags);
>
> You can use the API virDomainGetBlkioParameters as an example for all of this.
>
>> + * @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 set or display block I/O throttling setting for specified domain.
>> + *
>> + * Returns 0 if the operation has started, -1 on failure.
>> + */
>> +int virDomainBlockIoThrottle(virDomainPtr dom,
>> + const char *disk,
>> + virDomainBlockIoThrottleInfoPtr info,
>> + virDomainBlockIoThrottleInfoPtr reply,
>> + unsigned int flags)
>> +{
>> + virConnectPtr conn;
>> +
>> + VIR_DOMAIN_DEBUG(dom, "disk=%p, info=%p, reply=%p,flags=%x",
>> + disk, info, reply, 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,
>> + _("disk name is NULL"));
>
> We are using a newer error convention now. This should be:
> virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__);
> Be sure to fix all of these in your patch.
>
>> + goto error;
>> + }
>> +
>> + if (!reply) {
>> + virLibDomainError(VIR_ERR_INVALID_ARG,
>> + _("reply is NULL"));
>> + goto error;
>> + }
>> +
>> + if (conn->driver->domainBlockIoThrottle) {
>> + int ret;
>> + ret = conn->driver->domainBlockIoThrottle(dom, disk, info, reply, 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 afea29b..0a79167 100644
>> --- a/src/libvirt_public.syms
>> +++ b/src/libvirt_public.syms
>> @@ -493,6 +493,7 @@ LIBVIRT_0.9.7 {
>> global:
>> virDomainReset;
>> virDomainSnapshotGetParent;
>> + virDomainBlockIoThrottle;
>> } 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..14b35b2 100644
>> --- a/src/util/xml.h
>> +++ b/src/util/xml.h
>> @@ -50,6 +50,9 @@ 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
>
--
Regards,
Zhi Yong Wu
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list