qemu-block
[Top][All Lists]
Advanced

[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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]