Re: [PATCH 23/24] block: Take graph lock for most of .bdrv_open

[Eric Blake]

On Fri, Oct 27, 2023 at 05:53:32PM +0200, Kevin Wolf wrote:
> Most implementations of .bdrv_open first open their file child (which is
> an operation that internally takes the write lock and therefore we
> shouldn't hold the graph lock while calling it), and afterwards many
> operations that require holding the graph lock, e.g. for accessing
> bs->file.
> 
> This changes block drivers that follow this pattern to take the graph
> lock after opening the child node.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/blkdebug.c          | 16 ++++++++++------
>  block/bochs.c             |  4 ++++
>  block/cloop.c             |  4 ++++
>  block/copy-before-write.c |  2 ++
>  block/copy-on-read.c      |  4 ++--
>  block/crypto.c            |  4 ++++
>  block/dmg.c               |  5 +++++
>  block/filter-compress.c   |  2 ++
>  block/parallels.c         |  4 ++--
>  block/preallocate.c       |  4 ++++
>  block/qcow.c              | 11 +++++++----
>  block/raw-format.c        |  6 ++++--
>  block/snapshot-access.c   |  3 +++
>  block/throttle.c          |  3 +++
>  block/vdi.c               |  4 ++--
>  block/vpc.c               |  4 ++--
>  16 files changed, 60 insertions(+), 20 deletions(-)
> 

> +++ b/block/qcow.c
> @@ -124,9 +124,11 @@ static int qcow_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  
>      ret = bdrv_open_file_child(NULL, options, "file", bs, errp);
>      if (ret < 0) {
> -        goto fail;
> +        goto fail_unlocked;
>      }
>  
> +    bdrv_graph_rdlock_main_loop();
> +
>      ret = bdrv_pread(bs->file, 0, sizeof(header), &header, 0);
>      if (ret < 0) {
>          goto fail;
> @@ -301,11 +303,9 @@ static int qcow_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      }
>  
>      /* Disable migration when qcow images are used */
> -    bdrv_graph_rdlock_main_loop();
>      error_setg(&s->migration_blocker, "The qcow format used by node '%s' "
>                 "does not support live migration",
>                 bdrv_get_device_or_node_name(bs));
> -    bdrv_graph_rdunlock_main_loop();
>  
>      ret = migrate_add_blocker(s->migration_blocker, errp);
>      if (ret < 0) {
> @@ -316,9 +316,12 @@ static int qcow_open(BlockDriverState *bs, QDict 
> *options, int flags,
>      qobject_unref(encryptopts);
>      qapi_free_QCryptoBlockOpenOptions(crypto_opts);
>      qemu_co_mutex_init(&s->lock);
> +    bdrv_graph_rdunlock_main_loop();
>      return 0;
>  
> - fail:
> +fail:

Why the change in indentation?  At least emacs intentionally prefers
to indent top-level labels 1 column in, so that they cannot be
confused with function declarations in the first column.  But that's cosmetic.

> +    bdrv_graph_rdunlock_main_loop();
> +fail_unlocked:
>      g_free(s->l1_table);
>      qemu_vfree(s->l2_cache);
>      g_free(s->cluster_cache);

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

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