Re: [PATCH] qio: Inherit follow_coroutine_ctx across TLS

[Stefan Hajnoczi]

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 <stefanha@redhat.com>
> CC: Daniel P. Berrangé <berrange@redhat.com>
> CC: qemu-stable@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 <eblake@redhat.com>
> 
> ---
> 
> 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 <stefanha@redhat.com>

signature.asc
Description: PGP signature