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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 5554 bytes --]

On 2018-08-27 17:54, John Snow wrote:
> 
> 
> On 08/25/2018 09:05 AM, Max Reitz wrote:
>> On 2018-08-22 23:52, John Snow wrote:
>>>
>>>
>>> On 08/22/2018 07:43 AM, Max Reitz wrote:
>>>> 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 <jsnow@redhat.com>
>>>>> ---
>>>>>  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?
>>>>
>>>
>>> Yep.
>>>
>>>> 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.
>>>>
>>>
>>> Yeah, not all jobs will have anything meaningful to do in the main loop
>>> context. This is one of them.
>>>
>>>> ...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.
>>>>
>>>
>>> I think we need to execute job_complete in the main loop, or otherwise
>>> restructure the code that can run between job_completed and
>>> job_finalize_single so that .prepare/.commit/.abort/.clean run in the
>>> main thread, which is something we want to preserve.
>>
>> Sure.
>>
>>> It's simpler just to say that complete will run from the main thread,
>>> like it does presently.
>>
>> Yes, but we don't say that.
>>
>>> Why would we not want to call user_resume from the main loop? That's
>>> directly where it's called from, since it gets invoked directly from the
>>> qmp thread.
>>
>> Hmm!  True indeed.
>>
>> The reason why we might not want to do it is because the job may not run
>> in the main loop, so modifying the job (especially invoking a job
>> method) may be dangerous without taking precautions.
>>
>>>> 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()?
>>>>
>>>
>>> I don't think we need to do anything -- at least, these functions
>>> *already* run from the main loop.
>>
>> Yeah, but we don't mark that anywhere.  I really don't like that.  Jobs
>> need to know which of their functions are run in which AioContext.
>>
>>> mirror_exit et al get scheduled from job_defer_to_main_loop and call
>>> job_completed there, so it's already always done from the main loop; I'm
>>> just cutting out the part where the jobs have to manually schedule this.
>>
>> I'm not saying what you're doing is wrong, I'm just saying tracking
>> which things are running in which context is not easy because there are
>> no comments on how it's supposed to be run.  (Apart from your new
>> .exit() method which does say that it's run in the main loop.)
>>
>> No, I don't find it obvious which functions are run in which context
>> when first I have to think about in which context those functions are
>> used (e.g. user_resume is usually the result of a QMP command, so it's
>> run in the main loop; the transactional methods are part of completion,
>> which is done in the main loop, so they are also called in the main
>> loop; and so on).
>>
>> But that's not part of this series.  It just occurred to me when
>> tracking down which function belongs to which context when reviewing
>> this patch.
>>
>> Max
>>
> 
> Oh, I see. I can mark up the functions I/we expect to run in the main
> thread with comments above the function implementation, would that help?

Sure, that's exactly what I mean. :-)

> Probably also a top level document would also help... We're overdue for
> one after all the changes recently.

If you have the time, sure.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]