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: 2327 bytes --]

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?)

Max

>> +    } else {
>> +        BDRV_POLL_WHILE(bs, s->connection_co);
>> +    }
>>  
>>      nbd_client_detach_aio_context(bs);
>>      object_unref(OBJECT(s->sioc));
> 
> Kevin
> 



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