* [PULL 01/25] block: Mark bdrv_probe_blocksizes() and callers GRAPH_RDLOCK
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 02/25] block: Mark bdrv_has_zero_init() " Kevin Wolf
` (24 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_probe_blocksizes() need to hold a reader lock for the graph because
it calls bdrv_filter_bs(), which accesses bs->file/backing.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-2-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-global-state.h | 2 +-
include/block/block_int-common.h | 3 ++-
block/block-backend.c | 2 ++
block/raw-format.c | 3 ++-
4 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 6bfafe781d..fca0a40dbd 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -281,7 +281,7 @@ bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
BdrvChild *ignore_child, Error **errp);
-int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz);
+int GRAPH_RDLOCK bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz);
int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo);
void GRAPH_WRLOCK
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index b8d9d24f39..8abdd2724b 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -386,7 +386,8 @@ struct BlockDriver {
* On success, store them in @bsz and return zero.
* On failure, return negative errno.
*/
- int (*bdrv_probe_blocksizes)(BlockDriverState *bs, BlockSizes *bsz);
+ int GRAPH_RDLOCK_PTR (*bdrv_probe_blocksizes)(
+ BlockDriverState *bs, BlockSizes *bsz);
/**
* Try to get @bs's geometry (cyls, heads, sectors)
* On success, store them in @geo and return 0.
diff --git a/block/block-backend.c b/block/block-backend.c
index 39aac1bbce..53cf3bb8b8 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2666,6 +2666,8 @@ int blk_load_vmstate(BlockBackend *blk, uint8_t *buf, int64_t pos, int size)
int blk_probe_blocksizes(BlockBackend *blk, BlockSizes *bsz)
{
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (!blk_is_available(blk)) {
return -ENOMEDIUM;
}
diff --git a/block/raw-format.c b/block/raw-format.c
index 8ff03adfa4..3fb77b0097 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -543,7 +543,8 @@ static int raw_probe(const uint8_t *buf, int buf_size, const char *filename)
return 1;
}
-static int raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
+static int GRAPH_RDLOCK
+raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
{
BDRVRawState *s = bs->opaque;
int ret;
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 02/25] block: Mark bdrv_has_zero_init() and callers GRAPH_RDLOCK
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
2023-11-07 18:45 ` [PULL 01/25] block: Mark bdrv_probe_blocksizes() and callers GRAPH_RDLOCK Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 03/25] block: Mark bdrv_filter_bs() " Kevin Wolf
` (23 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_has_zero_init() need to hold a reader lock for the graph because
it calls bdrv_filter_bs(), which accesses bs->file/backing.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-3-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-global-state.h | 2 +-
include/block/block_int-common.h | 2 +-
block.c | 2 +-
block/qcow2.c | 3 ++-
block/raw-format.c | 2 +-
block/vdi.c | 2 +-
block/vhdx.c | 13 +++++++++----
block/vmdk.c | 2 +-
block/vpc.c | 2 +-
blockdev.c | 2 ++
qemu-img.c | 2 ++
11 files changed, 22 insertions(+), 12 deletions(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index fca0a40dbd..3ae468ea15 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -189,7 +189,7 @@ void bdrv_drain_all(void);
void bdrv_aio_cancel(BlockAIOCB *acb);
int bdrv_has_zero_init_1(BlockDriverState *bs);
-int bdrv_has_zero_init(BlockDriverState *bs);
+int coroutine_mixed_fn GRAPH_RDLOCK bdrv_has_zero_init(BlockDriverState *bs);
BlockDriverState *bdrv_find_node(const char *node_name);
BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, Error **errp);
XDbgBlockGraph * GRAPH_RDLOCK bdrv_get_xdbg_block_graph(Error **errp);
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 8abdd2724b..c0db862de7 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -349,7 +349,7 @@ struct BlockDriver {
* Returns 1 if newly created images are guaranteed to contain only
* zeros, 0 otherwise.
*/
- int (*bdrv_has_zero_init)(BlockDriverState *bs);
+ int GRAPH_RDLOCK_PTR (*bdrv_has_zero_init)(BlockDriverState *bs);
/*
* Remove fd handlers, timers, and other event loop callbacks so the event
diff --git a/block.c b/block.c
index a527aa1a4c..1171f47570 100644
--- a/block.c
+++ b/block.c
@@ -6587,7 +6587,7 @@ int bdrv_has_zero_init_1(BlockDriverState *bs)
return 1;
}
-int bdrv_has_zero_init(BlockDriverState *bs)
+int coroutine_mixed_fn bdrv_has_zero_init(BlockDriverState *bs)
{
BlockDriverState *filtered;
GLOBAL_STATE_CODE();
diff --git a/block/qcow2.c b/block/qcow2.c
index aa01d9e7b5..a1443a31aa 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5302,7 +5302,8 @@ static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
return spec_info;
}
-static int coroutine_mixed_fn qcow2_has_zero_init(BlockDriverState *bs)
+static int coroutine_mixed_fn GRAPH_RDLOCK
+qcow2_has_zero_init(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
bool preallocated;
diff --git a/block/raw-format.c b/block/raw-format.c
index 3fb77b0097..8ca74c1cf3 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -452,7 +452,7 @@ raw_co_ioctl(BlockDriverState *bs, unsigned long int req, void *buf)
return bdrv_co_ioctl(bs->file->bs, req, buf);
}
-static int raw_has_zero_init(BlockDriverState *bs)
+static int GRAPH_RDLOCK raw_has_zero_init(BlockDriverState *bs)
{
return bdrv_has_zero_init(bs->file->bs);
}
diff --git a/block/vdi.c b/block/vdi.c
index 7cfd12b50d..8e144ce523 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -990,7 +990,7 @@ static void vdi_close(BlockDriverState *bs)
migrate_del_blocker(&s->migration_blocker);
}
-static int vdi_has_zero_init(BlockDriverState *bs)
+static int GRAPH_RDLOCK vdi_has_zero_init(BlockDriverState *bs)
{
BDRVVdiState *s = bs->opaque;
diff --git a/block/vhdx.c b/block/vhdx.c
index a9d08742f9..e136ba1ae1 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -1695,7 +1695,7 @@ exit:
* Fixed images: default state of the BAT is fully populated, with
* file offsets and state PAYLOAD_BLOCK_FULLY_PRESENT.
*/
-static int coroutine_fn
+static int coroutine_fn GRAPH_UNLOCKED
vhdx_create_bat(BlockBackend *blk, BDRVVHDXState *s,
uint64_t image_size, VHDXImageType type,
bool use_zero_blocks, uint64_t file_offset,
@@ -1708,6 +1708,7 @@ vhdx_create_bat(BlockBackend *blk, BDRVVHDXState *s,
uint64_t unused;
int block_state;
VHDXSectorInfo sinfo;
+ bool has_zero_init;
assert(s->bat == NULL);
@@ -1737,9 +1738,13 @@ vhdx_create_bat(BlockBackend *blk, BDRVVHDXState *s,
goto exit;
}
+ bdrv_graph_co_rdlock();
+ has_zero_init = bdrv_has_zero_init(blk_bs(blk));
+ bdrv_graph_co_rdunlock();
+
if (type == VHDX_TYPE_FIXED ||
use_zero_blocks ||
- bdrv_has_zero_init(blk_bs(blk)) == 0) {
+ has_zero_init == 0) {
/* for a fixed file, the default BAT entry is not zero */
s->bat = g_try_malloc0(length);
if (length && s->bat == NULL) {
@@ -1782,7 +1787,7 @@ exit:
* to create the BAT itself, we will also cause the BAT to be
* created.
*/
-static int coroutine_fn
+static int coroutine_fn GRAPH_UNLOCKED
vhdx_create_new_region_table(BlockBackend *blk, uint64_t image_size,
uint32_t block_size, uint32_t sector_size,
uint32_t log_size, bool use_zero_blocks,
@@ -2173,7 +2178,7 @@ static int coroutine_fn vhdx_co_check(BlockDriverState *bs,
return 0;
}
-static int vhdx_has_zero_init(BlockDriverState *bs)
+static int GRAPH_RDLOCK vhdx_has_zero_init(BlockDriverState *bs)
{
BDRVVHDXState *s = bs->opaque;
int state;
diff --git a/block/vmdk.c b/block/vmdk.c
index 85864b8045..91ed7a8d93 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -2894,7 +2894,7 @@ vmdk_co_get_allocated_file_size(BlockDriverState *bs)
return ret;
}
-static int vmdk_has_zero_init(BlockDriverState *bs)
+static int GRAPH_RDLOCK vmdk_has_zero_init(BlockDriverState *bs)
{
int i;
BDRVVmdkState *s = bs->opaque;
diff --git a/block/vpc.c b/block/vpc.c
index aa1a48ae0e..483775103c 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -1170,7 +1170,7 @@ fail:
}
-static int vpc_has_zero_init(BlockDriverState *bs)
+static int GRAPH_RDLOCK vpc_has_zero_init(BlockDriverState *bs)
{
BDRVVPCState *s = bs->opaque;
diff --git a/blockdev.c b/blockdev.c
index e9b7e38dc4..148df99e00 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3156,9 +3156,11 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
return;
}
+ bdrv_graph_rdlock_main_loop();
zero_target = (arg->sync == MIRROR_SYNC_MODE_FULL &&
(arg->mode == NEW_IMAGE_MODE_EXISTING ||
!bdrv_has_zero_init(target_bs)));
+ bdrv_graph_rdunlock_main_loop();
/* Honor bdrv_try_change_aio_context() context acquisition requirements. */
diff --git a/qemu-img.c b/qemu-img.c
index 369c2e8ddf..c061fd0634 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -2099,7 +2099,9 @@ static int convert_do_copy(ImgConvertState *s)
/* Check whether we have zero initialisation or can get it efficiently */
if (!s->has_zero_init && s->target_is_new && s->min_sparse &&
!s->target_has_backing) {
+ bdrv_graph_rdlock_main_loop();
s->has_zero_init = bdrv_has_zero_init(blk_bs(s->target));
+ bdrv_graph_rdunlock_main_loop();
}
/* Allocate buffer for copied data. For compressed images, only one cluster
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 03/25] block: Mark bdrv_filter_bs() and callers GRAPH_RDLOCK
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
2023-11-07 18:45 ` [PULL 01/25] block: Mark bdrv_probe_blocksizes() and callers GRAPH_RDLOCK Kevin Wolf
2023-11-07 18:45 ` [PULL 02/25] block: Mark bdrv_has_zero_init() " Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 04/25] block: Mark bdrv_root_attach_child() GRAPH_WRLOCK Kevin Wolf
` (22 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_filter_bs() need to hold a reader lock for the graph because
it calls bdrv_filter_child(), which accesses bs->file/backing.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-4-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-io.h | 2 +-
include/block/block_int-io.h | 3 ++-
block.c | 9 +++++++--
block/stream.c | 2 ++
migration/block-dirty-bitmap.c | 4 ++++
5 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/include/block/block-io.h b/include/block/block-io.h
index ad270b6ad2..58c4cf50a0 100644
--- a/include/block/block-io.h
+++ b/include/block/block-io.h
@@ -183,7 +183,7 @@ bdrv_co_eject(BlockDriverState *bs, bool eject_flag);
const char *bdrv_get_format_name(BlockDriverState *bs);
-bool bdrv_supports_compressed_writes(BlockDriverState *bs);
+bool GRAPH_RDLOCK bdrv_supports_compressed_writes(BlockDriverState *bs);
const char *bdrv_get_node_name(const BlockDriverState *bs);
const char * GRAPH_RDLOCK
diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h
index 34eac72d7a..26bff94e4e 100644
--- a/include/block/block_int-io.h
+++ b/include/block/block_int-io.h
@@ -143,7 +143,8 @@ static inline BlockDriverState *bdrv_cow_bs(BlockDriverState *bs)
return child_bs(bdrv_cow_child(bs));
}
-static inline BlockDriverState *bdrv_filter_bs(BlockDriverState *bs)
+static inline BlockDriverState * GRAPH_RDLOCK
+bdrv_filter_bs(BlockDriverState *bs)
{
IO_CODE();
return child_bs(bdrv_filter_child(bs));
diff --git a/block.c b/block.c
index 1171f47570..d85738b7dc 100644
--- a/block.c
+++ b/block.c
@@ -820,12 +820,17 @@ int bdrv_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
{
BlockDriver *drv = bs->drv;
- BlockDriverState *filtered = bdrv_filter_bs(bs);
+ BlockDriverState *filtered;
+
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
if (drv && drv->bdrv_probe_geometry) {
return drv->bdrv_probe_geometry(bs, geo);
- } else if (filtered) {
+ }
+
+ filtered = bdrv_filter_bs(bs);
+ if (filtered) {
return bdrv_probe_geometry(filtered, geo);
}
diff --git a/block/stream.c b/block/stream.c
index ddaab7dbbd..b22d9c236b 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -268,6 +268,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
assert(!bottom->drv->is_filter);
base_overlay = above_base = bottom;
} else {
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
base_overlay = bdrv_find_overlay(bs, base);
if (!base_overlay) {
error_setg(errp, "'%s' is not in the backing chain of '%s'",
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 03cb2e72ee..24347ab0f7 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -607,6 +607,10 @@ static int init_dirty_bitmap_migration(DBMSaveState *s)
BlockBackend *blk;
GHashTable *alias_map = NULL;
+ /* Runs in the migration thread, but holds the iothread lock */
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (migrate_has_block_bitmap_mapping()) {
alias_map = construct_alias_map(migrate_block_bitmap_mapping(), true,
&error_abort);
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 04/25] block: Mark bdrv_root_attach_child() GRAPH_WRLOCK
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (2 preceding siblings ...)
2023-11-07 18:45 ` [PULL 03/25] block: Mark bdrv_filter_bs() " Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 05/25] block: Mark block_job_add_bdrv() GRAPH_WRLOCK Kevin Wolf
` (21 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
Instead of taking the writer lock internally, require callers to already
hold it when calling bdrv_root_attach_child(). 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 <kwolf@redhat.com>
Message-ID: <20231027155333.420094-5-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block_int-global-state.h | 13 +++++++------
block.c | 5 +----
block/block-backend.c | 2 ++
blockjob.c | 2 ++
4 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h
index 074b677838..afce6c4416 100644
--- a/include/block/block_int-global-state.h
+++ b/include/block/block_int-global-state.h
@@ -196,12 +196,13 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
BlockCompletionFunc *cb, void *opaque,
JobTxn *txn, Error **errp);
-BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
- const char *child_name,
- const BdrvChildClass *child_class,
- BdrvChildRole child_role,
- uint64_t perm, uint64_t shared_perm,
- void *opaque, Error **errp);
+BdrvChild * GRAPH_WRLOCK
+bdrv_root_attach_child(BlockDriverState *child_bs, const char *child_name,
+ const BdrvChildClass *child_class,
+ BdrvChildRole child_role,
+ uint64_t perm, uint64_t shared_perm,
+ void *opaque, Error **errp);
+
void GRAPH_WRLOCK bdrv_root_unref_child(BdrvChild *child);
void GRAPH_RDLOCK bdrv_get_cumulative_perm(BlockDriverState *bs, uint64_t *perm,
diff --git a/block.c b/block.c
index d85738b7dc..5f92eb4950 100644
--- a/block.c
+++ b/block.c
@@ -3214,8 +3214,6 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
GLOBAL_STATE_CODE();
- bdrv_graph_wrlock(child_bs);
-
child = bdrv_attach_child_common(child_bs, child_name, child_class,
child_role, perm, shared_perm, opaque,
tran, errp);
@@ -3228,9 +3226,8 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
out:
tran_finalize(tran, ret);
- bdrv_graph_wrunlock();
- bdrv_unref(child_bs);
+ bdrv_schedule_unref(child_bs);
return ret < 0 ? NULL : child;
}
diff --git a/block/block-backend.c b/block/block-backend.c
index 53cf3bb8b8..075a0dfa95 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -931,10 +931,12 @@ int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp)
ThrottleGroupMember *tgm = &blk->public.throttle_group_member;
GLOBAL_STATE_CODE();
bdrv_ref(bs);
+ bdrv_graph_wrlock(bs);
blk->root = bdrv_root_attach_child(bs, "root", &child_root,
BDRV_CHILD_FILTERED | BDRV_CHILD_PRIMARY,
blk->perm, blk->shared_perm,
blk, errp);
+ bdrv_graph_wrunlock();
if (blk->root == NULL) {
return -EPERM;
}
diff --git a/blockjob.c b/blockjob.c
index 5b24de356d..7920f6e500 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -248,8 +248,10 @@ int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
}
aio_context_acquire(ctx);
}
+ bdrv_graph_wrlock(bs);
c = bdrv_root_attach_child(bs, name, &child_job, 0, perm, shared_perm, job,
errp);
+ bdrv_graph_wrunlock();
if (need_context_ops) {
aio_context_release(ctx);
if (job->job.aio_context != qemu_get_aio_context()) {
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 05/25] block: Mark block_job_add_bdrv() GRAPH_WRLOCK
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (3 preceding siblings ...)
2023-11-07 18:45 ` [PULL 04/25] block: Mark bdrv_root_attach_child() GRAPH_WRLOCK Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 06/25] block: Mark bdrv_filter_or_cow_bs() and callers GRAPH_RDLOCK Kevin Wolf
` (20 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
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 <kwolf@redhat.com>
Message-ID: <20231027155333.420094-6-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/blockjob.h | 5 +++--
include/block/blockjob_int.h | 9 +++++----
block/backup.c | 21 +++++++++++++++------
block/commit.c | 5 +++++
block/mirror.c | 5 +++++
block/stream.c | 4 ++++
blockjob.c | 8 +++++---
tests/unit/test-bdrv-drain.c | 3 +++
8 files changed, 45 insertions(+), 15 deletions(-)
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 95854f1477..e594c10d23 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -138,8 +138,9 @@ BlockJob *block_job_get_locked(const char *id);
* @job. This means that all operations will be blocked on @bs while
* @job exists.
*/
-int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
- uint64_t perm, uint64_t shared_perm, Error **errp);
+int GRAPH_WRLOCK
+block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
+ uint64_t perm, uint64_t shared_perm, Error **errp);
/**
* block_job_remove_all_bdrv:
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 18ee6f7bf0..4c3d2e25a2 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -111,10 +111,11 @@ struct BlockJobDriver {
* This function is not part of the public job interface; it should be
* called from a wrapper that is specific to the job type.
*/
-void *block_job_create(const char *job_id, const BlockJobDriver *driver,
- JobTxn *txn, BlockDriverState *bs, uint64_t perm,
- uint64_t shared_perm, int64_t speed, int flags,
- BlockCompletionFunc *cb, void *opaque, Error **errp);
+void * GRAPH_UNLOCKED
+block_job_create(const char *job_id, const BlockJobDriver *driver,
+ JobTxn *txn, BlockDriverState *bs, uint64_t perm,
+ uint64_t shared_perm, int64_t speed, int flags,
+ BlockCompletionFunc *cb, void *opaque, Error **errp);
/**
* block_job_free:
diff --git a/block/backup.c b/block/backup.c
index 9a3c4bdc82..5bad7d116f 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -374,7 +374,6 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
assert(bs);
assert(target);
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
/* QMP interface protects us from these cases */
assert(sync_mode != MIRROR_SYNC_MODE_INCREMENTAL);
@@ -385,31 +384,33 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
return NULL;
}
+ bdrv_graph_rdlock_main_loop();
if (!bdrv_is_inserted(bs)) {
error_setg(errp, "Device is not inserted: %s",
bdrv_get_device_name(bs));
- return NULL;
+ goto error_rdlock;
}
if (!bdrv_is_inserted(target)) {
error_setg(errp, "Device is not inserted: %s",
bdrv_get_device_name(target));
- return NULL;
+ goto error_rdlock;
}
if (compress && !bdrv_supports_compressed_writes(target)) {
error_setg(errp, "Compression is not supported for this drive %s",
bdrv_get_device_name(target));
- return NULL;
+ goto error_rdlock;
}
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) {
- return NULL;
+ goto error_rdlock;
}
if (bdrv_op_is_blocked(target, BLOCK_OP_TYPE_BACKUP_TARGET, errp)) {
- return NULL;
+ goto error_rdlock;
}
+ bdrv_graph_rdunlock_main_loop();
if (perf->max_workers < 1 || perf->max_workers > INT_MAX) {
error_setg(errp, "max-workers must be between 1 and %d", INT_MAX);
@@ -437,6 +438,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
len = bdrv_getlength(bs);
if (len < 0) {
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
error_setg_errno(errp, -len, "Unable to get length for '%s'",
bdrv_get_device_or_node_name(bs));
goto error;
@@ -444,6 +446,7 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
target_len = bdrv_getlength(target);
if (target_len < 0) {
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
error_setg_errno(errp, -target_len, "Unable to get length for '%s'",
bdrv_get_device_or_node_name(bs));
goto error;
@@ -493,8 +496,10 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
block_copy_set_speed(bcs, speed);
/* Required permissions are taken by copy-before-write filter target */
+ bdrv_graph_wrlock(target);
block_job_add_bdrv(&job->common, "target", target, 0, BLK_PERM_ALL,
&error_abort);
+ bdrv_graph_wrunlock();
return &job->common;
@@ -507,4 +512,8 @@ BlockJob *backup_job_create(const char *job_id, BlockDriverState *bs,
}
return NULL;
+
+error_rdlock:
+ bdrv_graph_rdunlock_main_loop();
+ return NULL;
}
diff --git a/block/commit.c b/block/commit.c
index 43d1de7577..fc3ad79749 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -342,6 +342,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
*/
iter_shared_perms = BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE;
+ bdrv_graph_wrlock(top);
for (iter = top; iter != base; iter = bdrv_filter_or_cow_bs(iter)) {
if (iter == filtered_base) {
/*
@@ -354,16 +355,20 @@ void commit_start(const char *job_id, BlockDriverState *bs,
ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
iter_shared_perms, errp);
if (ret < 0) {
+ bdrv_graph_wrunlock();
goto fail;
}
}
if (bdrv_freeze_backing_chain(commit_top_bs, base, errp) < 0) {
+ bdrv_graph_wrunlock();
goto fail;
}
s->chain_frozen = true;
ret = block_job_add_bdrv(&s->common, "base", base, 0, BLK_PERM_ALL, errp);
+ bdrv_graph_wrunlock();
+
if (ret < 0) {
goto fail;
}
diff --git a/block/mirror.c b/block/mirror.c
index c839542774..a03247a31b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1888,11 +1888,13 @@ static BlockJob *mirror_start_job(
*/
bdrv_disable_dirty_bitmap(s->dirty_bitmap);
+ bdrv_graph_wrlock(bs);
ret = block_job_add_bdrv(&s->common, "source", bs, 0,
BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE |
BLK_PERM_CONSISTENT_READ,
errp);
if (ret < 0) {
+ bdrv_graph_wrunlock();
goto fail;
}
@@ -1937,14 +1939,17 @@ static BlockJob *mirror_start_job(
ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
iter_shared_perms, errp);
if (ret < 0) {
+ bdrv_graph_wrunlock();
goto fail;
}
}
if (bdrv_freeze_backing_chain(mirror_top_bs, target, errp) < 0) {
+ bdrv_graph_wrunlock();
goto fail;
}
}
+ bdrv_graph_wrunlock();
QTAILQ_INIT(&s->ops_in_flight);
diff --git a/block/stream.c b/block/stream.c
index b22d9c236b..51333e460b 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -352,8 +352,10 @@ void stream_start(const char *job_id, BlockDriverState *bs,
* already have our own plans. Also don't allow resize as the image size is
* queried only at the job start and then cached.
*/
+ bdrv_graph_wrlock(bs);
if (block_job_add_bdrv(&s->common, "active node", bs, 0,
basic_flags | BLK_PERM_WRITE, errp)) {
+ bdrv_graph_wrunlock();
goto fail;
}
@@ -373,9 +375,11 @@ void stream_start(const char *job_id, BlockDriverState *bs,
ret = block_job_add_bdrv(&s->common, "intermediate node", iter, 0,
basic_flags, errp);
if (ret < 0) {
+ bdrv_graph_wrunlock();
goto fail;
}
}
+ bdrv_graph_wrunlock();
s->base_overlay = base_overlay;
s->above_base = above_base;
diff --git a/blockjob.c b/blockjob.c
index 7920f6e500..af44322cbe 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -248,10 +248,8 @@ int block_job_add_bdrv(BlockJob *job, const char *name, BlockDriverState *bs,
}
aio_context_acquire(ctx);
}
- bdrv_graph_wrlock(bs);
c = bdrv_root_attach_child(bs, name, &child_job, 0, perm, shared_perm, job,
errp);
- bdrv_graph_wrunlock();
if (need_context_ops) {
aio_context_release(ctx);
if (job->job.aio_context != qemu_get_aio_context()) {
@@ -515,7 +513,8 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
BlockJob *job;
int ret;
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
+
+ bdrv_graph_wrlock(bs);
if (job_id == NULL && !(flags & JOB_INTERNAL)) {
job_id = bdrv_get_device_name(bs);
@@ -524,6 +523,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
job = job_create(job_id, &driver->job_driver, txn, bdrv_get_aio_context(bs),
flags, cb, opaque, errp);
if (job == NULL) {
+ bdrv_graph_wrunlock();
return NULL;
}
@@ -563,9 +563,11 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
goto fail;
}
+ bdrv_graph_wrunlock();
return job;
fail:
+ bdrv_graph_wrunlock();
job_early_fail(&job->job);
return NULL;
}
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:
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 06/25] block: Mark bdrv_filter_or_cow_bs() and callers GRAPH_RDLOCK
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (4 preceding siblings ...)
2023-11-07 18:45 ` [PULL 05/25] block: Mark block_job_add_bdrv() GRAPH_WRLOCK Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 07/25] block: Mark bdrv_skip_implicit_filters() " Kevin Wolf
` (19 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_filter_or_cow_bs() need to hold a reader lock for the graph because
it calls bdrv_filter_or_cow_child(), which accesses bs->file/backing.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-7-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block_int-io.h | 3 ++-
block.c | 31 ++++++++++++++++++-------------
block/stream.c | 4 ++++
blockdev.c | 2 +-
nbd/server.c | 6 ++++++
5 files changed, 31 insertions(+), 15 deletions(-)
diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h
index 26bff94e4e..6800af7590 100644
--- a/include/block/block_int-io.h
+++ b/include/block/block_int-io.h
@@ -150,7 +150,8 @@ bdrv_filter_bs(BlockDriverState *bs)
return child_bs(bdrv_filter_child(bs));
}
-static inline BlockDriverState *bdrv_filter_or_cow_bs(BlockDriverState *bs)
+static inline BlockDriverState * GRAPH_RDLOCK
+bdrv_filter_or_cow_bs(BlockDriverState *bs)
{
IO_CODE();
return child_bs(bdrv_filter_or_cow_child(bs));
diff --git a/block.c b/block.c
index 5f92eb4950..a6060eddbc 100644
--- a/block.c
+++ b/block.c
@@ -5435,17 +5435,6 @@ static int bdrv_replace_node_common(BlockDriverState *from,
GLOBAL_STATE_CODE();
- if (detach_subchain) {
- assert(bdrv_chain_contains(from, to));
- assert(from != to);
- for (to_cow_parent = from;
- bdrv_filter_or_cow_bs(to_cow_parent) != to;
- to_cow_parent = bdrv_filter_or_cow_bs(to_cow_parent))
- {
- ;
- }
- }
-
/* Make sure that @from doesn't go away until we have successfully attached
* all of its parents to @to. */
bdrv_ref(from);
@@ -5457,6 +5446,17 @@ static int bdrv_replace_node_common(BlockDriverState *from,
bdrv_graph_wrlock(to);
+ if (detach_subchain) {
+ assert(bdrv_chain_contains(from, to));
+ assert(from != to);
+ for (to_cow_parent = from;
+ bdrv_filter_or_cow_bs(to_cow_parent) != to;
+ to_cow_parent = bdrv_filter_or_cow_bs(to_cow_parent))
+ {
+ ;
+ }
+ }
+
/*
* Do the replacement without permission update.
* Replacement may influence the permissions, we should calculate new
@@ -5504,10 +5504,14 @@ int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
int bdrv_drop_filter(BlockDriverState *bs, Error **errp)
{
+ BlockDriverState *child_bs;
+
GLOBAL_STATE_CODE();
+ bdrv_graph_rdlock_main_loop();
+ child_bs = bdrv_filter_or_cow_bs(bs);
+ bdrv_graph_rdunlock_main_loop();
- return bdrv_replace_node_common(bs, bdrv_filter_or_cow_bs(bs), true, true,
- errp);
+ return bdrv_replace_node_common(bs, child_bs, true, true, errp);
}
/*
@@ -6509,6 +6513,7 @@ bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base)
{
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
while (top && top != base) {
top = bdrv_filter_or_cow_bs(top);
diff --git a/block/stream.c b/block/stream.c
index 51333e460b..2781441191 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -60,6 +60,8 @@ static int stream_prepare(Job *job)
Error *local_err = NULL;
int ret = 0;
+ GLOBAL_STATE_CODE();
+
/* We should drop filter at this point, as filter hold the backing chain */
bdrv_cor_filter_drop(s->cor_filter_bs);
s->cor_filter_bs = NULL;
@@ -78,8 +80,10 @@ static int stream_prepare(Job *job)
bdrv_drained_begin(unfiltered_bs_cow);
}
+ bdrv_graph_rdlock_main_loop();
base = bdrv_filter_or_cow_bs(s->above_base);
unfiltered_base = bdrv_skip_filters(base);
+ bdrv_graph_rdunlock_main_loop();
if (bdrv_cow_child(unfiltered_bs)) {
const char *base_id = NULL, *base_fmt = NULL;
diff --git a/blockdev.c b/blockdev.c
index 148df99e00..4cb8e1d91a 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2501,8 +2501,8 @@ void qmp_block_stream(const char *job_id, const char *device,
/*
* Check for op blockers in the whole chain between bs and base (or bottom)
*/
- iter_end = bottom ? bdrv_filter_or_cow_bs(bottom_bs) : base_bs;
bdrv_graph_rdlock_main_loop();
+ iter_end = bottom ? bdrv_filter_or_cow_bs(bottom_bs) : base_bs;
for (iter = bs; iter && iter != iter_end;
iter = bdrv_filter_or_cow_bs(iter))
{
diff --git a/nbd/server.c b/nbd/server.c
index 859c163d19..895cf0a752 100644
--- a/nbd/server.c
+++ b/nbd/server.c
@@ -1689,6 +1689,7 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
size_t i;
int ret;
+ GLOBAL_STATE_CODE();
assert(exp_args->type == BLOCK_EXPORT_TYPE_NBD);
if (!nbd_server_is_running()) {
@@ -1743,6 +1744,8 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
}
exp->size = QEMU_ALIGN_DOWN(size, BDRV_SECTOR_SIZE);
+ bdrv_graph_rdlock_main_loop();
+
for (bitmaps = arg->bitmaps; bitmaps; bitmaps = bitmaps->next) {
exp->nr_export_bitmaps++;
}
@@ -1825,9 +1828,12 @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,
QTAILQ_INSERT_TAIL(&exports, exp, next);
+ bdrv_graph_rdunlock_main_loop();
+
return 0;
fail:
+ bdrv_graph_rdunlock_main_loop();
g_free(exp->export_bitmaps);
g_free(exp->name);
g_free(exp->description);
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 07/25] block: Mark bdrv_skip_implicit_filters() and callers GRAPH_RDLOCK
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (5 preceding siblings ...)
2023-11-07 18:45 ` [PULL 06/25] block: Mark bdrv_filter_or_cow_bs() and callers GRAPH_RDLOCK Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 08/25] block: Mark bdrv_skip_filters() " Kevin Wolf
` (18 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_skip_implicit_filters() need to hold a reader lock for the graph
because it calls bdrv_filter_child(), which accesses bs->file/backing.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-8-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block_int-global-state.h | 3 ++-
block.c | 28 +++++++++++++++++---------
block/monitor/block-hmp-cmds.c | 3 +++
blockdev.c | 14 +++++++------
4 files changed, 32 insertions(+), 16 deletions(-)
diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h
index afce6c4416..ef31c58bb3 100644
--- a/include/block/block_int-global-state.h
+++ b/include/block/block_int-global-state.h
@@ -277,7 +277,8 @@ BdrvDirtyBitmap *block_dirty_bitmap_remove(const char *node, const char *name,
Error **errp);
-BlockDriverState *bdrv_skip_implicit_filters(BlockDriverState *bs);
+BlockDriverState * GRAPH_RDLOCK
+bdrv_skip_implicit_filters(BlockDriverState *bs);
/**
* bdrv_add_aio_context_notifier:
diff --git a/block.c b/block.c
index a6060eddbc..7e8b39711b 100644
--- a/block.c
+++ b/block.c
@@ -4778,6 +4778,8 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
return 0;
}
+ bdrv_graph_rdlock_main_loop();
+
switch (qobject_type(value)) {
case QTYPE_QNULL:
assert(is_backing); /* The 'file' option does not allow a null value */
@@ -4787,17 +4789,16 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
str = qstring_get_str(qobject_to(QString, value));
new_child_bs = bdrv_lookup_bs(NULL, str, errp);
if (new_child_bs == NULL) {
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_rdlock;
}
- bdrv_graph_rdlock_main_loop();
has_child = bdrv_recurse_has_child(new_child_bs, bs);
- bdrv_graph_rdunlock_main_loop();
-
if (has_child) {
error_setg(errp, "Making '%s' a %s child of '%s' would create a "
"cycle", str, child_name, bs->node_name);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_rdlock;
}
break;
default:
@@ -4809,18 +4810,21 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
}
if (old_child_bs == new_child_bs) {
- return 0;
+ ret = 0;
+ goto out_rdlock;
}
if (old_child_bs) {
if (bdrv_skip_implicit_filters(old_child_bs) == new_child_bs) {
- return 0;
+ ret = 0;
+ goto out_rdlock;
}
if (old_child_bs->implicit) {
error_setg(errp, "Cannot replace implicit %s child of %s",
child_name, bs->node_name);
- return -EPERM;
+ ret = -EPERM;
+ goto out_rdlock;
}
}
@@ -4831,7 +4835,8 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
*/
error_setg(errp, "'%s' is a %s filter node that does not support a "
"%s child", bs->node_name, bs->drv->format_name, child_name);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out_rdlock;
}
if (is_backing) {
@@ -4852,6 +4857,7 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
aio_context_acquire(ctx);
}
+ bdrv_graph_rdunlock_main_loop();
bdrv_graph_wrlock(new_child_bs);
ret = bdrv_set_file_or_backing_noperm(bs, new_child_bs, is_backing,
@@ -4870,6 +4876,10 @@ bdrv_reopen_parse_file_or_backing(BDRVReopenState *reopen_state,
}
return ret;
+
+out_rdlock:
+ bdrv_graph_rdunlock_main_loop();
+ return ret;
}
/*
diff --git a/block/monitor/block-hmp-cmds.c b/block/monitor/block-hmp-cmds.c
index 5b2c597e7a..c729cbf1eb 100644
--- a/block/monitor/block-hmp-cmds.c
+++ b/block/monitor/block-hmp-cmds.c
@@ -206,6 +206,9 @@ void hmp_commit(Monitor *mon, const QDict *qdict)
BlockBackend *blk;
int ret;
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (!strcmp(device, "all")) {
ret = blk_commit_all();
} else {
diff --git a/blockdev.c b/blockdev.c
index 4cb8e1d91a..6cdf48beb1 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1746,10 +1746,10 @@ static void drive_backup_action(DriveBackup *backup,
assert(format);
if (source) {
/* Implicit filters should not appear in the filename */
- BlockDriverState *explicit_backing =
- bdrv_skip_implicit_filters(source);
+ BlockDriverState *explicit_backing;
bdrv_graph_rdlock_main_loop();
+ explicit_backing = bdrv_skip_implicit_filters(source);
bdrv_refresh_filename(explicit_backing);
bdrv_graph_rdunlock_main_loop();
@@ -3108,16 +3108,18 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
bdrv_img_create(arg->target, format,
NULL, NULL, NULL, size, flags, false, &local_err);
} else {
- /* Implicit filters should not appear in the filename */
- BlockDriverState *explicit_backing =
- bdrv_skip_implicit_filters(target_backing_bs);
+ BlockDriverState *explicit_backing;
switch (arg->mode) {
case NEW_IMAGE_MODE_EXISTING:
break;
case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
- /* create new image with backing file */
+ /*
+ * Create new image with backing file.
+ * Implicit filters should not appear in the filename.
+ */
bdrv_graph_rdlock_main_loop();
+ explicit_backing = bdrv_skip_implicit_filters(target_backing_bs);
bdrv_refresh_filename(explicit_backing);
bdrv_graph_rdunlock_main_loop();
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 08/25] block: Mark bdrv_skip_filters() and callers GRAPH_RDLOCK
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (6 preceding siblings ...)
2023-11-07 18:45 ` [PULL 07/25] block: Mark bdrv_skip_implicit_filters() " Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 09/25] block: Mark bdrv_(un)freeze_backing_chain() " Kevin Wolf
` (17 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_skip_filters() need to hold a reader lock for the graph because it
calls bdrv_filter_child(), which accesses bs->file/backing.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-9-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-global-state.h | 8 ++++---
include/block/block_int-io.h | 4 ++--
block/block-backend.c | 1 +
block/block-copy.c | 9 +++++++-
block/commit.c | 5 ++++-
block/mirror.c | 34 +++++++++++++++++++++---------
block/stream.c | 22 ++++++++++++-------
blockdev.c | 7 +++---
qemu-img.c | 18 +++++++++++++---
9 files changed, 77 insertions(+), 31 deletions(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 3ae468ea15..b6860ae43b 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -144,9 +144,11 @@ int bdrv_change_backing_file(BlockDriverState *bs, const char *backing_file,
void bdrv_register(BlockDriver *bdrv);
int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
const char *backing_file_str);
-BlockDriverState *bdrv_find_overlay(BlockDriverState *active,
- BlockDriverState *bs);
-BlockDriverState *bdrv_find_base(BlockDriverState *bs);
+
+BlockDriverState * GRAPH_RDLOCK
+bdrv_find_overlay(BlockDriverState *active, BlockDriverState *bs);
+
+BlockDriverState * GRAPH_RDLOCK bdrv_find_base(BlockDriverState *bs);
bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
Error **errp);
int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h
index 6800af7590..4e7bf57a5e 100644
--- a/include/block/block_int-io.h
+++ b/include/block/block_int-io.h
@@ -134,8 +134,8 @@ BdrvChild *bdrv_cow_child(BlockDriverState *bs);
BdrvChild *bdrv_filter_child(BlockDriverState *bs);
BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs);
BdrvChild * GRAPH_RDLOCK bdrv_primary_child(BlockDriverState *bs);
-BlockDriverState *bdrv_skip_filters(BlockDriverState *bs);
-BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs);
+BlockDriverState * GRAPH_RDLOCK bdrv_skip_filters(BlockDriverState *bs);
+BlockDriverState * GRAPH_RDLOCK bdrv_backing_chain_next(BlockDriverState *bs);
static inline BlockDriverState *bdrv_cow_bs(BlockDriverState *bs)
{
diff --git a/block/block-backend.c b/block/block-backend.c
index 075a0dfa95..4053134781 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -2730,6 +2730,7 @@ int blk_commit_all(void)
{
BlockBackend *blk = NULL;
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
while ((blk = blk_all_next(blk)) != NULL) {
AioContext *aio_context = blk_get_aio_context(blk);
diff --git a/block/block-copy.c b/block/block-copy.c
index 1c60368d72..6b2be3d204 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -313,7 +313,12 @@ static int64_t block_copy_calculate_cluster_size(BlockDriverState *target,
{
int ret;
BlockDriverInfo bdi;
- bool target_does_cow = bdrv_backing_chain_next(target);
+ bool target_does_cow;
+
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
+ target_does_cow = bdrv_backing_chain_next(target);
/*
* If there is no backing file on the target, we cannot rely on COW if our
@@ -355,6 +360,8 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
BdrvDirtyBitmap *copy_bitmap;
bool is_fleecing;
+ GLOBAL_STATE_CODE();
+
cluster_size = block_copy_calculate_cluster_size(target->bs, errp);
if (cluster_size < 0) {
return NULL;
diff --git a/block/commit.c b/block/commit.c
index fc3ad79749..05eb57d9ea 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -255,10 +255,13 @@ void commit_start(const char *job_id, BlockDriverState *bs,
GLOBAL_STATE_CODE();
assert(top != bs);
+ bdrv_graph_rdlock_main_loop();
if (bdrv_skip_filters(top) == bdrv_skip_filters(base)) {
error_setg(errp, "Invalid files for merge: top and base are the same");
+ bdrv_graph_rdunlock_main_loop();
return;
}
+ bdrv_graph_rdunlock_main_loop();
base_size = bdrv_getlength(base);
if (base_size < 0) {
@@ -324,6 +327,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
* this is the responsibility of the interface (i.e. whoever calls
* commit_start()).
*/
+ bdrv_graph_wrlock(top);
s->base_overlay = bdrv_find_overlay(top, base);
assert(s->base_overlay);
@@ -342,7 +346,6 @@ void commit_start(const char *job_id, BlockDriverState *bs,
*/
iter_shared_perms = BLK_PERM_WRITE_UNCHANGED | BLK_PERM_WRITE;
- bdrv_graph_wrlock(top);
for (iter = top; iter != base; iter = bdrv_filter_or_cow_bs(iter)) {
if (iter == filtered_base) {
/*
diff --git a/block/mirror.c b/block/mirror.c
index a03247a31b..75e826dac8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -714,7 +714,6 @@ static int mirror_exit_common(Job *job)
bdrv_graph_rdlock_main_loop();
bdrv_child_refresh_perms(mirror_top_bs, mirror_top_bs->backing,
&error_abort);
- bdrv_graph_rdunlock_main_loop();
if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
BlockDriverState *backing = s->is_none_mode ? src : s->base;
@@ -737,6 +736,7 @@ static int mirror_exit_common(Job *job)
local_err = NULL;
}
}
+ bdrv_graph_rdunlock_main_loop();
if (s->to_replace) {
replace_aio_context = bdrv_get_aio_context(s->to_replace);
@@ -992,13 +992,13 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
} else {
s->target_cluster_size = BDRV_SECTOR_SIZE;
}
- bdrv_graph_co_rdunlock();
if (backing_filename[0] && !bdrv_backing_chain_next(target_bs) &&
s->granularity < s->target_cluster_size) {
s->buf_size = MAX(s->buf_size, s->target_cluster_size);
s->cow_bitmap = bitmap_new(length);
}
s->max_iov = MIN(bs->bl.max_iov, target_bs->bl.max_iov);
+ bdrv_graph_co_rdunlock();
s->buf = qemu_try_blockalign(bs, s->buf_size);
if (s->buf == NULL) {
@@ -1744,12 +1744,15 @@ static BlockJob *mirror_start_job(
buf_size = DEFAULT_MIRROR_BUF_SIZE;
}
+ bdrv_graph_rdlock_main_loop();
if (bdrv_skip_filters(bs) == bdrv_skip_filters(target)) {
error_setg(errp, "Can't mirror node into itself");
+ bdrv_graph_rdunlock_main_loop();
return NULL;
}
target_is_backing = bdrv_chain_contains(bs, target);
+ bdrv_graph_rdunlock_main_loop();
/* In the case of active commit, add dummy driver to provide consistent
* reads on the top, while disabling it in the intermediate nodes, and make
@@ -1832,14 +1835,19 @@ static BlockJob *mirror_start_job(
}
target_shared_perms |= BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE;
- } else if (bdrv_chain_contains(bs, bdrv_skip_filters(target))) {
- /*
- * We may want to allow this in the future, but it would
- * require taking some extra care.
- */
- error_setg(errp, "Cannot mirror to a filter on top of a node in the "
- "source's backing chain");
- goto fail;
+ } else {
+ bdrv_graph_rdlock_main_loop();
+ if (bdrv_chain_contains(bs, bdrv_skip_filters(target))) {
+ /*
+ * We may want to allow this in the future, but it would
+ * require taking some extra care.
+ */
+ error_setg(errp, "Cannot mirror to a filter on top of a node in "
+ "the source's backing chain");
+ bdrv_graph_rdunlock_main_loop();
+ goto fail;
+ }
+ bdrv_graph_rdunlock_main_loop();
}
s->target = blk_new(s->common.job.aio_context,
@@ -1860,6 +1868,7 @@ static BlockJob *mirror_start_job(
blk_set_allow_aio_context_change(s->target, true);
blk_set_disable_request_queuing(s->target, true);
+ bdrv_graph_rdlock_main_loop();
s->replaces = g_strdup(replaces);
s->on_source_error = on_source_error;
s->on_target_error = on_target_error;
@@ -1875,6 +1884,7 @@ static BlockJob *mirror_start_job(
if (auto_complete) {
s->should_complete = true;
}
+ bdrv_graph_rdunlock_main_loop();
s->dirty_bitmap = bdrv_create_dirty_bitmap(s->mirror_top_bs, granularity,
NULL, errp);
@@ -2007,8 +2017,12 @@ void mirror_start(const char *job_id, BlockDriverState *bs,
MirrorSyncMode_str(mode));
return;
}
+
+ bdrv_graph_rdlock_main_loop();
is_none_mode = mode == MIRROR_SYNC_MODE_NONE;
base = mode == MIRROR_SYNC_MODE_TOP ? bdrv_backing_chain_next(bs) : NULL;
+ bdrv_graph_rdunlock_main_loop();
+
mirror_start_job(job_id, bs, creation_flags, target, replaces,
speed, granularity, buf_size, backing_mode, zero_target,
on_source_error, on_target_error, unmap, NULL, NULL,
diff --git a/block/stream.c b/block/stream.c
index 2781441191..5323a9976d 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -53,8 +53,8 @@ static int coroutine_fn stream_populate(BlockBackend *blk,
static int stream_prepare(Job *job)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
- BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
- BlockDriverState *unfiltered_bs_cow = bdrv_cow_bs(unfiltered_bs);
+ BlockDriverState *unfiltered_bs;
+ BlockDriverState *unfiltered_bs_cow;
BlockDriverState *base;
BlockDriverState *unfiltered_base;
Error *local_err = NULL;
@@ -62,6 +62,11 @@ static int stream_prepare(Job *job)
GLOBAL_STATE_CODE();
+ bdrv_graph_rdlock_main_loop();
+ unfiltered_bs = bdrv_skip_filters(s->target_bs);
+ unfiltered_bs_cow = bdrv_cow_bs(unfiltered_bs);
+ bdrv_graph_rdunlock_main_loop();
+
/* We should drop filter at this point, as filter hold the backing chain */
bdrv_cor_filter_drop(s->cor_filter_bs);
s->cor_filter_bs = NULL;
@@ -142,18 +147,19 @@ static void stream_clean(Job *job)
static int coroutine_fn stream_run(Job *job, Error **errp)
{
StreamBlockJob *s = container_of(job, StreamBlockJob, common.job);
- BlockDriverState *unfiltered_bs = bdrv_skip_filters(s->target_bs);
+ BlockDriverState *unfiltered_bs;
int64_t len;
int64_t offset = 0;
int error = 0;
int64_t n = 0; /* bytes */
- if (unfiltered_bs == s->base_overlay) {
- /* Nothing to stream */
- return 0;
- }
-
WITH_GRAPH_RDLOCK_GUARD() {
+ unfiltered_bs = bdrv_skip_filters(s->target_bs);
+ if (unfiltered_bs == s->base_overlay) {
+ /* Nothing to stream */
+ return 0;
+ }
+
len = bdrv_co_getlength(s->target_bs);
if (len < 0) {
return len;
diff --git a/blockdev.c b/blockdev.c
index 6cdf48beb1..5f15ea3b1d 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -1710,7 +1710,6 @@ static void drive_backup_action(DriveBackup *backup,
bdrv_graph_rdunlock_main_loop();
goto out;
}
- bdrv_graph_rdunlock_main_loop();
flags = bs->open_flags | BDRV_O_RDWR;
@@ -1735,6 +1734,7 @@ static void drive_backup_action(DriveBackup *backup,
flags |= BDRV_O_NO_BACKING;
set_backing_hd = true;
}
+ bdrv_graph_rdunlock_main_loop();
size = bdrv_getlength(bs);
if (size < 0) {
@@ -3054,7 +3054,6 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
bdrv_graph_rdunlock_main_loop();
return;
}
- bdrv_graph_rdunlock_main_loop();
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
@@ -3076,6 +3075,7 @@ void qmp_drive_mirror(DriveMirror *arg, Error **errp)
if (arg->sync == MIRROR_SYNC_MODE_NONE) {
target_backing_bs = bs;
}
+ bdrv_graph_rdunlock_main_loop();
size = bdrv_getlength(bs);
if (size < 0) {
@@ -3450,15 +3450,16 @@ void qmp_change_backing_file(const char *device,
goto out;
}
+ bdrv_graph_rdlock_main_loop();
if (bdrv_find_base(image_bs) == image_bs) {
error_setg(errp, "not allowing backing file change on an image "
"without a backing file");
+ bdrv_graph_rdunlock_main_loop();
goto out;
}
/* even though we are not necessarily operating on bs, we need it to
* determine if block ops are currently prohibited on the chain */
- bdrv_graph_rdlock_main_loop();
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_CHANGE, errp)) {
bdrv_graph_rdunlock_main_loop();
goto out;
diff --git a/qemu-img.c b/qemu-img.c
index c061fd0634..33f3ab5fba 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -1050,12 +1050,14 @@ static int img_commit(int argc, char **argv)
qemu_progress_init(progress, 1.f);
qemu_progress_print(0.f, 100);
+ bdrv_graph_rdlock_main_loop();
if (base) {
base_bs = bdrv_find_backing_image(bs, base);
if (!base_bs) {
error_setg(&local_err,
"Did not find '%s' in the backing chain of '%s'",
base, filename);
+ bdrv_graph_rdunlock_main_loop();
goto done;
}
} else {
@@ -1065,9 +1067,11 @@ static int img_commit(int argc, char **argv)
base_bs = bdrv_backing_chain_next(bs);
if (!base_bs) {
error_setg(&local_err, "Image does not have a backing file");
+ bdrv_graph_rdunlock_main_loop();
goto done;
}
}
+ bdrv_graph_rdunlock_main_loop();
cbi = (CommonBlockJobCBInfo){
.errp = &local_err,
@@ -1713,7 +1717,8 @@ static void convert_select_part(ImgConvertState *s, int64_t sector_num,
}
}
-static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
+static int coroutine_mixed_fn GRAPH_RDLOCK
+convert_iteration_sectors(ImgConvertState *s, int64_t sector_num)
{
int64_t src_cur_offset;
int ret, n, src_cur;
@@ -2115,7 +2120,9 @@ static int convert_do_copy(ImgConvertState *s)
}
while (sector_num < s->total_sectors) {
+ bdrv_graph_rdlock_main_loop();
n = convert_iteration_sectors(s, sector_num);
+ bdrv_graph_rdunlock_main_loop();
if (n < 0) {
return n;
}
@@ -2757,8 +2764,10 @@ static int img_convert(int argc, char **argv)
* s.target_backing_sectors has to be negative, which it will
* be automatically). The backing file length is used only
* for optimizations, so such a case is not fatal. */
+ bdrv_graph_rdlock_main_loop();
s.target_backing_sectors =
bdrv_nb_sectors(bdrv_backing_chain_next(out_bs));
+ bdrv_graph_rdunlock_main_loop();
} else {
s.target_backing_sectors = -1;
}
@@ -3145,6 +3154,9 @@ static int get_block_status(BlockDriverState *bs, int64_t offset,
int64_t map;
char *filename = NULL;
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
/* As an optimization, we could cache the current range of unallocated
* clusters in each file of the chain, and avoid querying the same
* range repeatedly.
@@ -3173,9 +3185,7 @@ static int get_block_status(BlockDriverState *bs, int64_t offset,
has_offset = !!(ret & BDRV_BLOCK_OFFSET_VALID);
if (file && has_offset) {
- bdrv_graph_rdlock_main_loop();
bdrv_refresh_filename(file);
- bdrv_graph_rdunlock_main_loop();
filename = file->filename;
}
@@ -3663,7 +3673,9 @@ static int img_rebase(int argc, char **argv)
}
bs = blk_bs(blk);
+ bdrv_graph_rdlock_main_loop();
unfiltered_bs = bdrv_skip_filters(bs);
+ bdrv_graph_rdunlock_main_loop();
if (compress && !block_driver_can_compress(unfiltered_bs->drv)) {
error_report("Compression not supported for this file format");
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 09/25] block: Mark bdrv_(un)freeze_backing_chain() and callers GRAPH_RDLOCK
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (7 preceding siblings ...)
2023-11-07 18:45 ` [PULL 08/25] block: Mark bdrv_skip_filters() " Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 10/25] block: Mark bdrv_chain_contains() " Kevin Wolf
` (16 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_(un)freeze_backing_chain() need to hold a reader lock for the
graph because it calls bdrv_filter_or_cow_child(), which accesses
bs->file/backing.
Use the opportunity to make bdrv_is_backing_chain_frozen() static, it
has no external callers.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-10-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/copy-on-read.h | 3 ++-
include/block/block-global-state.h | 11 ++++++-----
block.c | 5 +++--
block/commit.c | 6 ++++++
block/copy-on-read.c | 19 +++++++++++++++----
block/mirror.c | 3 +++
block/stream.c | 16 +++++++++++-----
7 files changed, 46 insertions(+), 17 deletions(-)
diff --git a/block/copy-on-read.h b/block/copy-on-read.h
index 1d8ad38c74..72f9b378ea 100644
--- a/block/copy-on-read.h
+++ b/block/copy-on-read.h
@@ -27,6 +27,7 @@
#include "block/block_int.h"
-void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs);
+void no_coroutine_fn GRAPH_UNLOCKED
+bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs);
#endif /* BLOCK_COPY_ON_READ_H */
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index b6860ae43b..545708c35a 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -149,11 +149,12 @@ BlockDriverState * GRAPH_RDLOCK
bdrv_find_overlay(BlockDriverState *active, BlockDriverState *bs);
BlockDriverState * GRAPH_RDLOCK bdrv_find_base(BlockDriverState *bs);
-bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
- Error **errp);
-int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
- Error **errp);
-void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
+
+int GRAPH_RDLOCK
+bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
+ Error **errp);
+void GRAPH_RDLOCK
+bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
/*
* The units of offset and total_work_size may be chosen arbitrarily by the
diff --git a/block.c b/block.c
index 7e8b39711b..dc1980ee42 100644
--- a/block.c
+++ b/block.c
@@ -5843,8 +5843,9 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs)
* between @bs and @base is frozen. @errp is set if that's the case.
* @base must be reachable from @bs, or NULL.
*/
-bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
- Error **errp)
+static bool GRAPH_RDLOCK
+bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
+ Error **errp)
{
BlockDriverState *i;
BdrvChild *child;
diff --git a/block/commit.c b/block/commit.c
index 05eb57d9ea..d92af02ead 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -48,8 +48,10 @@ static int commit_prepare(Job *job)
{
CommitBlockJob *s = container_of(job, CommitBlockJob, common.job);
+ bdrv_graph_rdlock_main_loop();
bdrv_unfreeze_backing_chain(s->commit_top_bs, s->base_bs);
s->chain_frozen = false;
+ bdrv_graph_rdunlock_main_loop();
/* Remove base node parent that still uses BLK_PERM_WRITE/RESIZE before
* the normal backing chain can be restored. */
@@ -68,7 +70,9 @@ static void commit_abort(Job *job)
BlockDriverState *top_bs = blk_bs(s->top);
if (s->chain_frozen) {
+ bdrv_graph_rdlock_main_loop();
bdrv_unfreeze_backing_chain(s->commit_top_bs, s->base_bs);
+ bdrv_graph_rdunlock_main_loop();
}
/* Make sure commit_top_bs and top stay around until bdrv_replace_node() */
@@ -404,7 +408,9 @@ void commit_start(const char *job_id, BlockDriverState *bs,
fail:
if (s->chain_frozen) {
+ bdrv_graph_rdlock_main_loop();
bdrv_unfreeze_backing_chain(commit_top_bs, base);
+ bdrv_graph_rdunlock_main_loop();
}
if (s->base) {
blk_unref(s->base);
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 5149fcf63a..6f245b629a 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -35,8 +35,8 @@ typedef struct BDRVStateCOR {
} BDRVStateCOR;
-static int cor_open(BlockDriverState *bs, QDict *options, int flags,
- Error **errp)
+static int GRAPH_UNLOCKED
+cor_open(BlockDriverState *bs, QDict *options, int flags, Error **errp)
{
BlockDriverState *bottom_bs = NULL;
BDRVStateCOR *state = bs->opaque;
@@ -44,6 +44,8 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags,
const char *bottom_node = qdict_get_try_str(options, "bottom");
int ret;
+ GLOBAL_STATE_CODE();
+
ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
if (ret < 0) {
return ret;
@@ -59,6 +61,8 @@ static int cor_open(BlockDriverState *bs, QDict *options, int flags,
bs->file->bs->supported_zero_flags);
if (bottom_node) {
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
bottom_bs = bdrv_find_node(bottom_node);
if (!bottom_bs) {
error_setg(errp, "Bottom node '%s' not found", bottom_node);
@@ -227,13 +231,17 @@ cor_co_lock_medium(BlockDriverState *bs, bool locked)
}
-static void cor_close(BlockDriverState *bs)
+static void GRAPH_UNLOCKED cor_close(BlockDriverState *bs)
{
BDRVStateCOR *s = bs->opaque;
+ GLOBAL_STATE_CODE();
+
if (s->chain_frozen) {
+ bdrv_graph_rdlock_main_loop();
s->chain_frozen = false;
bdrv_unfreeze_backing_chain(bs, s->bottom_bs);
+ bdrv_graph_rdunlock_main_loop();
}
bdrv_unref(s->bottom_bs);
@@ -263,12 +271,15 @@ static BlockDriver bdrv_copy_on_read = {
};
-void bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs)
+void no_coroutine_fn bdrv_cor_filter_drop(BlockDriverState *cor_filter_bs)
{
BDRVStateCOR *s = cor_filter_bs->opaque;
+ GLOBAL_STATE_CODE();
+
/* unfreeze, as otherwise bdrv_replace_node() will fail */
if (s->chain_frozen) {
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
s->chain_frozen = false;
bdrv_unfreeze_backing_chain(cor_filter_bs, s->bottom_bs);
}
diff --git a/block/mirror.c b/block/mirror.c
index 75e826dac8..f8e439371c 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -678,6 +678,7 @@ static int mirror_exit_common(Job *job)
s->prepared = true;
aio_context_acquire(qemu_get_aio_context());
+ bdrv_graph_rdlock_main_loop();
mirror_top_bs = s->mirror_top_bs;
bs_opaque = mirror_top_bs->opaque;
@@ -696,6 +697,8 @@ static int mirror_exit_common(Job *job)
bdrv_ref(mirror_top_bs);
bdrv_ref(target_bs);
+ bdrv_graph_rdunlock_main_loop();
+
/*
* Remove target parent that still uses BLK_PERM_WRITE/RESIZE before
* inserting target_bs at s->to_replace, where we might not be able to get
diff --git a/block/stream.c b/block/stream.c
index 5323a9976d..c32c98339a 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -266,6 +266,8 @@ void stream_start(const char *job_id, BlockDriverState *bs,
assert(!(base && bottom));
assert(!(backing_file_str && bottom));
+ bdrv_graph_rdlock_main_loop();
+
if (bottom) {
/*
* New simple interface. The code is written in terms of old interface
@@ -278,13 +280,11 @@ void stream_start(const char *job_id, BlockDriverState *bs,
assert(!bottom->drv->is_filter);
base_overlay = above_base = bottom;
} else {
- GRAPH_RDLOCK_GUARD_MAINLOOP();
-
base_overlay = bdrv_find_overlay(bs, base);
if (!base_overlay) {
error_setg(errp, "'%s' is not in the backing chain of '%s'",
base->node_name, bs->node_name);
- return;
+ goto out_rdlock;
}
/*
@@ -306,7 +306,7 @@ void stream_start(const char *job_id, BlockDriverState *bs,
if (bs_read_only) {
/* Hold the chain during reopen */
if (bdrv_freeze_backing_chain(bs, above_base, errp) < 0) {
- return;
+ goto out_rdlock;
}
ret = bdrv_reopen_set_read_only(bs, false, errp);
@@ -315,10 +315,12 @@ void stream_start(const char *job_id, BlockDriverState *bs,
bdrv_unfreeze_backing_chain(bs, above_base);
if (ret < 0) {
- return;
+ goto out_rdlock;
}
}
+ bdrv_graph_rdunlock_main_loop();
+
opts = qdict_new();
qdict_put_str(opts, "driver", "copy-on-read");
@@ -413,4 +415,8 @@ fail:
if (bs_read_only) {
bdrv_reopen_set_read_only(bs, true, NULL);
}
+ return;
+
+out_rdlock:
+ bdrv_graph_rdunlock_main_loop();
}
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 10/25] block: Mark bdrv_chain_contains() and callers GRAPH_RDLOCK
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (8 preceding siblings ...)
2023-11-07 18:45 ` [PULL 09/25] block: Mark bdrv_(un)freeze_backing_chain() " Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 11/25] block: Mark bdrv_filter_child() " Kevin Wolf
` (15 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_chain_contains() need to hold a reader lock for the graph because
it calls bdrv_filter_or_cow_bs(), which accesses bs->file/backing.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-11-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-global-state.h | 4 ++-
block.c | 1 -
block/block-copy.c | 2 ++
blockdev.c | 48 +++++++++++++++++-------------
4 files changed, 32 insertions(+), 23 deletions(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 545708c35a..9a33bd7ef9 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -199,7 +199,9 @@ XDbgBlockGraph * GRAPH_RDLOCK bdrv_get_xdbg_block_graph(Error **errp);
BlockDriverState *bdrv_lookup_bs(const char *device,
const char *node_name,
Error **errp);
-bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base);
+bool GRAPH_RDLOCK
+bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base);
+
BlockDriverState *bdrv_next_node(BlockDriverState *bs);
BlockDriverState *bdrv_next_all_states(BlockDriverState *bs);
diff --git a/block.c b/block.c
index dc1980ee42..bb322df7d8 100644
--- a/block.c
+++ b/block.c
@@ -6524,7 +6524,6 @@ bool bdrv_chain_contains(BlockDriverState *top, BlockDriverState *base)
{
GLOBAL_STATE_CODE();
- GRAPH_RDLOCK_GUARD_MAINLOOP();
while (top && top != base) {
top = bdrv_filter_or_cow_bs(top);
diff --git a/block/block-copy.c b/block/block-copy.c
index 6b2be3d204..9ee3dd7ef5 100644
--- a/block/block-copy.c
+++ b/block/block-copy.c
@@ -399,7 +399,9 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, BdrvChild *target,
* For more information see commit f8d59dfb40bb and test
* tests/qemu-iotests/222
*/
+ bdrv_graph_rdlock_main_loop();
is_fleecing = bdrv_chain_contains(target->bs, source->bs);
+ bdrv_graph_rdunlock_main_loop();
s = g_new(BlockCopyState, 1);
*s = (BlockCopyState) {
diff --git a/blockdev.c b/blockdev.c
index 5f15ea3b1d..f04faf6373 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2450,11 +2450,12 @@ void qmp_block_stream(const char *job_id, const char *device,
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
+ bdrv_graph_rdlock_main_loop();
if (base) {
base_bs = bdrv_find_backing_image(bs, base);
if (base_bs == NULL) {
error_setg(errp, "Can't find '%s' in the backing chain", base);
- goto out;
+ goto out_rdlock;
}
assert(bdrv_get_aio_context(base_bs) == aio_context);
}
@@ -2462,38 +2463,36 @@ void qmp_block_stream(const char *job_id, const char *device,
if (base_node) {
base_bs = bdrv_lookup_bs(NULL, base_node, errp);
if (!base_bs) {
- goto out;
+ goto out_rdlock;
}
if (bs == base_bs || !bdrv_chain_contains(bs, base_bs)) {
error_setg(errp, "Node '%s' is not a backing image of '%s'",
base_node, device);
- goto out;
+ goto out_rdlock;
}
assert(bdrv_get_aio_context(base_bs) == aio_context);
- bdrv_graph_rdlock_main_loop();
bdrv_refresh_filename(base_bs);
- bdrv_graph_rdunlock_main_loop();
}
if (bottom) {
bottom_bs = bdrv_lookup_bs(NULL, bottom, errp);
if (!bottom_bs) {
- goto out;
+ goto out_rdlock;
}
if (!bottom_bs->drv) {
error_setg(errp, "Node '%s' is not open", bottom);
- goto out;
+ goto out_rdlock;
}
if (bottom_bs->drv->is_filter) {
error_setg(errp, "Node '%s' is a filter, use a non-filter node "
"as 'bottom'", bottom);
- goto out;
+ goto out_rdlock;
}
if (!bdrv_chain_contains(bs, bottom_bs)) {
error_setg(errp, "Node '%s' is not in a chain starting from '%s'",
bottom, device);
- goto out;
+ goto out_rdlock;
}
assert(bdrv_get_aio_context(bottom_bs) == aio_context);
}
@@ -2501,14 +2500,12 @@ void qmp_block_stream(const char *job_id, const char *device,
/*
* Check for op blockers in the whole chain between bs and base (or bottom)
*/
- bdrv_graph_rdlock_main_loop();
iter_end = bottom ? bdrv_filter_or_cow_bs(bottom_bs) : base_bs;
for (iter = bs; iter && iter != iter_end;
iter = bdrv_filter_or_cow_bs(iter))
{
if (bdrv_op_is_blocked(iter, BLOCK_OP_TYPE_STREAM, errp)) {
- bdrv_graph_rdunlock_main_loop();
- goto out;
+ goto out_rdlock;
}
}
bdrv_graph_rdunlock_main_loop();
@@ -2540,6 +2537,11 @@ void qmp_block_stream(const char *job_id, const char *device,
out:
aio_context_release(aio_context);
+ return;
+
+out_rdlock:
+ bdrv_graph_rdunlock_main_loop();
+ aio_context_release(aio_context);
}
void qmp_block_commit(const char *job_id, const char *device,
@@ -3439,39 +3441,38 @@ void qmp_change_backing_file(const char *device,
aio_context = bdrv_get_aio_context(bs);
aio_context_acquire(aio_context);
+ bdrv_graph_rdlock_main_loop();
+
image_bs = bdrv_lookup_bs(NULL, image_node_name, &local_err);
if (local_err) {
error_propagate(errp, local_err);
- goto out;
+ goto out_rdlock;
}
if (!image_bs) {
error_setg(errp, "image file not found");
- goto out;
+ goto out_rdlock;
}
- bdrv_graph_rdlock_main_loop();
if (bdrv_find_base(image_bs) == image_bs) {
error_setg(errp, "not allowing backing file change on an image "
"without a backing file");
- bdrv_graph_rdunlock_main_loop();
- goto out;
+ goto out_rdlock;
}
/* even though we are not necessarily operating on bs, we need it to
* determine if block ops are currently prohibited on the chain */
if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_CHANGE, errp)) {
- bdrv_graph_rdunlock_main_loop();
- goto out;
+ goto out_rdlock;
}
- bdrv_graph_rdunlock_main_loop();
/* final sanity check */
if (!bdrv_chain_contains(bs, image_bs)) {
error_setg(errp, "'%s' and image file are not in the same chain",
device);
- goto out;
+ goto out_rdlock;
}
+ bdrv_graph_rdunlock_main_loop();
/* if not r/w, reopen to make r/w */
ro = bdrv_is_read_only(image_bs);
@@ -3499,6 +3500,11 @@ void qmp_change_backing_file(const char *device,
out:
aio_context_release(aio_context);
+ return;
+
+out_rdlock:
+ bdrv_graph_rdunlock_main_loop();
+ aio_context_release(aio_context);
}
void qmp_blockdev_add(BlockdevOptions *options, Error **errp)
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 11/25] block: Mark bdrv_filter_child() and callers GRAPH_RDLOCK
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (9 preceding siblings ...)
2023-11-07 18:45 ` [PULL 10/25] block: Mark bdrv_chain_contains() " Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 12/25] block: Mark bdrv_cow_child() " Kevin Wolf
` (14 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_filter_child() need to hold a reader lock for the graph because it
accesses bs->file/backing.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-12-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block_int-io.h | 4 ++--
block.c | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h
index 4e7bf57a5e..17547a2dab 100644
--- a/include/block/block_int-io.h
+++ b/include/block/block_int-io.h
@@ -131,8 +131,8 @@ int co_wrapper_mixed_bdrv_rdlock
bdrv_refresh_total_sectors(BlockDriverState *bs, int64_t hint);
BdrvChild *bdrv_cow_child(BlockDriverState *bs);
-BdrvChild *bdrv_filter_child(BlockDriverState *bs);
-BdrvChild *bdrv_filter_or_cow_child(BlockDriverState *bs);
+BdrvChild * GRAPH_RDLOCK bdrv_filter_child(BlockDriverState *bs);
+BdrvChild * GRAPH_RDLOCK bdrv_filter_or_cow_child(BlockDriverState *bs);
BdrvChild * GRAPH_RDLOCK bdrv_primary_child(BlockDriverState *bs);
BlockDriverState * GRAPH_RDLOCK bdrv_skip_filters(BlockDriverState *bs);
BlockDriverState * GRAPH_RDLOCK bdrv_backing_chain_next(BlockDriverState *bs);
diff --git a/block.c b/block.c
index bb322df7d8..499b147315 100644
--- a/block.c
+++ b/block.c
@@ -8491,8 +8491,8 @@ BdrvChild *bdrv_primary_child(BlockDriverState *bs)
return found;
}
-static BlockDriverState *bdrv_do_skip_filters(BlockDriverState *bs,
- bool stop_on_explicit_filter)
+static BlockDriverState * GRAPH_RDLOCK
+bdrv_do_skip_filters(BlockDriverState *bs, bool stop_on_explicit_filter)
{
BdrvChild *c;
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 12/25] block: Mark bdrv_cow_child() and callers GRAPH_RDLOCK
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (10 preceding siblings ...)
2023-11-07 18:45 ` [PULL 11/25] block: Mark bdrv_filter_child() " Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 13/25] block: Mark bdrv_set_backing_hd_drained() GRAPH_WRLOCK Kevin Wolf
` (13 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_cow_child() need to hold a reader lock for the graph because it
accesses bs->backing.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-13-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block_int-io.h | 5 +++--
block/stream.c | 2 +-
qemu-img.c | 11 ++++++++---
3 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h
index 17547a2dab..4a7cf2b4fd 100644
--- a/include/block/block_int-io.h
+++ b/include/block/block_int-io.h
@@ -130,14 +130,15 @@ bdrv_co_refresh_total_sectors(BlockDriverState *bs, int64_t hint);
int co_wrapper_mixed_bdrv_rdlock
bdrv_refresh_total_sectors(BlockDriverState *bs, int64_t hint);
-BdrvChild *bdrv_cow_child(BlockDriverState *bs);
+BdrvChild * GRAPH_RDLOCK bdrv_cow_child(BlockDriverState *bs);
BdrvChild * GRAPH_RDLOCK bdrv_filter_child(BlockDriverState *bs);
BdrvChild * GRAPH_RDLOCK bdrv_filter_or_cow_child(BlockDriverState *bs);
BdrvChild * GRAPH_RDLOCK bdrv_primary_child(BlockDriverState *bs);
BlockDriverState * GRAPH_RDLOCK bdrv_skip_filters(BlockDriverState *bs);
BlockDriverState * GRAPH_RDLOCK bdrv_backing_chain_next(BlockDriverState *bs);
-static inline BlockDriverState *bdrv_cow_bs(BlockDriverState *bs)
+static inline BlockDriverState * GRAPH_RDLOCK
+bdrv_cow_bs(BlockDriverState *bs)
{
IO_CODE();
return child_bs(bdrv_cow_child(bs));
diff --git a/block/stream.c b/block/stream.c
index c32c98339a..3f5d773535 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -90,7 +90,7 @@ static int stream_prepare(Job *job)
unfiltered_base = bdrv_skip_filters(base);
bdrv_graph_rdunlock_main_loop();
- if (bdrv_cow_child(unfiltered_bs)) {
+ if (unfiltered_bs_cow) {
const char *base_id = NULL, *base_fmt = NULL;
if (unfiltered_base) {
base_id = s->backing_file_str ?: unfiltered_base->filename;
diff --git a/qemu-img.c b/qemu-img.c
index 33f3ab5fba..5a77f67719 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -3541,7 +3541,7 @@ static int img_rebase(int argc, char **argv)
uint8_t *buf_old = NULL;
uint8_t *buf_new = NULL;
BlockDriverState *bs = NULL, *prefix_chain_bs = NULL;
- BlockDriverState *unfiltered_bs;
+ BlockDriverState *unfiltered_bs, *unfiltered_bs_cow;
BlockDriverInfo bdi = {0};
char *filename;
const char *fmt, *cache, *src_cache, *out_basefmt, *out_baseimg;
@@ -3675,6 +3675,7 @@ static int img_rebase(int argc, char **argv)
bdrv_graph_rdlock_main_loop();
unfiltered_bs = bdrv_skip_filters(bs);
+ unfiltered_bs_cow = bdrv_cow_bs(unfiltered_bs);
bdrv_graph_rdunlock_main_loop();
if (compress && !block_driver_can_compress(unfiltered_bs->drv)) {
@@ -3710,7 +3711,11 @@ static int img_rebase(int argc, char **argv)
/* For safe rebasing we need to compare old and new backing file */
if (!unsafe) {
QDict *options = NULL;
- BlockDriverState *base_bs = bdrv_cow_bs(unfiltered_bs);
+ BlockDriverState *base_bs;
+
+ bdrv_graph_rdlock_main_loop();
+ base_bs = bdrv_cow_bs(unfiltered_bs);
+ bdrv_graph_rdunlock_main_loop();
if (base_bs) {
blk_old_backing = blk_new(qemu_get_aio_context(),
@@ -3876,7 +3881,7 @@ static int img_rebase(int argc, char **argv)
* If cluster wasn't changed since prefix_chain, we don't need
* to take action
*/
- ret = bdrv_is_allocated_above(bdrv_cow_bs(unfiltered_bs),
+ ret = bdrv_is_allocated_above(unfiltered_bs_cow,
prefix_chain_bs, false,
offset, n, &n);
if (ret < 0) {
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 13/25] block: Mark bdrv_set_backing_hd_drained() GRAPH_WRLOCK
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (11 preceding siblings ...)
2023-11-07 18:45 ` [PULL 12/25] block: Mark bdrv_cow_child() " Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 14/25] block: Inline bdrv_set_backing_noperm() Kevin Wolf
` (12 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
Instead of taking the writer lock internally, require callers to already
hold it when calling bdrv_set_backing_hd_drained(). Basically everthing
in the function needs the lock and 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 <kwolf@redhat.com>
Message-ID: <20231027155333.420094-14-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-global-state.h | 7 ++++---
block.c | 4 ++--
block/stream.c | 2 ++
3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index 9a33bd7ef9..a1fd70ec97 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -101,9 +101,10 @@ bdrv_co_open_blockdev_ref(BlockdevRef *ref, Error **errp);
int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
Error **errp);
-int bdrv_set_backing_hd_drained(BlockDriverState *bs,
- BlockDriverState *backing_hd,
- Error **errp);
+int GRAPH_WRLOCK
+bdrv_set_backing_hd_drained(BlockDriverState *bs, BlockDriverState *backing_hd,
+ Error **errp);
+
int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
const char *bdref_key, Error **errp);
diff --git a/block.c b/block.c
index 499b147315..d79a6f41f9 100644
--- a/block.c
+++ b/block.c
@@ -3557,7 +3557,6 @@ int bdrv_set_backing_hd_drained(BlockDriverState *bs,
if (bs->backing) {
assert(bs->backing->bs->quiesce_counter > 0);
}
- bdrv_graph_wrlock(backing_hd);
ret = bdrv_set_backing_noperm(bs, backing_hd, tran, errp);
if (ret < 0) {
@@ -3567,7 +3566,6 @@ int bdrv_set_backing_hd_drained(BlockDriverState *bs,
ret = bdrv_refresh_perms(bs, tran, errp);
out:
tran_finalize(tran, ret);
- bdrv_graph_wrunlock();
return ret;
}
@@ -3580,7 +3578,9 @@ int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
bdrv_ref(drain_bs);
bdrv_drained_begin(drain_bs);
+ bdrv_graph_wrlock(backing_hd);
ret = bdrv_set_backing_hd_drained(bs, backing_hd, errp);
+ bdrv_graph_wrunlock();
bdrv_drained_end(drain_bs);
bdrv_unref(drain_bs);
diff --git a/block/stream.c b/block/stream.c
index 3f5d773535..0b92410c00 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -99,7 +99,9 @@ static int stream_prepare(Job *job)
}
}
+ bdrv_graph_wrlock(base);
bdrv_set_backing_hd_drained(unfiltered_bs, base, &local_err);
+ bdrv_graph_wrunlock();
/*
* This call will do I/O, so the graph can change again from here on.
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 14/25] block: Inline bdrv_set_backing_noperm()
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (12 preceding siblings ...)
2023-11-07 18:45 ` [PULL 13/25] block: Mark bdrv_set_backing_hd_drained() GRAPH_WRLOCK Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 15/25] block: Mark bdrv_replace_node_common() GRAPH_WRLOCK Kevin Wolf
` (11 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
It's only a single line and has a single caller. Inlining makes things
a bit easier to follow.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-15-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 14 +-------------
1 file changed, 1 insertion(+), 13 deletions(-)
diff --git a/block.c b/block.c
index d79a6f41f9..dc43e36f69 100644
--- a/block.c
+++ b/block.c
@@ -3532,19 +3532,7 @@ out:
*
* 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,
- BlockDriverState *backing_hd,
- Transaction *tran, Error **errp)
-{
- GLOBAL_STATE_CODE();
- return bdrv_set_file_or_backing_noperm(bs, backing_hd, true, tran, errp);
-}
-
int bdrv_set_backing_hd_drained(BlockDriverState *bs,
BlockDriverState *backing_hd,
Error **errp)
@@ -3558,7 +3546,7 @@ int bdrv_set_backing_hd_drained(BlockDriverState *bs,
assert(bs->backing->bs->quiesce_counter > 0);
}
- ret = bdrv_set_backing_noperm(bs, backing_hd, tran, errp);
+ ret = bdrv_set_file_or_backing_noperm(bs, backing_hd, true, tran, errp);
if (ret < 0) {
goto out;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 15/25] block: Mark bdrv_replace_node_common() GRAPH_WRLOCK
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (13 preceding siblings ...)
2023-11-07 18:45 ` [PULL 14/25] block: Inline bdrv_set_backing_noperm() Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 16/25] block: Mark bdrv_replace_node() GRAPH_WRLOCK Kevin Wolf
` (10 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
Instead of taking the writer lock internally, require callers to already
hold it when calling bdrv_replace_node_common(). Basically everthing in
the function needs the lock and 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 <kwolf@redhat.com>
Message-ID: <20231027155333.420094-16-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 68 ++++++++++++++++++++++++++++++++++++---------------------
1 file changed, 43 insertions(+), 25 deletions(-)
diff --git a/block.c b/block.c
index dc43e36f69..c7409cf658 100644
--- a/block.c
+++ b/block.c
@@ -5412,6 +5412,9 @@ bdrv_replace_node_noperm(BlockDriverState *from,
}
/*
+ * Switch all parents of @from to point to @to instead. @from and @to must be in
+ * the same AioContext and both must be drained.
+ *
* With auto_skip=true bdrv_replace_node_common skips updating from parents
* if it creates a parent-child relation loop or if parent is block-job.
*
@@ -5421,10 +5424,9 @@ bdrv_replace_node_noperm(BlockDriverState *from,
* With @detach_subchain=true @to must be in a backing chain of @from. In this
* case backing link of the cow-parent of @to is removed.
*/
-static int bdrv_replace_node_common(BlockDriverState *from,
- BlockDriverState *to,
- bool auto_skip, bool detach_subchain,
- Error **errp)
+static int GRAPH_WRLOCK
+bdrv_replace_node_common(BlockDriverState *from, BlockDriverState *to,
+ bool auto_skip, bool detach_subchain, Error **errp)
{
Transaction *tran = tran_new();
g_autoptr(GSList) refresh_list = NULL;
@@ -5433,16 +5435,9 @@ static int bdrv_replace_node_common(BlockDriverState *from,
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);
-
- assert(qemu_get_current_aio_context() == qemu_get_aio_context());
+ assert(from->quiesce_counter);
+ assert(to->quiesce_counter);
assert(bdrv_get_aio_context(from) == bdrv_get_aio_context(to));
- bdrv_drained_begin(from);
- bdrv_drained_begin(to);
-
- bdrv_graph_wrlock(to);
if (detach_subchain) {
assert(bdrv_chain_contains(from, to));
@@ -5483,33 +5478,51 @@ static int bdrv_replace_node_common(BlockDriverState *from,
out:
tran_finalize(tran, ret);
- bdrv_graph_wrunlock();
-
- bdrv_drained_end(to);
- bdrv_drained_end(from);
- bdrv_unref(from);
-
return ret;
}
int bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
Error **errp)
{
+ int ret;
+
GLOBAL_STATE_CODE();
- return bdrv_replace_node_common(from, to, true, false, errp);
+ /* 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;
}
int bdrv_drop_filter(BlockDriverState *bs, Error **errp)
{
BlockDriverState *child_bs;
+ int ret;
GLOBAL_STATE_CODE();
+
bdrv_graph_rdlock_main_loop();
child_bs = bdrv_filter_or_cow_bs(bs);
bdrv_graph_rdunlock_main_loop();
- return bdrv_replace_node_common(bs, child_bs, true, true, errp);
+ bdrv_drained_begin(child_bs);
+ bdrv_graph_wrlock(bs);
+ ret = bdrv_replace_node_common(bs, child_bs, true, true, errp);
+ bdrv_graph_wrunlock();
+ bdrv_drained_end(child_bs);
+
+ return ret;
}
/*
@@ -5957,15 +5970,15 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
bdrv_ref(top);
bdrv_drained_begin(base);
- bdrv_graph_rdlock_main_loop();
+ bdrv_graph_wrlock(base);
if (!top->drv || !base->drv) {
- goto exit;
+ goto exit_wrlock;
}
/* Make sure that base is in the backing chain of top */
if (!bdrv_chain_contains(top, base)) {
- goto exit;
+ goto exit_wrlock;
}
/* If 'base' recursively inherits from 'top' then we should set
@@ -5997,6 +6010,8 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
* That's a FIXME.
*/
bdrv_replace_node_common(top, base, false, false, &local_err);
+ bdrv_graph_wrunlock();
+
if (local_err) {
error_report_err(local_err);
goto exit;
@@ -6029,8 +6044,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base,
}
ret = 0;
+ goto exit;
+
+exit_wrlock:
+ bdrv_graph_wrunlock();
exit:
- bdrv_graph_rdunlock_main_loop();
bdrv_drained_end(base);
bdrv_unref(top);
return ret;
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 16/25] block: Mark bdrv_replace_node() GRAPH_WRLOCK
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (14 preceding siblings ...)
2023-11-07 18:45 ` [PULL 15/25] block: Mark bdrv_replace_node_common() GRAPH_WRLOCK Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 17/25] block: Protect bs->backing with graph_lock Kevin Wolf
` (9 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
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 <kwolf@redhat.com>
Message-ID: <20231027155333.420094-17-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-global-state.h | 6 ++++--
block.c | 30 +++++++++++-------------------
block/commit.c | 13 +++++++++++--
block/mirror.c | 26 ++++++++++++++++----------
blockdev.c | 5 +++++
tests/unit/test-bdrv-drain.c | 6 ++++++
tests/unit/test-bdrv-graph-mod.c | 13 +++++++++++--
7 files changed, 64 insertions(+), 35 deletions(-)
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/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/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);
}
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 17/25] block: Protect bs->backing with graph_lock
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (15 preceding siblings ...)
2023-11-07 18:45 ` [PULL 16/25] block: Mark bdrv_replace_node() GRAPH_WRLOCK Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 18/25] blkverify: Add locking for request_fn Kevin Wolf
` (8 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
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 <kwolf@redhat.com>
Message-ID: <20231027155333.420094-18-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block_int-common.h | 2 +-
block.c | 27 +++++++++++++++++----------
block/commit.c | 5 ++++-
block/mirror.c | 17 ++++++++++++-----
block/qed.c | 2 +-
block/replication.c | 7 ++++++-
block/vmdk.c | 7 ++++---
tests/unit/test-bdrv-drain.c | 22 ++++++++++++++++++----
tests/unit/test-bdrv-graph-mod.c | 5 ++++-
9 files changed, 67 insertions(+), 27 deletions(-)
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/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..aa5302aaae 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;
@@ -943,6 +947,7 @@ static int coroutine_fn mirror_run(Job *job, Error **errp)
}
bdrv_graph_co_rdlock();
+ bs = bdrv_filter_bs(s->mirror_top_bs);
s->bdev_length = bdrv_co_getlength(bs);
bdrv_graph_co_rdunlock();
@@ -1070,7 +1075,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 +1647,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/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);
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 18/25] blkverify: Add locking for request_fn
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (16 preceding siblings ...)
2023-11-07 18:45 ` [PULL 17/25] block: Protect bs->backing with graph_lock Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:45 ` [PULL 19/25] block: Introduce bdrv_co_change_backing_file() Kevin Wolf
` (7 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
This is either bdrv_co_preadv() or bdrv_co_pwritev() which both need to
have the graph locked. Annotate the function pointer accordingly and add
locking to its callers.
This shouldn't actually have resulted in a bug because the graph lock is
already held by blkverify_co_prwv(), which waits for the coroutines to
terminate. Annotate with GRAPH_RDLOCK as well to make this clearer.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-19-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/blkverify.c | 16 +++++++++++-----
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/block/blkverify.c b/block/blkverify.c
index dae9716a26..9ea2cb4cc5 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -33,8 +33,8 @@ typedef struct BlkverifyRequest {
uint64_t bytes;
int flags;
- int (*request_fn)(BdrvChild *, int64_t, int64_t, QEMUIOVector *,
- BdrvRequestFlags);
+ int GRAPH_RDLOCK_PTR (*request_fn)(
+ BdrvChild *, int64_t, int64_t, QEMUIOVector *, BdrvRequestFlags);
int ret; /* test image result */
int raw_ret; /* raw image result */
@@ -170,8 +170,11 @@ static void coroutine_fn blkverify_do_test_req(void *opaque)
BlkverifyRequest *r = opaque;
BDRVBlkverifyState *s = r->bs->opaque;
+ bdrv_graph_co_rdlock();
r->ret = r->request_fn(s->test_file, r->offset, r->bytes, r->qiov,
r->flags);
+ bdrv_graph_co_rdunlock();
+
r->done++;
qemu_coroutine_enter_if_inactive(r->co);
}
@@ -180,13 +183,16 @@ static void coroutine_fn blkverify_do_raw_req(void *opaque)
{
BlkverifyRequest *r = opaque;
+ bdrv_graph_co_rdlock();
r->raw_ret = r->request_fn(r->bs->file, r->offset, r->bytes, r->raw_qiov,
r->flags);
+ bdrv_graph_co_rdunlock();
+
r->done++;
qemu_coroutine_enter_if_inactive(r->co);
}
-static int coroutine_fn
+static int coroutine_fn GRAPH_RDLOCK
blkverify_co_prwv(BlockDriverState *bs, BlkverifyRequest *r, uint64_t offset,
uint64_t bytes, QEMUIOVector *qiov, QEMUIOVector *raw_qiov,
int flags, bool is_write)
@@ -222,7 +228,7 @@ blkverify_co_prwv(BlockDriverState *bs, BlkverifyRequest *r, uint64_t offset,
return r->ret;
}
-static int coroutine_fn
+static int coroutine_fn GRAPH_RDLOCK
blkverify_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
QEMUIOVector *qiov, BdrvRequestFlags flags)
{
@@ -251,7 +257,7 @@ blkverify_co_preadv(BlockDriverState *bs, int64_t offset, int64_t bytes,
return ret;
}
-static int coroutine_fn
+static int coroutine_fn GRAPH_RDLOCK
blkverify_co_pwritev(BlockDriverState *bs, int64_t offset, int64_t bytes,
QEMUIOVector *qiov, BdrvRequestFlags flags)
{
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 19/25] block: Introduce bdrv_co_change_backing_file()
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (17 preceding siblings ...)
2023-11-07 18:45 ` [PULL 18/25] blkverify: Add locking for request_fn Kevin Wolf
@ 2023-11-07 18:45 ` Kevin Wolf
2023-11-07 18:46 ` [PULL 20/25] block: Add missing GRAPH_RDLOCK annotations Kevin Wolf
` (6 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:45 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
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 <kwolf@redhat.com>
Message-ID: <20231027155333.420094-20-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
include/block/block-global-state.h | 3 +-
include/block/block-io.h | 8 ++
include/block/block_int-common.h | 5 +-
block.c | 11 +--
block/qcow2.c | 128 +++++++++++++++--------------
block/qed.c | 64 +++++++--------
tests/unit/test-bdrv-drain.c | 8 +-
7 files changed, 120 insertions(+), 107 deletions(-)
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/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/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)
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 20/25] block: Add missing GRAPH_RDLOCK annotations
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (18 preceding siblings ...)
2023-11-07 18:45 ` [PULL 19/25] block: Introduce bdrv_co_change_backing_file() Kevin Wolf
@ 2023-11-07 18:46 ` Kevin Wolf
2023-11-07 18:46 ` [PULL 21/25] qcow2: Take locks for accessing bs->file Kevin Wolf
` (5 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
This adds GRAPH_RDLOCK to some driver callbacks that are already called
with the graph lock held, and which will need the annotation because
they access bs->file, but don't have it yet.
This also covers a few callbacks that were not marked GRAPH_RDLOCK
before, but where updating BlockDriver is trivially possible.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-21-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2.h | 11 ++++++-----
include/block/block_int-common.h | 17 +++++++++--------
block/blkdebug.c | 13 +++++--------
block/blkverify.c | 2 +-
block/copy-before-write.c | 2 +-
block/filter-compress.c | 3 ++-
block/io.c | 2 ++
block/parallels.c | 12 ++++--------
block/preallocate.c | 4 ++--
block/qcow.c | 2 +-
block/qcow2.c | 4 ++--
block/qed.c | 10 ++++------
block/raw-format.c | 16 ++++++++--------
block/snapshot-access.c | 2 +-
block/vdi.c | 9 ++++-----
block/vhdx.c | 6 +++---
16 files changed, 55 insertions(+), 60 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 29958c512b..948335979f 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -972,11 +972,12 @@ int GRAPH_RDLOCK
qcow2_snapshot_delete(BlockDriverState *bs, const char *snapshot_id,
const char *name, Error **errp);
-int qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
-int qcow2_snapshot_load_tmp(BlockDriverState *bs,
- const char *snapshot_id,
- const char *name,
- Error **errp);
+int GRAPH_RDLOCK
+qcow2_snapshot_list(BlockDriverState *bs, QEMUSnapshotInfo **psn_tab);
+
+int GRAPH_RDLOCK
+qcow2_snapshot_load_tmp(BlockDriverState *bs, const char *snapshot_id,
+ const char *name, Error **errp);
void qcow2_free_snapshots(BlockDriverState *bs);
int coroutine_fn GRAPH_RDLOCK
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 59f6d7f195..63bc523d7c 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -310,7 +310,7 @@ struct BlockDriver {
* One example usage is to avoid waiting for an nbd target node reconnect
* timeout during job-cancel with force=true.
*/
- void (*bdrv_cancel_in_flight)(BlockDriverState *bs);
+ void GRAPH_RDLOCK_PTR (*bdrv_cancel_in_flight)(BlockDriverState *bs);
int GRAPH_RDLOCK_PTR (*bdrv_inactivate)(BlockDriverState *bs);
@@ -324,12 +324,12 @@ struct BlockDriver {
BlockDriverState *bs, const char *snapshot_id, const char *name,
Error **errp);
- int (*bdrv_snapshot_list)(BlockDriverState *bs,
- QEMUSnapshotInfo **psn_info);
- int (*bdrv_snapshot_load_tmp)(BlockDriverState *bs,
- const char *snapshot_id,
- const char *name,
- Error **errp);
+ int GRAPH_RDLOCK_PTR (*bdrv_snapshot_list)(
+ BlockDriverState *bs, QEMUSnapshotInfo **psn_info);
+
+ int GRAPH_RDLOCK_PTR (*bdrv_snapshot_load_tmp)(
+ BlockDriverState *bs, const char *snapshot_id, const char *name,
+ Error **errp);
int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_change_backing_file)(
BlockDriverState *bs, const char *backing_file,
@@ -396,7 +396,8 @@ struct BlockDriver {
* Only drivers that want to override guest geometry implement this
* callback; see hd_geometry_guess().
*/
- int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo);
+ int GRAPH_RDLOCK_PTR (*bdrv_probe_geometry)(
+ BlockDriverState *bs, HDGeometry *geo);
void GRAPH_WRLOCK_PTR (*bdrv_add_child)(
BlockDriverState *parent, BlockDriverState *child, Error **errp);
diff --git a/block/blkdebug.c b/block/blkdebug.c
index addad914b3..230efa9e8d 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -746,13 +746,10 @@ blkdebug_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
return bdrv_co_pdiscard(bs->file, offset, bytes);
}
-static int coroutine_fn blkdebug_co_block_status(BlockDriverState *bs,
- bool want_zero,
- int64_t offset,
- int64_t bytes,
- int64_t *pnum,
- int64_t *map,
- BlockDriverState **file)
+static int coroutine_fn GRAPH_RDLOCK
+blkdebug_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset,
+ int64_t bytes, int64_t *pnum, int64_t *map,
+ BlockDriverState **file)
{
int err;
@@ -973,7 +970,7 @@ blkdebug_co_getlength(BlockDriverState *bs)
return bdrv_co_getlength(bs->file->bs);
}
-static void blkdebug_refresh_filename(BlockDriverState *bs)
+static void GRAPH_RDLOCK blkdebug_refresh_filename(BlockDriverState *bs)
{
BDRVBlkdebugState *s = bs->opaque;
const QDictEntry *e;
diff --git a/block/blkverify.c b/block/blkverify.c
index 9ea2cb4cc5..a96905db35 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -288,7 +288,7 @@ blkverify_recurse_can_replace(BlockDriverState *bs,
bdrv_recurse_can_replace(s->test_file->bs, to_replace);
}
-static void blkverify_refresh_filename(BlockDriverState *bs)
+static void GRAPH_RDLOCK blkverify_refresh_filename(BlockDriverState *bs)
{
BDRVBlkverifyState *s = bs->opaque;
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 4ffabc5ca2..8193d3a4cd 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -335,7 +335,7 @@ cbw_co_pdiscard_snapshot(BlockDriverState *bs, int64_t offset, int64_t bytes)
return bdrv_co_pdiscard(s->target, offset, bytes);
}
-static void cbw_refresh_filename(BlockDriverState *bs)
+static void GRAPH_RDLOCK cbw_refresh_filename(BlockDriverState *bs)
{
pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
bs->file->bs->filename);
diff --git a/block/filter-compress.c b/block/filter-compress.c
index 320d9576fa..e3fc82f322 100644
--- a/block/filter-compress.c
+++ b/block/filter-compress.c
@@ -97,7 +97,8 @@ compress_co_pdiscard(BlockDriverState *bs, int64_t offset, int64_t bytes)
}
-static void compress_refresh_limits(BlockDriverState *bs, Error **errp)
+static void GRAPH_RDLOCK
+compress_refresh_limits(BlockDriverState *bs, Error **errp)
{
BlockDriverInfo bdi;
int ret;
diff --git a/block/io.c b/block/io.c
index 527a1de04e..7e62fabbf5 100644
--- a/block/io.c
+++ b/block/io.c
@@ -3685,6 +3685,8 @@ out:
void bdrv_cancel_in_flight(BlockDriverState *bs)
{
GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (!bs || !bs->drv) {
return;
}
diff --git a/block/parallels.c b/block/parallels.c
index 6318dd02e7..9b9bd1f993 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -415,14 +415,10 @@ parallels_co_flush_to_os(BlockDriverState *bs)
return 0;
}
-
-static int coroutine_fn parallels_co_block_status(BlockDriverState *bs,
- bool want_zero,
- int64_t offset,
- int64_t bytes,
- int64_t *pnum,
- int64_t *map,
- BlockDriverState **file)
+static int coroutine_fn GRAPH_RDLOCK
+parallels_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset,
+ int64_t bytes, int64_t *pnum, int64_t *map,
+ BlockDriverState **file)
{
BDRVParallelsState *s = bs->opaque;
int count;
diff --git a/block/preallocate.c b/block/preallocate.c
index bfb638d8b1..4e0c891ab2 100644
--- a/block/preallocate.c
+++ b/block/preallocate.c
@@ -541,8 +541,8 @@ static void preallocate_drop_resize_bh(void *opaque)
preallocate_drop_resize(opaque, NULL);
}
-static void preallocate_set_perm(BlockDriverState *bs,
- uint64_t perm, uint64_t shared)
+static void GRAPH_RDLOCK
+preallocate_set_perm(BlockDriverState *bs, uint64_t perm, uint64_t shared)
{
BDRVPreallocateState *s = bs->opaque;
diff --git a/block/qcow.c b/block/qcow.c
index eab68e387c..1e2835f1f4 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -1024,7 +1024,7 @@ fail:
return ret;
}
-static int qcow_make_empty(BlockDriverState *bs)
+static int GRAPH_RDLOCK qcow_make_empty(BlockDriverState *bs)
{
BDRVQcowState *s = bs->opaque;
uint32_t l1_length = s->l1_size * sizeof(uint64_t);
diff --git a/block/qcow2.c b/block/qcow2.c
index 875e613ea9..7ab399ffc5 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -5226,8 +5226,8 @@ qcow2_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
return 0;
}
-static ImageInfoSpecific *qcow2_get_specific_info(BlockDriverState *bs,
- Error **errp)
+static ImageInfoSpecific * GRAPH_RDLOCK
+qcow2_get_specific_info(BlockDriverState *bs, Error **errp)
{
BDRVQcow2State *s = bs->opaque;
ImageInfoSpecific *spec_info;
diff --git a/block/qed.c b/block/qed.c
index 996aa384fe..f4c1628a81 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -1443,12 +1443,10 @@ bdrv_qed_co_pwrite_zeroes(BlockDriverState *bs, int64_t offset, int64_t bytes,
QED_AIOCB_WRITE | QED_AIOCB_ZERO);
}
-static int coroutine_fn bdrv_qed_co_truncate(BlockDriverState *bs,
- int64_t offset,
- bool exact,
- PreallocMode prealloc,
- BdrvRequestFlags flags,
- Error **errp)
+static int coroutine_fn GRAPH_RDLOCK
+bdrv_qed_co_truncate(BlockDriverState *bs, int64_t offset, bool exact,
+ PreallocMode prealloc, BdrvRequestFlags flags,
+ Error **errp)
{
BDRVQEDState *s = bs->opaque;
uint64_t old_image_size;
diff --git a/block/raw-format.c b/block/raw-format.c
index 8ca74c1cf3..2640d54801 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -279,11 +279,10 @@ fail:
return ret;
}
-static int coroutine_fn raw_co_block_status(BlockDriverState *bs,
- bool want_zero, int64_t offset,
- int64_t bytes, int64_t *pnum,
- int64_t *map,
- BlockDriverState **file)
+static int coroutine_fn GRAPH_RDLOCK
+raw_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset,
+ int64_t bytes, int64_t *pnum, int64_t *map,
+ BlockDriverState **file)
{
BDRVRawState *s = bs->opaque;
*pnum = bytes;
@@ -397,7 +396,7 @@ raw_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi)
return bdrv_co_get_info(bs->file->bs, bdi);
}
-static void raw_refresh_limits(BlockDriverState *bs, Error **errp)
+static void GRAPH_RDLOCK raw_refresh_limits(BlockDriverState *bs, Error **errp)
{
bs->bl.has_variable_length = bs->file->bs->bl.has_variable_length;
@@ -561,7 +560,8 @@ raw_probe_blocksizes(BlockDriverState *bs, BlockSizes *bsz)
return 0;
}
-static int raw_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
+static int GRAPH_RDLOCK
+raw_probe_geometry(BlockDriverState *bs, HDGeometry *geo)
{
BDRVRawState *s = bs->opaque;
if (s->offset || s->has_size) {
@@ -611,7 +611,7 @@ static const char *const raw_strong_runtime_opts[] = {
NULL
};
-static void raw_cancel_in_flight(BlockDriverState *bs)
+static void GRAPH_RDLOCK raw_cancel_in_flight(BlockDriverState *bs)
{
bdrv_cancel_in_flight(bs->file->bs);
}
diff --git a/block/snapshot-access.c b/block/snapshot-access.c
index 8d4e8932b8..7c45739eb1 100644
--- a/block/snapshot-access.c
+++ b/block/snapshot-access.c
@@ -73,7 +73,7 @@ snapshot_access_co_pwritev_part(BlockDriverState *bs,
}
-static void snapshot_access_refresh_filename(BlockDriverState *bs)
+static void GRAPH_RDLOCK snapshot_access_refresh_filename(BlockDriverState *bs)
{
pstrcpy(bs->exact_filename, sizeof(bs->exact_filename),
bs->file->bs->filename);
diff --git a/block/vdi.c b/block/vdi.c
index 8e144ce523..84150180ec 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -520,11 +520,10 @@ static int vdi_reopen_prepare(BDRVReopenState *state,
return 0;
}
-static int coroutine_fn vdi_co_block_status(BlockDriverState *bs,
- bool want_zero,
- int64_t offset, int64_t bytes,
- int64_t *pnum, int64_t *map,
- BlockDriverState **file)
+static int coroutine_fn GRAPH_RDLOCK
+vdi_co_block_status(BlockDriverState *bs, bool want_zero, int64_t offset,
+ int64_t bytes, int64_t *pnum, int64_t *map,
+ BlockDriverState **file)
{
BDRVVdiState *s = (BDRVVdiState *)bs->opaque;
size_t bmap_index = offset / s->block_size;
diff --git a/block/vhdx.c b/block/vhdx.c
index e136ba1ae1..ac4d52e8c6 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -2163,9 +2163,9 @@ fail:
* r/w and any log has already been replayed, so there is nothing (currently)
* for us to do here
*/
-static int coroutine_fn vhdx_co_check(BlockDriverState *bs,
- BdrvCheckResult *result,
- BdrvCheckMode fix)
+static int coroutine_fn GRAPH_RDLOCK
+vhdx_co_check(BlockDriverState *bs, BdrvCheckResult *result,
+ BdrvCheckMode fix)
{
BDRVVHDXState *s = bs->opaque;
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 21/25] qcow2: Take locks for accessing bs->file
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (19 preceding siblings ...)
2023-11-07 18:46 ` [PULL 20/25] block: Add missing GRAPH_RDLOCK annotations Kevin Wolf
@ 2023-11-07 18:46 ` Kevin Wolf
2023-11-07 18:46 ` [PULL 22/25] vhdx: " Kevin Wolf
` (4 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
This updates the qcow2 code to add GRAPH_RDLOCK annotations for all
places that read bs->file.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-22-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/qcow2.h | 48 ++++++++++++++++++++++++++-----------------
block/qcow2-bitmap.c | 14 +++++++------
block/qcow2-cluster.c | 25 +++++++++++-----------
block/qcow2.c | 13 ++++++++----
4 files changed, 59 insertions(+), 41 deletions(-)
diff --git a/block/qcow2.h b/block/qcow2.h
index 948335979f..a9e3481c6e 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -641,7 +641,7 @@ static inline void set_l2_bitmap(BDRVQcow2State *s, uint64_t *l2_slice,
l2_slice[idx + 1] = cpu_to_be64(bitmap);
}
-static inline bool has_data_file(BlockDriverState *bs)
+static inline bool GRAPH_RDLOCK has_data_file(BlockDriverState *bs)
{
BDRVQcow2State *s = bs->opaque;
return (s->data_file != bs->file);
@@ -709,8 +709,8 @@ static inline int64_t qcow2_vm_state_offset(BDRVQcow2State *s)
return (int64_t)s->l1_vm_state_index << (s->cluster_bits + s->l2_bits);
}
-static inline QCow2ClusterType qcow2_get_cluster_type(BlockDriverState *bs,
- uint64_t l2_entry)
+static inline QCow2ClusterType GRAPH_RDLOCK
+qcow2_get_cluster_type(BlockDriverState *bs, uint64_t l2_entry)
{
BDRVQcow2State *s = bs->opaque;
@@ -743,7 +743,7 @@ static inline QCow2ClusterType qcow2_get_cluster_type(BlockDriverState *bs,
* (this checks the whole entry and bitmap, not only the bits related
* to subcluster @sc_index).
*/
-static inline
+static inline GRAPH_RDLOCK
QCow2SubclusterType qcow2_get_subcluster_type(BlockDriverState *bs,
uint64_t l2_entry,
uint64_t l2_bitmap,
@@ -834,9 +834,9 @@ int64_t qcow2_refcount_metadata_size(int64_t clusters, size_t cluster_size,
int refcount_order, bool generous_increase,
uint64_t *refblock_count);
-int qcow2_mark_dirty(BlockDriverState *bs);
-int qcow2_mark_corrupt(BlockDriverState *bs);
-int qcow2_update_header(BlockDriverState *bs);
+int GRAPH_RDLOCK qcow2_mark_dirty(BlockDriverState *bs);
+int GRAPH_RDLOCK qcow2_mark_corrupt(BlockDriverState *bs);
+int GRAPH_RDLOCK qcow2_update_header(BlockDriverState *bs);
void GRAPH_RDLOCK
qcow2_signal_corruption(BlockDriverState *bs, bool fatal, int64_t offset,
@@ -890,10 +890,11 @@ int GRAPH_RDLOCK qcow2_write_caches(BlockDriverState *bs);
int coroutine_fn qcow2_check_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
BdrvCheckMode fix);
-void qcow2_process_discards(BlockDriverState *bs, int ret);
+void GRAPH_RDLOCK qcow2_process_discards(BlockDriverState *bs, int ret);
-int qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
- int64_t size);
+int GRAPH_RDLOCK
+qcow2_check_metadata_overlap(BlockDriverState *bs, int ign, int64_t offset,
+ int64_t size);
int GRAPH_RDLOCK
qcow2_pre_write_overlap_check(BlockDriverState *bs, int ign, int64_t offset,
int64_t size, bool data_file);
@@ -939,8 +940,9 @@ qcow2_alloc_host_offset(BlockDriverState *bs, uint64_t offset,
int coroutine_fn GRAPH_RDLOCK
qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs, uint64_t offset,
int compressed_size, uint64_t *host_offset);
-void qcow2_parse_compressed_l2_entry(BlockDriverState *bs, uint64_t l2_entry,
- uint64_t *coffset, int *csize);
+void GRAPH_RDLOCK
+qcow2_parse_compressed_l2_entry(BlockDriverState *bs, uint64_t l2_entry,
+ uint64_t *coffset, int *csize);
int coroutine_fn GRAPH_RDLOCK
qcow2_alloc_cluster_link_l2(BlockDriverState *bs, QCowL2Meta *m);
@@ -993,8 +995,9 @@ qcow2_check_fix_snapshot_table(BlockDriverState *bs, BdrvCheckResult *result,
BdrvCheckMode fix);
/* qcow2-cache.c functions */
-Qcow2Cache *qcow2_cache_create(BlockDriverState *bs, int num_tables,
- unsigned table_size);
+Qcow2Cache * GRAPH_RDLOCK
+qcow2_cache_create(BlockDriverState *bs, int num_tables, unsigned table_size);
+
int qcow2_cache_destroy(Qcow2Cache *c);
void qcow2_cache_entry_mark_dirty(Qcow2Cache *c, void *table);
@@ -1020,17 +1023,24 @@ void *qcow2_cache_is_table_offset(Qcow2Cache *c, uint64_t offset);
void qcow2_cache_discard(Qcow2Cache *c, void *table);
/* qcow2-bitmap.c functions */
-int coroutine_fn
+int coroutine_fn GRAPH_RDLOCK
qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
void **refcount_table,
int64_t *refcount_table_size);
+
bool coroutine_fn GRAPH_RDLOCK
-qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated, Error **errp);
-bool qcow2_get_bitmap_info_list(BlockDriverState *bs,
- Qcow2BitmapInfoList **info_list, Error **errp);
+qcow2_load_dirty_bitmaps(BlockDriverState *bs, bool *header_updated,
+ Error **errp);
+
+bool GRAPH_RDLOCK
+qcow2_get_bitmap_info_list(BlockDriverState *bs,
+ Qcow2BitmapInfoList **info_list, Error **errp);
+
int GRAPH_RDLOCK qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
int GRAPH_RDLOCK qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
-int coroutine_fn qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
+
+int coroutine_fn GRAPH_RDLOCK
+qcow2_truncate_bitmaps_check(BlockDriverState *bs, Error **errp);
bool GRAPH_RDLOCK
qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, bool release_stored,
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 3058309c47..0e567ed588 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -105,7 +105,7 @@ static inline bool can_write(BlockDriverState *bs)
return !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE);
}
-static int update_header_sync(BlockDriverState *bs)
+static int GRAPH_RDLOCK update_header_sync(BlockDriverState *bs)
{
int ret;
@@ -221,8 +221,9 @@ clear_bitmap_table(BlockDriverState *bs, uint64_t *bitmap_table,
}
}
-static int bitmap_table_load(BlockDriverState *bs, Qcow2BitmapTable *tb,
- uint64_t **bitmap_table)
+static int GRAPH_RDLOCK
+bitmap_table_load(BlockDriverState *bs, Qcow2BitmapTable *tb,
+ uint64_t **bitmap_table)
{
int ret;
BDRVQcow2State *s = bs->opaque;
@@ -551,8 +552,9 @@ static uint32_t bitmap_list_count(Qcow2BitmapList *bm_list)
* Get bitmap list from qcow2 image. Actually reads bitmap directory,
* checks it and convert to bitmap list.
*/
-static Qcow2BitmapList *bitmap_list_load(BlockDriverState *bs, uint64_t offset,
- uint64_t size, Error **errp)
+static Qcow2BitmapList * GRAPH_RDLOCK
+bitmap_list_load(BlockDriverState *bs, uint64_t offset, uint64_t size,
+ Error **errp)
{
int ret;
BDRVQcow2State *s = bs->opaque;
@@ -961,7 +963,7 @@ static void set_readonly_helper(gpointer bitmap, gpointer value)
* If header_updated is not NULL then it is set appropriately regardless of
* the return value.
*/
-bool coroutine_fn GRAPH_RDLOCK
+bool coroutine_fn
qcow2_load_dirty_bitmaps(BlockDriverState *bs,
bool *header_updated, Error **errp)
{
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 5af439bd11..ce8c0076b3 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -391,11 +391,10 @@ fail:
* If the L2 entry is invalid return -errno and set @type to
* QCOW2_SUBCLUSTER_INVALID.
*/
-static int qcow2_get_subcluster_range_type(BlockDriverState *bs,
- uint64_t l2_entry,
- uint64_t l2_bitmap,
- unsigned sc_from,
- QCow2SubclusterType *type)
+static int GRAPH_RDLOCK
+qcow2_get_subcluster_range_type(BlockDriverState *bs, uint64_t l2_entry,
+ uint64_t l2_bitmap, unsigned sc_from,
+ QCow2SubclusterType *type)
{
BDRVQcow2State *s = bs->opaque;
uint32_t val;
@@ -442,9 +441,10 @@ static int qcow2_get_subcluster_range_type(BlockDriverState *bs,
* On failure return -errno and update @l2_index to point to the
* invalid entry.
*/
-static int count_contiguous_subclusters(BlockDriverState *bs, int nb_clusters,
- unsigned sc_index, uint64_t *l2_slice,
- unsigned *l2_index)
+static int GRAPH_RDLOCK
+count_contiguous_subclusters(BlockDriverState *bs, int nb_clusters,
+ unsigned sc_index, uint64_t *l2_slice,
+ unsigned *l2_index)
{
BDRVQcow2State *s = bs->opaque;
int i, count = 0;
@@ -1329,7 +1329,8 @@ calculate_l2_meta(BlockDriverState *bs, uint64_t host_cluster_offset,
* requires a new allocation (that is, if the cluster is unallocated
* or has refcount > 1 and therefore cannot be written in-place).
*/
-static bool cluster_needs_new_alloc(BlockDriverState *bs, uint64_t l2_entry)
+static bool GRAPH_RDLOCK
+cluster_needs_new_alloc(BlockDriverState *bs, uint64_t l2_entry)
{
switch (qcow2_get_cluster_type(bs, l2_entry)) {
case QCOW2_CLUSTER_NORMAL:
@@ -1360,9 +1361,9 @@ static bool cluster_needs_new_alloc(BlockDriverState *bs, uint64_t l2_entry)
* allocated and can be overwritten in-place (this includes clusters
* of type QCOW2_CLUSTER_ZERO_ALLOC).
*/
-static int count_single_write_clusters(BlockDriverState *bs, int nb_clusters,
- uint64_t *l2_slice, int l2_index,
- bool new_alloc)
+static int GRAPH_RDLOCK
+count_single_write_clusters(BlockDriverState *bs, int nb_clusters,
+ uint64_t *l2_slice, int l2_index, bool new_alloc)
{
BDRVQcow2State *s = bs->opaque;
uint64_t l2_entry = get_l2_entry(s, l2_slice, l2_index);
diff --git a/block/qcow2.c b/block/qcow2.c
index 7ab399ffc5..cf2468858f 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -95,9 +95,10 @@ static int qcow2_probe(const uint8_t *buf, int buf_size, const char *filename)
}
-static int qcow2_crypto_hdr_read_func(QCryptoBlock *block, size_t offset,
- uint8_t *buf, size_t buflen,
- void *opaque, Error **errp)
+static int GRAPH_RDLOCK
+qcow2_crypto_hdr_read_func(QCryptoBlock *block, size_t offset,
+ uint8_t *buf, size_t buflen,
+ void *opaque, Error **errp)
{
BlockDriverState *bs = opaque;
BDRVQcow2State *s = bs->opaque;
@@ -156,7 +157,7 @@ qcow2_crypto_hdr_init_func(QCryptoBlock *block, size_t headerlen, void *opaque,
/* The graph lock must be held when called in coroutine context */
-static int coroutine_mixed_fn
+static int coroutine_mixed_fn GRAPH_RDLOCK
qcow2_crypto_hdr_write_func(QCryptoBlock *block, size_t offset,
const uint8_t *buf, size_t buflen,
void *opaque, Error **errp)
@@ -2029,6 +2030,8 @@ static void qcow2_reopen_commit(BDRVReopenState *state)
{
BDRVQcow2State *s = state->bs->opaque;
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
qcow2_update_options_commit(state->bs, state->opaque);
if (!s->data_file) {
/*
@@ -2064,6 +2067,8 @@ static void qcow2_reopen_abort(BDRVReopenState *state)
{
BDRVQcow2State *s = state->bs->opaque;
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (!s->data_file) {
/*
* If we don't have an external data file, s->data_file was cleared by
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 22/25] vhdx: Take locks for accessing bs->file
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (20 preceding siblings ...)
2023-11-07 18:46 ` [PULL 21/25] qcow2: Take locks for accessing bs->file Kevin Wolf
@ 2023-11-07 18:46 ` Kevin Wolf
2023-11-07 18:46 ` [PULL 23/25] block: Take graph lock for most of .bdrv_open Kevin Wolf
` (3 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
This updates the vhdx code to add GRAPH_RDLOCK annotations for all
places that read bs->file.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-23-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/vhdx.h | 9 ++++++---
block/vhdx-log.c | 40 ++++++++++++++++++++++------------------
block/vhdx.c | 18 +++++++++++-------
3 files changed, 39 insertions(+), 28 deletions(-)
diff --git a/block/vhdx.h b/block/vhdx.h
index 85594a5380..c6dd4d6040 100644
--- a/block/vhdx.h
+++ b/block/vhdx.h
@@ -401,8 +401,9 @@ typedef struct BDRVVHDXState {
void vhdx_guid_generate(MSGUID *guid);
-int vhdx_update_headers(BlockDriverState *bs, BDRVVHDXState *s, bool rw,
- MSGUID *log_guid);
+int GRAPH_RDLOCK
+vhdx_update_headers(BlockDriverState *bs, BDRVVHDXState *s, bool rw,
+ MSGUID *log_guid);
uint32_t vhdx_update_checksum(uint8_t *buf, size_t size, int crc_offset);
uint32_t vhdx_checksum_calc(uint32_t crc, uint8_t *buf, size_t size,
@@ -448,6 +449,8 @@ void vhdx_metadata_header_le_import(VHDXMetadataTableHeader *hdr);
void vhdx_metadata_header_le_export(VHDXMetadataTableHeader *hdr);
void vhdx_metadata_entry_le_import(VHDXMetadataTableEntry *e);
void vhdx_metadata_entry_le_export(VHDXMetadataTableEntry *e);
-int vhdx_user_visible_write(BlockDriverState *bs, BDRVVHDXState *s);
+
+int GRAPH_RDLOCK
+vhdx_user_visible_write(BlockDriverState *bs, BDRVVHDXState *s);
#endif
diff --git a/block/vhdx-log.c b/block/vhdx-log.c
index d8ed651b70..4385a2d4f6 100644
--- a/block/vhdx-log.c
+++ b/block/vhdx-log.c
@@ -55,8 +55,9 @@ static const MSGUID zero_guid = { 0 };
/* Allow peeking at the hdr entry at the beginning of the current
* read index, without advancing the read index */
-static int vhdx_log_peek_hdr(BlockDriverState *bs, VHDXLogEntries *log,
- VHDXLogEntryHeader *hdr)
+static int GRAPH_RDLOCK
+vhdx_log_peek_hdr(BlockDriverState *bs, VHDXLogEntries *log,
+ VHDXLogEntryHeader *hdr)
{
int ret = 0;
uint64_t offset;
@@ -107,7 +108,7 @@ static int vhdx_log_inc_idx(uint32_t idx, uint64_t length)
/* Reset the log to empty */
-static void vhdx_log_reset(BlockDriverState *bs, BDRVVHDXState *s)
+static void GRAPH_RDLOCK vhdx_log_reset(BlockDriverState *bs, BDRVVHDXState *s)
{
MSGUID guid = { 0 };
s->log.read = s->log.write = 0;
@@ -127,9 +128,10 @@ static void vhdx_log_reset(BlockDriverState *bs, BDRVVHDXState *s)
* not modified.
*
* 0 is returned on success, -errno otherwise. */
-static int vhdx_log_read_sectors(BlockDriverState *bs, VHDXLogEntries *log,
- uint32_t *sectors_read, void *buffer,
- uint32_t num_sectors, bool peek)
+static int GRAPH_RDLOCK
+vhdx_log_read_sectors(BlockDriverState *bs, VHDXLogEntries *log,
+ uint32_t *sectors_read, void *buffer,
+ uint32_t num_sectors, bool peek)
{
int ret = 0;
uint64_t offset;
@@ -333,9 +335,9 @@ static int vhdx_compute_desc_sectors(uint32_t desc_cnt)
* will allocate all the space for buffer, which must be NULL when
* passed into this function. Each descriptor will also be validated,
* and error returned if any are invalid. */
-static int vhdx_log_read_desc(BlockDriverState *bs, BDRVVHDXState *s,
- VHDXLogEntries *log, VHDXLogDescEntries **buffer,
- bool convert_endian)
+static int GRAPH_RDLOCK
+vhdx_log_read_desc(BlockDriverState *bs, BDRVVHDXState *s, VHDXLogEntries *log,
+ VHDXLogDescEntries **buffer, bool convert_endian)
{
int ret = 0;
uint32_t desc_sectors;
@@ -412,8 +414,9 @@ exit:
* For a zero descriptor, it may describe multiple sectors to fill with zeroes.
* In this case, it should be noted that zeroes are written to disk, and the
* image file is not extended as a sparse file. */
-static int vhdx_log_flush_desc(BlockDriverState *bs, VHDXLogDescriptor *desc,
- VHDXLogDataSector *data)
+static int GRAPH_RDLOCK
+vhdx_log_flush_desc(BlockDriverState *bs, VHDXLogDescriptor *desc,
+ VHDXLogDataSector *data)
{
int ret = 0;
uint64_t seq, file_offset;
@@ -484,8 +487,8 @@ exit:
* file, and then set the log to 'empty' status once complete.
*
* The log entries should be validate prior to flushing */
-static int vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s,
- VHDXLogSequence *logs)
+static int GRAPH_RDLOCK
+vhdx_log_flush(BlockDriverState *bs, BDRVVHDXState *s, VHDXLogSequence *logs)
{
int ret = 0;
int i;
@@ -584,9 +587,10 @@ exit:
return ret;
}
-static int vhdx_validate_log_entry(BlockDriverState *bs, BDRVVHDXState *s,
- VHDXLogEntries *log, uint64_t seq,
- bool *valid, VHDXLogEntryHeader *entry)
+static int GRAPH_RDLOCK
+vhdx_validate_log_entry(BlockDriverState *bs, BDRVVHDXState *s,
+ VHDXLogEntries *log, uint64_t seq,
+ bool *valid, VHDXLogEntryHeader *entry)
{
int ret = 0;
VHDXLogEntryHeader hdr;
@@ -663,8 +667,8 @@ free_and_exit:
/* Search through the log circular buffer, and find the valid, active
* log sequence, if any exists
* */
-static int vhdx_log_search(BlockDriverState *bs, BDRVVHDXState *s,
- VHDXLogSequence *logs)
+static int GRAPH_RDLOCK
+vhdx_log_search(BlockDriverState *bs, BDRVVHDXState *s, VHDXLogSequence *logs)
{
int ret = 0;
uint32_t tail;
diff --git a/block/vhdx.c b/block/vhdx.c
index ac4d52e8c6..5aa1a13506 100644
--- a/block/vhdx.c
+++ b/block/vhdx.c
@@ -353,8 +353,9 @@ exit:
*
* - non-current header is updated with largest sequence number
*/
-static int vhdx_update_header(BlockDriverState *bs, BDRVVHDXState *s,
- bool generate_data_write_guid, MSGUID *log_guid)
+static int GRAPH_RDLOCK
+vhdx_update_header(BlockDriverState *bs, BDRVVHDXState *s,
+ bool generate_data_write_guid, MSGUID *log_guid)
{
int ret = 0;
int hdr_idx = 0;
@@ -416,8 +417,8 @@ int vhdx_update_headers(BlockDriverState *bs, BDRVVHDXState *s,
}
/* opens the specified header block from the VHDX file header section */
-static void vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s,
- Error **errp)
+static void GRAPH_RDLOCK
+vhdx_parse_header(BlockDriverState *bs, BDRVVHDXState *s, Error **errp)
{
int ret;
VHDXHeader *header1;
@@ -517,7 +518,8 @@ exit:
}
-static int vhdx_open_region_tables(BlockDriverState *bs, BDRVVHDXState *s)
+static int GRAPH_RDLOCK
+vhdx_open_region_tables(BlockDriverState *bs, BDRVVHDXState *s)
{
int ret = 0;
uint8_t *buffer;
@@ -634,7 +636,8 @@ fail:
* Also, if the File Parameters indicate this is a differencing file,
* we must also look for the Parent Locator metadata item.
*/
-static int vhdx_parse_metadata(BlockDriverState *bs, BDRVVHDXState *s)
+static int GRAPH_RDLOCK
+vhdx_parse_metadata(BlockDriverState *bs, BDRVVHDXState *s)
{
int ret = 0;
uint8_t *buffer;
@@ -885,7 +888,8 @@ static void vhdx_calc_bat_entries(BDRVVHDXState *s)
}
-static int vhdx_check_bat_entries(BlockDriverState *bs, int *errcnt)
+static int coroutine_mixed_fn GRAPH_RDLOCK
+vhdx_check_bat_entries(BlockDriverState *bs, int *errcnt)
{
BDRVVHDXState *s = bs->opaque;
int64_t image_file_size = bdrv_getlength(bs->file->bs);
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 23/25] block: Take graph lock for most of .bdrv_open
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (21 preceding siblings ...)
2023-11-07 18:46 ` [PULL 22/25] vhdx: " Kevin Wolf
@ 2023-11-07 18:46 ` Kevin Wolf
2023-11-07 18:46 ` [PULL 24/25] block: Protect bs->file with graph_lock Kevin Wolf
` (2 subsequent siblings)
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
Most implementations of .bdrv_open first open their file child (which is
an operation that internally takes the write lock and therefore we
shouldn't hold the graph lock while calling it), and afterwards many
operations that require holding the graph lock, e.g. for accessing
bs->file.
This changes block drivers that follow this pattern to take the graph
lock after opening the child node.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Message-ID: <20231027155333.420094-24-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/blkdebug.c | 16 ++++++++++------
block/bochs.c | 4 ++++
block/cloop.c | 4 ++++
block/copy-before-write.c | 2 ++
block/copy-on-read.c | 4 ++--
block/crypto.c | 4 ++++
block/dmg.c | 5 +++++
block/filter-compress.c | 2 ++
block/parallels.c | 4 ++--
block/preallocate.c | 4 ++++
block/qcow.c | 11 +++++++----
block/raw-format.c | 6 ++++--
block/snapshot-access.c | 3 +++
block/throttle.c | 3 +++
block/vdi.c | 4 ++--
block/vpc.c | 4 ++--
16 files changed, 60 insertions(+), 20 deletions(-)
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 230efa9e8d..9da8c9eddc 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -508,6 +508,8 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
goto out;
}
+ bdrv_graph_rdlock_main_loop();
+
bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
(BDRV_REQ_FUA & bs->file->bs->supported_write_flags);
bs->supported_zero_flags = BDRV_REQ_WRITE_UNCHANGED |
@@ -520,7 +522,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
if (s->align && (s->align >= INT_MAX || !is_power_of_2(s->align))) {
error_setg(errp, "Cannot meet constraints with align %" PRIu64,
s->align);
- goto out;
+ goto out_rdlock;
}
align = MAX(s->align, bs->file->bs->bl.request_alignment);
@@ -530,7 +532,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
!QEMU_IS_ALIGNED(s->max_transfer, align))) {
error_setg(errp, "Cannot meet constraints with max-transfer %" PRIu64,
s->max_transfer);
- goto out;
+ goto out_rdlock;
}
s->opt_write_zero = qemu_opt_get_size(opts, "opt-write-zero", 0);
@@ -539,7 +541,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
!QEMU_IS_ALIGNED(s->opt_write_zero, align))) {
error_setg(errp, "Cannot meet constraints with opt-write-zero %" PRIu64,
s->opt_write_zero);
- goto out;
+ goto out_rdlock;
}
s->max_write_zero = qemu_opt_get_size(opts, "max-write-zero", 0);
@@ -549,7 +551,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
MAX(s->opt_write_zero, align)))) {
error_setg(errp, "Cannot meet constraints with max-write-zero %" PRIu64,
s->max_write_zero);
- goto out;
+ goto out_rdlock;
}
s->opt_discard = qemu_opt_get_size(opts, "opt-discard", 0);
@@ -558,7 +560,7 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
!QEMU_IS_ALIGNED(s->opt_discard, align))) {
error_setg(errp, "Cannot meet constraints with opt-discard %" PRIu64,
s->opt_discard);
- goto out;
+ goto out_rdlock;
}
s->max_discard = qemu_opt_get_size(opts, "max-discard", 0);
@@ -568,12 +570,14 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
MAX(s->opt_discard, align)))) {
error_setg(errp, "Cannot meet constraints with max-discard %" PRIu64,
s->max_discard);
- goto out;
+ goto out_rdlock;
}
bdrv_debug_event(bs, BLKDBG_NONE);
ret = 0;
+out_rdlock:
+ bdrv_graph_rdunlock_main_loop();
out:
if (ret < 0) {
qemu_mutex_destroy(&s->lock);
diff --git a/block/bochs.c b/block/bochs.c
index 8c659fa9b9..b099fb52fe 100644
--- a/block/bochs.c
+++ b/block/bochs.c
@@ -105,6 +105,8 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
struct bochs_header bochs;
int ret;
+ GLOBAL_STATE_CODE();
+
/* No write support yet */
bdrv_graph_rdlock_main_loop();
ret = bdrv_apply_auto_read_only(bs, NULL, errp);
@@ -118,6 +120,8 @@ static int bochs_open(BlockDriverState *bs, QDict *options, int flags,
return ret;
}
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
ret = bdrv_pread(bs->file, 0, sizeof(bochs), &bochs, 0);
if (ret < 0) {
return ret;
diff --git a/block/cloop.c b/block/cloop.c
index 773d7918be..443af1444e 100644
--- a/block/cloop.c
+++ b/block/cloop.c
@@ -67,6 +67,8 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
uint32_t offsets_size, max_compressed_block_size = 1, i;
int ret;
+ GLOBAL_STATE_CODE();
+
bdrv_graph_rdlock_main_loop();
ret = bdrv_apply_auto_read_only(bs, NULL, errp);
bdrv_graph_rdunlock_main_loop();
@@ -79,6 +81,8 @@ static int cloop_open(BlockDriverState *bs, QDict *options, int flags,
return ret;
}
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
/* read header */
ret = bdrv_pread(bs->file, 128, 4, &s->block_size, 0);
if (ret < 0) {
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index 8193d3a4cd..ad3b73cc4a 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -433,6 +433,8 @@ static int cbw_open(BlockDriverState *bs, QDict *options, int flags,
return -EINVAL;
}
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
ctx = bdrv_get_aio_context(bs);
aio_context_acquire(ctx);
diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 6f245b629a..c36f253d16 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c
@@ -51,6 +51,8 @@ cor_open(BlockDriverState *bs, QDict *options, int flags, Error **errp)
return ret;
}
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
bs->supported_read_flags = BDRV_REQ_PREFETCH;
bs->supported_write_flags = BDRV_REQ_WRITE_UNCHANGED |
@@ -61,8 +63,6 @@ cor_open(BlockDriverState *bs, QDict *options, int flags, Error **errp)
bs->file->bs->supported_zero_flags);
if (bottom_node) {
- GRAPH_RDLOCK_GUARD_MAINLOOP();
-
bottom_bs = bdrv_find_node(bottom_node);
if (!bottom_bs) {
error_setg(errp, "Bottom node '%s' not found", bottom_node);
diff --git a/block/crypto.c b/block/crypto.c
index b3f0233d53..6ee0cac4b6 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -263,11 +263,15 @@ static int block_crypto_open_generic(QCryptoBlockFormat format,
unsigned int cflags = 0;
QDict *cryptoopts = NULL;
+ GLOBAL_STATE_CODE();
+
ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
if (ret < 0) {
return ret;
}
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
bs->supported_write_flags = BDRV_REQ_FUA &
bs->file->bs->supported_write_flags;
diff --git a/block/dmg.c b/block/dmg.c
index 38ee72bbe5..853ad36a00 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -452,6 +452,8 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
int64_t offset;
int ret;
+ GLOBAL_STATE_CODE();
+
bdrv_graph_rdlock_main_loop();
ret = bdrv_apply_auto_read_only(bs, NULL, errp);
bdrv_graph_rdunlock_main_loop();
@@ -463,6 +465,9 @@ static int dmg_open(BlockDriverState *bs, QDict *options, int flags,
if (ret < 0) {
return ret;
}
+
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
/*
* NB: if uncompress submodules are absent,
* ie block_module_load return value == 0, the function pointers
diff --git a/block/filter-compress.c b/block/filter-compress.c
index e3fc82f322..9b68a2be64 100644
--- a/block/filter-compress.c
+++ b/block/filter-compress.c
@@ -36,6 +36,8 @@ static int compress_open(BlockDriverState *bs, QDict *options, int flags,
return ret;
}
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (!bs->file->bs->drv || !block_driver_can_compress(bs->file->bs->drv)) {
error_setg(errp,
"Compression is not supported for underlying format: %s",
diff --git a/block/parallels.c b/block/parallels.c
index 9b9bd1f993..a04f836cc2 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -1255,6 +1255,8 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
return ret;
}
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
file_nb_sectors = bdrv_nb_sectors(bs->file->bs);
if (file_nb_sectors < 0) {
return -EINVAL;
@@ -1359,11 +1361,9 @@ static int parallels_open(BlockDriverState *bs, QDict *options, int flags,
bitmap_new(DIV_ROUND_UP(s->header_size, s->bat_dirty_block));
/* Disable migration until bdrv_activate method is added */
- bdrv_graph_rdlock_main_loop();
error_setg(&s->migration_blocker, "The Parallels format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
- bdrv_graph_rdunlock_main_loop();
ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
if (ret < 0) {
diff --git a/block/preallocate.c b/block/preallocate.c
index 4e0c891ab2..f302d17d3f 100644
--- a/block/preallocate.c
+++ b/block/preallocate.c
@@ -143,6 +143,8 @@ static int preallocate_open(BlockDriverState *bs, QDict *options, int flags,
BDRVPreallocateState *s = bs->opaque;
int ret;
+ GLOBAL_STATE_CODE();
+
/*
* s->data_end and friends should be initialized on permission update.
* For this to work, mark them invalid.
@@ -155,6 +157,8 @@ static int preallocate_open(BlockDriverState *bs, QDict *options, int flags,
return ret;
}
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (!preallocate_absorb_opts(&s->opts, options, bs->file->bs, errp)) {
return -EINVAL;
}
diff --git a/block/qcow.c b/block/qcow.c
index 1e2835f1f4..c6d0e15f1e 100644
--- a/block/qcow.c
+++ b/block/qcow.c
@@ -124,9 +124,11 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
if (ret < 0) {
- goto fail;
+ goto fail_unlocked;
}
+ bdrv_graph_rdlock_main_loop();
+
ret = bdrv_pread(bs->file, 0, sizeof(header), &header, 0);
if (ret < 0) {
goto fail;
@@ -301,11 +303,9 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
}
/* Disable migration when qcow images are used */
- bdrv_graph_rdlock_main_loop();
error_setg(&s->migration_blocker, "The qcow format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
- bdrv_graph_rdunlock_main_loop();
ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
if (ret < 0) {
@@ -315,9 +315,12 @@ static int qcow_open(BlockDriverState *bs, QDict *options, int flags,
qobject_unref(encryptopts);
qapi_free_QCryptoBlockOpenOptions(crypto_opts);
qemu_co_mutex_init(&s->lock);
+ bdrv_graph_rdunlock_main_loop();
return 0;
- fail:
+fail:
+ bdrv_graph_rdunlock_main_loop();
+fail_unlocked:
g_free(s->l1_table);
qemu_vfree(s->l2_cache);
g_free(s->cluster_cache);
diff --git a/block/raw-format.c b/block/raw-format.c
index 2640d54801..66741be954 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -473,6 +473,8 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
BdrvChildRole file_role;
int ret;
+ GLOBAL_STATE_CODE();
+
ret = raw_read_options(options, &offset, &has_size, &size, errp);
if (ret < 0) {
return ret;
@@ -490,6 +492,8 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
file_role, false, errp);
+
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
if (!bs->file) {
return -EINVAL;
}
@@ -504,9 +508,7 @@ static int raw_open(BlockDriverState *bs, QDict *options, int flags,
BDRV_REQ_ZERO_WRITE;
if (bs->probed && !bdrv_is_read_only(bs)) {
- bdrv_graph_rdlock_main_loop();
bdrv_refresh_filename(bs->file->bs);
- bdrv_graph_rdunlock_main_loop();
fprintf(stderr,
"WARNING: Image format was not specified for '%s' and probing "
"guessed raw.\n"
diff --git a/block/snapshot-access.c b/block/snapshot-access.c
index 7c45739eb1..84d0d13f86 100644
--- a/block/snapshot-access.c
+++ b/block/snapshot-access.c
@@ -85,6 +85,9 @@ static int snapshot_access_open(BlockDriverState *bs, QDict *options, int flags,
bdrv_open_child(NULL, options, "file", bs, &child_of_bds,
BDRV_CHILD_DATA | BDRV_CHILD_PRIMARY,
false, errp);
+
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (!bs->file) {
return -EINVAL;
}
diff --git a/block/throttle.c b/block/throttle.c
index 1098a4ae9a..97972d1f15 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -84,6 +84,9 @@ static int throttle_open(BlockDriverState *bs, QDict *options,
if (ret < 0) {
return ret;
}
+
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
bs->supported_write_flags = bs->file->bs->supported_write_flags |
BDRV_REQ_WRITE_UNCHANGED;
bs->supported_zero_flags = bs->file->bs->supported_zero_flags |
diff --git a/block/vdi.c b/block/vdi.c
index 84150180ec..3b57becb9f 100644
--- a/block/vdi.c
+++ b/block/vdi.c
@@ -383,6 +383,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
return ret;
}
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
logout("\n");
ret = bdrv_pread(bs->file, 0, sizeof(header), &header, 0);
@@ -492,11 +494,9 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
}
/* Disable migration when vdi images are used */
- bdrv_graph_rdlock_main_loop();
error_setg(&s->migration_blocker, "The vdi format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
- bdrv_graph_rdunlock_main_loop();
ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
if (ret < 0) {
diff --git a/block/vpc.c b/block/vpc.c
index 483775103c..d95a204612 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -238,6 +238,8 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
return ret;
}
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
opts = qemu_opts_create(&vpc_runtime_opts, NULL, 0, &error_abort);
if (!qemu_opts_absorb_qdict(opts, options, errp)) {
ret = -EINVAL;
@@ -446,11 +448,9 @@ static int vpc_open(BlockDriverState *bs, QDict *options, int flags,
}
/* Disable migration when VHD images are used */
- bdrv_graph_rdlock_main_loop();
error_setg(&s->migration_blocker, "The vpc format used by node '%s' "
"does not support live migration",
bdrv_get_device_or_node_name(bs));
- bdrv_graph_rdunlock_main_loop();
ret = migrate_add_blocker_normal(&s->migration_blocker, errp);
if (ret < 0) {
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 24/25] block: Protect bs->file with graph_lock
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (22 preceding siblings ...)
2023-11-07 18:46 ` [PULL 23/25] block: Take graph lock for most of .bdrv_open Kevin Wolf
@ 2023-11-07 18:46 ` Kevin Wolf
2023-11-07 18:46 ` [PULL 25/25] hw/ide/ahci: trigger either error IRQ or regular IRQ, not both Kevin Wolf
2023-11-08 1:44 ` [PULL 00/25] Block layer patches Stefan Hajnoczi
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
Almost all functions that access bs->file 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 <kwolf@redhat.com>
Message-ID: <20231027155333.420094-25-kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/parallels.h | 5 +++--
block/qed.h | 2 +-
include/block/block_int-common.h | 2 +-
block.c | 11 ++++++++---
block/blkreplay.c | 8 +++++++-
block/copy-before-write.c | 2 +-
block/crypto.c | 6 ++++++
block/dmg.c | 16 ++++++++++------
block/parallels-ext.c | 21 ++++++++++-----------
block/parallels.c | 6 ++++--
block/preallocate.c | 19 ++++++++++++++-----
block/qed.c | 12 ++++++++++--
block/raw-format.c | 9 ++++++---
block/replication.c | 5 ++++-
block/vmdk.c | 14 ++++++++++++--
15 files changed, 97 insertions(+), 41 deletions(-)
diff --git a/block/parallels.h b/block/parallels.h
index 6b199443cf..423b2ad727 100644
--- a/block/parallels.h
+++ b/block/parallels.h
@@ -90,7 +90,8 @@ typedef struct BDRVParallelsState {
Error *migration_blocker;
} BDRVParallelsState;
-int parallels_read_format_extension(BlockDriverState *bs,
- int64_t ext_off, Error **errp);
+int GRAPH_RDLOCK
+parallels_read_format_extension(BlockDriverState *bs, int64_t ext_off,
+ Error **errp);
#endif
diff --git a/block/qed.h b/block/qed.h
index 988654cb86..26d4bf038c 100644
--- a/block/qed.h
+++ b/block/qed.h
@@ -185,7 +185,7 @@ enum {
/**
* Header functions
*/
-int qed_write_header_sync(BDRVQEDState *s);
+int GRAPH_RDLOCK qed_write_header_sync(BDRVQEDState *s);
/**
* L2 cache functions
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 63bc523d7c..4e31d161c5 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1181,7 +1181,7 @@ struct BlockDriverState {
*/
QLIST_HEAD(, BdrvChild GRAPH_RDLOCK_PTR) children;
BdrvChild * GRAPH_RDLOCK_PTR backing;
- BdrvChild *file;
+ BdrvChild * GRAPH_RDLOCK_PTR file;
QLIST_HEAD(, BdrvChild GRAPH_RDLOCK_PTR) parents;
diff --git a/block.c b/block.c
index 4910b95d7d..eac105a504 100644
--- a/block.c
+++ b/block.c
@@ -1707,12 +1707,14 @@ bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv, const char *node_name,
return 0;
open_failed:
bs->drv = NULL;
+
+ bdrv_graph_wrlock(NULL);
if (bs->file != NULL) {
- bdrv_graph_wrlock(NULL);
bdrv_unref_child(bs, bs->file);
- bdrv_graph_wrunlock();
assert(!bs->file);
}
+ bdrv_graph_wrunlock();
+
g_free(bs->opaque);
bs->opaque = NULL;
return ret;
@@ -1854,9 +1856,12 @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,
Error *local_err = NULL;
bool ro;
+ GLOBAL_STATE_CODE();
+
+ bdrv_graph_rdlock_main_loop();
assert(bs->file == NULL);
assert(options != NULL && bs->options != options);
- GLOBAL_STATE_CODE();
+ bdrv_graph_rdunlock_main_loop();
opts = qemu_opts_create(&bdrv_runtime_opts, NULL, 0, &error_abort);
if (!qemu_opts_absorb_qdict(opts, options, errp)) {
diff --git a/block/blkreplay.c b/block/blkreplay.c
index 04f53eea41..792d980aa9 100644
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -130,7 +130,13 @@ static int coroutine_fn GRAPH_RDLOCK blkreplay_co_flush(BlockDriverState *bs)
static int blkreplay_snapshot_goto(BlockDriverState *bs,
const char *snapshot_id)
{
- return bdrv_snapshot_goto(bs->file->bs, snapshot_id, NULL);
+ BlockDriverState *file_bs;
+
+ bdrv_graph_rdlock_main_loop();
+ file_bs = bs->file->bs;
+ bdrv_graph_rdunlock_main_loop();
+
+ return bdrv_snapshot_goto(file_bs, snapshot_id, NULL);
}
static BlockDriver bdrv_blkreplay = {
diff --git a/block/copy-before-write.c b/block/copy-before-write.c
index ad3b73cc4a..13972879b1 100644
--- a/block/copy-before-write.c
+++ b/block/copy-before-write.c
@@ -203,7 +203,7 @@ static int coroutine_fn GRAPH_RDLOCK cbw_co_flush(BlockDriverState *bs)
* It's guaranteed that guest writes will not interact in the region until
* cbw_snapshot_read_unlock() called.
*/
-static coroutine_fn BlockReq *
+static BlockReq * coroutine_fn GRAPH_RDLOCK
cbw_snapshot_read_lock(BlockDriverState *bs, int64_t offset, int64_t bytes,
int64_t *pnum, BdrvChild **file)
{
diff --git a/block/crypto.c b/block/crypto.c
index 6ee0cac4b6..921933a5e5 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -65,6 +65,9 @@ static int block_crypto_read_func(QCryptoBlock *block,
BlockDriverState *bs = opaque;
ssize_t ret;
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
ret = bdrv_pread(bs->file, offset, buflen, buf, 0);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not read encryption header");
@@ -83,6 +86,9 @@ static int block_crypto_write_func(QCryptoBlock *block,
BlockDriverState *bs = opaque;
ssize_t ret;
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
ret = bdrv_pwrite(bs->file, offset, buflen, buf, 0);
if (ret < 0) {
error_setg_errno(errp, -ret, "Could not write encryption header");
diff --git a/block/dmg.c b/block/dmg.c
index 853ad36a00..33dcb3a349 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -70,7 +70,8 @@ static int dmg_probe(const uint8_t *buf, int buf_size, const char *filename)
return 0;
}
-static int read_uint64(BlockDriverState *bs, int64_t offset, uint64_t *result)
+static int GRAPH_RDLOCK
+read_uint64(BlockDriverState *bs, int64_t offset, uint64_t *result)
{
uint64_t buffer;
int ret;
@@ -84,7 +85,8 @@ static int read_uint64(BlockDriverState *bs, int64_t offset, uint64_t *result)
return 0;
}
-static int read_uint32(BlockDriverState *bs, int64_t offset, uint32_t *result)
+static int GRAPH_RDLOCK
+read_uint32(BlockDriverState *bs, int64_t offset, uint32_t *result)
{
uint32_t buffer;
int ret;
@@ -321,8 +323,9 @@ fail:
return ret;
}
-static int dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
- uint64_t info_begin, uint64_t info_length)
+static int GRAPH_RDLOCK
+dmg_read_resource_fork(BlockDriverState *bs, DmgHeaderState *ds,
+ uint64_t info_begin, uint64_t info_length)
{
BDRVDMGState *s = bs->opaque;
int ret;
@@ -388,8 +391,9 @@ fail:
return ret;
}
-static int dmg_read_plist_xml(BlockDriverState *bs, DmgHeaderState *ds,
- uint64_t info_begin, uint64_t info_length)
+static int GRAPH_RDLOCK
+dmg_read_plist_xml(BlockDriverState *bs, DmgHeaderState *ds,
+ uint64_t info_begin, uint64_t info_length)
{
BDRVDMGState *s = bs->opaque;
int ret;
diff --git a/block/parallels-ext.c b/block/parallels-ext.c
index 4d8ecf5047..b4e14c88f2 100644
--- a/block/parallels-ext.c
+++ b/block/parallels-ext.c
@@ -59,11 +59,10 @@ typedef struct ParallelsDirtyBitmapFeature {
} QEMU_PACKED ParallelsDirtyBitmapFeature;
/* Given L1 table read bitmap data from the image and populate @bitmap */
-static int parallels_load_bitmap_data(BlockDriverState *bs,
- const uint64_t *l1_table,
- uint32_t l1_size,
- BdrvDirtyBitmap *bitmap,
- Error **errp)
+static int GRAPH_RDLOCK
+parallels_load_bitmap_data(BlockDriverState *bs, const uint64_t *l1_table,
+ uint32_t l1_size, BdrvDirtyBitmap *bitmap,
+ Error **errp)
{
BDRVParallelsState *s = bs->opaque;
int ret = 0;
@@ -120,10 +119,9 @@ finish:
* @data buffer (of @data_size size) is the Dirty bitmaps feature which
* consists of ParallelsDirtyBitmapFeature followed by L1 table.
*/
-static BdrvDirtyBitmap *parallels_load_bitmap(BlockDriverState *bs,
- uint8_t *data,
- size_t data_size,
- Error **errp)
+static BdrvDirtyBitmap * GRAPH_RDLOCK
+parallels_load_bitmap(BlockDriverState *bs, uint8_t *data, size_t data_size,
+ Error **errp)
{
int ret;
ParallelsDirtyBitmapFeature bf;
@@ -183,8 +181,9 @@ static BdrvDirtyBitmap *parallels_load_bitmap(BlockDriverState *bs,
return bitmap;
}
-static int parallels_parse_format_extension(BlockDriverState *bs,
- uint8_t *ext_cluster, Error **errp)
+static int GRAPH_RDLOCK
+parallels_parse_format_extension(BlockDriverState *bs, uint8_t *ext_cluster,
+ Error **errp)
{
BDRVParallelsState *s = bs->opaque;
int ret;
diff --git a/block/parallels.c b/block/parallels.c
index a04f836cc2..9205a0864f 100644
--- a/block/parallels.c
+++ b/block/parallels.c
@@ -200,7 +200,7 @@ static int mark_used(BlockDriverState *bs, unsigned long *bitmap,
* bitmap anyway, as much as we can. This information will be used for
* error resolution.
*/
-static int parallels_fill_used_bitmap(BlockDriverState *bs)
+static int GRAPH_RDLOCK parallels_fill_used_bitmap(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
int64_t payload_bytes;
@@ -1185,7 +1185,7 @@ static int parallels_probe(const uint8_t *buf, int buf_size,
return 0;
}
-static int parallels_update_header(BlockDriverState *bs)
+static int GRAPH_RDLOCK parallels_update_header(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
unsigned size = MAX(bdrv_opt_mem_align(bs->file->bs),
@@ -1428,6 +1428,8 @@ static void parallels_close(BlockDriverState *bs)
{
BDRVParallelsState *s = bs->opaque;
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if ((bs->open_flags & BDRV_O_RDWR) && !(bs->open_flags & BDRV_O_INACTIVE)) {
s->header->inuse = 0;
parallels_update_header(bs);
diff --git a/block/preallocate.c b/block/preallocate.c
index f302d17d3f..d215bc5d6d 100644
--- a/block/preallocate.c
+++ b/block/preallocate.c
@@ -173,7 +173,8 @@ static int preallocate_open(BlockDriverState *bs, QDict *options, int flags,
return 0;
}
-static int preallocate_truncate_to_real_size(BlockDriverState *bs, Error **errp)
+static int GRAPH_RDLOCK
+preallocate_truncate_to_real_size(BlockDriverState *bs, Error **errp)
{
BDRVPreallocateState *s = bs->opaque;
int ret;
@@ -204,6 +205,9 @@ static void preallocate_close(BlockDriverState *bs)
{
BDRVPreallocateState *s = bs->opaque;
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
qemu_bh_cancel(s->drop_resize_bh);
qemu_bh_delete(s->drop_resize_bh);
@@ -227,6 +231,9 @@ static int preallocate_reopen_prepare(BDRVReopenState *reopen_state,
PreallocateOpts *opts = g_new0(PreallocateOpts, 1);
int ret;
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
if (!preallocate_absorb_opts(opts, reopen_state->options,
reopen_state->bs->file->bs, errp)) {
g_free(opts);
@@ -287,7 +294,7 @@ static bool can_write_resize(uint64_t perm)
return (perm & BLK_PERM_WRITE) && (perm & BLK_PERM_RESIZE);
}
-static bool has_prealloc_perms(BlockDriverState *bs)
+static bool GRAPH_RDLOCK has_prealloc_perms(BlockDriverState *bs)
{
BDRVPreallocateState *s = bs->opaque;
@@ -503,7 +510,8 @@ preallocate_co_getlength(BlockDriverState *bs)
return ret;
}
-static int preallocate_drop_resize(BlockDriverState *bs, Error **errp)
+static int GRAPH_RDLOCK
+preallocate_drop_resize(BlockDriverState *bs, Error **errp)
{
BDRVPreallocateState *s = bs->opaque;
int ret;
@@ -529,15 +537,16 @@ static int preallocate_drop_resize(BlockDriverState *bs, Error **errp)
*/
s->data_end = s->file_end = s->zero_start = -EINVAL;
- bdrv_graph_rdlock_main_loop();
bdrv_child_refresh_perms(bs, bs->file, NULL);
- bdrv_graph_rdunlock_main_loop();
return 0;
}
static void preallocate_drop_resize_bh(void *opaque)
{
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
/*
* In case of errors, we'll simply keep the exclusive lock on the image
* indefinitely.
diff --git a/block/qed.c b/block/qed.c
index f4c1628a81..bc2f0a61c0 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -612,7 +612,7 @@ static int bdrv_qed_reopen_prepare(BDRVReopenState *state,
return 0;
}
-static void bdrv_qed_close(BlockDriverState *bs)
+static void GRAPH_RDLOCK bdrv_qed_do_close(BlockDriverState *bs)
{
BDRVQEDState *s = bs->opaque;
@@ -631,6 +631,14 @@ static void bdrv_qed_close(BlockDriverState *bs)
qemu_vfree(s->l1_table);
}
+static void GRAPH_UNLOCKED bdrv_qed_close(BlockDriverState *bs)
+{
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
+ bdrv_qed_do_close(bs);
+}
+
static int coroutine_fn GRAPH_UNLOCKED
bdrv_qed_co_create(BlockdevCreateOptions *opts, Error **errp)
{
@@ -1574,7 +1582,7 @@ bdrv_qed_co_invalidate_cache(BlockDriverState *bs, Error **errp)
BDRVQEDState *s = bs->opaque;
int ret;
- bdrv_qed_close(bs);
+ bdrv_qed_do_close(bs);
bdrv_qed_init_state(bs);
qemu_co_mutex_lock(&s->table_lock);
diff --git a/block/raw-format.c b/block/raw-format.c
index 66741be954..1111dffd54 100644
--- a/block/raw-format.c
+++ b/block/raw-format.c
@@ -95,9 +95,9 @@ end:
return ret;
}
-static int raw_apply_options(BlockDriverState *bs, BDRVRawState *s,
- uint64_t offset, bool has_size, uint64_t size,
- Error **errp)
+static int GRAPH_RDLOCK
+raw_apply_options(BlockDriverState *bs, BDRVRawState *s, uint64_t offset,
+ bool has_size, uint64_t size, Error **errp)
{
int64_t real_size = 0;
@@ -145,6 +145,9 @@ static int raw_reopen_prepare(BDRVReopenState *reopen_state,
uint64_t offset, size;
int ret;
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
assert(reopen_state != NULL);
assert(reopen_state->bs != NULL);
diff --git a/block/replication.c b/block/replication.c
index 49ecc608b2..43e259444b 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -311,7 +311,7 @@ static void GRAPH_UNLOCKED
secondary_do_checkpoint(BlockDriverState *bs, Error **errp)
{
BDRVReplicationState *s = bs->opaque;
- BdrvChild *active_disk = bs->file;
+ BdrvChild *active_disk;
Error *local_err = NULL;
int ret;
@@ -328,6 +328,7 @@ secondary_do_checkpoint(BlockDriverState *bs, Error **errp)
return;
}
+ active_disk = bs->file;
if (!active_disk->bs->drv) {
error_setg(errp, "Active disk %s is ejected",
active_disk->bs->node_name);
@@ -755,11 +756,13 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
return;
}
+ bdrv_graph_rdlock_main_loop();
s->stage = BLOCK_REPLICATION_FAILOVER;
s->commit_job = commit_active_start(
NULL, bs->file->bs, s->secondary_disk->bs,
JOB_INTERNAL, 0, BLOCKDEV_ON_ERROR_REPORT,
NULL, replication_done, bs, true, errp);
+ bdrv_graph_rdunlock_main_loop();
break;
default:
aio_context_release(aio_context);
diff --git a/block/vmdk.c b/block/vmdk.c
index 5c789bb65b..dda783f06b 100644
--- a/block/vmdk.c
+++ b/block/vmdk.c
@@ -300,7 +300,8 @@ static void vmdk_free_last_extent(BlockDriverState *bs)
}
/* Return -ve errno, or 0 on success and write CID into *pcid. */
-static int vmdk_read_cid(BlockDriverState *bs, int parent, uint32_t *pcid)
+static int GRAPH_RDLOCK
+vmdk_read_cid(BlockDriverState *bs, int parent, uint32_t *pcid)
{
char *desc;
uint32_t cid;
@@ -415,6 +416,9 @@ static int vmdk_reopen_prepare(BDRVReopenState *state,
BDRVVmdkReopenState *rs;
int i;
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
assert(state != NULL);
assert(state->bs != NULL);
assert(state->opaque == NULL);
@@ -451,6 +455,9 @@ static void vmdk_reopen_commit(BDRVReopenState *state)
BDRVVmdkReopenState *rs = state->opaque;
int i;
+ GLOBAL_STATE_CODE();
+ GRAPH_RDLOCK_GUARD_MAINLOOP();
+
for (i = 0; i < s->num_extents; i++) {
if (rs->extents_using_bs_file[i]) {
s->extents[i].file = state->bs->file;
@@ -465,7 +472,7 @@ static void vmdk_reopen_abort(BDRVReopenState *state)
vmdk_reopen_clean(state);
}
-static int vmdk_parent_open(BlockDriverState *bs)
+static int GRAPH_RDLOCK vmdk_parent_open(BlockDriverState *bs)
{
char *p_name;
char *desc;
@@ -2547,7 +2554,10 @@ vmdk_co_do_create(int64_t size,
ret = -EINVAL;
goto exit;
}
+
+ bdrv_graph_co_rdlock();
ret = vmdk_read_cid(blk_bs(backing), 0, &parent_cid);
+ bdrv_graph_co_rdunlock();
blk_co_unref(backing);
if (ret) {
error_setg(errp, "Failed to read parent CID");
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* [PULL 25/25] hw/ide/ahci: trigger either error IRQ or regular IRQ, not both
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (23 preceding siblings ...)
2023-11-07 18:46 ` [PULL 24/25] block: Protect bs->file with graph_lock Kevin Wolf
@ 2023-11-07 18:46 ` Kevin Wolf
2023-11-08 1:44 ` [PULL 00/25] Block layer patches Stefan Hajnoczi
25 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2023-11-07 18:46 UTC (permalink / raw)
To: qemu-devel; +Cc: kwolf
From: Niklas Cassel <niklas.cassel@wdc.com>
According to AHCI 1.3.1, 5.3.8.1 RegFIS:Entry, if ERR_STAT is set,
we jump to state ERR:FatalTaskfile, which will raise a TFES IRQ
unconditionally, regardless if the I bit is set in the FIS or not.
Thus, we should never raise a normal IRQ after having sent an error
IRQ.
NOTE: for QEMU platforms that use SeaBIOS, this patch depends on QEMU
commit 784155cdcb02 ("seabios: update submodule to git snapshot"), and
QEMU commit 14f5a7bae4cb ("seabios: update binaries to git snapshot"),
which update SeaBIOS to a version that contains SeaBIOS commit 1281e340
("ahci: handle TFES irq correctly").
Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
Message-ID: <20231011131220.1992064-1-nks@flawful.org>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
hw/ide/ahci.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/hw/ide/ahci.c b/hw/ide/ahci.c
index fcc5476e9e..7676e2d871 100644
--- a/hw/ide/ahci.c
+++ b/hw/ide/ahci.c
@@ -897,11 +897,10 @@ static bool ahci_write_fis_d2h(AHCIDevice *ad, bool d2h_fis_i)
pr->tfdata = (ad->port.ifs[0].error << 8) |
ad->port.ifs[0].status;
+ /* TFES IRQ is always raised if ERR_STAT is set, regardless of I bit. */
if (d2h_fis[2] & ERR_STAT) {
ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_TFES);
- }
-
- if (d2h_fis_i) {
+ } else if (d2h_fis_i) {
ahci_trigger_irq(ad->hba, ad, AHCI_PORT_IRQ_BIT_DHRS);
}
--
2.41.0
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [PULL 00/25] Block layer patches
2023-11-07 18:45 [PULL 00/25] Block layer patches Kevin Wolf
` (24 preceding siblings ...)
2023-11-07 18:46 ` [PULL 25/25] hw/ide/ahci: trigger either error IRQ or regular IRQ, not both Kevin Wolf
@ 2023-11-08 1:44 ` Stefan Hajnoczi
25 siblings, 0 replies; 27+ messages in thread
From: Stefan Hajnoczi @ 2023-11-08 1:44 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel
On Wed, 8 Nov 2023 at 02:47, Kevin Wolf <kwolf@redhat.com> wrote:
>
> The following changes since commit 462ad017ed76889d46696a3581e1b52343f9b683:
>
> Merge tag 'pixman-pull-request' of https://gitlab.com/marcandre.lureau/qemu into staging (2023-11-07 19:00:03 +0800)
>
> are available in the Git repository at:
>
> https://repo.or.cz/qemu/kevin.git tags/for-upstream
>
> for you to fetch changes up to 5d4a5080e43681b753f922c5c2a7682bc1d67497:
>
> hw/ide/ahci: trigger either error IRQ or regular IRQ, not both (2023-11-07 19:14:20 +0100)
>
> ----------------------------------------------------------------
> Block layer patches
>
> - Graph locking part 6 (bs->file/backing)
> - ahci: trigger either error IRQ or regular IRQ, not both
>
> ----------------------------------------------------------------
> Kevin Wolf (24):
> block: Mark bdrv_probe_blocksizes() and callers GRAPH_RDLOCK
> block: Mark bdrv_has_zero_init() and callers GRAPH_RDLOCK
> block: Mark bdrv_filter_bs() and callers GRAPH_RDLOCK
> block: Mark bdrv_root_attach_child() GRAPH_WRLOCK
> block: Mark block_job_add_bdrv() GRAPH_WRLOCK
> block: Mark bdrv_filter_or_cow_bs() and callers GRAPH_RDLOCK
> block: Mark bdrv_skip_implicit_filters() and callers GRAPH_RDLOCK
> block: Mark bdrv_skip_filters() and callers GRAPH_RDLOCK
> block: Mark bdrv_(un)freeze_backing_chain() and callers GRAPH_RDLOCK
> block: Mark bdrv_chain_contains() and callers GRAPH_RDLOCK
> block: Mark bdrv_filter_child() and callers GRAPH_RDLOCK
> block: Mark bdrv_cow_child() and callers GRAPH_RDLOCK
> block: Mark bdrv_set_backing_hd_drained() GRAPH_WRLOCK
> block: Inline bdrv_set_backing_noperm()
> block: Mark bdrv_replace_node_common() GRAPH_WRLOCK
> block: Mark bdrv_replace_node() GRAPH_WRLOCK
> block: Protect bs->backing with graph_lock
Hi Kevin,
This patch series fails CI:
commit ecdd779f1332d0625cbb6195e30db8e3c179cc76
Author: Kevin Wolf <kwolf@redhat.com>
Date: Fri Oct 27 17:53:26 2023 +0200
block: Protect bs->backing with graph_lock
../block/mirror.c: In function 'mirror_run':
../block/mirror.c:1173:9: error: 'bs' may be used uninitialized in
this function [-Werror=maybe-uninitialized]
1173 | bdrv_drained_begin(bs);
| ^~~~~~~~~~~~~~~~~~~~~~
https://gitlab.com/qemu-project/qemu/-/jobs/5483228890
Please take a look. Thanks!
Stefan
> blkverify: Add locking for request_fn
> block: Introduce bdrv_co_change_backing_file()
> block: Add missing GRAPH_RDLOCK annotations
> qcow2: Take locks for accessing bs->file
> vhdx: Take locks for accessing bs->file
> block: Take graph lock for most of .bdrv_open
> block: Protect bs->file with graph_lock
>
> Niklas Cassel (1):
> hw/ide/ahci: trigger either error IRQ or regular IRQ, not both
>
> block/copy-on-read.h | 3 +-
> block/parallels.h | 5 +-
> block/qcow2.h | 59 +++++-----
> block/qed.h | 2 +-
> block/vhdx.h | 9 +-
> include/block/block-global-state.h | 43 ++++----
> include/block/block-io.h | 10 +-
> include/block/block_int-common.h | 31 +++---
> include/block/block_int-global-state.h | 16 +--
> include/block/block_int-io.h | 19 ++--
> include/block/blockjob.h | 5 +-
> include/block/blockjob_int.h | 9 +-
> block.c | 192 +++++++++++++++++++--------------
> block/backup.c | 21 ++--
> block/blkdebug.c | 29 ++---
> block/blkreplay.c | 8 +-
> block/blkverify.c | 18 ++--
> block/block-backend.c | 5 +
> block/block-copy.c | 11 +-
> block/bochs.c | 4 +
> block/cloop.c | 4 +
> block/commit.c | 32 +++++-
> block/copy-before-write.c | 6 +-
> block/copy-on-read.c | 19 +++-
> block/crypto.c | 10 ++
> block/dmg.c | 21 ++--
> block/filter-compress.c | 5 +-
> block/io.c | 2 +
> block/mirror.c | 85 ++++++++++-----
> block/monitor/block-hmp-cmds.c | 3 +
> block/parallels-ext.c | 21 ++--
> block/parallels.c | 22 ++--
> block/preallocate.c | 27 +++--
> block/qcow.c | 13 ++-
> block/qcow2-bitmap.c | 14 +--
> block/qcow2-cluster.c | 25 ++---
> block/qcow2.c | 148 +++++++++++++------------
> block/qed.c | 88 ++++++++-------
> block/raw-format.c | 36 ++++---
> block/replication.c | 12 ++-
> block/snapshot-access.c | 5 +-
> block/stream.c | 48 ++++++---
> block/throttle.c | 3 +
> block/vdi.c | 15 ++-
> block/vhdx-log.c | 40 +++----
> block/vhdx.c | 37 ++++---
> block/vmdk.c | 23 ++--
> block/vpc.c | 6 +-
> blockdev.c | 72 ++++++++-----
> blockjob.c | 6 +-
> hw/ide/ahci.c | 5 +-
> migration/block-dirty-bitmap.c | 4 +
> nbd/server.c | 6 ++
> qemu-img.c | 31 ++++--
> tests/unit/test-bdrv-drain.c | 39 +++++--
> tests/unit/test-bdrv-graph-mod.c | 18 +++-
> 56 files changed, 923 insertions(+), 527 deletions(-)
>
>
^ permalink raw reply [flat|nested] 27+ messages in thread