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@xxxxxxxxxx> 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> 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list