Re: [Qemu-devel] [PATCH v3 4/6] blockjob: add block_job_start

[John Snow]

On 11/07/2016 09:05 PM, Jeff Cody wrote:
> On Mon, Nov 07, 2016 at 09:02:14PM -0500, John Snow wrote:
> > On 11/03/2016 08:17 AM, Kevin Wolf wrote:
> > > Am 02.11.2016 um 18:50 hat John Snow geschrieben:
> > > > Instead of automatically starting jobs at creation time via backup_start
> > > > et al, we'd like to return a job object pointer that can be started
> > > > manually at later point in time.
> > > >
> > > > For now, add the block_job_start mechanism and start the jobs
> > > > automatically as we have been doing, with conversions job-by-job coming
> > > > in later patches.
> > > >
> > > > Of note: cancellation of unstarted jobs will perform all the normal
> > > > cleanup as if the job had started, particularly abort and clean. The
> > > > only difference is that we will not emit any events, because the job
> > > > never actually started.
> > > >
> > > > Signed-off-by: John Snow <address@hidden>
> > >
> > > > diff --git a/block/commit.c b/block/commit.c
> > > > index 20d27e2..5b7c454 100644
> > > > --- a/block/commit.c
> > > > +++ b/block/commit.c
> > > > @@ -289,10 +289,9 @@ void commit_start(const char *job_id, BlockDriverState *bs,
> > > >     s->backing_file_str = g_strdup(backing_file_str);
> > > >
> > > >     s->on_error = on_error;
> > > > -    s->common.co = qemu_coroutine_create(s->common.driver->start, s);
> > > >
> > > >     trace_commit_start(bs, base, top, s, s->common.co);
> > >
> > > s->common.co is now uninitialised and should probably be removed from
> > > the tracepoint arguments. The same is true for mirror and stream.
> > >
> > > > -    qemu_coroutine_enter(s->common.co);
> > > > +    block_job_start(&s->common);
> > > > }
> > >
> > > > diff --git a/blockjob.c b/blockjob.c
> > > > index e3c458c..16c5159 100644
> > > > --- a/blockjob.c
> > > > +++ b/blockjob.c
> > > > @@ -174,7 +174,8 @@ void *block_job_create(const char *job_id, const 
> > > > BlockJobDriver *driver,
> > > >     job->blk           = blk;
> > > >     job->cb            = cb;
> > > >     job->opaque        = opaque;
> > > > -    job->busy          = true;
> > > > +    job->busy          = false;
> > > > +    job->paused        = true;
> > > >     job->refcnt        = 1;
> > > >     bs->job = job;
> > > >
> > > > @@ -202,6 +203,21 @@ bool block_job_is_internal(BlockJob *job)
> > > >     return (job->id == NULL);
> > > > }
> > > >
> > > > +static bool block_job_started(BlockJob *job)
> > > > +{
> > > > +    return job->co;
> > > > +}
> > > > +
> > > > +void block_job_start(BlockJob *job)
> > > > +{
> > > > +    assert(job && !block_job_started(job) && job->paused &&
> > > > +           !job->busy && job->driver->start);
> > > > +    job->paused = false;
> > > > +    job->busy = true;
> > > > +    job->co = qemu_coroutine_create(job->driver->start, job);
> > > > +    qemu_coroutine_enter(job->co);
> > > > +}
> > >
> > > We allow the user to pause a job while it's not started yet. You
> > > classified this as "harmless". But if we accept this, can we really
> > > unconditionally enter the coroutine even if the job has been paused?
> > > Can't a user expect that a job remains in paused state when they
> > > explicitly requested a pause and the job was already internally paused,
> > > like in this case by block_job_create()?
> >
> > What will end up happening is that we'll enter the job, and then it'll pause
> > immediately upon entrance. Is that a problem?
> >
> > If the jobs themselves are not checking their pause state fastidiously, it
> > could be (but block/backup does -- after it creates a write notifier.)
> >
> > Do we want a stronger guarantee here?
> >
> > Naively I think it's OK as-is, but I could add a stronger boolean in that
> > lets us know if it's okay to start or not, and we could delay the actual
> > creation and start until the 'resume' comes in if you'd like.
> >
> > I'd like to avoid the complexity if we can help it, but perhaps I'm not
> > thinking carefully enough about the existing edge cases.
>
> Is there any reason we can't just use job->pause_count here?  When the job
> is created, set job->paused = true, and job->pause_count = 1.  In the
> block_job_start(), check the pause_count prior to qemu_coroutine_enter():
>
>     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);
>         job->paused = --job->pause_count > 0;
>         if (!job->paused) {
>             job->busy = true;
>             qemu_coroutine_enter(job->co);
>         }
>     }

Solid point. Let's do it this way.
Thanks!

> > > The same probably also applies to the internal job pausing during
> > > bdrv_drain_all_begin/end, though as you know there is a larger problem
> > > with starting jobs under drain_all anyway. For now, we just need to keep
> > > in mind that we can neither create nor start a job in such sections.
> >
> > Yeah, there are deeper problems there. As long as the existing critical
> > sections don't allow us to create jobs (started or not) I think we're
> > probably already OK.
> >
> > > Kevin