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 Litke<agl(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.
I think the answer to this question lies in how it will be used.
Looking at your patch, right now, we have:
<domain>
<blkiotune>
<weight>nnn</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>
<bps>nnn</bps>
<bps_rd>nnn</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 eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
--
Adam Litke <agl(a)us.ibm.com>
IBM Linux Technology Center