The function bdrv_new() calls bdrv_drained_begin(), which must be
called with the graph unlocked.

Marking bdrv_new() as GRAPH_UNLOCKED requires making the locked
section in bdrv_open_inherit() shorter.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

I'm not sure if the TODO comment is only intended for the
lower half of the function, i.e. is moving it like this okay?

 block.c                            | 7 ++++---
 include/block/block-global-state.h | 2 +-
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 6f470aac2f..1b9c99dda9 100644
--- a/block.c
+++ b/block.c
@@ -3995,10 +3995,8 @@ bdrv_open_inherit(const char *filename, const char *reference, QDict *options,
     GLOBAL_STATE_CODE();
     assert(!qemu_in_coroutine());
 
-    /* TODO We'll eventually have to take a writer lock in this function */
-    GRAPH_RDLOCK_GUARD_MAINLOOP();
-
     if (reference) {
+        GRAPH_RDLOCK_GUARD_MAINLOOP();
         bool options_non_empty = options ? qdict_size(options) : false;
         qobject_unref(options);
 
@@ -4019,6 +4017,9 @@ bdrv_open_inherit(const char *filename, const char *reference, QDict *options,
 
     bs = bdrv_new();
 
+    /* TODO We'll eventually have to take a writer lock in this function */
+    GRAPH_RDLOCK_GUARD_MAINLOOP();
+
     /* NULL means an empty set of options */
     if (options == NULL) {
         options = qdict_new();
diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index eec92a98da..b1f826dca6 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -67,7 +67,7 @@ int co_wrapper bdrv_create(BlockDriver *drv, const char *filename,
 int coroutine_fn GRAPH_UNLOCKED
 bdrv_co_create_file(const char *filename, QemuOpts *opts, Error **errp);
 
-BlockDriverState *bdrv_new(void);
+BlockDriverState * GRAPH_UNLOCKED bdrv_new(void);
 int bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top,
                 Error **errp);
 
-- 
2.39.5

Am 30.05.2025 um 17:11 hat Fiona Ebner geschrieben:
> The function bdrv_new() calls bdrv_drained_begin(), which must be
> called with the graph unlocked.
> 
> Marking bdrv_new() as GRAPH_UNLOCKED requires making the locked
> section in bdrv_open_inherit() shorter.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
> 
> I'm not sure if the TODO comment is only intended for the
> lower half of the function, i.e. is moving it like this okay?

The thing that should require locking is when you attach the new node to
something, which is after the place where you moved it to. Currently,
these functions take the lock internally, and I'm not sure if that can
possibly be changed because opening an image usually involves a mix of
I/O to read image metadata (which is incompatible with having a writer
lock) and graph changing operations. It's not clear if this TODO can
ever be resolved...

But I'm not sure if bdrv_new() really should be GRAPH_UNLOCKED. We know
that we don't have any active I/O for a node that we only just created
and that isn't even linked in the global list yet. So maybe the other
option is using bdrv_do_drained_begin_quiesce(bs, NULL) in bdrv_new()
instead? Then callers can hold the lock if they want to.

Kevin