Re: [Qemu-devel] [RFC] block-insert-node and block-job-delete

[Kevin Wolf]

Am 27.07.2017 um 12:07 hat Stefan Hajnoczi geschrieben:
> On Wed, Jul 26, 2017 at 09:23:20PM +0300, Manos Pitsidianakis wrote:
> > On Wed, Jul 26, 2017 at 04:12:21PM +0100, Stefan Hajnoczi wrote:
> > > On Wed, Jul 26, 2017 at 05:19:24PM +0300, Manos Pitsidianakis wrote:
> > > > This proposal follows a discussion we had with Kevin and Stefan on 
> > > > filter
> > > > node management.
> > > > 
> > > > With block filter drivers arises a need to configure filter nodes on 
> > > > runtime
> > > > with QMP on live graphs. A problem with doing live graph modifications 
> > > > is
> > > > that some block jobs modify the graph when they are done and don't 
> > > > operate
> > > > under any synchronisation, resulting in a race condition if we try to 
> > > > insert
> > > > a filter node in the place of an existing edge.
> > > 
> > > But maybe you are thinking about higher level race conditions between
> > > QMP commands.  Can you give an example of the race?
> > 
> > Exactly, synchronisation between the QMP/user actions. Here's an example,
> > correct me if I'm wrong:
> > 
> > User issues a block-commit to this tree to commit A onto B:
> >    BB -> A -> B
> > 
> > This inserts the dummy mirror node at the top since this is a mirror job
> > (active commit)
> >    BB -> dummy -> A -> B
> > 
> > User wants to add a filter node F below the BB. First we create the node:
> >    BB -> dummy -> A -> B
> >       F /
> > 
> > Commit finishes before we do block-insert-node, dummy and A is removed from
> > the BB's chain, mirror_exit calls bdrv_replace_node()
> > 
> >    BB ------------> B
> >                F -> /
> > 
> > Now inserting F under BB will error since dummy does not exist any more.
> 
> I see the diagrams but the flow isn't clear without the user's QMP
> commands.
> 
> F is created using blockdev-add?  What are the parameters and how does
> it know about dummy?  I think this is an interesting question in itself
> because dummy is a transient internal node that QMP users don't
> necessarily know about.

We can expect that users of block-insert-node also know about block job
filter nodes, simply because the former is newer than the latter.

(I also don't like the name "dummy", this makes it sound like it's not
really a proper node. In reality, there is little reason to treat it
specially.)

> What is the full block-insert-node command?
> 
> > The proposal doesn't guard against the following:
> > User issues a block-commit to this tree to commit B onto C:
> >    BB -> A -> B -> C
> > 
> > The dummy node (commit's top bs is A):
> >    BB -> A -> dummy -> B -> C
> > 
> > blockdev-add of a filter we wish to eventually be between A and C:
> >    BB -> A -> dummy -> B -> C
> >             F/
> > 
> > if commit finishes before we issue block-insert-node (commit_complete()
> > calls bdrv_set_backing_hd() which only touches A)
> >    BB -> A --------------> C
> >           F -> dummy -> B /
> >    resulting in error when issuing block-insert-node,
> > 
> > else if we set the job to manual-delete and insert F:
> >    BB -> A -> F -> dummy -> B -> C
> > deleting the job will result in F being lost since the job's top bs wasn't
> > updated.
> 
> manual-delete is a solution for block jobs.  The write threshold
> feature is a plain QMP command that in the future will create a
> transient internal node (before write notifier).

Yes, that becomes a legacy QMP command then. The new way is blockdev-add
and block-insert-node for write threshold, too.

Apart from that, a write threshold node never disappears by itself, it
is only inserted or removed in the context of a QMP command. That makes
it a lot less dangerous than automatic block completion.

> I'm not sure it makes sense to turn the write threshold feature into a
> block job.  I guess it could work, but it seems a little unnatural and
> maybe there will be other features that use transient internal nodes.

Yeah, no reason to do so. Just a manually created filter node.

> What I'm getting at is that there might be alternative to manual-delete
> that work in the general case.  For example, if blockdev-add +
> block-insert-node are part of a QMP 'transaction' command, can we lock
> the graph to protect it against modifications?

We thought about this at the last STR meeting. However, not for much
longer than a minute. Markus and I both agreed that we wouldn't be the
ones to try and make blockdev-add transactionable. It sounds like a huge
amount of work and a high risk of bugs for little benefit.

Let's just make sure that we provide a way for management tools to avoid
any surprise changes in the block graph.

Kevin

signature.asc
Description: PGP signature