Re: [Qemu-devel] [RFC 1/5] block/nbd: Fix hang in .bdrv_close()

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4100 bytes --]

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 <mreitz@redhat.com>
>>>> ---
>>>>  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();
>  }
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]