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

[Paolo Bonzini <pbonzini@redhat.com>]

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.

>> 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.
 If the nodes disappear or move elsewhere in the graph it's okay, you've
just done useless work.  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.

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

> 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?
> 
> In that solution, splitting the function by moving aio_poll() out
> wouldn't get rid of a parameter and simplify the API any more. It might
> still be cleaner, though?
> 
> Kevin
>