qemu-devel
[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: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v11 18/21] job.c: enable job lock/unlock and remove Aiocontext locks
Date: Thu, 15 Sep 2022 17:52:01 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.1

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



reply via email to

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