Re: [Qemu-devel] nbd: Possible regression in 2.9 RCs

[Kevin Wolf <kwolf@redhat.com>]

Am 06.04.2017 um 10:48 hat Kevin Wolf geschrieben:
> Am 05.04.2017 um 23:13 hat Paolo Bonzini geschrieben:
> > On 05/04/2017 13:01, Kevin Wolf wrote:
> > > Am 04.04.2017 um 17:09 hat Paolo Bonzini geschrieben:
> > >> On 04/04/2017 16:53, Kevin Wolf wrote:
> > >>>> The big question is how this fits into release management.  We have
> > >>>> another important regression from the op blocker work and only a week
> > >>>> to go before the last rc.  Are we going to delay 2.9 arbitrarily?  Are
> > >>>> we going to shorten the 2.10 development period correspondingly?  (I
> > >>>> vote yes and yes, FWIW).
> > >>> Which is the other regression?
> > >>
> > >> The assertion failure for snapshot_blkdev with iothreads.
> > > 
> > > Ah, right, I keep forgetting that this started appearing with the op
> > > blocker series because the failure mode is completely different, so it
> > > seems to have been a latent bug somewhere else that was uncovered by it.
> > > 
> > > If we're sure that the change of the order in bdrv_append() is what
> > > caused the bug to appear, we can just undo that for 2.9, at the cost of
> > > a messed up graph in the error case when bdrv_set_backing_hd() fails
> > > (because we have no way to undo bdrv_replace_node()).
> > 
> > I don't know if that is enough to fix all of the issues, but the bug is
> > easy to reproduce.
> > 
> > The issue is the lack of understanding of what node movement does to
> > quiesce_counter.  The invariant is that children cannot have a lower
> > quiesce_counter than parents, I think (paths in the graph can only join
> > in the children direction, right?).
> 
> Maybe I'm missing something, but I think this isn't true at all. Drains
> are propagated to the parents, so that this specific node doesn't
> receive new requests, but not to the children. The assumption is that
> children don't do anything anyway without requests from their parents,
> so they are effectively quiesced even with quiesce_counter == 0.
> 
> So if anything, the invariant should be the exact opposite: Parents
> cannot have a lower quiesce_counter than their children.
> 
> I think the exact thing that the quiesce_counter of a node is expected
> to be is the number of paths from itself to an explicitly drained node
> in the directed block driver graph (counting one path if it is
> explicitly drained itself). A path counts multiple times if a node is
> explicitly drained multiple times.
> 
> > Is it checked, and are there violations already?  Maybe we need a
> > get_quiesce_counter method in BdrvChildRole, to cover BlockBackend's
> > quiesce_counter?  Then we can use that information to adjust the
> > quiesce_counter when nodes move in the graph.
> 
> We would need that if we had a downwards propagation and if a
> BlockBackend could be drained, but as it stands, I don't see what could
> be missing from bdrv_replace_child_noperm() (well, except that I think
> your patch is right to avoid calling drained_end/begin if both nodes
> were drained because new requests could sneak in this way in theory).

Actually, to get this part completely right, we also need to drain the
BlockBackend _before_ attaching the new BDS. Otherwise, if the old BDS
wasn't quiesced, but the new one is, the BdrvChildRole.drained_begin()
callback could send requests to the already drained new BDS.

Kevin

> > The block layer has good tests, but as the internal logic grows more
> > complex we should probably have more C level tests.  I'm constantly
> > impressed by the amount of tricky cases that test-replication.c catches
> > in the block job code.
> 
> Never really noticed test-replication specifically catching things when
> I worked on the op blockers code which changed a lot around block jobs,
> but that we should consider this type of tests more often is probably a
> good point.
> 
> Kevin