Re: [PATCH 04/24] block: Mark bdrv_root_attach_child() GRAPH_WRLOCK

[Eric Blake]

On Fri, Nov 03, 2023 at 10:45:11AM +0100, Kevin Wolf wrote:
> Am 27.10.2023 um 22:22 hat Eric Blake geschrieben:
> > On Fri, Oct 27, 2023 at 05:53:13PM +0200, Kevin Wolf wrote:
> > > 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>
> > > ---
> > >  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(-)
> > > 
> > > +++ 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,
> > 
> > Do we need some sort of assertion that the caller did indeed grab the
> > lock at this point?  Or is that redundant with assertions made in all
> > helper functions we are calling where the lock already matters?
> 
> The GRAPH_WRLOCK in the header file makes it a compiler error with TSA
> enabled if the caller doesn't already hold the lock. And our CI has some
> builds with TSA, so even if the maintainer only uses gcc, we'll catch
> it.

TSA is awesome - guaranteeing code correctness during CI has been an
awesome result of this massive refactoring effort.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org