[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close()
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close() |
Date: |
Fri, 12 Jul 2019 13:09:48 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.2 |
On 12.07.19 13:01, Kevin Wolf wrote:
> Am 12.07.2019 um 12:47 hat Max Reitz geschrieben:
>> On 12.07.19 11:24, Kevin Wolf wrote:
>>> Am 11.07.2019 um 21:58 hat Max Reitz geschrieben:
>>>> When nbd_close() is called from a coroutine, the connection_co never
>>>> gets to run, and thus nbd_teardown_connection() hangs.
>>>>
>>>> This is because aio_co_enter() only puts the connection_co into the main
>>>> coroutine's wake-up queue, so this main coroutine needs to yield and
>>>> reschedule itself to let the connection_co run.
>>>>
>>>> Signed-off-by: Max Reitz <address@hidden>
>>>> ---
>>>> block/nbd.c | 12 +++++++++++-
>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/block/nbd.c b/block/nbd.c
>>>> index 81edabbf35..b83b6cd43e 100644
>>>> --- a/block/nbd.c
>>>> +++ b/block/nbd.c
>>>> @@ -135,7 +135,17 @@ static void nbd_teardown_connection(BlockDriverState
>>>> *bs)
>>>> qio_channel_shutdown(s->ioc,
>>>> QIO_CHANNEL_SHUTDOWN_BOTH,
>>>> NULL);
>>>> - BDRV_POLL_WHILE(bs, s->connection_co);
>>>> +
>>>> + if (qemu_in_coroutine()) {
>>>> + /* Let our caller poll and just yield until connection_co is done
>>>> */
>>>> + while (s->connection_co) {
>>>> + aio_co_schedule(qemu_get_current_aio_context(),
>>>> + qemu_coroutine_self());
>>>> + qemu_coroutine_yield();
>>>> + }
>>>
>>> Isn't this busy waiting? Why not let s->connection_co wake us up when
>>> it's about to terminate instead of immediately rescheduling ourselves?
>>
>> Yes, it is busy waiting, but I didn’t find that bad. The connection_co
>> will be invoked in basically every iteration, and once there is no
>> pending data, it will quit.
>>
>> The answer to “why not...” of course is because it’d be more complicated.
>>
>> But anyway.
>>
>> Adding a new function qemu_coroutine_run_after(target) that adds
>> qemu_coroutine_self() to the given @target coroutine’s wake-up queue and
>> then using that instead of scheduling works, too, yes.
>>
>> I don’t really like being responsible for coroutine code, though...
>>
>> (And maybe it’d be better to make it qemu_coroutine_yield_for(target),
>> which does the above and then yields?)
>
> Or just do something like this, which is arguably not only a fix for the
> busy wait, but also a code simplification:
1. Is that guaranteed to work? What if data sneaks in, the
connection_co handles that, and doesn’t wake up the teardown_co? Will
it be re-scheduled?
2. I precisely didn’t want to do this because we have this functionality
already in the form of Coroutine.co_queue_wakeup. Why duplicate it here?
Max
> diff --git a/block/nbd.c b/block/nbd.c
> index b83b6cd43e..c061bd1bfc 100644
> --- a/block/nbd.c
> +++ b/block/nbd.c
> @@ -61,6 +61,7 @@ typedef struct BDRVNBDState {
> CoMutex send_mutex;
> CoQueue free_sema;
> Coroutine *connection_co;
> + Coroutine *teardown_co;
> int in_flight;
>
> NBDClientRequest requests[MAX_NBD_REQUESTS];
> @@ -137,12 +138,9 @@ static void nbd_teardown_connection(BlockDriverState *bs)
> NULL);
>
> if (qemu_in_coroutine()) {
> - /* Let our caller poll and just yield until connection_co is done */
> - while (s->connection_co) {
> - aio_co_schedule(qemu_get_current_aio_context(),
> - qemu_coroutine_self());
> - qemu_coroutine_yield();
> - }
> + /* just yield until connection_co is done */
> + s->teardown_co = qemu_coroutine_self();
> + qemu_coroutine_yield();
> } else {
> BDRV_POLL_WHILE(bs, s->connection_co);
> }
> @@ -217,6 +215,9 @@ static coroutine_fn void nbd_connection_entry(void
> *opaque)
> bdrv_dec_in_flight(s->bs);
>
> s->connection_co = NULL;
> + if (s->teardown_co) {
> + aio_co_wake(s->teardown_co);
> + }
> aio_wait_kick();
> }
>
signature.asc
Description: OpenPGP digital signature
- [Qemu-devel] [RFC 0/5] block: Generic file truncation/creation fallbacks, Max Reitz, 2019/07/11
- [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close(), Max Reitz, 2019/07/11
- Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close(), Kevin Wolf, 2019/07/12
- Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close(), Max Reitz, 2019/07/12
- Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close(), Kevin Wolf, 2019/07/12
- Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close(),
Max Reitz <=
- Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close(), Kevin Wolf, 2019/07/12
- Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close(), Max Reitz, 2019/07/12
- Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close(), Kevin Wolf, 2019/07/12
[Qemu-devel] [RFC 3/5] block: Fall back to fallback truncate function, Max Reitz, 2019/07/11
[Qemu-devel] [RFC 2/5] block: Generic truncation fallback, Max Reitz, 2019/07/11