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 <[email protected]> Reviewed-by: Stefan Hajnoczi <[email protected]> ---
[..]
--- 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
