All accesses of bs->quiesce_counter are in the main thread, either
after a GLOBAL_STATE_CODE() macro or in a function with GRAPH_WRLOCK
annotation.

This is essentially a revert of 414c2ec358 ("block: access
quiesce_counter with atomic ops"). At that time, neither the
GLOBAL_STATE_CODE() macro nor the GRAPH_WRLOCK annotation existed.
Even if the field was only accessed in the main thread back then (did
not check if that is actually the case), it wouldn't have been easy to
verify.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 block/io.c                       | 7 ++-----
 include/block/block_int-common.h | 2 +-
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/block/io.c b/block/io.c
index ac5c7174f6..9bd8ba8431 100644
--- a/block/io.c
+++ b/block/io.c
@@ -361,7 +361,7 @@ static void bdrv_do_drained_begin(BlockDriverState *bs, BdrvChild *parent,
     GLOBAL_STATE_CODE();
 
     /* Stop things in parent-to-child order */
-    if (qatomic_fetch_inc(&bs->quiesce_counter) == 0) {
+    if (bs->quiesce_counter++ == 0) {
         GRAPH_RDLOCK_GUARD_MAINLOOP();
         bdrv_parent_drained_begin(bs, parent);
         if (bs->drv && bs->drv->bdrv_drain_begin) {
@@ -401,8 +401,6 @@ bdrv_drained_begin(BlockDriverState *bs)
  */
 static void bdrv_do_drained_end(BlockDriverState *bs, BdrvChild *parent)
 {
-    int old_quiesce_counter;
-
     IO_OR_GS_CODE();
 
     if (qemu_in_coroutine()) {
@@ -415,8 +413,7 @@ static void bdrv_do_drained_end(BlockDriverState *bs, BdrvChild *parent)
     assert(bs->quiesce_counter > 0);
 
     /* Re-enable things in child-to-parent order */
-    old_quiesce_counter = qatomic_fetch_dec(&bs->quiesce_counter);
-    if (old_quiesce_counter == 1) {
+    if (--bs->quiesce_counter == 0) {
         GRAPH_RDLOCK_GUARD_MAINLOOP();
         if (bs->drv && bs->drv->bdrv_drain_end) {
             bs->drv->bdrv_drain_end(bs);
diff --git a/include/block/block_int-common.h b/include/block/block_int-common.h
index 925a3e7353..e96c6a6a03 100644
--- a/include/block/block_int-common.h
+++ b/include/block/block_int-common.h
@@ -1253,7 +1253,7 @@ struct BlockDriverState {
     /* do we need to tell the quest if we have a volatile write cache? */
     int enable_write_cache;
 
-    /* Accessed with atomic ops.  */
+    /* Accessed only in the main thread. */
     int quiesce_counter;
 
     unsigned int write_gen;               /* Current data generation */
-- 
2.39.5

Am 30.05.25 um 17:11 schrieb Fiona Ebner:
> All accesses of bs->quiesce_counter are in the main thread, either
> after a GLOBAL_STATE_CODE() macro or in a function with GRAPH_WRLOCK
> annotation.

Now I wonder if that is actually good enough? Because vCPUs threads can
also satisfy the qemu_in_main_thread() condition.

Am 02.06.2025 um 16:45 hat Fiona Ebner geschrieben:
> Am 30.05.25 um 17:11 schrieb Fiona Ebner:
> > All accesses of bs->quiesce_counter are in the main thread, either
> > after a GLOBAL_STATE_CODE() macro or in a function with GRAPH_WRLOCK
> > annotation.
> 
> Now I wonder if that is actually good enough? Because vCPUs threads can
> also satisfy the qemu_in_main_thread() condition.

It is. vcpu threads can only act as the main thread while holding the
BQL, so in this case you already have the necessary synchronisation
without atomics. Basically all the otherwise non-thread-safe code
running in the main loop depends on this.

Kevin