[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH RFC v3 7/8] block: remove legacy I/
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [Qemu-block] [PATCH RFC v3 7/8] block: remove legacy I/O throttling |
Date: |
Mon, 26 Jun 2017 16:44:44 +0100 |
User-agent: |
Mutt/1.8.0 (2017-02-23) |
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.
signature.asc
Description: PGP signature
- [Qemu-devel] [PATCH RFC v3 4/8] block: convert ThrottleGroup to object with QOM, (continued)
- [Qemu-devel] [PATCH RFC v3 4/8] block: convert ThrottleGroup to object with QOM, Manos Pitsidianakis, 2017/06/23
- Re: [Qemu-devel] [Qemu-block] [PATCH RFC v3 4/8] block: convert ThrottleGroup to object with QOM, Stefan Hajnoczi, 2017/06/26
- Re: [Qemu-devel] [Qemu-block] [PATCH RFC v3 4/8] block: convert ThrottleGroup to object with QOM, Manos Pitsidianakis, 2017/06/26
- Re: [Qemu-devel] [Qemu-block] [PATCH RFC v3 4/8] block: convert ThrottleGroup to object with QOM, Stefan Hajnoczi, 2017/06/27
- Re: [Qemu-devel] [Qemu-block] [PATCH RFC v3 4/8] block: convert ThrottleGroup to object with QOM, Alberto Garcia, 2017/06/27
- Re: [Qemu-devel] [Qemu-block] [PATCH RFC v3 4/8] block: convert ThrottleGroup to object with QOM, Manos Pitsidianakis, 2017/06/27
- Re: [Qemu-devel] [Qemu-block] [PATCH RFC v3 4/8] block: convert ThrottleGroup to object with QOM, Stefan Hajnoczi, 2017/06/28
[Qemu-devel] [PATCH RFC v3 7/8] block: remove legacy I/O throttling, Manos Pitsidianakis, 2017/06/23
- Re: [Qemu-devel] [Qemu-block] [PATCH RFC v3 7/8] block: remove legacy I/O throttling,
Stefan Hajnoczi <=
[Qemu-devel] [PATCH RFC v3 8/8] block: add throttle block filter driver interface tests, Manos Pitsidianakis, 2017/06/23
[Qemu-devel] [PATCH RFC v3 5/8] block: add BlockDevOptionsThrottle to QAPI, Manos Pitsidianakis, 2017/06/23