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