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

[Kevin Wolf]

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.

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?

Kevin