On Wed, Mar 27, 2013 at 5:14 PM, Stefan Hajnoczi <stefa...@gmail.com> wrote: > On Wed, Mar 27, 2013 at 9:50 AM, Zhi Yong Wu <zwu.ker...@gmail.com> wrote: >> On Thu, Mar 21, 2013 at 10:49 PM, Stefan Hajnoczi <stefa...@redhat.com> >> 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 <stefa...@redhat.com> >>> --- >>> 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. Yes, i also realized this just when i came home by subway. > > Stefan
-- Regards, Zhi Yong Wu