Re: [Qemu-block] [Qemu-devel] [PATCH v2 04/11] blockjobs: Always use block_job_get_aio_context

[Paolo Bonzini]

On 06/10/2016 22:22, John Snow wrote:
> Calls .complete(), for which the only implementation is
> mirror_complete. Uh, this actually seems messy. Looks like there's
> nothing to prevent us from calling this after we've already told it to
> complete once.

Yeah, it should have an

    if (s->should_complete) {
        return;
    }

at the beginning.  I have other mirror.c patches in my queue so I can
take care of that.

> block_job_cancel and block_job_complete are different.
> 
> block_job_cancel is called in many places, but we can just add a similar
> block_job_user_cancel if we wanted a version which takes care to acquire
> context and one that does not. (Or we could just acquire the context
> regardless, but Paolo warned me ominously that recursive locks are EVIL.
> He sounded serious.)

Not that many places:

- block_job_finish_sync calls it, and you can just release/reacquire
around the call to "finish(job, &local_err)".

- there are two callers in blockdev.c, and you can just remove the
acquire/release from blockdev.c if you push it in block_job_cancel.

As to block_job_cancel_sync:

- replication_stop is not acquiring s->secondary_disk->bs's AioContext.

- there is no need to hold the AioContext between ->prepare and ->clean.
 My suggestion is to ref the blockjob after storing it in state->job
(you probably should do that anyway) and unref'ing it in ->clean.  Then
you can call again let block_job_cancel_sync(bs->job) take the
AioContext, which it will do in block_job_finish_sync.

> block_job_complete has no direct callers outside of QMP, but it is also
> used as a callback by block_job_complete_sync, used in qemu-img for
> run_block_job. I can probably rewrite qemu_img to avoid this usage.

No need to: qemu-img is not acquiring the AioContext, so it's okay to
let block_job_complete do that (like block_job_cancel,
block_job_complete will be called by block_job_finish_sync without the
AioContext acquired).

Paolo