[PATCH 0/2] aio-posix: fix fdmon-io_uring memory leak

[PATCH 0/2] aio-posix: fix fdmon-io_uring memory leak

[Stefan Hajnoczi]

This bug was introduced in QEMU 5.0 and causes guests to slow down because
AioHandlers are not freed when the fdmon-io_uring file descriptor monitoring
implementation is used by the main loop thread's glib event loop. This issue
does not apply to IOThread usage of fdmon-io_uring.

In practice few distros build with io_uring support enabled at the moment, so
the number of affected users is likely to be small. The fix is still suitable
for a stable release though.

https://bugs.launchpad.net/qemu/+bug/1877716
https://bugs.launchpad.net/qemu/+bug/1873032

Stefan Hajnoczi (2):
  aio-posix: don't duplicate fd handler deletion in
    fdmon_io_uring_destroy()
  aio-posix: disable fdmon-io_uring when GSource is used

 include/block/aio.h   |  3 +++
 util/aio-posix.c      | 13 +++++++++++++
 util/aio-win32.c      |  4 ++++
 util/async.c          |  1 +
 util/fdmon-io_uring.c | 13 ++++++++++---
 5 files changed, 31 insertions(+), 3 deletions(-)

-- 
2.25.3

[PATCH 1/2] aio-posix: don't duplicate fd handler deletion in fdmon_io_uring_destroy()

[Stefan Hajnoczi]

The io_uring file descriptor monitoring implementation has an internal
list of fd handlers that are pending submission to io_uring.
fdmon_io_uring_destroy() deletes all fd handlers on the list.

Don't delete fd handlers directly in fdmon_io_uring_destroy() for two
reasons:
1. This duplicates the aio-posix.c AioHandler deletion code and could
   become outdated if the struct changes.
2. Only handlers with the FDMON_IO_URING_REMOVE flag set are safe to
   remove. If the flag is not set then something still has a pointer to
   the fd handler. Let aio-posix.c and its user worry about that. In
   practice this isn't an issue because fdmon_io_uring_destroy() is only
   called when shutting down so all users have removed their fd
   handlers, but the next patch will need this!

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/aio-posix.c      |  1 +
 util/fdmon-io_uring.c | 13 ++++++++++---
 2 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/util/aio-posix.c b/util/aio-posix.c
index c3613d299e..8af334ab19 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -679,6 +679,7 @@ void aio_context_destroy(AioContext *ctx)
 {
     fdmon_io_uring_destroy(ctx);
     fdmon_epoll_disable(ctx);
+    aio_free_deleted_handlers(ctx);
 }
 
 void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
diff --git a/util/fdmon-io_uring.c b/util/fdmon-io_uring.c
index d5a80ed6fb..1d14177df0 100644
--- a/util/fdmon-io_uring.c
+++ b/util/fdmon-io_uring.c
@@ -342,11 +342,18 @@ void fdmon_io_uring_destroy(AioContext *ctx)
 
         io_uring_queue_exit(&ctx->fdmon_io_uring);
 
-        /* No need to submit these anymore, just free them. */
+        /* Move handlers due to be removed onto the deleted list */
         while ((node = QSLIST_FIRST_RCU(&ctx->submit_list))) {
+            unsigned flags = atomic_fetch_and(&node->flags,
+                    ~(FDMON_IO_URING_PENDING |
+                      FDMON_IO_URING_ADD |
+                      FDMON_IO_URING_REMOVE));
+
+            if (flags & FDMON_IO_URING_REMOVE) {
+                QLIST_INSERT_HEAD_RCU(&ctx->deleted_aio_handlers, node, node_deleted);
+            }
+
             QSLIST_REMOVE_HEAD_RCU(&ctx->submit_list, node_submitted);
-            QLIST_REMOVE(node, node);
-            g_free(node);
         }
 
         ctx->fdmon_ops = &fdmon_poll_ops;
-- 
2.25.3

Re: [PATCH 1/2] aio-posix: don't duplicate fd handler deletion in fdmon_io_uring_destroy()

[Oleksandr Natalenko]

On Mon, May 11, 2020 at 07:36:29PM +0100, Stefan Hajnoczi wrote:
> The io_uring file descriptor monitoring implementation has an internal
> list of fd handlers that are pending submission to io_uring.
> fdmon_io_uring_destroy() deletes all fd handlers on the list.
> 
> Don't delete fd handlers directly in fdmon_io_uring_destroy() for two
> reasons:
> 1. This duplicates the aio-posix.c AioHandler deletion code and could
>    become outdated if the struct changes.
> 2. Only handlers with the FDMON_IO_URING_REMOVE flag set are safe to
>    remove. If the flag is not set then something still has a pointer to
>    the fd handler. Let aio-posix.c and its user worry about that. In
>    practice this isn't an issue because fdmon_io_uring_destroy() is only
>    called when shutting down so all users have removed their fd
>    handlers, but the next patch will need this!
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  util/aio-posix.c      |  1 +
>  util/fdmon-io_uring.c | 13 ++++++++++---
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/util/aio-posix.c b/util/aio-posix.c
> index c3613d299e..8af334ab19 100644
> --- a/util/aio-posix.c
> +++ b/util/aio-posix.c
> @@ -679,6 +679,7 @@ void aio_context_destroy(AioContext *ctx)
>  {
>      fdmon_io_uring_destroy(ctx);
>      fdmon_epoll_disable(ctx);
> +    aio_free_deleted_handlers(ctx);
>  }
>  
>  void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
> diff --git a/util/fdmon-io_uring.c b/util/fdmon-io_uring.c
> index d5a80ed6fb..1d14177df0 100644
> --- a/util/fdmon-io_uring.c
> +++ b/util/fdmon-io_uring.c
> @@ -342,11 +342,18 @@ void fdmon_io_uring_destroy(AioContext *ctx)
>  
>          io_uring_queue_exit(&ctx->fdmon_io_uring);
>  
> -        /* No need to submit these anymore, just free them. */
> +        /* Move handlers due to be removed onto the deleted list */
>          while ((node = QSLIST_FIRST_RCU(&ctx->submit_list))) {
> +            unsigned flags = atomic_fetch_and(&node->flags,
> +                    ~(FDMON_IO_URING_PENDING |
> +                      FDMON_IO_URING_ADD |
> +                      FDMON_IO_URING_REMOVE));
> +
> +            if (flags & FDMON_IO_URING_REMOVE) {
> +                QLIST_INSERT_HEAD_RCU(&ctx->deleted_aio_handlers, node, node_deleted);
> +            }
> +
>              QSLIST_REMOVE_HEAD_RCU(&ctx->submit_list, node_submitted);
> -            QLIST_REMOVE(node, node);
> -            g_free(node);
>          }
>  
>          ctx->fdmon_ops = &fdmon_poll_ops;

Tested-by: Oleksandr Natalenko <oleksandr@redhat.com>

(run Windows 10 VM with storage accessible via io_uring on qemu v5.0.0
with these 2 patches)

Thank you.

-- 
  Best regards,
    Oleksandr Natalenko (post-factum)
    Principal Software Maintenance Engineer

[PATCH 2/2] aio-posix: disable fdmon-io_uring when GSource is used

[Stefan Hajnoczi]

The glib event loop does not call fdmon_io_uring_wait() so fd handlers
waiting to be submitted build up in the list. There is no benefit is
using io_uring when the glib GSource is being used, so disable it
instead of implementing a more complex fix.

This fixes a memory leak where AioHandlers would build up and increasing
amounts of CPU time were spent iterating them in aio_pending(). The
symptom is that guests become slow when QEMU is built with io_uring
support.

Buglink: https://bugs.launchpad.net/qemu/+bug/1877716
Fixes: 73fd282e7b6dd4e4ea1c3bbb3d302c8db51e4ccf ("aio-posix: add io_uring fd monitoring implementation")
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/block/aio.h |  3 +++
 util/aio-posix.c    | 12 ++++++++++++
 util/aio-win32.c    |  4 ++++
 util/async.c        |  1 +
 4 files changed, 20 insertions(+)

diff --git a/include/block/aio.h b/include/block/aio.h
index 62ed954344..b2f703fa3f 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -701,6 +701,9 @@ void aio_context_setup(AioContext *ctx);
  */
 void aio_context_destroy(AioContext *ctx);
 
+/* Used internally, do not call outside AioContext code */
+void aio_context_use_g_source(AioContext *ctx);
+
 /**
  * aio_context_set_poll_params:
  * @ctx: the aio context
diff --git a/util/aio-posix.c b/util/aio-posix.c
index 8af334ab19..1b2a3af65b 100644
--- a/util/aio-posix.c
+++ b/util/aio-posix.c
@@ -682,6 +682,18 @@ void aio_context_destroy(AioContext *ctx)
     aio_free_deleted_handlers(ctx);
 }
 
+void aio_context_use_g_source(AioContext *ctx)
+{
+    /*
+     * Disable io_uring when the glib main loop is used because it doesn't
+     * support mixed glib/aio_poll() usage. It relies on aio_poll() being
+     * called regularly so that changes to the monitored file descriptors are
+     * submitted, otherwise a list of pending fd handlers builds up.
+     */
+    fdmon_io_uring_destroy(ctx);
+    aio_free_deleted_handlers(ctx);
+}
+
 void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
                                  int64_t grow, int64_t shrink, Error **errp)
 {
diff --git a/util/aio-win32.c b/util/aio-win32.c
index 729d533faf..953c56ab48 100644
--- a/util/aio-win32.c
+++ b/util/aio-win32.c
@@ -414,6 +414,10 @@ void aio_context_destroy(AioContext *ctx)
 {
 }
 
+void aio_context_use_g_source(AioContext *ctx)
+{
+}
+
 void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
                                  int64_t grow, int64_t shrink, Error **errp)
 {
diff --git a/util/async.c b/util/async.c
index 3165a28f2f..1319eee3bc 100644
--- a/util/async.c
+++ b/util/async.c
@@ -362,6 +362,7 @@ static GSourceFuncs aio_source_funcs = {
 
 GSource *aio_get_g_source(AioContext *ctx)
 {
+    aio_context_use_g_source(ctx);
     g_source_ref(&ctx->source);
     return &ctx->source;
 }
-- 
2.25.3

Re: [PATCH 2/2] aio-posix: disable fdmon-io_uring when GSource is used

[Oleksandr Natalenko]

On Mon, May 11, 2020 at 07:36:30PM +0100, Stefan Hajnoczi wrote:
> The glib event loop does not call fdmon_io_uring_wait() so fd handlers
> waiting to be submitted build up in the list. There is no benefit is
> using io_uring when the glib GSource is being used, so disable it
> instead of implementing a more complex fix.
> 
> This fixes a memory leak where AioHandlers would build up and increasing
> amounts of CPU time were spent iterating them in aio_pending(). The
> symptom is that guests become slow when QEMU is built with io_uring
> support.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1877716
> Fixes: 73fd282e7b6dd4e4ea1c3bbb3d302c8db51e4ccf ("aio-posix: add io_uring fd monitoring implementation")
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  include/block/aio.h |  3 +++
>  util/aio-posix.c    | 12 ++++++++++++
>  util/aio-win32.c    |  4 ++++
>  util/async.c        |  1 +
>  4 files changed, 20 insertions(+)
> 
> diff --git a/include/block/aio.h b/include/block/aio.h
> index 62ed954344..b2f703fa3f 100644
> --- a/include/block/aio.h
> +++ b/include/block/aio.h
> @@ -701,6 +701,9 @@ void aio_context_setup(AioContext *ctx);
>   */
>  void aio_context_destroy(AioContext *ctx);
>  
> +/* Used internally, do not call outside AioContext code */
> +void aio_context_use_g_source(AioContext *ctx);
> +
>  /**
>   * aio_context_set_poll_params:
>   * @ctx: the aio context
> diff --git a/util/aio-posix.c b/util/aio-posix.c
> index 8af334ab19..1b2a3af65b 100644
> --- a/util/aio-posix.c
> +++ b/util/aio-posix.c
> @@ -682,6 +682,18 @@ void aio_context_destroy(AioContext *ctx)
>      aio_free_deleted_handlers(ctx);
>  }
>  
> +void aio_context_use_g_source(AioContext *ctx)
> +{
> +    /*
> +     * Disable io_uring when the glib main loop is used because it doesn't
> +     * support mixed glib/aio_poll() usage. It relies on aio_poll() being
> +     * called regularly so that changes to the monitored file descriptors are
> +     * submitted, otherwise a list of pending fd handlers builds up.
> +     */
> +    fdmon_io_uring_destroy(ctx);
> +    aio_free_deleted_handlers(ctx);
> +}
> +
>  void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
>                                   int64_t grow, int64_t shrink, Error **errp)
>  {
> diff --git a/util/aio-win32.c b/util/aio-win32.c
> index 729d533faf..953c56ab48 100644
> --- a/util/aio-win32.c
> +++ b/util/aio-win32.c
> @@ -414,6 +414,10 @@ void aio_context_destroy(AioContext *ctx)
>  {
>  }
>  
> +void aio_context_use_g_source(AioContext *ctx)
> +{
> +}
> +
>  void aio_context_set_poll_params(AioContext *ctx, int64_t max_ns,
>                                   int64_t grow, int64_t shrink, Error **errp)
>  {
> diff --git a/util/async.c b/util/async.c
> index 3165a28f2f..1319eee3bc 100644
> --- a/util/async.c
> +++ b/util/async.c
> @@ -362,6 +362,7 @@ static GSourceFuncs aio_source_funcs = {
>  
>  GSource *aio_get_g_source(AioContext *ctx)
>  {
> +    aio_context_use_g_source(ctx);
>      g_source_ref(&ctx->source);
>      return &ctx->source;
>  }

Tested-by: Oleksandr Natalenko <oleksandr@redhat.com>

(run Windows 10 VM with storage accessible via io_uring on qemu v5.0.0
with these 2 patches)

Thank you.

-- 
  Best regards,
    Oleksandr Natalenko (post-factum)
    Principal Software Maintenance Engineer

Re: [PATCH 0/2] aio-posix: fix fdmon-io_uring memory leak

[Stefan Hajnoczi]

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

On Mon, May 11, 2020 at 07:36:28PM +0100, Stefan Hajnoczi wrote:
> This bug was introduced in QEMU 5.0 and causes guests to slow down because
> AioHandlers are not freed when the fdmon-io_uring file descriptor monitoring
> implementation is used by the main loop thread's glib event loop. This issue
> does not apply to IOThread usage of fdmon-io_uring.
> 
> In practice few distros build with io_uring support enabled at the moment, so
> the number of affected users is likely to be small. The fix is still suitable
> for a stable release though.
> 
> https://bugs.launchpad.net/qemu/+bug/1877716
> https://bugs.launchpad.net/qemu/+bug/1873032
> 
> Stefan Hajnoczi (2):
>   aio-posix: don't duplicate fd handler deletion in
>     fdmon_io_uring_destroy()
>   aio-posix: disable fdmon-io_uring when GSource is used
> 
>  include/block/aio.h   |  3 +++
>  util/aio-posix.c      | 13 +++++++++++++
>  util/aio-win32.c      |  4 ++++
>  util/async.c          |  1 +
>  util/fdmon-io_uring.c | 13 ++++++++++---
>  5 files changed, 31 insertions(+), 3 deletions(-)

This has been merged into qemu.git/master.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]