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

[Kevin Wolf <kwolf@redhat.com>]

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.

Kevin