[v3,15/15] mirror: follow AioContext change gracefully

Commit Message

Stefan Hajnoczi June 13, 2016, 5:05 p.m. UTC

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 <[e-mail]>
Signed-off-by: Fam Zheng <[e-mail]>
---
 block/mirror.c | 42 ++++++++++++++++++++++++++++++++++--------
 1 file changed, 34 insertions(+), 8 deletions(-)

Comments

Paolo Bonzini June 14, 2016, 12:09 p.m. UTC | #1

On 13/06/2016 19:05, Stefan Hajnoczi wrote:
> index 80fd3c7..046e95c 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);

I/O can be in-flight here, so your description of the function is not
100% accurate.  I suppose it's okay because s->waiting_for_io is false
at the pause points you specified?

I guess a comment that explains it will do; and apart from this detail,
the patches are really nice.  Or maybe it's just that you scared with
that 00/15 in the cover letter. :->

Paolo

>          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,38 @@ static void mirror_complete(BlockJob *job, Error **errp)
>      block_job_enter(&s->common);
>  }

Stefan Hajnoczi June 14, 2016, 12:47 p.m. UTC | #2

On Tue, Jun 14, 2016 at 1:09 PM, Paolo Bonzini <[e-mail]> wrote:
> On 13/06/2016 19:05, Stefan Hajnoczi wrote:
>> index 80fd3c7..046e95c 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);
>
> I/O can be in-flight here, so your description of the function is not
> 100% accurate.  I suppose it's okay because s->waiting_for_io is false
> at the pause points you specified?

You are right, this brings up a subtle issue that already exists
without my patches:

The co-routine may sleep with aio requests pending.  This makes the
job "paused" (!job->busy) while there is still activity!

The solution for AioContext switching that this patch relies on is:

+static void mirror_detach_aio_context(BlockJob *job)
+{
+    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
+
+    while (s->in_flight > 0) {
+        aio_poll(blk_get_aio_context(job->blk), true);
+    }
+}

blockjob.c:block_job_detach_aio_context() first calls
block_job_pause(), then mirror_detach_aio_context() above, and finally
waits for !job->busy.

I chose this approach because it safely handles existing sleep or
block_job_yield() callers while there is still activity.

Now that I think more about it, we should solve the problem not just
for AioContext switching but also for pausing sleeping or
block_job_yield() coroutines.

I'll try to come up with a solution in v4.

Stefan

Patch

diff --git a/block/mirror.c b/block/mirror.c
index 80fd3c7..046e95c 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,38 @@  static void mirror_complete(BlockJob *job, Error **errp)
     block_job_enter(&s->common);
 }
 
+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 void mirror_detach_aio_context(BlockJob *job)
+{
+    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
+
+    while (s->in_flight > 0) {
+        aio_poll(blk_get_aio_context(job->blk), true);
+    }
+}
+
 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,
+    .attached_aio_context   = mirror_attached_aio_context,
+    .detach_aio_context     = mirror_detach_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,
+    .attached_aio_context   = mirror_attached_aio_context,
+    .detach_aio_context     = mirror_detach_aio_context,
 };
 
 static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target,