[PATCH] Fix data races in test-bdrv-drain test

[PATCH] Fix data races in test-bdrv-drain test

[Vitalii Mordan]

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 <mordan@ispras.ru>
---
 include/qemu/job.h           |  2 ++
 job.c                        |  6 ++++++
 tests/unit/test-bdrv-drain.c | 20 ++++++++++----------
 3 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/include/qemu/job.h b/include/qemu/job.h
index 2b873f2576..f27551a9ad 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -520,6 +520,8 @@ bool job_is_internal(Job *job);
  */
 bool job_is_cancelled(Job *job);
 
+bool job_is_paused(Job *job);
+
 /* Same as job_is_cancelled(), but called with job lock held. */
 bool job_is_cancelled_locked(Job *job);
 
diff --git a/job.c b/job.c
index 660ce22c56..d9b2dd8532 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..65041c9230 100644
--- a/tests/unit/test-bdrv-drain.c
+++ b/tests/unit/test-bdrv-drain.c
@@ -667,10 +667,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 +685,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 +791,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);
         }
     }
@@ -825,7 +825,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 +858,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);
         }
     }
@@ -1422,7 +1422,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 +1541,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);
@@ -1565,7 +1565,7 @@ 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 +1700,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);
-- 
2.34.1

Re: [PATCH] Fix data races in test-bdrv-drain test

[Kevin Wolf]

Am 02.04.2025 um 12:21 hat Vitalii Mordan geschrieben:
> 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 <mordan@ispras.ru>

Considering that we're nearing the end of the code freeze for 10.0, I
fixed up a few trivial problems myself instead of asking for a v2 (see
diff below).

Thanks, applied to the block branch.

Kevin

diff --git a/include/qemu/job.h b/include/qemu/job.h
index f27551a9ad..a5a04155ea 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -520,8 +520,6 @@ bool job_is_internal(Job *job);
  */
 bool job_is_cancelled(Job *job);

-bool job_is_paused(Job *job);
-
 /* Same as job_is_cancelled(), but called with job lock held. */
 bool job_is_cancelled_locked(Job *job);

@@ -547,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 d9b2dd8532..0653bc2ba6 100644
--- a/job.c
+++ b/job.c
@@ -253,8 +253,8 @@ bool job_is_cancelled_locked(Job *job)

 bool job_is_paused(Job *job)
 {
-	JOB_LOCK_GUARD();
-	return job->paused;
+    JOB_LOCK_GUARD();
+    return job->paused;
 }

 bool job_is_cancelled(Job *job)
diff --git a/tests/unit/test-bdrv-drain.c b/tests/unit/test-bdrv-drain.c
index 65041c9230..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;
@@ -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() */
     }

@@ -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)
@@ -1557,8 +1561,10 @@ 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)