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
