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

[Manos Pitsidianakis]

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