>
>> }
>> @@ -501,8 +481,12 @@ void job_unref_locked(Job *job)>
>> assert(!job->txn);
>> if (job->driver->free) {
>> + AioContext *aio_context = job->aio_context;
>> job_unlock();
>> + /* FIXME: aiocontext lock is required because cb calls
>> blk_unref */
>> + aio_context_acquire(aio_context);
>> job->driver->free(job);
>> + aio_context_release(aio_context);
>
> So, job_unref_locked() must be called with aio_context not locked,
> otherwise we dead-lock here? That should be documented in function
> declaration comment.
>
> For example in qemu-img.c in run_block_job() we do exactly that: call
> job_unref_locked() inside aio-context lock critical seaction..
No, job_unref_locked has also status and refcnt and all the other fields
that need to be protectd. Only free must be called without job lock held.
>
>
>> job_lock();
>> }
>> @@ -581,21 +565,17 @@ void job_enter_cond_locked(Job *job,
>> bool(*fn)(Job *job))
>> return;
>> }
>> - real_job_lock();
>> if (job->busy) {
>> - real_job_unlock();
>> return;
>> }
>> if (fn && !fn(job)) {
>> - real_job_unlock();
>> return;
>> }
>> assert(!job->deferred_to_main_loop);
>> timer_del(&job->sleep_timer);
>> job->busy = true;
>> - real_job_unlock();
>> job_unlock();
>> aio_co_wake(job->co);
>> job_lock();
>> @@ -626,13 +606,11 @@ static void coroutine_fn job_do_yield_locked(Job
>> *job, uint64_t ns)
>> {
>> AioContext *next_aio_context;
>> - real_job_lock();
>> if (ns != -1) {
>> timer_mod(&job->sleep_timer, ns);
>> }
>> job->busy = false;
>> job_event_idle_locked(job);
>> - real_job_unlock();
>> job_unlock();
>> qemu_coroutine_yield();
>> job_lock();
>> @@ -922,6 +900,7 @@ static void job_clean(Job *job)
>> static int job_finalize_single_locked(Job *job)
>> {
>> int job_ret;
>> + AioContext *ctx = job->aio_context;
>> assert(job_is_completed_locked(job));
>> @@ -929,6 +908,7 @@ static int job_finalize_single_locked(Job *job)
>> job_update_rc_locked(job);
>> job_unlock();
>> + aio_context_acquire(ctx);
>
> Hmm, and this function and all its callers now should be called with
> aio-context lock not locked?
Why not leave it as it is?
>
> For example job_exit is scheduled as as BH. Aren't BHs called with
> aio-context lock held?
I see no aiocontext call in aio_bh_schedule_oneshot and callees...
So summing up, no, I don't think I will apply your suggestions for this
patch here (assume the opposite for all the others).
Emanuele
>
>> if (!job->ret) {
>> job_commit(job);
>> @@ -937,6 +917,7 @@ static int job_finalize_single_locked(Job *job)
>> }
>> job_clean(job);
>> + aio_context_release(ctx);
>> job_lock();
>> if (job->cb) {
>
> [..]
>
>
