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 <stefa...@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
