Re: [Qemu-block] [PATCH] nbd/server: attach client channel to the export's AioContext

[Sergio Lopez]

Kevin Wolf <address@hidden> writes:

> Am 11.09.2019 um 18:15 hat Sergio Lopez geschrieben:
>> On creation, the export's AioContext is set to the same one as the
>> BlockBackend, while the AioContext in the client QIOChannel is left
>> untouched.
>> 
>> As a result, when using data-plane, nbd_client_receive_next_request()
>> schedules coroutines in the IOThread AioContext, while the client's
>> QIOChannel is serviced from the main_loop, potentially triggering the
>> assertion at qio_channel_restart_[read|write].
>> 
>> To fix this, as soon we have the export corresponding to the client,
>> we call qio_channel_attach_aio_context() to attach the QIOChannel
>> context to the export's AioContext. This matches with the logic in
>> blk_aio_attached().
>> 
>> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1748253
>> Signed-off-by: Sergio Lopez <address@hidden>
>
> Oh, looks like I only fixed switching the AioContext after the fact, but
> not starting the NBD server for a node that is already in a different
> AioContext... :-/
>
>> diff --git a/nbd/server.c b/nbd/server.c
>> index 10faedcfc5..51322e2343 100644
>> --- a/nbd/server.c
>> +++ b/nbd/server.c
>> @@ -471,6 +471,7 @@ static int nbd_negotiate_handle_export_name(NBDClient 
>> *client,
>>      QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
>>      nbd_export_get(client->exp);
>>      nbd_check_meta_export(client);
>> +    qio_channel_attach_aio_context(client->ioc, client->exp->ctx);
>>  
>>      return 0;
>>  }
>> @@ -673,6 +674,7 @@ static int nbd_negotiate_handle_info(NBDClient *client, 
>> uint16_t myflags,
>>          QTAILQ_INSERT_TAIL(&client->exp->clients, client, next);
>>          nbd_export_get(client->exp);
>>          nbd_check_meta_export(client);
>> +        qio_channel_attach_aio_context(client->ioc, exp->ctx);
>>          rc = 1;
>>      }
>>      return rc;
>
> I think I would rather do this once at the end of nbd_negotiate()
> instead of duplicating it across the different way to open an export.
> During the negotiation phase, we don't start requests yet, so doing
> everything from the main thread should be fine.

OK.

> Actually, not doing everything from the main thread sounds nasty because
> I think the next QIOChannel callback could then already be executed in
> the iothread while this one hasn't completed yet. Or do we have any
> locking in place for the negotiation?

This is the first time I look at NBD code, but IIUC all the negotiation
is done with synchronous nbd_[read|write]() calls, so even if the
coroutine yields due to EWOULDBLOCK, nothing else should be making
progress.

Sergio.

signature.asc
Description: PGP signature