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

[Stefan Hajnoczi <stefanha@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 6898 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]