[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 09/24] block: Mark bdrv_(un)freeze_backing_chain() and caller
From: |
Eric Blake |
Subject: |
Re: [PATCH 09/24] block: Mark bdrv_(un)freeze_backing_chain() and callers GRAPH_RDLOCK |
Date: |
Fri, 27 Oct 2023 16:00:10 -0500 |
User-agent: |
NeoMutt/20231023 |
On Fri, Oct 27, 2023 at 05:53:18PM +0200, Kevin Wolf wrote:
> 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>
> ---
> 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(-)
>
...
> +++ b/block/copy-on-read.c
...
> -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();
Why the two-line addition here...
> }
>
> 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);
> }
...vs. the magic one-line per-scope change here? Both work, so I
don't see any problems, but it does seem odd to mix styles in the same
patch. (I can see other places where you have intentionally picked
the version that required the least reindenting; adding a scope just
to use GRAPH_RDLOCK_GUARD_MAINLOOP() without having to carefully pair
an unlock on every early exit path is fewer lines of code overall, but
more lines of churn in the patch itself.)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
- [PATCH 03/24] block: Mark bdrv_filter_bs() and callers GRAPH_RDLOCK, (continued)
- [PATCH 03/24] block: Mark bdrv_filter_bs() and callers GRAPH_RDLOCK, Kevin Wolf, 2023/10/27
- [PATCH 05/24] block: Mark block_job_add_bdrv() GRAPH_WRLOCK, Kevin Wolf, 2023/10/27
- [PATCH 15/24] block: Mark bdrv_replace_node_common() GRAPH_WRLOCK, Kevin Wolf, 2023/10/27
- [PATCH 10/24] block: Mark bdrv_chain_contains() and callers GRAPH_RDLOCK, Kevin Wolf, 2023/10/27
- [PATCH 09/24] block: Mark bdrv_(un)freeze_backing_chain() and callers GRAPH_RDLOCK, Kevin Wolf, 2023/10/27
- Re: [PATCH 09/24] block: Mark bdrv_(un)freeze_backing_chain() and callers GRAPH_RDLOCK,
Eric Blake <=
- [PATCH 14/24] block: Inline bdrv_set_backing_noperm(), Kevin Wolf, 2023/10/27
- [PATCH 04/24] block: Mark bdrv_root_attach_child() GRAPH_WRLOCK, Kevin Wolf, 2023/10/27
- [PATCH 21/24] qcow2: Take locks for accessing bs->file, Kevin Wolf, 2023/10/27
- [PATCH 06/24] block: Mark bdrv_filter_or_cow_bs() and callers GRAPH_RDLOCK, Kevin Wolf, 2023/10/27
- [PATCH 07/24] block: Mark bdrv_skip_implicit_filters() and callers GRAPH_RDLOCK, Kevin Wolf, 2023/10/27