As context to everyone else as to why I'm going down the rabbit hole of trying to remove external references to AioContext at all, see

On 10/07/2016 03:49 AM, Paolo Bonzini wrote:

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) {

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

Or something up the stack at block_job_complete so it's not up to job implementations? What if the next implementer "forgets."

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)".

This makes me a little nervous because we went through the trouble of creating this callback, but we're going to assume we know that it's a public interface that will take the lock for itself (or otherwise does not require a lock.)

In practice it works, but it seems needlessly mystifying in terms of proving correctness.

- 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.

Makes sense; I don't like the association of (bs :: job) here anyway. Again we're grabbing context for a job where that job may not even be running.

As to block_job_cancel_sync:

Which I didn't audit, because no callers use job->blk to get the AioContext before calling this; they use bs if bs->job is present.

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

Seems like a bug on their part. Would be fixed by having cancel acquire context for itself.

- 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.

Yeah, I should be reffing it anyway.

The rest of this... What I think you mean is acquiring and releasing the context as needed for EACH of prepare, commit, abort, and clean as necessary, right?

And then in this case, it simply wouldn't be necessary for abort, as the sync cancel would do it for us.

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).

Eh? Oh, you're right, it just gets it for the sake of aio_poll.



Say I *do* push the acquisitions down into blockjob.c. What benefit does that provide? Won't I still need the block_job_get_aio_context() function (At least internally) to know which context to acquire? This would preclude you from deleting it.

Plus... we remove some fairly simple locking mechanisms and then inflate it tenfold. I'm not convinced this is an improvement.

As context and a refresher (for me when I re-read this email in 12 hours,) there are three places externally that are using an AioContext lock as acquired from *within* a BlockJob, excluding those that acquire a context separately from a Job and use that to reason that accesses to the job are safe (For example, blockdev_mark_auto_del.)

(1) QMP interface for job management
(2) bdrv_drain_all, in block/io.c

(1) AFAICT, the QMP interface is concerned with assuring itself it has unique access to the BlockJob structure itself, and it doesn't really authentically care about the AIOContext itself -- just race-free access to the Job.

This is not necessarily buggy today because, even though we grab the BlockBackend's context unconditionally, we already know the main/monitor thread is not accessing the blockjob. It's still silly, though.

(2) bdrv_drain_all appears to be worried about the same thing; we just need to safely deliver pause/resume messages.

I'm less sure about where this can run from, and suspect that if the job has deferred to main that this could be buggy. If bdrv_drain_all is called from context A and the job is running on context M having deferred from B, we may lock against context B (from A) and have racey access from between A/M. (Maybe?)

It looks like all of these usages don't actually really care what context they're getting, just that they're getting safe access to the job itself. If you want to decouple jobs from their BlockBackend's AIO Context, perhaps we just need to replace these by a simple mutex...?

As it stands, though, pushing AIOContext acquisitions down into blockjob.c isn't actually going to fix anything, is it? Why not just leave it be for right now?

Would you be terribly offended if I left this patch as-is for now and we can work on removing the AioContext locks afterwards, or are you adamant that I cooperate in getting block_job_get_aio_context removed before I add more usages?

Sorry I'm being so obtuse about this all.


Reply via email to