Re: [PATCH v10 18/21] job.c: enable job lock/unlock and remove Aiocontext locks

[Emanuele Giuseppe Esposito]

Am 05/08/2022 um 15:01 schrieb Kevin Wolf:
> Am 25.07.2022 um 09:38 hat Emanuele Giuseppe Esposito geschrieben:
>> Change the job_{lock/unlock} and macros to use job_mutex.
>>
>> Now that they are not nop anymore, remove the aiocontext
>> to avoid deadlocks.
> 
> Okay, so this is the big bad patch where we need to verify the
> completeness of all changes made so far. Let's see...
> 
>> Therefore:
>> - when possible, remove completely the aiocontext lock/unlock pair
>> - if it is used by some other function too, reduce the locking
>>   section as much as possible, leaving the job API outside.
>> - change AIO_WAIT_WHILE in AIO_WAIT_WHILE_UNLOCKED, since we
>>   are not using the aiocontext lock anymore
> 
> Does this imply that there is a new rule that job_*() must not be called
> with the AioContext lock held? Or is it optional now?
> 
> If it's a rule, it should be documented.
> 
> 
> (Coming back after reviewing more of the patch:)
> 
> 
> It doesn't seem to be a rule, or at least not a rule that is obeyed.
> 
> Actually each function in job.c belongs in one of at most three
> categories: Must hold the AioContext (because it calls callbacks that
> need it), may hold the AioContext optionally, and must not hold it
> (everything that would cause the deadlocks you're alluding to, but not
> explaining, in the commit message).
> 
> It is not obvious which function is in which category. So I maintain
> that we need some documentation for the assumptions made.
> 
> All coroutine_fns (which are called from the job coroutine) should be in
> the category that they still run with the AioContext locked, but that
> covers only a small minority of functions.
> 
> The driver callbacks look mostly correct at least with respect to
> AioContext locking, even if their status isn't documented. I suppose
> this is the most important part.
> 

Well I would say the general case is that no function should need the
aiocontext lock taken. The only ones that need the aiocontext lock are
the functions currently taking it, so just the driver callbacks. But
they take it internally, so what do you propose to document here?

After looking at your feedbacks, I think we just need the driver
callbacks to be documented, right?


>> @@ -197,23 +186,14 @@ static int job_txn_apply_locked(Job *job, int fn(Job 
>> *))
>>       * break AIO_WAIT_WHILE from within fn.
>>       */
>>      job_ref_locked(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);
> 
> Okay, so fn() is now called without the AioContext lock while it was
> called with it previously. This requires checking all callers.
> 
> What isn't immediately clear, but seems to be true, is that all notifiers
> don't need the AioContext lock. Probably worth documenting in struct
> Job. (Needed because of job_transition_to_pending_locked(), which is
> passed as fn.)

I am not sure what you mean here, but all fn() passed take the
AioContext lock internally if they need to take it (ie just for the
drivers callback). Reading also below comment, I will add the comment
"Called with AioContext lock held, since many callback implementations
use bdrv_* functions that require to hold the lock." to the following
callbacks:
- prepare
- free
- commit
- abort
- clean
- cancel

Sounds good?

> 
>>          if (rc) {
>>              break;
>>          }
>>      }
>>  
>> -    /*
>> -     * Note that job->aio_context might have been changed by calling fn, so 
>> we
>> -     * can't use a local variable to cache it.
>> -     */
>> -    aio_context_acquire(job->aio_context);
>>      job_unref_locked(job);
>>      return rc;
>>  }
>> @@ -501,8 +481,12 @@ void job_unref_locked(Job *job)
>>          assert(!job->txn);
>>  
>>          if (job->driver->free) {
>> +            AioContext *aio_context = job->aio_context;
>>              job_unlock();
>> +            /* FIXME: aiocontext lock is required because cb calls 
>> blk_unref */
>> +            aio_context_acquire(aio_context);
>>              job->driver->free(job);
>> +            aio_context_release(aio_context);
> 
> The documentation of JobDriver doesn't specify which callbacks are
> called with the AioContext locked and which are called without it. It
> probably should.
> 
> (A good part of the documentation clarifications I'm asking for in this
> review should probably done in a patch before this one, so that
> reviewing the documentation involves checking that the requirements of
> the callback match what we're documenting, and then the review of this
> patch can focus on that the documented contract is still obeyed.)

Makes sense, I'll do what I stated in the answer above.

> 
>>              job_lock();
>>          }
>>  
>> @@ -922,6 +900,7 @@ static void job_clean(Job *job)
>>  static int job_finalize_single_locked(Job *job)
>>  {
>>      int job_ret;
>> +    AioContext *ctx = job->aio_context;
>>  
>>      assert(job_is_completed_locked(job));
>>  
>> @@ -929,6 +908,7 @@ static int job_finalize_single_locked(Job *job)
>>      job_update_rc_locked(job);
>>  
>>      job_unlock();
>> +    aio_context_acquire(ctx);
>>  
>>      if (!job->ret) {
>>          job_commit(job);
>> @@ -937,6 +917,7 @@ static int job_finalize_single_locked(Job *job)
>>      }
>>      job_clean(job);
>>  
>> +    aio_context_release(ctx);
>>      job_lock();
> 
> Let's add comments to job_commit(), job_abort() and job_clean() that
> they are called with the AioContext lock held.
> 
> A few lines below we are now calling job->cb() without the AioContext
> lock even though previously it was called with it. Which way is right?
> The intended behaviour should be documented in struct Job.

I think we unfortunately need the aiocontext lock here too.

backup_job_create -> passes cb to block_job_create -> passes it to
job_create

cb is
backup_job_completed -> calls backup_job_cleanup -> calls
reopen_backing_file

Which has subtree drains and so on. So yes, I will wrap cb under
aiocontext lock, merging it in the same aiocontext lock section of
job_commit and friends.

> 
>>      if (job->cb) {
>> @@ -1002,7 +983,6 @@ static void job_cancel_async_locked(Job *job, bool 
>> force)
>>  /* Called with job_mutex held, but releases it temporarily. */
>>  static void job_completed_txn_abort_locked(Job *job)
>>  {
>> -    AioContext *ctx;
>>      JobTxn *txn = job->txn;
>>      Job *other_job;
>>  
>> @@ -1015,54 +995,31 @@ static void job_completed_txn_abort_locked(Job *job)
>>      txn->aborting = true;
>>      job_txn_ref_locked(txn);
>>  
>> -    /*
>> -     * We can only hold the single job's AioContext lock while calling
>> -     * job_finalize_single() because the finalization callbacks can involve
>> -     * calls of AIO_WAIT_WHILE(), which could deadlock otherwise.
>> -     * Note that the job's AioContext may change when it is finalized.
>> -     */
>>      job_ref_locked(job);
>> -    aio_context_release(job->aio_context);
>>  
>>      /* Other jobs are effectively cancelled by us, set the status for
>>       * them; this job, however, may or may not be cancelled, depending
>>       * on the caller, so leave it. */
>>      QLIST_FOREACH(other_job, &txn->jobs, txn_list) {
>>          if (other_job != job) {
>> -            ctx = other_job->aio_context;
>> -            aio_context_acquire(ctx);
>>              /*
>>               * This is a transaction: If one job failed, no result will 
>> matter.
>>               * Therefore, pass force=true to terminate all other jobs as 
>> quickly
>>               * as possible.
>>               */
>>              job_cancel_async_locked(other_job, true);
> 
> job_cancel_async_locked() calls job->driver->cancel() without the
> AioContext lock now. Some implementations call bdrv_cancel_in_flight().
> Generally bdrv_*() are called with the AioContext lock held.
> 
> If we want to make bdrv_cancel_in_flight() an exception, at the very
> least this need to be documented.
> 
> The more obvious solution would be to acquire the AioContext lock in
> job_cancel_async_locked() around the callback.
> 

Added to the above list of callbacks to document.


>> diff --git a/tests/unit/test-blockjob.c b/tests/unit/test-blockjob.c
>> index b0cd06c529..8a9350078f 100644
>> --- a/tests/unit/test-blockjob.c
>> +++ b/tests/unit/test-blockjob.c
>> @@ -228,10 +228,6 @@ static void cancel_common(CancelJob *s)
>>      BlockJob *job = &s->common;
>>      BlockBackend *blk = s->blk;
>>      JobStatus sts = job->job.status;
>> -    AioContext *ctx;
>> -
>> -    ctx = job->job.aio_context;
>> -    aio_context_acquire(ctx);
>>  
>>      job_cancel_sync(&job->job, true);
>>      WITH_JOB_LOCK_GUARD() {
>> @@ -244,7 +240,6 @@ static void cancel_common(CancelJob *s)
>>      }
>>      destroy_blk(blk);
>>  
>> -    aio_context_release(ctx);
> 
> destroy_blk() requires the AioContext to be locked.

Makes sense.
> 
>>  }
>>  
>>  static void test_cancel_created(void)
>> @@ -384,12 +379,10 @@ static void test_cancel_concluded(void)
>>      aio_poll(qemu_get_aio_context(), true);
>>      assert_job_status_is(job, JOB_STATUS_PENDING);
>>  
>> -    aio_context_acquire(job->aio_context);
>>      WITH_JOB_LOCK_GUARD() {
>>          job_finalize_locked(job, &error_abort);
>> +        assert(job->status == JOB_STATUS_CONCLUDED);
>>      }
>> -    aio_context_release(job->aio_context);
>> -    assert_job_status_is(job, JOB_STATUS_CONCLUDED);
>>  
>>      cancel_common(s);
>>  }
>> @@ -482,13 +475,11 @@ static void test_complete_in_standby(void)
>>  
>>      /* Wait for the job to become READY */
>>      job_start(job);
>> -    aio_context_acquire(ctx);
>>      /*
>>       * Here we are waiting for the status to change, so don't bother
>>       * protecting the read every time.
>>       */
>> -    AIO_WAIT_WHILE(ctx, job->status != JOB_STATUS_READY);
>> -    aio_context_release(ctx);
>> +    AIO_WAIT_WHILE_UNLOCKED(ctx, job->status != JOB_STATUS_READY);
>>  
>>      /* Begin the drained section, pausing the job */
>>      bdrv_drain_all_begin();
>> @@ -507,6 +498,7 @@ static void test_complete_in_standby(void)
>>          job_complete_locked(job, &error_abort);
>>  
>>          /* The test is done now, clean up. */
>> +        aio_context_release(ctx);
> 
> job_complete_locked() is not supposed to be called with the AioContext
> locked (otherwise blockdev.c would be wrong).
> 

Not sure what it has to do with blockdev.c, but you're right, the lock
is not needed there. Test is not affected by the lock position, I will
move it right after bdrv_drain_all_begin.

Emanuele