[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v11 18/21] job.c: enable job lock/unlock and remove Aiocontex
From: |
Emanuele Giuseppe Esposito |
Subject: |
Re: [PATCH v11 18/21] job.c: enable job lock/unlock and remove Aiocontext locks |
Date: |
Sun, 18 Sep 2022 19:12:48 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.2.0 |
Am 15/09/2022 um 16:52 schrieb Vladimir Sementsov-Ogievskiy:
> On 8/26/22 16:21, Emanuele Giuseppe Esposito wrote:
>> Change the job_{lock/unlock} and macros to use job_mutex.
>>
>> Now that they are not nop anymore, remove the aiocontext
>> to avoid deadlocks.
>>
>> 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
>>
>> The only functions that still need the aiocontext lock are the JobDriver
>> callbacks, already documented in job.h. Reduce the locking section to
>> only cover the callback invocation and document the functions that
>> take the AioContext lock, to avoid taking it twice.
>>
>> Also remove real_job_{lock/unlock}, as they are replaced by the
>> public functions.
>>
>> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
>> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> ---
>
> [..]
>
>> --- a/qemu-img.c
>> +++ b/qemu-img.c
>> @@ -911,7 +911,6 @@ static void run_block_job(BlockJob *job, Error
>> **errp)
>> AioContext *aio_context = block_job_get_aio_context(job);
>> int ret = 0;
>> - aio_context_acquire(aio_context);
>> job_lock();
>> job_ref_locked(&job->job);
>> do {
>
> aio_poll() call here, doesn't require aio_context to be acquired?
On the contrary I think, if you see in AIO_WAIT_WHILE we explicitly
release it because we don't want to allow something else to run with the
aiocontext acquired.
>
>> @@ -936,7 +935,6 @@ static void run_block_job(BlockJob *job, Error
>> **errp)
>> }
>> job_unref_locked(&job->job);
>> job_unlock();
>> - aio_context_release(aio_context);
>> /* publish completion progress only when success */
>> if (!ret) {
>
> [..]
>
> In replication_stop, we call job_cancel_sync() inside
> aio_context_acquire - aio_context_release section. Should it be fixed?
I don't think it breaks anything. The question is: what is the
aiocontext there protecting? Do we need it? I have no idea.
>
> Another question, sometimes you move job_start out of
> aio-context-acquire critical section, sometimes not. As I understand,
> it's of for job_start to be called both with acquired aio-context or not
> acquired?
>
Same as above, job_start does not need the aiocontext to be taken
(otherwise job_lock would be useless).
>
> Otherwise looks good to me!
>
Thank you,
Emanuele