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

Fri, 06 Mar 2026 05:42:57 -0800 

On 03.03.26 20:24, Peter Lieven wrote:




Am 10.11.2025 um 16:48 schrieb Hanna Czenczek <[email protected]>:

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: [email protected]
Signed-off-by: Hanna Czenczek <[email protected]>
---
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


Hanna




I have not confirmed, but this might also hold true for libiscsi which 
internally is quite identical (at least it was 3 years ago ;-))



You are right, the coroutine can only yield once, but it seems that 
the bottom half logic is still needed.
Maybe you have an idea why? I remember that we had this differently 
first and implemented it after Stefan pointed out that it might be needed.

I will try to find the thread about this discussion.

This patch here restores functionality for me:

*diff --git a/block/nfs.c b/block/nfs.c*

*index 1d3a34a30c..45731ea3da 100644*

*--- a/block/nfs.c*

*+++ b/block/nfs.c*

@@ -75,6 +75,12 @@typedef struct NFSRPC {

NFSClient *client;

 } NFSRPC;

+static void nfs_co_generic_bh_cb(void *opaque)

+{

+ NFSRPC *task = opaque;

+ aio_co_wake(task->co);

+}

+


 static int nfs_parse_uri(const char *filename, QDict *options, Error 
**errp)


 {

g_autoptr(GUri) uri = g_uri_parse(filename, G_URI_FLAGS_NONE, NULL);


@@ -248,15 +254,8 @@nfs_co_generic_cb(int ret, struct nfs_context 
*nfs, void *data,


error_report("NFS Error: %s", nfs_get_error(nfs));

     }

-    /*


-     * 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);

+ replay_bh_schedule_oneshot_event(task->client->aio_context,

+                               nfs_co_generic_bh_cb, task);

 }


 static int coroutine_fn nfs_co_preadv(BlockDriverState *bs, int64_t 
offset,



@@ -716,8 +715,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));

     }

-    /* Safe to call, see nfs_co_generic_cb() */

- aio_co_wake(task->co);

+ replay_bh_schedule_oneshot_event(task->client->aio_context,

+                               nfs_co_generic_bh_cb, task);

 }


 static int64_t coroutine_fn 
nfs_co_get_allocated_file_size(BlockDriverState *bs)





Best

Peter
























					Reply via email to