Re: [Qemu-block] [PATCH v2 03/11] Blockjobs: Internalize user_pause logic

[John Snow]

On 10/03/2016 08:57 PM, Jeff Cody wrote:
> On Fri, Sep 30, 2016 at 06:00:41PM -0400, John Snow wrote:
> > BlockJobs will begin hiding their state in preparation for some
> > refactorings anyway, so let's internalize the user_pause mechanism
> > instead of leaving it to callers to correctly manage.
> >
> > Signed-off-by: John Snow <address@hidden>
> > ---
> >  block/io.c               |  2 +-
> >  blockdev.c               | 10 ++++------
> >  blockjob.c               | 16 ++++++++++++----
> >  include/block/blockjob.h | 11 ++++++++++-
> >  4 files changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/block/io.c b/block/io.c
> > index fdf7080..868b065 100644
> > --- a/block/io.c
> > +++ b/block/io.c
> > @@ -291,7 +291,7 @@ void bdrv_drain_all(void)
> >          AioContext *aio_context = blk_get_aio_context(job->blk);
> >
> >          aio_context_acquire(aio_context);
> > -        block_job_pause(job);
> > +        block_job_pause(job, false);
>
> Hmm.  Maybe semantically, an enum might work better here than a bool.  When
> I see "block_job_pause(job, false)", it reads like you just un-paused the
> job, rather than paused it with some contextual info.  Perhaps
> JOB_USER_PAUSE and JOB_SYS_PAUSE (or just JOB_USER and JOB_SYS)?

Thanks for taking a look. Mostly I was trying to squish users of jobs 
internals into API accesses instead, in semi-prep for shifting things 
around a good bit later.

The real inspiration was trying to figure out if adding a new state was 
safe, and figuring out who-touched-what-and-when was easier if I split 
things into "internal" and "external" functions, then I could audit the 
external functions more carefully.

(This is a tip for auditing the safety of what I do later in this 
series...!)

To the point, you're probably right, this was a bit of a quick hack. 
I'll make this nicer tomorrow.

> >          aio_context_release(aio_context);
> >      }
> >
> > diff --git a/blockdev.c b/blockdev.c
> > index 03200e7..268452f 100644
> > --- a/blockdev.c
> > +++ b/blockdev.c
> > @@ -3629,7 +3629,7 @@ void qmp_block_job_cancel(const char *device,
> >          force = false;
> >      }
> >
> > -    if (job->user_paused && !force) {
> > +    if (block_job_paused(job) && !force) {
> >          error_setg(errp, "The block job for device '%s' is currently paused",
> >                     device);
> >          goto out;
> > @@ -3646,13 +3646,12 @@ void qmp_block_job_pause(const char *device, Error 
> > **errp)
> >      AioContext *aio_context;
> >      BlockJob *job = find_block_job(device, &aio_context, errp);
> >
> > -    if (!job || job->user_paused) {
> > +    if (!job || block_job_paused(job)) {
> >          return;
> >      }
> >
> > -    job->user_paused = true;
> >      trace_qmp_block_job_pause(job);
> > -    block_job_pause(job);
> > +    block_job_pause(job, true);
> >      aio_context_release(aio_context);
> >  }
> >
> > @@ -3661,11 +3660,10 @@ void qmp_block_job_resume(const char *device, Error 
> > **errp)
> >      AioContext *aio_context;
> >      BlockJob *job = find_block_job(device, &aio_context, errp);
> >
> > -    if (!job || !job->user_paused) {
> > +    if (!job || !block_job_paused(job)) {
> >          return;
> >      }
> >
> > -    job->user_paused = false;
> >      trace_qmp_block_job_resume(job);
> >      block_job_iostatus_reset(job);
> >      block_job_resume(job);
> > diff --git a/blockjob.c b/blockjob.c
> > index 6a300ba..2a35f50 100644
> > --- a/blockjob.c
> > +++ b/blockjob.c
> > @@ -104,7 +104,7 @@ static void block_job_detach_aio_context(void *opaque)
> >      /* In case the job terminates during aio_poll()... */
> >      block_job_ref(job);
> >
> > -    block_job_pause(job);
> > +    block_job_pause(job, false);
> >
> >      if (!job->paused) {
> >          /* If job is !job->busy this kicks it into the next pause point. */
> > @@ -343,9 +343,12 @@ void block_job_complete(BlockJob *job, Error **errp)
> >      job->driver->complete(job, errp);
> >  }
> >
> > -void block_job_pause(BlockJob *job)
> > +void block_job_pause(BlockJob *job, bool user)
> >  {
> >      job->pause_count++;
> > +    if (user) {
> > +        job->user_paused = true;
> > +    }
> >  }
> >
> >  static bool block_job_should_pause(BlockJob *job)
> > @@ -353,6 +356,11 @@ static bool block_job_should_pause(BlockJob *job)
> >      return job->pause_count > 0;
> >  }
> >
> > +bool block_job_paused(BlockJob *job)
>
> I think a more apt name for this would be "block_job_user_paused()", because
> it doesn't tell if the job is paused per se, but only if it is paused by a
> user.

Sure.

> > +{
> > +    return job ? job->user_paused : 0;
> > +}
> > +
> >  void coroutine_fn block_job_pause_point(BlockJob *job)
> >  {
> >      if (!block_job_should_pause(job)) {
> > @@ -386,6 +394,7 @@ void block_job_resume(BlockJob *job)
> >      if (job->pause_count) {
> >          return;
> >      }
> > +    job->user_paused = false;
>
> At first I was thinking block_job_resume() would need a way of knowing if it
> was a user resume, similar to block_job_pause().  But I guess not... if the
> user paused it, then job->pause_count should always be > 0, I think.
>
> But wait...could we ever get into a state where something like this happens:
>
> block_job_pause(job, true);  /* user paused */
>
> [...]
>
> block_job_pause(job, false); /* system paused */
>
> [...]
>
> block_job_resume(job);       /* user resumes */
>
> [...]   /* job->user_paused is still true, although it shouldn't be */
>
> block_job_resume(job);       /* system resumes */
>
>                              /* now job->user_paused is false */

Good question. And if it does happen, what's the impact?

(1) The user could not cancel the job until whatever was pausing it lets 
go, unless they used the force option.

Maybe this leads to new cancel failures that didn't exist previously.

(2) The user could not re-pause the job until the job resumed again.

This doesn't seem critical, though it is annoying to deny a request we 
could have easily serviced.

(3) The user may be able to re-resume the job.

This will ruin the pause counter.

That's all kind of annoying, so perhaps a counterpart change to the 
resume API is warranted.

> >      block_job_enter(job);
> >  }
> >
> > @@ -592,8 +601,7 @@ BlockErrorAction block_job_error_action(BlockJob *job, 
> > BlockdevOnError on_err,
> >                                      action, &error_abort);
> >      if (action == BLOCK_ERROR_ACTION_STOP) {
> >          /* make the pause user visible, which will be resumed from QMP. */
> > -        job->user_paused = true;
> > -        block_job_pause(job);
> > +        block_job_pause(job, true);
> >          block_job_iostatus_set_err(job, error);
> >      }
> >      return action;
> > diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> > index 4ddb4ae..081f6c2 100644
> > --- a/include/block/blockjob.h
> > +++ b/include/block/blockjob.h
> > @@ -347,10 +347,19 @@ void coroutine_fn block_job_pause_point(BlockJob *job);
> >  /**
> >   * block_job_pause:
> >   * @job: The job to be paused.
> > + * @user: Requested explicitly via user?
> >   *
> >   * Asynchronously pause the specified job.
> >   */
> > -void block_job_pause(BlockJob *job);
> > +void block_job_pause(BlockJob *job, bool user);
> > +
> > +/**
> > + * block_job_paused:
> > + * @job: The job to query.
> > + *
> > + * Returns true if the job is user-paused.
> > + */
> > +bool block_job_paused(BlockJob *job);
> >
> >  /**
> >   * block_job_resume:
> > --
> > 2.7.4