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 <js...@redhat.com> --- 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)?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.
Done.
+{ + 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 */
I decided to fix both problems by just having a block_job_user_pause() and a block_job_user_resume().
I'll wait for more reviews before I send out the v2.
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
--js
