[Qemu-devel] [PATCH 0/2] Minor throttling updates

[Qemu-devel] [PATCH 0/2] Minor throttling updates

[Alberto Garcia]

Here's a couple of patches for the throttling code. I think the commit
messages are clear enough, but if you have any comments or questions
I'll be glad to hear them.

Berto

Alberto Garcia (2):
  throttle: Check for pending requests in throttle_group_unregister_bs()
  throttle: Use bs->throttle_state instead of bs->io_limits_enabled

 block.c                   | 6 +++---
 block/qapi.c              | 2 +-
 block/throttle-groups.c   | 7 +++++++
 blockdev.c                | 4 ++--
 include/block/block_int.h | 5 ++++-
 5 files changed, 17 insertions(+), 7 deletions(-)

-- 
2.6.2

[Qemu-devel] [PATCH 1/2] throttle: Check for pending requests in throttle_group_unregister_bs()

[Alberto Garcia]

throttle_group_unregister_bs() removes a BlockDriverState from its
throttling group and destroys the timers. This means that there must
be no pending throttled requests at that point (because it would be
impossible to complete them), so the caller has to drain them first.

At the moment throttle_group_unregister_bs() is only called from
bdrv_io_limits_disable(), which already takes care of draining the
requests, so there's nothing to worry about, but this patch makes
this invariant explicit in the documentation and adds the relevant
assertions.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block/throttle-groups.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 3419af7..13b5baa 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -437,6 +437,9 @@ void throttle_group_register_bs(BlockDriverState *bs, const char *groupname)
  * list, destroying the timers and setting the throttle_state pointer
  * to NULL.
  *
+ * The BlockDriverState must not have pending throttled requests, so
+ * the caller has to drain them first.
+ *
  * The group will be destroyed if it's empty after this operation.
  *
  * @bs: the BlockDriverState to remove
@@ -446,6 +449,10 @@ void throttle_group_unregister_bs(BlockDriverState *bs)
     ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
     int i;
 
+    assert(bs->pending_reqs[0] == 0 && bs->pending_reqs[1] == 0);
+    assert(qemu_co_queue_empty(&bs->throttled_reqs[0]));
+    assert(qemu_co_queue_empty(&bs->throttled_reqs[1]));
+
     qemu_mutex_lock(&tg->lock);
     for (i = 0; i < 2; i++) {
         if (tg->tokens[i] == bs) {
-- 
2.6.2

[Qemu-devel] [PATCH 2/2] throttle: Use bs->throttle_state instead of bs->io_limits_enabled

[Alberto Garcia]

There are two ways to check for I/O limits in a BlockDriverState:

- bs->throttle_state: if this pointer is not NULL, it means that this
  BDS is member of a throttling group, its ThrottleTimers structure
  has been initialized and its I/O limits are ready to be applied.

- bs->io_limits_enabled: if true it means that the throttle_state
  pointer is valid _and_ the limits are currently enabled.

The latter is used in several places to check whether a BDS has I/O
limits configured, but what it really checks is whether requests
are being throttled or not. For example, io_limits_enabled can be
temporarily set to false in cases like bdrv_read_unthrottled() without
otherwise touching the throtting configuration of that BDS.

This patch replaces bs->io_limits_enabled with bs->throttle_state in
all cases where what we really want to check is the existence of I/O
limits, not whether they are currently enabled or not.

Signed-off-by: Alberto Garcia <berto@igalia.com>
---
 block.c                   | 6 +++---
 block/qapi.c              | 2 +-
 blockdev.c                | 4 ++--
 include/block/block_int.h | 5 ++++-
 4 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/block.c b/block.c
index e9f40dc..2f433d6 100644
--- a/block.c
+++ b/block.c
@@ -1901,7 +1901,7 @@ void bdrv_close(BlockDriverState *bs)
     }
 
     /* Disable I/O limits and drain all pending throttled requests */
-    if (bs->io_limits_enabled) {
+    if (bs->throttle_state) {
         bdrv_io_limits_disable(bs);
     }
 
@@ -3706,7 +3706,7 @@ void bdrv_detach_aio_context(BlockDriverState *bs)
         baf->detach_aio_context(baf->opaque);
     }
 
-    if (bs->io_limits_enabled) {
+    if (bs->throttle_state) {
         throttle_timers_detach_aio_context(&bs->throttle_timers);
     }
     if (bs->drv->bdrv_detach_aio_context) {
@@ -3742,7 +3742,7 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
     if (bs->drv->bdrv_attach_aio_context) {
         bs->drv->bdrv_attach_aio_context(bs, new_context);
     }
-    if (bs->io_limits_enabled) {
+    if (bs->throttle_state) {
         throttle_timers_attach_aio_context(&bs->throttle_timers, new_context);
     }
 
diff --git a/block/qapi.c b/block/qapi.c
index ec0f513..89d4274 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -64,7 +64,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockDriverState *bs, Error **errp)
     info->backing_file_depth = bdrv_get_backing_file_depth(bs);
     info->detect_zeroes = bs->detect_zeroes;
 
-    if (bs->io_limits_enabled) {
+    if (bs->throttle_state) {
         ThrottleConfig cfg;
 
         throttle_group_get_config(bs, &cfg);
diff --git a/blockdev.c b/blockdev.c
index 8b8bfa9..a418fbf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2161,14 +2161,14 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
     if (throttle_enabled(&cfg)) {
         /* Enable I/O limits if they're not enabled yet, otherwise
          * just update the throttling group. */
-        if (!bs->io_limits_enabled) {
+        if (!bs->throttle_state) {
             bdrv_io_limits_enable(bs, has_group ? group : device);
         } else if (has_group) {
             bdrv_io_limits_update_group(bs, group);
         }
         /* Set the new throttling configuration */
         bdrv_set_io_limits(bs, &cfg);
-    } else if (bs->io_limits_enabled) {
+    } else if (bs->throttle_state) {
         /* If all throttling settings are set to 0, disable I/O limits */
         bdrv_io_limits_disable(bs);
     }
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 3ceeb5a..533500e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -390,7 +390,10 @@ struct BlockDriverState {
     /* number of in-flight serialising requests */
     unsigned int serialising_in_flight;
 
-    /* I/O throttling */
+    /* 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 */
     CoQueue      throttled_reqs[2];
     bool         io_limits_enabled;
     /* The following fields are protected by the ThrottleGroup lock.
-- 
2.6.2

Re: [Qemu-devel] [PATCH 0/2] Minor throttling updates

[Kevin Wolf]

Am 04.11.2015 um 14:15 hat Alberto Garcia geschrieben:
> Here's a couple of patches for the throttling code. I think the commit
> messages are clear enough, but if you have any comments or questions
> I'll be glad to hear them.

Thanks, applied to the block branch.

Kevin