Re: [PATCH 08/20] block: .bdrv_open is non-coroutine and unlocked

[Eric Blake]

On Tue, Apr 25, 2023 at 07:31:46PM +0200, Kevin Wolf wrote:
> Drivers were a bit confused about whether .bdrv_open can run in a
> coroutine and whether or not it holds a graph lock.
> 
> It cannot keep a graph lock from the caller across the whole function
> because it both changes the graph (requires a writer lock) and does I/O
> (requires a reader lock). Therefore, it should take these locks
> internally as needed.
> 
> The functions used to be called in coroutine context during image
> creation. This was buggy for other reasons, and as of commit 32192301,
> all block drivers go through no_co_wrappers. So it is not called in
> coroutine context any more.
> 
> Fix qcow2 and qed to work with the correct assumptions: The graph lock
> needs to be taken internally instead of just assuming it's already
> there, and the coroutine path is dead code that can be removed.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block_int-common.h |  8 ++++----
>  block.c                          |  6 +++---
>  block/qcow2.c                    | 15 ++++++---------
>  block/qed.c                      | 18 ++++++++----------
>  4 files changed, 21 insertions(+), 26 deletions(-)
> 
> diff --git a/include/block/block_int-common.h 
> b/include/block/block_int-common.h
> index 013d419444..6fb28cd8fa 100644
> --- a/include/block/block_int-common.h
> +++ b/include/block/block_int-common.h
> @@ -236,12 +236,12 @@ struct BlockDriver {
>      void (*bdrv_reopen_abort)(BDRVReopenState *reopen_state);
>      void (*bdrv_join_options)(QDict *options, QDict *old_options);
>  
> -    int (*bdrv_open)(BlockDriverState *bs, QDict *options, int flags,
> -                     Error **errp);
> +    int GRAPH_UNLOCKED_PTR (*bdrv_open)(
> +        BlockDriverState *bs, QDict *options, int flags, Error **errp);
>  
>      /* Protocol drivers should implement this instead of bdrv_open */
> -    int (*bdrv_file_open)(BlockDriverState *bs, QDict *options, int flags,
> -                          Error **errp);
> +    int GRAPH_UNLOCKED_PTR (*bdrv_file_open)(
> +        BlockDriverState *bs, QDict *options, int flags, Error **errp);

May conflict with Paolo's work to eliminate bdrv_file_open as
redundant.  But that should be easy to figure out.

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

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org