Re: [Qemu-devel] [PATCH v2 1/3] blockjob: add block_job_start_shim

[Jeff Cody <jcody@redhat.com>]

On Wed, Mar 22, 2017 at 08:57:51AM -0400, Jeff Cody wrote:
> On Thu, Mar 16, 2017 at 05:23:49PM -0400, John Snow wrote:
> > The purpose of this shim is to allow us to pause pre-started jobs.
> > The purpose of *that* is to allow us to buffer a pause request that
> > will be able to take effect before the job ever does any work, allowing
> > us to create jobs during a quiescent state (under which they will be
> > automatically paused), then resuming the jobs after the critical section
> > in any order, either:
> > 
> > (1) -block_job_start
> >     -block_job_resume (via e.g. drained_end)
> > 
> > (2) -block_job_resume (via e.g. drained_end)
> >     -block_job_start
> > 
> > The problem that requires a startup wrapper is the idea that a job must
> > start in the busy=true state only its first time-- all subsequent entries
> > require busy to be false, and the toggling of this state is otherwise
> > handled during existing pause and yield points.
> > 
> > The wrapper simply allows us to mandate that a job can "start," set busy
> > to true, then immediately pause only if necessary. We could avoid
> > requiring a wrapper, but all jobs would need to do it, so it's been
> > factored out here.
> 
> I think this makes sense.  So when this happens:
> 
> * block_job_create
> * block_job_pause
> * block_job_resume  <-- only effects pause_count, rest is noop
> * block_job_start
> 
> The block_job_resume is mostly a no-op, only affecting the pause_count but
> since there is no job coroutine created yet, the block_job_enter does
> nothing.
>

I should have added:

Reviewed-by: Jeff Cody <jcody@redhat.com>

> > 
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  blockjob.c | 26 +++++++++++++++++++-------
> >  1 file changed, 19 insertions(+), 7 deletions(-)
> > 
> > diff --git a/blockjob.c b/blockjob.c
> > index 69126af..69b4ec6 100644
> > --- a/blockjob.c
> > +++ b/blockjob.c
> > @@ -250,16 +250,28 @@ static bool block_job_started(BlockJob *job)
> >      return job->co;
> >  }
> >  
> > +/**
> > + * All jobs must allow a pause point before entering their job proper. This
> > + * ensures that jobs can be paused prior to being started, then resumed later.
> > + */
> > +static void coroutine_fn block_job_co_entry(void *opaque)
> > +{
> > +    BlockJob *job = opaque;
> > +
> > +    assert(job && job->driver && job->driver->start);
> > +    block_job_pause_point(job);
> > +    job->driver->start(job);
> > +}
> > +
> >  void block_job_start(BlockJob *job)
> >  {
> >      assert(job && !block_job_started(job) && job->paused &&
> > -           !job->busy && job->driver->start);
> > -    job->co = qemu_coroutine_create(job->driver->start, job);
> > -    if (--job->pause_count == 0) {
> > -        job->paused = false;
> > -        job->busy = true;
> > -        qemu_coroutine_enter(job->co);
> > -    }
> > +           job->driver && job->driver->start);
> > +    job->co = qemu_coroutine_create(block_job_co_entry, job);
> > +    job->pause_count--;
> > +    job->busy = true;
> > +    job->paused = false;
> > +    qemu_coroutine_enter(job->co);
> >  }
> >  
> >  void block_job_ref(BlockJob *job)
> > -- 
> > 2.9.3
> >