[Qemu-devel] [PATCH 0/2] blockjob: Fix dead loop with block_job_finish_sync on dataplane disks

[Qemu-devel] [PATCH 0/2] blockjob: Fix dead loop with block_job_finish_sync on dataplane disks

[Fam Zheng]

I noticed this bug when reviewing Max's bdrv_close_all() series, so here goes
the fix.


Fam Zheng (2):
  blockjob: Rename block_job_defer_to_main_loop
  blockjob: Fix hang in block_job_finish_sync

 block/backup.c            |  2 +-
 block/commit.c            |  2 +-
 block/mirror.c            |  2 +-
 block/stream.c            |  2 +-
 blockjob.c                |  8 +++++---
 include/block/blockjob.h  | 14 +++++++++-----
 tests/test-blockjob-txn.c |  2 +-
 7 files changed, 19 insertions(+), 13 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/2] blockjob: Rename block_job_defer_to_main_loop

[Fam Zheng]

The next patch will make this function more restrictive than it is now,
rename it and update comment to reflect the change.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/backup.c            |  2 +-
 block/commit.c            |  2 +-
 block/mirror.c            |  2 +-
 block/stream.c            |  2 +-
 blockjob.c                |  4 ++--
 include/block/blockjob.h  | 12 +++++++-----
 tests/test-blockjob-txn.c |  2 +-
 7 files changed, 14 insertions(+), 12 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 00cafdb..b429666 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -482,7 +482,7 @@ static void coroutine_fn backup_run(void *opaque)
 
     data = g_malloc(sizeof(*data));
     data->ret = ret;
-    block_job_defer_to_main_loop(&job->common, backup_complete, data);
+    block_job_coroutine_complete(&job->common, backup_complete, data);
 }
 
 void backup_start(BlockDriverState *bs, BlockDriverState *target,
diff --git a/block/commit.c b/block/commit.c
index 446a3ae..f6b93bd 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -181,7 +181,7 @@ out:
 
     data = g_malloc(sizeof(*data));
     data->ret = ret;
-    block_job_defer_to_main_loop(&s->common, commit_complete, data);
+    block_job_coroutine_complete(&s->common, commit_complete, data);
 }
 
 static void commit_set_speed(BlockJob *job, int64_t speed, Error **errp)
diff --git a/block/mirror.c b/block/mirror.c
index e9e151c..d665f2b 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -626,7 +626,7 @@ immediate_exit:
     /* Before we switch to target in mirror_exit, make sure data doesn't
      * change. */
     bdrv_drained_begin(s->common.bs);
-    block_job_defer_to_main_loop(&s->common, mirror_exit, data);
+    block_job_coroutine_complete(&s->common, mirror_exit, data);
 }
 
 static void mirror_set_speed(BlockJob *job, int64_t speed, Error **errp)
diff --git a/block/stream.c b/block/stream.c
index cafaa07..9572ce3 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -194,7 +194,7 @@ wait:
     data = g_malloc(sizeof(*data));
     data->ret = ret;
     data->reached_end = sector_num == end;
-    block_job_defer_to_main_loop(&s->common, stream_complete, data);
+    block_job_coroutine_complete(&s->common, stream_complete, data);
 }
 
 static void stream_set_speed(BlockJob *job, int64_t speed, Error **errp)
diff --git a/blockjob.c b/blockjob.c
index 80adb9d..4b16720 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -471,7 +471,7 @@ static void block_job_defer_to_main_loop_bh(void *opaque)
 
     qemu_bh_delete(data->bh);
 
-    /* Prevent race with block_job_defer_to_main_loop() */
+    /* Prevent race with block_job_coroutine_complete() */
     aio_context_acquire(data->aio_context);
 
     /* Fetch BDS AioContext again, in case it has changed */
@@ -487,7 +487,7 @@ static void block_job_defer_to_main_loop_bh(void *opaque)
     g_free(data);
 }
 
-void block_job_defer_to_main_loop(BlockJob *job,
+void block_job_coroutine_complete(BlockJob *job,
                                   BlockJobDeferToMainLoopFn *fn,
                                   void *opaque)
 {
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index d84ccd8..de59fc2 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -393,18 +393,20 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
 typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque);
 
 /**
- * block_job_defer_to_main_loop:
+ * block_job_coroutine_complete:
  * @job: The job
  * @fn: The function to run in the main loop
  * @opaque: The opaque value that is passed to @fn
  *
- * Execute a given function in the main loop with the BlockDriverState
- * AioContext acquired.  Block jobs must call bdrv_unref(), bdrv_close(), and
- * anything that uses bdrv_drain_all() in the main loop.
+ * Complete the block job coroutine and execute a given function in the main
+ * loop with the BlockDriverState AioContext acquired.  Block jobs must call
+ * bdrv_unref(), bdrv_close(), and anything that uses bdrv_drain_all() in the
+ * main loop. After calling this, the block job coroutine should complete right
+ * away, without doing any heavy operations such as I/O or block_job_yield().
  *
  * The @job AioContext is held while @fn executes.
  */
-void block_job_defer_to_main_loop(BlockJob *job,
+void block_job_coroutine_complete(BlockJob *job,
                                   BlockJobDeferToMainLoopFn *fn,
                                   void *opaque);
 
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index 34747e9..56442f2 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -57,7 +57,7 @@ static void coroutine_fn test_block_job_run(void *opaque)
         }
     }
 
-    block_job_defer_to_main_loop(job, test_block_job_complete,
+    block_job_coroutine_complete(job, test_block_job_complete,
                                  (void *)(intptr_t)s->rc);
 }
 
-- 
2.4.3

[Qemu-devel] [PATCH 2/2] blockjob: Fix hang in block_job_finish_sync

[Fam Zheng]

With a mirror job running on a virtio-blk dataplane disk, sending "q" to
HMP will cause a dead loop in block_job_finish_sync.

This is because the aio_poll() only processes the AIO context of bs
which has no more work to do, while the main loop BH that is scheduled
for setting the job->completed flag is never processed.

Fix this by adding a "ctx" pointer in BlockJob structure, to track which
context to poll for the block job to make progress. Its value is set to
the BDS context at block job creation, until
block_job_coroutine_complete() is called by the block job coroutine.
After that point, the block job's work is deferred to main loop BH.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 blockjob.c               | 4 +++-
 include/block/blockjob.h | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/blockjob.c b/blockjob.c
index 4b16720..4ea1ce0 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -74,6 +74,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
     job->opaque        = opaque;
     job->busy          = true;
     job->refcnt        = 1;
+    job->ctx           = bdrv_get_aio_context(bs);
     bs->job = job;
 
     /* Only set speed when necessary to avoid NotSupported error */
@@ -304,7 +305,7 @@ static int block_job_finish_sync(BlockJob *job,
         return -EBUSY;
     }
     while (!job->completed) {
-        aio_poll(bdrv_get_aio_context(bs), true);
+        aio_poll(job->ctx, true);
     }
     ret = (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret;
     block_job_unref(job);
@@ -497,6 +498,7 @@ void block_job_coroutine_complete(BlockJob *job,
     data->aio_context = bdrv_get_aio_context(job->bs);
     data->fn = fn;
     data->opaque = opaque;
+    job->ctx = qemu_get_aio_context();
 
     qemu_bh_schedule(data->bh);
 }
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index de59fc2..5c6a884 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -92,6 +92,8 @@ struct BlockJob {
      */
     char *id;
 
+    AioContext *ctx;
+
     /**
      * The coroutine that executes the job.  If not NULL, it is
      * reentered when busy is false and the job is cancelled.
-- 
2.4.3

Re: [Qemu-devel] [PATCH 2/2] blockjob: Fix hang in block_job_finish_sync

[Paolo Bonzini]

On 28/01/2016 08:02, Fam Zheng wrote:
> 
> This is because the aio_poll() only processes the AIO context of bs
> which has no more work to do, while the main loop BH that is scheduled
> for setting the job->completed flag is never processed.
> 
> Fix this by adding a "ctx" pointer in BlockJob structure, to track which
> context to poll for the block job to make progress. Its value is set to
> the BDS context at block job creation, until
> block_job_coroutine_complete() is called by the block job coroutine.
> After that point, the block job's work is deferred to main loop BH.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  blockjob.c               | 4 +++-
>  include/block/blockjob.h | 2 ++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/blockjob.c b/blockjob.c
> index 4b16720..4ea1ce0 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -74,6 +74,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
>      job->opaque        = opaque;
>      job->busy          = true;
>      job->refcnt        = 1;
> +    job->ctx           = bdrv_get_aio_context(bs);

Can the context change if dataplane is started/stopped in the middle of
a job?  (For example if you start migration).  Perhaps job->ctx == NULL
could mean "use bdrv_get_aio_context(bs)".

Paolo

>      bs->job = job;
>  
>      /* Only set speed when necessary to avoid NotSupported error */
> @@ -304,7 +305,7 @@ static int block_job_finish_sync(BlockJob *job,
>          return -EBUSY;
>      }
>      while (!job->completed) {
> -        aio_poll(bdrv_get_aio_context(bs), true);
> +        aio_poll(job->ctx, true);
>      }
>      ret = (job->cancelled && job->ret == 0) ? -ECANCELED : job->ret;
>      block_job_unref(job);
> @@ -497,6 +498,7 @@ void block_job_coroutine_complete(BlockJob *job,
>      data->aio_context = bdrv_get_aio_context(job->bs);
>      data->fn = fn;
>      data->opaque = opaque;
> +    job->ctx = qemu_get_aio_context();
>  
>      qemu_bh_schedule(data->bh);
>  }
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index de59fc2..5c6a884 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -92,6 +92,8 @@ struct BlockJob {
>       */
>      char *id;
>  
> +    AioContext *ctx;
> +
>      /**
>       * The coroutine that executes the job.  If not NULL, it is
>       * reentered when busy is false and the job is cancelled.
> -- 2.4.3

Re: [Qemu-devel] [PATCH 2/2] blockjob: Fix hang in block_job_finish_sync

[Fam Zheng]

On Thu, 01/28 10:10, Paolo Bonzini wrote:
> 
> 
> On 28/01/2016 08:02, Fam Zheng wrote:
> > 
> > This is because the aio_poll() only processes the AIO context of bs
> > which has no more work to do, while the main loop BH that is scheduled
> > for setting the job->completed flag is never processed.
> > 
> > Fix this by adding a "ctx" pointer in BlockJob structure, to track which
> > context to poll for the block job to make progress. Its value is set to
> > the BDS context at block job creation, until
> > block_job_coroutine_complete() is called by the block job coroutine.
> > After that point, the block job's work is deferred to main loop BH.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  blockjob.c               | 4 +++-
> >  include/block/blockjob.h | 2 ++
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/blockjob.c b/blockjob.c
> > index 4b16720..4ea1ce0 100644
> > --- a/blockjob.c
> > +++ b/blockjob.c
> > @@ -74,6 +74,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
> >      job->opaque        = opaque;
> >      job->busy          = true;
> >      job->refcnt        = 1;
> > +    job->ctx           = bdrv_get_aio_context(bs);
> 
> Can the context change if dataplane is started/stopped in the middle of
> a job?  (For example if you start migration).  Perhaps job->ctx == NULL
> could mean "use bdrv_get_aio_context(bs)".

Yes, that's a good idea.

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] blockjob: Rename block_job_defer_to_main_loop

[Stefan Hajnoczi]

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

On Thu, Jan 28, 2016 at 03:02:50PM +0800, Fam Zheng wrote:
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index d84ccd8..de59fc2 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -393,18 +393,20 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
>  typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque);
>  
>  /**
> - * block_job_defer_to_main_loop:
> + * block_job_coroutine_complete:
>   * @job: The job
>   * @fn: The function to run in the main loop
>   * @opaque: The opaque value that is passed to @fn
>   *
> - * Execute a given function in the main loop with the BlockDriverState
> - * AioContext acquired.  Block jobs must call bdrv_unref(), bdrv_close(), and
> - * anything that uses bdrv_drain_all() in the main loop.
> + * Complete the block job coroutine and execute a given function in the main

This function does not "complete the block job coroutine" so this seems
confusing to me.  How about "Call this function to schedule clean up
code to run in the main loop when completing a block job"?

That said, I'm not sure if changing the scope of this function is
necessary at all.  If the next patch adds a job->aio_context pointer
then it could set the pointer back after calling the user's function
from the main loop.  Then this function could also be used in cases
where the job/coroutine stays alive.

> + * loop with the BlockDriverState AioContext acquired.  Block jobs must call
> + * bdrv_unref(), bdrv_close(), and anything that uses bdrv_drain_all() in the
> + * main loop. After calling this, the block job coroutine should complete right
> + * away, without doing any heavy operations such as I/O or block_job_yield().

"heavy operations" is too vague.  What are the specific constraints
here?

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

Re: [Qemu-devel] [Qemu-block] [PATCH 2/2] blockjob: Fix hang in block_job_finish_sync

[Stefan Hajnoczi]

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

On Thu, Jan 28, 2016 at 10:10:06AM +0100, Paolo Bonzini wrote:
> 
> 
> On 28/01/2016 08:02, Fam Zheng wrote:
> > 
> > This is because the aio_poll() only processes the AIO context of bs
> > which has no more work to do, while the main loop BH that is scheduled
> > for setting the job->completed flag is never processed.
> > 
> > Fix this by adding a "ctx" pointer in BlockJob structure, to track which
> > context to poll for the block job to make progress. Its value is set to
> > the BDS context at block job creation, until
> > block_job_coroutine_complete() is called by the block job coroutine.
> > After that point, the block job's work is deferred to main loop BH.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  blockjob.c               | 4 +++-
> >  include/block/blockjob.h | 2 ++
> >  2 files changed, 5 insertions(+), 1 deletion(-)
> > 
> > diff --git a/blockjob.c b/blockjob.c
> > index 4b16720..4ea1ce0 100644
> > --- a/blockjob.c
> > +++ b/blockjob.c
> > @@ -74,6 +74,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
> >      job->opaque        = opaque;
> >      job->busy          = true;
> >      job->refcnt        = 1;
> > +    job->ctx           = bdrv_get_aio_context(bs);
> 
> Can the context change if dataplane is started/stopped in the middle of
> a job?  (For example if you start migration).  Perhaps job->ctx == NULL
> could mean "use bdrv_get_aio_context(bs)".

Yes, it can change.

Normally a block job doesn't need to know its AioContext since the
coroutine only runs from inside the AioContext anyway.

Perhaps it's easier to make this special case explicit with a flag in
job->deferred_to_main_loop so the synchronous wait can do:

while (!job->completed) {
    AioContext *ctx = job->deferred_to_main_loop ?
                      qemu_get_aio_context() :
                      bdrv_get_aio_context(bs);

    aio_poll(ctx, true);
}

That way there's no need to store a possibly stale AioContext pointer.

Stefan

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

Re: [Qemu-devel] [Qemu-block] [PATCH 1/2] blockjob: Rename block_job_defer_to_main_loop

[Fam Zheng]

On Thu, 01/28 11:53, Stefan Hajnoczi wrote:
> On Thu, Jan 28, 2016 at 03:02:50PM +0800, Fam Zheng wrote:
> > diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> > index d84ccd8..de59fc2 100644
> > --- a/include/block/blockjob.h
> > +++ b/include/block/blockjob.h
> > @@ -393,18 +393,20 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs,
> >  typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque);
> >  
> >  /**
> > - * block_job_defer_to_main_loop:
> > + * block_job_coroutine_complete:
> >   * @job: The job
> >   * @fn: The function to run in the main loop
> >   * @opaque: The opaque value that is passed to @fn
> >   *
> > - * Execute a given function in the main loop with the BlockDriverState
> > - * AioContext acquired.  Block jobs must call bdrv_unref(), bdrv_close(), and
> > - * anything that uses bdrv_drain_all() in the main loop.
> > + * Complete the block job coroutine and execute a given function in the main
> 
> This function does not "complete the block job coroutine" so this seems
> confusing to me.  How about "Call this function to schedule clean up
> code to run in the main loop when completing a block job"?

Sounds good.

> 
> That said, I'm not sure if changing the scope of this function is
> necessary at all.  If the next patch adds a job->aio_context pointer
> then it could set the pointer back after calling the user's function
> from the main loop.  Then this function could also be used in cases
> where the job/coroutine stays alive.

Yes, that can be done.

Fam