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