Re: [PATCH] qio: Inherit follow_coroutine_ctx across TLS
On Wed, May 15, 2024 at 09:14:06PM -0500, Eric Blake wrote:
> Since qemu 8.2, the combination of NBD + TLS + iothread crashes on an
> assertion failure:
>
> qemu-kvm: ../io/channel.c:534: void qio_channel_restart_read(void *):
> Assertion `qemu_get_current_aio_context() ==
> qemu_coroutine_get_aio_context(co)' failed.
>
> It turns out that when we removed AioContext locking, we did so by
> having NBD tell its qio channels that it wanted to opt in to
> qio_channel_set_follow_coroutine_ctx(); but while we opted in on the
> main channel, we did not opt in on the TLS wrapper channel.
> qemu-iotests has coverage of NBD+iothread and NBD+TLS, but apparently
> no coverage of NBD+TLS+iothread, or we would have noticed this
> regression sooner. (I'll add that in the next patch)
>
> But while we could manually opt in to the TLS thread in nbd/server.c,
> it is more generic if all qio channels that wrap other channels
> inherit the follow status, in the same way that they inherit feature
> bits.
>
> CC: Stefan Hajnoczi
> CC: Daniel P. Berrangé
> CC: qemu-sta...@nongnu.org
> Fixes: https://issues.redhat.com/browse/RHEL-34786
> Fixes: 06e0f098 ("io: follow coroutine AioContext in qio_channel_yield()",
> v8.2.0)
> Signed-off-by: Eric Blake
Reviewed-by: Daniel P. Berrangé
> ---
>
> Maybe we should turn ioc->follow_coroutine_ctx into a
> QIO_CHANNEL_FEATURE_* bit?
The features thus far have all been about properties of the underlying
channel, rather than properties related to the API usage pattern. So
I think it is fine to have it separate.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] qio: Inherit follow_coroutine_ctx across TLS
On Wed, May 15, 2024 at 09:14:06PM -0500, Eric Blake wrote:
> Since qemu 8.2, the combination of NBD + TLS + iothread crashes on an
> assertion failure:
>
> qemu-kvm: ../io/channel.c:534: void qio_channel_restart_read(void *):
> Assertion `qemu_get_current_aio_context() ==
> qemu_coroutine_get_aio_context(co)' failed.
>
> It turns out that when we removed AioContext locking, we did so by
> having NBD tell its qio channels that it wanted to opt in to
> qio_channel_set_follow_coroutine_ctx(); but while we opted in on the
> main channel, we did not opt in on the TLS wrapper channel.
> qemu-iotests has coverage of NBD+iothread and NBD+TLS, but apparently
> no coverage of NBD+TLS+iothread, or we would have noticed this
> regression sooner. (I'll add that in the next patch)
>
> But while we could manually opt in to the TLS thread in nbd/server.c,
> it is more generic if all qio channels that wrap other channels
> inherit the follow status, in the same way that they inherit feature
> bits.
>
> CC: Stefan Hajnoczi
> CC: Daniel P. Berrangé
> CC: qemu-sta...@nongnu.org
> Fixes: https://issues.redhat.com/browse/RHEL-34786
> Fixes: 06e0f098 ("io: follow coroutine AioContext in qio_channel_yield()",
> v8.2.0)
> Signed-off-by: Eric Blake
>
> ---
>
> Maybe we should turn ioc->follow_coroutine_ctx into a
> QIO_CHANNEL_FEATURE_* bit?
It seems like existing feature bits are for characteristics inherent in
the specific channel, like whether it can pass file descriptors.
Following the coroutine AioContext is not an inherent in the specific
channel, it's something that can be toggled at runtime on any channel.
If larger changes are being considered, I would look into always
following the coroutine's AioContext and getting rid of the API to
toggle this behavior completely. From commit 06e0f098d612's description:
While the API is has become simpler, there is one wart: QIOChannel has a
special case for the iohandler AioContext (used for handlers that must not run
in nested event loops). I didn't find an elegant way preserve that behavior,
so
I added a new API called qio_channel_set_follow_coroutine_ctx(ioc, true|false)
for opting in to the new AioContext model. By default QIOChannel uses the
iohandler AioHandler. Code that formerly called
qio_channel_attach_aio_context() now calls
qio_channel_set_follow_coroutine_ctx(ioc, true) once after the QIOChannel is
created.
> And I have not yet written the promised qemu-iotests patch, but I
> wanted to get this on the list before I'm offline for a week.
>
> ---
> io/channel-tls.c | 26 +++---
> io/channel-websock.c | 1 +
> 2 files changed, 16 insertions(+), 11 deletions(-)
Reviewed-by: Stefan Hajnoczi
signature.asc
Description: PGP signature
[PATCH] qio: Inherit follow_coroutine_ctx across TLS
Since qemu 8.2, the combination of NBD + TLS + iothread crashes on an
assertion failure:
qemu-kvm: ../io/channel.c:534: void qio_channel_restart_read(void *): Assertion
`qemu_get_current_aio_context() == qemu_coroutine_get_aio_context(co)' failed.
It turns out that when we removed AioContext locking, we did so by
having NBD tell its qio channels that it wanted to opt in to
qio_channel_set_follow_coroutine_ctx(); but while we opted in on the
main channel, we did not opt in on the TLS wrapper channel.
qemu-iotests has coverage of NBD+iothread and NBD+TLS, but apparently
no coverage of NBD+TLS+iothread, or we would have noticed this
regression sooner. (I'll add that in the next patch)
But while we could manually opt in to the TLS thread in nbd/server.c,
it is more generic if all qio channels that wrap other channels
inherit the follow status, in the same way that they inherit feature
bits.
CC: Stefan Hajnoczi
CC: Daniel P. Berrangé
CC: qemu-sta...@nongnu.org
Fixes: https://issues.redhat.com/browse/RHEL-34786
Fixes: 06e0f098 ("io: follow coroutine AioContext in qio_channel_yield()",
v8.2.0)
Signed-off-by: Eric Blake
---
Maybe we should turn ioc->follow_coroutine_ctx into a
QIO_CHANNEL_FEATURE_* bit?
And I have not yet written the promised qemu-iotests patch, but I
wanted to get this on the list before I'm offline for a week.
---
io/channel-tls.c | 26 +++---
io/channel-websock.c | 1 +
2 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/io/channel-tls.c b/io/channel-tls.c
index 1d9c9c72bfb..67b9760 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -69,37 +69,40 @@ qio_channel_tls_new_server(QIOChannel *master,
const char *aclname,
Error **errp)
{
-QIOChannelTLS *ioc;
+QIOChannelTLS *tioc;
+QIOChannel *ioc;
-ioc = QIO_CHANNEL_TLS(object_new(TYPE_QIO_CHANNEL_TLS));
+tioc = QIO_CHANNEL_TLS(object_new(TYPE_QIO_CHANNEL_TLS));
+ioc = QIO_CHANNEL(tioc);
-ioc->master = master;
+tioc->master = master;
+ioc->follow_coroutine_ctx = master->follow_coroutine_ctx;
if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
-qio_channel_set_feature(QIO_CHANNEL(ioc),
QIO_CHANNEL_FEATURE_SHUTDOWN);
+qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
}
object_ref(OBJECT(master));
-ioc->session = qcrypto_tls_session_new(
+tioc->session = qcrypto_tls_session_new(
creds,
NULL,
aclname,
QCRYPTO_TLS_CREDS_ENDPOINT_SERVER,
errp);
-if (!ioc->session) {
+if (!tioc->session) {
goto error;
}
qcrypto_tls_session_set_callbacks(
-ioc->session,
+tioc->session,
qio_channel_tls_write_handler,
qio_channel_tls_read_handler,
-ioc);
+tioc);
-trace_qio_channel_tls_new_server(ioc, master, creds, aclname);
-return ioc;
+trace_qio_channel_tls_new_server(tioc, master, creds, aclname);
+return tioc;
error:
-object_unref(OBJECT(ioc));
+object_unref(OBJECT(tioc));
return NULL;
}
@@ -116,6 +119,7 @@ qio_channel_tls_new_client(QIOChannel *master,
ioc = QIO_CHANNEL(tioc);
tioc->master = master;
+ioc->follow_coroutine_ctx = master->follow_coroutine_ctx;
if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
}
diff --git a/io/channel-websock.c b/io/channel-websock.c
index a12acc27cf2..de39f0d182d 100644
--- a/io/channel-websock.c
+++ b/io/channel-websock.c
@@ -883,6 +883,7 @@ qio_channel_websock_new_server(QIOChannel *master)
ioc = QIO_CHANNEL(wioc);
wioc->master = master;
+ioc->follow_coroutine_ctx = master->follow_coroutine_ctx;
if (qio_channel_has_feature(master, QIO_CHANNEL_FEATURE_SHUTDOWN)) {
qio_channel_set_feature(ioc, QIO_CHANNEL_FEATURE_SHUTDOWN);
}
base-commit: 922582ace2df59572a671f5c0c5c6c5c706995e5
--
2.45.0
