Re: [Qemu-devel] [PATCH v2 5/5] block: keep AioContext pointer in BlockBackend

[Fam Zheng <famz@redhat.com>]

On Wed, 09/28 19:47, Max Reitz wrote:
> On 27.09.2016 08:37, Fam Zheng wrote:
> > From: Stefan Hajnoczi <stefanha@redhat.com>
> > 
> > blk_get/set_aio_context() delegate to BlockDriverState without storing
> > the AioContext pointer in BlockBackend.
> > 
> > There are two flaws:
> > 
> > 1. BlockBackend falls back to the QEMU main loop AioContext when there
> >    is no root BlockDriverState.  This means the drive loses its
> >    AioContext during media change and would break dataplane.
> > 
> > 2. BlockBackend state used from multiple threads has no lock.  Race
> >    conditions will creep in as functionality is moved from
> >    BlockDriverState to BlockBackend due to the absense of a lock.  The
> >    monitor cannot access BlockBackend state safely while an IOThread is
> >    also accessing the state.
> > 
> > Issue #1 can be triggered by "change" on virtio-scsi dataplane, causing
> > a assertion failure (virtio-blk is fine because medium change is not
> > possible). #2 may be possible with block accounting statistics in
> > BlockBackend but I'm not aware of a crash that can be triggered.
> > 
> > This patch stores the AioContext pointer in BlockBackend and puts newly
> > inserted BlockDriverStates into the AioContext.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  block/block-backend.c | 24 +++++++++++++++++-------
> >  1 file changed, 17 insertions(+), 7 deletions(-)
> > 
> > diff --git a/block/block-backend.c b/block/block-backend.c
> > index b71babe..cda67cc 100644
> > --- a/block/block-backend.c
> > +++ b/block/block-backend.c
> > @@ -31,6 +31,7 @@ static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb);
> >  struct BlockBackend {
> >      char *name;
> >      int refcnt;
> > +    AioContext *aio_context;
> >      BdrvChild *root;
> >      DriveInfo *legacy_dinfo;    /* null unless created by drive_new() */
> >      QTAILQ_ENTRY(BlockBackend) link;         /* for block_backends */
> > @@ -121,6 +122,7 @@ static BlockBackend *blk_new_with_ctx(AioContext *ctx)
> >  
> >      blk = g_new0(BlockBackend, 1);
> >      blk->refcnt = 1;
> > +    blk->aio_context = ctx;
> >      blk_set_enable_write_cache(blk, true);
> >  
> >      qemu_co_queue_init(&blk->public.throttled_reqs[0]);
> > @@ -510,6 +512,8 @@ void blk_remove_bs(BlockBackend *blk)
> >  void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
> >  {
> >      bdrv_ref(bs);
> > +
> > +    assert(blk->aio_context == bdrv_get_aio_context(bs));
> >      blk->root = bdrv_root_attach_child(bs, "root", &child_root, blk);
> >  
> >      notifier_list_notify(&blk->insert_bs_notifiers, blk);
> > @@ -1413,13 +1417,7 @@ void blk_op_unblock_all(BlockBackend *blk, Error *reason)
> >  
> >  AioContext *blk_get_aio_context(BlockBackend *blk)
> >  {
> > -    BlockDriverState *bs = blk_bs(blk);
> > -
> > -    if (bs) {
> > -        return bdrv_get_aio_context(bs);
> > -    } else {
> > -        return qemu_get_aio_context();
> > -    }
> > +    return blk->aio_context;
> >  }
> >  
> >  static AioContext *blk_aiocb_get_aio_context(BlockAIOCB *acb)
> > @@ -1432,7 +1430,19 @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
> >  {
> >      BlockDriverState *bs = blk_bs(blk);
> >  
> > +    blk->aio_context = new_context;
> > +
> >      if (bs) {
> > +        AioContext *ctx = bdrv_get_aio_context(bs);
> > +
> > +        if (ctx == new_context) {
> > +            return;
> > +        }
> > +        /* Moving context around happens when a block device is
> > +         * enabling/disabling data plane, in which case we own the root BDS and
> > +         * it cannot be associated with another AioContext. */
> > +        assert(ctx == qemu_get_aio_context() ||
> > +               new_context == qemu_get_aio_context());
> 
> I don't really see the point behind this assertion. I know it's not
> currently possible, but you are basically asserting that we do not move
> a BDS tree directly from some non-main-loop context to another
> non-main-loop context, which in theory sounds completely fine to me.
> 
> Based on the "Write code for now and not for the future" rule, I'm fine
> with this assertion if you can tell me what good it does us now.
> 
> The only thing I can personally imagine is that it's a safeguard that we
> don't try to place a BDS tree into some other AioContext while having
> ignored that there are still some other BBs attached to it which don't
> want to agree on that new AioContext. But I think that should rather be
> fixed before patch 2, i.e. as I said we need an infrastructure which can
> tell us beforehand (and without failing assertions) whether we can move
> a certain BDS tree to some other context.
> 
> So whether we can move a certain BB from some context to another depends
> on what the frontend supports, I don't think there is a generic answer
> we can implement here in the generic BB code. NBD for instance allows
> any movement; but devices probably only allow movements they have
> initiated themselves (e.g. dataplane will allow exactly what you
> describe here with that assertion, and any other device will probably
> not allow anything but the main loop).

Indeed, you make me think this should be an op blocker (that applies on whole
graph).

> 
> Max
> 
> >          if (blk->public.throttle_state) {
> >              throttle_timers_detach_aio_context(&blk->public.throttle_timers);
> >          }
> > 
> 
>