Re: [Qemu-devel] [PATCH v3 4/7] block: remove legacy I/O throttling

[Kevin Wolf <kwolf@redhat.com>]

Am 25.08.2017 um 15:23 hat Manos Pitsidianakis geschrieben:
> This commit removes all I/O throttling from block/block-backend.c. In
> order to support the existing interface, it is changed to use the
> block/throttle.c filter driver.
> 
> The throttle filter node that is created by the legacy interface is
> stored in a 'throttle_node' field in the BlockBackendPublic of the
> device. The legacy throttle node is managed by the legacy interface
> completely. More advanced configurations with the filter drive are
> possible using the QMP API, but these will be ignored by the legacy
> interface.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>

This patch doesn't apply cleanly any more and needs a rebase.

>  /* should be called before blk_set_io_limits if a limit is set */
> -void blk_io_limits_enable(BlockBackend *blk, const char *group)
> +void blk_io_limits_enable(BlockBackend *blk, const char *group,  Error **errp)
>  {
> -    assert(!blk->public.throttle_group_member.throttle_state);
> -    throttle_group_register_tgm(&blk->public.throttle_group_member,
> -                                group, blk_get_aio_context(blk));
> +    BlockDriverState *bs = blk_bs(blk), *throttle_node;
> +    QDict *options = qdict_new();
> +    Error *local_err = NULL;
> +    ThrottleState *ts;
> +
> +    bdrv_drained_begin(bs);

bs can be NULL:

$ x86_64-softmmu/qemu-system-x86_64 -drive media=cdrom,bps=1024
Segmentation fault (core dumped)

>  static void blk_root_drained_begin(BdrvChild *child)
>  {
> +    ThrottleGroupMember *tgm;
>      BlockBackend *blk = child->opaque;
>  
>      if (++blk->quiesce_counter == 1) {
> @@ -1997,19 +2025,25 @@ static void blk_root_drained_begin(BdrvChild *child)
>  
>      /* Note that blk->root may not be accessible here yet if we are just
>       * attaching to a BlockDriverState that is drained. Use child instead. */
> -
> -    if (atomic_fetch_inc(&blk->public.throttle_group_member.io_limits_disabled) == 0) {
> -        throttle_group_restart_tgm(&blk->public.throttle_group_member);
> +    if (blk->public.throttle_node) {
> +        tgm = throttle_get_tgm(blk->public.throttle_node);
> +        if (atomic_fetch_inc(&tgm->io_limits_disabled) == 0) {
> +            throttle_group_restart_tgm(tgm);
> +        }
>      }
>  }
>  
>  static void blk_root_drained_end(BdrvChild *child)
>  {
> +    ThrottleGroupMember *tgm;
>      BlockBackend *blk = child->opaque;
>      assert(blk->quiesce_counter);
>  
> -    assert(blk->public.throttle_group_member.io_limits_disabled);
> -    atomic_dec(&blk->public.throttle_group_member.io_limits_disabled);
> +    if (blk->public.throttle_node) {
> +        tgm = throttle_get_tgm(blk->public.throttle_node);
> +        assert(tgm->io_limits_disabled);
> +        atomic_dec(&tgm->io_limits_disabled);
> +    }

We shouldn't really need any throttling code in
blk_root_drained_begin/end any more now because the throttle node will
be drained. If this code is necessary, a bdrv_drain() on an explicit
throttle node will work differently from one on an implicit one.

Unfortunately, this seems to be true about the throttle node. Implicit
throttle nodes will keep ignoring the throttle limit in order to
complete the drain request quickly, where as explicit throttle nodes
will process their requests at the configured speed before the drain
request can be completed.

This doesn't feel right to me, both should behave the same.

Kevin