On 21/01/2022 13:33, Emanuele Giuseppe Esposito wrote:
On 19/01/2022 11:31, Paolo Bonzini wrote:
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.
Yes, as their callbacks .get_parent_aio_context and .set_aio_context are
defined as GS functions in block_int-common.h
@@ -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.
Wouldn't it be better to keep job_get_aio_context and implement like this:
AioContext *job_get_aio_context(Job *job)
{
/*
* Job AioContext can be written under BQL+job_mutex,
* but can be read with just the BQL held.
*/
assert(qemu_in_main_thread());
return job->aio_context;
}
Uhm ok this one doesn't really work, because it's ok to read it under
either BQL or job lock. So I will get rid of job_get_aio_context, but
add job_set_aio_context (and use it in child_job_set_aio_ctx).
Emanuele
and instead job_set_aio_context:
void job_set_aio_context(Job *job, AioContext *ctx)
{
JOB_LOCK_GUARD();
assert(qemu_in_main_thread());
job->aio_context = ctx;
}
(obviously implement also _locked version, if needed, and probably move
the comment in get_aio_context in job.h).
Thank you,
Emanuele
