Re: [Qemu-devel] [PATCH V5 3/5] block: Enable the new throttling code in the block layer.

[Stefan Hajnoczi]

On Mon, Aug 12, 2013 at 06:53:14PM +0200, Benoît Canet wrote:
> +/* this function drain all the throttled IOs */
> +static bool bdrv_drain_throttled(BlockDriverState *bs)
> +{
> +    bool drained = false;
> +    bool enabled = bs->io_limits_enabled;
> +    int i;
>  
> -    do {} while (qemu_co_enter_next(&bs->throttled_reqs));
> +    bs->io_limits_enabled = false;
>  
> -    if (bs->block_timer) {
> -        qemu_del_timer(bs->block_timer);
> -        qemu_free_timer(bs->block_timer);
> -        bs->block_timer = NULL;
> +    for (i = 0; i < 2; i++) {
> +        while (qemu_co_enter_next(&bs->throttled_reqs[i])) {
> +            drained = true;
> +        }
>      }

This function submits throttled requests but it doesn't drain them -
they might still be executing when this function returns!

> +/* should be called before bdrv_set_io_limits if a limit is set */
>  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);

Make sure we never double-initialized ->throttle_state:

assert(!bs->io_limits enabled);

> @@ -2512,11 +2552,6 @@ static int coroutine_fn 
> bdrv_co_do_readv(BlockDriverState *bs,
>          return -EIO;
>      }
>  
> -    /* throttling disk read I/O */
> -    if (bs->io_limits_enabled) {
> -        bdrv_io_limits_intercept(bs, false, nb_sectors);
> -    }
> -

Why move bdrv_io_limits_intercept() into tracked_request_begin()?  IMO
tracked_request_begin() should only create the tracked request, it
shouldn't do unrelated stuff like I/O throttling and yielding.  If it
does then it must be declared coroutine_fn.