qio: Add QIONetListener API for using AioContext

The user calling himself "John Doe" reported a deadlock when
attempting to use qemu-storage-daemon to serve both a base file over
NBD, and a qcow2 file with that NBD export as its backing file, from
the same process, even though it worked just fine when there were two
q-s-d processes.  The bulk of the NBD server code properly uses
coroutines to make progress in an event-driven manner, but the code
for spawning a new coroutine at the point when listen(2) detects a new
client was hard-coded to use the global GMainContext; in other words,
the callback that triggers nbd_client_new to let the server start the
negotiation sequence with the client requires the main loop to be
making progress.  However, the code for bdrv_open of a qcow2 image
with an NBD backing file uses an AIO_WAIT_WHILE nested event loop to
ensure that the entire qcow2 backing chain is either fully loaded or
rejected, without any side effects from the main loop causing unwanted
changes to the disk being loaded (in short, an AioContext represents
the set of actions that are known to be safe while handling block
layer I/O, while excluding any other pending actions in the global
main loop with potentially larger risk of unwanted side effects).

This creates a classic case of deadlock: the server can't progress to
the point of accept(2)ing the client to write to the NBD socket
because the main loop is being starved until the AIO_WAIT_WHILE
completes the bdrv_open, but the AIO_WAIT_WHILE can't progress because
it is blocked on the client coroutine stuck in a read() of the
expected magic number from the server side of the socket.

This patch adds a new API to allow clients to opt in to listening via
an AioContext rather than a GMainContext.  This will allow NBD to fix
the deadlock by performing all actions during bdrv_open in the main
loop AioContext.

Technical debt warning: I would have loved to utilize a notify
function with AioContext to guarantee that we don't finalize listener
due to an object_unref if there is any callback still running (the way
GSource does), but wiring up notify functions into AioContext is a
bigger task that will be deferred to a later QEMU release.  But for
solving the NBD deadlock, it is sufficient to note that the QMP
commands for enabling and disabling the NBD server are really the only
points where we want to change the listener's callback.  Furthermore,
those commands are serviced in the main loop, which is the same
AioContext that is also listening for connections.  Since a thread
cannot interrupt itself, we are ensured that at the point where we are
changing the watch, there are no callbacks active.  This is NOT as
powerful as the GSource cross-thread safety, but sufficient for the
needs of today.

An upcoming patch will then add a unit test (kept separate to make it
easier to rearrange the series to demonstrate the deadlock without
this patch).

Fixes: https://gitlab.com/qemu-project/qemu/-/issues/3169
Signed-off-by: Eric Blake <eblake@redhat.com>
Message-ID: <20251113011625.878876-26-eblake@redhat.com>
Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/include/io/net-listener.h b/include/io/net-listener.h
index 93608f7fe8998303cf0b532c9589deb720706212..fdc5f1c81adcb3b7db3250212673147ed3a74e07 100644 (file)
--- a/include/io/net-listener.h
+++ b/include/io/net-listener.h
@@ -152,6 +152,27 @@ void qio_net_listener_set_client_func(QIONetListener *listener,
                                       gpointer data,
                                       GDestroyNotify notify);
 
+/**
+ * qio_net_listener_set_client_aio_func:
+ * @listener: the network listener object
+ * @func: the callback function
+ * @data: opaque data to pass to @func
+ * @context: AioContext that @func will be bound to; if NULL, this will
+ *           will use qemu_get_aio_context().
+ *
+ * Similar to qio_net_listener_set_client_func_full(), except that the polling
+ * will be done by an AioContext rather than a GMainContext.
+ *
+ * Because AioContext does not (yet) support a clean way to deregister
+ * a callback from one thread while another thread might be in that
+ * callback, this function is only safe to call from the thread
+ * currently associated with @context.
+ */
+void qio_net_listener_set_client_aio_func(QIONetListener *listener,
+                                          QIONetListenerClientFunc func,
+                                          void *data,
+                                          AioContext *context);
+
 /**
  * qio_net_listener_wait_client:
  * @listener: the network listener object

diff --git a/io/net-listener.c b/io/net-listener.c
index 49399ec926a59a6824a5fe4fcc906c29c34ac327..9410d72da9c261c3fcb1c743f9cf9c093ece8802 100644 (file)
--- a/io/net-listener.c
+++ b/io/net-listener.c
@@ -85,6 +85,17 @@ static gboolean qio_net_listener_channel_func(QIOChannel *ioc,
 }
 
 
+static void qio_net_listener_aio_func(void *opaque)
+{
+    QIONetListenerSource *data = opaque;
+
+    assert(data->io_source == NULL);
+    assert(data->listener->aio_context != NULL);
+    qio_net_listener_channel_func(QIO_CHANNEL(data->sioc), G_IO_IN,
+                                  data->listener);
+}
+
+
 int qio_net_listener_open_sync(QIONetListener *listener,
                                SocketAddress *addr,
                                int num,
@@ -157,8 +168,26 @@ qio_net_listener_watch(QIONetListener *listener, size_t i, const char *caller)
                 qio_net_listener_channel_func,
                 listener, (GDestroyNotify)object_unref, listener->context);
         } else {
-            /* The user passed an AioContext. Not supported yet. */
-            g_assert_not_reached();
+            /*
+             * The user passed an AioContext.  At this point,
+             * AioContext lacks a clean way to call a notify function
+             * to release a final reference after any callback is
+             * complete.  But we asserted earlier that the async
+             * callback is changed only from the thread associated
+             * with aio_context, which means no other thread is in the
+             * middle of running the callback when we are changing the
+             * refcount on listener here.  Therefore, a single
+             * reference here is sufficient to ensure listener is not
+             * finalized during the callback.
+             */
+            assert(listener->context == NULL);
+            if (i == 0) {
+                object_ref(OBJECT(listener));
+            }
+            qio_channel_set_aio_fd_handler(
+                QIO_CHANNEL(listener->source[i]->sioc),
+                listener->aio_context, qio_net_listener_aio_func,
+                NULL, NULL, listener->source[i]);
         }
     }
 }
@@ -184,7 +213,13 @@ qio_net_listener_unwatch(QIONetListener *listener, const char *caller)
                 listener->source[i]->io_source = NULL;
             }
         } else {
-            g_assert_not_reached();
+            assert(listener->context == NULL);
+            qio_channel_set_aio_fd_handler(
+                QIO_CHANNEL(listener->source[i]->sioc),
+                listener->aio_context, NULL, NULL, NULL, NULL);
+            if (i == listener->nsioc - 1) {
+                object_unref(OBJECT(listener));
+            }
         }
     }
 }
@@ -259,6 +294,31 @@ void qio_net_listener_set_client_func(QIONetListener *listener,
                                               notify, NULL, NULL);
 }
 
+void qio_net_listener_set_client_aio_func(QIONetListener *listener,
+                                          QIONetListenerClientFunc func,
+                                          void *data,
+                                          AioContext *context)
+{
+    if (!context) {
+        assert(qemu_in_main_thread());
+        context = qemu_get_aio_context();
+    } else {
+        /*
+         * TODO: The API was intentionally designed to allow a caller
+         * to pass an alternative AioContext for future expansion;
+         * however, actually implementating that is not possible
+         * without notify callbacks wired into AioContext similar to
+         * how they work in GSource.  So for now, this code hard-codes
+         * the knowledge that the only client needing AioContext is
+         * the NBD server, which uses the global context and does not
+         * suffer from cross-thread safety issues.
+         */
+        g_assert_not_reached();
+    }
+    qio_net_listener_set_client_func_internal(listener, func, data,
+                                              NULL, NULL, context);
+}
+
 struct QIONetListenerClientWaitData {
     QIOChannelSocket *sioc;
     GMainLoop *loop;