From 6dca7cc6632a4b4632786e8a087289dff1ad6ef8 Mon Sep 17 00:00:00 2001

From: Kevin Wolf <kwolf@redhat.com>

Date: Wed, 10 Oct 2018 20:08:41 +0100

Subject: [PATCH 10/49] block: Really pause block jobs on drain

RH-Author: Kevin Wolf <kwolf@redhat.com>

Message-id: <20181010200843.6710-8-kwolf@redhat.com>

Patchwork-id: 82587

O-Subject: [RHEL-8 qemu-kvm PATCH 07/44] block: Really pause block jobs on drain

Bugzilla: 1637976

RH-Acked-by: Max Reitz <mreitz@redhat.com>

RH-Acked-by: John Snow <jsnow@redhat.com>

RH-Acked-by: Thomas Huth <thuth@redhat.com>

We already requested that block jobs be paused in .bdrv_drained_begin,

but no guarantee was made that the job was actually inactive at the

point where bdrv_drained_begin() returned.

This introduces a new callback BdrvChildRole.bdrv_drained_poll() and

uses it to make bdrv_drain_poll() consider block jobs using the node to

be drained.

For the test case to work as expected, we have to switch from

block_job_sleep_ns() to qemu_co_sleep_ns() so that the test job is even

considered active and must be waited for when draining the node.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

(cherry picked from commit 89bd030533e3592ca0a995450dcfc5d53e459e20)

Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Signed-off-by: Danilo C. L. de Paula <ddepaula@redhat.com>

---

 block.c                      |  9 +++++++++

 block/io.c                   | 40 ++++++++++++++++++++++++++++++++++------

 block/mirror.c               |  8 ++++++++

 blockjob.c                   | 23 +++++++++++++++++++++++

 include/block/block.h        |  8 ++++++++

 include/block/block_int.h    |  7 +++++++

 include/block/blockjob_int.h |  8 ++++++++

 tests/test-bdrv-drain.c      | 18 ++++++++++--------

 8 files changed, 107 insertions(+), 14 deletions(-)

diff --git a/block.c b/block.c

index 10a1ece..0d9698a 100644

--- a/block.c

+++ b/block.c

@@ -821,6 +821,12 @@ static void bdrv_child_cb_drained_begin(BdrvChild *child)

     bdrv_drained_begin(bs);

 }

+static bool bdrv_child_cb_drained_poll(BdrvChild *child)

+{

+    BlockDriverState *bs = child->opaque;

+    return bdrv_drain_poll(bs, NULL);

+}

+

 static void bdrv_child_cb_drained_end(BdrvChild *child)

 {

     BlockDriverState *bs = child->opaque;

@@ -905,6 +911,7 @@ const BdrvChildRole child_file = {

     .get_parent_desc = bdrv_child_get_parent_desc,

     .inherit_options = bdrv_inherited_options,

     .drained_begin   = bdrv_child_cb_drained_begin,

+    .drained_poll    = bdrv_child_cb_drained_poll,

     .drained_end     = bdrv_child_cb_drained_end,

     .attach          = bdrv_child_cb_attach,

     .detach          = bdrv_child_cb_detach,

@@ -929,6 +936,7 @@ const BdrvChildRole child_format = {

     .get_parent_desc = bdrv_child_get_parent_desc,

     .inherit_options = bdrv_inherited_fmt_options,

     .drained_begin   = bdrv_child_cb_drained_begin,

+    .drained_poll    = bdrv_child_cb_drained_poll,

     .drained_end     = bdrv_child_cb_drained_end,

     .attach          = bdrv_child_cb_attach,

     .detach          = bdrv_child_cb_detach,

@@ -1048,6 +1056,7 @@ const BdrvChildRole child_backing = {

     .detach          = bdrv_backing_detach,

     .inherit_options = bdrv_backing_options,

     .drained_begin   = bdrv_child_cb_drained_begin,

+    .drained_poll    = bdrv_child_cb_drained_poll,

     .drained_end     = bdrv_child_cb_drained_end,

     .inactivate      = bdrv_child_cb_inactivate,

     .update_filename = bdrv_backing_update_filename,

diff --git a/block/io.c b/block/io.c

index 4d332c3..e260394 100644

--- a/block/io.c

+++ b/block/io.c

@@ -69,6 +69,23 @@ void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore)

     }

 }

+static bool bdrv_parent_drained_poll(BlockDriverState *bs, BdrvChild *ignore)

+{

+    BdrvChild *c, *next;

+    bool busy = false;

+

+    QLIST_FOREACH_SAFE(c, &bs->parents, next_parent, next) {

+        if (c == ignore) {

+            continue;

+        }

+        if (c->role->drained_poll) {

+            busy |= c->role->drained_poll(c);

+        }

+    }

+

+    return busy;

+}

+

 static void bdrv_merge_limits(BlockLimits *dst, const BlockLimits *src)

 {

     dst->opt_transfer = MAX(dst->opt_transfer, src->opt_transfer);

@@ -182,21 +199,32 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)

 }

 /* Returns true if BDRV_POLL_WHILE() should go into a blocking aio_poll() */

-static bool bdrv_drain_poll(BlockDriverState *bs)

+bool bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent)

+{

+    if (bdrv_parent_drained_poll(bs, ignore_parent)) {

+        return true;

+    }

+

+    return atomic_read(&bs->in_flight);

+}

+

+static bool bdrv_drain_poll_top_level(BlockDriverState *bs,

+                                      BdrvChild *ignore_parent)

 {

     /* Execute pending BHs first and check everything else only after the BHs

      * have executed. */

     while (aio_poll(bs->aio_context, false));

-    return atomic_read(&bs->in_flight);

+

+    return bdrv_drain_poll(bs, ignore_parent);

 }

-static bool bdrv_drain_recurse(BlockDriverState *bs)

+static bool bdrv_drain_recurse(BlockDriverState *bs, BdrvChild *parent)

 {

     BdrvChild *child, *tmp;

     bool waited;

     /* Wait for drained requests to finish */

-    waited = BDRV_POLL_WHILE(bs, bdrv_drain_poll(bs));

+    waited = BDRV_POLL_WHILE(bs, bdrv_drain_poll_top_level(bs, parent));

     QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {

         BlockDriverState *bs = child->bs;

@@ -213,7 +241,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)

              */

             bdrv_ref(bs);

         }

-        waited |= bdrv_drain_recurse(bs);

+        waited |= bdrv_drain_recurse(bs, child);

         if (in_main_loop) {

             bdrv_unref(bs);

         }

@@ -289,7 +317,7 @@ void bdrv_do_drained_begin(BlockDriverState *bs, bool recursive,

     bdrv_parent_drained_begin(bs, parent);

     bdrv_drain_invoke(bs, true);

-    bdrv_drain_recurse(bs);

+    bdrv_drain_recurse(bs, parent);

     if (recursive) {

         bs->recursive_quiesce_counter++;

diff --git a/block/mirror.c b/block/mirror.c

index 313e6e9..4b27f71 100644

--- a/block/mirror.c

+++ b/block/mirror.c

@@ -976,6 +976,12 @@ static void mirror_pause(Job *job)

     mirror_wait_for_all_io(s);

 }

+static bool mirror_drained_poll(BlockJob *job)

+{

+    MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);

+    return !!s->in_flight;

+}

+

 static void mirror_attached_aio_context(BlockJob *job, AioContext *new_context)

 {

     MirrorBlockJob *s = container_of(job, MirrorBlockJob, common);

@@ -1011,6 +1017,7 @@ static const BlockJobDriver mirror_job_driver = {

         .pause                  = mirror_pause,

         .complete               = mirror_complete,

     },

+    .drained_poll           = mirror_drained_poll,

     .attached_aio_context   = mirror_attached_aio_context,

     .drain                  = mirror_drain,

 };

@@ -1028,6 +1035,7 @@ static const BlockJobDriver commit_active_job_driver = {

         .pause                  = mirror_pause,

         .complete               = mirror_complete,

     },

+    .drained_poll           = mirror_drained_poll,

     .attached_aio_context   = mirror_attached_aio_context,

     .drain                  = mirror_drain,

 };

diff --git a/blockjob.c b/blockjob.c

index 0306533..be5903a 100644

--- a/blockjob.c

+++ b/blockjob.c

@@ -155,6 +155,28 @@ static void child_job_drained_begin(BdrvChild *c)

     job_pause(&job->job);

 }

+static bool child_job_drained_poll(BdrvChild *c)

+{

+    BlockJob *bjob = c->opaque;

+    Job *job = &bjob->job;

+    const BlockJobDriver *drv = block_job_driver(bjob);

+

+    /* An inactive or completed job doesn't have any pending requests. Jobs

+     * with !job->busy are either already paused or have a pause point after

+     * being reentered, so no job driver code will run before they pause. */

+    if (!job->busy || job_is_completed(job) || job->deferred_to_main_loop) {

+        return false;

+    }

+

+    /* Otherwise, assume that it isn't fully stopped yet, but allow the job to

+     * override this assumption. */

+    if (drv->drained_poll) {

+        return drv->drained_poll(bjob);

+    } else {

+        return true;

+    }

+}

+

 static void child_job_drained_end(BdrvChild *c)

 {

     BlockJob *job = c->opaque;

@@ -164,6 +186,7 @@ static void child_job_drained_end(BdrvChild *c)

 static const BdrvChildRole child_job = {

     .get_parent_desc    = child_job_get_parent_desc,

     .drained_begin      = child_job_drained_begin,

+    .drained_poll       = child_job_drained_poll,

     .drained_end        = child_job_drained_end,

     .stay_at_node       = true,

 };

diff --git a/include/block/block.h b/include/block/block.h

index 8f87eea..8c91d4c 100644

--- a/include/block/block.h

+++ b/include/block/block.h

@@ -596,6 +596,14 @@ void bdrv_parent_drained_begin(BlockDriverState *bs, BdrvChild *ignore);

 void bdrv_parent_drained_end(BlockDriverState *bs, BdrvChild *ignore);

 /**

+ * bdrv_drain_poll:

+ *

+ * Poll for pending requests in @bs and its parents (except for

+ * @ignore_parent). This is part of bdrv_drained_begin.

+ */

+bool bdrv_drain_poll(BlockDriverState *bs, BdrvChild *ignore_parent);

+

+/**

  * bdrv_drained_begin:

  *

  * Begin a quiesced section for exclusive access to the BDS, by disabling

diff --git a/include/block/block_int.h b/include/block/block_int.h

index 341cbe8..beeacde 100644

--- a/include/block/block_int.h

+++ b/include/block/block_int.h

@@ -610,6 +610,13 @@ struct BdrvChildRole {

     void (*drained_begin)(BdrvChild *child);

     void (*drained_end)(BdrvChild *child);

+    /*

+     * Returns whether the parent has pending requests for the child. This

+     * callback is polled after .drained_begin() has been called until all

+     * activity on the child has stopped.

+     */

+    bool (*drained_poll)(BdrvChild *child);

+

     /* Notifies the parent that the child has been activated/inactivated (e.g.

      * when migration is completing) and it can start/stop requesting

      * permissions and doing I/O on it. */

diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h

index 5cd50c6..e4a318d 100644

--- a/include/block/blockjob_int.h

+++ b/include/block/blockjob_int.h

@@ -39,6 +39,14 @@ struct BlockJobDriver {

     JobDriver job_driver;

     /*

+     * Returns whether the job has pending requests for the child or will

+     * submit new requests before the next pause point. This callback is polled

+     * in the context of draining a job node after requesting that the job be

+     * paused, until all activity on the child has stopped.

+     */

+    bool (*drained_poll)(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.

diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c

index f5d85c9..49786ea 100644

--- a/tests/test-bdrv-drain.c

+++ b/tests/test-bdrv-drain.c

@@ -681,7 +681,11 @@ static int coroutine_fn test_job_run(Job *job, Error **errp)

     job_transition_to_ready(&s->common.job);

     while (!s->should_complete) {

-        job_sleep_ns(&s->common.job, 100000);

+        /* Avoid block_job_sleep_ns() because it marks the job as !busy. We

+         * want to emulate some actual activity (probably some I/O) here so

+         * that drain has to wait for this acitivity to stop. */

+        qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 100000);

+        job_pause_point(&s->common.job);

     }

     return 0;

@@ -728,7 +732,7 @@ static void test_blockjob_common(enum drain_type drain_type)

     g_assert_cmpint(job->job.pause_count, ==, 0);

     g_assert_false(job->job.paused);

-    g_assert_false(job->job.busy); /* We're in job_sleep_ns() */

+    g_assert_true(job->job.busy); /* We're in job_sleep_ns() */

     do_drain_begin(drain_type, src);

@@ -738,15 +742,14 @@ static void test_blockjob_common(enum drain_type drain_type)

     } else {

         g_assert_cmpint(job->job.pause_count, ==, 1);

     }

-    /* XXX We don't wait until the job is actually paused. Is this okay? */

-    /* g_assert_true(job->job.paused); */

+    g_assert_true(job->job.paused);

     g_assert_false(job->job.busy); /* The job is paused */

     do_drain_end(drain_type, src);

     g_assert_cmpint(job->job.pause_count, ==, 0);

     g_assert_false(job->job.paused);

-    g_assert_false(job->job.busy); /* We're in job_sleep_ns() */

+    g_assert_true(job->job.busy); /* We're in qemu_co_sleep_ns() */

     do_drain_begin(drain_type, target);

@@ -756,15 +759,14 @@ static void test_blockjob_common(enum drain_type drain_type)

     } else {

         g_assert_cmpint(job->job.pause_count, ==, 1);

     }

-    /* XXX We don't wait until the job is actually paused. Is this okay? */

-    /* g_assert_true(job->job.paused); */

+    g_assert_true(job->job.paused);

     g_assert_false(job->job.busy); /* The job is paused */

     do_drain_end(drain_type, target);

     g_assert_cmpint(job->job.pause_count, ==, 0);

     g_assert_false(job->job.paused);

-    g_assert_false(job->job.busy); /* We're in job_sleep_ns() */

+    g_assert_true(job->job.busy); /* We're in job_sleep_ns() */

     ret = job_complete_sync(&job->job, &error_abort);

     g_assert_cmpint(ret, ==, 0);

-- 

1.8.3.1