Re: [PATCH v4 06/20] block/export: wait for vhost-user-blk requests when draining

[Kevin Wolf <kwolf@xxxxxxxxxx>]

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@xxxxxxxxxx>
> ---
>  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