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.
