Re: [PATCH v2 04/19] nfs: Run co BH CB in the coroutine’s AioContext

[Kevin Wolf <kwolf@redhat.com>]

Am 06.03.2026 um 14:57 hat Peter Lieven geschrieben:
> 
> 
> > Am 06.03.2026 um 14:41 schrieb Hanna Czenczek <hreitz@redhat.com>:
> > 
> > On 03.03.26 20:24, Peter Lieven wrote:
> >> 
> >> 
> >>> Am 10.11.2025 um 16:48 schrieb Hanna Czenczek <hreitz@redhat.com> <mailto:hreitz@redhat.com>:
> >>> 
> >>> Like in “rbd: Run co BH CB in the coroutine’s AioContext”, drop the
> >>> completion flag, yield exactly once, and run the BH in the coroutine’s
> >>> AioContext.
> >>> 
> >>> (Can be reproduced with multiqueue by adding a usleep(100000) before the
> >>> `while (!task.complete)` loops.)
> >>> 
> >>> Like in “iscsi: Run co BH CB in the coroutine’s AioContext”, this makes
> >>> nfs_co_generic_bh_cb() trivial, so we can drop it in favor of just
> >>> calling aio_co_wake() directly.
> >>> 
> >>> Cc: qemu-stable@nongnu.org <mailto:qemu-stable@nongnu.org>
> >>> Signed-off-by: Hanna Czenczek <hreitz@redhat.com> <mailto:hreitz@redhat.com>
> >>> ---
> >>> block/nfs.c | 41 ++++++++++++++++-------------------------
> >>> 1 file changed, 16 insertions(+), 25 deletions(-)
> >>> 
> >>> diff --git a/block/nfs.c b/block/nfs.c
> >>> index 0a7d38db09..1d3a34a30c 100644
> >>> --- a/block/nfs.c
> >>> +++ b/block/nfs.c
> >>> @@ -69,7 +69,6 @@ typedef struct NFSClient {
> >>> typedef struct NFSRPC {
> >>>     BlockDriverState *bs;
> >>>     int ret;
> >>> -    int complete;
> >>>     QEMUIOVector *iov;
> >>>     struct stat *st;
> >>>     Coroutine *co;
> >>> @@ -230,14 +229,6 @@ static void coroutine_fn nfs_co_init_task(BlockDriverState *bs, NFSRPC *task)
> >>>     };
> >>> }
> >>> 
> >>> -static void nfs_co_generic_bh_cb(void *opaque)
> >>> -{
> >>> -    NFSRPC *task = opaque;
> >>> -
> >>> -    task->complete = 1;
> >>> -    aio_co_wake(task->co);
> >>> -}
> >>> -
> >>> /* Called (via nfs_service) with QemuMutex held.  */
> >>> static void
> >>> nfs_co_generic_cb(int ret, struct nfs_context *nfs, void *data,
> >>> @@ -256,8 +247,16 @@ nfs_co_generic_cb(int ret, struct nfs_context *nfs, void *data,
> >>>     if (task->ret < 0) {
> >>>         error_report("NFS Error: %s", nfs_get_error(nfs));
> >>>     }
> >>> -    replay_bh_schedule_oneshot_event(task->client->aio_context,
> >>> -                                     nfs_co_generic_bh_cb, task);
> >>> +
> >>> +    /*
> >>> +     * Safe to call: nfs_service(), which called us, is only run from the FD
> >>> +     * handlers, never from the request coroutine.  The request coroutine in
> >>> +     * turn will yield unconditionally.
> >>> +     * No need to release the lock, even if we directly enter the coroutine, as
> >>> +     * the lock is never re-taken after yielding.  (Note: If we do enter the
> >>> +     * coroutine, @task will probably be dangling once aio_co_wake() returns.)
> >>> +     */
> >>> +    aio_co_wake(task->co);
> >>> }
> >>> 
> >>> static int coroutine_fn nfs_co_preadv(BlockDriverState *bs, int64_t offset,
> >>> @@ -278,9 +277,7 @@ static int coroutine_fn nfs_co_preadv(BlockDriverState *bs, int64_t offset,
> >>> 
> >>>         nfs_set_events(client);
> >>>     }
> >>> -    while (!task.complete) {
> >>> -        qemu_coroutine_yield();
> >>> -    }
> >>> +    qemu_coroutine_yield();
> >>> 
> >>>     if (task.ret < 0) {
> >>>         return task.ret;
> >>> @@ -328,9 +325,7 @@ static int coroutine_fn nfs_co_pwritev(BlockDriverState *bs, int64_t offset,
> >>> 
> >>>         nfs_set_events(client);
> >>>     }
> >>> -    while (!task.complete) {
> >>> -        qemu_coroutine_yield();
> >>> -    }
> >>> +    qemu_coroutine_yield();
> >>> 
> >>>     if (my_buffer) {
> >>>         g_free(buf);
> >>> @@ -358,9 +353,7 @@ static int coroutine_fn nfs_co_flush(BlockDriverState *bs)
> >>> 
> >>>         nfs_set_events(client);
> >>>     }
> >>> -    while (!task.complete) {
> >>> -        qemu_coroutine_yield();
> >>> -    }
> >>> +    qemu_coroutine_yield();
> >>> 
> >>>     return task.ret;
> >>> }
> >>> @@ -723,8 +716,8 @@ nfs_get_allocated_file_size_cb(int ret, struct nfs_context *nfs, void *data,
> >>>     if (task->ret < 0) {
> >>>         error_report("NFS Error: %s", nfs_get_error(nfs));
> >>>     }
> >>> -    replay_bh_schedule_oneshot_event(task->client->aio_context,
> >>> -                                     nfs_co_generic_bh_cb, task);
> >>> +    /* Safe to call, see nfs_co_generic_cb() */
> >>> +    aio_co_wake(task->co);
> >>> }
> >>> 
> >>> static int64_t coroutine_fn nfs_co_get_allocated_file_size(BlockDriverState *bs)
> >>> @@ -748,9 +741,7 @@ static int64_t coroutine_fn nfs_co_get_allocated_file_size(BlockDriverState *bs)
> >>> 
> >>>         nfs_set_events(client);
> >>>     }
> >>> -    while (!task.complete) {
> >>> -        qemu_coroutine_yield();
> >>> -    }
> >>> +    qemu_coroutine_yield();
> >>> 
> >>>     return (task.ret < 0 ? task.ret : st.st_blocks * 512);
> >>> }
> >>> -- 
> >>> 2.51.1
> >>> 
> >> 
> >> Hallo Hanna,
> >> 
> >> it has been a long time where I was inactive in Qemu development and I should have checked this earlier, but it seems that this patch accidentally broke libnfs usage with at least qemu cmdline tools like qemu-img. Again, sorry for not testing this earlier, but I have only recently entered the Qemu show again.
> >> 
> >> I found this glitch while working on a patch to include libnfs v6 support in qemu to avoid dropping libnfs support.
> >> 
> >> A simple call like:
> >> 
> >> qemu-img create -f qcow2 nfs://nfsserver/image.qcow2 10G
> >> 
> >> hangs forever in the second write request.
> > 
> > I sent a patch for this, which is basically the same as yours below: https://lists.nongnu.org/archive/html/qemu-block/2026-01/msg00000.html
> 
> Thanks, I will rebase by NFS patches for libnfs v6 on top of that.
> 
> Do we need the same fix for iSCSI?

iscsi drops the lock temporarily. This means that the deadlock won't
happen. What might be worth double checking is if temporarily dropping
the lock still gives enough guarantees for iscsi_service() and its
callers to work correctly.

I don't think I fully understand what the lock is protecting.
Apparently it comes from mass conversions from the original automatic
locking in AioContext callbacks, so nobody really seems to have thought
about it other than asserting that it's as safe as before. Maybe locking
more locally could be a better alternative that would be easier to
understand.

Kevin