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

[Stefan Hajnoczi]

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
>

signature.asc
Description: PGP signature