In previous patches, we changed some transactionable functions to be
marked as GRAPH_WRLOCK, but required that tran_finalize() is still
called without the lock. This was because all callbacks that can be in
the same transaction need to follow the same convention.
Now that we don't have conflicting requirements any more, we can switch
all of the transaction callbacks to be declared GRAPH_WRLOCK, too, and
call tran_finalize() with the lock held.
Document for each of these transactionable functions that the lock needs
to be held when completing the transaction, and make sure that all
callers down to the place where the transaction is finalised actually
have the writer lock.
Signed-off-by: Kevin Wolf
---
block.c | 61 +
1 file changed, 44 insertions(+), 17 deletions(-)
diff --git a/block.c b/block.c
index 780d129007..064851e0e1 100644
--- a/block.c
+++ b/block.c
@@ -2373,21 +2373,21 @@ typedef struct BdrvReplaceChildState {
BlockDriverState *old_bs;
} BdrvReplaceChildState;
-static void bdrv_replace_child_commit(void *opaque)
+static void GRAPH_WRLOCK bdrv_replace_child_commit(void *opaque)
{
BdrvReplaceChildState *s = opaque;
GLOBAL_STATE_CODE();
-bdrv_unref(s->old_bs);
+bdrv_schedule_unref(s->old_bs);
}
-static void bdrv_replace_child_abort(void *opaque)
+static void GRAPH_WRLOCK bdrv_replace_child_abort(void *opaque)
{
BdrvReplaceChildState *s = opaque;
BlockDriverState *new_bs = s->child->bs;
GLOBAL_STATE_CODE();
-bdrv_graph_wrlock(s->old_bs);
+assert_bdrv_graph_writable();
/* old_bs reference is transparently moved from @s to @s->child */
if (!s->child->bs) {
@@ -2406,7 +2406,6 @@ static void bdrv_replace_child_abort(void *opaque)
assert(s->child->quiesced_parent);
bdrv_replace_child_noperm(s->child, s->old_bs);
-bdrv_graph_wrunlock();
bdrv_unref(new_bs);
}
@@ -2424,6 +2423,9 @@ static TransactionActionDrv bdrv_replace_child_drv = {
* Both @child->bs and @new_bs (if non-NULL) must be drained. @new_bs must be
* kept drained until the transaction is completed.
*
+ * After calling this function, the transaction @tran may only be completed
+ * while holding a writer lock for the graph.
+ *
* The function doesn't update permissions, caller is responsible for this.
*/
static void GRAPH_WRLOCK
@@ -2949,16 +2951,15 @@ typedef struct BdrvAttachChildCommonState {
AioContext *old_child_ctx;
} BdrvAttachChildCommonState;
-static void bdrv_attach_child_common_abort(void *opaque)
+static void GRAPH_WRLOCK bdrv_attach_child_common_abort(void *opaque)
{
BdrvAttachChildCommonState *s = opaque;
BlockDriverState *bs = s->child->bs;
GLOBAL_STATE_CODE();
+assert_bdrv_graph_writable();
-bdrv_graph_wrlock(NULL);
bdrv_replace_child_noperm(s->child, NULL);
-bdrv_graph_wrunlock();
if (bdrv_get_aio_context(bs) != s->old_child_ctx) {
bdrv_try_change_aio_context(bs, s->old_child_ctx, NULL, &error_abort);
@@ -2982,7 +2983,7 @@ static void bdrv_attach_child_common_abort(void *opaque)
tran_commit(tran);
}
-bdrv_unref(bs);
+bdrv_schedule_unref(bs);
bdrv_child_free(s->child);
}
@@ -2996,6 +2997,9 @@ static TransactionActionDrv bdrv_attach_child_common_drv
= {
*
* Function doesn't update permissions, caller is responsible for this.
*
+ * After calling this function, the transaction @tran may only be completed
+ * while holding a writer lock for the graph.
+ *
* Returns new created child.
*
* The caller must hold the AioContext lock for @child_bs. Both @parent_bs and
@@ -3112,6 +3116,9 @@ bdrv_attach_child_common(BlockDriverState *child_bs,
* The caller must hold the AioContext lock for @child_bs. Both @parent_bs and
* @child_bs can move to a different AioContext in this function. Callers must
* make sure that their AioContext locking is still correct after this.
+ *
+ * After calling this function, the transaction @tran may only be completed
+ * while holding a writer lock for the graph.
*/
static BdrvChild * GRAPH_WRLOCK
bdrv_attach_child_noperm(BlockDriverState *parent_bs,
@@ -3178,8 +3185,8 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState
*child_bs,
ret = bdrv_refresh_perms(child_bs, tran, errp);
out:
-bdrv_graph_wrunlock();
tran_finalize(tran, ret);
+bdrv_graph_wrunlock();
bdrv_unref(child_bs);
@@ -3225,8 +3232,8 @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,
}
out:
-bdrv_graph_wrunlock();
tran_finalize(tran, ret);
+bdrv_graph_wrunlock();
bdrv_unref(child_bs);
@@ -3391,6 +3398,9 @@ static BdrvChildRole bdrv_backing_role(BlockDriverState
*bs)
* The caller must hold the AioContext lock for @child_bs. Both @parent_bs and
* @child_bs can move to a different AioContext in this function. Callers must
* make sure that their AioContext locking is still correct after this.
+ *
+ *
