Am 12.09.2019 um 12:13 hat Sergio Lopez geschrieben: > > Kevin Wolf <kw...@redhat.com> 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 <s...@redhat.com> > > > > 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