[02/14] block: Cancel jobs first in bdrv_close_all()

Commit Message

Kevin Wolf May 4, 2016, 9:39 a.m. UTC

So far, bdrv_close_all() first removed all root BlockDriverStates of
BlockBackends and monitor owned BDSes, and then assumed that the
remaining BDSes must be related to jobs and cancelled these jobs.

This order doesn't work that well any more when block jobs use
BlockBackends internally because then they will lose their BDS before
being cancelled.

This patch changes bdrv_close_all() to first cancel all jobs and then
remove all root BDSes from the remaining BBs.

Signed-off-by: Kevin Wolf <[e-mail]>
---
 block.c                  | 23 +----------------------
 blockjob.c               | 13 +++++++++++++
 include/block/blockjob.h |  7 +++++++
 3 files changed, 21 insertions(+), 22 deletions(-)

Comments

Alberto Garcia May 6, 2016, 9:17 a.m. UTC | #1

On Wed 04 May 2016 11:39:13 AM CEST, Kevin Wolf wrote:

>  void bdrv_close_all(void)
>  {
> -    BlockDriverState *bs;
> -    AioContext *aio_context;
> +    block_job_cancel_sync_all();
>  
>      /* Drop references from requests still in flight, such as canceled block
>       * jobs whose AIO context has not been polled yet */
> @@ -2174,26 +2173,6 @@ void bdrv_close_all(void)
>  
>      blk_remove_all_bs();
>      blockdev_close_all_bdrv_states();
> -
> -    /* Cancel all block jobs */
> -    while (!QTAILQ_EMPTY(&all_bdrv_states)) {
> -        QTAILQ_FOREACH(bs, &all_bdrv_states, bs_list) {
> -            aio_context = bdrv_get_aio_context(bs);
> -
> -            aio_context_acquire(aio_context);
> -            if (bs->job) {
> -                block_job_cancel_sync(bs->job);
> -                aio_context_release(aio_context);
> -                break;
> -            }
> -            aio_context_release(aio_context);
> -        }
> -
> -        /* All the remaining BlockDriverStates are referenced directly or
> -         * indirectly from block jobs, so there needs to be at least one BDS
> -         * directly used by a block job */
> -        assert(bs);
> -    }
>  }

You could add an assert(QTAILQ_EMPTY(&all_bdrv_states)) as it was
suggested some days ago.

The rest looks good.

Reviewed-by: Alberto Garcia <[e-mail]>

Berto

Max Reitz May 13, 2016, 1:14 p.m. UTC | #2

On 04.05.2016 11:39, Kevin Wolf wrote:
> So far, bdrv_close_all() first removed all root BlockDriverStates of
> BlockBackends and monitor owned BDSes, and then assumed that the
> remaining BDSes must be related to jobs and cancelled these jobs.
> 
> This order doesn't work that well any more when block jobs use
> BlockBackends internally because then they will lose their BDS before
> being cancelled.
> 
> This patch changes bdrv_close_all() to first cancel all jobs and then
> remove all root BDSes from the remaining BBs.
> 
> Signed-off-by: Kevin Wolf <[e-mail]>
> ---
>  block.c                  | 23 +----------------------
>  blockjob.c               | 13 +++++++++++++
>  include/block/blockjob.h |  7 +++++++
>  3 files changed, 21 insertions(+), 22 deletions(-)

I agree with Berto about the assertion because it was me who suggested
it. :-)

And because I didn't give Berto an R-b because of that I'm afraid I
can't give you one either. O:-)

Max

Patch

diff --git a/block.c b/block.c
index 37aa0a3..bf6c204 100644
--- a/block.c
+++ b/block.c
@@ -2165,8 +2165,7 @@  static void bdrv_close(BlockDriverState *bs)
 
 void bdrv_close_all(void)
 {
-    BlockDriverState *bs;
-    AioContext *aio_context;
+    block_job_cancel_sync_all();
 
     /* Drop references from requests still in flight, such as canceled block
      * jobs whose AIO context has not been polled yet */
@@ -2174,26 +2173,6 @@  void bdrv_close_all(void)
 
     blk_remove_all_bs();
     blockdev_close_all_bdrv_states();
-
-    /* Cancel all block jobs */
-    while (!QTAILQ_EMPTY(&all_bdrv_states)) {
-        QTAILQ_FOREACH(bs, &all_bdrv_states, bs_list) {
-            aio_context = bdrv_get_aio_context(bs);
-
-            aio_context_acquire(aio_context);
-            if (bs->job) {
-                block_job_cancel_sync(bs->job);
-                aio_context_release(aio_context);
-                break;
-            }
-            aio_context_release(aio_context);
-        }
-
-        /* All the remaining BlockDriverStates are referenced directly or
-         * indirectly from block jobs, so there needs to be at least one BDS
-         * directly used by a block job */
-        assert(bs);
-    }
 }
 
 static void change_parent_backing_link(BlockDriverState *from,
diff --git a/blockjob.c b/blockjob.c
index 0f1bc77..e916b41 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -331,6 +331,19 @@  int block_job_cancel_sync(BlockJob *job)
     return block_job_finish_sync(job, &block_job_cancel_err, NULL);
 }
 
+void block_job_cancel_sync_all(void)
+{
+    BlockJob *job;
+    AioContext *aio_context;
+
+    while ((job = QLIST_FIRST(&block_jobs))) {
+        aio_context = bdrv_get_aio_context(job->bs);
+        aio_context_acquire(aio_context);
+        block_job_cancel_sync(job);
+        aio_context_release(aio_context);
+    }
+}
+
 int block_job_complete_sync(BlockJob *job, Error **errp)
 {
     return block_job_finish_sync(job, &block_job_complete, errp);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 30bb2c6..4ac6831 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -371,6 +371,13 @@  bool block_job_is_paused(BlockJob *job);
 int block_job_cancel_sync(BlockJob *job);
 
 /**
+ * block_job_cancel_sync_all:
+ *
+ * Synchronously cancels all jobs using block_job_cancel_sync().
+ */
+void block_job_cancel_sync_all(void);
+
+/**
  * block_job_complete_sync:
  * @job: The job to be completed.
  * @errp: Error object which may be set by block_job_complete(); this is not