Re: [Qemu-devel] [PATCH 4/6] throttle: Add throttle group support

[Stefan Hajnoczi]

On Tue, Mar 10, 2015 at 05:30:48PM +0200, Alberto Garcia wrote:
> @@ -179,10 +179,11 @@ static void bdrv_throttle_write_timer_cb(void *opaque)
>  }
>  
>  /* should be called before bdrv_set_io_limits if a limit is set */
> -void bdrv_io_limits_enable(BlockDriverState *bs)
> +void bdrv_io_limits_enable(BlockDriverState *bs, const char *group)
>  {
>      assert(!bs->io_limits_enabled);
> -    throttle_init(&bs->throttle_state);
> +
> +    throttle_group_register_bs(bs, group ? group : bdrv_get_device_name(bs));

Please don't allow a NULL group argument.  blockdev_init() should pick
the group name instead of requiring bdrv_io_limits_enable() to generate
a unique name.

This way we avoid silently adding all BDS without a BlockBackend to a ""
throttling group.  I think that's a bug waiting to happen :).

> @@ -201,29 +219,7 @@ static void bdrv_io_limits_intercept(BlockDriverState 
> *bs,
>                                       unsigned int bytes,
>                                       bool is_write)
>  {
> -    /* does this io must wait */
> -    bool must_wait = throttle_schedule_timer(&bs->throttle_state,
> -                                             &bs->throttle_timers,
> -                                             is_write);
> -
> -    /* if must wait or any request of this type throttled queue the IO */
> -    if (must_wait ||
> -        !qemu_co_queue_empty(&bs->throttled_reqs[is_write])) {
> -        qemu_co_queue_wait(&bs->throttled_reqs[is_write]);
> -    }
> -
> -    /* the IO will be executed, do the accounting */
> -    throttle_account(&bs->throttle_state, is_write, bytes);
> -
> -
> -    /* if the next request must wait -> do nothing */
> -    if (throttle_schedule_timer(&bs->throttle_state, &bs->throttle_timers,
> -                                is_write)) {
> -        return;
> -    }
> -
> -    /* else queue next request for execution */
> -    qemu_co_queue_next(&bs->throttled_reqs[is_write]);
> +    throttle_group_io_limits_intercept(bs, bytes, is_write);
>  }

bdrv_io_limits_intercept() is no longer useful.  Can you replace
bdrv_io_limits_intercept() calls with
throttle_group_io_limits_intercept() to eliminate this indirection?

>  
>  size_t bdrv_opt_mem_align(BlockDriverState *bs)
> @@ -2050,15 +2046,16 @@ static void bdrv_move_feature_fields(BlockDriverState 
> *bs_dest,
>      bs_dest->enable_write_cache = bs_src->enable_write_cache;
>  
>      /* i/o throttled req */
> -    memcpy(&bs_dest->throttle_state,
> -           &bs_src->throttle_state,
> -           sizeof(ThrottleState));
> +    bs_dest->throttle_state     = bs_src->throttle_state,
> +    bs_dest->io_limits_enabled  = bs_src->io_limits_enabled;
> +    bs_dest->throttled_reqs[0]  = bs_src->throttled_reqs[0];
> +    bs_dest->throttled_reqs[1]  = bs_src->throttled_reqs[1];
> +    memcpy(&bs_dest->round_robin,
> +           &bs_src->round_robin,
> +           sizeof(bs_dest->round_robin));

The bdrv_swap() code is tricky to think about...I think this is okay
because bs_dest and bs_src linked list pointers will be unchanged
(although they are now pointing to a different block driver instance).

Just highlighting this in case anyone else spots a problem with the
pointer swapping.

> @@ -145,6 +147,131 @@ static BlockDriverState 
> *throttle_group_next_bs(BlockDriverState *bs)
>      return next;
>  }
>  
> +/* Return the next BlockDriverState in the round-robin sequence with
> + * pending I/O requests.
> + *
> + * @bs:        the current BlockDriverState
> + * @is_write:  the type of operation (read/write)
> + * @ret:       the next BlockDriverState with pending requests, or bs
> + *             if there is none.

Assumes tg->lock is held.  It's helpful to document this.

pgpFiTuciL1D0.pgp
Description: PGP signature