yeahuh / rpms / qemu-kvm

Forked from rpms/qemu-kvm 2 years ago
Clone
ae23c9
From 6dca7cc6632a4b4632786e8a087289dff1ad6ef8 Mon Sep 17 00:00:00 2001
ae23c9
From: Kevin Wolf <kwolf@redhat.com>
ae23c9
Date: Wed, 10 Oct 2018 20:08:41 +0100
ae23c9
Subject: [PATCH 10/49] block: Really pause block jobs on drain
ae23c9
ae23c9
RH-Author: Kevin Wolf <kwolf@redhat.com>
ae23c9
Message-id: <20181010200843.6710-8-kwolf@redhat.com>
ae23c9
Patchwork-id: 82587
ae23c9
O-Subject: [RHEL-8 qemu-kvm PATCH 07/44] block: Really pause block jobs on drain
ae23c9
Bugzilla: 1637976
ae23c9
RH-Acked-by: Max Reitz <mreitz@redhat.com>
ae23c9
RH-Acked-by: John Snow <jsnow@redhat.com>
ae23c9
RH-Acked-by: Thomas Huth <thuth@redhat.com>
ae23c9
ae23c9
We already requested that block jobs be paused in .bdrv_drained_begin,
ae23c9
but no guarantee was made that the job was actually inactive at the
ae23c9
point where bdrv_drained_begin() returned.
ae23c9
ae23c9
This introduces a new callback BdrvChildRole.bdrv_drained_poll() and
ae23c9
uses it to make bdrv_drain_poll() consider block jobs using the node to
ae23c9
be drained.
ae23c9
ae23c9
For the test case to work as expected, we have to switch from
ae23c9
block_job_sleep_ns() to qemu_co_sleep_ns() so that the test job is even
ae23c9
considered active and must be waited for when draining the node.
ae23c9
ae23c9
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
ae23c9
(cherry picked from commit 89bd030533e3592ca0a995450dcfc5d53e459e20)
ae23c9
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
ae23c9
Signed-off-by: Danilo C. L. de Paula <ddepaula@redhat.com>
ae23c9
---
ae23c9
 block.c                      |  9 +++++++++
ae23c9
 block/io.c                   | 40 ++++++++++++++++++++++++++++++++++------
ae23c9
 block/mirror.c               |  8 ++++++++
ae23c9
 blockjob.c                   | 23 +++++++++++++++++++++++
ae23c9
 include/block/block.h        |  8 ++++++++
ae23c9
 include/block/block_int.h    |  7 +++++++
ae23c9
 include/block/blockjob_int.h |  8 ++++++++
ae23c9
 tests/test-bdrv-drain.c      | 18 ++++++++++--------
ae23c9
 8 files changed, 107 insertions(+), 14 deletions(-)
ae23c9
ae23c9
diff --git a/block.c b/block.c
ae23c9
index 10a1ece..0d9698a 100644
ae23c9
--- a/block.c
ae23c9
+++ b/block.c
ae23c9
@@ -821,6 +821,12 @@ static void bdrv_child_cb_drained_begin(BdrvChild *child)
ae23c9
     bdrv_drained_begin(bs);
ae23c9
 }
ae23c9
 
ae23c9
+static bool bdrv_child_cb_drained_poll(BdrvChild *child)
ae23c9
+{
ae23c9
+    BlockDriverState *bs = child->opaque;
ae23c9
+    return bdrv_drain_poll(bs, NULL);
ae23c9
+}
ae23c9
+
ae23c9
 static void bdrv_child_cb_drained_end(BdrvChild *child)
ae23c9
 {
ae23c9
     BlockDriverState *bs = child->opaque;
ae23c9
@@ -905,6 +911,7 @@ const BdrvChildRole child_file = {
ae23c9
     .get_parent_desc = bdrv_child_get_parent_desc,
ae23c9
     .inherit_options = bdrv_inherited_options,
ae23c9
     .drained_begin   = bdrv_child_cb_drained_begin,
ae23c9
+    .drained_poll    = bdrv_child_cb_drained_poll,
ae23c9
     .drained_end     = bdrv_child_cb_drained_end,
ae23c9
     .attach          = bdrv_child_cb_attach,
ae23c9
     .detach          = bdrv_child_cb_detach,
ae23c9
@@ -929,6 +936,7 @@ const BdrvChildRole child_format = {
ae23c9
     .get_parent_desc = bdrv_child_get_parent_desc,
ae23c9
     .inherit_options = bdrv_inherited_fmt_options,
ae23c9
     .drained_begin   = bdrv_child_cb_drained_begin,
ae23c9
+    .drained_poll    = bdrv_child_cb_drained_poll,
ae23c9
     .drained_end     = bdrv_child_cb_drained_end,
ae23c9
     .attach          = bdrv_child_cb_attach,
ae23c9
     .detach          = bdrv_child_cb_detach,
ae23c9
@@ -1048,6 +1056,7 @@ const BdrvChildRole child_backing = {
ae23c9
     .detach          = bdrv_backing_detach,
ae23c9
     .inherit_options = bdrv_backing_options,
ae23c9
     .drained_begin   = bdrv_child_cb_drained_begin,
ae23c9
+    .drained_poll    = bdrv_child_cb_drained_poll,
ae23c9
     .drained_end     = bdrv_child_cb_drained_end,
ae23c9
     .inactivate      = bdrv_child_cb_inactivate,
ae23c9
     .update_filename = bdrv_backing_update_filename,
ae23c9
diff --git a/block/io.c b/block/io.c
ae23c9
index 4d332c3..e260394 100644
ae23c9
--- a/block/io.c
ae23c9
+++ b/block/io.c
ae23c9
@@ -69,6 +69,23 @@ void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore)
ae23c9
     }
ae23c9
 }
ae23c9
 
ae23c9
+static bool bdrv_parent_drained_poll(BlockDriverState *bs, BdrvChild *ignore)
ae23c9
+{
ae23c9
+    BdrvChild *c, *next;
ae23c9
+    bool busy = false;
ae23c9
+
ae23c9
+    QLIST_FOREACH_SAFE(c, &bs->parents, next_parent, next) {
ae23c9
+        if (c == ignore) {
ae23c9
+            continue;
ae23c9
+        }
ae23c9
+        if (c->role->drained_poll) {
ae23c9
+            busy |= c->role->drained_poll(c);
ae23c9
+        }
ae23c9
+    }
ae23c9
+
ae23c9
+    return busy;
ae23c9
+}
ae23c9
+
ae23c9
 static void bdrv_merge_limits(BlockLimits *dst, const BlockLimits *src)
ae23c9
 {
ae23c9
     dst->opt_transfer = MAX(dst->opt_transfer, src->opt_transfer);
ae23c9
@@ -182,21 +199,32 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
ae23c9
 }
ae23c9
 
ae23c9
 /* Returns true if BDRV_POLL_WHILE() should go into a blocking aio_poll() */
ae23c9
-static bool bdrv_drain_poll(BlockDriverState *bs)
ae23c9
+bool bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent)
ae23c9
+{
ae23c9
+    if (bdrv_parent_drained_poll(bs, ignore_parent)) {
ae23c9
+        return true;
ae23c9
+    }
ae23c9
+
ae23c9
+    return atomic_read(&bs->in_flight);
ae23c9
+}
ae23c9
+
ae23c9
+static bool bdrv_drain_poll_top_level(BlockDriverState *bs,
ae23c9
+                                      BdrvChild *ignore_parent)
ae23c9
 {
ae23c9
     /* Execute pending BHs first and check everything else only after the BHs
ae23c9
      * have executed. */
ae23c9
     while (aio_poll(bs->aio_context, false));
ae23c9
-    return atomic_read(&bs->in_flight);
ae23c9
+
ae23c9
+    return bdrv_drain_poll(bs, ignore_parent);
ae23c9
 }
ae23c9
 
ae23c9
-static bool bdrv_drain_recurse(BlockDriverState *bs)
ae23c9
+static bool bdrv_drain_recurse(BlockDriverState *bs, BdrvChild *parent)
ae23c9
 {
ae23c9
     BdrvChild *child, *tmp;
ae23c9
     bool waited;
ae23c9
 
ae23c9
     /* Wait for drained requests to finish */
ae23c9
-    waited = BDRV_POLL_WHILE(bs, bdrv_drain_poll(bs));
ae23c9
+    waited = BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, parent));
ae23c9
 
ae23c9
     QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
ae23c9
         BlockDriverState *bs = child->bs;
ae23c9
@@ -213,7 +241,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)
ae23c9
              */
ae23c9
             bdrv_ref(bs);
ae23c9
         }
ae23c9
-        waited |= bdrv_drain_recurse(bs);
ae23c9
+        waited |= bdrv_drain_recurse(bs, child);
ae23c9
         if (in_main_loop) {
ae23c9
             bdrv_unref(bs);
ae23c9
         }
ae23c9
@@ -289,7 +317,7 @@ void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,
ae23c9
 
ae23c9
     bdrv_parent_drained_begin(bs, parent);
ae23c9
     bdrv_drain_invoke(bs, true);
ae23c9
-    bdrv_drain_recurse(bs);
ae23c9
+    bdrv_drain_recurse(bs, parent);
ae23c9
 
ae23c9
     if (recursive) {
ae23c9
         bs->recursive_quiesce_counter++;
ae23c9
diff --git a/block/mirror.c b/block/mirror.c
ae23c9
index 313e6e9..4b27f71 100644
ae23c9
--- a/block/mirror.c
ae23c9
+++ b/block/mirror.c
ae23c9
@@ -976,6 +976,12 @@ static void mirror_pause(Job *job)
ae23c9
     mirror_wait_for_all_io(s);
ae23c9
 }
ae23c9
 
ae23c9
+static bool mirror_drained_poll(BlockJob *job)
ae23c9
+{
ae23c9
+    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
ae23c9
+    return !!s->in_flight;
ae23c9
+}
ae23c9
+
ae23c9
 static void mirror_attached_aio_context(BlockJob *job, AioContext *new_context)
ae23c9
 {
ae23c9
     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);
ae23c9
@@ -1011,6 +1017,7 @@ static const BlockJobDriver mirror_job_driver = {
ae23c9
         .pause                  = mirror_pause,
ae23c9
         .complete               = mirror_complete,
ae23c9
     },
ae23c9
+    .drained_poll           = mirror_drained_poll,
ae23c9
     .attached_aio_context   = mirror_attached_aio_context,
ae23c9
     .drain                  = mirror_drain,
ae23c9
 };
ae23c9
@@ -1028,6 +1035,7 @@ static const BlockJobDriver commit_active_job_driver = {
ae23c9
         .pause                  = mirror_pause,
ae23c9
         .complete               = mirror_complete,
ae23c9
     },
ae23c9
+    .drained_poll           = mirror_drained_poll,
ae23c9
     .attached_aio_context   = mirror_attached_aio_context,
ae23c9
     .drain                  = mirror_drain,
ae23c9
 };
ae23c9
diff --git a/blockjob.c b/blockjob.c
ae23c9
index 0306533..be5903a 100644
ae23c9
--- a/blockjob.c
ae23c9
+++ b/blockjob.c
ae23c9
@@ -155,6 +155,28 @@ static void child_job_drained_begin(BdrvChild *c)
ae23c9
     job_pause(&job->job);
ae23c9
 }
ae23c9
 
ae23c9
+static bool child_job_drained_poll(BdrvChild *c)
ae23c9
+{
ae23c9
+    BlockJob *bjob = c->opaque;
ae23c9
+    Job *job = &bjob->job;
ae23c9
+    const BlockJobDriver *drv = block_job_driver(bjob);
ae23c9
+
ae23c9
+    /* An inactive or completed job doesn't have any pending requests. Jobs
ae23c9
+     * with !job->busy are either already paused or have a pause point after
ae23c9
+     * being reentered, so no job driver code will run before they pause. */
ae23c9
+    if (!job->busy || job_is_completed(job) || job->deferred_to_main_loop) {
ae23c9
+        return false;
ae23c9
+    }
ae23c9
+
ae23c9
+    /* Otherwise, assume that it isn't fully stopped yet, but allow the job to
ae23c9
+     * override this assumption. */
ae23c9
+    if (drv->drained_poll) {
ae23c9
+        return drv->drained_poll(bjob);
ae23c9
+    } else {
ae23c9
+        return true;
ae23c9
+    }
ae23c9
+}
ae23c9
+
ae23c9
 static void child_job_drained_end(BdrvChild *c)
ae23c9
 {
ae23c9
     BlockJob *job = c->opaque;
ae23c9
@@ -164,6 +186,7 @@ static void child_job_drained_end(BdrvChild *c)
ae23c9
 static const BdrvChildRole child_job = {
ae23c9
     .get_parent_desc    = child_job_get_parent_desc,
ae23c9
     .drained_begin      = child_job_drained_begin,
ae23c9
+    .drained_poll       = child_job_drained_poll,
ae23c9
     .drained_end        = child_job_drained_end,
ae23c9
     .stay_at_node       = true,
ae23c9
 };
ae23c9
diff --git a/include/block/block.h b/include/block/block.h
ae23c9
index 8f87eea..8c91d4c 100644
ae23c9
--- a/include/block/block.h
ae23c9
+++ b/include/block/block.h
ae23c9
@@ -596,6 +596,14 @@ void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore);
ae23c9
 void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore);
ae23c9
 
ae23c9
 /**
ae23c9
+ * bdrv_drain_poll:
ae23c9
+ *
ae23c9
+ * Poll for pending requests in @bs and its parents (except for
ae23c9
+ * @ignore_parent). This is part of bdrv_drained_begin.
ae23c9
+ */
ae23c9
+bool bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent);
ae23c9
+
ae23c9
+/**
ae23c9
  * bdrv_drained_begin:
ae23c9
  *
ae23c9
  * Begin a quiesced section for exclusive access to the BDS, by disabling
ae23c9
diff --git a/include/block/block_int.h b/include/block/block_int.h
ae23c9
index 341cbe8..beeacde 100644
ae23c9
--- a/include/block/block_int.h
ae23c9
+++ b/include/block/block_int.h
ae23c9
@@ -610,6 +610,13 @@ struct BdrvChildRole {
ae23c9
     void (*drained_begin)(BdrvChild *child);
ae23c9
     void (*drained_end)(BdrvChild *child);
ae23c9
 
ae23c9
+    /*
ae23c9
+     * Returns whether the parent has pending requests for the child. This
ae23c9
+     * callback is polled after .drained_begin() has been called until all
ae23c9
+     * activity on the child has stopped.
ae23c9
+     */
ae23c9
+    bool (*drained_poll)(BdrvChild *child);
ae23c9
+
ae23c9
     /* Notifies the parent that the child has been activated/inactivated (e.g.
ae23c9
      * when migration is completing) and it can start/stop requesting
ae23c9
      * permissions and doing I/O on it. */
ae23c9
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
ae23c9
index 5cd50c6..e4a318d 100644
ae23c9
--- a/include/block/blockjob_int.h
ae23c9
+++ b/include/block/blockjob_int.h
ae23c9
@@ -39,6 +39,14 @@ struct BlockJobDriver {
ae23c9
     JobDriver job_driver;
ae23c9
 
ae23c9
     /*
ae23c9
+     * Returns whether the job has pending requests for the child or will
ae23c9
+     * submit new requests before the next pause point. This callback is polled
ae23c9
+     * in the context of draining a job node after requesting that the job be
ae23c9
+     * paused, until all activity on the child has stopped.
ae23c9
+     */
ae23c9
+    bool (*drained_poll)(BlockJob *job);
ae23c9
+
ae23c9
+    /*
ae23c9
      * If the callback is not NULL, it will be invoked before the job is
ae23c9
      * resumed in a new AioContext.  This is the place to move any resources
ae23c9
      * besides job->blk to the new AioContext.
ae23c9
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
ae23c9
index f5d85c9..49786ea 100644
ae23c9
--- a/tests/test-bdrv-drain.c
ae23c9
+++ b/tests/test-bdrv-drain.c
ae23c9
@@ -681,7 +681,11 @@ static int coroutine_fn test_job_run(Job *job, Error **errp)
ae23c9
 
ae23c9
     job_transition_to_ready(&s->common.job);
ae23c9
     while (!s->should_complete) {
ae23c9
-        job_sleep_ns(&s->common.job, 100000);
ae23c9
+        /* Avoid block_job_sleep_ns() because it marks the job as !busy. We
ae23c9
+         * want to emulate some actual activity (probably some I/O) here so
ae23c9
+         * that drain has to wait for this acitivity to stop. */
ae23c9
+        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000);
ae23c9
+        job_pause_point(&s->common.job);
ae23c9
     }
ae23c9
 
ae23c9
     return 0;
ae23c9
@@ -728,7 +732,7 @@ static void test_blockjob_common(enum drain_type drain_type)
ae23c9
 
ae23c9
     g_assert_cmpint(job->job.pause_count, ==, 0);
ae23c9
     g_assert_false(job->job.paused);
ae23c9
-    g_assert_false(job->job.busy); /* We're in job_sleep_ns() */
ae23c9
+    g_assert_true(job->job.busy); /* We're in job_sleep_ns() */
ae23c9
 
ae23c9
     do_drain_begin(drain_type, src);
ae23c9
 
ae23c9
@@ -738,15 +742,14 @@ static void test_blockjob_common(enum drain_type drain_type)
ae23c9
     } else {
ae23c9
         g_assert_cmpint(job->job.pause_count, ==, 1);
ae23c9
     }
ae23c9
-    /* XXX We don't wait until the job is actually paused. Is this okay? */
ae23c9
-    /* g_assert_true(job->job.paused); */
ae23c9
+    g_assert_true(job->job.paused);
ae23c9
     g_assert_false(job->job.busy); /* The job is paused */
ae23c9
 
ae23c9
     do_drain_end(drain_type, src);
ae23c9
 
ae23c9
     g_assert_cmpint(job->job.pause_count, ==, 0);
ae23c9
     g_assert_false(job->job.paused);
ae23c9
-    g_assert_false(job->job.busy); /* We're in job_sleep_ns() */
ae23c9
+    g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */
ae23c9
 
ae23c9
     do_drain_begin(drain_type, target);
ae23c9
 
ae23c9
@@ -756,15 +759,14 @@ static void test_blockjob_common(enum drain_type drain_type)
ae23c9
     } else {
ae23c9
         g_assert_cmpint(job->job.pause_count, ==, 1);
ae23c9
     }
ae23c9
-    /* XXX We don't wait until the job is actually paused. Is this okay? */
ae23c9
-    /* g_assert_true(job->job.paused); */
ae23c9
+    g_assert_true(job->job.paused);
ae23c9
     g_assert_false(job->job.busy); /* The job is paused */
ae23c9
 
ae23c9
     do_drain_end(drain_type, target);
ae23c9
 
ae23c9
     g_assert_cmpint(job->job.pause_count, ==, 0);
ae23c9
     g_assert_false(job->job.paused);
ae23c9
-    g_assert_false(job->job.busy); /* We're in job_sleep_ns() */
ae23c9
+    g_assert_true(job->job.busy); /* We're in job_sleep_ns() */
ae23c9
 
ae23c9
     ret = job_complete_sync(&job->job, &error_abort);
ae23c9
     g_assert_cmpint(ret, ==, 0);
ae23c9
-- 
ae23c9
1.8.3.1
ae23c9