| Message ID | 20190912110032.26395-1-slp@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] nbd/server: attach client channel to the export's AioContext | expand |
On 9/12/19 6:00 AM, Sergio Lopez wrote: > 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 at > blk_aio_attached(). > > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1748253 > Signed-off-by: Sergio Lopez <slp@redhat.com> > --- > Changelog > > v2: > - Attach the channel once after negotiation completes, avoiding > duplication. (thanks Kevin Wolf). > --- > nbd/server.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/nbd/server.c b/nbd/server.c > index 28c3c8be85..31d624e146 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -1297,6 +1297,11 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp) > return ret; > } > > + /* Attach the channel to the same AioContext as the export */ > + if (client->exp && client->exp->ctx) { > + qio_channel_attach_aio_context(client->ioc, client->exp->ctx); > + } > + Reviewed-by: Eric Blake <eblake@redhat.com> Will queue through my NBD tree. > assert(!client->optlen); > trace_nbd_negotiate_success(); > >
On 9/12/19 7:00 AM, Sergio Lopez wrote: > 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 at > blk_aio_attached(). > > RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1748253 > Signed-off-by: Sergio Lopez <slp@redhat.com> > --- > Changelog > > v2: > - Attach the channel once after negotiation completes, avoiding > duplication. (thanks Kevin Wolf). > --- > nbd/server.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/nbd/server.c b/nbd/server.c > index 28c3c8be85..31d624e146 100644 > --- a/nbd/server.c > +++ b/nbd/server.c > @@ -1297,6 +1297,11 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp) > return ret; > } > > + /* Attach the channel to the same AioContext as the export */ > + if (client->exp && client->exp->ctx) { > + qio_channel_attach_aio_context(client->ioc, client->exp->ctx); > + } > + > assert(!client->optlen); > trace_nbd_negotiate_success(); > > I assume this patch has been superseded by Eric's later patches? --js
On 9/20/19 2:12 PM, John Snow wrote: > > > On 9/12/19 7:00 AM, Sergio Lopez wrote: >> 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 at >> blk_aio_attached(). >> >> RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1748253 >> Signed-off-by: Sergio Lopez <slp@redhat.com> >> --- >> Changelog >> >> v2: >> - Attach the channel once after negotiation completes, avoiding >> duplication. (thanks Kevin Wolf). >> --- >> nbd/server.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/nbd/server.c b/nbd/server.c >> index 28c3c8be85..31d624e146 100644 >> --- a/nbd/server.c >> +++ b/nbd/server.c >> @@ -1297,6 +1297,11 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp) >> return ret; >> } >> >> + /* Attach the channel to the same AioContext as the export */ >> + if (client->exp && client->exp->ctx) { >> + qio_channel_attach_aio_context(client->ioc, client->exp->ctx); >> + } >> + >> assert(!client->optlen); >> trace_nbd_negotiate_success(); >> >> > > I assume this patch has been superseded by Eric's later patches? Nevermind -- my filtering got messed up slightly and I missed the followup. I see that Eric staged this. --js
On 9/20/19 1:49 PM, John Snow wrote: > >>> 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 at >>> blk_aio_attached(). >>> >> >> I assume this patch has been superseded by Eric's later patches? > > Nevermind -- my filtering got messed up slightly and I missed the > followup. I see that Eric staged this. I actually think both patches are needed: this one covers transactions, while my later patch was on top of this to protect shutdown. But now you've made me curious; I'll see if my patch hoisted in front still solves everything, or if we really do need both.
On 9/20/19 2:11 PM, Eric Blake wrote: > On 9/20/19 1:49 PM, John Snow wrote: >> > >>>> 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 at >>>> blk_aio_attached(). >>>> > >>> >>> I assume this patch has been superseded by Eric's later patches? >> >> Nevermind -- my filtering got messed up slightly and I missed the >> followup. I see that Eric staged this. > > I actually think both patches are needed: this one covers transactions, > while my later patch was on top of this to protect shutdown. But now > you've made me curious; I'll see if my patch hoisted in front still > solves everything, or if we really do need both. > Nope, both patches are still needed. Sergio's fixes the assertion: (qemu) qemu-kvm: io/channel.c:411: qio_channel_restart_read: Assertion `qemu_get_current_aio_context() == qemu_coroutine_get_aio_context(co)' failed. while mine fixes: +qemu: qemu_mutex_unlock_impl: Operation not permitted
diff --git a/nbd/server.c b/nbd/server.c index 28c3c8be85..31d624e146 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1297,6 +1297,11 @@ static coroutine_fn int nbd_negotiate(NBDClient *client, Error **errp) return ret; } + /* Attach the channel to the same AioContext as the export */ + if (client->exp && client->exp->ctx) { + qio_channel_attach_aio_context(client->ioc, client->exp->ctx); + } + assert(!client->optlen); trace_nbd_negotiate_success();
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 at blk_aio_attached(). RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1748253 Signed-off-by: Sergio Lopez <slp@redhat.com> --- Changelog v2: - Attach the channel once after negotiation completes, avoiding duplication. (thanks Kevin Wolf). --- nbd/server.c | 5 +++++ 1 file changed, 5 insertions(+)