[Qemu-devel] [PATCH v3 01/14] block: Make sure throttled BDSes always have a BB

From: Kevin Wolf <kwolf@redhat.com>
To: qemu-block@nongnu.org
Cc: kwolf@redhat.com, stefanha@redhat.com, berto@igalia.com,
	pbonzini@redhat.com, mreitz@redhat.com, eblake@redhat.com,
	qemu-devel@nongnu.org
Subject: [Qemu-devel] [PATCH v3 01/14] block: Make sure throttled BDSes always have a BB
Date: Wed, 11 May 2016 15:24:12 +0200	[thread overview]
Message-ID: <1462973065-9876-2-git-send-email-kwolf@redhat.com> (raw)
In-Reply-To: <1462973065-9876-1-git-send-email-kwolf@redhat.com>

It was already true in principle that a throttled BDS always has a BB
attached, except that the order of operations while attaching or
detaching a BDS to/from a BB wasn't careful enough.

This commit breaks graph manipulations while I/O throttling is enabled.
It would have been possible to keep things working with some temporary
hacks, but quite cumbersome, so it's not worth the hassle. We'll fix
things again in a minute.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
---
 block.c               | 14 ++++++++++++++
 block/block-backend.c |  3 +++
 blockdev.c            |  4 ++--
 tests/test-throttle.c | 11 ++++++++---
 4 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/block.c b/block.c
index 724b3b2..9bad665 100644
--- a/block.c
+++ b/block.c
@@ -2259,8 +2259,22 @@ static void swap_feature_fields(BlockDriverState *bs_top,
 
     assert(!bs_new->throttle_state);
     if (bs_top->throttle_state) {
+        /*
+         * FIXME Need to break I/O throttling with graph manipulations
+         * temporarily because of conflicting invariants (3. will go away when
+         * throttling is fully converted to work on BlockBackends):
+         *
+         * 1. Every BlockBackend has a single root BDS
+         * 2. I/O throttling functions require an attached BlockBackend
+         * 3. We need to first enable throttling on the new BDS and then
+         *    disable it on the old one (because of throttle group refcounts)
+         */
+#if 0
         bdrv_io_limits_enable(bs_new, throttle_group_get_name(bs_top));
         bdrv_io_limits_disable(bs_top);
+#else
+        abort();
+#endif
     }
 }
 
diff --git a/block/block-backend.c b/block/block-backend.c
index a1e2c7f..53fd9d2 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -420,6 +420,9 @@ void blk_remove_bs(BlockBackend *blk)
     notifier_list_notify(&blk->remove_bs_notifiers, blk);
 
     blk_update_root_state(blk);
+    if (blk->root->bs->throttle_state) {
+        bdrv_io_limits_disable(blk->root->bs);
+    }
 
     blk->root->bs->blk = NULL;
     bdrv_root_unref_child(blk->root);
diff --git a/blockdev.c b/blockdev.c
index f74eb43..585a28a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2570,8 +2570,6 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
         goto fail;
     }
 
-    blk_apply_root_state(blk, medium_bs);
-
     bdrv_add_key(medium_bs, NULL, &err);
     if (err) {
         error_propagate(errp, err);
@@ -2596,6 +2594,8 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
         goto fail;
     }
 
+    blk_apply_root_state(blk, medium_bs);
+
     qmp_blockdev_close_tray(device, errp);
 
 fail:
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index 744a524..77b95d6 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -574,11 +574,16 @@ static void test_accounting(void)
 static void test_groups(void)
 {
     ThrottleConfig cfg1, cfg2;
+    BlockBackend *blk1, *blk2, *blk3;
     BlockDriverState *bdrv1, *bdrv2, *bdrv3;
 
-    bdrv1 = bdrv_new();
-    bdrv2 = bdrv_new();
-    bdrv3 = bdrv_new();
+    blk1 = blk_new_with_bs(&error_abort);
+    blk2 = blk_new_with_bs(&error_abort);
+    blk3 = blk_new_with_bs(&error_abort);
+
+    bdrv1 = blk_bs(blk1);
+    bdrv2 = blk_bs(blk2);
+    bdrv3 = blk_bs(blk3);
 
     g_assert(bdrv1->throttle_state == NULL);
     g_assert(bdrv2->throttle_state == NULL);
-- 
1.8.3.1
