Re: [libvirt] [RFC PATCH 1/5] Add new API virDomainBlockIoThrottle
On Wed, Oct 12, 2011 at 03:49:26PM -0600, Eric Blake wrote: On 10/12/2011 01:07 AM, Zhi Yong Wu wrote: On Tue, Oct 11, 2011 at 10:59 PM, Adam Litkea...@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 thedisk tag rather than alongside the blkiotune settings. I think the answer to this question lies in how it will be used. Looking at your patch, right now, we have: domain blkiotune weightnnn/weight /blkiotune /domain which is global to the domain, but blkio throttling is specified per-disk and can vary across multiple disks. So mention in your commit message that you are proposing a per-disk element, which is why it does not fit into the existing blkiotune: domain devices disk iothrottle .../ /disk /devices /domain Also, we need to agree on the final xml layout chosen - with 6 parameters, and the possibility of extension, while your patch did it all as attributes: iothrottle bps='nnn' bps_rd='nnn' .../ I'm thinking it would be better to use sub-elements (as was done with blkiotune/weight): iothrottle bpsnnn/bps bps_rdnnn/bps /iothrottle Also, is that naming the best? While qemu's command line might be terse, the XML should be something that reads well and which might even be portable to other hypervisors, so longer names might be more appropriate. Yes, good point Eric. I would also prefer to see these tags be expanded to more meaningful names. I propose: 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 -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- Adam Litke a...@us.ibm.com IBM Linux Technology Center -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [RFC PATCH 1/5] Add new API virDomainBlockIoThrottle
On Tue, Oct 11, 2011 at 10:59 PM, Adam Litke a...@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 wu...@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) {
+
Re: [libvirt] [RFC PATCH 1/5] Add new API virDomainBlockIoThrottle
On 10/12/2011 01:07 AM, Zhi Yong Wu wrote: On Tue, Oct 11, 2011 at 10:59 PM, Adam Litkea...@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 thedisk tag rather than alongside the blkiotune settings. I think the answer to this question lies in how it will be used. Looking at your patch, right now, we have: domain blkiotune weightnnn/weight /blkiotune /domain which is global to the domain, but blkio throttling is specified per-disk and can vary across multiple disks. So mention in your commit message that you are proposing a per-disk element, which is why it does not fit into the existing blkiotune: domain devices disk iothrottle .../ /disk /devices /domain Also, we need to agree on the final xml layout chosen - with 6 parameters, and the possibility of extension, while your patch did it all as attributes: iothrottle bps='nnn' bps_rd='nnn' .../ I'm thinking it would be better to use sub-elements (as was done with blkiotune/weight): iothrottle bpsnnn/bps bps_rdnnn/bps /iothrottle Also, is that naming the best? While qemu's command line might be terse, the XML should be something that reads well and which might even be portable to other hypervisors, so longer names might be more appropriate. -- Eric Blake ebl...@redhat.com+1-801-349-2682 Libvirt virtualization library http://libvirt.org -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 wu...@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 @@ intvirDomainBlockJobSetSpeed(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.
+
+intvirDomainBlockIoThrottle(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 @@
