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();
--
2.21.0
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. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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 -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
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(); > > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
© 2016 - 2024 Red Hat, Inc.