[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: |
Sergio Lopez |
Subject: |
Re: [Qemu-block] [PATCH] nbd/server: attach client channel to the export's AioContext |
Date: |
Thu, 12 Sep 2019 12:13:04 +0200 |
User-agent: |
mu4e 1.2.0; emacs 26.2 |
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
- 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, 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
- Re: [Qemu-block] [PATCH] nbd/server: attach client channel to the export's AioContext,
Sergio Lopez <=