[libvirt] [RFC PATCH 1/5] Add new API virDomainBlockIoThrottle

[Adam Litke]

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 at 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 at redhat.com    +1-801-349-2682
> Libvirt virtualization library http://libvirt.org
> 

-- 
Adam Litke <agl at us.ibm.com>
IBM Linux Technology Center