On Wed, May 28, 2025 at 05:12:14PM -0500, Eric Blake wrote:
> On Wed, May 28, 2025 at 03:09:13PM -0400, 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 <stefa...@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);
>
> Why change the return type to void? That forces you to have to check
> errp. If you still return bool (true for errp unchanged, false when
> errp set), callers might have a simpler interface.
QEMU has both forms. I prefer void foo(Error **errp) because it
eliminates these awkward states:
1. Return true with errp set. There is a risk of leaking errp here.
2. Return false with errp NULL. This results in no error message.
Sometimes it is handy to have a return value but I think that void is a
good default return type.
I have CCed Markus in case he has suggestions.
>
> > 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 */
>
> Really? I thought that was part of why we added ERRP_GUARD, so that
> error_abort is pinned closer to the spot where the error is triggered
> rather than where it was chained. But your use of errp is a bit
> different than usual here, so you may be correct that it doesn't
> handle error_abort in the way that you want (allowing a graceful
> downgrade to epoll if it is the first failure, but aborting if it is
> the second AioContext that fails).
The ERRP_GUARD() doc comment explains why it behaves this way:
* Note: &error_abort is not rewritten, because that would move the
* abort from the place where the error is created to the place where
* it's propagated.
>
> > +
> > + /* 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 */
> >
> > 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;
>
> This is where I question whether you should still return bool.
>
> > }
> >
> > 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)
> > --
> > 2.49.0
> >
> >
>
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.
> Virtualization: qemu.org | libguestfs.org
>
signature.asc
Description: PGP signature
