Re: [PATCH v3 4/4] io: follow coroutine AioContext in qio_channel_yield()
On Thu, Sep 07, 2023 at 03:41:08PM -0500, Eric Blake wrote:
> On Wed, Aug 30, 2023 at 06:48:02PM -0400, Stefan Hajnoczi wrote:
> > The ongoing QEMU multi-queue block layer effort makes it possible for
> > multiple
> > threads to process I/O in parallel. The nbd block driver is not compatible
> > with
> > the multi-queue block layer yet because QIOChannel cannot be used easily
> > from
> > coroutines running in multiple threads. This series changes the QIOChannel
> > API
> > to make that possible.
> >
> ...
> >
> > This API change allows the nbd block driver to use QIOChannel from any
> > thread.
> > It's important to keep in mind that the block driver already synchronizes
> > QIOChannel access and ensures that two coroutines never read simultaneously
> > or
> > write simultaneously.
> >
> > This patch updates all users of qio_channel_attach_aio_context() to the
> > new API. Most conversions are simple, but vhost-user-server requires a
> > new qemu_coroutine_yield() call to quiesce the vu_client_trip()
> > coroutine when not attached to any AioContext.
> >
> > 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.
> >
> > Signed-off-by: Stefan Hajnoczi
> > Reviewed-by: Eric Blake
> > Acked-by: Daniel P. Berrangé
> > ---
> > include/io/channel-util.h| 23 ++
> > include/io/channel.h | 69 --
> > include/qemu/vhost-user-server.h | 1 +
> > block/nbd.c | 11 +--
> > io/channel-command.c | 10 ++-
> > io/channel-file.c| 9 ++-
> > io/channel-null.c| 3 +-
> > io/channel-socket.c | 9 ++-
> > io/channel-tls.c | 6 +-
> > io/channel-util.c| 24 +++
> > io/channel.c | 120 ++-
> > migration/channel-block.c| 3 +-
> > nbd/server.c | 14 +---
> > scsi/qemu-pr-helper.c| 4 +-
> > util/vhost-user-server.c | 27 +--
> > 15 files changed, 216 insertions(+), 117 deletions(-)
>
> Looks like migration/rdma.c is also impacted:
>
> ../migration/rdma.c: In function ‘qio_channel_rdma_class_init’:
> ../migration/rdma.c:4037:38: error: assignment to ‘void (*)(QIOChannel *,
> AioContext *, void (*)(void *), AioContext *, void (*)(void *), void *)’ from
> incompatible pointer type ‘void (*)(QIOChannel *, AioContext *, void (*)(void
> *), void (*)(void *), void *)’ [-Werror=incompatible-pointer-types]
> 4037 | ioc_klass->io_set_aio_fd_handler =
> qio_channel_rdma_set_aio_fd_handler;
> | ^
>
> I'm squashing this in:
>
> diff --git i/migration/rdma.c w/migration/rdma.c
> index ca430d319d9..a2a3db35b1d 100644
> --- i/migration/rdma.c
> +++ w/migration/rdma.c
> @@ -3103,22 +3103,23 @@ static GSource
> *qio_channel_rdma_create_watch(QIOChannel *ioc,
> }
>
> static void qio_channel_rdma_set_aio_fd_handler(QIOChannel *ioc,
> - AioContext *ctx,
> - IOHandler *io_read,
> - IOHandler *io_write,
> - void *opaque)
> +AioContext *read_ctx,
> +IOHandler *io_read,
> +AioContext *write_ctx,
> +IOHandler *io_write,
> +void *opaque)
> {
> QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(ioc);
> if (io_read) {
> -aio_set_fd_handler(ctx, rioc->rdmain->recv_comp_channel->fd, io_read,
> - io_write, NULL, NULL, opaque);
> -aio_set_fd_handler(ctx, rioc->rdmain->send_comp_channel->fd, io_read,
> - io_write, NULL, NULL, opaque);
> +aio_set_fd_handler(read_ctx, rioc->rdmain->recv_comp_channel->fd,
> + io_read, io_write, NULL, NULL, opaque);
> +aio_set_fd_handler(read_ctx, rioc->rdmain->send_comp_channel->fd,
> + io_read, io_write, NULL, NULL, opaque);
> } else {
> -aio_set_fd_handler(ctx, rioc->rdmaout->recv_comp_channel->fd,
> io_read,
> - io_write, NULL, NULL,
Re: [PATCH v3 4/4] io: follow coroutine AioContext in qio_channel_yield()
On Wed, Aug 30, 2023 at 06:48:02PM -0400, Stefan Hajnoczi wrote:
> The ongoing QEMU multi-queue block layer effort makes it possible for multiple
> threads to process I/O in parallel. The nbd block driver is not compatible
> with
> the multi-queue block layer yet because QIOChannel cannot be used easily from
> coroutines running in multiple threads. This series changes the QIOChannel API
> to make that possible.
>
...
>
> This API change allows the nbd block driver to use QIOChannel from any thread.
> It's important to keep in mind that the block driver already synchronizes
> QIOChannel access and ensures that two coroutines never read simultaneously or
> write simultaneously.
>
> This patch updates all users of qio_channel_attach_aio_context() to the
> new API. Most conversions are simple, but vhost-user-server requires a
> new qemu_coroutine_yield() call to quiesce the vu_client_trip()
> coroutine when not attached to any AioContext.
>
> 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.
>
> Signed-off-by: Stefan Hajnoczi
> Reviewed-by: Eric Blake
> Acked-by: Daniel P. Berrangé
> ---
> include/io/channel-util.h| 23 ++
> include/io/channel.h | 69 --
> include/qemu/vhost-user-server.h | 1 +
> block/nbd.c | 11 +--
> io/channel-command.c | 10 ++-
> io/channel-file.c| 9 ++-
> io/channel-null.c| 3 +-
> io/channel-socket.c | 9 ++-
> io/channel-tls.c | 6 +-
> io/channel-util.c| 24 +++
> io/channel.c | 120 ++-
> migration/channel-block.c| 3 +-
> nbd/server.c | 14 +---
> scsi/qemu-pr-helper.c| 4 +-
> util/vhost-user-server.c | 27 +--
> 15 files changed, 216 insertions(+), 117 deletions(-)
Looks like migration/rdma.c is also impacted:
../migration/rdma.c: In function ‘qio_channel_rdma_class_init’:
../migration/rdma.c:4037:38: error: assignment to ‘void (*)(QIOChannel *,
AioContext *, void (*)(void *), AioContext *, void (*)(void *), void *)’ from
incompatible pointer type ‘void (*)(QIOChannel *, AioContext *, void (*)(void
*), void (*)(void *), void *)’ [-Werror=incompatible-pointer-types]
4037 | ioc_klass->io_set_aio_fd_handler =
qio_channel_rdma_set_aio_fd_handler;
| ^
I'm squashing this in:
diff --git i/migration/rdma.c w/migration/rdma.c
index ca430d319d9..a2a3db35b1d 100644
--- i/migration/rdma.c
+++ w/migration/rdma.c
@@ -3103,22 +3103,23 @@ static GSource
*qio_channel_rdma_create_watch(QIOChannel *ioc,
}
static void qio_channel_rdma_set_aio_fd_handler(QIOChannel *ioc,
- AioContext *ctx,
- IOHandler *io_read,
- IOHandler *io_write,
- void *opaque)
+AioContext *read_ctx,
+IOHandler *io_read,
+AioContext *write_ctx,
+IOHandler *io_write,
+void *opaque)
{
QIOChannelRDMA *rioc = QIO_CHANNEL_RDMA(ioc);
if (io_read) {
-aio_set_fd_handler(ctx, rioc->rdmain->recv_comp_channel->fd, io_read,
- io_write, NULL, NULL, opaque);
-aio_set_fd_handler(ctx, rioc->rdmain->send_comp_channel->fd, io_read,
- io_write, NULL, NULL, opaque);
+aio_set_fd_handler(read_ctx, rioc->rdmain->recv_comp_channel->fd,
+ io_read, io_write, NULL, NULL, opaque);
+aio_set_fd_handler(read_ctx, rioc->rdmain->send_comp_channel->fd,
+ io_read, io_write, NULL, NULL, opaque);
} else {
-aio_set_fd_handler(ctx, rioc->rdmaout->recv_comp_channel->fd, io_read,
- io_write, NULL, NULL, opaque);
-aio_set_fd_handler(ctx, rioc->rdmaout->send_comp_channel->fd, io_read,
- io_write, NULL, NULL, opaque);
+aio_set_fd_handler(write_ctx, rioc->rdmaout->recv_comp_channel->fd,
+ io_read, io_write, NULL, NULL, opaque);
+
[PATCH v3 4/4] io: follow coroutine AioContext in qio_channel_yield()
The ongoing QEMU multi-queue block layer effort makes it possible for multiple
threads to process I/O in parallel. The nbd block driver is not compatible with
the multi-queue block layer yet because QIOChannel cannot be used easily from
coroutines running in multiple threads. This series changes the QIOChannel API
to make that possible.
In the current API, calling qio_channel_attach_aio_context() sets the
AioContext where qio_channel_yield() installs an fd handler prior to yielding:
qio_channel_attach_aio_context(ioc, my_ctx);
...
qio_channel_yield(ioc); // my_ctx is used here
...
qio_channel_detach_aio_context(ioc);
This API design has limitations: reading and writing must be done in the same
AioContext and moving between AioContexts involves a cumbersome sequence of API
calls that is not suitable for doing on a per-request basis.
There is no fundamental reason why a QIOChannel needs to run within the
same AioContext every time qio_channel_yield() is called. QIOChannel
only uses the AioContext while inside qio_channel_yield(). The rest of
the time, QIOChannel is independent of any AioContext.
In the new API, qio_channel_yield() queries the AioContext from the current
coroutine using qemu_coroutine_get_aio_context(). There is no need to
explicitly attach/detach AioContexts anymore and
qio_channel_attach_aio_context() and qio_channel_detach_aio_context() are gone.
One coroutine can read from the QIOChannel while another coroutine writes from
a different AioContext.
This API change allows the nbd block driver to use QIOChannel from any thread.
It's important to keep in mind that the block driver already synchronizes
QIOChannel access and ensures that two coroutines never read simultaneously or
write simultaneously.
This patch updates all users of qio_channel_attach_aio_context() to the
new API. Most conversions are simple, but vhost-user-server requires a
new qemu_coroutine_yield() call to quiesce the vu_client_trip()
coroutine when not attached to any AioContext.
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.
Signed-off-by: Stefan Hajnoczi
Reviewed-by: Eric Blake
Acked-by: Daniel P. Berrangé
---
include/io/channel-util.h| 23 ++
include/io/channel.h | 69 --
include/qemu/vhost-user-server.h | 1 +
block/nbd.c | 11 +--
io/channel-command.c | 10 ++-
io/channel-file.c| 9 ++-
io/channel-null.c| 3 +-
io/channel-socket.c | 9 ++-
io/channel-tls.c | 6 +-
io/channel-util.c| 24 +++
io/channel.c | 120 ++-
migration/channel-block.c| 3 +-
nbd/server.c | 14 +---
scsi/qemu-pr-helper.c| 4 +-
util/vhost-user-server.c | 27 +--
15 files changed, 216 insertions(+), 117 deletions(-)
diff --git a/include/io/channel-util.h b/include/io/channel-util.h
index a5d720d9a0..fa18a3756d 100644
--- a/include/io/channel-util.h
+++ b/include/io/channel-util.h
@@ -49,4 +49,27 @@
QIOChannel *qio_channel_new_fd(int fd,
Error **errp);
+/**
+ * qio_channel_util_set_aio_fd_handler:
+ * @read_fd: the file descriptor for the read handler
+ * @read_ctx: the AioContext for the read handler
+ * @io_read: the read handler
+ * @write_fd: the file descriptor for the write handler
+ * @write_ctx: the AioContext for the write handler
+ * @io_write: the write handler
+ * @opaque: the opaque argument to the read and write handler
+ *
+ * Set the read and write handlers when @read_ctx and @write_ctx are non-NULL,
+ * respectively. To leave a handler in its current state, pass a NULL
+ * AioContext. To clear a handler, pass a non-NULL AioContext and a NULL
+ * handler.
+ */
+void qio_channel_util_set_aio_fd_handler(int read_fd,
+ AioContext *read_ctx,
+ IOHandler *io_read,
+ int write_fd,
+ AioContext *write_ctx,
+ IOHandler *io_write,
+ void *opaque);
+
#endif /* QIO_CHANNEL_UTIL_H */
diff --git a/include/io/channel.h b/include/io/channel.h
index 229bf36910..5f9dbaab65 100644
--- a/include/io/channel.h
+++ b/include/io/channel.h
@@ -81,9 +81,11 @@ struct QIOChannel {
