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 <kw...@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 <ebl...@redhat.com>
--
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization: qemu.org | libguestfs.org
