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

[John Snow]

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.

> 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