[4/7] block: introduce bdrv_no_throttling_begin/end

Commit Message

Paolo Bonzini March 24, 2016, 4:39 p.m. UTC

Extract the handling of throttling from bdrv_flush_io_queue.  These
new functions will soon become BdrvChildRole callbacks, as they can
be generalized to "beginning of drain" and "end of drain".

Signed-off-by: Paolo Bonzini <[e-mail]>
---
	v2->v3: rebase, drop recursion

 block.c                   |  1 -
 block/block-backend.c     |  6 ++----
 block/io.c                | 30 ++++++++++++++++++------------
 block/throttle-groups.c   |  4 ++++
 include/block/block_int.h |  9 ++++++---
 5 files changed, 30 insertions(+), 20 deletions(-)

Comments

Alberto Garcia March 30, 2016, 12:24 p.m. UTC | #1

On Thu 24 Mar 2016 05:39:23 PM CET, Paolo Bonzini wrote:
> Extract the handling of throttling from bdrv_flush_io_queue.  These
> new functions will soon become BdrvChildRole callbacks, as they can
> be generalized to "beginning of drain" and "end of drain".
>
> Signed-off-by: Paolo Bonzini <[e-mail]>

I like the idea, thanks for the patch!

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

> +void bdrv_no_throttling_end(BlockDriverState *bs)
> +{
> +    --bs->io_limits_disabled;
>  }

Not very important, but you could assert that this doesn't go below 0.

Berto

Paolo Bonzini March 30, 2016, 12:27 p.m. UTC | #2

On 30/03/2016 14:24, Alberto Garcia wrote:
>> > +void bdrv_no_throttling_end(BlockDriverState *bs)
>> > +{
>> > +    --bs->io_limits_disabled;
>> >  }
> Not very important, but you could assert that this doesn't go below 0.

Sure, why not.

Paolo

Patch

diff --git a/block.c b/block.c
index b4107fc..9cc0eb4 100644
--- a/block.c
+++ b/block.c
@@ -2300,7 +2300,6 @@  static void swap_feature_fields(BlockDriverState *bs_top,
 
     assert(!bs_new->throttle_state);
     if (bs_top->throttle_state) {
-        assert(bs_top->io_limits_enabled);
         bdrv_io_limits_enable(bs_new, throttle_group_get_name(bs_top));
         bdrv_io_limits_disable(bs_top);
     }
diff --git a/block/block-backend.c b/block/block-backend.c
index dca21d1..51e9e49 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -800,7 +800,6 @@  int blk_read_unthrottled(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
                          int nb_sectors)
 {
     BlockDriverState *bs = blk_bs(blk);
-    bool enabled;
     int ret;
 
     ret = blk_check_request(blk, sector_num, nb_sectors);
@@ -808,10 +807,9 @@  int blk_read_unthrottled(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
         return ret;
     }
 
-    enabled = bs->io_limits_enabled;
-    bs->io_limits_enabled = false;
+    bdrv_no_throttling_begin(bs);
     ret = blk_read(blk, sector_num, buf, nb_sectors);
-    bs->io_limits_enabled = enabled;
+    bdrv_no_throttling_end(bs);
     return ret;
 }
 
diff --git a/block/io.c b/block/io.c
index 93696df..516cfb9 100644
--- a/block/io.c
+++ b/block/io.c
@@ -63,28 +63,31 @@  void bdrv_set_io_limits(BlockDriverState *bs,
     throttle_group_config(bs, cfg);
 }
 
-static void bdrv_start_throttled_reqs(BlockDriverState *bs)
+void bdrv_no_throttling_begin(BlockDriverState *bs)
 {
-    bool enabled = bs->io_limits_enabled;
+    if (bs->io_limits_disabled++ == 0) {
+        throttle_group_restart_bs(bs);
+    }
+}
 
-    bs->io_limits_enabled = false;
-    throttle_group_restart_bs(bs);
-    bs->io_limits_enabled = enabled;
+void bdrv_no_throttling_end(BlockDriverState *bs)
+{
+    --bs->io_limits_disabled;
 }
 
 void bdrv_io_limits_disable(BlockDriverState *bs)
 {
-    bs->io_limits_enabled = false;
-    bdrv_start_throttled_reqs(bs);
+    assert(bs->throttle_state);
+    bdrv_no_throttling_begin(bs);
     throttle_group_unregister_bs(bs);
+    bdrv_no_throttling_end(bs);
 }
 
 /* should be called before bdrv_set_io_limits if a limit is set */
 void bdrv_io_limits_enable(BlockDriverState *bs, const char *group)
 {
-    assert(!bs->io_limits_enabled);
+    assert(!bs->throttle_state);
     throttle_group_register_bs(bs, group);
-    bs->io_limits_enabled = true;
 }
 
 void bdrv_io_limits_update_group(BlockDriverState *bs, const char *group)
@@ -249,6 +252,7 @@  void bdrv_drain(BlockDriverState *bs)
 {
     bool busy = true;
 
+    bdrv_no_throttling_begin(bs);
     bdrv_drain_recurse(bs);
     while (busy) {
         /* Keep iterating */
@@ -256,6 +260,7 @@  void bdrv_drain(BlockDriverState *bs)
          busy = bdrv_requests_pending(bs);
          busy |= aio_poll(bdrv_get_aio_context(bs), busy);
     }
+    bdrv_no_throttling_end(bs);
 }
 
 /*
@@ -278,6 +283,7 @@  void bdrv_drain_all(void)
         if (bs->job) {
             block_job_pause(bs->job);
         }
+        bdrv_no_throttling_begin(bs);
         bdrv_drain_recurse(bs);
         aio_context_release(aio_context);
 
@@ -319,6 +325,7 @@  void bdrv_drain_all(void)
         AioContext *aio_context = bdrv_get_aio_context(bs);
 
         aio_context_acquire(aio_context);
+        bdrv_no_throttling_end(bs);
         if (bs->job) {
             block_job_resume(bs->job);
         }
@@ -921,7 +928,7 @@  int coroutine_fn bdrv_co_do_preadv(BlockDriverState *bs,
     }
 
     /* throttling disk I/O */
-    if (bs->io_limits_enabled) {
+    if (bs->throttle_state) {
         throttle_group_co_io_limits_intercept(bs, bytes, false);
     }
 
@@ -1263,7 +1270,7 @@  int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
     }
 
     /* throttling disk I/O */
-    if (bs->io_limits_enabled) {
+    if (bs->throttle_state) {
         throttle_group_co_io_limits_intercept(bs, bytes, true);
     }
 
@@ -2698,7 +2705,6 @@  void bdrv_flush_io_queue(BlockDriverState *bs)
     } else if (bs->file) {
         bdrv_flush_io_queue(bs->file->bs);
     }
-    bdrv_start_throttled_reqs(bs);
 }
 
 void bdrv_drained_begin(BlockDriverState *bs)
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 9f52d2b..b348886 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -219,6 +219,10 @@  static bool throttle_group_schedule_timer(BlockDriverState *bs,
     ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
     bool must_wait;
 
+    if (bs->io_limits_disabled) {
+        return false;
+    }
+
     /* Check if any of the timers in this group is already armed */
     if (tg->any_timer_armed[is_write]) {
         return true;
diff --git a/include/block/block_int.h b/include/block/block_int.h
index ba6e9ac..cfcbd96 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -412,10 +412,10 @@  struct BlockDriverState {
 
     /* I/O throttling.
      * throttle_state tells us if this BDS has I/O limits configured.
-     * io_limits_enabled tells us if they are currently being
-     * enforced, but it can be temporarily set to false */
+     * io_limits_disabled tells us if they are currently being enforced */
     CoQueue      throttled_reqs[2];
-    bool         io_limits_enabled;
+    unsigned int io_limits_disabled;
+
     /* The following fields are protected by the ThrottleGroup lock.
      * See the ThrottleGroup documentation for details. */
     ThrottleState *throttle_state;
@@ -722,4 +722,7 @@  void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in);
 
 void blockdev_close_all_bdrv_states(void);
 
+void bdrv_no_throttling_begin(BlockDriverState *bs);
+void bdrv_no_throttling_end(BlockDriverState *bs);
+
 #endif /* BLOCK_INT_H */