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

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4250 bytes --]

On Fri, Jun 23, 2017 at 03:46:59PM +0300, Manos Pitsidianakis wrote:
> @@ -1914,45 +1878,115 @@ int blk_commit_all(void)
>  /* throttling disk I/O limits */
>  void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg)
>  {
> -    throttle_group_config(&blk->public.throttle_group_member, cfg);
> +    ThrottleGroupMember *tgm;
> +
> +    assert(blk->public.throttle_node);
> +    tgm = blk->public.throttle_node->opaque;
> +    throttle_group_config(tgm, cfg);

block-backend.c should not access ->opaque.  Instead block/throttle.c
could provide an interface:

  void throttle_node_set_config(BlockDriverState *bs,
                                ThrottleConfig *cfg);

We know bs is always a throttle node but it's also possible for
block/trottle.c to check that:

  assert(bs->drv == &throttle_driver_ops);

>  }
>  
> -void blk_io_limits_disable(BlockBackend *blk)
> +void blk_io_limits_disable(BlockBackend *blk, Error **errp)
>  {
> -    assert(blk->public.throttle_group_member.throttle_state);
> -    bdrv_drained_begin(blk_bs(blk));
> -    throttle_group_unregister_tgm(&blk->public.throttle_group_member);
> -    bdrv_drained_end(blk_bs(blk));
> +    Error *local_err = NULL;
> +    BlockDriverState *bs, *throttle_node;
> +
> +    throttle_node = blk_get_public(blk)->throttle_node;
> +
> +    assert(throttle_node && throttle_node->refcnt == 1);

I'm not sure if we can enforce refcnt == 1.  What stops other graph
manipulation operations from inserting a node above or a BB that uses
throttle_node as the root?

> +
> +    bs = throttle_node->file->bs;
> +    blk_get_public(blk)->throttle_node = NULL;

Missing drained_begin/end region around code that modifies the graph.

> +
> +    /* ref throttle_node's child bs so that it isn't lost when throttle_node is
> +     * destroyed */
> +    bdrv_ref(bs);
> +
> +    /* this destroys throttle_node */
> +    blk_remove_bs(blk);

This assumes that throttle_node is the top node.  How is this constraint enforced?

> +
> +    blk_insert_bs(blk, bs, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        blk_insert_bs(blk, bs, NULL);

How does this handle the error? :)

If there's no way to handle the error then error_abort should be used.

> +    }
> +    bdrv_unref(bs);
>  }
>  
>  /* 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)
>  {
> -    blk->public.throttle_group_member.aio_context = blk_get_aio_context(blk);
> -    assert(!blk->public.throttle_group_member.throttle_state);
> -    throttle_group_register_tgm(&blk->public.throttle_group_member, group);

It would be nice to do:

assert(!blk->public.throttle_node);

> +    BlockDriverState *bs = blk_bs(blk), *throttle_node;
> +    Error *local_err = NULL;
> +    /*
> +     * increase bs refcount so it doesn't get deleted when removed
> +     * from the BlockBackend's root
> +     * */
> +    bdrv_ref(bs);
> +    blk_remove_bs(blk);
> +
> +    QDict *options = qdict_new();
> +    qdict_set_default_str(options, "file", bs->node_name);
> +    qdict_set_default_str(options, "throttling-group", group);
> +    throttle_node = bdrv_new_open_driver(bdrv_find_format("throttle"),
> +            NULL, bdrv_get_flags(bs), options, &local_err);
> +
> +    QDECREF(options);

Perhaps it's more consistent to use bdrv_open_inherit() ownership
semantics instead.  Then callers don't need to worry about freeing
options.

> +    if (local_err) {
> +        blk_insert_bs(blk, bs, NULL);

&error_abort

> +        bdrv_unref(bs);
> +        error_propagate(errp, local_err);
> +        return;
> +    }
> +    /* bs will be throttle_node's child now so unref it*/
> +    bdrv_unref(bs);
> +
> +    blk_insert_bs(blk, throttle_node, &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);

The only blk_insert_bs() errors are permission errors.  Can the code
guarantee that permissions will always be usable?  Then you can drop the
error handling and just use &error_abort.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]