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 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
On Tue, Oct 11, 2011 at 10:59 PM, Adam Litke<agl@us.ibm.com> wrote: 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>
Since the top level element is 'blkiotune', I think it would make sense for the per-disk element to be 'iotune' instead of 'iothrottle', because not all of the tunables will neccessarily imply throttling. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|