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 <mor...@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)