Re: [Qemu-devel] [RFC 1/4] block: fix I/O throttling accounting blind spot

[Stefan Hajnoczi]

On Wed, Mar 27, 2013 at 9:50 AM, Zhi Yong Wu <address@hidden> wrote:
> On Thu, Mar 21, 2013 at 10:49 PM, Stefan Hajnoczi <address@hidden> wrote:
>> I/O throttling relies on bdrv_acct_done() which is called when a request
>> completes.  This leaves a blind spot since we only charge for completed
>> requests, not submitted requests.
>>
>> For example, if there is 1 operation remaining in this time slice the
>> guest could submit 3 operations and they will all be submitted
>> successfully since they don't actually get accounted for until they
>> complete.
>>
>> Originally we probably thought this is okay since the requests will be
>> accounted when the time slice is extended.  In practice it causes
>> fluctuations since the guest can exceed its I/O limit and it will be
>> punished for this later on.
>>
>> Account for I/O upon submission so that I/O limits are enforced
>> properly.
>>
>> Signed-off-by: Stefan Hajnoczi <address@hidden>
>> ---
>>  block.c                   | 19 ++++++++-----------
>>  include/block/block_int.h |  2 +-
>>  2 files changed, 9 insertions(+), 12 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index 0a062c9..31fb0b0 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -141,7 +141,6 @@ void bdrv_io_limits_disable(BlockDriverState *bs)
>>      bs->slice_start = 0;
>>      bs->slice_end   = 0;
>>      bs->slice_time  = 0;
>> -    memset(&bs->io_base, 0, sizeof(bs->io_base));
> If we try some concussive operations, enable I/O throttling at first,
> then disable it, and then enable it, how about? I guess that
> bs->slice_submitted will maybe be not correct.

The memset() was moved...

>> @@ -3772,11 +3773,7 @@ static bool bdrv_exceed_io_limits(BlockDriverState 
>> *bs, int nb_sectors,
>>          bs->slice_start = now;
>>          bs->slice_end   = now + bs->slice_time;
>>
>> -        bs->io_base.bytes[is_write]  = bs->nr_bytes[is_write];
>> -        bs->io_base.bytes[!is_write] = bs->nr_bytes[!is_write];
>> -
>> -        bs->io_base.ios[is_write]    = bs->nr_ops[is_write];
>> -        bs->io_base.ios[!is_write]   = bs->nr_ops[!is_write];
>> +        memset(&bs->slice_submitted, 0, sizeof(bs->slice_submitted));
>>      }
>>
>>      elapsed_time  = now - bs->slice_start;

...here.

Since bs->slice_start = 0 when I/O throttling is disabled we will
start a new slice next time bdrv_exceed_io_limits() is called.

Therefore bs->slice_submitted is consistent across disable/enable.

Stefan