Re: [PATCH v11 18/21] job.c: enable job lock/unlock and remove Aiocontext locks

[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?

> @@ -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?

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?


Otherwise looks good to me!

--
Best regards,
Vladimir