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> > > 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@xxxxxxxxxx +1-801-349-2682 > Libvirt virtualization library http://libvirt.org > -- Adam Litke <agl@xxxxxxxxxx> IBM Linux Technology Center -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list