On Tue, Jul 23, 2013 at 02:21:04PM +0200, Benoît Canet wrote: > @@ -152,6 +254,10 @@ void bdrv_io_limits_enable(BlockDriverState *bs) > qemu_co_queue_init(&bs->throttled_reqs); > bs->block_timer = qemu_new_timer_ns(vm_clock, bdrv_block_timer, bs); > bs->io_limits_enabled = true; > + bs->previous_leak = qemu_get_clock_ns(rt_clock); > + qemu_mod_timer(bs->block_timer, > + qemu_get_clock_ns(vm_clock) + > + BLOCK_IO_THROTTLE_PERIOD); > }
I don't fully understand the patch yet but: 1. Can we activate the timer only when requests are actually pending? Imagine a host with 1000 guests, even a 1 second timer becomes wasteful. 2. You don't vary the wait time, does this mean a throttled request must wait for max 1 second? If yes, then it introduces a big variance on request latency. > +/* This function check if the correct bandwith threshold has been exceeded > + * > + * @is_write: true if the current IO is a write, false if it's a read > + * @ret: true if threshold has been exceeded else false > + */ > +static bool bdrv_is_bps_threshold_exceeded(BlockDriverState *bs, bool > is_write) > +{ > + /* limit is on total read + write bps : do the sum and compare with total > + * threshold > + */ > + if (bs->io_limits.bps[BLOCK_IO_LIMIT_TOTAL]) { > + int64_t bytes = bs->leaky_buckets.bytes[0] + > + bs->leaky_buckets.bytes[1]; BLOCK_IO_LIMIT_READ, BLOCK_IO_LIMIT_WRITE instead of 0/1? > + return bs->io_limits.bps_threshold[BLOCK_IO_LIMIT_TOTAL] < bytes; > + } > + > + /* check wether the threshold corresponding to the current io type (read, > + * write has been exceeded s/write/write)/ > + */ > + if (bs->io_limits.bps[is_write]) { > + return bs->io_limits.bps_threshold[is_write] < > + bs->leaky_buckets.bytes[is_write]; > + } > + > + /* no limit */ > + return false; > +} > + > +/* This function check if the correct iops threshold has been exceeded > + * > + * @is_write: true if the current IO is a write, false if it's a read > + * @ret: true if threshold has been exceeded else false > + */ > +static bool bdrv_is_iops_threshold_exceeded(BlockDriverState *bs, bool > is_write) > +{ Same comments apply as to bdrv_is_bps_threshold_exceeded(). > @@ -280,10 +280,25 @@ static int parse_block_error_action(const char *buf, > bool is_read) > } > } > > +static bool invalid(int64_t limit) Please choose a more specific function name like check_io_limit().