| Message ID | 20220208143513.1077229-5-eesposit@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | job: replace AioContext lock with job_mutex | expand |
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 >
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 will add this to the commit description, but in my opinion the AioContext lock was not protecting any of the job fields in this patch. It is only taken because the callbacks assume it is always held. No job field in this patch is protected by the AioContext lock because they are also read/written in functions that never take the lock. Emanuele > > 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 >>
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
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; }
Instead of having the lock in job_tnx_apply, move it inside 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(-)