On Mon, Jun 26, 2017 at 04:44:44PM +0100, Stefan Hajnoczi wrote:
On Fri, Jun 23, 2017 at 03:46:59PM +0300, Manos Pitsidianakis wrote:-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?
Is this possible unless the user explicitly does this? I suppose going down the path and removing it from any place is okay. If the throttle_node has more than one parent then the result would be invalid anyway. I don't see anything in the code doing this (removing a BDS from a BB->leaf path) or wrapping it in some way, is that what should be done?
+ + /* 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?
Kevin had said in a previous discussion:
The throttle node affects only the backing file now, but not the new top-level node. With manually created filter nodes we may need to provide a way so that the QMP command tells where in the tree the snapshot is actually taken. With automatically created ones, we need to make sure that they always stay on top. Similar problems arise when block jobs manipulate the graph. For example, think of a commit block job, which will remove the topmost node from the backing file chain. We don't want it to remove the throttling filter together with the qcow2 node. (Or do we? It might be something that needs to be configurable.) We also have several QMP that currently default to working on the topmost image of the chain. Some of them may actually want to work on the topmost node that isn't an automatically inserted filter. I hope this helps you understand why I keep saying that automatic insertion/removal of filter nodes is tricky and we should do it as little as we possibly can.
Yes, this constraint isn't enforced. The automatically inserted filter is not a very clean concept. I'll have to think about it a little more, unless there are some ideas.
signature.asc
Description: PGP signature