[Qemu-devel] [PATCH v4 0/5] blockjob: AioContext change support for mirror and backup

[Qemu-devel] [PATCH v4 0/5] blockjob: AioContext change support for mirror and backup

[Stefan Hajnoczi]

v4:
 * Add .pause()/.resume() callbacks to really quiesce during
   block_job_pause_point() [Paolo]
 * Add AioContext change support for backup block job
 * Tested drive_mirror + migration and drive_backup + reboot

v3:
 * Push infrastructure down into blockjob.c so other jobs can reuse it [Stefan]
 * Tested with drive_mirror + migration [Stefan]

v2:
 * Fam introduced the concept of a synchronous aio_poll() loop to quiesce the
   block job during detach

When dataplane is enabled or disabled the drive switches to a new AioContext.
The mirror and backup block jobs must also move to the new AioContext so that
drive accesses are always made within its AioContext.

This series extends the block job pause functionality so that detaching from an
AioContext pauses the job and attaching to the new AioContext resumes the job.

Pause points are added to the mirror job so that long I/O loops can yield for
an AioContext switch.  Other block jobs need pause points too but this can be
done as a follow-up series.

Stefan Hajnoczi (5):
  blockjob: move iostatus reset out of block_job_enter()
  blockjob: add pause points
  blockjob: add AioContext attached callback
  mirror: follow AioContext change gracefully
  backup: follow AioContext change gracefully

 block/backup.c           | 22 ++++++++-----
 block/mirror.c           | 45 +++++++++++++++++++++-----
 blockdev.c               |  1 +
 blockjob.c               | 84 ++++++++++++++++++++++++++++++++++++++++++------
 include/block/blockjob.h | 42 ++++++++++++++++++++++--
 5 files changed, 166 insertions(+), 28 deletions(-)

-- 
2.5.5

[Qemu-devel] [PATCH v4 1/5] blockjob: move iostatus reset out of block_job_enter()

[Stefan Hajnoczi]

The QMP block-job-resume command and cancellation may want to reset the
job's iostatus.  The next patches add a user who does not want to reset
iostatus so move it up to block_job_enter() callers.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockdev.c | 1 +
 blockjob.c | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/blockdev.c b/blockdev.c
index 7fd515a..19b963c 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -3799,6 +3799,7 @@ void qmp_block_job_resume(const char *device, Error **errp)
 
     job->user_paused = false;
     trace_qmp_block_job_resume(job);
+    block_job_iostatus_reset(job);
     block_job_resume(job);
     aio_context_release(aio_context);
 }
diff --git a/blockjob.c b/blockjob.c
index c095cc5..463bccf 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -269,7 +269,6 @@ void block_job_resume(BlockJob *job)
 
 void block_job_enter(BlockJob *job)
 {
-    block_job_iostatus_reset(job);
     if (job->co && !job->busy) {
         qemu_coroutine_enter(job->co, NULL);
     }
@@ -278,6 +277,7 @@ void block_job_enter(BlockJob *job)
 void block_job_cancel(BlockJob *job)
 {
     job->cancelled = true;
+    block_job_iostatus_reset(job);
     block_job_enter(job);
 }
 
-- 
2.5.5

[Qemu-devel] [PATCH v4 2/5] blockjob: add pause points

[Stefan Hajnoczi]

Block jobs are coroutines that usually perform I/O but sometimes also
sleep or yield.  Currently only sleeping or yielded block jobs can be
paused.  This means jobs that do not sleep or yield (using
block_job_yield()) are unaffected by block_job_pause().

Add block_job_pause_point() so that block jobs can mark quiescent points
that are suitable for pausing.  This solves the problem that it can take
a block job a long time to pause if it is performing a long series of
I/O operations.

Transitioning to paused state involves a .pause()/.resume() callback.
These callbacks are used to ensure that I/O and event loop activity has
ceased while the job is at a pause point.

Note that this patch introduces a stricter pause state than previously.
The job->busy flag was incorrectly documented as a quiescent state
without I/O pending.  This is violated by any job that has I/O pending
across sleep or block_job_yield(), like the mirror block job.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockjob.c               | 44 ++++++++++++++++++++++++++++++++++++--------
 include/block/blockjob.h | 35 ++++++++++++++++++++++++++++++++---
 2 files changed, 68 insertions(+), 11 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 463bccf..1a383d1 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -247,6 +247,30 @@ void block_job_complete(BlockJob *job, Error **errp)
     job->driver->complete(job, errp);
 }
 
+void block_job_pause_point(BlockJob *job)
+{
+    if (!block_job_is_paused(job)) {
+        return;
+    }
+    if (block_job_is_cancelled(job)) {
+        return;
+    }
+
+    if (job->driver->pause) {
+        job->driver->pause(job);
+    }
+
+    job->paused = true;
+    job->busy = false;
+    qemu_coroutine_yield(); /* wait for block_job_resume() */
+    job->busy = true;
+    job->paused = false;
+
+    if (job->driver->resume) {
+        job->driver->resume(job);
+    }
+}
+
 void block_job_pause(BlockJob *job)
 {
     job->pause_count++;
@@ -360,13 +384,13 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns)
         return;
     }
 
-    job->busy = false;
-    if (block_job_is_paused(job)) {
-        qemu_coroutine_yield();
-    } else {
+    if (!block_job_is_paused(job)) {
+        job->busy = false;
         co_aio_sleep_ns(blk_get_aio_context(job->blk), type, ns);
+        job->busy = true;
     }
-    job->busy = true;
+
+    block_job_pause_point(job);
 }
 
 void block_job_yield(BlockJob *job)
@@ -378,9 +402,13 @@ void block_job_yield(BlockJob *job)
         return;
     }
 
-    job->busy = false;
-    qemu_coroutine_yield();
-    job->busy = true;
+    if (!block_job_is_paused(job)) {
+        job->busy = false;
+        qemu_coroutine_yield();
+        job->busy = true;
+    }
+
+    block_job_pause_point(job);
 }
 
 BlockJobInfo *block_job_query(BlockJob *job)
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 00ac418..154c48b 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -70,6 +70,20 @@ typedef struct BlockJobDriver {
      * never both.
      */
     void (*abort)(BlockJob *job);
+
+    /**
+     * If the callback is not NULL, it will be invoked when the job transitions
+     * into the paused state.  Paused jobs must not perform any asynchronous
+     * I/O or event loop activity.  This callback is used to quiesce jobs.
+     */
+    void (*pause)(BlockJob *job);
+
+    /**
+     * If the callback is not NULL, it will be invoked when the job transitions
+     * out of the paused state.  Any asynchronous I/O or event loop activity
+     * should be restarted from this callback.
+     */
+    void (*resume)(BlockJob *job);
 } BlockJobDriver;
 
 /**
@@ -119,13 +133,19 @@ struct BlockJob {
     bool user_paused;
 
     /**
-     * Set to false by the job while it is in a quiescent state, where
-     * no I/O is pending and the job has yielded on any condition
-     * that is not detected by #aio_poll, such as a timer.
+     * Set to false by the job while the coroutine has yielded and may be
+     * re-entered by block_job_enter().  There may still be I/O or event loop
+     * activity pending.
      */
     bool busy;
 
     /**
+     * Set to true by the job while it is in a quiescent state, where
+     * no I/O or event loop activity is pending.
+     */
+    bool paused;
+
+    /**
      * Set to true when the job is ready to be completed.
      */
     bool ready;
@@ -299,6 +319,15 @@ bool block_job_is_cancelled(BlockJob *job);
 BlockJobInfo *block_job_query(BlockJob *job);
 
 /**
+ * block_job_pause_point:
+ * @job: The job that is ready to pause.
+ *
+ * Pause now if block_job_pause() has been called.  Block jobs that perform
+ * lots of I/O must call this between requests so that the job can be paused.
+ */
+void coroutine_fn block_job_pause_point(BlockJob *job);
+
+/**
  * block_job_pause:
  * @job: The job to be paused.
  *
-- 
2.5.5

[Qemu-devel] [PATCH v4 3/5] blockjob: add AioContext attached callback

[Stefan Hajnoczi]

Block jobs that use additional BDSes or event loop resources need a
callback to get their affairs in order when the AioContext is switched.

Simple block jobs don't need an attach callback, they automatically work
thanks to the generic attach/detach notifiers that this patch adds.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 blockjob.c               | 38 ++++++++++++++++++++++++++++++++++++++
 include/block/blockjob.h |  7 +++++++
 2 files changed, 45 insertions(+)

diff --git a/blockjob.c b/blockjob.c
index 1a383d1..a5ce982 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -60,6 +60,38 @@ BlockJob *block_job_next(BlockJob *job)
     return QLIST_NEXT(job, job_list);
 }
 
+static void block_job_attached_aio_context(AioContext *new_context,
+                                           void *opaque)
+{
+    BlockJob *job = opaque;
+
+    if (job->driver->attached_aio_context) {
+        job->driver->attached_aio_context(job, new_context);
+    }
+
+    block_job_resume(job);
+}
+
+static void block_job_detach_aio_context(void *opaque)
+{
+    BlockJob *job = opaque;
+
+    /* In case the job terminates during aio_poll()... */
+    block_job_ref(job);
+
+    block_job_pause(job);
+
+    if (!job->paused) {
+        /* If job is !job->busy this kicks it into the next pause point. */
+        block_job_enter(job);
+    }
+    while (!job->paused && !job->completed) {
+        aio_poll(blk_get_aio_context(job->blk), true);
+    }
+
+    block_job_unref(job);
+}
+
 void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
                        int64_t speed, BlockCompletionFunc *cb,
                        void *opaque, Error **errp)
@@ -92,6 +124,9 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
 
     QLIST_INSERT_HEAD(&block_jobs, job, job_list);
 
+    blk_add_aio_context_notifier(blk, block_job_attached_aio_context,
+                                 block_job_detach_aio_context, job);
+
     /* Only set speed when necessary to avoid NotSupported error */
     if (speed != 0) {
         Error *local_err = NULL;
@@ -117,6 +152,9 @@ void block_job_unref(BlockJob *job)
         BlockDriverState *bs = blk_bs(job->blk);
         bs->job = NULL;
         bdrv_op_unblock_all(bs, job->blocker);
+        blk_remove_aio_context_notifier(job->blk,
+                                        block_job_attached_aio_context,
+                                        block_job_detach_aio_context, job);
         blk_unref(job->blk);
         error_free(job->blocker);
         g_free(job->id);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 154c48b..98550bc 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -84,6 +84,13 @@ typedef struct BlockJobDriver {
      * should be restarted from this callback.
      */
     void (*resume)(BlockJob *job);
+
+    /*
+     * If the callback is not NULL, it will be invoked before the job is
+     * resumed in a new AioContext.  This is the place to move any resources
+     * besides job->blk to the new AioContext.
+     */
+    void (*attached_aio_context)(BlockJob *job, AioContext *new_context);
 } BlockJobDriver;
 
 /**
-- 
2.5.5

[Qemu-devel] [PATCH v4 4/5] mirror: follow AioContext change gracefully

[Stefan Hajnoczi]

Add block_job_pause_point() calls to mark quiescent points and make sure
to complete in-flight requests when switching AioContexts.

This patch solves undefined behavior in the mirror block job when the
BDS AioContext is changed by dataplane.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c | 45 +++++++++++++++++++++++++++++++++++++--------
 1 file changed, 37 insertions(+), 8 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 80fd3c7..4c4e55b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -331,6 +331,8 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
         mirror_wait_for_io(s);
     }
 
+    block_job_pause_point(&s->common);
+
     /* Find the number of consective dirty chunks following the first dirty
      * one, and wait for in flight requests in them. */
     while (nb_chunks * sectors_per_chunk < (s->buf_size >> BDRV_SECTOR_BITS)) {
@@ -581,6 +583,8 @@ static void coroutine_fn mirror_run(void *opaque)
             if (now - last_pause_ns > SLICE_TIME) {
                 last_pause_ns = now;
                 block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, 0);
+            } else {
+                block_job_pause_point(&s->common);
             }
 
             if (block_job_is_cancelled(&s->common)) {
@@ -612,6 +616,8 @@ static void coroutine_fn mirror_run(void *opaque)
             goto immediate_exit;
         }
 
+        block_job_pause_point(&s->common);
+
         cnt = bdrv_get_dirty_count(s->dirty_bitmap);
         /* s->common.offset contains the number of bytes already processed so
          * far, cnt is the number of dirty sectors remaining and
@@ -781,18 +787,41 @@ static void mirror_complete(BlockJob *job, Error **errp)
     block_job_enter(&s->common);
 }
 
+/* There is no matching mirror_resume() because mirror_run() will begin
+ * iterating again when the job is resumed.
+ */
+static void mirror_pause(BlockJob *job)
+{
+    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
+
+    while (s->in_flight > 0) {
+        aio_poll(blk_get_aio_context(job->blk), true);
+    }
+}
+
+static void mirror_attached_aio_context(BlockJob *job, AioContext *new_context)
+{
+    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
+
+    blk_set_aio_context(s->target, new_context);
+}
+
 static const BlockJobDriver mirror_job_driver = {
-    .instance_size = sizeof(MirrorBlockJob),
-    .job_type      = BLOCK_JOB_TYPE_MIRROR,
-    .set_speed     = mirror_set_speed,
-    .complete      = mirror_complete,
+    .instance_size          = sizeof(MirrorBlockJob),
+    .job_type               = BLOCK_JOB_TYPE_MIRROR,
+    .set_speed              = mirror_set_speed,
+    .complete               = mirror_complete,
+    .pause                  = mirror_pause,
+    .attached_aio_context   = mirror_attached_aio_context,
 };
 
 static const BlockJobDriver commit_active_job_driver = {
-    .instance_size = sizeof(MirrorBlockJob),
-    .job_type      = BLOCK_JOB_TYPE_COMMIT,
-    .set_speed     = mirror_set_speed,
-    .complete      = mirror_complete,
+    .instance_size          = sizeof(MirrorBlockJob),
+    .job_type               = BLOCK_JOB_TYPE_COMMIT,
+    .set_speed              = mirror_set_speed,
+    .complete               = mirror_complete,
+    .pause                  = mirror_pause,
+    .attached_aio_context   = mirror_attached_aio_context,
 };
 
 static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,
-- 
2.5.5

[Qemu-devel] [PATCH v4 5/5] backup: follow AioContext change gracefully

[Stefan Hajnoczi]

Move s->target to the new AioContext when there is an AioContext change.

The backup_run() coroutine does not use asynchronous I/O so there is no
need to wait for in-flight requests in a BlockJobDriver->pause()
callback.

Guest writes are intercepted by the backup job.  Treat them as guest
activity and do it even while the job is paused.  This is necessary
since the only alternative would be to fail a job that experienced guest
writes during pause once the job is resumed.  In practice the guest
writes don't interfere with AioContext switching since bdrv_drain() is
used by bdrv_set_aio_context().

Loops already contain pause points because of block_job_sleep_ns() calls
in the yield_and_check() helper function.  It is necessary to convert a
raw qemu_coroutine_yield() to block_job_yield() so the
MIRROR_SYNC_MODE_NONE case can pause.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/backup.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index feeb9f8..581269b 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -246,12 +246,20 @@ static void backup_abort(BlockJob *job)
     }
 }
 
+static void backup_attached_aio_context(BlockJob *job, AioContext *aio_context)
+{
+    BackupBlockJob *s = container_of(job, BackupBlockJob, common);
+
+    blk_set_aio_context(s->target, aio_context);
+}
+
 static const BlockJobDriver backup_job_driver = {
-    .instance_size  = sizeof(BackupBlockJob),
-    .job_type       = BLOCK_JOB_TYPE_BACKUP,
-    .set_speed      = backup_set_speed,
-    .commit         = backup_commit,
-    .abort          = backup_abort,
+    .instance_size          = sizeof(BackupBlockJob),
+    .job_type               = BLOCK_JOB_TYPE_BACKUP,
+    .set_speed              = backup_set_speed,
+    .commit                 = backup_commit,
+    .abort                  = backup_abort,
+    .attached_aio_context   = backup_attached_aio_context,
 };
 
 static BlockErrorAction backup_error_action(BackupBlockJob *job,
@@ -392,9 +400,7 @@ static void coroutine_fn backup_run(void *opaque)
         while (!block_job_is_cancelled(&job->common)) {
             /* Yield until the job is cancelled.  We just let our before_write
              * notify callback service CoW requests. */
-            job->common.busy = false;
-            qemu_coroutine_yield();
-            job->common.busy = true;
+            block_job_yield(&job->common);
         }
     } else if (job->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
         ret = backup_run_incremental(job);
-- 
2.5.5

Re: [Qemu-devel] [PATCH v4 0/5] blockjob: AioContext change support for mirror and backup

[Jason J. Herne]

On 06/14/2016 02:17 PM, Stefan Hajnoczi wrote:
> v4:
>   * Add .pause()/.resume() callbacks to really quiesce during
>     block_job_pause_point() [Paolo]
>   * Add AioContext change support for backup block job
>   * Tested drive_mirror + migration and drive_backup + reboot
>
> v3:
>   * Push infrastructure down into blockjob.c so other jobs can reuse it [Stefan]
>   * Tested with drive_mirror + migration [Stefan]
>
> v2:
>   * Fam introduced the concept of a synchronous aio_poll() loop to quiesce the
>     block job during detach
>
> When dataplane is enabled or disabled the drive switches to a new AioContext.
> The mirror and backup block jobs must also move to the new AioContext so that
> drive accesses are always made within its AioContext.
>
> This series extends the block job pause functionality so that detaching from an
> AioContext pauses the job and attaching to the new AioContext resumes the job.
>
> Pause points are added to the mirror job so that long I/O loops can yield for
> an AioContext switch.  Other block jobs need pause points too but this can be
> done as a follow-up series.

I just tested v4 on s390. It appears to fix our original problem without 
any hiccups.
Thank you Stefan! :) Let me know if you need any more testing.

-- 
-- Jason J. Herne (jjherne@linux.vnet.ibm.com)

Re: [Qemu-devel] [PATCH v4 1/5] blockjob: move iostatus reset out of block_job_enter()

[Fam Zheng]

On Tue, 06/14 19:17, Stefan Hajnoczi wrote:
> The QMP block-job-resume command and cancellation may want to reset the
> job's iostatus.  The next patches add a user who does not want to reset
> iostatus so move it up to block_job_enter() callers.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  blockdev.c | 1 +
>  blockjob.c | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/blockdev.c b/blockdev.c
> index 7fd515a..19b963c 100644
> --- a/blockdev.c
> +++ b/blockdev.c
> @@ -3799,6 +3799,7 @@ void qmp_block_job_resume(const char *device, Error **errp)
>  
>      job->user_paused = false;
>      trace_qmp_block_job_resume(job);
> +    block_job_iostatus_reset(job);
>      block_job_resume(job);
>      aio_context_release(aio_context);
>  }
> diff --git a/blockjob.c b/blockjob.c
> index c095cc5..463bccf 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -269,7 +269,6 @@ void block_job_resume(BlockJob *job)
>  
>  void block_job_enter(BlockJob *job)
>  {
> -    block_job_iostatus_reset(job);
>      if (job->co && !job->busy) {
>          qemu_coroutine_enter(job->co, NULL);
>      }
> @@ -278,6 +277,7 @@ void block_job_enter(BlockJob *job)
>  void block_job_cancel(BlockJob *job)
>  {
>      job->cancelled = true;
> +    block_job_iostatus_reset(job);
>      block_job_enter(job);
>  }
>  
> -- 
> 2.5.5
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v4 2/5] blockjob: add pause points

[Paolo Bonzini]

On 14/06/2016 20:17, Stefan Hajnoczi wrote:
> Block jobs are coroutines that usually perform I/O but sometimes also
> sleep or yield.  Currently only sleeping or yielded block jobs can be
> paused.  This means jobs that do not sleep or yield (using
> block_job_yield()) are unaffected by block_job_pause().
> 
> Add block_job_pause_point() so that block jobs can mark quiescent points
> that are suitable for pausing.  This solves the problem that it can take
> a block job a long time to pause if it is performing a long series of
> I/O operations.
> 
> Transitioning to paused state involves a .pause()/.resume() callback.
> These callbacks are used to ensure that I/O and event loop activity has
> ceased while the job is at a pause point.
> 
> Note that this patch introduces a stricter pause state than previously.
> The job->busy flag was incorrectly documented as a quiescent state
> without I/O pending.  This is violated by any job that has I/O pending
> across sleep or block_job_yield(), like the mirror block job.

Right, we should document job->busy as a quiescent state where no one
will re-enter the coroutine.

Paolo

Re: [Qemu-devel] [PATCH v4 0/5] blockjob: AioContext change support for mirror and backup

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1444 bytes --]

On Tue, Jun 14, 2016 at 03:06:11PM -0400, Jason J. Herne wrote:
> On 06/14/2016 02:17 PM, Stefan Hajnoczi wrote:
> > v4:
> >   * Add .pause()/.resume() callbacks to really quiesce during
> >     block_job_pause_point() [Paolo]
> >   * Add AioContext change support for backup block job
> >   * Tested drive_mirror + migration and drive_backup + reboot
> > 
> > v3:
> >   * Push infrastructure down into blockjob.c so other jobs can reuse it [Stefan]
> >   * Tested with drive_mirror + migration [Stefan]
> > 
> > v2:
> >   * Fam introduced the concept of a synchronous aio_poll() loop to quiesce the
> >     block job during detach
> > 
> > When dataplane is enabled or disabled the drive switches to a new AioContext.
> > The mirror and backup block jobs must also move to the new AioContext so that
> > drive accesses are always made within its AioContext.
> > 
> > This series extends the block job pause functionality so that detaching from an
> > AioContext pauses the job and attaching to the new AioContext resumes the job.
> > 
> > Pause points are added to the mirror job so that long I/O loops can yield for
> > an AioContext switch.  Other block jobs need pause points too but this can be
> > done as a follow-up series.
> 
> I just tested v4 on s390. It appears to fix our original problem without any
> hiccups.
> Thank you Stefan! :) Let me know if you need any more testing.

Thank you!

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v4 2/5] blockjob: add pause points

[Fam Zheng]

On Tue, 06/14 19:17, Stefan Hajnoczi wrote:
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -247,6 +247,30 @@ void block_job_complete(BlockJob *job, Error **errp)
>      job->driver->complete(job, errp);
>  }
>  
> +void block_job_pause_point(BlockJob *job)
> +{
> +    if (!block_job_is_paused(job)) {

I find this check ...

> +        return;
> +    }
> +    if (block_job_is_cancelled(job)) {
> +        return;
> +    }
> +
> +    if (job->driver->pause) {
> +        job->driver->pause(job);
> +    }
> +
> +    job->paused = true;

... and this assignment confusing. After reading more, I think we ought to
rename block_job_is_paused to block_job_should_pause and mark it static, in a
separate patch.

> +    job->busy = false;
> +    qemu_coroutine_yield(); /* wait for block_job_resume() */
> +    job->busy = true;
> +    job->paused = false;

Worth to "assert(!job->pause_count)" (or "assert(!block_job_should_pause(job))")?

Regardless,

Reviewed-by: Fam Zheng <famz@redhat.com>

> +
> +    if (job->driver->resume) {
> +        job->driver->resume(job);
> +    }
> +}
> +

Re: [Qemu-devel] [PATCH v4 4/5] mirror: follow AioContext change gracefully

[Paolo Bonzini]

On 14/06/2016 20:17, Stefan Hajnoczi wrote:
> +/* There is no matching mirror_resume() because mirror_run() will begin
> + * iterating again when the job is resumed.
> + */
> +static void mirror_pause(BlockJob *job)
> +{
> +    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
> +
> +    while (s->in_flight > 0) {
> +        aio_poll(blk_get_aio_context(job->blk), true);
> +    }

This is calling aio_poll from a coroutine, which is ugly -- see Fam's
recent introduction of bdrv_co_drain.  I think this should call
mirror_drain instead.

Paolo

> +}
> +

Re: [Qemu-devel] [PATCH v4 0/5] blockjob: AioContext change support for mirror and backup

[Paolo Bonzini]

On 14/06/2016 20:17, Stefan Hajnoczi wrote:
> v4:
>  * Add .pause()/.resume() callbacks to really quiesce during
>    block_job_pause_point() [Paolo]
>  * Add AioContext change support for backup block job
>  * Tested drive_mirror + migration and drive_backup + reboot

Also a nice improvement over v3!

I think the description of job->busy can be improved further and, more
important, I am not sure that mirror_pause is correct.  While you are at
it, you could annotate mirror_pause and (*pause) as coroutine_fn.

But anyway, apart from this

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo

Re: [Qemu-devel] [PATCH v4 2/5] blockjob: add pause points

[Paolo Bonzini]

On 15/06/2016 10:57, Fam Zheng wrote:
> > +    if (!block_job_is_paused(job)) {
> 
> I find this check ...
> 
> > +        return;
> > +    }
> > +    if (block_job_is_cancelled(job)) {
> > +        return;
> > +    }
> > +
> > +    if (job->driver->pause) {
> > +        job->driver->pause(job);
> > +    }
> > +
> > +    job->paused = true;
> 
> ... and this assignment confusing. After reading more, I think we ought to
> rename block_job_is_paused to block_job_should_pause and mark it static, in a
> separate patch.

Very good idea!

Paolo

Re: [Qemu-devel] [PATCH v4 3/5] blockjob: add AioContext attached callback

[Fam Zheng]

On Tue, 06/14 19:17, Stefan Hajnoczi wrote:
>  
> +static void block_job_attached_aio_context(AioContext *new_context,
> +                                           void *opaque)
> +{
> +    BlockJob *job = opaque;
> +
> +    if (job->driver->attached_aio_context) {
> +        job->driver->attached_aio_context(job, new_context);
> +    }
> +
> +    block_job_resume(job);
> +}
> +
> +static void block_job_detach_aio_context(void *opaque)
> +{
> +    BlockJob *job = opaque;
> +
> +    /* In case the job terminates during aio_poll()... */
> +    block_job_ref(job);
> +
> +    block_job_pause(job);
> +
> +    if (!job->paused) {
> +        /* If job is !job->busy this kicks it into the next pause point. */
> +        block_job_enter(job);
> +    }
> +    while (!job->paused && !job->completed) {
> +        aio_poll(blk_get_aio_context(job->blk), true);

For the complete part, we should do it like block_job_finish_sync:

    while (!job->completed) {
        aio_poll(job->deferred_to_main_loop ? qemu_get_aio_context() :
                                              blk_get_aio_context(job->blk),
                 true);
    }

> +    }
> +
> +    block_job_unref(job);
> +}
> +

Fam

Re: [Qemu-devel] [PATCH v4 3/5] blockjob: add AioContext attached callback

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1328 bytes --]

On Wed, Jun 15, 2016 at 05:05:28PM +0800, Fam Zheng wrote:
> On Tue, 06/14 19:17, Stefan Hajnoczi wrote:
> >  
> > +static void block_job_attached_aio_context(AioContext *new_context,
> > +                                           void *opaque)
> > +{
> > +    BlockJob *job = opaque;
> > +
> > +    if (job->driver->attached_aio_context) {
> > +        job->driver->attached_aio_context(job, new_context);
> > +    }
> > +
> > +    block_job_resume(job);
> > +}
> > +
> > +static void block_job_detach_aio_context(void *opaque)
> > +{
> > +    BlockJob *job = opaque;
> > +
> > +    /* In case the job terminates during aio_poll()... */
> > +    block_job_ref(job);
> > +
> > +    block_job_pause(job);
> > +
> > +    if (!job->paused) {
> > +        /* If job is !job->busy this kicks it into the next pause point. */
> > +        block_job_enter(job);
> > +    }
> > +    while (!job->paused && !job->completed) {
> > +        aio_poll(blk_get_aio_context(job->blk), true);
> 
> For the complete part, we should do it like block_job_finish_sync:
> 
>     while (!job->completed) {
>         aio_poll(job->deferred_to_main_loop ? qemu_get_aio_context() :
>                                               blk_get_aio_context(job->blk),
>                  true);
>     }

Will fix in v5.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v4 4/5] mirror: follow AioContext change gracefully

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 966 bytes --]

On Wed, Jun 15, 2016 at 10:57:49AM +0200, Paolo Bonzini wrote:
> On 14/06/2016 20:17, Stefan Hajnoczi wrote:
> > +/* There is no matching mirror_resume() because mirror_run() will begin
> > + * iterating again when the job is resumed.
> > + */
> > +static void mirror_pause(BlockJob *job)
> > +{
> > +    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
> > +
> > +    while (s->in_flight > 0) {
> > +        aio_poll(blk_get_aio_context(job->blk), true);
> > +    }
> 
> This is calling aio_poll from a coroutine, which is ugly -- see Fam's
> recent introduction of bdrv_co_drain.  I think this should call
> mirror_drain instead.

This is not called from coroutine context, so I couldn't use
mirror_drain().

BlockJobDriver->pause() and attached_aio_context() are both not
coroutine_fn.  That's because bdrv_attach_aio_context() is not
coroutine_fn and can be called from a monitor command or device
emulation code.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v4 2/5] blockjob: add pause points

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1283 bytes --]

On Wed, Jun 15, 2016 at 04:57:41PM +0800, Fam Zheng wrote:
> On Tue, 06/14 19:17, Stefan Hajnoczi wrote:
> > --- a/blockjob.c
> > +++ b/blockjob.c
> > @@ -247,6 +247,30 @@ void block_job_complete(BlockJob *job, Error **errp)
> >      job->driver->complete(job, errp);
> >  }
> >  
> > +void block_job_pause_point(BlockJob *job)
> > +{
> > +    if (!block_job_is_paused(job)) {
> 
> I find this check ...
> 
> > +        return;
> > +    }
> > +    if (block_job_is_cancelled(job)) {
> > +        return;
> > +    }
> > +
> > +    if (job->driver->pause) {
> > +        job->driver->pause(job);
> > +    }
> > +
> > +    job->paused = true;
> 
> ... and this assignment confusing. After reading more, I think we ought to
> rename block_job_is_paused to block_job_should_pause and mark it static, in a
> separate patch.
> 
> > +    job->busy = false;
> > +    qemu_coroutine_yield(); /* wait for block_job_resume() */
> > +    job->busy = true;
> > +    job->paused = false;
> 
> Worth to "assert(!job->pause_count)" (or "assert(!block_job_should_pause(job))")?
> 
> Regardless,
> 
> Reviewed-by: Fam Zheng <famz@redhat.com>

Nice solution!  I hesitated a bit with job->paused vs
block_job_is_paused() because the naming is indeed confusing.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v4 4/5] mirror: follow AioContext change gracefully

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1206 bytes --]



On 16/06/2016 12:17, Stefan Hajnoczi wrote:
> On Wed, Jun 15, 2016 at 10:57:49AM +0200, Paolo Bonzini wrote:
>> On 14/06/2016 20:17, Stefan Hajnoczi wrote:
>>> +/* There is no matching mirror_resume() because mirror_run() will begin
>>> + * iterating again when the job is resumed.
>>> + */
>>> +static void mirror_pause(BlockJob *job)
>>> +{
>>> +    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
>>> +
>>> +    while (s->in_flight > 0) {
>>> +        aio_poll(blk_get_aio_context(job->blk), true);
>>> +    }
>>
>> This is calling aio_poll from a coroutine, which is ugly -- see Fam's
>> recent introduction of bdrv_co_drain.  I think this should call
>> mirror_drain instead.
> 
> This is not called from coroutine context, so I couldn't use
> mirror_drain().
> 
> BlockJobDriver->pause() and attached_aio_context() are both not
> coroutine_fn.  That's because bdrv_attach_aio_context() is not
> coroutine_fn and can be called from a monitor command or device
> emulation code.

But mirror_pause is only called from block_job_pause_point, isn't it?
And block_job_pause_point is a coroutine_fn.

I think what you say was true for v3, though.

Paolo


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH v4 4/5] mirror: follow AioContext change gracefully

[Stefan Hajnoczi]

On Thu, Jun 16, 2016 at 11:21 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 16/06/2016 12:17, Stefan Hajnoczi wrote:
>> On Wed, Jun 15, 2016 at 10:57:49AM +0200, Paolo Bonzini wrote:
>>> On 14/06/2016 20:17, Stefan Hajnoczi wrote:
>>>> +/* There is no matching mirror_resume() because mirror_run() will begin
>>>> + * iterating again when the job is resumed.
>>>> + */
>>>> +static void mirror_pause(BlockJob *job)
>>>> +{
>>>> +    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
>>>> +
>>>> +    while (s->in_flight > 0) {
>>>> +        aio_poll(blk_get_aio_context(job->blk), true);
>>>> +    }
>>>
>>> This is calling aio_poll from a coroutine, which is ugly -- see Fam's
>>> recent introduction of bdrv_co_drain.  I think this should call
>>> mirror_drain instead.
>>
>> This is not called from coroutine context, so I couldn't use
>> mirror_drain().
>>
>> BlockJobDriver->pause() and attached_aio_context() are both not
>> coroutine_fn.  That's because bdrv_attach_aio_context() is not
>> coroutine_fn and can be called from a monitor command or device
>> emulation code.
>
> But mirror_pause is only called from block_job_pause_point, isn't it?
> And block_job_pause_point is a coroutine_fn.

You are right.  I managed to confuse myself!

I'll rework the APIs to make them coroutine_fn as appropriate.

Stefan

Re: [Qemu-devel] [PATCH v4 2/5] blockjob: add pause points

[Stefan Hajnoczi]

On Wed, Jun 15, 2016 at 9:53 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 14/06/2016 20:17, Stefan Hajnoczi wrote:
>> Block jobs are coroutines that usually perform I/O but sometimes also
>> sleep or yield.  Currently only sleeping or yielded block jobs can be
>> paused.  This means jobs that do not sleep or yield (using
>> block_job_yield()) are unaffected by block_job_pause().
>>
>> Add block_job_pause_point() so that block jobs can mark quiescent points
>> that are suitable for pausing.  This solves the problem that it can take
>> a block job a long time to pause if it is performing a long series of
>> I/O operations.
>>
>> Transitioning to paused state involves a .pause()/.resume() callback.
>> These callbacks are used to ensure that I/O and event loop activity has
>> ceased while the job is at a pause point.
>>
>> Note that this patch introduces a stricter pause state than previously.
>> The job->busy flag was incorrectly documented as a quiescent state
>> without I/O pending.  This is violated by any job that has I/O pending
>> across sleep or block_job_yield(), like the mirror block job.
>
> Right, we should document job->busy as a quiescent state where no one
> will re-enter the coroutine.

That statement doesn't correspond with how it's used:

block_job_sleep_ns() leaves a timer pending and the job will re-enter
when the timer expires.  So "no one will re-enter the coroutine" is
too strict.

The important thing is it's safe to call block_job_enter().  In the
block_job_sleep_ns() case the timer is cancelled to prevent doubly
re-entry.

The doc comment I have in v4 allows the block_job_sleep_ns() case:

  /*
   * Set to false by the job while the coroutine has yielded and may be
   * re-entered by block_job_enter().  There may still be I/O or event loop
   * activity pending.
   */
  bool busy;

Stefan

Re: [Qemu-devel] [PATCH v4 2/5] blockjob: add pause points

[Paolo Bonzini]

On 16/06/2016 15:17, Stefan Hajnoczi wrote:
>> Right, we should document job->busy as a quiescent state where no one
>> will re-enter the coroutine.
> 
> That statement doesn't correspond with how it's used:
> 
> block_job_sleep_ns() leaves a timer pending and the job will re-enter
> when the timer expires.  So "no one will re-enter the coroutine" is
> too strict.

And of course you're right. :)  What I (sloppily) meant was "where the
block job code will not re-enter the coroutine", which is what makes it
safe to call block_job_enter().

> The important thing is it's safe to call block_job_enter().  In the
> block_job_sleep_ns() case the timer is cancelled to prevent doubly
> re-entry.
> 
> The doc comment I have in v4 allows the block_job_sleep_ns() case:
> 
>   /*
>    * Set to false by the job while the coroutine has yielded and may be
>    * re-entered by block_job_enter().  There may still be I/O or event loop
>    * activity pending.
>    */
>   bool busy;

Sounds good!

Paolo