Re: [PATCH v2 07/24] block: mark bdrv_child_change_aio_context() GRAPH_RDLOCK

[Kevin Wolf <kwolf@redhat.com>]

Am 23.05.2025 um 19:20 hat Andrey Drobyshev geschrieben:
> On 5/20/25 1:29 PM, Fiona Ebner wrote:
> > This is a small step in preparation to mark bdrv_drained_begin() as
> > GRAPH_UNLOCKED. More concretely, it is in preparatoin to move the
> > drain out of bdrv_change_aio_context() and marking that function as
> > GRAPH_RDLOCK.
> > 
> > Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> > ---
> > 
> > New in v2.
> > 
> >  include/block/block-global-state.h | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
> > index 9be34b3c99..aad160956a 100644
> > --- a/include/block/block-global-state.h
> > +++ b/include/block/block-global-state.h
> > @@ -274,9 +274,10 @@ int bdrv_debug_remove_breakpoint(BlockDriverState *bs, const char *tag);
> >  int bdrv_debug_resume(BlockDriverState *bs, const char *tag);
> >  bool bdrv_debug_is_suspended(BlockDriverState *bs, const char *tag);
> >  
> > -bool bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
> > -                                   GHashTable *visited, Transaction *tran,
> > -                                   Error **errp);
> > +bool GRAPH_RDLOCK
> > +bdrv_child_change_aio_context(BdrvChild *c, AioContext *ctx,
> > +                              GHashTable *visited, Transaction *tran,
> > +                              Error **errp);
> >  int bdrv_try_change_aio_context(BlockDriverState *bs, AioContext *ctx,
> >                                  BdrvChild *ignore_child, Error **errp);
> >  
> 
> I think we might as well add the GRAPH_RDLOCK mark to the actual
> function definition in block.c for better readability.

We don't do this for public functions. The reason is that TSA can't
catch it if you annotate the function definition, but forget it in the
header file. So to the human reader (and in code after the definition in
the same file) it would look like the lock is taken, but in reality
callers in other source files can call the function without holding the
lock.

If https://github.com/llvm/llvm-project/pull/67520 is merged eventually,
we can reconsider this rule, but for now, it's better to keep it.

Kevin