[Qemu-devel] [PATCH 5/8] throttle: acquire the ThrottleGroup lock in bdrv_swap()

[Alberto Garcia]

bdrv_swap() touches the fields of a BlockDriverState that are
protected by the ThrottleGroup lock. Although those fields end up in
their original place, they are temporarily swapped in the process,
so there's a chance that an operation on a member of the same group
happening on a different thread can try to use them.

Signed-off-by: Alberto Garcia <address@hidden>
---
 block.c                         | 16 ++++++++++++++++
 block/throttle-groups.c         | 31 ++++++++++++++++++++++++++++++-
 include/block/throttle-groups.h |  3 +++
 3 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 1b249b2..290c2e3 100644
--- a/block.c
+++ b/block.c
@@ -36,6 +36,7 @@
 #include "qmp-commands.h"
 #include "qemu/timer.h"
 #include "qapi-event.h"
+#include "block/throttle-groups.h"
 
 #ifdef CONFIG_BSD
 #include <sys/types.h>
@@ -1876,11 +1877,20 @@ void bdrv_swap(BlockDriverState *bs_new, 
BlockDriverState *bs_old)
         QTAILQ_REMOVE(&graph_bdrv_states, bs_old, node_list);
     }
 
+    /* If the BlockDriverState is part of a throttling group acquire
+     * its lock since we're going to mess with the protected fields.
+     * Otherwise there's no need to worry since no one else can touch
+     * them. */
+    if (bs_old->throttle_state) {
+        throttle_group_lock(bs_old);
+    }
+
     /* bs_new must be unattached and shouldn't have anything fancy enabled */
     assert(!bs_new->blk);
     assert(QLIST_EMPTY(&bs_new->dirty_bitmaps));
     assert(bs_new->job == NULL);
     assert(bs_new->io_limits_enabled == false);
+    assert(bs_new->throttle_state == NULL);
     assert(!throttle_timers_are_initialized(&bs_new->throttle_timers));
 
     tmp = *bs_new;
@@ -1898,8 +1908,14 @@ void bdrv_swap(BlockDriverState *bs_new, 
BlockDriverState *bs_old)
     /* Check a few fields that should remain attached to the device */
     assert(bs_new->job == NULL);
     assert(bs_new->io_limits_enabled == false);
+    assert(bs_new->throttle_state == NULL);
     assert(!throttle_timers_are_initialized(&bs_new->throttle_timers));
 
+    /* Release the ThrottleGroup lock */
+    if (bs_old->throttle_state) {
+        throttle_group_unlock(bs_old);
+    }
+
     /* insert the nodes back into the graph node list if needed */
     if (bs_new->node_name[0] != '\0') {
         QTAILQ_INSERT_TAIL(&graph_bdrv_states, bs_new, node_list);
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index da8c70c..efc462f 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -33,7 +33,8 @@
  * its own locking.
  *
  * This locking is however handled internally in this file, so it's
- * transparent to outside users.
+ * mostly transparent to outside users (but see the documentation in
+ * throttle_groups_lock()).
  *
  * The whole ThrottleGroup structure is private and invisible to
  * outside users, that only use it through its ThrottleState.
@@ -459,6 +460,34 @@ void throttle_group_unregister_bs(BlockDriverState *bs)
     bs->throttle_state = NULL;
 }
 
+/* Acquire the lock of this throttling group.
+ *
+ * You won't normally need to use this. None of the functions from the
+ * ThrottleGroup API require you to acquire the lock since all of them
+ * deal with it internally.
+ *
+ * This should only be used in exceptional cases when you want to
+ * access the protected fields of a BlockDriverState directly
+ * (e.g. bdrv_swap()).
+ *
+ * @bs: a BlockDriverState that is member of the group
+ */
+void throttle_group_lock(BlockDriverState *bs)
+{
+    ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
+    qemu_mutex_lock(&tg->lock);
+}
+
+/* Release the lock of this throttling group.
+ *
+ * See the comments in throttle_group_lock().
+ */
+void throttle_group_unlock(BlockDriverState *bs)
+{
+    ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
+    qemu_mutex_unlock(&tg->lock);
+}
+
 static void throttle_groups_init(void)
 {
     qemu_mutex_init(&throttle_groups_lock);
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 322139a..fab113f 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -40,4 +40,7 @@ void coroutine_fn 
throttle_group_co_io_limits_intercept(BlockDriverState *bs,
                                                         unsigned int bytes,
                                                         bool is_write);
 
+void throttle_group_lock(BlockDriverState *bs);
+void throttle_group_unlock(BlockDriverState *bs);
+
 #endif
-- 
2.1.4