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

[Kevin Wolf]

Am 27.10.2023 um 23:00 hat Eric Blake geschrieben:
> 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);

I don't remember if there were more reasons without having a closer look
at the code, but just here from the context, bdrv_unref() is supposed to
be called without the graph lock held. We don't enforce this yet because
there are some cases that are not easy to fix, but I don't want to make
adding the GRAPH_UNLOCKED annotation to bdrv_unref() harder than it
needs to be.

> > @@ -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.)

Generally I used the scoped locks only when I was quite sure that
nothing in the function will require dropping the lock. The safe default
is individually locking smaller sections.

With GRAPH_RDLOCK_GUARD_MAINLOOP(), the whole situation is a bit fuzzy
because it doesn't actually do anything apart from asserting that we
don't need real locking. So taking it while calling functions that want
to be called unlocked still works in practice, but it's a bit unclean,
and it would conflict with an actual GRAPH_UNLOCKED annotation.

Kevin