[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH v8 15/20] job: detect change of aiocontext within job coroutine
From: |
Emanuele Giuseppe Esposito |
Subject: |
[PATCH v8 15/20] job: detect change of aiocontext within job coroutine |
Date: |
Wed, 29 Jun 2022 10:15:33 -0400 |
From: Paolo Bonzini <pbonzini@redhat.com>
We want to make sure access of job->aio_context is always done
under either BQL or job_mutex. The problem is that using
aio_co_enter(job->aiocontext, job->co) in job_start and job_enter_cond
makes the coroutine immediately resume, so we can't hold the job lock.
And caching it is not safe either, as it might change.
job_start is under BQL, so it can freely read job->aiocontext, but
job_enter_cond is not. In order to fix this, use aio_co_wake():
the advantage is that it won't use job->aiocontext, but the
main disadvantage is that it won't be able to detect a change of
job AioContext.
Calling bdrv_try_set_aio_context() will issue the following calls
(simplified):
* in terms of bdrv callbacks:
.drained_begin -> .set_aio_context -> .drained_end
* in terms of child_job functions:
child_job_drained_begin -> child_job_set_aio_context -> child_job_drained_end
* in terms of job functions:
job_pause_locked -> job_set_aio_context -> job_resume_locked
We can see that after setting the new aio_context, job_resume_locked
calls again job_enter_cond, which then invokes aio_co_wake(). But
while job->aiocontext has been set in job_set_aio_context,
job->co->ctx has not changed, so the coroutine would be entering in
the wrong aiocontext.
Using aio_co_schedule in job_resume_locked() might seem as a valid
alternative, but the problem is that the bh resuming the coroutine
is not scheduled immediately, and if in the meanwhile another
bdrv_try_set_aio_context() is run (see test_propagate_mirror() in
test-block-iothread.c), we would have the first schedule in the
wrong aiocontext, and the second set of drains won't even manage
to schedule the coroutine, as job->busy would still be true from
the previous job_resume_locked().
The solution is to stick with aio_co_wake(), but then detect every time
the coroutine resumes back from yielding if job->aio_context
has changed. If so, we can reschedule it to the new context.
Check for the aiocontext change in job_do_yield_locked because:
1) aio_co_reschedule_self requires to be in the running coroutine
2) since child_job_set_aio_context allows changing the aiocontext only
while the job is paused, this is the exact place where the coroutine
resumes, before running JobDriver's code.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
job.c | 22 +++++++++++++++++++---
1 file changed, 19 insertions(+), 3 deletions(-)
diff --git a/job.c b/job.c
index 19d711dc73..8db80b8086 100644
--- a/job.c
+++ b/job.c
@@ -581,11 +581,12 @@ void job_enter_cond_locked(Job *job, bool(*fn)(Job *job))
return;
}
- assert(!job->deferred_to_main_loop);
timer_del(&job->sleep_timer);
job->busy = true;
real_job_unlock();
- aio_co_enter(job->aio_context, job->co);
+ job_unlock();
+ aio_co_wake(job->co);
+ job_lock();
}
void job_enter_cond(Job *job, bool(*fn)(Job *job))
@@ -611,6 +612,8 @@ void job_enter(Job *job)
*/
static void coroutine_fn job_do_yield_locked(Job *job, uint64_t ns)
{
+ AioContext *next_aio_context;
+
real_job_lock();
if (ns != -1) {
timer_mod(&job->sleep_timer, ns);
@@ -622,7 +625,20 @@ static void coroutine_fn job_do_yield_locked(Job *job,
uint64_t ns)
qemu_coroutine_yield();
job_lock();
- /* Set by job_enter_cond() before re-entering the coroutine. */
+ next_aio_context = job->aio_context;
+ /*
+ * Coroutine has resumed, but in the meanwhile the job AioContext
+ * might have changed via bdrv_try_set_aio_context(), so we need to move
+ * the coroutine too in the new aiocontext.
+ */
+ while (qemu_get_current_aio_context() != next_aio_context) {
+ job_unlock();
+ aio_co_reschedule_self(next_aio_context);
+ job_lock();
+ next_aio_context = job->aio_context;
+ }
+
+ /* Set by job_enter_cond_locked() before re-entering the coroutine. */
assert(job->busy);
}
--
2.31.1
- [PATCH v8 07/20] job.h: add _locked public functions, (continued)
- [PATCH v8 07/20] job.h: add _locked public functions, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 01/20] job.c: make job_mutex and job_lock/unlock() public, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 09/20] blockjob: rename notifier callbacks as _locked, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 04/20] aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 02/20] job.h: categorize fields in struct Job, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 10/20] jobs: add job lock in find_* functions, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 06/20] job.h: define functions called without job lock held, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 11/20] jobs: use job locks also in the unit tests, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 08/20] blockjob.h: introduce block_job _locked() APIs, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 03/20] job.c: API functions not used outside should be static, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 15/20] job: detect change of aiocontext within job coroutine,
Emanuele Giuseppe Esposito <=
- [PATCH v8 20/20] job: remove unused functions, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 17/20] job.c: enable job lock/unlock and remove Aiocontext locks, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 13/20] jobs: group together API calls under the same job lock, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 05/20] job.c: add job_lock/unlock while keeping job.h intact, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 18/20] block_job_query: remove atomic read, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 19/20] blockjob: remove unused functions, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 12/20] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 16/20] jobs: protect job.aio_context with BQL and job_mutex, Emanuele Giuseppe Esposito, 2022/06/29
- [PATCH v8 14/20] commit and mirror: create new nodes using bdrv_get_aio_context, and not the job aiocontext, Emanuele Giuseppe Esposito, 2022/06/29