Each vhost-user-blk request runs in a coroutine. When the BlockBackend
enters a drained section we need to enter a quiescent state. Currently
any in-flight requests race with bdrv_drained_begin() because it is
unaware of vhost-user-blk requests.
When blk_co_preadv/pwritev()/etc returns it wakes the
bdrv_drained_begin() thread but vhost-user-blk request processing has
not yet finished. The request coroutine continues executing while the
main loop thread thinks it is in a drained section.
One example where this is unsafe is for blk_set_aio_context() where
bdrv_drained_begin() is called before .aio_context_detached() and
.aio_context_attach(). If request coroutines are still running after
bdrv_drained_begin(), then the AioContext could change underneath them
and they race with new requests processed in the new AioContext. This
could lead to virtqueue corruption, for example.
(This example is theoretical, I came across this while reading the
code and have not tried to reproduce it.)
It's easy to make bdrv_drained_begin() wait for in-flight requests: add
a .drained_poll() callback that checks the VuServer's in-flight counter.
VuServer just needs an API that returns true when there are requests in
flight. The in-flight counter needs to be atomic.
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/qemu/vhost-user-server.h | 4 +++-
block/export/vhost-user-blk-server.c | 16 ++++++++++++++++
util/vhost-user-server.c | 14 ++++++++++----
3 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/include/qemu/vhost-user-server.h b/include/qemu/vhost-user-server.h
index bc0ac9ddb6..b1c1cda886 100644
--- a/include/qemu/vhost-user-server.h
+++ b/include/qemu/vhost-user-server.h
@@ -40,8 +40,9 @@ typedef struct {
int max_queues;
const VuDevIface *vu_iface;
+ unsigned int in_flight; /* atomic */
+
/* Protected by ctx lock */
- unsigned int in_flight;
bool wait_idle;
VuDev vu_dev;
QIOChannel *ioc; /* The I/O channel with the client */
@@ -62,6 +63,7 @@ void vhost_user_server_stop(VuServer *server);
void vhost_user_server_inc_in_flight(VuServer *server);
void vhost_user_server_dec_in_flight(VuServer *server);
+bool vhost_user_server_has_in_flight(VuServer *server);
void vhost_user_server_attach_aio_context(VuServer *server, AioContext *ctx);
void vhost_user_server_detach_aio_context(VuServer *server);
diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
index 841acb36e3..092b86aae4 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -272,7 +272,20 @@ static void vu_blk_exp_resize(void *opaque)
vu_config_change_msg(&vexp->vu_server.vu_dev);
}
+/*
+ * Ensures that bdrv_drained_begin() waits until in-flight requests complete.
+ *
+ * Called with vexp->export.ctx acquired.
+ */
+static bool vu_blk_drained_poll(void *opaque)
+{
+ VuBlkExport *vexp = opaque;
+
+ return vhost_user_server_has_in_flight(&vexp->vu_server);
+}
+
static const BlockDevOps vu_blk_dev_ops = {
+ .drained_poll = vu_blk_drained_poll,
.resize_cb = vu_blk_exp_resize,
};
@@ -314,6 +327,7 @@ static int vu_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
vu_blk_initialize_config(blk_bs(exp->blk), &vexp->blkcfg,
logical_block_size, num_queues);
+ blk_set_dev_ops(exp->blk, &vu_blk_dev_ops, vexp);
blk_add_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
vexp);
@@ -323,6 +337,7 @@ static int vu_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
num_queues, &vu_blk_iface, errp)) {
blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
blk_aio_detach, vexp);
+ blk_set_dev_ops(exp->blk, NULL, NULL);
g_free(vexp->handler.serial);
return -EADDRNOTAVAIL;
}
@@ -336,6 +351,7 @@ static void vu_blk_exp_delete(BlockExport *exp)
blk_remove_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
vexp);
+ blk_set_dev_ops(exp->blk, NULL, NULL);
g_free(vexp->handler.serial);
}
diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
index 1622f8cfb3..2e6b640050 100644
--- a/util/vhost-user-server.c
+++ b/util/vhost-user-server.c
@@ -78,17 +78,23 @@ static void panic_cb(VuDev *vu_dev, const char *buf)
void vhost_user_server_inc_in_flight(VuServer *server)
{
assert(!server->wait_idle);
- server->in_flight++;
+ qatomic_inc(&server->in_flight);
}
void vhost_user_server_dec_in_flight(VuServer *server)
{
- server->in_flight--;
- if (server->wait_idle && !server->in_flight) {
- aio_co_wake(server->co_trip);
+ if (qatomic_fetch_dec(&server->in_flight) == 1) {
+ if (server->wait_idle) {
+ aio_co_wake(server->co_trip);
+ }
}
}
+bool vhost_user_server_has_in_flight(VuServer *server)
+{
+ return qatomic_load_acquire(&server->in_flight) > 0;
+}
+
static bool coroutine_fn
vu_message_read(VuDev *vu_dev, int conn_fd, VhostUserMsg *vmsg)
{
--
2.39.2Am 25.04.2023 um 19:27 hat Stefan Hajnoczi geschrieben:
> Each vhost-user-blk request runs in a coroutine. When the BlockBackend
> enters a drained section we need to enter a quiescent state. Currently
> any in-flight requests race with bdrv_drained_begin() because it is
> unaware of vhost-user-blk requests.
>
> When blk_co_preadv/pwritev()/etc returns it wakes the
> bdrv_drained_begin() thread but vhost-user-blk request processing has
> not yet finished. The request coroutine continues executing while the
> main loop thread thinks it is in a drained section.
>
> One example where this is unsafe is for blk_set_aio_context() where
> bdrv_drained_begin() is called before .aio_context_detached() and
> .aio_context_attach(). If request coroutines are still running after
> bdrv_drained_begin(), then the AioContext could change underneath them
> and they race with new requests processed in the new AioContext. This
> could lead to virtqueue corruption, for example.
>
> (This example is theoretical, I came across this while reading the
> code and have not tried to reproduce it.)
>
> It's easy to make bdrv_drained_begin() wait for in-flight requests: add
> a .drained_poll() callback that checks the VuServer's in-flight counter.
> VuServer just needs an API that returns true when there are requests in
> flight. The in-flight counter needs to be atomic.
>
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
> include/qemu/vhost-user-server.h | 4 +++-
> block/export/vhost-user-blk-server.c | 16 ++++++++++++++++
> util/vhost-user-server.c | 14 ++++++++++----
> 3 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/include/qemu/vhost-user-server.h b/include/qemu/vhost-user-server.h
> index bc0ac9ddb6..b1c1cda886 100644
> --- a/include/qemu/vhost-user-server.h
> +++ b/include/qemu/vhost-user-server.h
> @@ -40,8 +40,9 @@ typedef struct {
> int max_queues;
> const VuDevIface *vu_iface;
>
> + unsigned int in_flight; /* atomic */
> +
> /* Protected by ctx lock */
> - unsigned int in_flight;
> bool wait_idle;
> VuDev vu_dev;
> QIOChannel *ioc; /* The I/O channel with the client */
> @@ -62,6 +63,7 @@ void vhost_user_server_stop(VuServer *server);
>
> void vhost_user_server_inc_in_flight(VuServer *server);
> void vhost_user_server_dec_in_flight(VuServer *server);
> +bool vhost_user_server_has_in_flight(VuServer *server);
>
> void vhost_user_server_attach_aio_context(VuServer *server, AioContext *ctx);
> void vhost_user_server_detach_aio_context(VuServer *server);
> diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
> index 841acb36e3..092b86aae4 100644
> --- a/block/export/vhost-user-blk-server.c
> +++ b/block/export/vhost-user-blk-server.c
> @@ -272,7 +272,20 @@ static void vu_blk_exp_resize(void *opaque)
> vu_config_change_msg(&vexp->vu_server.vu_dev);
> }
>
> +/*
> + * Ensures that bdrv_drained_begin() waits until in-flight requests complete.
> + *
> + * Called with vexp->export.ctx acquired.
> + */
> +static bool vu_blk_drained_poll(void *opaque)
> +{
> + VuBlkExport *vexp = opaque;
> +
> + return vhost_user_server_has_in_flight(&vexp->vu_server);
> +}
> +
> static const BlockDevOps vu_blk_dev_ops = {
> + .drained_poll = vu_blk_drained_poll,
> .resize_cb = vu_blk_exp_resize,
> };
You're adding a new function pointer to an existing BlockDevOps...
> @@ -314,6 +327,7 @@ static int vu_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
> vu_blk_initialize_config(blk_bs(exp->blk), &vexp->blkcfg,
> logical_block_size, num_queues);
>
> + blk_set_dev_ops(exp->blk, &vu_blk_dev_ops, vexp);
> blk_add_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
> vexp);
>
> blk_set_dev_ops(exp->blk, &vu_blk_dev_ops, vexp);
..but still add a second blk_set_dev_ops(). Maybe a bad merge conflict
resolution with commit ca858a5fe94?
> @@ -323,6 +337,7 @@ static int vu_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
> num_queues, &vu_blk_iface, errp)) {
> blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
> blk_aio_detach, vexp);
> + blk_set_dev_ops(exp->blk, NULL, NULL);
> g_free(vexp->handler.serial);
> return -EADDRNOTAVAIL;
> }
> @@ -336,6 +351,7 @@ static void vu_blk_exp_delete(BlockExport *exp)
>
> blk_remove_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
> vexp);
> + blk_set_dev_ops(exp->blk, NULL, NULL);
> g_free(vexp->handler.serial);
> }
These two hunks are then probably already fixes for ca858a5fe94 and
should be a separate patch if so.
> diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
> index 1622f8cfb3..2e6b640050 100644
> --- a/util/vhost-user-server.c
> +++ b/util/vhost-user-server.c
> @@ -78,17 +78,23 @@ static void panic_cb(VuDev *vu_dev, const char *buf)
> void vhost_user_server_inc_in_flight(VuServer *server)
> {
> assert(!server->wait_idle);
> - server->in_flight++;
> + qatomic_inc(&server->in_flight);
> }
>
> void vhost_user_server_dec_in_flight(VuServer *server)
> {
> - server->in_flight--;
> - if (server->wait_idle && !server->in_flight) {
> - aio_co_wake(server->co_trip);
> + if (qatomic_fetch_dec(&server->in_flight) == 1) {
> + if (server->wait_idle) {
> + aio_co_wake(server->co_trip);
> + }
> }
> }
>
> +bool vhost_user_server_has_in_flight(VuServer *server)
> +{
> + return qatomic_load_acquire(&server->in_flight) > 0;
> +}
> +
Any reason why you left the server->in_flight accesses in
vu_client_trip() non-atomic?
Kevin
On Tue, May 02, 2023 at 05:42:51PM +0200, Kevin Wolf wrote:
> Am 25.04.2023 um 19:27 hat Stefan Hajnoczi geschrieben:
> > Each vhost-user-blk request runs in a coroutine. When the BlockBackend
> > enters a drained section we need to enter a quiescent state. Currently
> > any in-flight requests race with bdrv_drained_begin() because it is
> > unaware of vhost-user-blk requests.
> >
> > When blk_co_preadv/pwritev()/etc returns it wakes the
> > bdrv_drained_begin() thread but vhost-user-blk request processing has
> > not yet finished. The request coroutine continues executing while the
> > main loop thread thinks it is in a drained section.
> >
> > One example where this is unsafe is for blk_set_aio_context() where
> > bdrv_drained_begin() is called before .aio_context_detached() and
> > .aio_context_attach(). If request coroutines are still running after
> > bdrv_drained_begin(), then the AioContext could change underneath them
> > and they race with new requests processed in the new AioContext. This
> > could lead to virtqueue corruption, for example.
> >
> > (This example is theoretical, I came across this while reading the
> > code and have not tried to reproduce it.)
> >
> > It's easy to make bdrv_drained_begin() wait for in-flight requests: add
> > a .drained_poll() callback that checks the VuServer's in-flight counter.
> > VuServer just needs an API that returns true when there are requests in
> > flight. The in-flight counter needs to be atomic.
> >
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> > include/qemu/vhost-user-server.h | 4 +++-
> > block/export/vhost-user-blk-server.c | 16 ++++++++++++++++
> > util/vhost-user-server.c | 14 ++++++++++----
> > 3 files changed, 29 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/qemu/vhost-user-server.h b/include/qemu/vhost-user-server.h
> > index bc0ac9ddb6..b1c1cda886 100644
> > --- a/include/qemu/vhost-user-server.h
> > +++ b/include/qemu/vhost-user-server.h
> > @@ -40,8 +40,9 @@ typedef struct {
> > int max_queues;
> > const VuDevIface *vu_iface;
> >
> > + unsigned int in_flight; /* atomic */
> > +
> > /* Protected by ctx lock */
> > - unsigned int in_flight;
> > bool wait_idle;
> > VuDev vu_dev;
> > QIOChannel *ioc; /* The I/O channel with the client */
> > @@ -62,6 +63,7 @@ void vhost_user_server_stop(VuServer *server);
> >
> > void vhost_user_server_inc_in_flight(VuServer *server);
> > void vhost_user_server_dec_in_flight(VuServer *server);
> > +bool vhost_user_server_has_in_flight(VuServer *server);
> >
> > void vhost_user_server_attach_aio_context(VuServer *server, AioContext *ctx);
> > void vhost_user_server_detach_aio_context(VuServer *server);
> > diff --git a/block/export/vhost-user-blk-server.c b/block/export/vhost-user-blk-server.c
> > index 841acb36e3..092b86aae4 100644
> > --- a/block/export/vhost-user-blk-server.c
> > +++ b/block/export/vhost-user-blk-server.c
> > @@ -272,7 +272,20 @@ static void vu_blk_exp_resize(void *opaque)
> > vu_config_change_msg(&vexp->vu_server.vu_dev);
> > }
> >
> > +/*
> > + * Ensures that bdrv_drained_begin() waits until in-flight requests complete.
> > + *
> > + * Called with vexp->export.ctx acquired.
> > + */
> > +static bool vu_blk_drained_poll(void *opaque)
> > +{
> > + VuBlkExport *vexp = opaque;
> > +
> > + return vhost_user_server_has_in_flight(&vexp->vu_server);
> > +}
> > +
> > static const BlockDevOps vu_blk_dev_ops = {
> > + .drained_poll = vu_blk_drained_poll,
> > .resize_cb = vu_blk_exp_resize,
> > };
>
> You're adding a new function pointer to an existing BlockDevOps...
>
> > @@ -314,6 +327,7 @@ static int vu_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
> > vu_blk_initialize_config(blk_bs(exp->blk), &vexp->blkcfg,
> > logical_block_size, num_queues);
> >
> > + blk_set_dev_ops(exp->blk, &vu_blk_dev_ops, vexp);
> > blk_add_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
> > vexp);
> >
> > blk_set_dev_ops(exp->blk, &vu_blk_dev_ops, vexp);
>
> ..but still add a second blk_set_dev_ops(). Maybe a bad merge conflict
> resolution with commit ca858a5fe94?
Thanks, I probably didn't have ca858a5fe94 in my tree when writing this
code.
> > @@ -323,6 +337,7 @@ static int vu_blk_exp_create(BlockExport *exp, BlockExportOptions *opts,
> > num_queues, &vu_blk_iface, errp)) {
> > blk_remove_aio_context_notifier(exp->blk, blk_aio_attached,
> > blk_aio_detach, vexp);
> > + blk_set_dev_ops(exp->blk, NULL, NULL);
> > g_free(vexp->handler.serial);
> > return -EADDRNOTAVAIL;
> > }
> > @@ -336,6 +351,7 @@ static void vu_blk_exp_delete(BlockExport *exp)
> >
> > blk_remove_aio_context_notifier(exp->blk, blk_aio_attached, blk_aio_detach,
> > vexp);
> > + blk_set_dev_ops(exp->blk, NULL, NULL);
> > g_free(vexp->handler.serial);
> > }
>
> These two hunks are then probably already fixes for ca858a5fe94 and
> should be a separate patch if so.
Sure, I can split them out.
hw/ doesn't need to call blk_set_dev_ops(blk, NULL, NULL) because
hw/core/qdev-properties-system.c:release_drive() -> blk_detach_dev()
does it automatically, but block/export does. It's easy to overlook and
that's probably why ca858a5fe94 didn't include it.
> > diff --git a/util/vhost-user-server.c b/util/vhost-user-server.c
> > index 1622f8cfb3..2e6b640050 100644
> > --- a/util/vhost-user-server.c
> > +++ b/util/vhost-user-server.c
> > @@ -78,17 +78,23 @@ static void panic_cb(VuDev *vu_dev, const char *buf)
> > void vhost_user_server_inc_in_flight(VuServer *server)
> > {
> > assert(!server->wait_idle);
> > - server->in_flight++;
> > + qatomic_inc(&server->in_flight);
> > }
> >
> > void vhost_user_server_dec_in_flight(VuServer *server)
> > {
> > - server->in_flight--;
> > - if (server->wait_idle && !server->in_flight) {
> > - aio_co_wake(server->co_trip);
> > + if (qatomic_fetch_dec(&server->in_flight) == 1) {
> > + if (server->wait_idle) {
> > + aio_co_wake(server->co_trip);
> > + }
> > }
> > }
> >
> > +bool vhost_user_server_has_in_flight(VuServer *server)
> > +{
> > + return qatomic_load_acquire(&server->in_flight) > 0;
> > +}
> > +
>
> Any reason why you left the server->in_flight accesses in
> vu_client_trip() non-atomic?
I don't remember if it was a mistake or if there is a reason why it's
safe. I'll replace those accesses with calls to
vhost_user_server_has_in_flight().
Stefan
© 2016 - 2026 Red Hat, Inc.