[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 02/21] jobs: add exit shim
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH 02/21] jobs: add exit shim |
Date: |
Wed, 8 Aug 2018 17:47:44 +0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
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 <address@hidden>
> >>
> >> 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
[Qemu-devel] [PATCH 18/21] block/commit: refactor stream to use job callbacks, John Snow, 2018/08/07
[Qemu-devel] [PATCH 15/21] block/stream: add block job creation flags, John Snow, 2018/08/07
[Qemu-devel] [PATCH 03/21] block/backup: utilize job_exit shim, John Snow, 2018/08/07
[Qemu-devel] [PATCH 14/21] block/mirror: add block job creation flags, John Snow, 2018/08/07
[Qemu-devel] [PATCH 10/21] tests/test-bdrv-drain: utilize job_exit shim, John Snow, 2018/08/07
[Qemu-devel] [PATCH 19/21] qapi/block-commit: expose new job properties, John Snow, 2018/08/07
[Qemu-devel] [PATCH 20/21] qapi/block-mirror: expose new job properties, John Snow, 2018/08/07