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

[Fam Zheng]

From: Stefan Hajnoczi <address@hidden>

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 <address@hidden>
Signed-off-by: Fam Zheng <address@hidden>
---
 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());
         if (blk->public.throttle_state) {
             throttle_timers_detach_aio_context(&blk->public.throttle_timers);
         }
-- 
2.7.4