block: Call transaction callbacks with lock held
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 <kwolf@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-ID: <20230911094620.45040-12-kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
diff --git a/block.c b/block.c
index f6e7cf4..f06de58 100644
--- a/block.c
+++ b/block.c
@@ -2375,21 +2375,21 @@
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) {
@@ -2408,7 +2408,6 @@
assert(s->child->quiesced_parent);
bdrv_replace_child_noperm(s->child, s->old_bs);
- bdrv_graph_wrunlock();
bdrv_unref(new_bs);
}
@@ -2426,6 +2425,9 @@
* 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
@@ -2951,16 +2953,15 @@
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);
@@ -2984,7 +2985,7 @@
tran_commit(tran);
}
- bdrv_unref(bs);
+ bdrv_schedule_unref(bs);
bdrv_child_free(s->child);
}
@@ -2998,6 +2999,9 @@
*
* 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
@@ -3114,6 +3118,9 @@
* 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,
@@ -3180,8 +3187,8 @@
ret = bdrv_refresh_perms(child_bs, tran, errp);
out:
- bdrv_graph_wrunlock();
tran_finalize(tran, ret);
+ bdrv_graph_wrunlock();
bdrv_unref(child_bs);
@@ -3227,8 +3234,8 @@
}
out:
- bdrv_graph_wrunlock();
tran_finalize(tran, ret);
+ bdrv_graph_wrunlock();
bdrv_unref(child_bs);
@@ -3393,6 +3400,9 @@
* 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 int GRAPH_WRLOCK
bdrv_set_file_or_backing_noperm(BlockDriverState *parent_bs,
@@ -3488,6 +3498,9 @@
*
* If a backing child is already present (i.e. we're detaching a node), that
* child node must be drained.
+ *
+ * After calling this function, the transaction @tran may only be completed
+ * while holding a writer lock for the graph.
*/
static int GRAPH_WRLOCK
bdrv_set_backing_noperm(BlockDriverState *bs,
@@ -3519,8 +3532,8 @@
ret = bdrv_refresh_perms(bs, tran, errp);
out:
- bdrv_graph_wrunlock();
tran_finalize(tran, ret);
+ bdrv_graph_wrunlock();
return ret;
}
@@ -4594,7 +4607,9 @@
aio_context_release(ctx);
}
+ bdrv_graph_wrlock(NULL);
tran_commit(tran);
+ bdrv_graph_wrunlock();
QTAILQ_FOREACH_REVERSE(bs_entry, bs_queue, entry) {
BlockDriverState *bs = bs_entry->state.bs;
@@ -4611,7 +4626,9 @@
goto cleanup;
abort:
+ bdrv_graph_wrlock(NULL);
tran_abort(tran);
+ bdrv_graph_wrunlock();
QTAILQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
if (bs_entry->prepared) {
@@ -4678,6 +4695,9 @@
* true and reopen_state->new_backing_bs contains a pointer to the new
* backing BlockDriverState (or NULL).
*
+ * After calling this function, the transaction @tran may only be completed
+ * while holding a writer lock for the graph.
+ *
* Return 0 on success, otherwise return < 0 and set @errp.
*
* The caller must hold the AioContext lock of @reopen_state->bs.
@@ -4811,6 +4831,9 @@
* commit() for any other BDS that have been left in a prepare() state
*
* The caller must hold the AioContext lock of @reopen_state->bs.
+ *
+ * After calling this function, the transaction @change_child_tran may only be
+ * completed while holding a writer lock for the graph.
*/
static int bdrv_reopen_prepare(BDRVReopenState *reopen_state,
BlockReopenQueue *queue,
@@ -5262,6 +5285,9 @@
* Function doesn't update permissions, caller is responsible for this.
*
* @child->bs (if non-NULL) must be drained.
+ *
+ * After calling this function, the transaction @tran may only be completed
+ * while holding a writer lock for the graph.
*/
static void GRAPH_WRLOCK bdrv_remove_child(BdrvChild *child, Transaction *tran)
{
@@ -5280,6 +5306,9 @@
/*
* Both @from and @to (if non-NULL) must be drained. @to 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.
*/
static int GRAPH_WRLOCK
bdrv_replace_node_noperm(BlockDriverState *from,
@@ -5386,8 +5415,8 @@
ret = 0;
out:
- bdrv_graph_wrunlock();
tran_finalize(tran, ret);
+ bdrv_graph_wrunlock();
bdrv_drained_end(to);
bdrv_drained_end(from);
@@ -5483,12 +5512,10 @@
ret = bdrv_refresh_perms(bs_new, tran, errp);
out:
- bdrv_graph_wrunlock();
tran_finalize(tran, ret);
- bdrv_graph_rdlock_main_loop();
bdrv_refresh_limits(bs_top, NULL, NULL);
- bdrv_graph_rdunlock_main_loop();
+ bdrv_graph_wrunlock();
bdrv_drained_end(bs_top);
bdrv_drained_end(bs_new);
@@ -5523,10 +5550,10 @@
refresh_list = g_slist_prepend(refresh_list, new_bs);
ret = bdrv_list_refresh_perms(refresh_list, NULL, tran, errp);
- bdrv_graph_wrunlock();
tran_finalize(tran, ret);
+ bdrv_graph_wrunlock();
bdrv_drained_end(old_bs);
bdrv_drained_end(new_bs);
bdrv_unref(old_bs);
