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

[Kevin Wolf]

Am 12.09.2019 um 12:13 hat Sergio Lopez geschrieben:
> 
> 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.

Ah, yes, you're right. We don't even have fd handlers installed if we
aren't currently waiting for the coroutine to be re-entered. So as
everything is tied to the one coroutine, this should not be a problem.

Let's avoid the duplication anyway.

Kevin

signature.asc
Description: PGP signature