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

[Manos Pitsidianakis <el13635@mail.ntua.gr>]

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

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.

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