[PATCH v5 04/20] job.c: move inner aiocontext lock in callbacks
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
---
job.c | 17 -
1 file changed, 12 insertions(+), 5 deletions(-)
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
Re: [PATCH v5 04/20] job.c: move inner aiocontext lock in callbacks
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
> ---
> 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
Re: [PATCH v5 04/20] job.c: move inner aiocontext lock in callbacks
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
>> ---
>> 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
>>
Re: [PATCH v5 04/20] job.c: move inner aiocontext lock in callbacks
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
>> ---
>> 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
