Re: [RFC 08/11] aio-posix: gracefully handle io_uring_queue_init() failure

[Brian <hibriansong@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5284 bytes --]

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<stefanha@redhat.com>
> ---
>   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.
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)

[-- Attachment #2: Type: text/html, Size: 6457 bytes --]