Re: [Qemu-devel] [PATCH 3/7] jobs: add exit shim

[Max Reitz]

On 2018-08-17 21:04, John Snow wrote:
> All jobs do the same thing when they leave their running loop:
> - Store the return code in a structure
> - wait to receive this structure in the main thread
> - signal job completion via job_completed
> 
> Few jobs do anything beyond exactly this. Consolidate this exit
> logic for a net reduction in SLOC.
> 
> More seriously, when we utilize job_defer_to_main_loop_bh to call
> a function that calls job_completed, job_finalize_single will run
> in a context where it has recursively taken the aio_context lock,
> which can cause hangs if it puts down a reference that causes a flush.
> 
> You can observe this in practice by looking at mirror_exit's careful
> placement of job_completed and bdrv_unref calls.
> 
> If we centralize job exiting, we can signal job completion from outside
> of the aio_context, which should allow for job cleanup code to run with
> only one lock, which makes cleanup callbacks less tricky to write.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>  include/qemu/job.h |  7 +++++++
>  job.c              | 19 +++++++++++++++++++
>  2 files changed, 26 insertions(+)

Currently all jobs do this, the question of course is why.  The answer
is because they are block jobs that need to do some graph manipulation
in the main thread, right?

OK, that's reasonable enough, that sounds like even non-block jobs may
need this (i.e. modify some global qemu state that you can only do in
the main loop).  Interestingly, the create job only calls
job_completed() of which it says nowhere that it needs to be executed in
the main loop.

...on second thought, do we really want to execute job_complete() in the
main loop?  First of all, all of the transactional functions will run in
the main loop.  Which makes sense, but it isn't noted anywhere.
Secondly, we may end up calling JobDriver.user_resume(), which is
probably not something we want to call in the main loop.

OTOH, job_finish_sync() is something that has to be run in the main loop
because it polls the main loop (and as far as my FUSE experiments have
told me, polling a foreign AioContext doesn't work).

So...  I suppose it would be nice if we had a real distinction which
functions are run in which AioContext.  It seems like we indeed want to
run job_completed() in the main loop, but what to do about the
user_resume() call in job_cancel_async()?

(And it should be noted for all of the transactional methods that they
are called in the main loop.)


OK, so that's that.  Now that I know what it's for, I'd like to ask for
a different name.  exit() means kill the process.  JobDriver.exit() will
not mean kill the job.  That's where I get a headache.

This function is for allowing the job to carry out global qemu state
modifications in the main loop.  Neither is that exiting in the sense
that the job is destroyed (as this is done only later, and the job gets
to take part in it through the transactional callbacks, and .free()),
nor is it exiting in the sense that the job needs to do all
pre-transactional clean-ups here (they are supposed to do that in .run()
-- *unlees* something needs the main loop).

I'd like .main_loop_settle().  Or .main_loop_post_run().  I think it's
OK to have names that aren't as cool and tense as possible, when in
return they actually tell you what they're doing.  (Sure,
.main_loop_post_run() sounds really stupid, but it tells you exactly
when the function is called and what it's for.)

(Maybe the problem of all your naming woes really is just that you
always try to find a single word that describes what's going on :-) -- I
don't want to go into ProblemSolveFactoryObserverFactorySingleton
either, but it's OK to use an underscore once in a while.)

Max

signature.asc
Description: OpenPGP digital signature