block: drop wrapper for bdrv_set_backing_hd_drained()

Nearly all callers (outside of the tests) are already using the
_drained() variant of the function. It doesn't seem worth keeping.
Simply adapt the remaining callers of bdrv_set_backing_hd() and rename
bdrv_set_backing_hd_drained() to bdrv_set_backing_hd().

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
Message-ID: <20250530151125.955508-31-f.ebner@proxmox.com>
Reviewed-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Kevin Wolf <kwolf@redhat.com>

block.c

@@ -3570,9 +3570,8 @@ out:
  *
  * All block nodes must be drained.
  */
-int bdrv_set_backing_hd_drained(BlockDriverState *bs,
+int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
-                                BlockDriverState *backing_hd,
+                        Error **errp)
-                                Error **errp)
 {
     int ret;
     Transaction *tran = tran_new();
@@ -3594,19 +3593,6 @@ out:
     return ret;
 }
 
-int bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
-                        Error **errp)
-{
-    int ret;
-    GLOBAL_STATE_CODE();
-
-    bdrv_graph_wrlock_drained();
-    ret = bdrv_set_backing_hd_drained(bs, backing_hd, errp);
-    bdrv_graph_wrunlock();
-
-    return ret;
-}
-
 /*
  * Opens the backing file for a BlockDriverState if not yet open
  *
@@ -3715,7 +3701,9 @@ int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
     /* Hook up the backing file link; drop our reference, bs owns the
      * backing_hd reference now */
     bdrv_graph_rdunlock_main_loop();
+    bdrv_graph_wrlock_drained();
     ret = bdrv_set_backing_hd(bs, backing_hd, errp);
+    bdrv_graph_wrunlock();
     bdrv_graph_rdlock_main_loop();
     bdrv_unref(backing_hd);
 

block/commit.c

@@ -566,8 +566,8 @@ int bdrv_commit(BlockDriverState *bs)
     bdrv_graph_rdunlock_main_loop();
 
     bdrv_graph_wrlock_drained();
-    bdrv_set_backing_hd_drained(commit_top_bs, backing_file_bs, &error_abort);
+    bdrv_set_backing_hd(commit_top_bs, backing_file_bs, &error_abort);
-    bdrv_set_backing_hd_drained(bs, commit_top_bs, &error_abort);
+    bdrv_set_backing_hd(bs, commit_top_bs, &error_abort);
     bdrv_graph_wrunlock();
 
     bdrv_graph_rdlock_main_loop();
@@ -649,7 +649,7 @@ ro_cleanup:
     bdrv_graph_rdunlock_main_loop();
     bdrv_graph_wrlock_drained();
     if (bdrv_cow_bs(bs) != backing_file_bs) {
-        bdrv_set_backing_hd_drained(bs, backing_file_bs, &error_abort);
+        bdrv_set_backing_hd(bs, backing_file_bs, &error_abort);
     }
     bdrv_graph_wrunlock();
     bdrv_graph_rdlock_main_loop();

block/mirror.c

@@ -772,7 +772,7 @@ static int mirror_exit_common(Job *job)
 
         backing = s->sync_mode == MIRROR_SYNC_MODE_NONE ? src : s->base;
         if (bdrv_cow_bs(unfiltered_target) != backing) {
-            bdrv_set_backing_hd_drained(unfiltered_target, backing, &local_err);
+            bdrv_set_backing_hd(unfiltered_target, backing, &local_err);
             if (local_err) {
                 error_report_err(local_err);
                 local_err = NULL;

block/stream.c

@@ -73,12 +73,11 @@ static int stream_prepare(Job *job)
     s->cor_filter_bs = NULL;
 
     /*
-     * bdrv_set_backing_hd() requires that the unfiltered_bs and the COW child
+     * bdrv_set_backing_hd() requires that all block nodes are drained. Drain
-     * of unfiltered_bs is drained. Drain already here and use
+     * already here, because the polling during drained_begin() might change the
-     * bdrv_set_backing_hd_drained() instead because the polling during
+     * graph, and if we do this only later, we may end up working with the wrong
-     * drained_begin() might change the graph, and if we do this only later, we
+     * base node (or it might even have gone away by the time we want to use
-     * may end up working with the wrong base node (or it might even have gone
+     * it).
-     * away by the time we want to use it).
      */
     if (unfiltered_bs_cow) {
         bdrv_ref(unfiltered_bs_cow);
@@ -105,7 +104,7 @@ static int stream_prepare(Job *job)
         }
 
         bdrv_graph_wrlock();
-        bdrv_set_backing_hd_drained(unfiltered_bs, base, &local_err);
+        bdrv_set_backing_hd(unfiltered_bs, base, &local_err);
         bdrv_graph_wrunlock();
 
         /*

blockdev.c

@@ -1587,12 +1587,12 @@ static void external_snapshot_abort(void *opaque)
             /*
              * Note that state->old_bs would not disappear during the
              * write-locked section, because the unref from
-             * bdrv_set_backing_hd_drained() only happens at the end of the
+             * bdrv_set_backing_hd() only happens at the end of the write-locked
-             * write-locked section. However, just be explicit about keeping a
+             * section. However, just be explicit about keeping a reference and
-             * reference and don't rely on that implicit detail.
+             * don't rely on that implicit detail.
              */
             bdrv_ref(state->old_bs);
-            bdrv_set_backing_hd_drained(state->new_bs, NULL, &error_abort);
+            bdrv_set_backing_hd(state->new_bs, NULL, &error_abort);
 
             /*
              * The call to bdrv_set_backing_hd() above returns state->old_bs to
@@ -1776,7 +1776,10 @@ static void drive_backup_action(DriveBackup *backup,
     }
 
     if (set_backing_hd) {
-        if (bdrv_set_backing_hd(target_bs, source, errp) < 0) {
+        bdrv_graph_wrlock_drained();
+        ret = bdrv_set_backing_hd(target_bs, source, errp);
+        bdrv_graph_wrunlock();
+        if (ret < 0) {
             goto unref;
         }
     }

include/block/block-global-state.h

@@ -100,12 +100,9 @@ bdrv_open_blockdev_ref(BlockdevRef *ref, Error **errp);
 BlockDriverState * coroutine_fn no_co_wrapper
 bdrv_co_open_blockdev_ref(BlockdevRef *ref, Error **errp);
 
-int GRAPH_UNLOCKED
+int GRAPH_WRLOCK
 bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd,
                     Error **errp);
-int GRAPH_WRLOCK
-bdrv_set_backing_hd_drained(BlockDriverState *bs, BlockDriverState *backing_hd,
-                            Error **errp);
 
 int bdrv_open_backing_file(BlockDriverState *bs, QDict *parent_options,
                            const char *bdref_key, Error **errp);

tests/unit/test-bdrv-drain.c

@@ -193,7 +193,9 @@ static BlockBackend * no_coroutine_fn test_setup(void)
     blk_insert_bs(blk, bs, &error_abort);
 
     backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort);
+    bdrv_graph_wrlock_drained();
     bdrv_set_backing_hd(bs, backing, &error_abort);
+    bdrv_graph_wrunlock();
 
     bdrv_unref(backing);
     bdrv_unref(bs);
@@ -386,7 +388,9 @@ static void test_nested(void)
 
     backing = bdrv_new_open_driver(&bdrv_test, "backing", 0, &error_abort);
     backing_s = backing->opaque;
+    bdrv_graph_wrlock_drained();
     bdrv_set_backing_hd(bs, backing, &error_abort);
+    bdrv_graph_wrunlock();
 
     for (outer = 0; outer < DRAIN_TYPE_MAX; outer++) {
         for (inner = 0; inner < DRAIN_TYPE_MAX; inner++) {
@@ -733,10 +737,12 @@ static void test_blockjob_common_drain_node(enum drain_type drain_type,
     src_overlay = bdrv_new_open_driver(&bdrv_test, "source-overlay",
                                        BDRV_O_RDWR, &error_abort);
 
+    bdrv_graph_wrlock_drained();
     bdrv_set_backing_hd(src_overlay, src, &error_abort);
     bdrv_unref(src);
     bdrv_set_backing_hd(src, src_backing, &error_abort);
     bdrv_unref(src_backing);
+    bdrv_graph_wrunlock();
 
     blk_src = blk_new(qemu_get_aio_context(), BLK_PERM_ALL, BLK_PERM_ALL);
     blk_insert_bs(blk_src, src_overlay, &error_abort);
@@ -1436,8 +1442,10 @@ static void test_drop_backing_job_commit(Job *job)
     TestDropBackingBlockJob *s =
         container_of(job, TestDropBackingBlockJob, common.job);
 
+    bdrv_graph_wrlock_drained();
     bdrv_set_backing_hd(s->bs, NULL, &error_abort);
     bdrv_set_backing_hd(s->detach_also, NULL, &error_abort);
+    bdrv_graph_wrunlock();
 
     *s->did_complete = true;
 }
@@ -1530,7 +1538,9 @@ static void test_blockjob_commit_by_drained_end(void)
         snprintf(name, sizeof(name), "parent-node-%i", i);
         bs_parents[i] = bdrv_new_open_driver(&bdrv_test, name, BDRV_O_RDWR,
                                              &error_abort);
+        bdrv_graph_wrlock_drained();
         bdrv_set_backing_hd(bs_parents[i], bs_child, &error_abort);
+        bdrv_graph_wrunlock();
     }
 
     job = block_job_create("job", &test_drop_backing_job_driver, NULL,
@@ -1679,13 +1689,13 @@ static void test_drop_intermediate_poll(void)
 
     job_node = bdrv_new_open_driver(&bdrv_test, "job-node", BDRV_O_RDWR,
                                     &error_abort);
+    bdrv_graph_wrlock_drained();
     bdrv_set_backing_hd(job_node, chain[1], &error_abort);
 
     /*
      * Establish the chain last, so the chain links are the first
      * elements in the BDS.parents lists
      */
-    bdrv_graph_wrlock_drained();
     for (i = 0; i < 3; i++) {
         if (i) {
             /* Takes the reference to chain[i - 1] */

tests/unit/test-bdrv-graph-mod.c

@@ -202,9 +202,9 @@ static void test_should_update_child(void)
 
     blk_insert_bs(root, bs, &error_abort);
 
+    bdrv_graph_wrlock_drained();
     bdrv_set_backing_hd(target, bs, &error_abort);
 
-    bdrv_graph_wrlock_drained();
     g_assert(target->backing->bs == bs);
     bdrv_attach_child(filter, target, "target", &child_of_bds,
                       BDRV_CHILD_DATA, &error_abort);