[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH] nbd/server: attach client channel to the export
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH] nbd/server: attach client channel to the export's AioContext |
Date: |
Thu, 12 Sep 2019 12:25:46 +0200 |
User-agent: |
Mutt/1.12.1 (2019-06-15) |
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
- Re: [Qemu-block] [PATCH] nbd/server: attach client channel to the export's AioContext, (continued)
- Re: [Qemu-block] [PATCH] nbd/server: attach client channel to the export's AioContext, Eric Blake, 2019/09/11
- Re: [Qemu-block] [PATCH] nbd/server: attach client channel to the export's AioContext, Kevin Wolf, 2019/09/12
- Re: [Qemu-block] [PATCH] nbd/server: attach client channel to the export's AioContext, Sergio Lopez, 2019/09/12
- Re: [Qemu-block] [PATCH] nbd/server: attach client channel to the export's AioContext, Kevin Wolf, 2019/09/12
- Re: [Qemu-block] [PATCH] nbd/server: attach client channel to the export's AioContext, Eric Blake, 2019/09/16
Re: [Qemu-block] [PATCH] nbd/server: attach client channel to the export's AioContext, Kevin Wolf, 2019/09/12