Am 08.08.2018 um 17:38 hat John Snow geschrieben: > On 08/08/2018 11:23 AM, Kevin Wolf wrote: > > Am 08.08.2018 um 06:02 hat Jeff Cody geschrieben: > >> On Tue, Aug 07, 2018 at 12:33:30AM -0400, John Snow wrote: > >>> Most 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 > >>> > >>> 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. > >>> > >>> The job infrastructure is perfectly capable of registering job > >>> completion itself when we leave the job's entry point. In this > >>> context, we can signal job completion from outside of the aio_context, > >>> which should allow for job cleanup code to run with only one lock. > >>> > >>> Signed-off-by: John Snow <js...@redhat.com> > >> > >> I like the simplification, both in SLOC and in exit logic (as seen in > >> patches 3-7). > > > > I agree, unifying this seems like a good idea. > > > > Like in the first patch, I'm not convinced of the details, though. > > Essentially, this is my objection regarding job->err extended to > > job->ret: You rely on jobs setting job->ret and job->err, but the > > interfaces don't really show this. > > > >>> @@ -546,6 +559,12 @@ static void coroutine_fn job_co_entry(void *opaque) > >>> assert(job && job->driver && job->driver->start); > >>> job_pause_point(job); > >>> job->driver->start(job); > >> > >> One nit-picky observation here, that is unrelated to this patch: reading > >> through, it may not be so obvious that 'start' is really a 'run' or > >> 'execute', (linguistically, to me 'start' implies a kick-off rather than > >> ongoing execution). > > > > I had exactly the same thought. My proposal is to change the existing... > > > > CoroutineEntry *start; > > > > ...which is just short for... > > > > void coroutine_fn start(void *opaque); > > > > ...into this one: > > > > int coroutine_fn run(void *opaque, Error **errp); > > > > I see that at the end of the series, you actually introduced an int > > return value already. I would have done that from the start, but as long > > the final state makes sense, I won't insist. > > > > But can we have the Error **errp addition, too? Pretty please? > > > > Kevin > > > > I'm actually glad you want that addition, I was considering very > strongly adding it but I felt like I had made the series long enough > already and didn't want to change too much all at once. > > The basic thought was just: > > "It'd sure be nice to have a generic function entry point that looks > like it returns the same error information as our non-coroutine functions." > > I can absolutely work that in, and break this series into two parts: > > (1) Rework jobs infrastructure to use the new run signature, and > (2) Rework jobs to use the finalization callbacks. > > Sound good?
I haven't looked at the rest of the series yet, but so far this sounds good to me. Kevin