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

[Max Reitz <mreitz@redhat.com>]

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

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).

Max

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 480 bytes --]