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

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

to represent the throttling inside the <disk> tag rather than alongside the <blkiotune> settings.

> Signed-off-by: Zhi Yong Wu <wuzhy(a)linux.vnet.ibm.com&gt; > --- >  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(a)us.ibm.com&gt; IBM Linux Technology Center -- libvir-list mailing list libvir-list(a)redhat.com https://www.redhat.com/mailman/listinfo/libvir-list