On 11/15/2011 02:02 AM, Lei Li wrote: 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). 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. 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. s/throtting/throttling/ 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). s/optinal/optional/ Missing a >. 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. Wrong location; it is better to list this among other sub-elements, rather than among the attributes of <disk>. There is no <define name='unsignedLongLong'> for this <ref> to resolve to; however, http://www.cafeconleche.org/slides/sd2006west/relaxng/RELAX__Schemas_Don%... documents unsignedLong as an 8-byte primitive, which matches our use case. 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). Another typo. This typo here, along with several missing /,... 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 @@ &lt;source file='/var/lib/xen/images/fv0'/ startupPolicy='optional'&gt; &lt;target dev='hda' bus='ide'/&gt; &lt;iotune&gt; - &lt;total_bytes_sec&gt;n&lt;/total_bytes_sec&gt; - &lt;read_bytes_sec&gt;n&lt;/read_bytes_sec&gt; - &lt;write_bytes_sec&gt;n&lt;/write_bytes_sec&gt; - &lt;total_iops_sec&gt;n&lt;/total_iops_sec&gt; - &lt;read_bytes_sec&gt;n&lt;/read_bytes_sec&gt; - &lt;write_bytes_sec&gt;n&lt;/write_bytes_sec&gt; + &lt;total_bytes_sec&gt;10000000&lt;/total_bytes_sec&gt; + &lt;read_iops_sec&gt;400000&lt;/read_iops_sec&gt; + &lt;write_iops_sec&gt;100000&lt;/write_iops_sec&gt; &lt;/iotune&gt; &lt;boot order='2'/&gt; &lt;encryption type='...'&gt; @@ -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>&lt;blkiotune&gt;</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(a)redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org