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

[Stefan Hajnoczi]

On Tue, Jun 27, 2017 at 01:45:07AM +0300, Manos Pitsidianakis wrote:
> 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?

We cannot assume the user will keep their hands off the graph :).

In general, QEMU cannot assume that external interfaces (e.g.
command-line, monitor, VNC, ...) are only called in convenient and safe
ways.

The code must handle all possible situations.  Usually the simplest
solution is to reject requests that would put QEMU into an invalid
state.

> > 
> > > +
> > > +    /* 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.

The block layer has an "op blockers" and permissions API to reject
operations that would violate the graph constraints.  Kevin has recently
worked on it and I hope he can give you some advice in this area.

Stefan

signature.asc
Description: PGP signature