Re: [Qemu-devel] [PATCH V2 for-1.6 2/5] block: Modify the throttling code to implement the leaky bucket algorithm.

[Stefan Hajnoczi]

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().