[libvirt] [PATCH 6/8] Docs: Add description and validation for blkdeviotune
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> --- docs/formatdomain.html.in | 31 +++++++++++++++++++++++++++++++ docs/schemas/domaincommon.rng | 24 ++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 0 deletions(-) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index cbad196..733062d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -893,6 +893,14 @@ <driver name="tap" type="aio" cache="default"/> <source file='/var/lib/xen/images/fv0'/ startupPolicy='optional'> <target dev='hda' bus='ide'/> + <iotune> + <total_bytes_sec>n</total_bytes_sec> + <read_bytes_sec>n</read_bytes_sec> + <write_bytes_sec>n</write_bytes_sec> + <total_iops_sec>n</total_iops_sec> + <read_bytes_sec>n</read_bytes_sec> + <write_bytes_sec>n</write_bytes_sec> + </iotune> <boot order='2'/> <encryption type='...'> ... @@ -1010,6 +1018,29 @@ <span class="since">Since 0.0.3; <code>bus</code> attribute since 0.4.3; "usb" attribute value since after 0.4.4; "sata" attribute value since 0.9.7</span></dd> + <dt><code>iotune</code></dt> + <dd>The optional <code>iotune</code> element provides the ability + to set or get block I/O throttling for the device. Block I/O + throtting be implemented by qemu, is specified per-disk and can + vary across multiple disks.</dd> + <dt><code>total_bytes_sec</code></dt> + <dd>The optinal <code>total_bytes_sec</code> element is the total throughput + limit in bytes per second.</dd> + <dt><code>read_bytes_sec</code></dt> + <dd>The optinal <code>read_bytes_sec</code> element is the read throughput + limit in bytes per second.</dd> + <dt><code>write_bytes_sec</code</dt> + <dd>The optinal <code>write_bytes_sec</code> element is the write throughput + limit in bytes per second.</dd> + <dt><code>total_iops_sec</code></dt> + <dd>The optional <code>total_iops_sec</code> element is the total I/O operations + per second.</dd> + <dt><code>read_iops_sec</code></dt> + <dd>The optional <code>read_iops_sec</code> element is the read I/O operations + per second.</dd> + <dt><code>write_iops_sec</code></dt> + <dd>The optional <code>write_iops_sec</code> element is the write I/O operations + per second.</dd> <dt><code>driver</code></dt> <dd> The optional driver element allows specifying further details diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b6f858e..c6873a0 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -698,6 +698,30 @@ <optional> <ref name="snapshot"/> </optional> + <optional> + <element name="iotune"> + <choice> + <element name="total_bytes_sec"> + <ref name="unsignedLongLong"> + </element> + <element name="read_bytes_sec"> + <ref name="unsignedLongLong"> + </element> + <element name="write_bytes_sec"> + <ref name="unsignedLongLong"> + </element> + <element name="total_iops_sec"> + <ref name="unsignedLongLong"> + </element> + <element name="read_iops_sec"> + <ref name="unsignedLongLong"> + </element> + <element name="write_iopw_sec"> + <ref name="unsignedLongLong"> + </emement> + </choice> + </element> + </optional> <choice> <group> <attribute name="type"> -- 1.7.1
On 11/15/2011 02:02 AM, Lei Li wrote:
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> --- docs/formatdomain.html.in | 31 +++++++++++++++++++++++++++++++ docs/schemas/domaincommon.rng | 24 ++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 0 deletions(-)
I plan to squash this in with 4/8, and apply it prior to 3/8, although I'll review it independently since you submitted it alone. I find it slightly easier to read domain_conf.c parser/printer changes if you have .rng changes to compare them to (both now during series review, and later in git researching to see when a feature was added).
diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index cbad196..733062d 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -893,6 +893,14 @@ <driver name="tap" type="aio" cache="default"/> <source file='/var/lib/xen/images/fv0'/ startupPolicy='optional'> <target dev='hda' bus='ide'/> + <iotune> + <total_bytes_sec>n</total_bytes_sec>
These should be typical numbers, not a placeholder "n", so that someone can get an idea of what to copy-and-paste into their XML.
+ <read_bytes_sec>n</read_bytes_sec> + <write_bytes_sec>n</write_bytes_sec> + <total_iops_sec>n</total_iops_sec> + <read_bytes_sec>n</read_bytes_sec> + <write_bytes_sec>n</write_bytes_sec>
You said here: https://www.redhat.com/archives/libvir-list/2011-November/msg00442.html that the code enforces some additional constraints: total cannot be set at the same time as read or write limits. This example violates those constraints.
+ </iotune> <boot order='2'/> <encryption type='...'> ... @@ -1010,6 +1018,29 @@ <span class="since">Since 0.0.3; <code>bus</code> attribute since 0.4.3; "usb" attribute value since after 0.4.4; "sata" attribute value since 0.9.7</span></dd> + <dt><code>iotune</code></dt> + <dd>The optional <code>iotune</code> element provides the ability + to set or get block I/O throttling for the device. Block I/O + throtting be implemented by qemu, is specified per-disk and can
s/throtting/throttling/
+ vary across multiple disks.</dd>
This needs to be reworded to reflect the generic nature of the API (IO throttling is currently the only tuning parameter, but we may expand to other tuning parameters in the future).
+ <dt><code>total_bytes_sec</code></dt> + <dd>The optinal <code>total_bytes_sec</code> element is the total throughput
s/optinal/optional/
+ limit in bytes per second.</dd> + <dt><code>read_bytes_sec</code></dt> + <dd>The optinal <code>read_bytes_sec</code> element is the read throughput + limit in bytes per second.</dd> + <dt><code>write_bytes_sec</code</dt>
Missing a >.
+ <dd>The optinal <code>write_bytes_sec</code> element is the write throughput + limit in bytes per second.</dd> + <dt><code>total_iops_sec</code></dt> + <dd>The optional <code>total_iops_sec</code> element is the total I/O operations + per second.</dd> + <dt><code>read_iops_sec</code></dt> + <dd>The optional <code>read_iops_sec</code> element is the read I/O operations + per second.</dd> + <dt><code>write_iops_sec</code></dt> + <dd>The optional <code>write_iops_sec</code> element is the write I/O operations + per second.</dd>
The additional constraints in the code should also be documented here. I'm assuming that '0' has a special meaning of no limit; and back-compatibility demands that an omitted tuning has the same effect, which means if the user inputs '0' it is easier if we just omit it on output (and only output the non-zero values). Handling of '0' should be documented.
<dt><code>driver</code></dt> <dd> The optional driver element allows specifying further details diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index b6f858e..c6873a0 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -698,6 +698,30 @@ <optional> <ref name="snapshot"/> </optional> + <optional> + <element name="iotune">
Wrong location; it is better to list this among other sub-elements, rather than among the attributes of <disk>.
+ <choice> + <element name="total_bytes_sec"> + <ref name="unsignedLongLong">
There is no <define name='unsignedLongLong'> for this <ref> to resolve to; however, http://www.cafeconleche.org/slides/sd2006west/relaxng/RELAX__Schemas_Don%27t... documents unsignedLong as an 8-byte primitive, which matches our use case.
+ </element> + <element name="read_bytes_sec"> + <ref name="unsignedLongLong"> + </element>
Won't work. <choice> means that you can have exactly one tuning element, when in reality you want <interleave><oneOrMore> to allow combinations of elements. We can also make the XML enforce the constraints (total and read/write are mutually exclusive).
+ <element name="write_bytes_sec"> + <ref name="unsignedLongLong"> + </element> + <element name="total_iops_sec"> + <ref name="unsignedLongLong"> + </element> + <element name="read_iops_sec"> + <ref name="unsignedLongLong"> + </element> + <element name="write_iopw_sec">
Another typo.
+ <ref name="unsignedLongLong"> + </emement>
This typo here, along with several missing /,...
+ </choice> + </element> + </optional> <choice> <group> <attribute name="type">
completely breaks 'make check' on every test in domainschematest and domainsnapshotschematest. A sample of 'make -C tests check TESTS=domainsnapshotschematest VIR_TEST_DEBUG=1' shows: ref: Relax-NG parser error : Internal found no define for ref domain Relax-NG schema /home/remote/eblake/libvirt/tests/../docs/schemas/domainsnapshot.rng failed to compile It's hard to trust this series if basic things like 'make check' and 'make syntax-check' fail. Here's what I'm squashing in, before I merge this with 4/8 (hmm, I practically rewrote this entire patch): diff --git i/docs/formatdomain.html.in w/docs/formatdomain.html.in index 1bd263e..f45d0ce 100644 --- i/docs/formatdomain.html.in +++ w/docs/formatdomain.html.in @@ -923,12 +923,9 @@ <source file='/var/lib/xen/images/fv0'/ startupPolicy='optional'> <target dev='hda' bus='ide'/> <iotune> - <total_bytes_sec>n</total_bytes_sec> - <read_bytes_sec>n</read_bytes_sec> - <write_bytes_sec>n</write_bytes_sec> - <total_iops_sec>n</total_iops_sec> - <read_bytes_sec>n</read_bytes_sec> - <write_bytes_sec>n</write_bytes_sec> + <total_bytes_sec>10000000</total_bytes_sec> + <read_iops_sec>400000</read_iops_sec> + <write_iops_sec>100000</write_iops_sec> </iotune> <boot order='2'/> <encryption type='...'> @@ -1048,28 +1045,39 @@ "usb" attribute value since after 0.4.4; "sata" attribute value since 0.9.7</span></dd> <dt><code>iotune</code></dt> - <dd>The optional <code>iotune</code> element provides the ability - to set or get block I/O throttling for the device. Block I/O - throtting be implemented by qemu, is specified per-disk and can - vary across multiple disks.</dd> - <dt><code>total_bytes_sec</code></dt> - <dd>The optinal <code>total_bytes_sec</code> element is the total throughput - limit in bytes per second.</dd> - <dt><code>read_bytes_sec</code></dt> - <dd>The optinal <code>read_bytes_sec</code> element is the read throughput - limit in bytes per second.</dd> - <dt><code>write_bytes_sec</code</dt> - <dd>The optinal <code>write_bytes_sec</code> element is the write throughput - limit in bytes per second.</dd> - <dt><code>total_iops_sec</code></dt> - <dd>The optional <code>total_iops_sec</code> element is the total I/O operations - per second.</dd> - <dt><code>read_iops_sec</code></dt> - <dd>The optional <code>read_iops_sec</code> element is the read I/O operations - per second.</dd> - <dt><code>write_iops_sec</code></dt> - <dd>The optional <code>write_iops_sec</code> element is the write I/O operations - per second.</dd> + <dd>The optional <code>iotune</code> element provides the + ability to provide additional per-device I/O tuning, with + values that can vary for each device (contrast this to + the <a href="#elementsBlockTuning"><code><blkiotune></code></a> + element, which applies globally to the domain). Currently, + the only tuning available is Block I/O throttling for qemu. + This element has optional sub-elements; any sub-element not + specified or given with a value of 0 implies no + limit. <span class="since">Since 0.9.8</span> + <dl> + <dt><code>total_bytes_sec</code></dt> + <dd>The optional <code>total_bytes_sec</code> element is the + total throughput limit in bytes per second. This cannot + appear with <code>read_bytes_sec</code> + or <code>write_bytes_sec</code>.</dd> + <dt><code>read_bytes_sec</code></dt> + <dd>The optional <code>read_bytes_sec</code> element is the + read throughput limit in bytes per second.</dd> + <dt><code>write_bytes_sec</code></dt> + <dd>The optional <code>write_bytes_sec</code> element is the + write throughput limit in bytes per second.</dd> + <dt><code>total_iops_sec</code></dt> + <dd>The optional <code>total_iops_sec</code> element is the + total I/O operations per second. This cannot + appear with <code>read_iops_sec</code> + or <code>write_iops_sec</code>.</dd> + <dt><code>read_iops_sec</code></dt> + <dd>The optional <code>read_iops_sec</code> element is the + read I/O operations per second.</dd> + <dt><code>write_iops_sec</code></dt> + <dd>The optional <code>write_iops_sec</code> element is the + write I/O operations per second.</dd> + </dl> <dt><code>driver</code></dt> <dd> The optional driver element allows specifying further details diff --git i/docs/schemas/domaincommon.rng w/docs/schemas/domaincommon.rng index 965e033..7adf9a8 100644 --- i/docs/schemas/domaincommon.rng +++ w/docs/schemas/domaincommon.rng @@ -635,6 +635,9 @@ <optional> <ref name="address"/> </optional> + <optional> + <ref name="diskIoTune"/> + </optional> </define> <define name="snapshot"> <attribute name="snapshot"> @@ -698,30 +701,6 @@ <optional> <ref name="snapshot"/> </optional> - <optional> - <element name="iotune"> - <choice> - <element name="total_bytes_sec"> - <ref name="unsignedLongLong"> - </element> - <element name="read_bytes_sec"> - <ref name="unsignedLongLong"> - </element> - <element name="write_bytes_sec"> - <ref name="unsignedLongLong"> - </element> - <element name="total_iops_sec"> - <ref name="unsignedLongLong"> - </element> - <element name="read_iops_sec"> - <ref name="unsignedLongLong"> - </element> - <element name="write_iopw_sec"> - <ref name="unsignedLongLong"> - </emement> - </choice> - </element> - </optional> <choice> <group> <attribute name="type"> @@ -809,7 +788,11 @@ <ref name="diskspec"/> </interleave> </group> - <ref name="diskspec"/> + <group> + <interleave> + <ref name="diskspec"/> + </interleave> + </group> </choice> </element> </define> @@ -2620,6 +2603,47 @@ </element> </define> + <define name='diskIoTune'> + <element name="iotune"> + <interleave> + <choice> + <element name="total_bytes_sec"> + <data type="unsignedLong"/> + </element> + <group> + <optional> + <element name="read_bytes_sec"> + <data type="unsignedLong"/> + </element> + </optional> + <optional> + <element name="write_bytes_sec"> + <data type="unsignedLong"/> + </element> + </optional> + </group> + </choice> + <choice> + <element name="total_iops_sec"> + <data type="unsignedLong"/> + </element> + <group> + <optional> + <element name="read_iops_sec"> + <data type="unsignedLong"/> + </element> + </optional> + <optional> + <element name="write_iops_sec"> + <data type="unsignedLong"/> + </element> + </optional> + </group> + </choice> + </interleave> + </element> + </define> + <!-- Optional hypervisor extensions in their own namespace: QEmu -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 11/22/2011 10:28 PM, Eric Blake wrote:
On 11/15/2011 02:02 AM, Lei Li wrote:
Signed-off-by: Lei Li <lilei@linux.vnet.ibm.com> Signed-off-by: Zhi Yong Wu <wuzhy@linux.vnet.ibm.com> --- docs/formatdomain.html.in | 31 +++++++++++++++++++++++++++++++ docs/schemas/domaincommon.rng | 24 ++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 0 deletions(-)
I plan to squash this in with 4/8, and apply it prior to 3/8, although I'll review it independently since you submitted it alone. I find it slightly easier to read domain_conf.c parser/printer changes if you have .rng changes to compare them to (both now during series review, and later in git researching to see when a feature was added).
Here's what I'm squashing in, before I merge this with 4/8 (hmm, I practically rewrote this entire patch):
I'm also adding this; since my testing found that the <interleave> was at the wrong level if we intend to allow <address> to come either before or after <iotune>, nor did I allow write_{bytes,iops>_sec to come before read_{bytes,iops>_sec. diff --git i/docs/schemas/domaincommon.rng w/docs/schemas/domaincommon.rng index 7adf9a8..bb6d94d 100644 --- i/docs/schemas/domaincommon.rng +++ w/docs/schemas/domaincommon.rng @@ -599,45 +599,47 @@ </element> </define> <define name="diskspec"> - <optional> - <ref name="driver"/> - </optional> - <optional> - <ref name="diskAuth"/> - </optional> - <ref name="target"/> - <optional> - <ref name="deviceBoot"/> - </optional> - <optional> - <element name="readonly"> - <empty/> - </element> - </optional> - <optional> - <element name="shareable"> - <empty/> - </element> - </optional> - <optional> - <element name="transient"> - <empty/> - </element> - </optional> - <optional> - <element name="serial"> - <ref name="diskSerial"/> - </element> - </optional> - <optional> - <ref name="encryption"/> - </optional> - <optional> - <ref name="address"/> - </optional> - <optional> - <ref name="diskIoTune"/> - </optional> + <interleave> + <optional> + <ref name="driver"/> + </optional> + <optional> + <ref name="diskAuth"/> + </optional> + <ref name="target"/> + <optional> + <ref name="deviceBoot"/> + </optional> + <optional> + <element name="readonly"> + <empty/> + </element> + </optional> + <optional> + <element name="shareable"> + <empty/> + </element> + </optional> + <optional> + <element name="transient"> + <empty/> + </element> + </optional> + <optional> + <element name="serial"> + <ref name="diskSerial"/> + </element> + </optional> + <optional> + <ref name="encryption"/> + </optional> + <optional> + <ref name="diskIoTune"/> + </optional> + <optional> + <ref name="address"/> + </optional> + </interleave> </define> <define name="snapshot"> <attribute name="snapshot"> @@ -788,11 +790,7 @@ <ref name="diskspec"/> </interleave> </group> - <group> - <interleave> - <ref name="diskspec"/> - </interleave> - </group> + <ref name="diskspec"/> </choice> </element> </define> @@ -2611,16 +2609,18 @@ <data type="unsignedLong"/> </element> <group> - <optional> - <element name="read_bytes_sec"> - <data type="unsignedLong"/> - </element> - </optional> - <optional> - <element name="write_bytes_sec"> - <data type="unsignedLong"/> - </element> - </optional> + <interleave> + <optional> + <element name="read_bytes_sec"> + <data type="unsignedLong"/> + </element> + </optional> + <optional> + <element name="write_bytes_sec"> + <data type="unsignedLong"/> + </element> + </optional> + </interleave> </group> </choice> <choice> @@ -2628,16 +2628,18 @@ <data type="unsignedLong"/> </element> <group> - <optional> - <element name="read_iops_sec"> - <data type="unsignedLong"/> - </element> - </optional> - <optional> - <element name="write_iops_sec"> - <data type="unsignedLong"/> - </element> - </optional> + <interleave> + <optional> + <element name="read_iops_sec"> + <data type="unsignedLong"/> + </element> + </optional> + <optional> + <element name="write_iops_sec"> + <data type="unsignedLong"/> + </element> + </optional> + </interleave> </group> </choice> </interleave> -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Lei Li