Re: [PATCH v3 11/13] qio: Add QIONetListener API for using AioContext

["Daniel P. Berrangé" <berrange@redhat.com>]

On Wed, Nov 12, 2025 at 04:31:11PM -0600, Eric Blake wrote:
> 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>
> 
> ---
> v2: Retitle and add new API rather than changing semantics of
> existing qio_net_listener_set_client_func; use qio accessor rather
> than direct access to the sioc fd and a lower-level aio call
> v3: limit reference counting change to just AioContext, R-b dropped
> ---
>  include/io/net-listener.h | 21 +++++++++++++
>  io/net-listener.c         | 66 +++++++++++++++++++++++++++++++++++++--
>  2 files changed, 84 insertions(+), 3 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|