Re: [PATCH v3 14/16] job.c: use job_get_aio_context()

[Emanuele Giuseppe Esposito <eesposit@redhat.com>]

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;
}

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