Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/17] block-backend: Decrease in_flight only after callback

[Paolo Bonzini]

On 18/09/2018 16:56, Kevin Wolf wrote:
> Am 18.09.2018 um 16:12 hat Paolo Bonzini geschrieben:
>> On 18/09/2018 13:34, Kevin Wolf wrote:
>>>> But then basically the main issue is mirror.c's call to
>>>> bdrv_drained_begin/end.  There are no other calls to
>>>> bdrv_drained_begin/end inside coroutines IIRC.
>>>
>>> Coroutine or not doesn't matter. What matters is that you drain inside
>>> some (high-level) operation that already needs to be completed itself
>>> for drain to return.
>>
>> Indeed.  However, are there any calls to bdrv_drained_begin/end that are
>> inside inc_in_flight/dec_in_flight, and are not inside coroutines?
>> (Sorry if I don't see the high-level issue, I'm more familiar with the
>> low-level functioning...).
> 
> inc_in_flight is only one thing that signals pending activity.
> Essentially, what we're interested in is everything that will make
> bdrv_drain_poll() return true.
> 
> For a BDS that is in use by a block job, that BDS will be considered
> busy as long as the job hasn't paused yet, because otherwise the job can
> still issue new requests to the BDS (see child_job_drained_poll()).
> 
> So there is no choice here: The bdrv_subtree_drained_begin() in reopen
> _must_ make sure that the block job is really quiescent and no callbacks
> for it are pending any more.
> 
> At the same time, the completion callbacks of the job (.prepare/.commit/
> .abort) use drain themselves because they contain reopens and perform
> graph reconfigurations. In this case, we don't want to wait for the
> completion callback to finish because that's a deadlock.
> 
> Whether we want to wait on the job doesn't depend on where in its code
> the job currently is. It only depends on whether the drain is issued by
> the job itself or by someone else. This is what makes it hard to return
> the right thing in child_job_drained_poll() because I don't see an easy
> way to know the caller.

I see.  But anyway, I now think your patch is correct (without the
ref/unref) as long as the AIO callback does not call drain directly:

- calling it through a coroutine is okay, because then
bdrv_drained_begin goes through bdrv_co_yield_to_drain and you have
in_flight=2 when bdrv_co_yield_to_drain yields, then soon in_flight=1
when the aio_co_wake in the AIO callback completes, then in_flight=0
after the bottom half starts.

- calling it through a bottom half would be okay too, as long as the AIO
callback remembers to do inc_in_flight/dec_in_flight just like
bdrv_co_yield_to_drain and bdrv_co_drain_bh_cb do

A few more important cases that come to mind:

- a coroutine that yields because of I/O is okay, with a sequence
similar to bdrv_co_yield_to_drain

- a coroutine that yields with no I/O pending will correctly decrease
in_flight to zero before yielding

- calling more AIO from the callback won't overflow the counter just
because of mutual recursion, because AIO functions always yield at least
once before invoking the callback.

Paolo