[Qemu-devel] [PATCH v2 0/3] add bdrv_co_drain_begin/end BlockDriver callbacks

[Qemu-devel] [PATCH v2 0/3] add bdrv_co_drain_begin/end BlockDriver callbacks

[Manos Pitsidianakis]

This patch series renames bdrv_co_drain to bdrv_co_drain_begin and adds a new 
bdrv_co_drain_end callback to match bdrv_drained_begin/end and 
drained_begin/end of BdrvChild. This is needed because the throttle driver 
(block/throttle.c) needs a way to mark the end of the drain in order to toggle 
io_limits_disabled correctly.

Based-on: <20170918202529.28379-1-el13635@mail.ntua.gr>
    "block/throttle-groups.c: allocate RestartData on the heap"
    Which fixes a coroutine crash in block/throttle-groups.c

v2: 
  add doc for callbacks and change order of request polling for completion 
  [Stefan]

Manos Pitsidianakis (3):
  block: add bdrv_co_drain_end callback
  block: rename bdrv_co_drain to bdrv_co_drain_begin
  block/throttle.c: add bdrv_co_drain_begin/end callbacks

 include/block/block_int.h |  8 +++++++-
 block/io.c                | 48 +++++++++++++++++++++++++++++++++--------------
 block/qed.c               |  6 +++---
 block/throttle.c          | 18 ++++++++++++++++++
 4 files changed, 62 insertions(+), 18 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end callback

[Manos Pitsidianakis]

BlockDriverState has a bdrv_do_drain() callback but no equivalent for the end
of the drain. The throttle driver (block/throttle.c) needs a way to mark the
end of the drain in order to toggle io_limits_disabled correctly, thus
bdrv_co_drain_end is needed.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 include/block/block_int.h |  6 ++++++
 block/io.c                | 48 +++++++++++++++++++++++++++++++++--------------
 2 files changed, 40 insertions(+), 14 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index ba4c383393..21950cfda3 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -356,8 +356,14 @@ struct BlockDriver {
     /**
      * Drain and stop any internal sources of requests in the driver, and
      * remain so until next I/O callback (e.g. bdrv_co_writev) is called.
+     *
+     * The callbacks are called at the beginning and ending of the drain
+     * respectively. They should be implemented if the driver needs to e.g.
+     * manage scheduled I/O requests, or toggle an internal state. After an
+     * bdrv_co_drain_end() invocation new requests will continue normally.
      */
     void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
+    void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
 
     void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
                            Error **errp);
diff --git a/block/io.c b/block/io.c
index 4378ae4c7d..b0a10ad3ef 100644
--- a/block/io.c
+++ b/block/io.c
@@ -153,6 +153,7 @@ typedef struct {
     Coroutine *co;
     BlockDriverState *bs;
     bool done;
+    bool begin;
 } BdrvCoDrainData;
 
 static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
@@ -160,18 +161,23 @@ static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
     BdrvCoDrainData *data = opaque;
     BlockDriverState *bs = data->bs;
 
-    bs->drv->bdrv_co_drain(bs);
+    if (data->begin) {
+        bs->drv->bdrv_co_drain(bs);
+    } else {
+        bs->drv->bdrv_co_drain_end(bs);
+    }
 
     /* Set data->done before reading bs->wakeup.  */
     atomic_mb_set(&data->done, true);
     bdrv_wakeup(bs);
 }
 
-static void bdrv_drain_invoke(BlockDriverState *bs)
+static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
 {
-    BdrvCoDrainData data = { .bs = bs, .done = false };
+    BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
 
-    if (!bs->drv || !bs->drv->bdrv_co_drain) {
+    if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
+            (!begin && !bs->drv->bdrv_co_drain_end)) {
         return;
     }
 
@@ -180,15 +186,16 @@ static void bdrv_drain_invoke(BlockDriverState *bs)
     BDRV_POLL_WHILE(bs, !data.done);
 }
 
-static bool bdrv_drain_recurse(BlockDriverState *bs)
+static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin)
 {
     BdrvChild *child, *tmp;
     bool waited;
 
-    waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);
-
     /* Ensure any pending metadata writes are submitted to bs->file.  */
-    bdrv_drain_invoke(bs);
+    bdrv_drain_invoke(bs, begin);
+
+    /* Wait for drained requests to finish */
+    waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);
 
     QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
         BlockDriverState *bs = child->bs;
@@ -205,7 +212,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)
              */
             bdrv_ref(bs);
         }
-        waited |= bdrv_drain_recurse(bs);
+        waited |= bdrv_drain_recurse(bs, begin);
         if (in_main_loop) {
             bdrv_unref(bs);
         }
@@ -221,12 +228,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)
     BlockDriverState *bs = data->bs;
 
     bdrv_dec_in_flight(bs);
-    bdrv_drained_begin(bs);
+    if (data->begin) {
+        bdrv_drained_begin(bs);
+    } else {
+        bdrv_drained_end(bs);
+    }
+
     data->done = true;
     aio_co_wake(co);
 }
 
-static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
+static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
+                                                bool begin)
 {
     BdrvCoDrainData data;
 
@@ -239,6 +252,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
         .co = qemu_coroutine_self(),
         .bs = bs,
         .done = false,
+        .begin = begin,
     };
     bdrv_inc_in_flight(bs);
     aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),
@@ -253,7 +267,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
 void bdrv_drained_begin(BlockDriverState *bs)
 {
     if (qemu_in_coroutine()) {
-        bdrv_co_yield_to_drain(bs);
+        bdrv_co_yield_to_drain(bs, true);
         return;
     }
 
@@ -262,17 +276,22 @@ void bdrv_drained_begin(BlockDriverState *bs)
         bdrv_parent_drained_begin(bs);
     }
 
-    bdrv_drain_recurse(bs);
+    bdrv_drain_recurse(bs, true);
 }
 
 void bdrv_drained_end(BlockDriverState *bs)
 {
+    if (qemu_in_coroutine()) {
+        bdrv_co_yield_to_drain(bs, false);
+        return;
+    }
     assert(bs->quiesce_counter > 0);
     if (atomic_fetch_dec(&bs->quiesce_counter) > 1) {
         return;
     }
 
     bdrv_parent_drained_end(bs);
+    bdrv_drain_recurse(bs, false);
     aio_enable_external(bdrv_get_aio_context(bs));
 }
 
@@ -350,7 +369,7 @@ void bdrv_drain_all_begin(void)
             aio_context_acquire(aio_context);
             for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
                 if (aio_context == bdrv_get_aio_context(bs)) {
-                    waited |= bdrv_drain_recurse(bs);
+                    waited |= bdrv_drain_recurse(bs, true);
                 }
             }
             aio_context_release(aio_context);
@@ -371,6 +390,7 @@ void bdrv_drain_all_end(void)
         aio_context_acquire(aio_context);
         aio_enable_external(aio_context);
         bdrv_parent_drained_end(bs);
+        bdrv_drain_recurse(bs, false);
         aio_context_release(aio_context);
     }
 
-- 
2.11.0

[Qemu-devel] [PATCH v2 2/3] block: rename bdrv_co_drain to bdrv_co_drain_begin

[Manos Pitsidianakis]

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 include/block/block_int.h | 2 +-
 block/io.c                | 4 ++--
 block/qed.c               | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/block/block_int.h b/include/block/block_int.h
index 21950cfda3..205d088271 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -362,7 +362,7 @@ struct BlockDriver {
      * manage scheduled I/O requests, or toggle an internal state. After an
      * bdrv_co_drain_end() invocation new requests will continue normally.
      */
-    void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
+    void coroutine_fn (*bdrv_co_drain_begin)(BlockDriverState *bs);
     void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
 
     void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
diff --git a/block/io.c b/block/io.c
index b0a10ad3ef..65e8094d7c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -162,7 +162,7 @@ static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
     BlockDriverState *bs = data->bs;
 
     if (data->begin) {
-        bs->drv->bdrv_co_drain(bs);
+        bs->drv->bdrv_co_drain_begin(bs);
     } else {
         bs->drv->bdrv_co_drain_end(bs);
     }
@@ -176,7 +176,7 @@ static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
 {
     BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
 
-    if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
+    if (!bs->drv || (begin && !bs->drv->bdrv_co_drain_begin) ||
             (!begin && !bs->drv->bdrv_co_drain_end)) {
         return;
     }
diff --git a/block/qed.c b/block/qed.c
index 28e2ec89e8..821dcaa055 100644
--- a/block/qed.c
+++ b/block/qed.c
@@ -265,7 +265,7 @@ static bool qed_plug_allocating_write_reqs(BDRVQEDState *s)
     assert(!s->allocating_write_reqs_plugged);
     if (s->allocating_acb != NULL) {
         /* Another allocating write came concurrently.  This cannot happen
-         * from bdrv_qed_co_drain, but it can happen when the timer runs.
+         * from bdrv_qed_co_drain_begin, but it can happen when the timer runs.
          */
         qemu_co_mutex_unlock(&s->table_lock);
         return false;
@@ -358,7 +358,7 @@ static void bdrv_qed_attach_aio_context(BlockDriverState *bs,
     }
 }
 
-static void coroutine_fn bdrv_qed_co_drain(BlockDriverState *bs)
+static void coroutine_fn bdrv_qed_co_drain_begin(BlockDriverState *bs)
 {
     BDRVQEDState *s = bs->opaque;
 
@@ -1608,7 +1608,7 @@ static BlockDriver bdrv_qed = {
     .bdrv_check               = bdrv_qed_check,
     .bdrv_detach_aio_context  = bdrv_qed_detach_aio_context,
     .bdrv_attach_aio_context  = bdrv_qed_attach_aio_context,
-    .bdrv_co_drain            = bdrv_qed_co_drain,
+    .bdrv_co_drain_begin      = bdrv_qed_co_drain_begin,
 };
 
 static void bdrv_qed_init(void)
-- 
2.11.0

[Qemu-devel] [PATCH v2 3/3] block/throttle.c: add bdrv_co_drain_begin/end callbacks

[Manos Pitsidianakis]

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block/throttle.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/block/throttle.c b/block/throttle.c
index 5bca76300f..833175ac77 100644
--- a/block/throttle.c
+++ b/block/throttle.c
@@ -197,6 +197,21 @@ static bool throttle_recurse_is_first_non_filter(BlockDriverState *bs,
     return bdrv_recurse_is_first_non_filter(bs->file->bs, candidate);
 }
 
+static void coroutine_fn throttle_co_drain_begin(BlockDriverState *bs)
+{
+    ThrottleGroupMember *tgm = bs->opaque;
+    if (atomic_fetch_inc(&tgm->io_limits_disabled) == 0) {
+        throttle_group_restart_tgm(tgm);
+    }
+}
+
+static void coroutine_fn throttle_co_drain_end(BlockDriverState *bs)
+{
+    ThrottleGroupMember *tgm = bs->opaque;
+    assert(tgm->io_limits_disabled);
+    atomic_dec(&tgm->io_limits_disabled);
+}
+
 static BlockDriver bdrv_throttle = {
     .format_name                        =   "throttle",
     .protocol_name                      =   "throttle",
@@ -226,6 +241,9 @@ static BlockDriver bdrv_throttle = {
     .bdrv_reopen_abort                  =   throttle_reopen_abort,
     .bdrv_co_get_block_status           =   bdrv_co_get_block_status_from_file,
 
+    .bdrv_co_drain_begin                =   throttle_co_drain_begin,
+    .bdrv_co_drain_end                  =   throttle_co_drain_end,
+
     .is_filter                          =   true,
 };
 
-- 
2.11.0

Re: [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end callback

[Fam Zheng]

On Thu, 09/21 16:17, Manos Pitsidianakis wrote:
> BlockDriverState has a bdrv_do_drain() callback but no equivalent for the end

s/bdrv_do_drain/bdrv_co_drain/

> of the drain. The throttle driver (block/throttle.c) needs a way to mark the
> end of the drain in order to toggle io_limits_disabled correctly, thus
> bdrv_co_drain_end is needed.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  include/block/block_int.h |  6 ++++++
>  block/io.c                | 48 +++++++++++++++++++++++++++++++++--------------
>  2 files changed, 40 insertions(+), 14 deletions(-)
> 
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index ba4c383393..21950cfda3 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -356,8 +356,14 @@ struct BlockDriver {
>      /**
>       * Drain and stop any internal sources of requests in the driver, and
>       * remain so until next I/O callback (e.g. bdrv_co_writev) is called.

This line needs update too, maybe:

       /**
        * bdrv_co_drain drains and stops any ... and remain so until
        * bdrv_co_drain_end is called.

> +     *
> +     * The callbacks are called at the beginning and ending of the drain
> +     * respectively. They should be implemented if the driver needs to e.g.

As implied by above change, should we explicitly require "should both be
implemented"? It may not be technically required, but I think is cleaner and
easier to reason about.

> +     * manage scheduled I/O requests, or toggle an internal state. After an

s/After an/After a/

> +     * bdrv_co_drain_end() invocation new requests will continue normally.
>       */
>      void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
> +    void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
>  
>      void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
>                             Error **errp);
> diff --git a/block/io.c b/block/io.c
> index 4378ae4c7d..b0a10ad3ef 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -153,6 +153,7 @@ typedef struct {
>      Coroutine *co;
>      BlockDriverState *bs;
>      bool done;
> +    bool begin;
>  } BdrvCoDrainData;
>  
>  static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
> @@ -160,18 +161,23 @@ static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
>      BdrvCoDrainData *data = opaque;
>      BlockDriverState *bs = data->bs;
>  
> -    bs->drv->bdrv_co_drain(bs);
> +    if (data->begin) {
> +        bs->drv->bdrv_co_drain(bs);
> +    } else {
> +        bs->drv->bdrv_co_drain_end(bs);
> +    }
>  
>      /* Set data->done before reading bs->wakeup.  */
>      atomic_mb_set(&data->done, true);
>      bdrv_wakeup(bs);
>  }
>  
> -static void bdrv_drain_invoke(BlockDriverState *bs)
> +static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
>  {
> -    BdrvCoDrainData data = { .bs = bs, .done = false };
> +    BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
>  
> -    if (!bs->drv || !bs->drv->bdrv_co_drain) {
> +    if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
> +            (!begin && !bs->drv->bdrv_co_drain_end)) {
>          return;
>      }
>  
> @@ -180,15 +186,16 @@ static void bdrv_drain_invoke(BlockDriverState *bs)
>      BDRV_POLL_WHILE(bs, !data.done);
>  }
>  
> -static bool bdrv_drain_recurse(BlockDriverState *bs)
> +static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin)
>  {
>      BdrvChild *child, *tmp;
>      bool waited;
>  
> -    waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);
> -
>      /* Ensure any pending metadata writes are submitted to bs->file.  */
> -    bdrv_drain_invoke(bs);
> +    bdrv_drain_invoke(bs, begin);
> +
> +    /* Wait for drained requests to finish */
> +    waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);
>  
>      QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
>          BlockDriverState *bs = child->bs;
> @@ -205,7 +212,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)
>               */
>              bdrv_ref(bs);
>          }
> -        waited |= bdrv_drain_recurse(bs);
> +        waited |= bdrv_drain_recurse(bs, begin);
>          if (in_main_loop) {
>              bdrv_unref(bs);
>          }
> @@ -221,12 +228,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)
>      BlockDriverState *bs = data->bs;
>  
>      bdrv_dec_in_flight(bs);
> -    bdrv_drained_begin(bs);
> +    if (data->begin) {
> +        bdrv_drained_begin(bs);
> +    } else {
> +        bdrv_drained_end(bs);
> +    }
> +
>      data->done = true;
>      aio_co_wake(co);
>  }
>  
> -static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
> +static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
> +                                                bool begin)
>  {
>      BdrvCoDrainData data;
>  
> @@ -239,6 +252,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
>          .co = qemu_coroutine_self(),
>          .bs = bs,
>          .done = false,
> +        .begin = begin,
>      };
>      bdrv_inc_in_flight(bs);
>      aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),
> @@ -253,7 +267,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
>  void bdrv_drained_begin(BlockDriverState *bs)
>  {
>      if (qemu_in_coroutine()) {
> -        bdrv_co_yield_to_drain(bs);
> +        bdrv_co_yield_to_drain(bs, true);
>          return;
>      }
>  
> @@ -262,17 +276,22 @@ void bdrv_drained_begin(BlockDriverState *bs)
>          bdrv_parent_drained_begin(bs);
>      }
>  
> -    bdrv_drain_recurse(bs);
> +    bdrv_drain_recurse(bs, true);
>  }
>  
>  void bdrv_drained_end(BlockDriverState *bs)
>  {
> +    if (qemu_in_coroutine()) {
> +        bdrv_co_yield_to_drain(bs, false);
> +        return;
> +    }
>      assert(bs->quiesce_counter > 0);
>      if (atomic_fetch_dec(&bs->quiesce_counter) > 1) {
>          return;
>      }
>  
>      bdrv_parent_drained_end(bs);
> +    bdrv_drain_recurse(bs, false);
>      aio_enable_external(bdrv_get_aio_context(bs));
>  }
>  
> @@ -350,7 +369,7 @@ void bdrv_drain_all_begin(void)
>              aio_context_acquire(aio_context);
>              for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>                  if (aio_context == bdrv_get_aio_context(bs)) {
> -                    waited |= bdrv_drain_recurse(bs);
> +                    waited |= bdrv_drain_recurse(bs, true);
>                  }
>              }
>              aio_context_release(aio_context);
> @@ -371,6 +390,7 @@ void bdrv_drain_all_end(void)
>          aio_context_acquire(aio_context);
>          aio_enable_external(aio_context);
>          bdrv_parent_drained_end(bs);
> +        bdrv_drain_recurse(bs, false);
>          aio_context_release(aio_context);
>      }
>  
> -- 
> 2.11.0
> 

Re: [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end callback

[Fam Zheng]

On Thu, 09/21 16:17, Manos Pitsidianakis wrote:
> BlockDriverState has a bdrv_do_drain() callback but no equivalent for the end
> of the drain. The throttle driver (block/throttle.c) needs a way to mark the
> end of the drain in order to toggle io_limits_disabled correctly, thus
> bdrv_co_drain_end is needed.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v2 2/3] block: rename bdrv_co_drain to bdrv_co_drain_begin

[Fam Zheng]

On Thu, 09/21 16:17, Manos Pitsidianakis wrote:
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v2 3/3] block/throttle.c: add bdrv_co_drain_begin/end callbacks

[Fam Zheng]

On Thu, 09/21 16:17, Manos Pitsidianakis wrote:
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>

Reviewed-by: Fam Zheng <famz@redhat.com>

Re: [Qemu-devel] [PATCH v2 0/3] add bdrv_co_drain_begin/end BlockDriver callbacks

[Fam Zheng]

On Thu, 09/21 16:17, Manos Pitsidianakis wrote:
> This patch series renames bdrv_co_drain to bdrv_co_drain_begin and adds a new 
> bdrv_co_drain_end callback to match bdrv_drained_begin/end and 
> drained_begin/end of BdrvChild. This is needed because the throttle driver 
> (block/throttle.c) needs a way to mark the end of the drain in order to toggle 
> io_limits_disabled correctly.

Is this a bug fix? I.e. do we need to Cc qemu-stable@?

Fam

Re: [Qemu-devel] [PATCH v2 0/3] add bdrv_co_drain_begin/end BlockDriver callbacks

[Manos Pitsidianakis]

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

On Thu, Sep 21, 2017 at 09:35:35PM +0800, Fam Zheng wrote:
>On Thu, 09/21 16:17, Manos Pitsidianakis wrote:
>> This patch series renames bdrv_co_drain to bdrv_co_drain_begin and adds a new
>> bdrv_co_drain_end callback to match bdrv_drained_begin/end and
>> drained_begin/end of BdrvChild. This is needed because the throttle driver
>> (block/throttle.c) needs a way to mark the end of the drain in order to toggle
>> io_limits_disabled correctly.
>
>Is this a bug fix? I.e. do we need to Cc qemu-stable@?
>
>Fam

No that's not needed, throttle is not in 2.10. The bug in this case is 
that the drain would wait for the throttled requests to be completed as 
they were scheduled by the I/O throttling instead of restarting the 
queue and scheduling them right away.

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

Re: [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end callback

[Manos Pitsidianakis]

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

On Thu, Sep 21, 2017 at 09:29:43PM +0800, Fam Zheng wrote:
>On Thu, 09/21 16:17, Manos Pitsidianakis wrote:
>> BlockDriverState has a bdrv_do_drain() callback but no equivalent for the end
>
>s/bdrv_do_drain/bdrv_co_drain/
>
>> of the drain. The throttle driver (block/throttle.c) needs a way to mark the
>> end of the drain in order to toggle io_limits_disabled correctly, thus
>> bdrv_co_drain_end is needed.
>>
>> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
>> ---
>>  include/block/block_int.h |  6 ++++++
>>  block/io.c                | 48 +++++++++++++++++++++++++++++++++--------------
>>  2 files changed, 40 insertions(+), 14 deletions(-)
>>
>> diff --git a/include/block/block_int.h b/include/block/block_int.h
>> index ba4c383393..21950cfda3 100644
>> --- a/include/block/block_int.h
>> +++ b/include/block/block_int.h
>> @@ -356,8 +356,14 @@ struct BlockDriver {
>>      /**
>>       * Drain and stop any internal sources of requests in the driver, and
>>       * remain so until next I/O callback (e.g. bdrv_co_writev) is called.
>
>This line needs update too, maybe:
>
>       /**
>        * bdrv_co_drain drains and stops any ... and remain so until
>        * bdrv_co_drain_end is called.
>
>> +     *
>> +     * The callbacks are called at the beginning and ending of the drain
>> +     * respectively. They should be implemented if the driver needs to e.g.
>
>As implied by above change, should we explicitly require "should both be
>implemented"? It may not be technically required, but I think is cleaner and
>easier to reason about.
>

It might imply to someone that there's an 
assert(drv->bdrv_co_drain_begin && drv->bdrv_co_drain_end) somewhere 
unless you state they don't have to be implemented at the same time. How 
about we be completely explicit:

  bdrv_co_drain_begin is called if implemented in the beggining of a
  drain operation to drain and stop any internal sources of requests in
  the driver.
  bdrv_co_drain_end is called if implemented at the end of the drain.

  They should be used by the driver to e.g. manage scheduled I/O
  requests, or toggle an internal state. After the end of the drain new
  requests will continue normally.

I hope this is easier for a reader to understand!

>> +     * manage scheduled I/O requests, or toggle an internal state. 
>> After an
>
>s/After an/After a/
>
>> +     * bdrv_co_drain_end() invocation new requests will continue normally.
>>       */
>>      void coroutine_fn (*bdrv_co_drain)(BlockDriverState *bs);
>> +    void coroutine_fn (*bdrv_co_drain_end)(BlockDriverState *bs);
>>
>>      void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child,
>>                             Error **errp);
>> diff --git a/block/io.c b/block/io.c
>> index 4378ae4c7d..b0a10ad3ef 100644
>> --- a/block/io.c
>> +++ b/block/io.c
>> @@ -153,6 +153,7 @@ typedef struct {
>>      Coroutine *co;
>>      BlockDriverState *bs;
>>      bool done;
>> +    bool begin;
>>  } BdrvCoDrainData;
>>
>>  static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
>> @@ -160,18 +161,23 @@ static void coroutine_fn bdrv_drain_invoke_entry(void *opaque)
>>      BdrvCoDrainData *data = opaque;
>>      BlockDriverState *bs = data->bs;
>>
>> -    bs->drv->bdrv_co_drain(bs);
>> +    if (data->begin) {
>> +        bs->drv->bdrv_co_drain(bs);
>> +    } else {
>> +        bs->drv->bdrv_co_drain_end(bs);
>> +    }
>>
>>      /* Set data->done before reading bs->wakeup.  */
>>      atomic_mb_set(&data->done, true);
>>      bdrv_wakeup(bs);
>>  }
>>
>> -static void bdrv_drain_invoke(BlockDriverState *bs)
>> +static void bdrv_drain_invoke(BlockDriverState *bs, bool begin)
>>  {
>> -    BdrvCoDrainData data = { .bs = bs, .done = false };
>> +    BdrvCoDrainData data = { .bs = bs, .done = false, .begin = begin};
>>
>> -    if (!bs->drv || !bs->drv->bdrv_co_drain) {
>> +    if (!bs->drv || (begin && !bs->drv->bdrv_co_drain) ||
>> +            (!begin && !bs->drv->bdrv_co_drain_end)) {
>>          return;
>>      }
>>
>> @@ -180,15 +186,16 @@ static void bdrv_drain_invoke(BlockDriverState *bs)
>>      BDRV_POLL_WHILE(bs, !data.done);
>>  }
>>
>> -static bool bdrv_drain_recurse(BlockDriverState *bs)
>> +static bool bdrv_drain_recurse(BlockDriverState *bs, bool begin)
>>  {
>>      BdrvChild *child, *tmp;
>>      bool waited;
>>
>> -    waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);
>> -
>>      /* Ensure any pending metadata writes are submitted to bs->file.  */
>> -    bdrv_drain_invoke(bs);
>> +    bdrv_drain_invoke(bs, begin);
>> +
>> +    /* Wait for drained requests to finish */
>> +    waited = BDRV_POLL_WHILE(bs, atomic_read(&bs->in_flight) > 0);
>>
>>      QLIST_FOREACH_SAFE(child, &bs->children, next, tmp) {
>>          BlockDriverState *bs = child->bs;
>> @@ -205,7 +212,7 @@ static bool bdrv_drain_recurse(BlockDriverState *bs)
>>               */
>>              bdrv_ref(bs);
>>          }
>> -        waited |= bdrv_drain_recurse(bs);
>> +        waited |= bdrv_drain_recurse(bs, begin);
>>          if (in_main_loop) {
>>              bdrv_unref(bs);
>>          }
>> @@ -221,12 +228,18 @@ static void bdrv_co_drain_bh_cb(void *opaque)
>>      BlockDriverState *bs = data->bs;
>>
>>      bdrv_dec_in_flight(bs);
>> -    bdrv_drained_begin(bs);
>> +    if (data->begin) {
>> +        bdrv_drained_begin(bs);
>> +    } else {
>> +        bdrv_drained_end(bs);
>> +    }
>> +
>>      data->done = true;
>>      aio_co_wake(co);
>>  }
>>
>> -static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
>> +static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs,
>> +                                                bool begin)
>>  {
>>      BdrvCoDrainData data;
>>
>> @@ -239,6 +252,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
>>          .co = qemu_coroutine_self(),
>>          .bs = bs,
>>          .done = false,
>> +        .begin = begin,
>>      };
>>      bdrv_inc_in_flight(bs);
>>      aio_bh_schedule_oneshot(bdrv_get_aio_context(bs),
>> @@ -253,7 +267,7 @@ static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
>>  void bdrv_drained_begin(BlockDriverState *bs)
>>  {
>>      if (qemu_in_coroutine()) {
>> -        bdrv_co_yield_to_drain(bs);
>> +        bdrv_co_yield_to_drain(bs, true);
>>          return;
>>      }
>>
>> @@ -262,17 +276,22 @@ void bdrv_drained_begin(BlockDriverState *bs)
>>          bdrv_parent_drained_begin(bs);
>>      }
>>
>> -    bdrv_drain_recurse(bs);
>> +    bdrv_drain_recurse(bs, true);
>>  }
>>
>>  void bdrv_drained_end(BlockDriverState *bs)
>>  {
>> +    if (qemu_in_coroutine()) {
>> +        bdrv_co_yield_to_drain(bs, false);
>> +        return;
>> +    }
>>      assert(bs->quiesce_counter > 0);
>>      if (atomic_fetch_dec(&bs->quiesce_counter) > 1) {
>>          return;
>>      }
>>
>>      bdrv_parent_drained_end(bs);
>> +    bdrv_drain_recurse(bs, false);
>>      aio_enable_external(bdrv_get_aio_context(bs));
>>  }
>>
>> @@ -350,7 +369,7 @@ void bdrv_drain_all_begin(void)
>>              aio_context_acquire(aio_context);
>>              for (bs = bdrv_first(&it); bs; bs = bdrv_next(&it)) {
>>                  if (aio_context == bdrv_get_aio_context(bs)) {
>> -                    waited |= bdrv_drain_recurse(bs);
>> +                    waited |= bdrv_drain_recurse(bs, true);
>>                  }
>>              }
>>              aio_context_release(aio_context);
>> @@ -371,6 +390,7 @@ void bdrv_drain_all_end(void)
>>          aio_context_acquire(aio_context);
>>          aio_enable_external(aio_context);
>>          bdrv_parent_drained_end(bs);
>> +        bdrv_drain_recurse(bs, false);
>>          aio_context_release(aio_context);
>>      }
>>
>> --
>> 2.11.0
>>
>

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

Re: [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end callback

[Fam Zheng]

On Thu, 09/21 18:39, Manos Pitsidianakis wrote:
> On Thu, Sep 21, 2017 at 09:29:43PM +0800, Fam Zheng wrote:
> > On Thu, 09/21 16:17, Manos Pitsidianakis wrote:
> > > BlockDriverState has a bdrv_do_drain() callback but no equivalent for the end
> > 
> > s/bdrv_do_drain/bdrv_co_drain/
> > 
> > > of the drain. The throttle driver (block/throttle.c) needs a way to mark the
> > > end of the drain in order to toggle io_limits_disabled correctly, thus
> > > bdrv_co_drain_end is needed.
> > > 
> > > Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> > > ---
> > >  include/block/block_int.h |  6 ++++++
> > >  block/io.c                | 48 +++++++++++++++++++++++++++++++++--------------
> > >  2 files changed, 40 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > > index ba4c383393..21950cfda3 100644
> > > --- a/include/block/block_int.h
> > > +++ b/include/block/block_int.h
> > > @@ -356,8 +356,14 @@ struct BlockDriver {
> > >      /**
> > >       * Drain and stop any internal sources of requests in the driver, and
> > >       * remain so until next I/O callback (e.g. bdrv_co_writev) is called.
> > 
> > This line needs update too, maybe:
> > 
> >       /**
> >        * bdrv_co_drain drains and stops any ... and remain so until
> >        * bdrv_co_drain_end is called.
> > 
> > > +     *
> > > +     * The callbacks are called at the beginning and ending of the drain
> > > +     * respectively. They should be implemented if the driver needs to e.g.
> > 
> > As implied by above change, should we explicitly require "should both be
> > implemented"? It may not be technically required, but I think is cleaner and
> > easier to reason about.
> > 
> 
> It might imply to someone that there's an assert(drv->bdrv_co_drain_begin &&
> drv->bdrv_co_drain_end) somewhere unless you state they don't have to be
> implemented at the same time. How about we be completely explicit:
> 
>  bdrv_co_drain_begin is called if implemented in the beggining of a
>  drain operation to drain and stop any internal sources of requests in
>  the driver.
>  bdrv_co_drain_end is called if implemented at the end of the drain.
> 
>  They should be used by the driver to e.g. manage scheduled I/O
>  requests, or toggle an internal state. After the end of the drain new
>  requests will continue normally.
> 
> I hope this is easier for a reader to understand!

I don't like the inconsistent semantics of when the drained section ends, if we
allow drivers to implement bdrv_co_drain_begin but omit bdrv_co_drained_end.
Currently the point where the section ends is, as said in the comment, when next
I/O callback is invoked. Now we are adding the explicit ".bdrv_co_drain_end"
into the fomular, if we still keep the previous convention, the interface
contract is just mixed of two things for no good reason. I don't think it's
technically necessary.

Let's just add the assert:

    assert(!!drv->bdrv_co_drain_begin == !!bdrv_co_drain_end);

in bdrv_drain_invoke, add a comment here, then add an empty .bdrv_co_drain_end
in qed.

Fam

Re: [Qemu-devel] [PATCH v2 3/3] block/throttle.c: add bdrv_co_drain_begin/end callbacks

[Stefan Hajnoczi]

On Thu, Sep 21, 2017 at 04:17:07PM +0300, Manos Pitsidianakis wrote:
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block/throttle.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end callback

[Kevin Wolf]

Am 22.09.2017 um 04:30 hat Fam Zheng geschrieben:
> On Thu, 09/21 18:39, Manos Pitsidianakis wrote:
> > On Thu, Sep 21, 2017 at 09:29:43PM +0800, Fam Zheng wrote:
> > > On Thu, 09/21 16:17, Manos Pitsidianakis wrote:
> > It might imply to someone that there's an assert(drv->bdrv_co_drain_begin &&
> > drv->bdrv_co_drain_end) somewhere unless you state they don't have to be
> > implemented at the same time. How about we be completely explicit:
> > 
> >  bdrv_co_drain_begin is called if implemented in the beggining of a
> >  drain operation to drain and stop any internal sources of requests in
> >  the driver.
> >  bdrv_co_drain_end is called if implemented at the end of the drain.
> > 
> >  They should be used by the driver to e.g. manage scheduled I/O
> >  requests, or toggle an internal state. After the end of the drain new
> >  requests will continue normally.
> > 
> > I hope this is easier for a reader to understand!
> 
> I don't like the inconsistent semantics of when the drained section
> ends, if we allow drivers to implement bdrv_co_drain_begin but omit
> bdrv_co_drained_end.  Currently the point where the section ends is,
> as said in the comment, when next I/O callback is invoked. Now we are
> adding the explicit ".bdrv_co_drain_end" into the fomular, if we still
> keep the previous convention, the interface contract is just mixed of
> two things for no good reason. I don't think it's technically
> necessary.

We don't keep the convention with the next I/O callback. We just allow
drivers to omit an empty implementation of either callback, which seems
to be a very sensible default to me.

> Let's just add the assert:
> 
>     assert(!!drv->bdrv_co_drain_begin == !!bdrv_co_drain_end);
> 
> in bdrv_drain_invoke, add a comment here

I'm not in favour of this, but if we do want to have it, wouldn't
bdrv_register() be a much better place for the assertion?

> then add an empty .bdrv_co_drain_end in qed.

If you need empty functions anywhere, it's a sign that we have a bad
default behaviour.

Kevin

Re: [Qemu-devel] [PATCH v2 1/3] block: add bdrv_co_drain_end callback

[Fam Zheng]

On Fri, 09/22 13:03, Kevin Wolf wrote:
> Am 22.09.2017 um 04:30 hat Fam Zheng geschrieben:
> > On Thu, 09/21 18:39, Manos Pitsidianakis wrote:
> > > On Thu, Sep 21, 2017 at 09:29:43PM +0800, Fam Zheng wrote:
> > > > On Thu, 09/21 16:17, Manos Pitsidianakis wrote:
> > > It might imply to someone that there's an assert(drv->bdrv_co_drain_begin &&
> > > drv->bdrv_co_drain_end) somewhere unless you state they don't have to be
> > > implemented at the same time. How about we be completely explicit:
> > > 
> > >  bdrv_co_drain_begin is called if implemented in the beggining of a
> > >  drain operation to drain and stop any internal sources of requests in
> > >  the driver.
> > >  bdrv_co_drain_end is called if implemented at the end of the drain.
> > > 
> > >  They should be used by the driver to e.g. manage scheduled I/O
> > >  requests, or toggle an internal state. After the end of the drain new
> > >  requests will continue normally.
> > > 
> > > I hope this is easier for a reader to understand!
> > 
> > I don't like the inconsistent semantics of when the drained section
> > ends, if we allow drivers to implement bdrv_co_drain_begin but omit
> > bdrv_co_drained_end.  Currently the point where the section ends is,
> > as said in the comment, when next I/O callback is invoked. Now we are
> > adding the explicit ".bdrv_co_drain_end" into the fomular, if we still
> > keep the previous convention, the interface contract is just mixed of
> > two things for no good reason. I don't think it's technically
> > necessary.
> 
> We don't keep the convention with the next I/O callback. We just allow
> drivers to omit an empty implementation of either callback, which seems
> to be a very sensible default to me.
> 

OK, I'm fine with this.

Fam