Re: [Qemu-block] [PATCH 07/19] block: Really pause block jobs on drain

[Kevin Wolf]

Am 12.04.2018 um 12:12 hat Paolo Bonzini geschrieben:
> On 12/04/2018 11:51, Kevin Wolf wrote:
> > Am 12.04.2018 um 10:37 hat Paolo Bonzini geschrieben:
> >> On 11/04/2018 18:39, Kevin Wolf wrote:
> >>> +bool bdrv_drain_poll(BlockDriverState *bs, bool top_level)
> >>>  {
> >>>      /* Execute pending BHs first and check everything else only after 
> >>> the BHs
> >>>       * have executed. */
> >>> -    while (aio_poll(bs->aio_context, false));
> >>> +    if (top_level) {
> >>> +        while (aio_poll(bs->aio_context, false));
> >>> +    }
> >>> +
> >>> +    if (bdrv_parent_drained_poll(bs)) {
> >>> +        return true;
> >>> +    }
> >>> +
> >>>      return atomic_read(&bs->in_flight);
> >>>  }
> >>>  
> >>
> >> Up until now I liked very much this series, but I like this patch a bit
> >> less for two reasons.
> >>
> >> 1) I think I would prefer to have the !top_level case in a separate
> >> function---making the API easier to use in the BdrvChildRole callback
> >> because there is no need to pass false.
> > 
> > Basically just move the aio_poll() out to a different function that
> > calls bdrv_drain_poll afterwards? Maybe it's a bit cleaner, yes.
> > However, see below.
> 
> Yes.
> 
> >> In addition, the callback is not really polling anything, but rather
> >> returning whether the children are quiescent.  So a better name would
> >> be bdrv_children_drained and c->role->drained.
> > 
> > Why isn't it polling? We're actively checking the state of the node, and
> > we keep calling the callback until it has the expected state. Would it
> > only become polling for you if the loop were in this function rather
> > than the its caller?
> 
> It's just checking the status, it's not invoking the event loop.  The
> polling (in the sense of aio_poll or AIO_POLL_WHILE) is done elsewhere,
> this function is just the condition.  It's just nomenclature I guess.

Yes, it doesn't poll the AioContext events. I don't think that the name
implies that, though. It does poll the state of the block node and its
users.

> >> 2) Worse, the main idea behind the first drain restructuring was that
> >> draining could proceed in topological order: first drain the roots' I/O,
> >> then call bdrv_drain to send the last requests to their children, then
> >> recurse.  It is not clear to me why you need to introduce this recursive
> >> step, which is also O(n^2) in the worst case.
> > 
> > I need to introduce it because it fixes the bug that we don't wait until
> > the parents are actually quiesced and don't send new requests any more.
> > I don't see how this could be fixed without going to the parents.
> 
> Yes, you do need to go to the parents.  I don't understand however why
> you need more than a walk of the graph in parent-before-child order
> (which is a topological order, so it is reverse depth-first order and
> it's easy to do the walk in a recursive function).  If you're draining X
> below:
> 
>      A
>      |
>      B   C
>       \ /
>        X
> 
> then you start by draining A/B/C in topological order (so A before B).
> If B happens to be already quiescent, you can skip not only B but A too.

Are we talking only about bdrv_drain_poll() here or the complete
bdrv_do_drained_begin() call?

If you don't want to recurse separately for .drain_begin and the polling
phase, but keep everything in one function like it's before this series,
you can't skip anything just because it's already quiescent. You don't
have control over that other drain and it might end too early; also,
.drain_begin/end must come in pairs.

If we're only talking about .drain_poll, then in theory you could skip A
if B knows that it's fully quiesced (i.e. not only .drain_begin was
called, but also .drain_poll has returned false at least once). We don't
store this information anywhere, though, and I'm not sure if the saved
recursion would be worth additional state.

>  If the nodes disappear or move elsewhere in the graph it's okay, you've
> just done useless work.

Not really. The node is drained now and bdrv_do_drained_end() won't end
the drained section because the moved node isn't reachable any more from
this original node.

> When you're done you ensure that every parent
> is quiescent, and if so you're done.  If it's not, , a new parent
> appeared---drain that too, using the same parent-before-child order, and
> loop.

That might work if we only ever were in a single drained section and
BlockDriverState had a bool quiesced. What we do have is a
quiesce_count, and when you look at the quiesce_count of a node, you
can't tell whether it came from you or someone else. So there is no way
to tell whether a parent was already drained by you or not.

And to be honest, even if it worked, it would still seem kind of
complicated to me. Just calling .drain_begin everywhere first and then
polling until nobody has anything in flight any more feels much simpler
conceptually than keeping track of which nodes we drained and repeating
that until no new nodes appear, and somehow undraining the nodes that
disappeared, but are potentially still referenced from somewhere else.

> Well, there is one gotcha: bdrv_ref protects against disappearance, but
> bdrv_ref/bdrv_unref are not thread-safe.  Am I missing something else?

Apart from the above, if we do an extra bdrv_ref/unref we'd also have
to keep track of all the nodes that we've referenced so that we unref
the same nodes again, even if the graph has changes.

So essentially you'd be introducing a new list of BDSes that we have to
manage and then check for every reachable node whether it's already in
that list or not, and for every node in the list whether it's still
reachable. I don't really see how that's going to reduce the complexity.

> > Is the O(n²) that you mean that we recursively iterate all children in
> > bdrv_do_drained_begin() (or later in the series in bdrv_drain_poll()),
> > and then come back from the children when they poll their parents?
> 
> Yes.
> 
> Paolo
> 
> > We could do the same thing as for bdrv_parent_drained_begin(), i.e. pass
> > the parent that we came from (BdrvChild *parent instead of bool
> > top_level, NULL instead of top_level=true) and then skip that parent
> > while calling the BdrvChildRole .drain_poll callbacks. Would that
> > address your concerns?

Ok, then this should be the solution. I can implement it for v2.

Kevin