[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[PATCH v4 16/19] job: detect change of aiocontext within job coroutine
From: |
Emanuele Giuseppe Esposito |
Subject: |
[PATCH v4 16/19] job: detect change of aiocontext within job coroutine |
Date: |
Fri, 28 Jan 2022 06:50:28 -0500 |
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.
As a small improvement, we don't need aio_co_enter() in job_start(),
we can simplify it with qemu_coroutine_enter(). If the aio_context
where it is running is wrong, job_do_yield_locked() will automatically
reschedule the coroutine where it belongs, once the first
job_pause_point is triggered in job_co_entry.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
job.c | 24 ++++++++++++++++++++----
1 file changed, 20 insertions(+), 4 deletions(-)
diff --git a/job.c b/job.c
index 468a387390..bce76b21d8 100644
--- a/job.c
+++ b/job.c
@@ -532,11 +532,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(Job *job)
@@ -556,6 +557,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);
@@ -567,6 +570,20 @@ static void coroutine_fn job_do_yield_locked(Job *job,
uint64_t ns)
qemu_coroutine_yield();
job_lock();
+ 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);
}
@@ -666,7 +683,6 @@ void job_resume_locked(Job *job)
if (job->pause_count) {
return;
}
-
/* kick only if no timer is pending */
job_enter_cond_locked(job, job_timer_not_pending);
}
@@ -1145,7 +1161,7 @@ void job_start(Job *job)
job->paused = false;
job_state_transition_locked(job, JOB_STATUS_RUNNING);
}
- aio_co_enter(job->aio_context, job->co);
+ qemu_coroutine_enter(job->co);
}
void job_cancel_locked(Job *job, bool force)
--
2.31.1
- [PATCH v4 00/19] job: replace AioContext lock with job_mutex, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 01/19] job.c: make job_mutex and job_lock/unlock() public, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 02/19] job.h: categorize fields in struct Job, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 04/19] job.c: move inner aiocontext lock in callbacks, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 03/19] job.c: make job_event_* functions static, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 05/19] aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 07/19] jobs: protect jobs with job_lock/unlock, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 14/19] blockjob: block_job_get_aio_context is a GS function, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 06/19] jobs: remove aiocontext locks since the functions are under BQL, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 09/19] jobs: use job locks also in the unit tests, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 16/19] job: detect change of aiocontext within job coroutine,
Emanuele Giuseppe Esposito <=
- [PATCH v4 12/19] job.h: define unlocked functions, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 17/19] jobs: protect job.aio_context with BQL and job_mutex, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 18/19] job.c: enable job lock/unlock and remove Aiocontext locks, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 19/19] block_job_query: remove atomic read, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 11/19] jobs: document all static functions and add _locked() suffix, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 08/19] jobs: add job lock in find_* functions, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 13/19] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 15/19] commit and mirror: create new nodes using bdrv_get_aio_context, and not the job aiocontext, Emanuele Giuseppe Esposito, 2022/01/28
- [PATCH v4 10/19] job.h: define locked functions, Emanuele Giuseppe Esposito, 2022/01/28