[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [RFC PATCH 1/5] Add new API virDomainBlockIoThrottle



On Tue, Oct 11, 2011 at 10:59 PM, Adam Litke <agl us ibm com> 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 linux vnet ibm com>
>> ---
>>  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 us ibm com>
> IBM Linux Technology Center
>
> --
> libvir-list mailing list
> libvir-list redhat com
> https://www.redhat.com/mailman/listinfo/libvir-list
>



-- 
Regards,

Zhi Yong Wu


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]