[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 14/16] job.c: use job_get_aio_context()
From: |
Paolo Bonzini |
Subject: |
Re: [PATCH v3 14/16] job.c: use job_get_aio_context() |
Date: |
Wed, 19 Jan 2022 11:31:28 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.0 |
Getters such as job_get_aio_context are often wrong because the
AioContext can change immediately after returning.
So, I wonder if job.aio_context should be protected with a kind of "fake
rwlock": read under BQL or job_lock, write under BQL+job_lock. For this
to work, you can add an assertion for qemu_in_main_thread() to
child_job_set_aio_ctx, or even better have the assertion in a wrapper
API job_set_aio_context_locked().
And then, we can remove job_get_aio_context().
Let's look at all cases individually:
On 1/5/22 15:02, Emanuele Giuseppe Esposito wrote:
diff --git a/block/commit.c b/block/commit.c
index f639eb49c5..961b57edf0 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -369,7 +369,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
goto fail;
}
- s->base = blk_new(s->common.job.aio_context,
+ s->base = blk_new(job_get_aio_context(&s->common.job),
base_perms,
BLK_PERM_CONSISTENT_READ
| BLK_PERM_GRAPH_MOD
Here the AioContext is the one of bs. It cannot change because we're
under BQL. Replace with bdrv_get_aio_context.
@@ -382,7 +382,7 @@ void commit_start(const char *job_id, BlockDriverState *bs,
s->base_bs = base;
/* Required permissions are already taken with block_job_add_bdrv() */
- s->top = blk_new(s->common.job.aio_context, 0, BLK_PERM_ALL);
+ s->top = blk_new(job_get_aio_context(&s->common.job), 0, BLK_PERM_ALL);
ret = blk_insert_bs(s->top, top, errp);
if (ret < 0) {
goto fail;
Same.
diff --git a/block/mirror.c b/block/mirror.c
index 41450df55c..72b4367b4e 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1743,7 +1743,7 @@ static BlockJob *mirror_start_job(
target_perms |= BLK_PERM_GRAPH_MOD;
}
- s->target = blk_new(s->common.job.aio_context,
+ s->target = blk_new(job_get_aio_context(&s->common.job),
target_perms, target_shared_perms);
ret = blk_insert_bs(s->target, target, errp);
if (ret < 0) {
Same.
diff --git a/block/replication.c b/block/replication.c
index 50ea778937..68018948b9 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -148,8 +148,8 @@ static void replication_close(BlockDriverState *bs)
}
if (s->stage == BLOCK_REPLICATION_FAILOVER) {
commit_job = &s->commit_job->job;
- assert(commit_job->aio_context == qemu_get_current_aio_context());
WITH_JOB_LOCK_GUARD() {
+ assert(commit_job->aio_context == qemu_get_current_aio_context());
job_cancel_sync_locked(commit_job, false);
}
}
Ok.
diff --git a/blockjob.c b/blockjob.c
index cf1f49f6c2..468ba735c5 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -155,14 +155,16 @@ static void child_job_set_aio_ctx(BdrvChild *c,
AioContext *ctx,
bdrv_set_aio_context_ignore(sibling->bs, ctx, ignore);
}
- job->job.aio_context = ctx;
+ WITH_JOB_LOCK_GUARD() {
+ job->job.aio_context = ctx;
+ }
}
static AioContext *child_job_get_parent_aio_context(BdrvChild *c)
{
BlockJob *job = c->opaque;
- return job->job.aio_context;
+ return job_get_aio_context(&job->job);
}
static const BdrvChildClass child_job = {
Both called with BQL held, I think.
@@ -218,19 +220,21 @@ int block_job_add_bdrv(BlockJob *job, const char *name,
BlockDriverState *bs,
{
BdrvChild *c;
bool need_context_ops;
+ AioContext *job_aiocontext;
assert(qemu_in_main_thread());
bdrv_ref(bs);
- need_context_ops = bdrv_get_aio_context(bs) != job->job.aio_context;
+ job_aiocontext = job_get_aio_context(&job->job);
+ need_context_ops = bdrv_get_aio_context(bs) != job_aiocontext;
- if (need_context_ops && job->job.aio_context != qemu_get_aio_context()) {
- aio_context_release(job->job.aio_context);
+ if (need_context_ops && job_aiocontext != qemu_get_aio_context()) {
+ aio_context_release(job_aiocontext);
}
c = bdrv_root_attach_child(bs, name, &child_job, 0, perm, shared_perm,
job,
errp);
- if (need_context_ops && job->job.aio_context != qemu_get_aio_context()) {
- aio_context_acquire(job->job.aio_context);
+ if (need_context_ops && job_aiocontext != qemu_get_aio_context()) {
+ aio_context_acquire(job_aiocontext);
}
if (c == NULL) {
return -EPERM;
BQL held, too.
diff --git a/job.c b/job.c
index f16a4ef542..8a5b710d9b 100644
--- a/job.c
+++ b/job.c
@@ -566,7 +566,7 @@ void job_enter_cond_locked(Job *job, bool(*fn)(Job *job))
job->busy = true;
real_job_unlock();
job_unlock();
- aio_co_enter(job->aio_context, job->co);
+ aio_co_enter(job_get_aio_context(job), job->co);
job_lock();
}
If you replace aio_co_enter with aio_co_schedule, you can call it
without dropping the lock. The difference being that aio_co_schedule
will always go through a bottom half.
@@ -1138,7 +1138,6 @@ static void coroutine_fn job_co_entry(void *opaque)
Job *job = opaque;
int ret;
- assert(job->aio_context == qemu_get_current_aio_context());
assert(job && job->driver && job->driver->run);
job_pause_point(job);
ret = job->driver->run(job, &job->err);
@@ -1177,7 +1176,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);
+ aio_co_enter(job_get_aio_context(job), job->co);
Better to use aio_co_schedule here, too, and move it under the previous
WITH_JOB_LOCK_GUARD.
}
void job_cancel_locked(Job *job, bool force)
@@ -1303,7 +1302,8 @@ int job_finish_sync_locked(Job *job, void (*finish)(Job
*, Error **errp),
}
job_unlock();
- AIO_WAIT_WHILE(job->aio_context, (job_enter(job), !job_is_completed(job)));
+ AIO_WAIT_WHILE(job_get_aio_context(job),
+ (job_enter(job), !job_is_completed(job)));
job_lock();
Here I think we are also holding the BQL, because this function is
"sync", so it's safe to use job->aio_context.
Paolo
- Re: [PATCH v3 02/16] job.h: categorize fields in struct Job, (continued)
- [PATCH v3 05/16] block/mirror.c: use of job helpers in drivers to avoid TOC/TOU, Emanuele Giuseppe Esposito, 2022/01/05
- [PATCH v3 08/16] aio-wait.h: introduce AIO_WAIT_WHILE_UNLOCKED, Emanuele Giuseppe Esposito, 2022/01/05
- [PATCH v3 07/16] job.c: move inner aiocontext lock in callbacks, Emanuele Giuseppe Esposito, 2022/01/05
- [PATCH v3 11/16] jobs: document all static functions and add _locked() suffix, Emanuele Giuseppe Esposito, 2022/01/05
- [PATCH v3 06/16] job.c: make job_event_* functions static, Emanuele Giuseppe Esposito, 2022/01/05
- [PATCH v3 15/16] job.c: enable job lock/unlock and remove Aiocontext locks, Emanuele Giuseppe Esposito, 2022/01/05
- [PATCH v3 14/16] job.c: use job_get_aio_context(), Emanuele Giuseppe Esposito, 2022/01/05
[PATCH v3 13/16] jobs: add job lock in find_* functions, Emanuele Giuseppe Esposito, 2022/01/05
[PATCH v3 10/16] jobs: protect jobs with job_lock/unlock, Emanuele Giuseppe Esposito, 2022/01/05
[PATCH v3 16/16] block_job_query: remove atomic read, Emanuele Giuseppe Esposito, 2022/01/05