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

[Stefan Hajnoczi]

On Fri, Jul 28, 2017 at 02:08:43PM +0200, Kevin Wolf wrote:
> 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.

Nice thinking - write threshold can be a node too!

signature.asc
Description: PGP signature