[PULL,4/4] test-bdrv-drain: Fix data races

Commit Message

Kevin Wolf April 8, 2025, 1 p.m. UTC

From: Vitalii Mordan <[e-mail]>

This patch addresses potential data races involving access to Job fields
in the test-bdrv-drain test.

Fixes: 7253220de4 ("test-bdrv-drain: Test drain vs. block jobs")
Resolves: https://gitlab.com/qemu-project/qemu/-/issues/2900
Signed-off-by: Vitalii Mordan <[e-mail]>
Message-ID: <[e-mail]>
[kwolf: Fixed up coding style and one missing atomic access]
Reviewed-by: Kevin Wolf <[e-mail]>
Signed-off-by: Kevin Wolf <[e-mail]>
---
 include/qemu/job.h           |  3 +++
 job.c                        |  6 ++++++
 tests/unit/test-bdrv-drain.c | 32 +++++++++++++++++++-------------
 3 files changed, 28 insertions(+), 13 deletions(-)

Patch

diff --git a/include/qemu/job.h b/include/qemu/job.h
index 2b873f2576..a5a04155ea 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -545,6 +545,9 @@  bool job_is_ready(Job *job);
 /* Same as job_is_ready(), but called with job lock held. */
 bool job_is_ready_locked(Job *job);
 
+/** Returns whether the job is paused. Called with job_mutex *not* held. */
+bool job_is_paused(Job *job);
+
 /**
  * Request @job to pause at the next pause point. Must be paired with
  * job_resume(). If the job is supposed to be resumed by user action, call
diff --git a/job.c b/job.c
index 660ce22c56..0653bc2ba6 100644
--- a/job.c
+++ b/job.c
@@ -251,6 +251,12 @@  bool job_is_cancelled_locked(Job *job)
     return job->force_cancel;
 }
 
+bool job_is_paused(Job *job)
+{
+    JOB_LOCK_GUARD();
+    return job->paused;
+}
+
 bool job_is_cancelled(Job *job)
 {
     JOB_LOCK_GUARD();
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 7410e6f352..290cd2a70e 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -632,6 +632,8 @@  typedef struct TestBlockJob {
     BlockDriverState *bs;
     int run_ret;
     int prepare_ret;
+
+    /* Accessed with atomics */
     bool running;
     bool should_complete;
 } TestBlockJob;
@@ -667,10 +669,10 @@  static int coroutine_fn test_job_run(Job *job, Error **errp)
 
     /* We are running the actual job code past the pause point in
      * job_co_entry(). */
-    s->running = true;
+    qatomic_set(&s->running, true);
 
     job_transition_to_ready(&s->common.job);
-    while (!s->should_complete) {
+    while (!qatomic_read(&s->should_complete)) {
         /* Avoid job_sleep_ns() because it marks the job as !busy. We want to
          * emulate some actual activity (probably some I/O) here so that drain
          * has to wait for this activity to stop. */
@@ -685,7 +687,7 @@  static int coroutine_fn test_job_run(Job *job, Error **errp)
 static void test_job_complete(Job *job, Error **errp)
 {
     TestBlockJob *s = container_of(job, TestBlockJob, common.job);
-    s->should_complete = true;
+    qatomic_set(&s->should_complete, true);
 }
 
 BlockJobDriver test_job_driver = {
@@ -791,7 +793,7 @@  static void test_blockjob_common_drain_node(enum drain_type drain_type,
         /* job_co_entry() is run in the I/O thread, wait for the actual job
          * code to start (we don't want to catch the job in the pause point in
          * job_co_entry(). */
-        while (!tjob->running) {
+        while (!qatomic_read(&tjob->running)) {
             aio_poll(qemu_get_aio_context(), false);
         }
     }
@@ -799,7 +801,7 @@  static void test_blockjob_common_drain_node(enum drain_type drain_type,
     WITH_JOB_LOCK_GUARD() {
         g_assert_cmpint(job->job.pause_count, ==, 0);
         g_assert_false(job->job.paused);
-        g_assert_true(tjob->running);
+        g_assert_true(qatomic_read(&tjob->running));
         g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */
     }
 
@@ -825,7 +827,7 @@  static void test_blockjob_common_drain_node(enum drain_type drain_type,
          *
          * paused is reset in the I/O thread, wait for it
          */
-        while (job->job.paused) {
+        while (job_is_paused(&job->job)) {
             aio_poll(qemu_get_aio_context(), false);
         }
     }
@@ -858,7 +860,7 @@  static void test_blockjob_common_drain_node(enum drain_type drain_type,
          *
          * paused is reset in the I/O thread, wait for it
          */
-        while (job->job.paused) {
+        while (job_is_paused(&job->job)) {
             aio_poll(qemu_get_aio_context(), false);
         }
     }
@@ -1411,10 +1413,12 @@  static void test_set_aio_context(void)
 
 typedef struct TestDropBackingBlockJob {
     BlockJob common;
-    bool should_complete;
     bool *did_complete;
     BlockDriverState *detach_also;
     BlockDriverState *bs;
+
+    /* Accessed with atomics */
+    bool should_complete;
 } TestDropBackingBlockJob;
 
 static int coroutine_fn test_drop_backing_job_run(Job *job, Error **errp)
@@ -1422,7 +1426,7 @@  static int coroutine_fn test_drop_backing_job_run(Job *job, Error **errp)
     TestDropBackingBlockJob *s =
         container_of(job, TestDropBackingBlockJob, common.job);
 
-    while (!s->should_complete) {
+    while (!qatomic_read(&s->should_complete)) {
         job_sleep_ns(job, 0);
     }
 
@@ -1541,7 +1545,7 @@  static void test_blockjob_commit_by_drained_end(void)
 
     job_start(&job->common.job);
 
-    job->should_complete = true;
+    qatomic_set(&job->should_complete, true);
     bdrv_drained_begin(bs_child);
     g_assert(!job_has_completed);
     bdrv_drained_end(bs_child);
@@ -1557,15 +1561,17 @@  static void test_blockjob_commit_by_drained_end(void)
 
 typedef struct TestSimpleBlockJob {
     BlockJob common;
-    bool should_complete;
     bool *did_complete;
+
+    /* Accessed with atomics */
+    bool should_complete;
 } TestSimpleBlockJob;
 
 static int coroutine_fn test_simple_job_run(Job *job, Error **errp)
 {
     TestSimpleBlockJob *s = container_of(job, TestSimpleBlockJob, common.job);
 
-    while (!s->should_complete) {
+    while (!qatomic_read(&s->should_complete)) {
         job_sleep_ns(job, 0);
     }
 
@@ -1700,7 +1706,7 @@  static void test_drop_intermediate_poll(void)
     job->did_complete = &job_has_completed;
 
     job_start(&job->common.job);
-    job->should_complete = true;
+    qatomic_set(&job->should_complete, true);
 
     g_assert(!job_has_completed);
     ret = bdrv_drop_intermediate(chain[1], chain[0], NULL, false);