Re: [PATCH 09/24] block: Mark bdrv_(un)freeze_backing_chain() and callers GRAPH_RDLOCK

[Eric Blake]

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