diff mbox series

[19/20] block: Mark bdrv_recurse_can_replace() and callers GRAPH_RDLOCK

Message ID 20230425173158.574203-20-kwolf@redhat.com (mailing list archive)
State New, archived
Headers show
Series Graph locking, part 3 (more block drivers) | expand

Commit Message

Kevin Wolf April 25, 2023, 5:31 p.m. UTC 
This adds GRAPH_RDLOCK annotations to declare that callers of
bdrv_recurse_can_replace() need to hold a reader lock for the graph
because it accesses the children list of a node.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/block/block-global-state.h     | 5 +++--
 include/block/block_int-common.h       | 4 ++--
 include/block/block_int-global-state.h | 4 ++--
 block/blkverify.c                      | 5 +++--
 block/mirror.c                         | 4 ++++
 block/quorum.c                         | 4 ++--
 blockdev.c                             | 3 +++
 7 files changed, 19 insertions(+), 10 deletions(-)

Comments

Eric Blake April 25, 2023, 9:22 p.m. UTC | #1 
On Tue, Apr 25, 2023 at 07:31:57PM +0200, Kevin Wolf wrote:
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_recurse_can_replace() need to hold a reader lock for the graph
> because it accesses the children list of a node.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-global-state.h     | 5 +++--
>  include/block/block_int-common.h       | 4 ++--
>  include/block/block_int-global-state.h | 4 ++--
>  block/blkverify.c                      | 5 +++--
>  block/mirror.c                         | 4 ++++
>  block/quorum.c                         | 4 ++--
>  blockdev.c                             | 3 +++
>  7 files changed, 19 insertions(+), 10 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>
Stefan Hajnoczi May 1, 2023, 7:10 p.m. UTC | #2 
On Tue, Apr 25, 2023 at 07:31:57PM +0200, Kevin Wolf wrote:
> This adds GRAPH_RDLOCK annotations to declare that callers of
> bdrv_recurse_can_replace() need to hold a reader lock for the graph
> because it accesses the children list of a node.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/block-global-state.h     | 5 +++--
>  include/block/block_int-common.h       | 4 ++--
>  include/block/block_int-global-state.h | 4 ++--
>  block/blkverify.c                      | 5 +++--
>  block/mirror.c                         | 4 ++++
>  block/quorum.c                         | 4 ++--
>  blockdev.c                             | 3 +++
>  7 files changed, 19 insertions(+), 10 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff mbox series

Patch

diff --git a/include/block/block-global-state.h b/include/block/block-global-state.h
index ec3ddb17a8..f234bca0b6 100644
--- a/include/block/block-global-state.h
+++ b/include/block/block-global-state.h
@@ -163,8 +163,9 @@  int bdrv_amend_options(BlockDriverState *bs_new, QemuOpts *opts,
                        Error **errp);
 
 /* check if a named node can be replaced when doing drive-mirror */
-BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs,
-                                        const char *node_name, Error **errp);
+BlockDriverState * GRAPH_RDLOCK
+check_to_replace_node(BlockDriverState *parent_bs, const char *node_name,
+                      Error **errp);
 
 int no_coroutine_fn bdrv_activate(BlockDriverState *bs, Error **errp);
 
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 37d094796e..024ded4fc2 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -217,8 +217,8 @@  struct BlockDriver {
      * same data as @bs without it affecting @bs's behavior (that is,
      * without it being visible to @bs's parents).
      */
-    bool (*bdrv_recurse_can_replace)(BlockDriverState *bs,
-                                     BlockDriverState *to_replace);
+    bool GRAPH_RDLOCK_PTR (*bdrv_recurse_can_replace)(
+        BlockDriverState *bs, BlockDriverState *to_replace);
 
     int (*bdrv_probe_device)(const char *filename);
 
diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h
index 902406eb99..da5fb31089 100644
--- a/include/block/block_int-global-state.h
+++ b/include/block/block_int-global-state.h
@@ -225,8 +225,8 @@  int bdrv_child_try_set_perm(BdrvChild *c, uint64_t perm, uint64_t shared,
  */
 int bdrv_child_refresh_perms(BlockDriverState *bs, BdrvChild *c, Error **errp);
 
-bool bdrv_recurse_can_replace(BlockDriverState *bs,
-                              BlockDriverState *to_replace);
+bool GRAPH_RDLOCK bdrv_recurse_can_replace(BlockDriverState *bs,
+                                           BlockDriverState *to_replace);
 
 /*
  * Default implementation for BlockDriver.bdrv_child_perm() that can
diff --git a/block/blkverify.c b/block/blkverify.c
index 1c16f86b2e..7326461f30 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -265,8 +265,9 @@  static int coroutine_fn GRAPH_RDLOCK blkverify_co_flush(BlockDriverState *bs)
     return bdrv_co_flush(s->test_file->bs);
 }
 
-static bool blkverify_recurse_can_replace(BlockDriverState *bs,
-                                          BlockDriverState *to_replace)
+static bool GRAPH_RDLOCK
+blkverify_recurse_can_replace(BlockDriverState *bs,
+                              BlockDriverState *to_replace)
 {
     BDRVBlkverifyState *s = bs->opaque;
 
diff --git a/block/mirror.c b/block/mirror.c
index a2f46783cf..99409f893f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -747,7 +747,10 @@  static int mirror_exit_common(Job *job)
          * Cannot use check_to_replace_node() here, because that would
          * check for an op blocker on @to_replace, and we have our own
          * there.
+         *
+         * TODO Pull out the writer lock from bdrv_replace_node() to here
          */
+        bdrv_graph_rdlock_main_loop();
         if (bdrv_recurse_can_replace(src, to_replace)) {
             bdrv_replace_node(to_replace, target_bs, &local_err);
         } else {
@@ -756,6 +759,7 @@  static int mirror_exit_common(Job *job)
                        "would not lead to an abrupt change of visible data",
                        to_replace->node_name, target_bs->node_name);
         }
+        bdrv_graph_rdunlock_main_loop();
         bdrv_drained_end(target_bs);
         if (local_err) {
             error_report_err(local_err);
diff --git a/block/quorum.c b/block/quorum.c
index ff5a0a2da3..f28758cf2b 100644
--- a/block/quorum.c
+++ b/block/quorum.c
@@ -825,8 +825,8 @@  static coroutine_fn GRAPH_RDLOCK int quorum_co_flush(BlockDriverState *bs)
     return result;
 }
 
-static bool quorum_recurse_can_replace(BlockDriverState *bs,
-                                       BlockDriverState *to_replace)
+static bool GRAPH_RDLOCK
+quorum_recurse_can_replace(BlockDriverState *bs, BlockDriverState *to_replace)
 {
     BDRVQuorumState *s = bs->opaque;
     int i;
diff --git a/blockdev.c b/blockdev.c
index eb509cf964..bfca91ee4e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2951,6 +2951,9 @@  static void blockdev_mirror_common(const char *job_id, BlockDriverState *bs,
     BlockDriverState *unfiltered_bs;
     int job_flags = JOB_DEFAULT;
 
+    GLOBAL_STATE_CODE();
+    GRAPH_RDLOCK_GUARD_MAINLOOP();
+
     if (!has_speed) {
         speed = 0;
     }