diff mbox series

[RFC,v2,6/8] block: assert that graph read and writes are performed correctly

Message ID 20220426085114.199647-7-eesposit@redhat.com (mailing list archive)
State New, archived
Headers show
Series Removal of AioContext lock, bs->parents and ->children: new rwlock | expand

Commit Message

Emanuele Giuseppe Esposito April 26, 2022, 8:51 a.m. UTC 
Remove the old assert_bdrv_graph_writable, and replace it with
the new version using graph-lock API.
See the function documentation for more information.

Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
 block.c                                |  8 ++++----
 block/graph-lock.c                     | 11 +++++++++++
 include/block/block_int-global-state.h | 17 -----------------
 include/block/graph-lock.h             | 15 +++++++++++++++
 4 files changed, 30 insertions(+), 21 deletions(-)

Comments

Stefan Hajnoczi April 28, 2022, 2:43 p.m. UTC | #1 
On Tue, Apr 26, 2022 at 04:51:12AM -0400, Emanuele Giuseppe Esposito wrote:
> diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h
> index f171ba0527..2211d41286 100644
> --- a/include/block/graph-lock.h
> +++ b/include/block/graph-lock.h
> @@ -52,5 +52,20 @@ void coroutine_fn bdrv_graph_co_rdlock(void);
>   */
>  void coroutine_fn bdrv_graph_co_rdunlock(void);
>  
> +/*
> + * assert_bdrv_graph_readable:
> + * Make sure that the reader is either the main loop,
> + * or there is at least a reader helding the rdlock.
> + * In this way an incoming writer is aware of the read and waits.
> + */
> +void assert_bdrv_graph_readable(void);

The name confuses me. I suggest assert_bdrv_graph_rdlock_held().

> +
> +/*
> + * assert_bdrv_graph_writable:
> + * Make sure that the writer is the main loop and has set @has_writer,
> + * so that incoming readers will pause.
> + */
> +void assert_bdrv_graph_writable(void);

assert_bdrv_graph_wrlock_held().

> +
>  #endif /* BLOCK_LOCK_H */
>  
> -- 
> 2.31.1
>
diff mbox series

Patch

diff --git a/block.c b/block.c
index 6cd87e8dd3..b427d632e1 100644
--- a/block.c
+++ b/block.c
@@ -1438,7 +1438,7 @@  static bool bdrv_child_cb_attach(BdrvChild *child)
 {
     BlockDriverState *bs = child->opaque;
 
-    assert_bdrv_graph_writable(bs);
+    assert_bdrv_graph_writable();
     QLIST_INSERT_HEAD(&bs->children, child, next);
 
     /* Paired with bdrv_graph_wrlock() in bdrv_replace_child_noperm */
@@ -1466,7 +1466,7 @@  static bool bdrv_child_cb_detach(BdrvChild *child)
     /* Paired with bdrv_graph_wrunlock() in bdrv_replace_child_noperm */
     bdrv_graph_wrlock();
 
-    assert_bdrv_graph_writable(bs);
+    assert_bdrv_graph_writable();
     QLIST_REMOVE(child, next);
 
     return true;
@@ -2885,7 +2885,7 @@  static void bdrv_replace_child_noperm(BdrvChild **childp,
             bdrv_graph_wrlock();
         }
         locked = true;
-        assert_bdrv_graph_writable(old_bs);
+        assert_bdrv_graph_writable();
         QLIST_REMOVE(child, next_parent);
     }
 
@@ -2899,7 +2899,7 @@  static void bdrv_replace_child_noperm(BdrvChild **childp,
             bdrv_graph_wrlock();
             locked = true;
         }
-        assert_bdrv_graph_writable(new_bs);
+        assert_bdrv_graph_writable();
         QLIST_INSERT_HEAD(&new_bs->parents, child, next_parent);
 
         /*
diff --git a/block/graph-lock.c b/block/graph-lock.c
index a2904ff6d8..0edae31545 100644
--- a/block/graph-lock.c
+++ b/block/graph-lock.c
@@ -214,3 +214,14 @@  void coroutine_fn bdrv_graph_co_rdunlock(void)
         aio_wait_kick();
     }
 }
+
+void assert_bdrv_graph_readable(void)
+{
+    assert(qemu_in_main_thread() || reader_count());
+}
+
+void assert_bdrv_graph_writable(void)
+{
+    assert(qemu_in_main_thread());
+    assert(qatomic_read(&has_writer));
+}
diff --git a/include/block/block_int-global-state.h b/include/block/block_int-global-state.h
index 0f21b0570b..5078d6a6ea 100644
--- a/include/block/block_int-global-state.h
+++ b/include/block/block_int-global-state.h
@@ -309,21 +309,4 @@  void bdrv_remove_aio_context_notifier(BlockDriverState *bs,
  */
 void bdrv_drain_all_end_quiesce(BlockDriverState *bs);
 
-/**
- * Make sure that the function is running under both drain and BQL.
- * The latter protects from concurrent writings
- * from the GS API, while the former prevents concurrent reads
- * from I/O.
- */
-static inline void assert_bdrv_graph_writable(BlockDriverState *bs)
-{
-    /*
-     * TODO: this function is incomplete. Because the users of this
-     * assert lack the necessary drains, check only for BQL.
-     * Once the necessary drains are added,
-     * assert also for qatomic_read(&bs->quiesce_counter) > 0
-     */
-    assert(qemu_in_main_thread());
-}
-
 #endif /* BLOCK_INT_GLOBAL_STATE */
diff --git a/include/block/graph-lock.h b/include/block/graph-lock.h
index f171ba0527..2211d41286 100644
--- a/include/block/graph-lock.h
+++ b/include/block/graph-lock.h
@@ -52,5 +52,20 @@  void coroutine_fn bdrv_graph_co_rdlock(void);
  */
 void coroutine_fn bdrv_graph_co_rdunlock(void);
 
+/*
+ * assert_bdrv_graph_readable:
+ * Make sure that the reader is either the main loop,
+ * or there is at least a reader helding the rdlock.
+ * In this way an incoming writer is aware of the read and waits.
+ */
+void assert_bdrv_graph_readable(void);
+
+/*
+ * assert_bdrv_graph_writable:
+ * Make sure that the writer is the main loop and has set @has_writer,
+ * so that incoming readers will pause.
+ */
+void assert_bdrv_graph_writable(void);
+
 #endif /* BLOCK_LOCK_H */