On Mon, Jun 02, 2025 at 08:26:39AM -0400, Brian wrote: > On 5/28/25 3:09 PM, Stefan Hajnoczi wrote: > > io_uring may not be available at runtime due to system policies (e.g. > > the io_uring_disabled sysctl) or creation could fail due to file > > descriptor resource limits. > > > > Handle failure scenarios as follows: > > > > If another AioContext already has io_uring, then fail AioContext > > creation so that the aio_add_sqe() API is available uniformly from all > > QEMU threads. Otherwise fall back to epoll(7) if io_uring is > > unavailable. > > > > Notes: > > - Update the comment about selecting the fastest fdmon implementation. > > At this point it's not about speed anymore, it's about aio_add_sqe() > > API availability. > > - Uppercase the error message when converting from error_report() to > > error_setg_errno() for consistency (but there are instances of > > lowercase in the codebase). > > - It's easier to move the #ifdefs from aio-posix.h to aio-posix.c. > > > > Signed-off-by: Stefan Hajnoczi<[email protected]> > > --- > > util/aio-posix.h | 12 ++---------- > > util/aio-posix.c | 29 ++++++++++++++++++++++++++--- > > util/fdmon-io_uring.c | 8 ++++---- > > 3 files changed, 32 insertions(+), 17 deletions(-) > > > > diff --git a/util/aio-posix.h b/util/aio-posix.h > > index f9994ed79e..6f9d97d866 100644 > > --- a/util/aio-posix.h > > +++ b/util/aio-posix.h > > @@ -18,6 +18,7 @@ > > #define AIO_POSIX_H > > #include "block/aio.h" > > +#include "qapi/error.h" struct AioHandler { GPollFD pfd; @@ -72,17 > > +73,8 @@ static inline void fdmon_epoll_disable(AioContext *ctx) #endif > > /* !CONFIG_EPOLL_CREATE1 */ #ifdef CONFIG_LINUX_IO_URING -bool > > fdmon_io_uring_setup(AioContext *ctx); +void > > fdmon_io_uring_setup(AioContext *ctx, Error **errp); void > > fdmon_io_uring_destroy(AioContext *ctx); -#else -static inline bool > > fdmon_io_uring_setup(AioContext *ctx) -{ - return false; -} - -static > > inline void fdmon_io_uring_destroy(AioContext *ctx) -{ -} #endif /* > > !CONFIG_LINUX_IO_URING */ #endif /* AIO_POSIX_H */ diff --git > > a/util/aio-posix.c b/util/aio-posix.c index fa047fc7ad..44b3df61f9 > > 100644 --- a/util/aio-posix.c +++ b/util/aio-posix.c @@ -16,6 +16,7 @@ > > #include "qemu/osdep.h" > > #include "block/block.h" > > #include "block/thread-pool.h" > > +#include "qapi/error.h" > > #include "qemu/main-loop.h" > > #include "qemu/lockcnt.h" > > #include "qemu/rcu.h" > > @@ -717,17 +718,39 @@ void aio_context_setup(AioContext *ctx, Error **errp) > > ctx->epollfd = -1; > > ctx->epollfd_tag = NULL; > > - /* Use the fastest fd monitoring implementation if available */ > > - if (fdmon_io_uring_setup(ctx)) { > > - return; > > +#ifdef CONFIG_LINUX_IO_URING > > + { > > + static bool need_io_uring; > > + Error *local_err = NULL; /* ERRP_GUARD() doesn't handle > > error_abort */ > > + > > + /* io_uring takes precedence because it provides aio_add_sqe() > > support */ > > + fdmon_io_uring_setup(ctx, &local_err); > > + if (!local_err) { > > + /* > > + * If one AioContext gets io_uring, then all AioContexts need > > io_uring > > + * so that aio_add_sqe() support is available across all > > threads. > > + */ > > + need_io_uring = true; > > + return; > > + } > > + if (need_io_uring) { > > + error_propagate(errp, local_err); > > + return; > > + } > > + > > + warn_report_err_once(local_err); /* frees local_err */ > > + local_err = NULL; > > } > > +#endif /* CONFIG_LINUX_IO_URING */ > Is there a problem with the logic of this code snippet? > > If we fail at fdmon_io_uring_setup, specifically at io_uring_queue_init, > local_err (or errp) will be set to a non-NULL error value. In that case, > need_io_uring will be set to true, but the function will return immediately.
If local_err is non-NULL then this conditional is not taken:
if (!local_err) {
/*
* If one AioContext gets io_uring, then all AioContexts need io_uring
* so that aio_add_sqe() support is available across all threads.
*/
need_io_uring = true;
return;
}
If the logic you described is correct, please rephrase it. I don't see
how what you've written can happen.
> As a result, the later if (need_io_uring) block will never be executed
>
> > fdmon_epoll_setup(ctx);
> > }
> > void aio_context_destroy(AioContext *ctx)
> > {
> > +#ifdef CONFIG_LINUX_IO_URING
> > fdmon_io_uring_destroy(ctx);
> > +#endif
> > qemu_lockcnt_lock(&ctx->list_lock);
> > fdmon_epoll_disable(ctx);
> > diff --git a/util/fdmon-io_uring.c b/util/fdmon-io_uring.c
> > index 2092d08d24..ef1a866a03 100644
> > --- a/util/fdmon-io_uring.c
> > +++ b/util/fdmon-io_uring.c
> > @@ -45,6 +45,7 @@
> > #include "qemu/osdep.h"
> > #include <poll.h>
> > +#include "qapi/error.h"
> > #include "qemu/rcu_queue.h"
> > #include "aio-posix.h"
> > @@ -361,7 +362,7 @@ static const FDMonOps fdmon_io_uring_ops = {
> > .gsource_dispatch = fdmon_io_uring_gsource_dispatch,
> > };
> > -bool fdmon_io_uring_setup(AioContext *ctx)
> > +void fdmon_io_uring_setup(AioContext *ctx, Error **errp)
> > {
> > int ret;
> > @@ -369,15 +370,14 @@ bool fdmon_io_uring_setup(AioContext *ctx)
> > ret = io_uring_queue_init(FDMON_IO_URING_ENTRIES,
> > &ctx->fdmon_io_uring, 0);
> > if (ret != 0) {
> > - return false;
> > + error_setg_errno(errp, -ret, "Failed to initialize io_uring");
> > + return;
> > }
> > QSLIST_INIT(&ctx->submit_list);
> > ctx->fdmon_ops = &fdmon_io_uring_ops;
> > ctx->io_uring_fd_tag = g_source_add_unix_fd(&ctx->source,
> > ctx->fdmon_io_uring.ring_fd, G_IO_IN);
> > -
> > - return true;
> > }
> > void fdmon_io_uring_destroy(AioContext *ctx)
signature.asc
Description: PGP signature
