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

[Eric Blake]

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?

>                                     child_role, perm, shared_perm, opaque,
>                                     tran, errp);
> @@ -3228,9 +3226,8 @@ BdrvChild *bdrv_root_attach_child(BlockDriverState 
> *child_bs,
>  
>  out:
>      tran_finalize(tran, ret);
> -    bdrv_graph_wrunlock();
>  
> -    bdrv_unref(child_bs);
> +    bdrv_schedule_unref(child_bs);

The change to unref matches the change to the slightly longer lock
scope; makes sense.

Assuming I figured out the answer to my question above,

Reviewed-by: Eric Blake <eblake@redhat.com>

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