diff mbox

[v2,09/13] block: Drain throttling queue with BdrvChild callback

Message ID 1461346962-4676-10-git-send-email-kwolf@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kevin Wolf April 22, 2016, 5:42 p.m. UTC 
This removes the last part of I/O throttling from block/io.c and moves
it to the BlockBackend.

Instead of having knowledge about throttling inside io.c, we can call a
BdrvChild callback .drained_begin/end, which happens to drain the
throttled requests for BlockBackend parents.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/block-backend.c     | 32 +++++++++++++++++++++++++++-----
 block/io.c                | 39 ++++++++++++++++++---------------------
 include/block/block_int.h |  8 +++-----
 3 files changed, 48 insertions(+), 31 deletions(-)

Comments

Alberto Garcia May 6, 2016, 3:13 p.m. UTC | #1 
On Fri 22 Apr 2016 07:42:38 PM CEST, Kevin Wolf wrote:
> This removes the last part of I/O throttling from block/io.c and moves
> it to the BlockBackend.
>
> Instead of having knowledge about throttling inside io.c, we can call a
> BdrvChild callback .drained_begin/end, which happens to drain the
> throttled requests for BlockBackend parents.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>

Reviewed-by: Alberto Garcia <berto@igalia.com>

Berto
Stefan Hajnoczi May 9, 2016, 1:17 p.m. UTC | #2 
On Fri, Apr 22, 2016 at 07:42:38PM +0200, Kevin Wolf wrote:
> This removes the last part of I/O throttling from block/io.c and moves
> it to the BlockBackend.
> 
> Instead of having knowledge about throttling inside io.c, we can call a
> BdrvChild callback .drained_begin/end, which happens to drain the
> throttled requests for BlockBackend parents.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/block-backend.c     | 32 +++++++++++++++++++++++++++-----
>  block/io.c                | 39 ++++++++++++++++++---------------------
>  include/block/block_int.h |  8 +++-----
>  3 files changed, 48 insertions(+), 31 deletions(-)

I'm confused about the naming.  BdrvChildRole.drained_begin/end and
bdrv_parent_drained_begin/end have nothing to do with
bdrv_drained_begin()/bdrv_drained_end()?

If these were callbacks that happened at bdrv_drained_begin/end time
then I could understand, but that doesn't seem to be the case.

What are the semantics of these callbacks?  Maybe we can find a clearer
name.  I think the point is not to "drain" (in the sense of completing
requests) but simply to restart queued requests?
Kevin Wolf May 9, 2016, 3:42 p.m. UTC | #3 
Am 09.05.2016 um 15:17 hat Stefan Hajnoczi geschrieben:
> On Fri, Apr 22, 2016 at 07:42:38PM +0200, Kevin Wolf wrote:
> > This removes the last part of I/O throttling from block/io.c and moves
> > it to the BlockBackend.
> > 
> > Instead of having knowledge about throttling inside io.c, we can call a
> > BdrvChild callback .drained_begin/end, which happens to drain the
> > throttled requests for BlockBackend parents.
> > 
> > Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> > ---
> >  block/block-backend.c     | 32 +++++++++++++++++++++++++++-----
> >  block/io.c                | 39 ++++++++++++++++++---------------------
> >  include/block/block_int.h |  8 +++-----
> >  3 files changed, 48 insertions(+), 31 deletions(-)
> 
> I'm confused about the naming.  BdrvChildRole.drained_begin/end and
> bdrv_parent_drained_begin/end have nothing to do with
> bdrv_drained_begin()/bdrv_drained_end()?

Hm, you may have a point there. I think we need to add another pair of
calls in bdrv_drained_begin()/bdrv_drained_end().

We just want to call the callbacks as well on a simple bdrv_drain() or
bdrv_drain_all(), so the existing calls should be right.

> If these were callbacks that happened at bdrv_drained_begin/end time
> then I could understand, but that doesn't seem to be the case.
> 
> What are the semantics of these callbacks?  Maybe we can find a clearer
> name.  I think the point is not to "drain" (in the sense of completing
> requests) but simply to restart queued requests?

This is just a different perspective on the same thing, as these
callbacks are always called as part of a bdrv_drain(). Any request that
is restarted is also completed before bdrv_drain() returns.

The intended semantics is that a parent doesn't submit new requests
after returning from .drained_begin() until .drained_end() is called.
The easy implementation for throttling was to simply restart the
requests like the old implementation did.

Kevin
diff mbox

Patch

diff --git a/block/block-backend.c b/block/block-backend.c
index b72a73d..73ff5e6 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -91,9 +91,14 @@  static void blk_root_inherit_options(int *child_flags, QDict *child_options,
     /* We're not supposed to call this function for root nodes */
     abort();
 }
+static void blk_root_drained_begin(BdrvChild *child);
+static void blk_root_drained_end(BdrvChild *child);
 
 static const BdrvChildRole child_root = {
-    .inherit_options = blk_root_inherit_options,
+    .inherit_options    = blk_root_inherit_options,
+
+    .drained_begin      = blk_root_drained_begin,
+    .drained_end        = blk_root_drained_end,
 };
 
 /*
@@ -836,9 +841,9 @@  int blk_read_unthrottled(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
         return ret;
     }
 
-    bdrv_no_throttling_begin(blk_bs(blk));
+    blk_root_drained_begin(blk->root);
     ret = blk_read(blk, sector_num, buf, nb_sectors);
-    bdrv_no_throttling_end(blk_bs(blk));
+    blk_root_drained_end(blk->root);
     return ret;
 }
 
@@ -1677,9 +1682,9 @@  void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg)
 void blk_io_limits_disable(BlockBackend *blk)
 {
     assert(blk->public.throttle_state);
-    bdrv_no_throttling_begin(blk_bs(blk));
+    bdrv_drained_begin(blk_bs(blk));
     throttle_group_unregister_blk(blk);
-    bdrv_no_throttling_end(blk_bs(blk));
+    bdrv_drained_end(blk_bs(blk));
 }
 
 /* should be called before bdrv_set_io_limits if a limit is set */
@@ -1705,3 +1710,20 @@  void blk_io_limits_update_group(BlockBackend *blk, const char *group)
     blk_io_limits_disable(blk);
     blk_io_limits_enable(blk, group);
 }
+
+static void blk_root_drained_begin(BdrvChild *child)
+{
+    BlockBackend *blk = child->opaque;
+
+    if (blk->public.io_limits_disabled++ == 0) {
+        throttle_group_restart_blk(blk);
+    }
+}
+
+static void blk_root_drained_end(BdrvChild *child)
+{
+    BlockBackend *blk = child->opaque;
+
+    assert(blk->public.io_limits_disabled);
+    --blk->public.io_limits_disabled;
+}
diff --git a/block/io.c b/block/io.c
index b892fce..b239e97 100644
--- a/block/io.c
+++ b/block/io.c
@@ -27,7 +27,6 @@ 
 #include "sysemu/block-backend.h"
 #include "block/blockjob.h"
 #include "block/block_int.h"
-#include "block/throttle-groups.h"
 #include "qemu/cutils.h"
 #include "qapi/error.h"
 #include "qemu/error-report.h"
@@ -58,28 +57,26 @@  static void coroutine_fn bdrv_co_do_rw(void *opaque);
 static int coroutine_fn bdrv_co_do_write_zeroes(BlockDriverState *bs,
     int64_t sector_num, int nb_sectors, BdrvRequestFlags flags);
 
-void bdrv_no_throttling_begin(BlockDriverState *bs)
+static void bdrv_parent_drained_begin(BlockDriverState *bs)
 {
-    if (!bs->blk) {
-        return;
-    }
+    BdrvChild *c;
 
-    if (blk_get_public(bs->blk)->io_limits_disabled++ == 0) {
-        throttle_group_restart_blk(bs->blk);
+    QLIST_FOREACH(c, &bs->parents, next_parent) {
+        if (c->role->drained_begin) {
+            c->role->drained_begin(c);
+        }
     }
 }
 
-void bdrv_no_throttling_end(BlockDriverState *bs)
+static void bdrv_parent_drained_end(BlockDriverState *bs)
 {
-    BlockBackendPublic *blkp;
+    BdrvChild *c;
 
-    if (!bs->blk) {
-        return;
+    QLIST_FOREACH(c, &bs->parents, next_parent) {
+        if (c->role->drained_end) {
+            c->role->drained_end(c);
+        }
     }
-
-    blkp = blk_get_public(bs->blk);
-    assert(blkp->io_limits_disabled);
-    --blkp->io_limits_disabled;
 }
 
 void bdrv_setup_io_funcs(BlockDriver *bdrv)
@@ -278,17 +275,17 @@  static void coroutine_fn bdrv_co_yield_to_drain(BlockDriverState *bs)
  */
 void coroutine_fn bdrv_co_drain(BlockDriverState *bs)
 {
-    bdrv_no_throttling_begin(bs);
+    bdrv_parent_drained_begin(bs);
     bdrv_io_unplugged_begin(bs);
     bdrv_drain_recurse(bs);
     bdrv_co_yield_to_drain(bs);
     bdrv_io_unplugged_end(bs);
-    bdrv_no_throttling_end(bs);
+    bdrv_parent_drained_end(bs);
 }
 
 void bdrv_drain(BlockDriverState *bs)
 {
-    bdrv_no_throttling_begin(bs);
+    bdrv_parent_drained_begin(bs);
     bdrv_io_unplugged_begin(bs);
     bdrv_drain_recurse(bs);
     if (qemu_in_coroutine()) {
@@ -297,7 +294,7 @@  void bdrv_drain(BlockDriverState *bs)
         bdrv_drain_poll(bs);
     }
     bdrv_io_unplugged_end(bs);
-    bdrv_no_throttling_end(bs);
+    bdrv_parent_drained_end(bs);
 }
 
 /*
@@ -320,7 +317,7 @@  void bdrv_drain_all(void)
         if (bs->job) {
             block_job_pause(bs->job);
         }
-        bdrv_no_throttling_begin(bs);
+        bdrv_parent_drained_begin(bs);
         bdrv_io_unplugged_begin(bs);
         bdrv_drain_recurse(bs);
         aio_context_release(aio_context);
@@ -363,7 +360,7 @@  void bdrv_drain_all(void)
 
         aio_context_acquire(aio_context);
         bdrv_io_unplugged_end(bs);
-        bdrv_no_throttling_end(bs);
+        bdrv_parent_drained_end(bs);
         if (bs->job) {
             block_job_resume(bs->job);
         }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ddadb7f..cfcdaec 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -361,6 +361,9 @@  typedef struct BdrvAioNotifier {
 struct BdrvChildRole {
     void (*inherit_options)(int *child_flags, QDict *child_options,
                             int parent_flags, QDict *parent_options);
+
+    void (*drained_begin)(BdrvChild *child);
+    void (*drained_end)(BdrvChild *child);
 };
 
 extern const BdrvChildRole child_file;
@@ -518,8 +521,6 @@  int get_tmp_filename(char *filename, int size);
 BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
                             const char *filename);
 
-bool bdrv_start_throttled_reqs(BlockDriverState *bs);
-
 
 /**
  * bdrv_add_before_write_notifier:
@@ -702,9 +703,6 @@  BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,
                                   const BdrvChildRole *child_role);
 void bdrv_root_unref_child(BdrvChild *child);
 
-void bdrv_no_throttling_begin(BlockDriverState *bs);
-void bdrv_no_throttling_end(BlockDriverState *bs);
-
 void blk_dev_change_media_cb(BlockBackend *blk, bool load);
 bool blk_dev_has_removable_media(BlockBackend *blk);
 bool blk_dev_has_tray(BlockBackend *blk);