Re: [PATCH v2 1/1] nbd/server: do not poll within a coroutine context
On Mon, 1 Apr 2024 11:33:09AM -0500, Eric Blake wrote:
> On Mon, Apr 01, 2024 at 08:41:20PM +0800, Zhu Yangyang wrote:
> > Coroutines are not supposed to block. Instead, they should yield.
> >
> > Fixes: f95910f ("nbd: implement TLS support in the protocol negotiation")
> > Signed-off-by: Zhu Yangyang
> > ---
> > nbd/client.c | 7 ---
> > nbd/common.c | 19 ---
> > nbd/nbd-internal.h | 6 +++---
> > nbd/server.c | 10 +-
> > 4 files changed, 28 insertions(+), 14 deletions(-)
> >
> > diff --git a/nbd/client.c b/nbd/client.c
> > index 29ffc609a4..1ab91ed205 100644
> > --- a/nbd/client.c
> > +++ b/nbd/client.c
> > @@ -619,18 +619,19 @@ static QIOChannel *nbd_receive_starttls(QIOChannel
> > *ioc,
> > return NULL;
> > }
> > qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-client-tls");
> > -data.loop = g_main_loop_new(g_main_context_default(), FALSE);
> > trace_nbd_receive_starttls_tls_handshake();
> > qio_channel_tls_handshake(tioc,
> > - nbd_tls_handshake,
> > + nbd_client_tls_handshake,
> >,
> >NULL,
> >NULL);
> >
> > if (!data.complete) {
> > +data.loop = g_main_loop_new(g_main_context_default(), FALSE);
> > g_main_loop_run(data.loop);
> > +g_main_loop_unref(data.loop);
> > }
> > -g_main_loop_unref(data.loop);
> > +
>
> Aha - you figured out an elegant way around the client code not being
> in coroutine context that had been stumping me, at least for the sake
> of a minimal patch.
>
> > if (data.error) {
> > error_propagate(errp, data.error);
> > object_unref(OBJECT(tioc));
> > diff --git a/nbd/common.c b/nbd/common.c
> > index 3247c1d618..01ca30a5c4 100644
> > --- a/nbd/common.c
> > +++ b/nbd/common.c
> > @@ -47,14 +47,27 @@ int nbd_drop(QIOChannel *ioc, size_t size, Error **errp)
> > }
> >
> >
> > -void nbd_tls_handshake(QIOTask *task,
> > - void *opaque)
> > +void nbd_client_tls_handshake(QIOTask *task, void *opaque)
> > {
> > struct NBDTLSHandshakeData *data = opaque;
> >
> > qio_task_propagate_error(task, >error);
> > data->complete = true;
> > -g_main_loop_quit(data->loop);
> > +if (data->loop) {
> > +g_main_loop_quit(data->loop);
> > +}
> > +}
> > +
> > +
> > +void nbd_server_tls_handshake(QIOTask *task, void *opaque)
> > +{
> > +struct NBDTLSHandshakeData *data = opaque;
> > +
> > +qio_task_propagate_error(task, >error);
> > +data->complete = true;
> > +if (!qemu_coroutine_entered(data->co)) {
> > +aio_co_wake(data->co);
> > +}
> > }
>
> This matches up with what I was experimenting with last week, in that
> server is in coroutine context while client is not. However, it means
> we no longer have a common implementation between the two, so keeping
> the code isolated in nbd/common.c makes less sense than just placing
> the two specific callbacks in the two specific files right where their
> only use case exists (and even making them static at that point).
Yes, we can implement nbd_tls_handshake() on both client and server side.
It looks a lot clearer.
We can even extract the common code to an nbd_tls_handshake_complete().
nbd/common.c
void nbd_tls_handshake_complete(QIOTask *task, void *opaque) {
struct NBDTLSHandshakeData *data = opaque;
qio_task_propagate_error(task, >error);
data->complete = true;
}
server.c / client.c
static void nbd_tls_handshake(QIOTask *task, void *opaque)
{
struct NBDTLSHandshakeData *data = opaque;
nbd_tls_handshake_complete(task, opaque);
...
}
>
> Or, can we still merge it into one helper? It looks like we now have
> 3 viable possibilities:
>
> data->loop data->co
> non-NULL non-NULLimpossible
> NULL NULLclient, qio_task completed right away
> non-NULL NULLclient, qio_task did not complete right away
> NULL non-NULLserver, waking the coroutine depends on if we are in
> one
This seems a little complicated.
>
> With that, we can still get by with one function, but need good
> documentation. I'll post a v3 along those lines, to see what you
> think.
>
> >
> >
> > diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
> > index dfa02f77ee..99cca9382c 100644
> > --- a/nbd/nbd-internal.h
> > +++ b/nbd/nbd-internal.h
> > @@ -74,13 +74,13 @@ static inline int nbd_write(QIOChannel *ioc, const void
> > *buffer, size_t size,
> >
> > struct NBDTLSHandshakeData {
> > GMainLoop *loop;
> > +Coroutine *co;
> > bool complete;
> > Error *error;
> > };
>
> I had tried to get rid of the GMainLoop *loop member altogether, but
> your change has the benefit of a smaller diff than what I was facing
> (I got lost in the weeds trying to see if I could convert all of the
> client into running in
[PATCH v2 1/1] nbd/server: do not poll within a coroutine context
Coroutines are not supposed to block. Instead, they should yield.
Fixes: f95910f ("nbd: implement TLS support in the protocol negotiation")
Signed-off-by: Zhu Yangyang
---
nbd/client.c | 7 ---
nbd/common.c | 19 ---
nbd/nbd-internal.h | 6 +++---
nbd/server.c | 10 +-
4 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/nbd/client.c b/nbd/client.c
index 29ffc609a4..1ab91ed205 100644
--- a/nbd/client.c
+++ b/nbd/client.c
@@ -619,18 +619,19 @@ static QIOChannel *nbd_receive_starttls(QIOChannel *ioc,
return NULL;
}
qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-client-tls");
-data.loop = g_main_loop_new(g_main_context_default(), FALSE);
trace_nbd_receive_starttls_tls_handshake();
qio_channel_tls_handshake(tioc,
- nbd_tls_handshake,
+ nbd_client_tls_handshake,
,
NULL,
NULL);
if (!data.complete) {
+data.loop = g_main_loop_new(g_main_context_default(), FALSE);
g_main_loop_run(data.loop);
+g_main_loop_unref(data.loop);
}
-g_main_loop_unref(data.loop);
+
if (data.error) {
error_propagate(errp, data.error);
object_unref(OBJECT(tioc));
diff --git a/nbd/common.c b/nbd/common.c
index 3247c1d618..01ca30a5c4 100644
--- a/nbd/common.c
+++ b/nbd/common.c
@@ -47,14 +47,27 @@ int nbd_drop(QIOChannel *ioc, size_t size, Error **errp)
}
-void nbd_tls_handshake(QIOTask *task,
- void *opaque)
+void nbd_client_tls_handshake(QIOTask *task, void *opaque)
{
struct NBDTLSHandshakeData *data = opaque;
qio_task_propagate_error(task, >error);
data->complete = true;
-g_main_loop_quit(data->loop);
+if (data->loop) {
+g_main_loop_quit(data->loop);
+}
+}
+
+
+void nbd_server_tls_handshake(QIOTask *task, void *opaque)
+{
+struct NBDTLSHandshakeData *data = opaque;
+
+qio_task_propagate_error(task, >error);
+data->complete = true;
+if (!qemu_coroutine_entered(data->co)) {
+aio_co_wake(data->co);
+}
}
diff --git a/nbd/nbd-internal.h b/nbd/nbd-internal.h
index dfa02f77ee..99cca9382c 100644
--- a/nbd/nbd-internal.h
+++ b/nbd/nbd-internal.h
@@ -74,13 +74,13 @@ static inline int nbd_write(QIOChannel *ioc, const void
*buffer, size_t size,
struct NBDTLSHandshakeData {
GMainLoop *loop;
+Coroutine *co;
bool complete;
Error *error;
};
-
-void nbd_tls_handshake(QIOTask *task,
- void *opaque);
+void nbd_server_tls_handshake(QIOTask *task, void *opaque);
+void nbd_client_tls_handshake(QIOTask *task, void *opaque);
int nbd_drop(QIOChannel *ioc, size_t size, Error **errp);
diff --git a/nbd/server.c b/nbd/server.c
index c3484cc1eb..b218512ced 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -777,17 +777,17 @@ static QIOChannel
*nbd_negotiate_handle_starttls(NBDClient *client,
qio_channel_set_name(QIO_CHANNEL(tioc), "nbd-server-tls");
trace_nbd_negotiate_handle_starttls_handshake();
-data.loop = g_main_loop_new(g_main_context_default(), FALSE);
+data.co = qemu_coroutine_self();
qio_channel_tls_handshake(tioc,
- nbd_tls_handshake,
+ nbd_server_tls_handshake,
,
NULL,
NULL);
-if (!data.complete) {
-g_main_loop_run(data.loop);
+while (!data.complete) {
+qemu_coroutine_yield();
}
-g_main_loop_unref(data.loop);
+
if (data.error) {
object_unref(OBJECT(tioc));
error_propagate(errp, data.error);
--
2.33.0
[PATCH v2 0/1] coroutine: avoid inserting duplicate coroutine to co_queue_wakeup
The problem that inserting duplicate coroutine to co_queue_wakeu has been
resolved by 7c1f51bf38 ("nbd/server: Fix drained_poll to wake coroutine
in right AioContext") that avoids repeatedly waking up the same coroutine.
The key modifications are as follows:
static void qio_channel_restart_read(void *opaque)
{
QIOChannel *ioc = opaque;
- Coroutine *co = ioc->read_coroutine;
+ Coroutine *co = qatomic_xchg(>read_coroutine, NULL);
+
+ if (!co) {
+ return;
+ }
/* Assert that aio_co_wake() reenters the coroutine directly */
assert(qemu_get_current_aio_context() ==
qemu_coroutine_get_aio_context(co));
aio_co_wake(co);
}
The root cause is that poll() is invoked in coroutine context, so fix it.
Changes in v2:
Drop the changes to aio_co_enter and instead fix the poll() call in the
nbd/server.
Zhu Yangyang (1):
nbd/server: do not poll within a coroutine context
nbd/client.c | 7 ---
nbd/common.c | 19 ---
nbd/nbd-internal.h | 6 +++---
nbd/server.c | 10 +-
4 files changed, 28 insertions(+), 14 deletions(-)
--
2.33.0
Re: [PATCH v1] coroutine: avoid inserting duplicate coroutine to co_queue_wakeup
On Thu, 28 Mar 2024 07:40:14 -0500, Eric Blake wrote:
> On Mon, Mar 25, 2024 at 05:18:50PM +0800, zhuyangyang via wrote:
> > If g_main_loop_run()/aio_poll() is called in the coroutine context,
> > the pending coroutine may be woken up repeatedly, and the co_queue_wakeup
> > may be disordered.
> >
> > When the poll() syscall exited in g_main_loop_run()/aio_poll(), it means
> > some listened events is completed. Therefore, the completion callback
> > function is dispatched.
> >
> > If this callback function needs to invoke aio_co_enter(), it will only
> > wake up the coroutine (because we are already in coroutine context),
> > which may cause that the data on this listening event_fd/socket_fd
> > is not read/cleared. When the next poll () exits, it will be woken up again
> > and inserted into the wakeup queue again.
> >
> > For example, if TLS is enabled in NBD, the server will call
> > g_main_loop_run()
> > in the coroutine, and repeatedly wake up the io_read event on a socket.
> > The call stack is as follows:
> >
> > aio_co_enter()
> > aio_co_wake()
> > qio_channel_restart_read()
> > aio_dispatch_handler()
> > aio_dispatch_handlers()
> > aio_dispatch()
> > aio_ctx_dispatch()
> > g_main_context_dispatch()
> > g_main_loop_run()
> > nbd_negotiate_handle_starttls()
> > nbd_negotiate_options()
> > nbd_negotiate()
> > nbd_co_client_start()
> > coroutine_trampoline()
>
> zhuyangyang, do you have a reliable reproduction setup for how you
> were able to trigger this? Obviously, it only happens when TLS is
> enabled (we aren't creating a g_main_loop_run for any other NBD
> command), and only when the server is first starting to serve a
> client; is this a case where you were hammering a long-running qemu
> process running an NBD server with multiple clients trying to
> reconnect to the server all near the same time?
This problem cannot be reproduced after
7c1f51bf38 ("nbd/server: Fix drained_poll to wake coroutine in right
AioContext")
that avoids repeatedly waking up the same coroutine.
Invoking g_main_loop_run() in the coroutine will cause that
event completion callback function qio_channel_restart_read() is called
repeatedly,
but the coroutine is woken up only once.
The key modifications are as follows:
static void qio_channel_restart_read(void *opaque)
{
QIOChannel *ioc = opaque;
- Coroutine *co = ioc->read_coroutine;
+ Coroutine *co = qatomic_xchg(>read_coroutine, NULL);
+
+ if (!co) {
+ return;
+ }
/* Assert that aio_co_wake() reenters the coroutine directly */
assert(qemu_get_current_aio_context() ==
qemu_coroutine_get_aio_context(co));
aio_co_wake(co);
}
The root cause is that poll() is invoked in coroutine context.
>
> If we can come up with a reliable formula for reproducing the
> corrupted coroutine list, it would make a great iotest addition
> alongside the existing qemu-iotests 233 for ensuring that NBD TLS
> traffic is handled correctly in both server and client.
>
> >
> > Signed-off-by: zhuyangyang
>
> Side note: this appears to be your first qemu contribution (based on
> 'git shortlog --author zhuyangyang'). While I am not in a position to
> presume how you would like your name Anglicized, I will point out that
> the prevailing style is to separate given name from family name (just
> because your username at work has no spaces does not mean that your
> S-o-b has to follow suit). It is also permissible to list your name
> in native characters alongside or in place of the Anglicized version;
> for example, 'git log --author="Stefano Dong"' shows this technique.
--
Best Regards,
Zhu Yangyang
Re: [PATCH v1] coroutine: avoid inserting duplicate coroutine to co_queue_wakeup
On Thu, 28 Mar 2024 07:40:14 -0500, Eric Blake wrote:
> On Mon, Mar 25, 2024 at 05:18:50PM +0800, zhuyangyang via wrote:
> > If g_main_loop_run()/aio_poll() is called in the coroutine context,
> > the pending coroutine may be woken up repeatedly, and the co_queue_wakeup
> > may be disordered.
> >
> > When the poll() syscall exited in g_main_loop_run()/aio_poll(), it means
> > some listened events is completed. Therefore, the completion callback
> > function is dispatched.
> >
> > If this callback function needs to invoke aio_co_enter(), it will only
> > wake up the coroutine (because we are already in coroutine context),
> > which may cause that the data on this listening event_fd/socket_fd
> > is not read/cleared. When the next poll () exits, it will be woken up again
> > and inserted into the wakeup queue again.
> >
> > For example, if TLS is enabled in NBD, the server will call
> > g_main_loop_run()
> > in the coroutine, and repeatedly wake up the io_read event on a socket.
> > The call stack is as follows:
> >
> > aio_co_enter()
> > aio_co_wake()
> > qio_channel_restart_read()
> > aio_dispatch_handler()
> > aio_dispatch_handlers()
> > aio_dispatch()
> > aio_ctx_dispatch()
> > g_main_context_dispatch()
> > g_main_loop_run()
> > nbd_negotiate_handle_starttls()
> > nbd_negotiate_options()
> > nbd_negotiate()
> > nbd_co_client_start()
> > coroutine_trampoline()
>
> zhuyangyang, do you have a reliable reproduction setup for how you
> were able to trigger this? Obviously, it only happens when TLS is
> enabled (we aren't creating a g_main_loop_run for any other NBD
> command), and only when the server is first starting to serve a
> client; is this a case where you were hammering a long-running qemu
> process running an NBD server with multiple clients trying to
> reconnect to the server all near the same time?
I'm sorry I didn't make the background of the problem clear before,
this problem is not on the NBD command, but on the VM live migration
with qemu TLS.
Next, I'll detail how to reproduce the issue.
1. Make the problem more obvious.
When TLS is enabled during live migration, the migration progress
may be suspended because some I/O are not returned during the mirror job
on target host.
Now we know that the reason is that some coroutines are lost.
The entry function of these lost coroutines are nbd_trip().
Add an assertion on the target host side to make the problem
show up quickly.
$ git diff util/async.c
diff --git a/util/async.c b/util/async.c
index 0467890052..4e3547c3ea 100644
--- a/util/async.c
+++ b/util/async.c
@@ -705,6 +705,7 @@ void aio_co_enter(AioContext *ctx, Coroutine *co)
if (qemu_in_coroutine()) {
Coroutine *self = qemu_coroutine_self();
assert(self != co);
+assert(!co->co_queue_next.sqe_next);
QSIMPLEQ_INSERT_TAIL(>co_queue_wakeup, co, co_queue_next);
} else {
qemu_aio_coroutine_enter(ctx, co);
2. Reproduce the issue
1) start vm on the origin host
Note: Configuring multiple disks for a VM
(It is recommended to configure more than 6 disks)
These disk tasks(nbd_trip) will be performed simultaneously
with nbd_negotiate_handle_starttls() on the main thread during migrate.
centos7.3_64_server
4194304
4194304
4
/machine
hvm
destroy
restart
restart
/usr/bin/qemu-kvm
$ virsh create vm_x86.xml
Domain 'centos7.3_64_server' created from /home/vm_x86.xml
2) migrate the vm to target host
virsh migrate --live --p2p \
--migrateuri tcp:10.91.xxx.xxx centos7.3_64_server
qemu+tcp://10.91.xxx.xxx/system \
--copy-storage-all \
--tls
Than, An error is reported on the peer host
qemu-kvm: ../util/async.c:705: aio_co_enter: Assertion
`!co->co_queue_next.sqe_next' failed.
>
> If we can come up with a reliable formula for reproducing the
> corrupted coroutine list, it would make a great iotest addition
> alongside the existing qemu-iotests 233 for ensuring that NBD TLS
> traffic is handled correctly in both server and client.
I'm not sure if this can be used for testing of qemu-nbd
>
> >
> > Signed-off-by: zhuyangyang
>
> Side note: this appears to be your first qemu contribution (based on
> 'git shortlog --author zhuyangyang'). While I am not in a position to
> presume how you would like your name Anglicized, I will point out that
> the prevailing style is to separate given name from family name (just
> because your username at work has no spaces does not mean that your
> S-o-b has to follow suit). It is also permissible to list your name
> in native characters alongside or in place of the
