From 1b88457eaae483d34d7ec40d2fcd9cf771982910 Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 4 Sep 2023 12:03:04 +0200 Subject: block: complete public block status API Include both coroutine and non-coroutine versions, the latter being co_wrapper_mixed_bdrv_rdlock of the former. Reviewed-by: Eric Blake Signed-off-by: Paolo Bonzini Message-ID: <20230904100306.156197-3-pbonzini@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- include/block/block-io.h | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) (limited to 'include') diff --git a/include/block/block-io.h b/include/block/block-io.h index f1c796a..41f78f2 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -127,17 +127,22 @@ int coroutine_fn GRAPH_RDLOCK bdrv_co_zone_append(BlockDriverState *bs, BdrvRequestFlags flags); bool bdrv_can_write_zeroes_with_unmap(BlockDriverState *bs); -int bdrv_block_status(BlockDriverState *bs, int64_t offset, - int64_t bytes, int64_t *pnum, int64_t *map, - BlockDriverState **file); + +int coroutine_fn GRAPH_RDLOCK +bdrv_co_block_status(BlockDriverState *bs, int64_t offset, int64_t bytes, + int64_t *pnum, int64_t *map, BlockDriverState **file); +int co_wrapper_mixed_bdrv_rdlock +bdrv_block_status(BlockDriverState *bs, int64_t offset, int64_t bytes, + int64_t *pnum, int64_t *map, BlockDriverState **file); int coroutine_fn GRAPH_RDLOCK bdrv_co_block_status_above(BlockDriverState *bs, BlockDriverState *base, int64_t offset, int64_t bytes, int64_t *pnum, int64_t *map, BlockDriverState **file); -int bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base, - int64_t offset, int64_t bytes, int64_t *pnum, - int64_t *map, BlockDriverState **file); +int co_wrapper_mixed_bdrv_rdlock +bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base, + int64_t offset, int64_t bytes, int64_t *pnum, + int64_t *map, BlockDriverState **file); int coroutine_fn GRAPH_RDLOCK bdrv_co_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes, -- cgit v1.1 From 578ffa9ffb13d9a40790de2a3dda8730d4d43efc Mon Sep 17 00:00:00 2001 From: Paolo Bonzini Date: Mon, 4 Sep 2023 12:03:05 +0200 Subject: block: switch to co_wrapper for bdrv_is_allocated_* Reviewed-by: Eric Blake Signed-off-by: Paolo Bonzini Message-ID: <20230904100306.156197-4-pbonzini@redhat.com> Reviewed-by: Kevin Wolf Signed-off-by: Kevin Wolf --- include/block/block-io.h | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) (limited to 'include') diff --git a/include/block/block-io.h b/include/block/block-io.h index 41f78f2..6485b19 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -147,16 +147,18 @@ bdrv_block_status_above(BlockDriverState *bs, BlockDriverState *base, int coroutine_fn GRAPH_RDLOCK bdrv_co_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes, int64_t *pnum); -int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes, - int64_t *pnum); +int co_wrapper_mixed_bdrv_rdlock +bdrv_is_allocated(BlockDriverState *bs, int64_t offset, + int64_t bytes, int64_t *pnum); int coroutine_fn GRAPH_RDLOCK bdrv_co_is_allocated_above(BlockDriverState *top, BlockDriverState *base, bool include_base, int64_t offset, int64_t bytes, int64_t *pnum); -int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base, - bool include_base, int64_t offset, int64_t bytes, - int64_t *pnum); +int co_wrapper_mixed_bdrv_rdlock +bdrv_is_allocated_above(BlockDriverState *bs, BlockDriverState *base, + bool include_base, int64_t offset, + int64_t bytes, int64_t *pnum); int coroutine_fn GRAPH_RDLOCK bdrv_co_is_zero_fast(BlockDriverState *bs, int64_t offset, int64_t bytes); -- cgit v1.1 From e84c07bc73f63cd0251d9fd2c582ad051e27fb39 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:37 +0200 Subject: block-coroutine-wrapper: Add no_co_wrapper_bdrv_rdlock functions Add a new wrapper type for GRAPH_RDLOCK functions that should be called from coroutine context. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-3-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block-common.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/include/block/block-common.h b/include/block/block-common.h index 2d2af72..d759956 100644 --- a/include/block/block-common.h +++ b/include/block/block-common.h @@ -66,13 +66,16 @@ * function. The coroutine yields after scheduling the BH and is reentered when * the wrapped function returns. * - * A no_co_wrapper_bdrv_wrlock function is a no_co_wrapper function that - * automatically takes the graph wrlock when calling the wrapped function. + * A no_co_wrapper_bdrv_rdlock function is a no_co_wrapper function that + * automatically takes the graph rdlock when calling the wrapped function. In + * the same way, no_co_wrapper_bdrv_wrlock functions automatically take the + * graph wrlock. * * If the first parameter of the function is a BlockDriverState, BdrvChild or * BlockBackend pointer, the AioContext lock for it is taken in the wrapper. */ #define no_co_wrapper +#define no_co_wrapper_bdrv_rdlock #define no_co_wrapper_bdrv_wrlock #include "block/blockjob.h" -- cgit v1.1 From 2b3912f1350971fbc2c04d986a1d0c60ae757c78 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:39 +0200 Subject: block: Mark bdrv_first_blk() and bdrv_is_root_node() GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_first_blk() and bdrv_is_root_node() need to hold a reader lock for the graph. These functions are the only functions in block-backend.c that access the parent list of a node. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-5-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block-global-state.h | 9 +++++---- include/sysemu/block-backend-global-state.h | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) (limited to 'include') diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index 6061220..505a0d0 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -170,9 +170,10 @@ BlockDriverState * GRAPH_RDLOCK check_to_replace_node(BlockDriverState *parent_bs, const char *node_name, Error **errp); -int no_coroutine_fn bdrv_activate(BlockDriverState *bs, Error **errp); +int no_coroutine_fn GRAPH_RDLOCK +bdrv_activate(BlockDriverState *bs, Error **errp); -int coroutine_fn no_co_wrapper +int coroutine_fn no_co_wrapper_bdrv_rdlock bdrv_co_activate(BlockDriverState *bs, Error **errp); void bdrv_activate_all(Error **errp); @@ -208,8 +209,8 @@ typedef struct BdrvNextIterator { BlockDriverState *bs; } BdrvNextIterator; -BlockDriverState *bdrv_first(BdrvNextIterator *it); -BlockDriverState *bdrv_next(BdrvNextIterator *it); +BlockDriverState * GRAPH_RDLOCK bdrv_first(BdrvNextIterator *it); +BlockDriverState * GRAPH_RDLOCK bdrv_next(BdrvNextIterator *it); void bdrv_next_cleanup(BdrvNextIterator *it); BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs); diff --git a/include/sysemu/block-backend-global-state.h b/include/sysemu/block-backend-global-state.h index d5f6754..49c12b0f 100644 --- a/include/sysemu/block-backend-global-state.h +++ b/include/sysemu/block-backend-global-state.h @@ -59,8 +59,8 @@ BlockBackend *blk_by_public(BlockBackendPublic *public); void blk_remove_bs(BlockBackend *blk); int blk_insert_bs(BlockBackend *blk, BlockDriverState *bs, Error **errp); int blk_replace_bs(BlockBackend *blk, BlockDriverState *new_bs, Error **errp); -bool bdrv_has_blk(BlockDriverState *bs); -bool bdrv_is_root_node(BlockDriverState *bs); +bool GRAPH_RDLOCK bdrv_has_blk(BlockDriverState *bs); +bool GRAPH_RDLOCK bdrv_is_root_node(BlockDriverState *bs); int GRAPH_UNLOCKED blk_set_perm(BlockBackend *blk, uint64_t perm, uint64_t shared_perm, Error **errp); void blk_get_perm(BlockBackend *blk, uint64_t *perm, uint64_t *shared_perm); -- cgit v1.1 From d05ab380db649d882396653f9830b67d84bffbe1 Mon Sep 17 00:00:00 2001 From: Emanuele Giuseppe Esposito Date: Fri, 29 Sep 2023 16:51:40 +0200 Subject: block: Mark drain related functions GRAPH_RDLOCK Draining recursively traverses the graph, therefore we need to make sure that also such accesses to the graph are protected by the graph rdlock. There are 3 different drain callers to consider: 1. drain in the main loop: no issue at all, rdlock is nop. 2. drain in an iothread: rdlock only works in main loop or coroutines, so disallow it. 3. drain in a coroutine (regardless of AioContext): the drain mechanism takes care of scheduling a BH in the bs->aio_context that will then take care of perform the actual draining. This is wrong, because as pointed in (2) if bs->aio_context is an iothread then rdlock won't work. Therefore change bdrv_co_yield_to_drain to schedule the BH in the main loop. Caller (2) also implies that we need to modify test-bdrv-drain.c to disallow draining in the iothreads. For some places, we know that they will hold the lock, but we don't have the GRAPH_RDLOCK annotations yet. In this case, add assume_graph_lock() with a FIXME comment. These places will be removed once everything is properly annotated. Signed-off-by: Emanuele Giuseppe Esposito Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-6-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block-io.h | 23 ++++++++++++++++++----- include/block/block_int-common.h | 6 +++--- 2 files changed, 21 insertions(+), 8 deletions(-) (limited to 'include') diff --git a/include/block/block-io.h b/include/block/block-io.h index 6485b19..9707eb3 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -370,7 +370,7 @@ bdrv_writev_vmstate(BlockDriverState *bs, QEMUIOVector *qiov, int64_t pos); * * Begin a quiesced section for the parent of @c. */ -void bdrv_parent_drained_begin_single(BdrvChild *c); +void GRAPH_RDLOCK bdrv_parent_drained_begin_single(BdrvChild *c); /** * bdrv_parent_drained_poll_single: @@ -378,14 +378,14 @@ void bdrv_parent_drained_begin_single(BdrvChild *c); * Returns true if there is any pending activity to cease before @c can be * called quiesced, false otherwise. */ -bool bdrv_parent_drained_poll_single(BdrvChild *c); +bool GRAPH_RDLOCK bdrv_parent_drained_poll_single(BdrvChild *c); /** * bdrv_parent_drained_end_single: * * End a quiesced section for the parent of @c. */ -void bdrv_parent_drained_end_single(BdrvChild *c); +void GRAPH_RDLOCK bdrv_parent_drained_end_single(BdrvChild *c); /** * bdrv_drain_poll: @@ -398,8 +398,9 @@ void bdrv_parent_drained_end_single(BdrvChild *c); * * This is part of bdrv_drained_begin. */ -bool bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent, - bool ignore_bds_parents); +bool GRAPH_RDLOCK +bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent, + bool ignore_bds_parents); /** * bdrv_drained_begin: @@ -407,6 +408,12 @@ bool bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent, * Begin a quiesced section for exclusive access to the BDS, by disabling * external request sources including NBD server, block jobs, and device model. * + * This function can only be invoked by the main loop or a coroutine + * (regardless of the AioContext where it is running). + * If the coroutine is running in an Iothread AioContext, this function will + * just schedule a BH to run in the main loop. + * However, it cannot be directly called by an Iothread. + * * This function can be recursive. */ void bdrv_drained_begin(BlockDriverState *bs); @@ -423,6 +430,12 @@ void bdrv_do_drained_begin_quiesce(BlockDriverState *bs, BdrvChild *parent); * bdrv_drained_end: * * End a quiescent section started by bdrv_drained_begin(). + * + * This function can only be invoked by the main loop or a coroutine + * (regardless of the AioContext where it is running). + * If the coroutine is running in an Iothread AioContext, this function will + * just schedule a BH to run in the main loop. + * However, it cannot be directly called by an Iothread. */ void bdrv_drained_end(BlockDriverState *bs); diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 2ca3758..8ef6817 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -963,15 +963,15 @@ struct BdrvChildClass { * Note that this can be nested. If drained_begin() was called twice, new * I/O is allowed only after drained_end() was called twice, too. */ - void (*drained_begin)(BdrvChild *child); - void (*drained_end)(BdrvChild *child); + void GRAPH_RDLOCK_PTR (*drained_begin)(BdrvChild *child); + void GRAPH_RDLOCK_PTR (*drained_end)(BdrvChild *child); /* * Returns whether the parent has pending requests for the child. This * callback is polled after .drained_begin() has been called until all * activity on the child has stopped. */ - bool (*drained_poll)(BdrvChild *child); + bool GRAPH_RDLOCK_PTR (*drained_poll)(BdrvChild *child); /* * Notifies the parent that the filename of its child has changed (e.g. -- cgit v1.1 From a32e781838e7231f2239bde0ac2f105dc7072abb Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:42 +0200 Subject: block: Mark bdrv_snapshot_fallback() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_snapshot_fallback() need to hold a reader lock for the graph because it accesses the children list of a node. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-8-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block_int-common.h | 18 ++++++++++-------- include/block/snapshot.h | 24 ++++++++++++++---------- 2 files changed, 24 insertions(+), 18 deletions(-) (limited to 'include') diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 8ef6817..29c5b8a 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -313,14 +313,16 @@ struct BlockDriver { int GRAPH_RDLOCK_PTR (*bdrv_inactivate)(BlockDriverState *bs); - int (*bdrv_snapshot_create)(BlockDriverState *bs, - QEMUSnapshotInfo *sn_info); - int (*bdrv_snapshot_goto)(BlockDriverState *bs, - const char *snapshot_id); - int (*bdrv_snapshot_delete)(BlockDriverState *bs, - const char *snapshot_id, - const char *name, - Error **errp); + int GRAPH_RDLOCK_PTR (*bdrv_snapshot_create)( + BlockDriverState *bs, QEMUSnapshotInfo *sn_info); + + int GRAPH_UNLOCKED_PTR (*bdrv_snapshot_goto)( + BlockDriverState *bs, const char *snapshot_id); + + int GRAPH_RDLOCK_PTR (*bdrv_snapshot_delete)( + 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, diff --git a/include/block/snapshot.h b/include/block/snapshot.h index 50ff924..d49c559 100644 --- a/include/block/snapshot.h +++ b/include/block/snapshot.h @@ -25,6 +25,7 @@ #ifndef SNAPSHOT_H #define SNAPSHOT_H +#include "block/graph-lock.h" #include "qapi/qapi-builtin-types.h" #define SNAPSHOT_OPT_BASE "snapshot." @@ -59,16 +60,19 @@ bool bdrv_snapshot_find_by_id_and_name(BlockDriverState *bs, const char *name, QEMUSnapshotInfo *sn_info, Error **errp); -int bdrv_can_snapshot(BlockDriverState *bs); -int bdrv_snapshot_create(BlockDriverState *bs, - QEMUSnapshotInfo *sn_info); -int bdrv_snapshot_goto(BlockDriverState *bs, - const char *snapshot_id, - Error **errp); -int bdrv_snapshot_delete(BlockDriverState *bs, - const char *snapshot_id, - const char *name, - Error **errp); + +int GRAPH_RDLOCK bdrv_can_snapshot(BlockDriverState *bs); + +int GRAPH_RDLOCK +bdrv_snapshot_create(BlockDriverState *bs, QEMUSnapshotInfo *sn_info); + +int GRAPH_UNLOCKED +bdrv_snapshot_goto(BlockDriverState *bs, const char *snapshot_id, Error **errp); + +int GRAPH_RDLOCK +bdrv_snapshot_delete(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, -- cgit v1.1 From ce433d2942b78d38d31bdb7845dbf565c9dc1109 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:43 +0200 Subject: block: Take graph rdlock in parts of reopen Reopen isn't easy with respect to locking because many of its functions need to iterate the graph, some change it, and then you get some drains in the middle where you can't hold any locks. Therefore just documents most of the functions to be unlocked, and take locks internally before accessing the graph. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-9-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block_int-common.h | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) (limited to 'include') diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 29c5b8a..0373cbe 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -235,11 +235,14 @@ struct BlockDriver { Error **errp); /* For handling image reopen for split or non-split files. */ - int (*bdrv_reopen_prepare)(BDRVReopenState *reopen_state, - BlockReopenQueue *queue, Error **errp); - void (*bdrv_reopen_commit)(BDRVReopenState *reopen_state); - void (*bdrv_reopen_commit_post)(BDRVReopenState *reopen_state); - void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state); + int GRAPH_UNLOCKED_PTR (*bdrv_reopen_prepare)( + BDRVReopenState *reopen_state, BlockReopenQueue *queue, Error **errp); + void GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit)( + BDRVReopenState *reopen_state); + void GRAPH_UNLOCKED_PTR (*bdrv_reopen_commit_post)( + BDRVReopenState *reopen_state); + void GRAPH_UNLOCKED_PTR (*bdrv_reopen_abort)( + BDRVReopenState *reopen_state); void (*bdrv_join_options)(QDict *options, QDict *old_options); int GRAPH_UNLOCKED_PTR (*bdrv_open)( -- cgit v1.1 From 15f3f1fe57cd98ef0f45a25681b7a99dc3be0484 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:44 +0200 Subject: block: Mark bdrv_get_xdbg_block_graph() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_get_xdbg_block_graph() need to hold a reader lock for the graph because it accesses the children list of a node. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-10-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block-global-state.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'include') diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index 505a0d0..4d80b3d 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -192,7 +192,7 @@ int bdrv_has_zero_init_1(BlockDriverState *bs); int bdrv_has_zero_init(BlockDriverState *bs); BlockDriverState *bdrv_find_node(const char *node_name); BlockDeviceInfoList *bdrv_named_nodes_list(bool flat, Error **errp); -XDbgBlockGraph *bdrv_get_xdbg_block_graph(Error **errp); +XDbgBlockGraph * GRAPH_RDLOCK bdrv_get_xdbg_block_graph(Error **errp); BlockDriverState *bdrv_lookup_bs(const char *device, const char *node_name, Error **errp); -- cgit v1.1 From b7cfc7d58ec697a681a269036dc8f6444ffd495d Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:45 +0200 Subject: block: Mark bdrv_refresh_filename() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_refresh_filename() need to hold a reader lock for the graph because it accesses the children list of a node. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-11-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block-global-state.h | 9 ++++++--- include/block/block_int-common.h | 8 ++++---- include/block/qapi.h | 16 +++++++--------- 3 files changed, 17 insertions(+), 16 deletions(-) (limited to 'include') diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index 4d80b3d..ec623ef 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -132,7 +132,7 @@ int bdrv_reopen_set_read_only(BlockDriverState *bs, bool read_only, Error **errp); BlockDriverState *bdrv_find_backing_image(BlockDriverState *bs, const char *backing_file); -void bdrv_refresh_filename(BlockDriverState *bs); +void GRAPH_RDLOCK bdrv_refresh_filename(BlockDriverState *bs); void GRAPH_RDLOCK bdrv_refresh_limits(BlockDriverState *bs, Transaction *tran, Error **errp); @@ -216,8 +216,11 @@ void bdrv_next_cleanup(BdrvNextIterator *it); BlockDriverState *bdrv_next_monitor_owned(BlockDriverState *bs); void bdrv_iterate_format(void (*it)(void *opaque, const char *name), void *opaque, bool read_only); -char *bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp); -char *bdrv_dirname(BlockDriverState *bs, Error **errp); + +char * GRAPH_RDLOCK +bdrv_get_full_backing_filename(BlockDriverState *bs, Error **errp); + +char * GRAPH_RDLOCK bdrv_dirname(BlockDriverState *bs, Error **errp); void bdrv_img_create(const char *filename, const char *fmt, const char *base_filename, const char *base_fmt, diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 0373cbe..be80887 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -272,7 +272,7 @@ struct BlockDriver { * Refreshes the bs->exact_filename field. If that is impossible, * bs->exact_filename has to be left empty. */ - void (*bdrv_refresh_filename)(BlockDriverState *bs); + void GRAPH_RDLOCK_PTR (*bdrv_refresh_filename)(BlockDriverState *bs); /* * Gathers the open options for all children into @target. @@ -295,15 +295,15 @@ struct BlockDriver { * block driver which implements it is probably doing something * shady regarding its runtime option structure. */ - void (*bdrv_gather_child_options)(BlockDriverState *bs, QDict *target, - bool backing_overridden); + void GRAPH_RDLOCK_PTR (*bdrv_gather_child_options)( + BlockDriverState *bs, QDict *target, bool backing_overridden); /* * Returns an allocated string which is the directory name of this BDS: It * will be used to make relative filenames absolute by prepending this * function's return value to them. */ - char *(*bdrv_dirname)(BlockDriverState *bs, Error **errp); + char * GRAPH_RDLOCK_PTR (*bdrv_dirname)(BlockDriverState *bs, Error **errp); /* * This informs the driver that we are no longer interested in the result diff --git a/include/block/qapi.h b/include/block/qapi.h index 8663971..8872356 100644 --- a/include/block/qapi.h +++ b/include/block/qapi.h @@ -29,18 +29,16 @@ #include "block/snapshot.h" #include "qapi/qapi-types-block-core.h" -BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk, - BlockDriverState *bs, - bool flat, - Error **errp); +BlockDeviceInfo * GRAPH_RDLOCK +bdrv_block_device_info(BlockBackend *blk, BlockDriverState *bs, + bool flat, Error **errp); + int bdrv_query_snapshot_info_list(BlockDriverState *bs, SnapshotInfoList **p_list, Error **errp); -void bdrv_query_image_info(BlockDriverState *bs, - ImageInfo **p_info, - bool flat, - bool skip_implicit_filters, - Error **errp); +void GRAPH_RDLOCK +bdrv_query_image_info(BlockDriverState *bs, ImageInfo **p_info, bool flat, + bool skip_implicit_filters, Error **errp); void GRAPH_RDLOCK bdrv_query_block_graph_info(BlockDriverState *bs, BlockGraphInfo **p_info, Error **errp); -- cgit v1.1 From c0fc5123ad33158f6f289a896b568b9adce7d1f2 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:46 +0200 Subject: block: Mark bdrv_primary_child() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_primary_child() need to hold a reader lock for the graph because it accesses the children list of a node. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-12-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block_int-io.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h index eb0da72..2b6004a 100644 --- a/include/block/block_int-io.h +++ b/include/block/block_int-io.h @@ -133,7 +133,7 @@ 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 *bdrv_primary_child(BlockDriverState *bs); +BdrvChild * GRAPH_RDLOCK bdrv_primary_child(BlockDriverState *bs); BlockDriverState *bdrv_skip_filters(BlockDriverState *bs); BlockDriverState *bdrv_backing_chain_next(BlockDriverState *bs); @@ -155,7 +155,8 @@ static inline BlockDriverState *bdrv_filter_or_cow_bs(BlockDriverState *bs) return child_bs(bdrv_filter_or_cow_child(bs)); } -static inline BlockDriverState *bdrv_primary_bs(BlockDriverState *bs) +static inline BlockDriverState * GRAPH_RDLOCK +bdrv_primary_bs(BlockDriverState *bs) { IO_CODE(); return child_bs(bdrv_primary_child(bs)); -- cgit v1.1 From 4026f1c4f320aa072fa4cd299545cbc97315e246 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:47 +0200 Subject: block: Mark bdrv_get_parent_name() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_get_parent_name() need to hold a reader lock for the graph because it accesses the parents list of a node. For some places, we know that they will hold the lock, but we don't have the GRAPH_RDLOCK annotations yet. In this case, add assume_graph_lock() with a FIXME comment. These places will be removed once everything is properly annotated. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-13-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block-io.h | 8 ++++++-- include/block/block_int-io.h | 2 +- include/block/qapi.h | 7 ++++--- 3 files changed, 11 insertions(+), 6 deletions(-) (limited to 'include') diff --git a/include/block/block-io.h b/include/block/block-io.h index 9707eb3..2c0c7b1 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -183,8 +183,12 @@ const char *bdrv_get_format_name(BlockDriverState *bs); bool bdrv_supports_compressed_writes(BlockDriverState *bs); const char *bdrv_get_node_name(const BlockDriverState *bs); -const char *bdrv_get_device_name(const BlockDriverState *bs); -const char *bdrv_get_device_or_node_name(const BlockDriverState *bs); + +const char * GRAPH_RDLOCK +bdrv_get_device_name(const BlockDriverState *bs); + +const char * GRAPH_RDLOCK +bdrv_get_device_or_node_name(const BlockDriverState *bs); int coroutine_fn GRAPH_RDLOCK bdrv_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi); diff --git a/include/block/block_int-io.h b/include/block/block_int-io.h index 2b6004a..34eac72 100644 --- a/include/block/block_int-io.h +++ b/include/block/block_int-io.h @@ -99,7 +99,7 @@ BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size, */ void bdrv_wakeup(BlockDriverState *bs); -const char *bdrv_get_parent_name(const BlockDriverState *bs); +const char * GRAPH_RDLOCK bdrv_get_parent_name(const BlockDriverState *bs); bool blk_dev_has_tray(BlockBackend *blk); bool blk_dev_is_tray_open(BlockBackend *blk); diff --git a/include/block/qapi.h b/include/block/qapi.h index 8872356..54c48de 100644 --- a/include/block/qapi.h +++ b/include/block/qapi.h @@ -33,9 +33,10 @@ BlockDeviceInfo * GRAPH_RDLOCK bdrv_block_device_info(BlockBackend *blk, BlockDriverState *bs, bool flat, Error **errp); -int bdrv_query_snapshot_info_list(BlockDriverState *bs, - SnapshotInfoList **p_list, - Error **errp); +int GRAPH_RDLOCK +bdrv_query_snapshot_info_list(BlockDriverState *bs, + SnapshotInfoList **p_list, + Error **errp); void GRAPH_RDLOCK bdrv_query_image_info(BlockDriverState *bs, ImageInfo **p_info, bool flat, bool skip_implicit_filters, Error **errp); -- cgit v1.1 From bd131d6705b6996f2cdccee3db017af570ce91ad Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:48 +0200 Subject: block: Mark bdrv_amend_options() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_amend_options() need to hold a reader lock for the graph. This removes an assume_graph_lock() call in crypto's implementation. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-14-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block-global-state.h | 8 ++++---- include/block/block_int-common.h | 10 ++++------ 2 files changed, 8 insertions(+), 10 deletions(-) (limited to 'include') diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index ec623ef..9dc3513 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -160,10 +160,10 @@ void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base); */ typedef void BlockDriverAmendStatusCB(BlockDriverState *bs, int64_t offset, int64_t total_work_size, void *opaque); -int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts, - BlockDriverAmendStatusCB *status_cb, void *cb_opaque, - bool force, - Error **errp); +int GRAPH_RDLOCK +bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts, + BlockDriverAmendStatusCB *status_cb, void *cb_opaque, + bool force, Error **errp); /* check if a named node can be replaced when doing drive-mirror */ BlockDriverState * GRAPH_RDLOCK diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index be80887..c866177 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -259,12 +259,10 @@ struct BlockDriver { int coroutine_fn GRAPH_UNLOCKED_PTR (*bdrv_co_create_opts)( BlockDriver *drv, const char *filename, QemuOpts *opts, Error **errp); - int (*bdrv_amend_options)(BlockDriverState *bs, - QemuOpts *opts, - BlockDriverAmendStatusCB *status_cb, - void *cb_opaque, - bool force, - Error **errp); + int GRAPH_RDLOCK_PTR (*bdrv_amend_options)( + BlockDriverState *bs, QemuOpts *opts, + BlockDriverAmendStatusCB *status_cb, void *cb_opaque, + bool force, Error **errp); int (*bdrv_make_empty)(BlockDriverState *bs); -- cgit v1.1 From 0bb79c97fd8e355aca433f4331d7d45e7e72b4b6 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:49 +0200 Subject: qcow2: Mark qcow2_signal_corruption() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of qcow2_signal_corruption() need to hold a reader lock for the graph because it calls bdrv_get_node_name(), which accesses the parents list of a node. For some places, we know that they will hold the lock, but we don't have the GRAPH_RDLOCK annotations yet. In this case, add assume_graph_lock() with a FIXME comment. These places will be removed once everything is properly annotated. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-15-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block-global-state.h | 2 +- include/block/block_int-common.h | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index 9dc3513..794ef34 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -138,7 +138,7 @@ void GRAPH_RDLOCK bdrv_refresh_limits(BlockDriverState *bs, Transaction *tran, Error **errp); int bdrv_commit(BlockDriverState *bs); -int bdrv_make_empty(BdrvChild *c, Error **errp); +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); diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index c866177..d971d73 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -264,7 +264,7 @@ struct BlockDriver { BlockDriverAmendStatusCB *status_cb, void *cb_opaque, bool force, Error **errp); - int (*bdrv_make_empty)(BlockDriverState *bs); + int GRAPH_RDLOCK_PTR (*bdrv_make_empty)(BlockDriverState *bs); /* * Refreshes the bs->exact_filename field. If that is impossible, -- cgit v1.1 From 277f2007ce187a6ff467579cb016824f80a2be10 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:52 +0200 Subject: block: Mark bdrv_op_is_blocked() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_op_is_blocked() need to hold a reader lock for the graph because it calls bdrv_get_device_or_node_name(), which accesses the parents list of a node. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-18-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block-global-state.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'include') diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h index 794ef34..6bfafe7 100644 --- a/include/block/block-global-state.h +++ b/include/block/block-global-state.h @@ -246,7 +246,9 @@ bdrv_attach_child(BlockDriverState *parent_bs, BdrvChildRole child_role, Error **errp); -bool bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp); +bool GRAPH_RDLOCK +bdrv_op_is_blocked(BlockDriverState *bs, BlockOpType op, Error **errp); + void bdrv_op_block(BlockDriverState *bs, BlockOpType op, Error *reason); void bdrv_op_unblock(BlockDriverState *bs, BlockOpType op, Error *reason); void bdrv_op_block_all(BlockDriverState *bs, Error *reason); -- cgit v1.1 From 018f9dea9c905506c49f8f37c018470dddfc50f1 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:53 +0200 Subject: block: Mark bdrv_apply_auto_read_only() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_apply_auto_read_only() need to hold a reader lock for the graph because it calls bdrv_can_set_read_only(), which indirectly accesses the parents list of a node. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-19-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block-io.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/include/block/block-io.h b/include/block/block-io.h index 2c0c7b1..e051e9b 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -163,8 +163,10 @@ bdrv_is_allocated_above(BlockDriverState *bs, BlockDriverState *base, int coroutine_fn GRAPH_RDLOCK bdrv_co_is_zero_fast(BlockDriverState *bs, int64_t offset, int64_t bytes); -int bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg, - Error **errp); +int GRAPH_RDLOCK +bdrv_apply_auto_read_only(BlockDriverState *bs, const char *errmsg, + Error **errp); + bool bdrv_is_read_only(BlockDriverState *bs); bool bdrv_is_writable(BlockDriverState *bs); bool bdrv_is_sg(BlockDriverState *bs); -- cgit v1.1 From 3574499a1e5e420c3e72d0e283cfb2b13bce672e Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:54 +0200 Subject: block: Mark bdrv_get_specific_info() and callers GRAPH_RDLOCK This adds GRAPH_RDLOCK annotations to declare that callers of bdrv_get_specific_info() need to hold a reader lock for the graph. This removes an assume_graph_lock() call in vmdk's implementation. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-20-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block-io.h | 5 +++-- include/block/block_int-common.h | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) (limited to 'include') diff --git a/include/block/block-io.h b/include/block/block-io.h index e051e9b..ad270b6 100644 --- a/include/block/block-io.h +++ b/include/block/block-io.h @@ -198,8 +198,9 @@ bdrv_co_get_info(BlockDriverState *bs, BlockDriverInfo *bdi); int co_wrapper_mixed_bdrv_rdlock bdrv_get_info(BlockDriverState *bs, BlockDriverInfo *bdi); -ImageInfoSpecific *bdrv_get_specific_info(BlockDriverState *bs, - Error **errp); +ImageInfoSpecific * GRAPH_RDLOCK +bdrv_get_specific_info(BlockDriverState *bs, Error **errp); + BlockStatsSpecific *bdrv_get_specific_stats(BlockDriverState *bs); void bdrv_round_to_subclusters(BlockDriverState *bs, int64_t offset, int64_t bytes, diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index d971d73..024262b 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -728,8 +728,8 @@ struct BlockDriver { int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_get_info)( BlockDriverState *bs, BlockDriverInfo *bdi); - ImageInfoSpecific *(*bdrv_get_specific_info)(BlockDriverState *bs, - Error **errp); + ImageInfoSpecific * GRAPH_RDLOCK_PTR (*bdrv_get_specific_info)( + BlockDriverState *bs, Error **errp); BlockStatsSpecific *(*bdrv_get_specific_stats)(BlockDriverState *bs); int coroutine_fn GRAPH_RDLOCK_PTR (*bdrv_co_save_vmstate)( -- cgit v1.1 From b59b466071391cb76b39584e1558be2d0797c054 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:55 +0200 Subject: block: Protect bs->parents with graph_lock Almost all functions that access the parent link already take the graph lock now. Add locking to the remaining user in a test case and finally annotate the struct field itself as protected by the graph lock. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-21-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block_int-common.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 024262b..0e37acd 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -1043,7 +1043,7 @@ struct BdrvChild { bool quiesced_parent; QLIST_ENTRY(BdrvChild) next; - QLIST_ENTRY(BdrvChild) next_parent; + QLIST_ENTRY(BdrvChild GRAPH_RDLOCK_PTR) next_parent; }; /* @@ -1180,7 +1180,7 @@ struct BlockDriverState { BdrvChild *backing; BdrvChild *file; - QLIST_HEAD(, BdrvChild) parents; + QLIST_HEAD(, BdrvChild GRAPH_RDLOCK_PTR) parents; QDict *options; QDict *explicit_options; -- cgit v1.1 From 680e0cc40c5830ebcbfa0bce99bf932e1a4cf6c6 Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:56 +0200 Subject: block: Protect bs->children with graph_lock Almost all functions that access the child links already take the graph lock now. Add locking to the remaining users and finally annotate the struct field itself as protected by the graph lock. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-22-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/block_int-common.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h index 0e37acd..b8d9d24 100644 --- a/include/block/block_int-common.h +++ b/include/block/block_int-common.h @@ -1042,7 +1042,7 @@ struct BdrvChild { */ bool quiesced_parent; - QLIST_ENTRY(BdrvChild) next; + QLIST_ENTRY(BdrvChild GRAPH_RDLOCK_PTR) next; QLIST_ENTRY(BdrvChild GRAPH_RDLOCK_PTR) next_parent; }; @@ -1176,7 +1176,7 @@ struct BlockDriverState { * See also comment in include/block/block.h, to learn how backing and file * are connected with BdrvChildRole. */ - QLIST_HEAD(, BdrvChild) children; + QLIST_HEAD(, BdrvChild GRAPH_RDLOCK_PTR) children; BdrvChild *backing; BdrvChild *file; -- cgit v1.1 From e6e964b8b021446c8d3d1f91c0208f653e9ec92c Mon Sep 17 00:00:00 2001 From: Kevin Wolf Date: Fri, 29 Sep 2023 16:51:57 +0200 Subject: block: Add assertion for bdrv_graph_wrlock() bdrv_graph_wrlock() can't run in a coroutine (because it polls) and requires holding the BQL. We already have GLOBAL_STATE_CODE() to assert the latter. Assert the former as well and add a no_coroutine_fn marker. Signed-off-by: Kevin Wolf Message-ID: <20230929145157.45443-23-kwolf@redhat.com> Reviewed-by: Stefan Hajnoczi Signed-off-by: Kevin Wolf --- include/block/graph-lock.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'include') diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h index 7e04f98..6f1cd12 100644 --- a/include/block/graph-lock.h +++ b/include/block/graph-lock.h @@ -116,7 +116,8 @@ void unregister_aiocontext(AioContext *ctx); * This function polls. Callers must not hold the lock of any AioContext other * than the current one and the one of @bs. */ -void bdrv_graph_wrlock(BlockDriverState *bs) TSA_ACQUIRE(graph_lock) TSA_NO_TSA; +void no_coroutine_fn TSA_ACQUIRE(graph_lock) TSA_NO_TSA +bdrv_graph_wrlock(BlockDriverState *bs); /* * bdrv_graph_wrunlock: -- cgit v1.1