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

[Jeff Cody <jcody@redhat.com>]

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 <jsnow@redhat.com>
> >
> >>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);
        }
    }


> >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
> >