From f3bbc53dc56c5d410f76442da6ad15ec8f9439fc Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 27 Oct 2023 17:53:14 +0200 Subject: block: Mark block_job_add_bdrv() GRAPH_WRLOCK Instead of taking the writer lock internally, require callers to already hold it when calling block_job_add_bdrv(). These callers will typically already hold the graph lock once the locking work is completed, which means that they can't call functions that take it internally. Signed-off-by: Kevin Wolf Message-ID: <20231027155333.420094-6-kwolf@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- tests/unit/test-bdrv-drain.c | 3 +++ 1 file changed, 3 insertions(+) (limited to 'tests') diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index f67e9df01c..40d17b4c5a 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -794,7 +794,10 @@ static void test_blockjob_common_drain_node(enum drain_type drain_type, 0, 0, NULL, NULL, &error_abort); tjob->bs = src; job = &tjob->common; + + bdrv_graph_wrlock(target); block_job_add_bdrv(job, "target", target, 0, BLK_PERM_ALL, &error_abort); + bdrv_graph_wrunlock(); switch (result) { case TEST_JOB_SUCCESS: -- cgit 1.4.1 From ccd6a37947574707613e826e2bf04d55f1d5f238 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 27 Oct 2023 17:53:25 +0200 Subject: block: Mark bdrv_replace_node() GRAPH_WRLOCK Instead of taking the writer lock internally, require callers to already hold it when calling bdrv_replace_node(). Its callers may already want to hold the graph lock and so wouldn't be able to call functions that take it internally. Signed-off-by: Kevin Wolf Message-ID: <20231027155333.420094-17-kwolf@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block.c | 30 +++++++++++------------------- block/commit.c | 13 +++++++++++-- block/mirror.c | 26 ++++++++++++++++---------- blockdev.c | 5 +++++ include/block/block-global-state.h | 6 ++++-- tests/unit/test-bdrv-drain.c | 6 ++++++ tests/unit/test-bdrv-graph-mod.c | 13 +++++++++++-- 7 files changed, 64 insertions(+), 35 deletions(-) (limited to 'tests') diff --git a/block.c b/block.c index c7409cf658..cac517ab8b 100644 --- a/block.c +++ b/block.c @@ -5484,25 +5484,7 @@ out: int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, Error **errp) { - int ret; - - GLOBAL_STATE_CODE(); - - /* Make sure that @from doesn't go away until we have successfully attached - * all of its parents to @to. */ - bdrv_ref(from); - bdrv_drained_begin(from); - bdrv_drained_begin(to); - bdrv_graph_wrlock(to); - - ret = bdrv_replace_node_common(from, to, true, false, errp); - - bdrv_graph_wrunlock(); - bdrv_drained_end(to); - bdrv_drained_end(from); - bdrv_unref(from); - - return ret; + return bdrv_replace_node_common(from, to, true, false, errp); } int bdrv_drop_filter(BlockDriverState *bs, Error **errp) @@ -5717,9 +5699,19 @@ BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *options, goto fail; } + /* + * Make sure that @bs doesn't go away until we have successfully attached + * all of its parents to @new_node_bs and undrained it again. + */ + bdrv_ref(bs); bdrv_drained_begin(bs); + bdrv_drained_begin(new_node_bs); + bdrv_graph_wrlock(new_node_bs); ret = bdrv_replace_node(bs, new_node_bs, errp); + bdrv_graph_wrunlock(); + bdrv_drained_end(new_node_bs); bdrv_drained_end(bs); + bdrv_unref(bs); if (ret < 0) { error_prepend(errp, "Could not replace node: "); diff --git a/block/commit.c b/block/commit.c index d92af02ead..30bc082edc 100644 --- a/block/commit.c +++ b/block/commit.c @@ -68,6 +68,7 @@ static void commit_abort(Job *job) { CommitBlockJob *s = container_of(job, CommitBlockJob, common.job); BlockDriverState *top_bs = blk_bs(s->top); + BlockDriverState *commit_top_backing_bs; if (s->chain_frozen) { bdrv_graph_rdlock_main_loop(); @@ -94,8 +95,12 @@ static void commit_abort(Job *job) * XXX Can (or should) we somehow keep 'consistent read' blocked even * after the failed/cancelled commit job is gone? If we already wrote * something to base, the intermediate images aren't valid any more. */ - bdrv_replace_node(s->commit_top_bs, s->commit_top_bs->backing->bs, - &error_abort); + commit_top_backing_bs = s->commit_top_bs->backing->bs; + bdrv_drained_begin(commit_top_backing_bs); + bdrv_graph_wrlock(commit_top_backing_bs); + bdrv_replace_node(s->commit_top_bs, commit_top_backing_bs, &error_abort); + bdrv_graph_wrunlock(); + bdrv_drained_end(commit_top_backing_bs); bdrv_unref(s->commit_top_bs); bdrv_unref(top_bs); @@ -425,7 +430,11 @@ fail: /* commit_top_bs has to be replaced after deleting the block job, * otherwise this would fail because of lack of permissions. */ if (commit_top_bs) { + bdrv_drained_begin(top); + bdrv_graph_wrlock(top); bdrv_replace_node(commit_top_bs, top, &error_abort); + bdrv_graph_wrunlock(); + bdrv_drained_end(top); } } diff --git a/block/mirror.c b/block/mirror.c index f8e439371c..dfc1c416e8 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -712,6 +712,7 @@ static int mirror_exit_common(Job *job) * these permissions any more means that we can't allow any new requests on * mirror_top_bs from now on, so keep it drained. */ bdrv_drained_begin(mirror_top_bs); + bdrv_drained_begin(target_bs); bs_opaque->stop = true; bdrv_graph_rdlock_main_loop(); @@ -757,15 +758,13 @@ static int mirror_exit_common(Job *job) /* The mirror job has no requests in flight any more, but we need to * drain potential other users of the BDS before changing the graph. */ assert(s->in_drain); - bdrv_drained_begin(target_bs); + bdrv_drained_begin(to_replace); /* * Cannot use check_to_replace_node() here, because that would * check for an op blocker on @to_replace, and we have our own * there. - * - * TODO Pull out the writer lock from bdrv_replace_node() to here */ - bdrv_graph_rdlock_main_loop(); + bdrv_graph_wrlock(target_bs); if (bdrv_recurse_can_replace(src, to_replace)) { bdrv_replace_node(to_replace, target_bs, &local_err); } else { @@ -774,8 +773,8 @@ static int mirror_exit_common(Job *job) "would not lead to an abrupt change of visible data", to_replace->node_name, target_bs->node_name); } - bdrv_graph_rdunlock_main_loop(); - bdrv_drained_end(target_bs); + bdrv_graph_wrunlock(); + bdrv_drained_end(to_replace); if (local_err) { error_report_err(local_err); ret = -EPERM; @@ -790,7 +789,6 @@ static int mirror_exit_common(Job *job) aio_context_release(replace_aio_context); } g_free(s->replaces); - bdrv_unref(target_bs); /* * Remove the mirror filter driver from the graph. Before this, get rid of @@ -798,7 +796,12 @@ static int mirror_exit_common(Job *job) * valid. */ block_job_remove_all_bdrv(bjob); + bdrv_graph_wrlock(mirror_top_bs); bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs, &error_abort); + bdrv_graph_wrunlock(); + + bdrv_drained_end(target_bs); + bdrv_unref(target_bs); bs_opaque->job = NULL; @@ -1987,11 +1990,14 @@ fail: } bs_opaque->stop = true; - bdrv_graph_rdlock_main_loop(); + bdrv_drained_begin(bs); + bdrv_graph_wrlock(bs); + assert(mirror_top_bs->backing->bs == bs); bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing, &error_abort); - bdrv_graph_rdunlock_main_loop(); - bdrv_replace_node(mirror_top_bs, mirror_top_bs->backing->bs, &error_abort); + bdrv_replace_node(mirror_top_bs, bs, &error_abort); + bdrv_graph_wrunlock(); + bdrv_drained_end(bs); bdrv_unref(mirror_top_bs); diff --git a/blockdev.c b/blockdev.c index f04faf6373..5bc921236c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1610,7 +1610,12 @@ static void external_snapshot_abort(void *opaque) aio_context_acquire(aio_context); } + bdrv_drained_begin(state->new_bs); + bdrv_graph_wrlock(state->old_bs); bdrv_replace_node(state->new_bs, state->old_bs, &error_abort); + bdrv_graph_wrunlock(); + bdrv_drained_end(state->new_bs); + bdrv_unref(state->old_bs); /* bdrv_replace_node() ref'ed old_bs */ aio_context_release(aio_context); diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index a1fd70ec97..9e0ccc1c32 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -71,8 +71,10 @@ bdrv_co_create_file(const char *filename, QemuOpts *opts, Error **errp); BlockDriverState *bdrv_new(void); int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, Error **errp); -int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, - Error **errp); + +int GRAPH_WRLOCK +bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, Error **errp); + int bdrv_replace_child_bs(BdrvChild *child, BlockDriverState *new_bs, Error **errp); BlockDriverState *bdrv_insert_node(BlockDriverState *bs, QDict *node_options, diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index 40d17b4c5a..b16f831c23 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -2000,7 +2000,13 @@ static void do_test_replace_child_mid_drain(int old_drain_count, parent_s->was_undrained = false; g_assert(parent_bs->quiesce_counter == old_drain_count); + bdrv_drained_begin(old_child_bs); + bdrv_drained_begin(new_child_bs); + bdrv_graph_wrlock(NULL); bdrv_replace_node(old_child_bs, new_child_bs, &error_abort); + bdrv_graph_wrunlock(); + bdrv_drained_end(new_child_bs); + bdrv_drained_end(old_child_bs); g_assert(parent_bs->quiesce_counter == new_drain_count); if (!old_drain_count && !new_drain_count) { diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c index 8609f7f42b..22d4cd83f6 100644 --- a/tests/unit/test-bdrv-graph-mod.c +++ b/tests/unit/test-bdrv-graph-mod.c @@ -234,11 +234,16 @@ static void test_parallel_exclusive_write(void) BlockDriverState *fl1 = pass_through_node("fl1"); BlockDriverState *fl2 = pass_through_node("fl2"); + bdrv_drained_begin(fl1); + bdrv_drained_begin(fl2); + /* * bdrv_attach_child() eats child bs reference, so we need two @base - * references for two filters: + * references for two filters. We also need an additional @fl1 reference so + * that it still exists when we want to undrain it. */ bdrv_ref(base); + bdrv_ref(fl1); bdrv_graph_wrlock(NULL); bdrv_attach_child(top, fl1, "backing", &child_of_bds, @@ -250,10 +255,14 @@ static void test_parallel_exclusive_write(void) bdrv_attach_child(fl2, base, "backing", &child_of_bds, BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY, &error_abort); - bdrv_graph_wrunlock(); bdrv_replace_node(fl1, fl2, &error_abort); + bdrv_graph_wrunlock(); + + bdrv_drained_end(fl2); + bdrv_drained_end(fl1); + bdrv_unref(fl1); bdrv_unref(fl2); bdrv_unref(top); } -- cgit 1.4.1 From 004915a96a7a40e942ac85e6d22518cbcd283506 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 27 Oct 2023 17:53:26 +0200 Subject: block: Protect bs->backing with graph_lock Almost all functions that access bs->backing already take the graph lock now. Add locking to the remaining users and finally annotate the struct field itself as protected by the graph lock. Signed-off-by: Kevin Wolf Message-ID: <20231027155333.420094-18-kwolf@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block.c | 27 +++++++++++++++++---------- block/commit.c | 5 ++++- block/mirror.c | 20 +++++++++++++++----- block/qed.c | 2 +- block/replication.c | 7 ++++++- block/vmdk.c | 7 ++++--- include/block/block_int-common.h | 2 +- tests/unit/test-bdrv-drain.c | 22 ++++++++++++++++++---- tests/unit/test-bdrv-graph-mod.c | 5 ++++- 9 files changed, 70 insertions(+), 27 deletions(-) (limited to 'tests') diff --git a/block.c b/block.c index cac517ab8b..ed0afdffbd 100644 --- a/block.c +++ b/block.c @@ -3560,10 +3560,14 @@ out: int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd, Error **errp) { - BlockDriverState *drain_bs = bs->backing ? bs->backing->bs : bs; + BlockDriverState *drain_bs; int ret; GLOBAL_STATE_CODE(); + bdrv_graph_rdlock_main_loop(); + drain_bs = bs->backing ? bs->backing->bs : bs; + bdrv_graph_rdunlock_main_loop(); + bdrv_ref(drain_bs); bdrv_drained_begin(drain_bs); bdrv_graph_wrlock(backing_hd); @@ -3602,6 +3606,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, Error *local_err = NULL; GLOBAL_STATE_CODE(); + GRAPH_RDLOCK_GUARD_MAINLOOP(); if (bs->backing != NULL) { goto free_exit; @@ -3643,10 +3648,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, implicit_backing = !strcmp(bs->auto_backing_file, bs->backing_file); } - bdrv_graph_rdlock_main_loop(); backing_filename = bdrv_get_full_backing_filename(bs, &local_err); - bdrv_graph_rdunlock_main_loop(); - if (local_err) { ret = -EINVAL; error_propagate(errp, local_err); @@ -3677,9 +3679,7 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options, } if (implicit_backing) { - bdrv_graph_rdlock_main_loop(); bdrv_refresh_filename(backing_hd); - bdrv_graph_rdunlock_main_loop(); pstrcpy(bs->auto_backing_file, sizeof(bs->auto_backing_file), backing_hd->filename); } @@ -4750,8 +4750,8 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state, { BlockDriverState *bs = reopen_state->bs; BlockDriverState *new_child_bs; - BlockDriverState *old_child_bs = is_backing ? child_bs(bs->backing) : - child_bs(bs->file); + BlockDriverState *old_child_bs; + const char *child_name = is_backing ? "backing" : "file"; QObject *value; const char *str; @@ -4797,6 +4797,7 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state, g_assert_not_reached(); } + old_child_bs = is_backing ? child_bs(bs->backing) : child_bs(bs->file); if (old_child_bs == new_child_bs) { ret = 0; goto out_rdlock; @@ -5008,13 +5009,16 @@ bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, * file or if the image file has a backing file name as part of * its metadata. Otherwise the 'backing' option can be omitted. */ + bdrv_graph_rdlock_main_loop(); if (drv->supports_backing && reopen_state->backing_missing && (reopen_state->bs->backing || reopen_state->bs->backing_file[0])) { error_setg(errp, "backing is missing for '%s'", reopen_state->bs->node_name); + bdrv_graph_rdunlock_main_loop(); ret = -EINVAL; goto error; } + bdrv_graph_rdunlock_main_loop(); /* * Allow changing the 'backing' option. The new value can be @@ -5204,10 +5208,11 @@ static void bdrv_close(BlockDriverState *bs) QLIST_FOREACH_SAFE(child, &bs->children, next, next) { bdrv_unref_child(bs, child); } - bdrv_graph_wrunlock(); assert(!bs->backing); assert(!bs->file); + bdrv_graph_wrunlock(); + g_free(bs->opaque); bs->opaque = NULL; qatomic_set(&bs->copy_on_read, 0); @@ -5531,7 +5536,9 @@ int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top, GLOBAL_STATE_CODE(); + bdrv_graph_rdlock_main_loop(); assert(!bs_new->backing); + bdrv_graph_rdunlock_main_loop(); old_context = bdrv_get_aio_context(bs_top); bdrv_drained_begin(bs_top); @@ -8115,7 +8122,7 @@ static bool append_strong_runtime_options(QDict *d, BlockDriverState *bs) /* Note: This function may return false positives; it may return true * even if opening the backing file specified by bs's image header * would result in exactly bs->backing. */ -static bool bdrv_backing_overridden(BlockDriverState *bs) +static bool GRAPH_RDLOCK bdrv_backing_overridden(BlockDriverState *bs) { GLOBAL_STATE_CODE(); if (bs->backing) { diff --git a/block/commit.c b/block/commit.c index 30bc082edc..eb3dc01f45 100644 --- a/block/commit.c +++ b/block/commit.c @@ -95,7 +95,10 @@ static void commit_abort(Job *job) * XXX Can (or should) we somehow keep 'consistent read' blocked even * after the failed/cancelled commit job is gone? If we already wrote * something to base, the intermediate images aren't valid any more. */ + bdrv_graph_rdlock_main_loop(); commit_top_backing_bs = s->commit_top_bs->backing->bs; + bdrv_graph_rdunlock_main_loop(); + bdrv_drained_begin(commit_top_backing_bs); bdrv_graph_wrlock(commit_top_backing_bs); bdrv_replace_node(s->commit_top_bs, commit_top_backing_bs, &error_abort); @@ -219,7 +222,7 @@ bdrv_commit_top_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes, return bdrv_co_preadv(bs->backing, offset, bytes, qiov, flags); } -static void bdrv_commit_top_refresh_filename(BlockDriverState *bs) +static GRAPH_RDLOCK void bdrv_commit_top_refresh_filename(BlockDriverState *bs) { pstrcpy(bs->exact_filename, sizeof(bs->exact_filename), bs->backing->bs->filename); diff --git a/block/mirror.c b/block/mirror.c index dfc1c416e8..2096fade90 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -479,7 +479,7 @@ static unsigned mirror_perform(MirrorBlockJob *s, int64_t offset, return bytes_handled; } -static void coroutine_fn mirror_iteration(MirrorBlockJob *s) +static void coroutine_fn GRAPH_RDLOCK mirror_iteration(MirrorBlockJob *s) { BlockDriverState *source = s->mirror_top_bs->backing->bs; MirrorOp *pseudo_op; @@ -839,14 +839,18 @@ static void coroutine_fn mirror_throttle(MirrorBlockJob *s) } } -static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s) +static int coroutine_fn GRAPH_UNLOCKED mirror_dirty_init(MirrorBlockJob *s) { int64_t offset; - BlockDriverState *bs = s->mirror_top_bs->backing->bs; + BlockDriverState *bs; BlockDriverState *target_bs = blk_bs(s->target); int ret; int64_t count; + bdrv_graph_co_rdlock(); + bs = s->mirror_top_bs->backing->bs; + bdrv_graph_co_rdunlock(); + if (s->zero_target) { if (!bdrv_can_write_zeroes_with_unmap(target_bs)) { bdrv_set_dirty_bitmap(s->dirty_bitmap, 0, s->bdev_length); @@ -926,7 +930,7 @@ static int coroutine_fn mirror_flush(MirrorBlockJob *s) static int coroutine_fn mirror_run(Job *job, Error **errp) { MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job); - BlockDriverState *bs = s->mirror_top_bs->backing->bs; + BlockDriverState *bs; MirrorBDSOpaque *mirror_top_opaque = s->mirror_top_bs->opaque; BlockDriverState *target_bs = blk_bs(s->target); bool need_drain = true; @@ -938,6 +942,10 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) checking for a NULL string */ int ret = 0; + bdrv_graph_co_rdlock(); + bs = bdrv_filter_bs(s->mirror_top_bs); + bdrv_graph_co_rdunlock(); + if (job_is_cancelled(&s->common.job)) { goto immediate_exit; } @@ -1070,7 +1078,9 @@ static int coroutine_fn mirror_run(Job *job, Error **errp) mirror_wait_for_free_in_flight_slot(s); continue; } else if (cnt != 0) { + bdrv_graph_co_rdlock(); mirror_iteration(s); + bdrv_graph_co_rdunlock(); } } @@ -1640,7 +1650,7 @@ bdrv_mirror_top_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes) offset, bytes, NULL, 0); } -static void bdrv_mirror_top_refresh_filename(BlockDriverState *bs) +static void GRAPH_RDLOCK bdrv_mirror_top_refresh_filename(BlockDriverState *bs) { if (bs->backing == NULL) { /* we can be here after failed bdrv_attach_child in diff --git a/block/qed.c b/block/qed.c index 45ae320290..686ad711f7 100644 --- a/block/qed.c +++ b/block/qed.c @@ -1138,7 +1138,7 @@ out: /** * Check if the QED_F_NEED_CHECK bit should be set during allocating write */ -static bool qed_should_set_need_check(BDRVQEDState *s) +static bool GRAPH_RDLOCK qed_should_set_need_check(BDRVQEDState *s) { /* The flush before L2 update path ensures consistency */ if (s->bs->backing) { diff --git a/block/replication.c b/block/replication.c index d522c7396f..49ecc608b2 100644 --- a/block/replication.c +++ b/block/replication.c @@ -363,6 +363,9 @@ static void reopen_backing_file(BlockDriverState *bs, bool writable, BdrvChild *hidden_disk, *secondary_disk; BlockReopenQueue *reopen_queue = NULL; + GLOBAL_STATE_CODE(); + GRAPH_RDLOCK_GUARD_MAINLOOP(); + /* * s->hidden_disk and s->secondary_disk may not be set yet, as they will * only be set after the children are writable. @@ -496,9 +499,11 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode, case REPLICATION_MODE_PRIMARY: break; case REPLICATION_MODE_SECONDARY: + bdrv_graph_rdlock_main_loop(); active_disk = bs->file; if (!active_disk || !active_disk->bs || !active_disk->bs->backing) { error_setg(errp, "Active disk doesn't have backing file"); + bdrv_graph_rdunlock_main_loop(); aio_context_release(aio_context); return; } @@ -506,11 +511,11 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode, hidden_disk = active_disk->bs->backing; if (!hidden_disk->bs || !hidden_disk->bs->backing) { error_setg(errp, "Hidden disk doesn't have backing file"); + bdrv_graph_rdunlock_main_loop(); aio_context_release(aio_context); return; } - bdrv_graph_rdlock_main_loop(); secondary_disk = hidden_disk->bs->backing; if (!secondary_disk->bs || !bdrv_has_blk(secondary_disk->bs)) { error_setg(errp, "The secondary disk doesn't have block backend"); diff --git a/block/vmdk.c b/block/vmdk.c index 91ed7a8d93..5c789bb65b 100644 --- a/block/vmdk.c +++ b/block/vmdk.c @@ -380,7 +380,7 @@ out: return ret; } -static int coroutine_fn vmdk_is_cid_valid(BlockDriverState *bs) +static int coroutine_fn GRAPH_RDLOCK vmdk_is_cid_valid(BlockDriverState *bs) { BDRVVmdkState *s = bs->opaque; uint32_t cur_pcid; @@ -3044,8 +3044,9 @@ vmdk_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) return 0; } -static void vmdk_gather_child_options(BlockDriverState *bs, QDict *target, - bool backing_overridden) +static void GRAPH_RDLOCK +vmdk_gather_child_options(BlockDriverState *bs, QDict *target, + bool backing_overridden) { /* No children but file and backing can be explicitly specified (TODO) */ qdict_put(target, "file", diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index c0db862de7..ed6066929a 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -1178,7 +1178,7 @@ struct BlockDriverState { * are connected with BdrvChildRole. */ QLIST_HEAD(, BdrvChild GRAPH_RDLOCK_PTR) children; - BdrvChild *backing; + BdrvChild * GRAPH_RDLOCK_PTR backing; BdrvChild *file; QLIST_HEAD(, BdrvChild GRAPH_RDLOCK_PTR) parents; diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index b16f831c23..ba4e42b197 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -218,8 +218,14 @@ static void do_drain_end_unlocked(enum drain_type drain_type, BlockDriverState * } } -static void test_drv_cb_common(BlockBackend *blk, enum drain_type drain_type, - bool recursive) +/* + * Locking the block graph would be a bit cumbersome here because this function + * is called both in coroutine and non-coroutine context. We know this is a test + * and nothing else is running, so don't bother with TSA. + */ +static void coroutine_mixed_fn TSA_NO_TSA +test_drv_cb_common(BlockBackend *blk, enum drain_type drain_type, + bool recursive) { BlockDriverState *bs = blk_bs(blk); BlockDriverState *backing = bs->backing->bs; @@ -307,8 +313,14 @@ static void test_drv_cb_co_drain(void) blk_unref(blk); } -static void test_quiesce_common(BlockBackend *blk, enum drain_type drain_type, - bool recursive) +/* + * Locking the block graph would be a bit cumbersome here because this function + * is called both in coroutine and non-coroutine context. We know this is a test + * and nothing else is running, so don't bother with TSA. + */ +static void coroutine_mixed_fn TSA_NO_TSA +test_quiesce_common(BlockBackend *blk, enum drain_type drain_type, + bool recursive) { BlockDriverState *bs = blk_bs(blk); BlockDriverState *backing = bs->backing->bs; @@ -1868,6 +1880,8 @@ static void bdrv_replace_test_drain_end(BlockDriverState *bs) { BDRVReplaceTestState *s = bs->opaque; + GRAPH_RDLOCK_GUARD_MAINLOOP(); + if (!s->setup_completed) { return; } diff --git a/tests/unit/test-bdrv-graph-mod.c b/tests/unit/test-bdrv-graph-mod.c index 22d4cd83f6..878544dbd5 100644 --- a/tests/unit/test-bdrv-graph-mod.c +++ b/tests/unit/test-bdrv-graph-mod.c @@ -206,15 +206,18 @@ static void test_should_update_child(void) bdrv_set_backing_hd(target, bs, &error_abort); - g_assert(target->backing->bs == bs); bdrv_graph_wrlock(NULL); + g_assert(target->backing->bs == bs); bdrv_attach_child(filter, target, "target", &child_of_bds, BDRV_CHILD_DATA, &error_abort); bdrv_graph_wrunlock(); aio_context_acquire(qemu_get_aio_context()); bdrv_append(filter, bs, &error_abort); aio_context_release(qemu_get_aio_context()); + + bdrv_graph_rdlock_main_loop(); g_assert(target->backing->bs == bs); + bdrv_graph_rdunlock_main_loop(); bdrv_unref(filter); bdrv_unref(bs); -- cgit 1.4.1 From e2dd273754eb9a47c33660b4e14074e8e96ada4d Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 27 Oct 2023 17:53:28 +0200 Subject: block: Introduce bdrv_co_change_backing_file() bdrv_change_backing_file() is called both inside and outside coroutine context. This makes it difficult for it to take the graph lock internally. It also means that driver implementations need to be able to run outside of coroutines, too. Switch it to the usual model with a coroutine based implementation and a co_wrapper instead. The new function is marked GRAPH_RDLOCK. As the co_wrapper now runs the function in the AioContext of the node (as it should always have done), this is not GLOBAL_STATE_CODE() any more. Signed-off-by: Kevin Wolf Message-ID: <20231027155333.420094-20-kwolf@redhat.com> Reviewed-by: Eric Blake Signed-off-by: Kevin Wolf --- block.c | 11 ++-- block/qcow2.c | 128 +++++++++++++++++++------------------ block/qed.c | 64 +++++++++---------- include/block/block-global-state.h | 3 +- include/block/block-io.h | 8 +++ include/block/block_int-common.h | 5 +- tests/unit/test-bdrv-drain.c | 8 +-- 7 files changed, 120 insertions(+), 107 deletions(-) (limited to 'tests') diff --git a/block.c b/block.c index ed0afdffbd..4910b95d7d 100644 --- a/block.c +++ b/block.c @@ -5764,13 +5764,14 @@ int coroutine_fn bdrv_co_check(BlockDriverState *bs, * image file header * -ENOTSUP - format driver doesn't support changing the backing file */ -int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file, - const char *backing_fmt, bool require) +int coroutine_fn +bdrv_co_change_backing_file(BlockDriverState *bs, const char *backing_file, + const char *backing_fmt, bool require) { BlockDriver *drv = bs->drv; int ret; - GLOBAL_STATE_CODE(); + IO_CODE(); if (!drv) { return -ENOMEDIUM; @@ -5785,8 +5786,8 @@ int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file, return -EINVAL; } - if (drv->bdrv_change_backing_file != NULL) { - ret = drv->bdrv_change_backing_file(bs, backing_file, backing_fmt); + if (drv->bdrv_co_change_backing_file != NULL) { + ret = drv->bdrv_co_change_backing_file(bs, backing_file, backing_fmt); } else { ret = -ENOTSUP; } diff --git a/block/qcow2.c b/block/qcow2.c index a1443a31aa..875e613ea9 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -3155,8 +3155,9 @@ fail: return ret; } -static int qcow2_change_backing_file(BlockDriverState *bs, - const char *backing_file, const char *backing_fmt) +static int coroutine_fn GRAPH_RDLOCK +qcow2_co_change_backing_file(BlockDriverState *bs, const char *backing_file, + const char *backing_fmt) { BDRVQcow2State *s = bs->opaque; @@ -3816,8 +3817,11 @@ qcow2_co_create(BlockdevCreateOptions *create_options, Error **errp) backing_format = BlockdevDriver_str(qcow2_opts->backing_fmt); } - ret = bdrv_change_backing_file(blk_bs(blk), qcow2_opts->backing_file, - backing_format, false); + bdrv_graph_co_rdlock(); + ret = bdrv_co_change_backing_file(blk_bs(blk), qcow2_opts->backing_file, + backing_format, false); + bdrv_graph_co_rdunlock(); + if (ret < 0) { error_setg_errno(errp, -ret, "Could not assign backing file '%s' " "with format '%s'", qcow2_opts->backing_file, @@ -6115,64 +6119,64 @@ static const char *const qcow2_strong_runtime_opts[] = { }; BlockDriver bdrv_qcow2 = { - .format_name = "qcow2", - .instance_size = sizeof(BDRVQcow2State), - .bdrv_probe = qcow2_probe, - .bdrv_open = qcow2_open, - .bdrv_close = qcow2_close, - .bdrv_reopen_prepare = qcow2_reopen_prepare, - .bdrv_reopen_commit = qcow2_reopen_commit, - .bdrv_reopen_commit_post = qcow2_reopen_commit_post, - .bdrv_reopen_abort = qcow2_reopen_abort, - .bdrv_join_options = qcow2_join_options, - .bdrv_child_perm = bdrv_default_perms, - .bdrv_co_create_opts = qcow2_co_create_opts, - .bdrv_co_create = qcow2_co_create, - .bdrv_has_zero_init = qcow2_has_zero_init, - .bdrv_co_block_status = qcow2_co_block_status, - - .bdrv_co_preadv_part = qcow2_co_preadv_part, - .bdrv_co_pwritev_part = qcow2_co_pwritev_part, - .bdrv_co_flush_to_os = qcow2_co_flush_to_os, - - .bdrv_co_pwrite_zeroes = qcow2_co_pwrite_zeroes, - .bdrv_co_pdiscard = qcow2_co_pdiscard, - .bdrv_co_copy_range_from = qcow2_co_copy_range_from, - .bdrv_co_copy_range_to = qcow2_co_copy_range_to, - .bdrv_co_truncate = qcow2_co_truncate, - .bdrv_co_pwritev_compressed_part = qcow2_co_pwritev_compressed_part, - .bdrv_make_empty = qcow2_make_empty, - - .bdrv_snapshot_create = qcow2_snapshot_create, - .bdrv_snapshot_goto = qcow2_snapshot_goto, - .bdrv_snapshot_delete = qcow2_snapshot_delete, - .bdrv_snapshot_list = qcow2_snapshot_list, - .bdrv_snapshot_load_tmp = qcow2_snapshot_load_tmp, - .bdrv_measure = qcow2_measure, - .bdrv_co_get_info = qcow2_co_get_info, - .bdrv_get_specific_info = qcow2_get_specific_info, - - .bdrv_co_save_vmstate = qcow2_co_save_vmstate, - .bdrv_co_load_vmstate = qcow2_co_load_vmstate, - - .is_format = true, - .supports_backing = true, - .bdrv_change_backing_file = qcow2_change_backing_file, - - .bdrv_refresh_limits = qcow2_refresh_limits, - .bdrv_co_invalidate_cache = qcow2_co_invalidate_cache, - .bdrv_inactivate = qcow2_inactivate, - - .create_opts = &qcow2_create_opts, - .amend_opts = &qcow2_amend_opts, - .strong_runtime_opts = qcow2_strong_runtime_opts, - .mutable_opts = mutable_opts, - .bdrv_co_check = qcow2_co_check, - .bdrv_amend_options = qcow2_amend_options, - .bdrv_co_amend = qcow2_co_amend, - - .bdrv_detach_aio_context = qcow2_detach_aio_context, - .bdrv_attach_aio_context = qcow2_attach_aio_context, + .format_name = "qcow2", + .instance_size = sizeof(BDRVQcow2State), + .bdrv_probe = qcow2_probe, + .bdrv_open = qcow2_open, + .bdrv_close = qcow2_close, + .bdrv_reopen_prepare = qcow2_reopen_prepare, + .bdrv_reopen_commit = qcow2_reopen_commit, + .bdrv_reopen_commit_post = qcow2_reopen_commit_post, + .bdrv_reopen_abort = qcow2_reopen_abort, + .bdrv_join_options = qcow2_join_options, + .bdrv_child_perm = bdrv_default_perms, + .bdrv_co_create_opts = qcow2_co_create_opts, + .bdrv_co_create = qcow2_co_create, + .bdrv_has_zero_init = qcow2_has_zero_init, + .bdrv_co_block_status = qcow2_co_block_status, + + .bdrv_co_preadv_part = qcow2_co_preadv_part, + .bdrv_co_pwritev_part = qcow2_co_pwritev_part, + .bdrv_co_flush_to_os = qcow2_co_flush_to_os, + + .bdrv_co_pwrite_zeroes = qcow2_co_pwrite_zeroes, + .bdrv_co_pdiscard = qcow2_co_pdiscard, + .bdrv_co_copy_range_from = qcow2_co_copy_range_from, + .bdrv_co_copy_range_to = qcow2_co_copy_range_to, + .bdrv_co_truncate = qcow2_co_truncate, + .bdrv_co_pwritev_compressed_part = qcow2_co_pwritev_compressed_part, + .bdrv_make_empty = qcow2_make_empty, + + .bdrv_snapshot_create = qcow2_snapshot_create, + .bdrv_snapshot_goto = qcow2_snapshot_goto, + .bdrv_snapshot_delete = qcow2_snapshot_delete, + .bdrv_snapshot_list = qcow2_snapshot_list, + .bdrv_snapshot_load_tmp = qcow2_snapshot_load_tmp, + .bdrv_measure = qcow2_measure, + .bdrv_co_get_info = qcow2_co_get_info, + .bdrv_get_specific_info = qcow2_get_specific_info, + + .bdrv_co_save_vmstate = qcow2_co_save_vmstate, + .bdrv_co_load_vmstate = qcow2_co_load_vmstate, + + .is_format = true, + .supports_backing = true, + .bdrv_co_change_backing_file = qcow2_co_change_backing_file, + + .bdrv_refresh_limits = qcow2_refresh_limits, + .bdrv_co_invalidate_cache = qcow2_co_invalidate_cache, + .bdrv_inactivate = qcow2_inactivate, + + .create_opts = &qcow2_create_opts, + .amend_opts = &qcow2_amend_opts, + .strong_runtime_opts = qcow2_strong_runtime_opts, + .mutable_opts = mutable_opts, + .bdrv_co_check = qcow2_co_check, + .bdrv_amend_options = qcow2_amend_options, + .bdrv_co_amend = qcow2_co_amend, + + .bdrv_detach_aio_context = qcow2_detach_aio_context, + .bdrv_attach_aio_context = qcow2_attach_aio_context, .bdrv_supports_persistent_dirty_bitmap = qcow2_supports_persistent_dirty_bitmap, diff --git a/block/qed.c b/block/qed.c index 686ad711f7..996aa384fe 100644 --- a/block/qed.c +++ b/block/qed.c @@ -1498,9 +1498,9 @@ bdrv_qed_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi) return 0; } -static int bdrv_qed_change_backing_file(BlockDriverState *bs, - const char *backing_file, - const char *backing_fmt) +static int coroutine_fn GRAPH_RDLOCK +bdrv_qed_co_change_backing_file(BlockDriverState *bs, const char *backing_file, + const char *backing_fmt) { BDRVQEDState *s = bs->opaque; QEDHeader new_header, le_header; @@ -1562,7 +1562,7 @@ static int bdrv_qed_change_backing_file(BlockDriverState *bs, } /* Write new header */ - ret = bdrv_pwrite_sync(bs->file, 0, buffer_len, buffer, 0); + ret = bdrv_co_pwrite_sync(bs->file, 0, buffer_len, buffer, 0); g_free(buffer); if (ret == 0) { memcpy(&s->header, &new_header, sizeof(new_header)); @@ -1636,34 +1636,34 @@ static QemuOptsList qed_create_opts = { }; static BlockDriver bdrv_qed = { - .format_name = "qed", - .instance_size = sizeof(BDRVQEDState), - .create_opts = &qed_create_opts, - .is_format = true, - .supports_backing = true, - - .bdrv_probe = bdrv_qed_probe, - .bdrv_open = bdrv_qed_open, - .bdrv_close = bdrv_qed_close, - .bdrv_reopen_prepare = bdrv_qed_reopen_prepare, - .bdrv_child_perm = bdrv_default_perms, - .bdrv_co_create = bdrv_qed_co_create, - .bdrv_co_create_opts = bdrv_qed_co_create_opts, - .bdrv_has_zero_init = bdrv_has_zero_init_1, - .bdrv_co_block_status = bdrv_qed_co_block_status, - .bdrv_co_readv = bdrv_qed_co_readv, - .bdrv_co_writev = bdrv_qed_co_writev, - .bdrv_co_pwrite_zeroes = bdrv_qed_co_pwrite_zeroes, - .bdrv_co_truncate = bdrv_qed_co_truncate, - .bdrv_co_getlength = bdrv_qed_co_getlength, - .bdrv_co_get_info = bdrv_qed_co_get_info, - .bdrv_refresh_limits = bdrv_qed_refresh_limits, - .bdrv_change_backing_file = bdrv_qed_change_backing_file, - .bdrv_co_invalidate_cache = bdrv_qed_co_invalidate_cache, - .bdrv_co_check = bdrv_qed_co_check, - .bdrv_detach_aio_context = bdrv_qed_detach_aio_context, - .bdrv_attach_aio_context = bdrv_qed_attach_aio_context, - .bdrv_drain_begin = bdrv_qed_drain_begin, + .format_name = "qed", + .instance_size = sizeof(BDRVQEDState), + .create_opts = &qed_create_opts, + .is_format = true, + .supports_backing = true, + + .bdrv_probe = bdrv_qed_probe, + .bdrv_open = bdrv_qed_open, + .bdrv_close = bdrv_qed_close, + .bdrv_reopen_prepare = bdrv_qed_reopen_prepare, + .bdrv_child_perm = bdrv_default_perms, + .bdrv_co_create = bdrv_qed_co_create, + .bdrv_co_create_opts = bdrv_qed_co_create_opts, + .bdrv_has_zero_init = bdrv_has_zero_init_1, + .bdrv_co_block_status = bdrv_qed_co_block_status, + .bdrv_co_readv = bdrv_qed_co_readv, + .bdrv_co_writev = bdrv_qed_co_writev, + .bdrv_co_pwrite_zeroes = bdrv_qed_co_pwrite_zeroes, + .bdrv_co_truncate = bdrv_qed_co_truncate, + .bdrv_co_getlength = bdrv_qed_co_getlength, + .bdrv_co_get_info = bdrv_qed_co_get_info, + .bdrv_refresh_limits = bdrv_qed_refresh_limits, + .bdrv_co_change_backing_file = bdrv_qed_co_change_backing_file, + .bdrv_co_invalidate_cache = bdrv_qed_co_invalidate_cache, + .bdrv_co_check = bdrv_qed_co_check, + .bdrv_detach_aio_context = bdrv_qed_detach_aio_context, + .bdrv_attach_aio_context = bdrv_qed_attach_aio_context, + .bdrv_drain_begin = bdrv_qed_drain_begin, }; static void bdrv_qed_init(void) diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index 9e0ccc1c32..6b21fbc73f 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -142,8 +142,7 @@ bdrv_refresh_limits(BlockDriverState *bs, Transaction *tran, Error **errp); int bdrv_commit(BlockDriverState *bs); int GRAPH_RDLOCK bdrv_make_empty(BdrvChild *c, Error **errp); -int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file, - const char *backing_fmt, bool warn); + void bdrv_register(BlockDriver *bdrv); int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, const char *backing_file_str); diff --git a/include/block/block-io.h b/include/block/block-io.h index 58c4cf50a0..f8729ccc55 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -210,6 +210,14 @@ void bdrv_round_to_subclusters(BlockDriverState *bs, void bdrv_get_backing_filename(BlockDriverState *bs, char *filename, int filename_size); +int coroutine_fn GRAPH_RDLOCK +bdrv_co_change_backing_file(BlockDriverState *bs, const char *backing_file, + const char *backing_fmt, bool warn); + +int co_wrapper_bdrv_rdlock +bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file, + const char *backing_fmt, bool warn); + int bdrv_save_vmstate(BlockDriverState *bs, const uint8_t *buf, int64_t pos, int size); diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index ed6066929a..59f6d7f195 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -331,8 +331,9 @@ struct BlockDriver { const char *name, Error **errp); - int (*bdrv_change_backing_file)(BlockDriverState *bs, - const char *backing_file, const char *backing_fmt); + int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_change_backing_file)( + BlockDriverState *bs, const char *backing_file, + const char *backing_fmt); /* TODO Better pass a option string/QDict/QemuOpts to add any rule? */ int (*bdrv_debug_breakpoint)(BlockDriverState *bs, const char *event, diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c index ba4e42b197..8d05538bf6 100644 --- a/tests/unit/test-bdrv-drain.c +++ b/tests/unit/test-bdrv-drain.c @@ -96,9 +96,9 @@ static int coroutine_fn bdrv_test_co_preadv(BlockDriverState *bs, return 0; } -static int bdrv_test_change_backing_file(BlockDriverState *bs, - const char *backing_file, - const char *backing_fmt) +static int bdrv_test_co_change_backing_file(BlockDriverState *bs, + const char *backing_file, + const char *backing_fmt) { return 0; } @@ -116,7 +116,7 @@ static BlockDriver bdrv_test = { .bdrv_child_perm = bdrv_default_perms, - .bdrv_change_backing_file = bdrv_test_change_backing_file, + .bdrv_co_change_backing_file = bdrv_test_co_change_backing_file, }; static void aio_ret_cb(void *opaque, int ret) -- cgit 1.4.1