Re: [PATCH for-6.2 v3 01/12] job: Context changes in job_completed_txn_abort()

[Hanna Reitz <hreitz@redhat.com>]

On 01.09.21 12:05, Vladimir Sementsov-Ogievskiy wrote:
> 06.08.2021 12:38, Max Reitz wrote:
>> Finalizing the job may cause its AioContext to change.  This is noted by
>> job_exit(), which points at job_txn_apply() to take this fact into
>> account.
>>
>> However, job_completed() does not necessarily invoke job_txn_apply()
>> (through job_completed_txn_success()), but potentially also
>> job_completed_txn_abort().  The latter stores the context in a local
>> variable, and so always acquires the same context at its end that it has
>> released in the beginning -- which may be a different context from the
>> one that job_exit() releases at its end.  If it is different, qemu
>> aborts ("qemu_mutex_unlock_impl: Operation not permitted").
>>
>> Drop the local @outer_ctx variable from job_completed_txn_abort(), and
>> instead re-acquire the actual job's context at the end of the function,
>> so job_exit() will release the same.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   job.c | 23 ++++++++++++++++++-----
>>   1 file changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/job.c b/job.c
>> index e7a5d28854..3fe23bb77e 100644
>> --- a/job.c
>> +++ b/job.c
>> @@ -737,7 +737,6 @@ static void job_cancel_async(Job *job, bool force)
>>     static void job_completed_txn_abort(Job *job)
>>   {
>> -    AioContext *outer_ctx = job->aio_context;
>>       AioContext *ctx;
>>       JobTxn *txn = job->txn;
>>       Job *other_job;
>> @@ -751,10 +750,14 @@ static void job_completed_txn_abort(Job *job)
>>       txn->aborting = true;
>>       job_txn_ref(txn);
>>   -    /* We can only hold the single job's AioContext lock while 
>> calling
>> +    /*
>> +     * 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. */
>> -    aio_context_release(outer_ctx);
>> +     * calls of AIO_WAIT_WHILE(), which could deadlock otherwise.
>> +     * Note that the job's AioContext may change when it is finalized.
>> +     */
>> +    job_ref(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
>> @@ -769,6 +772,10 @@ static void job_completed_txn_abort(Job *job)
>>       }
>>       while (!QLIST_EMPTY(&txn->jobs)) {
>>           other_job = QLIST_FIRST(&txn->jobs);
>> +        /*
>> +         * The job's AioContext may change, so store it in @ctx so we
>> +         * release the same context that we have acquired before.
>> +         */
>>           ctx = other_job->aio_context;
>>           aio_context_acquire(ctx);
>>           if (!job_is_completed(other_job)) {
>> @@ -779,7 +786,13 @@ static void job_completed_txn_abort(Job *job)
>>           aio_context_release(ctx);
>>       }
>>   -    aio_context_acquire(outer_ctx);
>> +    /*
>> +     * Use job_ref()/job_unref() so we can read the AioContext here
>> +     * even if the job went away during job_finalize_single().
>> +     */
>> +    ctx = job->aio_context;
>> +    job_unref(job);
>> +    aio_context_acquire(ctx);
>
>
> why to use ctx variable and not do it exactly same as in 
> job_txn_apply() :
>
>    aio_context_acquire(job->aio_context);
>    job_unref(job);
>
> ?

Oh, I just didn’t think of that.  Sounds good, thanks!

Hanna

> anyway:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>