diff mbox series

[v2,1/3] job: Add job_wait_unpaused() for block-job-complete

Message ID 20210409132948.195511-2-mreitz@redhat.com
State New
Headers show
Series job: Add job_wait_unpaused() for block-job-complete | expand

Commit Message

Max Reitz April 9, 2021, 1:29 p.m. UTC
block-job-complete can only be applied when the job is READY, not when
it is on STANDBY (ready, but paused).  Draining a job technically pauses
it (which makes a READY job enter STANDBY), and ending the drained
section does not synchronously resume it, but only schedules the job,
which will then be resumed.  So attempting to complete a job immediately
after a drained section may sometimes fail.

That is bad at least because users cannot really work nicely around
this: A job may be paused and resumed at any time, so waiting for the
job to be in the READY state and then issuing a block-job-complete poses
a TOCTTOU problem.  The only way around it would be to issue
block-job-complete until it no longer fails due to the job being in the
STANDBY state, but that would not be nice.

We can solve the problem by allowing block-job-complete to be invoked on
jobs that are on STANDBY, if that status is the result of a drained
section (not because the user has paused the job), and that section has
ended.  That is, if the job is on STANDBY, but scheduled to be resumed.

Perhaps we could actually just directly allow this, seeing that mirror
is the only user of ready/complete, and that mirror_complete() could
probably work under the given circumstances, but there may be many side
effects to consider.

It is simpler to add a function job_wait_unpaused() that waits for the
job to be resumed (under said circumstances), and to make
qmp_block_job_complete() use it to delay job_complete() until then.

Note that for the future, we probably want to make block-job-complete a
coroutine QMP handler, so instead of polling job_wait_unpaused() would
yield and have job_pause_point() wake it up.  That would get around the
problem of nested polling, which is currently the reason for returning
an error when job->pause_point > 0.

Buglink: https://bugzilla.redhat.com/show_bug.cgi?id=1945635
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 include/qemu/job.h | 15 +++++++++++++
 blockdev.c         |  3 +++
 job.c              | 53 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 71 insertions(+)
diff mbox series

Patch

diff --git a/include/qemu/job.h b/include/qemu/job.h
index efc6fa7544..cf3082b6d7 100644
--- a/include/qemu/job.h
+++ b/include/qemu/job.h
@@ -563,4 +563,19 @@  void job_dismiss(Job **job, Error **errp);
  */
 int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp);
 
+/**
+ * If the job has been paused because of a drained section, and that
+ * section has ended, wait until the job is resumed.
+ *
+ * Return 0 if the job is not paused, or if it has been successfully
+ * resumed.
+ * Return an error if the job has been paused in such a way that
+ * waiting will not resume it, i.e. if it has been paused by the user,
+ * or if it is still drained.
+ *
+ * Callers must be in the home AioContext and hold the AioContext lock
+ * of job->aio_context.
+ */
+int job_wait_unpaused(Job *job, Error **errp);
+
 #endif
diff --git a/blockdev.c b/blockdev.c
index a57590aae4..c0cc2fa364 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3414,6 +3414,9 @@  void qmp_block_job_complete(const char *device, Error **errp)
         return;
     }
 
+    if (job_wait_unpaused(&job->job, errp) < 0) {
+        return;
+    }
     trace_qmp_block_job_complete(job);
     job_complete(&job->job, errp);
     aio_context_release(aio_context);
diff --git a/job.c b/job.c
index 289edee143..fbccd4b32a 100644
--- a/job.c
+++ b/job.c
@@ -505,6 +505,7 @@  void coroutine_fn job_pause_point(Job *job)
         job_do_yield(job, -1);
         job->paused = false;
         job_state_transition(job, status);
+        aio_wait_kick();
     }
 
     if (job->driver->resume) {
@@ -1023,3 +1024,55 @@  int job_finish_sync(Job *job, void (*finish)(Job *, Error **errp), Error **errp)
     job_unref(job);
     return ret;
 }
+
+int job_wait_unpaused(Job *job, Error **errp)
+{
+    /*
+     * Only run this function from the main context, because this is
+     * what we need, and this way we do not have to think about what
+     * happens if the user concurrently pauses the job from the main
+     * monitor.
+     */
+    assert(qemu_get_current_aio_context() == qemu_get_aio_context());
+
+    /*
+     * Quick path (e.g. so we do not get an error if pause_count > 0
+     * but the job is not even paused)
+     */
+    if (!job->paused) {
+        return 0;
+    }
+
+    /* If the user has paused the job, waiting will not help */
+    if (job->user_paused) {
+        error_setg(errp, "Job '%s' has been paused and needs to be explicitly "
+                   "resumed", job->id);
+        return -EBUSY;
+    }
+
+    /*
+     * Similarly, if the job is still drained, waiting may not help
+     * either: If the drain came from an IO thread, waiting would
+     * probably help.  However, if the drain came from the main thread
+     * (which may be possible if the QMP handler calling this function
+     * has been invoked by BDRV_POLL_WHILE() from a drain_begin), then
+     * waiting will only deadlock.
+     * Better be safe and return an error.  Drains from IO threads
+     * probably do not occur anyway.
+     */
+    if (job->pause_count > 0) {
+        error_setg(errp, "Job '%s' is blocked and cannot be unpaused", job->id);
+        return -EBUSY;
+    }
+
+    /*
+     * This function is specifically for waiting for a job to be
+     * resumed after a drained section.  Ending the drained section
+     * includes a job_enter(), which schedules the job loop to be run,
+     * and once it does, job->paused will be cleared.  Therefore, we
+     * do not need to invoke job_enter() here.
+     */
+    AIO_WAIT_WHILE(job->aio_context, job->paused);
+
+    return 0;
+}