[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/17] block-backend: Decrease i

From: Paolo Bonzini
Subject: Re: [Qemu-devel] [Qemu-block] [PATCH v2 11/17] block-backend: Decrease in_flight only after callback
Date: Wed, 19 Sep 2018 18:57:11 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1

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.


reply via email to

[Prev in Thread] Current Thread [Next in Thread]