We currently do not keep track of how many times a child has quiesced
its parent.  We just guess based on the child’s quiesce_counter.  That
keeps biting us when we try to leave drained sections or detach children
(see e.g. commit 5cb2737e925042e).

I think we need an explicit counter to keep track of how many times a
parent has been quiesced (patch 1).  That just makes the code more
resilient.

Actually, no, we don’t need a counter, we need a boolean.  See patch 2
for the explanation.

Yes, it is a bit weird to introduce a counter first (patch 1) and then
immediately make it a boolean (patch 2).  But I believe this to be the
most logical change set.

(“Our current model relies on counting, so adding an explicit counter
makes sense.  It then turns out that counting is probably not the best
idea, so why not make it a boolean?”)


Max Reitz (4):
  block: Introduce BdrvChild.parent_quiesce_counter
  block: Make @parent_quiesced a bool
  iotests: Add @has_quit to vm.shutdown()
  iotests: Test commit with a filter on the chain

 include/block/block.h      |  9 +++---
 include/block/block_int.h  |  6 ++++
 block.c                    | 18 ++++-------
 block/io.c                 | 61 +++++++++++++++++++++++++++++---------
 tests/test-bdrv-drain.c    | 31 +++++++++++--------
 python/qemu/__init__.py    |  5 ++--
 tests/qemu-iotests/040     | 40 ++++++++++++++++++++++++-
 tests/qemu-iotests/040.out |  4 +--
 tests/qemu-iotests/255     |  2 +-
 9 files changed, 126 insertions(+), 50 deletions(-)

-- 
2.21.0

Am 05.06.2019 um 18:11 hat Max Reitz geschrieben:
> We currently do not keep track of how many times a child has quiesced
> its parent.  We just guess based on the child’s quiesce_counter.  That
> keeps biting us when we try to leave drained sections or detach children
> (see e.g. commit 5cb2737e925042e).
> 
> I think we need an explicit counter to keep track of how many times a
> parent has been quiesced (patch 1).  That just makes the code more
> resilient.
> 
> Actually, no, we don’t need a counter, we need a boolean.  See patch 2
> for the explanation.
> 
> Yes, it is a bit weird to introduce a counter first (patch 1) and then
> immediately make it a boolean (patch 2).  But I believe this to be the
> most logical change set.
> 
> (“Our current model relies on counting, so adding an explicit counter
> makes sense.  It then turns out that counting is probably not the best
> idea, so why not make it a boolean?”)

Trying to summarise an IRC discussion I just had with Max:

The real root problem isn't that the recursion in bdrv_do_drained_end()
doesn't correctly deal with graph changes, but that those graph changes
happen in the first place. The one basic guiding principle in my drain
rewrite was that during the recursion (both to children and parents), no
graph changes are allowed, which means that no aio_poll() calls are
allowed either.

Of course, while I think the principle is right and greatly simplifies
the code (or actually is the only thing that gives us any hope to get
things right), I messed up the implementation because
bdrv_drain_invoke() does use BDRV_POLL_WHILE() for ending a drained
section. This is wrong, and it could still cause crashes after this
series because a recursive call could remove a node that is currently
processed somewhere up the call chain.

The fix for the observed bugs should be to make drained_end completely
symmetric to drained_begin: Just start the bdrv_drain_invoke_entry()
coroutine, do the recursion and call all the callbacks (none of which
may modify the graph) and only after all of this is done, poll once at
the top level drain. (The poll condition could be simplified to just
wait for bdrv_drain_invoke() to be completed, we don't care about other
running requests in drained_end. But this is only an optimisation.)

Despite this being a full fix, we also agreed that patch 1 is a nice
cleanup and we want to keep it even if it doesn't strictly fix a bug any
more.

Kevin