Re: [PATCH v5 04/20] job.c: move inner aiocontext lock in callbacks

[Emanuele Giuseppe Esposito]

On 17/02/2022 14:45, Stefan Hajnoczi wrote:
> On Tue, Feb 08, 2022 at 09:34:57AM -0500, Emanuele Giuseppe Esposito wrote:
>> Instead of having the lock in job_tnx_apply, move it inside
> 
> s/tnx/txn/
> 
>> in the callback. This will be helpful for next commits, when
>> we introduce job_lock/unlock pairs.
>>
>> job_transition_to_pending() and job_needs_finalize() do not
>> need to be protected by the aiocontext lock.
>>
>> No functional change intended.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> ---
>>  job.c | 17 ++++++++++++-----
>>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> I find this hard to review. The patch reduces the scope of the
> AioContext lock region and accesses Job in places where the AioContext
> was previously held. The commit description doesn't explain why it is
> safe to do this.
> 
> I may need to audit the code myself but will try reviewing a few more
> patches first to see if I get the hang of this.
> 
>>
>> diff --git a/job.c b/job.c
>> index f13939d3c6..d8c13ac0d1 100644
>> --- a/job.c
>> +++ b/job.c
>> @@ -149,7 +149,6 @@ static void job_txn_del_job(Job *job)
>>  
>>  static int job_txn_apply(Job *job, int fn(Job *))
>>  {
>> -    AioContext *inner_ctx;
>>      Job *other_job, *next;
>>      JobTxn *txn = job->txn;
>>      int rc = 0;
>> @@ -164,10 +163,7 @@ static int job_txn_apply(Job *job, int fn(Job *))
>>      aio_context_release(job->aio_context);
>>  
>>      QLIST_FOREACH_SAFE(other_job, &txn->jobs, txn_list, next) {
>> -        inner_ctx = other_job->aio_context;
>> -        aio_context_acquire(inner_ctx);
>>          rc = fn(other_job);
>> -        aio_context_release(inner_ctx);
>>          if (rc) {
>>              break;
>>          }
>> @@ -717,11 +713,15 @@ static void job_clean(Job *job)
>>  
>>  static int job_finalize_single(Job *job)
>>  {
>> +    AioContext *ctx = job->aio_context;
>> +
>>      assert(job_is_completed(job));
>>  
>>      /* Ensure abort is called for late-transactional failures */
>>      job_update_rc(job);
>>  
>> +    aio_context_acquire(ctx);
>> +
>>      if (!job->ret) {
>>          job_commit(job);
>>      } else {
>> @@ -729,6 +729,8 @@ static int job_finalize_single(Job *job)
>>      }
>>      job_clean(job);
>>  
>> +    aio_context_release(ctx);
>> +
>>      if (job->cb) {
>>          job->cb(job->opaque, job->ret);
>>      }
>> @@ -833,8 +835,8 @@ static void job_completed_txn_abort(Job *job)
>>              assert(job_cancel_requested(other_job));
>>              job_finish_sync(other_job, NULL, NULL);
>>          }
>> -        job_finalize_single(other_job);
>>          aio_context_release(ctx);
>> +        job_finalize_single(other_job);
>>      }
>>  
>>      /*
>> @@ -849,11 +851,16 @@ static void job_completed_txn_abort(Job *job)
>>  
>>  static int job_prepare(Job *job)
>>  {
>> +    AioContext *ctx = job->aio_context;
>>      assert(qemu_in_main_thread());
>> +
>>      if (job->ret == 0 && job->driver->prepare) {
>> +        aio_context_acquire(ctx);
>>          job->ret = job->driver->prepare(job);
>> +        aio_context_release(ctx);
>>          job_update_rc(job);
>>      }
>> +
>>      return job->ret;
>>  }
>>  
>> -- 
>> 2.31.1
>>

Maybe just for safety this should also go in patch 19, where I enable
job lock/unlock and reduce/remove AioContext locks.

Emanuele