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.
pgpkS9ITuO2gR.pgp
Description: PGP signature
