* [Qemu-devel] [PATCH v2 01/13] block: Make sure throttled BDSes always have a BB
2016-04-22 17:42 [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
@ 2016-04-22 17:42 ` Kevin Wolf
2016-05-04 12:44 ` Alberto Garcia
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 02/13] block: Introduce BlockBackendPublic Kevin Wolf
` (13 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2016-04-22 17:42 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, berto, mreitz, eblake, stefanha, qemu-devel
It was already true in principle that a throttled BDS always has a BB
attached, except that the order of operations while attaching or
detaching a BDS to/from a BB wasn't careful enough.
This commit breaks graph manipulations while I/O throttling is enabled.
It would have been possible to keep things working with some temporary
hacks, but quite cumbersome, so it's not worth the hassle. We'll fix
things again in a minute.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
block.c | 14 ++++++++++++++
block/block-backend.c | 3 +++
blockdev.c | 4 ++--
tests/test-throttle.c | 11 ++++++++---
4 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/block.c b/block.c
index 6cbad0e..5e907e7 100644
--- a/block.c
+++ b/block.c
@@ -2261,8 +2261,22 @@ static void swap_feature_fields(BlockDriverState *bs_top,
assert(!bs_new->throttle_state);
if (bs_top->throttle_state) {
+ /*
+ * FIXME Need to break I/O throttling with graph manipulations
+ * temporarily because of conflicting invariants (3. will go away when
+ * throttling is fully converted to work on BlockBackends):
+ *
+ * 1. Every BlockBackend has a single root BDS
+ * 2. I/O throttling functions require an attached BlockBackend
+ * 3. We need to first enable throttling on the new BDS and then
+ * disable it on the old one (because of throttle group refcounts)
+ */
+#if 0
bdrv_io_limits_enable(bs_new, throttle_group_get_name(bs_top));
bdrv_io_limits_disable(bs_top);
+#else
+ abort();
+#endif
}
}
diff --git a/block/block-backend.c b/block/block-backend.c
index 9538e79..e4839ca 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -420,6 +420,9 @@ void blk_remove_bs(BlockBackend *blk)
notifier_list_notify(&blk->remove_bs_notifiers, blk);
blk_update_root_state(blk);
+ if (blk->root->bs->throttle_state) {
+ bdrv_io_limits_disable(blk->root->bs);
+ }
blk->root->bs->blk = NULL;
bdrv_root_unref_child(blk->root);
diff --git a/blockdev.c b/blockdev.c
index f1f520a..29ded1b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2570,8 +2570,6 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
goto fail;
}
- blk_apply_root_state(blk, medium_bs);
-
bdrv_add_key(medium_bs, NULL, &err);
if (err) {
error_propagate(errp, err);
@@ -2596,6 +2594,8 @@ void qmp_blockdev_change_medium(const char *device, const char *filename,
goto fail;
}
+ blk_apply_root_state(blk, medium_bs);
+
qmp_blockdev_close_tray(device, errp);
fail:
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index 744a524..77b95d6 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -574,11 +574,16 @@ static void test_accounting(void)
static void test_groups(void)
{
ThrottleConfig cfg1, cfg2;
+ BlockBackend *blk1, *blk2, *blk3;
BlockDriverState *bdrv1, *bdrv2, *bdrv3;
- bdrv1 = bdrv_new();
- bdrv2 = bdrv_new();
- bdrv3 = bdrv_new();
+ blk1 = blk_new_with_bs(&error_abort);
+ blk2 = blk_new_with_bs(&error_abort);
+ blk3 = blk_new_with_bs(&error_abort);
+
+ bdrv1 = blk_bs(blk1);
+ bdrv2 = blk_bs(blk2);
+ bdrv3 = blk_bs(blk3);
g_assert(bdrv1->throttle_state == NULL);
g_assert(bdrv2->throttle_state == NULL);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 01/13] block: Make sure throttled BDSes always have a BB
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 01/13] block: Make sure throttled BDSes always have a BB Kevin Wolf
@ 2016-05-04 12:44 ` Alberto Garcia
0 siblings, 0 replies; 39+ messages in thread
From: Alberto Garcia @ 2016-05-04 12:44 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: pbonzini, mreitz, eblake, stefanha, qemu-devel
On Fri 22 Apr 2016 07:42:30 PM CEST, Kevin Wolf wrote:
> It was already true in principle that a throttled BDS always has a BB
> attached, except that the order of operations while attaching or
> detaching a BDS to/from a BB wasn't careful enough.
>
> This commit breaks graph manipulations while I/O throttling is enabled.
> It would have been possible to keep things working with some temporary
> hacks, but quite cumbersome, so it's not worth the hassle. We'll fix
> things again in a minute.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 02/13] block: Introduce BlockBackendPublic
2016-04-22 17:42 [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 01/13] block: Make sure throttled BDSes always have a BB Kevin Wolf
@ 2016-04-22 17:42 ` Kevin Wolf
2016-05-04 12:45 ` Alberto Garcia
2016-05-04 14:23 ` Eric Blake
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 03/13] block: throttle-groups: Use BlockBackend pointers internally Kevin Wolf
` (12 subsequent siblings)
14 siblings, 2 replies; 39+ messages in thread
From: Kevin Wolf @ 2016-04-22 17:42 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, berto, mreitz, eblake, stefanha, qemu-devel
Some features, like I/O throttling, are implemented outside
block-backend.c, but still want to keep information in BlockBackend,
e.g. list entries that allow keeping a list of BlockBackends.
In order to avoid exposing the whole struct layout in the public header
file, this patch introduces an embedded public struct where such
information can be added and a pair of functions to convert between
BlockBackend and BlockBackendPublic.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/block-backend.c | 17 +++++++++++++++++
include/sysemu/block-backend.h | 10 ++++++++++
2 files changed, 27 insertions(+)
diff --git a/block/block-backend.c b/block/block-backend.c
index e4839ca..34798a4 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -34,6 +34,7 @@ struct BlockBackend {
DriveInfo *legacy_dinfo; /* null unless created by drive_new() */
QTAILQ_ENTRY(BlockBackend) link; /* for block_backends */
QTAILQ_ENTRY(BlockBackend) monitor_link; /* for monitor_block_backends */
+ BlockBackendPublic public;
void *dev; /* attached device model, if any */
/* TODO change to DeviceState when all users are qdevified */
@@ -411,6 +412,22 @@ BlockBackend *blk_by_legacy_dinfo(DriveInfo *dinfo)
}
/*
+ * Returns a pointer to the publicly accessible fields of @blk.
+ */
+BlockBackendPublic *blk_get_public(BlockBackend *blk)
+{
+ return &blk->public;
+}
+
+/*
+ * Returns a BlockBackend given the associated @public fields.
+ */
+BlockBackend *blk_by_public(BlockBackendPublic *public)
+{
+ return container_of(public, BlockBackend, public);
+}
+
+/*
* Disassociates the currently associated BlockDriverState from @blk.
*/
void blk_remove_bs(BlockBackend *blk)
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index c62b6fe..3915ee7 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -59,6 +59,13 @@ typedef struct BlockDevOps {
void (*resize_cb)(void *opaque);
} BlockDevOps;
+/* This struct is embedded in (the private) BlockBackend struct and contains
+ * fields that must be public. This is in particular for QLIST_ENTRY() and
+ * friends so that BlockBackends can be kept in lists outside block-backend.c */
+typedef struct BlockBackendPublic {
+ int dummy; /* empty structs are illegal */
+} BlockBackendPublic;
+
BlockBackend *blk_new(Error **errp);
BlockBackend *blk_new_with_bs(Error **errp);
BlockBackend *blk_new_open(const char *filename, const char *reference,
@@ -74,6 +81,9 @@ BlockDriverState *blk_next_root_bs(BlockDriverState *bs);
bool monitor_add_blk(BlockBackend *blk, const char *name, Error **errp);
void monitor_remove_blk(BlockBackend *blk);
+BlockBackendPublic *blk_get_public(BlockBackend *blk);
+BlockBackend *blk_by_public(BlockBackendPublic *public);
+
BlockDriverState *blk_bs(BlockBackend *blk);
void blk_remove_bs(BlockBackend *blk);
void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 02/13] block: Introduce BlockBackendPublic
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 02/13] block: Introduce BlockBackendPublic Kevin Wolf
@ 2016-05-04 12:45 ` Alberto Garcia
2016-05-04 14:23 ` Eric Blake
1 sibling, 0 replies; 39+ messages in thread
From: Alberto Garcia @ 2016-05-04 12:45 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: pbonzini, mreitz, eblake, stefanha, qemu-devel
On Fri 22 Apr 2016 07:42:31 PM CEST, Kevin Wolf wrote:
> Some features, like I/O throttling, are implemented outside
> block-backend.c, but still want to keep information in BlockBackend,
> e.g. list entries that allow keeping a list of BlockBackends.
>
> In order to avoid exposing the whole struct layout in the public header
> file, this patch introduces an embedded public struct where such
> information can be added and a pair of functions to convert between
> BlockBackend and BlockBackendPublic.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v2 02/13] block: Introduce BlockBackendPublic
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 02/13] block: Introduce BlockBackendPublic Kevin Wolf
2016-05-04 12:45 ` Alberto Garcia
@ 2016-05-04 14:23 ` Eric Blake
1 sibling, 0 replies; 39+ messages in thread
From: Eric Blake @ 2016-05-04 14:23 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: pbonzini, berto, mreitz, stefanha, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1183 bytes --]
On 04/22/2016 11:42 AM, Kevin Wolf wrote:
> Some features, like I/O throttling, are implemented outside
> block-backend.c, but still want to keep information in BlockBackend,
> e.g. list entries that allow keeping a list of BlockBackends.
>
> In order to avoid exposing the whole struct layout in the public header
> file, this patch introduces an embedded public struct where such
> information can be added and a pair of functions to convert between
> BlockBackend and BlockBackendPublic.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> +/* This struct is embedded in (the private) BlockBackend struct and contains
> + * fields that must be public. This is in particular for QLIST_ENTRY() and
> + * friends so that BlockBackends can be kept in lists outside block-backend.c */
> +typedef struct BlockBackendPublic {
> + int dummy; /* empty structs are illegal */
Nit - it's not breaking any laws, so s/illegal/invalid/ sounds slightly
better. But not enough to stop me from reviewing.
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 03/13] block: throttle-groups: Use BlockBackend pointers internally
2016-04-22 17:42 [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 01/13] block: Make sure throttled BDSes always have a BB Kevin Wolf
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 02/13] block: Introduce BlockBackendPublic Kevin Wolf
@ 2016-04-22 17:42 ` Kevin Wolf
2016-05-04 12:51 ` Alberto Garcia
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 04/13] block: Convert throttle_group_get_name() to BlockBackend Kevin Wolf
` (11 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2016-04-22 17:42 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, berto, mreitz, eblake, stefanha, qemu-devel
As a first step towards moving I/O throttling to the BlockBackend level,
this patch changes all pointers in struct ThrottleGroup from referencing
a BlockDriverState to referencing a BlockBackend.
This change is valid because we made sure that throttling can only be
enabled on BDSes which have a BB attached.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/io.c | 4 +-
block/throttle-groups.c | 134 ++++++++++++++++++++--------------------
include/block/block_int.h | 1 -
include/block/throttle-groups.h | 4 +-
include/sysemu/block-backend.h | 5 +-
tests/test-throttle.c | 13 ++--
6 files changed, 83 insertions(+), 78 deletions(-)
diff --git a/block/io.c b/block/io.c
index b903270..2e7af98 100644
--- a/block/io.c
+++ b/block/io.c
@@ -82,7 +82,7 @@ void bdrv_io_limits_disable(BlockDriverState *bs)
{
assert(bs->throttle_state);
bdrv_no_throttling_begin(bs);
- throttle_group_unregister_bs(bs);
+ throttle_group_unregister_blk(bs->blk);
bdrv_no_throttling_end(bs);
}
@@ -90,7 +90,7 @@ void bdrv_io_limits_disable(BlockDriverState *bs)
void bdrv_io_limits_enable(BlockDriverState *bs, const char *group)
{
assert(!bs->throttle_state);
- throttle_group_register_bs(bs, group);
+ throttle_group_register_blk(bs->blk, group);
}
void bdrv_io_limits_update_group(BlockDriverState *bs, const char *group)
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 9ac063a..1e6e2e5 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -23,6 +23,7 @@
*/
#include "qemu/osdep.h"
+#include "sysemu/block-backend.h"
#include "block/throttle-groups.h"
#include "qemu/queue.h"
#include "qemu/thread.h"
@@ -57,8 +58,8 @@ typedef struct ThrottleGroup {
QemuMutex lock; /* This lock protects the following four fields */
ThrottleState ts;
- QLIST_HEAD(, BlockDriverState) head;
- BlockDriverState *tokens[2];
+ QLIST_HEAD(, BlockBackendPublic) head;
+ BlockBackend *tokens[2];
bool any_timer_armed[2];
/* These two are protected by the global throttle_groups_lock */
@@ -145,81 +146,81 @@ const char *throttle_group_get_name(BlockDriverState *bs)
return tg->name;
}
-/* Return the next BlockDriverState in the round-robin sequence,
- * simulating a circular list.
+/* Return the next BlockBackend in the round-robin sequence, simulating a
+ * circular list.
*
* This assumes that tg->lock is held.
*
- * @bs: the current BlockDriverState
- * @ret: the next BlockDriverState in the sequence
+ * @blk: the current BlockBackend
+ * @ret: the next BlockBackend in the sequence
*/
-static BlockDriverState *throttle_group_next_bs(BlockDriverState *bs)
+static BlockBackend *throttle_group_next_blk(BlockBackend *blk)
{
+ BlockDriverState *bs = blk_bs(blk);
ThrottleState *ts = bs->throttle_state;
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
- BlockDriverState *next = QLIST_NEXT(bs, round_robin);
+ BlockBackendPublic *next = QLIST_NEXT(blk_get_public(blk), round_robin);
if (!next) {
- return QLIST_FIRST(&tg->head);
+ next = QLIST_FIRST(&tg->head);
}
- return next;
+ return blk_by_public(next);
}
-/* Return the next BlockDriverState in the round-robin sequence with
- * pending I/O requests.
+/* Return the next BlockBackend in the round-robin sequence with pending I/O
+ * requests.
*
* This assumes that tg->lock is held.
*
- * @bs: the current BlockDriverState
+ * @blk: the current BlockBackend
* @is_write: the type of operation (read/write)
- * @ret: the next BlockDriverState with pending requests, or bs
- * if there is none.
+ * @ret: the next BlockBackend with pending requests, or blk if there is
+ * none.
*/
-static BlockDriverState *next_throttle_token(BlockDriverState *bs,
- bool is_write)
+static BlockBackend *next_throttle_token(BlockBackend *blk, bool is_write)
{
- ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
- BlockDriverState *token, *start;
+ ThrottleGroup *tg = container_of(blk_bs(blk)->throttle_state,
+ ThrottleGroup, ts);
+ BlockBackend *token, *start;
start = token = tg->tokens[is_write];
/* get next bs round in round robin style */
- token = throttle_group_next_bs(token);
- while (token != start && !token->pending_reqs[is_write]) {
- token = throttle_group_next_bs(token);
+ token = throttle_group_next_blk(token);
+ while (token != start && !blk_bs(token)->pending_reqs[is_write]) {
+ token = throttle_group_next_blk(token);
}
/* If no IO are queued for scheduling on the next round robin token
* then decide the token is the current bs because chances are
* the current bs get the current request queued.
*/
- if (token == start && !token->pending_reqs[is_write]) {
- token = bs;
+ if (token == start && !blk_bs(token)->pending_reqs[is_write]) {
+ token = blk;
}
return token;
}
-/* Check if the next I/O request for a BlockDriverState needs to be
- * throttled or not. If there's no timer set in this group, set one
- * and update the token accordingly.
+/* Check if the next I/O request for a BlockBackend needs to be throttled or
+ * not. If there's no timer set in this group, set one and update the token
+ * accordingly.
*
* This assumes that tg->lock is held.
*
- * @bs: the current BlockDriverState
+ * @blk: the current BlockBackend
* @is_write: the type of operation (read/write)
* @ret: whether the I/O request needs to be throttled or not
*/
-static bool throttle_group_schedule_timer(BlockDriverState *bs,
- bool is_write)
+static bool throttle_group_schedule_timer(BlockBackend *blk, bool is_write)
{
- ThrottleState *ts = bs->throttle_state;
- ThrottleTimers *tt = &bs->throttle_timers;
+ ThrottleState *ts = blk_bs(blk)->throttle_state;
+ ThrottleTimers *tt = &blk_bs(blk)->throttle_timers;
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
bool must_wait;
- if (bs->io_limits_disabled) {
+ if (blk_bs(blk)->io_limits_disabled) {
return false;
}
@@ -230,9 +231,9 @@ static bool throttle_group_schedule_timer(BlockDriverState *bs,
must_wait = throttle_schedule_timer(ts, tt, is_write);
- /* If a timer just got armed, set bs as the current token */
+ /* If a timer just got armed, set blk as the current token */
if (must_wait) {
- tg->tokens[is_write] = bs;
+ tg->tokens[is_write] = blk;
tg->any_timer_armed[is_write] = true;
}
@@ -243,18 +244,19 @@ static bool throttle_group_schedule_timer(BlockDriverState *bs,
*
* This assumes that tg->lock is held.
*
- * @bs: the current BlockDriverState
+ * @blk: the current BlockBackend
* @is_write: the type of operation (read/write)
*/
-static void schedule_next_request(BlockDriverState *bs, bool is_write)
+static void schedule_next_request(BlockBackend *blk, bool is_write)
{
+ BlockDriverState *bs = blk_bs(blk);
ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
bool must_wait;
- BlockDriverState *token;
+ BlockBackend *token;
/* Check if there's any pending request to schedule next */
- token = next_throttle_token(bs, is_write);
- if (!token->pending_reqs[is_write]) {
+ token = next_throttle_token(blk, is_write);
+ if (!blk_bs(token)->pending_reqs[is_write]) {
return;
}
@@ -266,9 +268,9 @@ static void schedule_next_request(BlockDriverState *bs, bool is_write)
/* Give preference to requests from the current bs */
if (qemu_in_coroutine() &&
qemu_co_queue_next(&bs->throttled_reqs[is_write])) {
- token = bs;
+ token = blk;
} else {
- ThrottleTimers *tt = &token->throttle_timers;
+ ThrottleTimers *tt = &blk_bs(token)->throttle_timers;
int64_t now = qemu_clock_get_ns(tt->clock_type);
timer_mod(tt->timers[is_write], now + 1);
tg->any_timer_armed[is_write] = true;
@@ -290,13 +292,13 @@ void coroutine_fn throttle_group_co_io_limits_intercept(BlockDriverState *bs,
bool is_write)
{
bool must_wait;
- BlockDriverState *token;
+ BlockBackend *token;
ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
qemu_mutex_lock(&tg->lock);
/* First we check if this I/O has to be throttled. */
- token = next_throttle_token(bs, is_write);
+ token = next_throttle_token(bs->blk, is_write);
must_wait = throttle_group_schedule_timer(token, is_write);
/* Wait if there's a timer set or queued requests of this type */
@@ -312,7 +314,7 @@ void coroutine_fn throttle_group_co_io_limits_intercept(BlockDriverState *bs,
throttle_account(bs->throttle_state, is_write, bytes);
/* Schedule the next request */
- schedule_next_request(bs, is_write);
+ schedule_next_request(bs->blk, is_write);
qemu_mutex_unlock(&tg->lock);
}
@@ -395,7 +397,7 @@ static void timer_cb(BlockDriverState *bs, bool is_write)
* scheduling the next one */
if (empty_queue) {
qemu_mutex_lock(&tg->lock);
- schedule_next_request(bs, is_write);
+ schedule_next_request(bs->blk, is_write);
qemu_mutex_unlock(&tg->lock);
}
}
@@ -410,17 +412,17 @@ static void write_timer_cb(void *opaque)
timer_cb(opaque, true);
}
-/* Register a BlockDriverState in the throttling group, also
- * initializing its timers and updating its throttle_state pointer to
- * point to it. If a throttling group with that name does not exist
- * yet, it will be created.
+/* Register a BlockBackend in the throttling group, also initializing its
+ * timers and updating its throttle_state pointer to point to it. If a
+ * throttling group with that name does not exist yet, it will be created.
*
- * @bs: the BlockDriverState to insert
+ * @blk: the BlockBackend to insert
* @groupname: the name of the group
*/
-void throttle_group_register_bs(BlockDriverState *bs, const char *groupname)
+void throttle_group_register_blk(BlockBackend *blk, const char *groupname)
{
int i;
+ BlockDriverState *bs = blk_bs(blk);
ThrottleState *ts = throttle_group_incref(groupname);
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
int clock_type = QEMU_CLOCK_REALTIME;
@@ -433,14 +435,14 @@ void throttle_group_register_bs(BlockDriverState *bs, const char *groupname)
bs->throttle_state = ts;
qemu_mutex_lock(&tg->lock);
- /* If the ThrottleGroup is new set this BlockDriverState as the token */
+ /* If the ThrottleGroup is new set this BlockBackend as the token */
for (i = 0; i < 2; i++) {
if (!tg->tokens[i]) {
- tg->tokens[i] = bs;
+ tg->tokens[i] = blk;
}
}
- QLIST_INSERT_HEAD(&tg->head, bs, round_robin);
+ QLIST_INSERT_HEAD(&tg->head, blk_get_public(blk), round_robin);
throttle_timers_init(&bs->throttle_timers,
bdrv_get_aio_context(bs),
@@ -452,19 +454,19 @@ void throttle_group_register_bs(BlockDriverState *bs, const char *groupname)
qemu_mutex_unlock(&tg->lock);
}
-/* Unregister a BlockDriverState from its group, removing it from the
- * list, destroying the timers and setting the throttle_state pointer
- * to NULL.
+/* Unregister a BlockBackend from its group, removing it from the 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 BlockBackend 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
+ * @blk: the BlockBackend to remove
*/
-void throttle_group_unregister_bs(BlockDriverState *bs)
+void throttle_group_unregister_blk(BlockBackend *blk)
{
+ BlockDriverState *bs = blk_bs(blk);
ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
int i;
@@ -474,10 +476,10 @@ void throttle_group_unregister_bs(BlockDriverState *bs)
qemu_mutex_lock(&tg->lock);
for (i = 0; i < 2; i++) {
- if (tg->tokens[i] == bs) {
- BlockDriverState *token = throttle_group_next_bs(bs);
+ if (tg->tokens[i] == blk) {
+ BlockBackend *token = throttle_group_next_blk(blk);
/* Take care of the case where this is the last bs in the group */
- if (token == bs) {
+ if (token == blk) {
token = NULL;
}
tg->tokens[i] = token;
@@ -485,7 +487,7 @@ void throttle_group_unregister_bs(BlockDriverState *bs)
}
/* remove the current bs from the list */
- QLIST_REMOVE(bs, round_robin);
+ QLIST_REMOVE(blk_get_public(blk), round_robin);
throttle_timers_destroy(&bs->throttle_timers);
qemu_mutex_unlock(&tg->lock);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index f1aabb9..639165e 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -432,7 +432,6 @@ struct BlockDriverState {
ThrottleState *throttle_state;
ThrottleTimers throttle_timers;
unsigned pending_reqs[2];
- QLIST_ENTRY(BlockDriverState) round_robin;
/* Offset after the highest byte written to */
uint64_t wr_highest_offset;
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 395f72d..b9114ee 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -36,8 +36,8 @@ void throttle_group_unref(ThrottleState *ts);
void throttle_group_config(BlockDriverState *bs, ThrottleConfig *cfg);
void throttle_group_get_config(BlockDriverState *bs, ThrottleConfig *cfg);
-void throttle_group_register_bs(BlockDriverState *bs, const char *groupname);
-void throttle_group_unregister_bs(BlockDriverState *bs);
+void throttle_group_register_blk(BlockBackend *blk, const char *groupname);
+void throttle_group_unregister_blk(BlockBackend *blk);
void throttle_group_restart_bs(BlockDriverState *bs);
void coroutine_fn throttle_group_co_io_limits_intercept(BlockDriverState *bs,
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index 3915ee7..bcd62b0 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -63,7 +63,10 @@ typedef struct BlockDevOps {
* fields that must be public. This is in particular for QLIST_ENTRY() and
* friends so that BlockBackends can be kept in lists outside block-backend.c */
typedef struct BlockBackendPublic {
- int dummy; /* empty structs are illegal */
+ /* I/O throttling */
+ /* The following fields are protected by the ThrottleGroup lock.
+ * See the ThrottleGroup documentation for details. */
+ QLIST_ENTRY(BlockBackendPublic) round_robin;
} BlockBackendPublic;
BlockBackend *blk_new(Error **errp);
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index 77b95d6..beaa2a3 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -20,6 +20,7 @@
#include "qemu/throttle.h"
#include "qemu/error-report.h"
#include "block/throttle-groups.h"
+#include "sysemu/block-backend.h"
static AioContext *ctx;
static LeakyBucket bkt;
@@ -589,9 +590,9 @@ static void test_groups(void)
g_assert(bdrv2->throttle_state == NULL);
g_assert(bdrv3->throttle_state == NULL);
- throttle_group_register_bs(bdrv1, "bar");
- throttle_group_register_bs(bdrv2, "foo");
- throttle_group_register_bs(bdrv3, "bar");
+ throttle_group_register_blk(blk1, "bar");
+ throttle_group_register_blk(blk2, "foo");
+ throttle_group_register_blk(blk3, "bar");
g_assert(bdrv1->throttle_state != NULL);
g_assert(bdrv2->throttle_state != NULL);
@@ -623,9 +624,9 @@ static void test_groups(void)
throttle_group_get_config(bdrv3, &cfg2);
g_assert(!memcmp(&cfg1, &cfg2, sizeof(cfg1)));
- throttle_group_unregister_bs(bdrv1);
- throttle_group_unregister_bs(bdrv2);
- throttle_group_unregister_bs(bdrv3);
+ throttle_group_unregister_blk(blk1);
+ throttle_group_unregister_blk(blk2);
+ throttle_group_unregister_blk(blk3);
g_assert(bdrv1->throttle_state == NULL);
g_assert(bdrv2->throttle_state == NULL);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v2 04/13] block: Convert throttle_group_get_name() to BlockBackend
2016-04-22 17:42 [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
` (2 preceding siblings ...)
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 03/13] block: throttle-groups: Use BlockBackend pointers internally Kevin Wolf
@ 2016-04-22 17:42 ` Kevin Wolf
2016-05-04 12:52 ` Alberto Garcia
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 05/13] block: Move throttling fields from BDS to BB Kevin Wolf
` (10 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2016-04-22 17:42 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, berto, mreitz, eblake, stefanha, qemu-devel
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/block-backend.c | 2 +-
block/io.c | 2 +-
block/qapi.c | 2 +-
block/throttle-groups.c | 12 ++++++------
include/block/throttle-groups.h | 2 +-
tests/test-throttle.c | 4 ++--
6 files changed, 12 insertions(+), 12 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 34798a4..57cf536 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1569,7 +1569,7 @@ void blk_update_root_state(BlockBackend *blk)
throttle_group_unref(blk->root_state.throttle_state);
}
if (blk->root->bs->throttle_state) {
- const char *name = throttle_group_get_name(blk->root->bs);
+ const char *name = throttle_group_get_name(blk);
blk->root_state.throttle_group = g_strdup(name);
blk->root_state.throttle_state = throttle_group_incref(name);
} else {
diff --git a/block/io.c b/block/io.c
index 2e7af98..fbf7f31 100644
--- a/block/io.c
+++ b/block/io.c
@@ -101,7 +101,7 @@ void bdrv_io_limits_update_group(BlockDriverState *bs, const char *group)
}
/* this bs is a part of the same group than the one we want */
- if (!g_strcmp0(throttle_group_get_name(bs), group)) {
+ if (!g_strcmp0(throttle_group_get_name(bs->blk), group)) {
return;
}
diff --git a/block/qapi.c b/block/qapi.c
index c5f6ba6..a3e514d 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -118,7 +118,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
info->iops_size = cfg.op_size;
info->has_group = true;
- info->group = g_strdup(throttle_group_get_name(bs));
+ info->group = g_strdup(throttle_group_get_name(bs->blk));
}
info->write_threshold = bdrv_write_threshold_get(bs);
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 1e6e2e5..e50ccaa 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -133,16 +133,16 @@ void throttle_group_unref(ThrottleState *ts)
qemu_mutex_unlock(&throttle_groups_lock);
}
-/* Get the name from a BlockDriverState's ThrottleGroup. The name (and
- * the pointer) is guaranteed to remain constant during the lifetime
- * of the group.
+/* Get the name from a BlockBackend's ThrottleGroup. The name (and the pointer)
+ * is guaranteed to remain constant during the lifetime of the group.
*
- * @bs: a BlockDriverState that is member of a throttling group
+ * @blk: a BlockBackend that is member of a throttling group
* @ret: the name of the group.
*/
-const char *throttle_group_get_name(BlockDriverState *bs)
+const char *throttle_group_get_name(BlockBackend *blk)
{
- ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
+ ThrottleGroup *tg = container_of(blk_bs(blk)->throttle_state,
+ ThrottleGroup, ts);
return tg->name;
}
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index b9114ee..bd55a34 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -28,7 +28,7 @@
#include "qemu/throttle.h"
#include "block/block_int.h"
-const char *throttle_group_get_name(BlockDriverState *bs);
+const char *throttle_group_get_name(BlockBackend *blk);
ThrottleState *throttle_group_incref(const char *name);
void throttle_group_unref(ThrottleState *ts);
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index beaa2a3..1a322f1 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -598,8 +598,8 @@ static void test_groups(void)
g_assert(bdrv2->throttle_state != NULL);
g_assert(bdrv3->throttle_state != NULL);
- g_assert(!strcmp(throttle_group_get_name(bdrv1), "bar"));
- g_assert(!strcmp(throttle_group_get_name(bdrv2), "foo"));
+ g_assert(!strcmp(throttle_group_get_name(blk1), "bar"));
+ g_assert(!strcmp(throttle_group_get_name(blk2), "foo"));
g_assert(bdrv1->throttle_state == bdrv3->throttle_state);
/* Setting the config of a group member affects the whole group */
--
1.8.3.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v2 05/13] block: Move throttling fields from BDS to BB
2016-04-22 17:42 [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
` (3 preceding siblings ...)
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 04/13] block: Convert throttle_group_get_name() to BlockBackend Kevin Wolf
@ 2016-04-22 17:42 ` Kevin Wolf
2016-05-04 13:36 ` Alberto Garcia
` (2 more replies)
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 06/13] block: Move actual I/O throttling to BlockBackend Kevin Wolf
` (9 subsequent siblings)
14 siblings, 3 replies; 39+ messages in thread
From: Kevin Wolf @ 2016-04-22 17:42 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, berto, mreitz, eblake, stefanha, qemu-devel
This patch changes where the throttling state is stored (used to be the
BlockDriverState, now it is the BlockBackend), but it doesn't actually
make it a BB level feature yet. For example, throttling is still
disabled when the BDS is detached from the BB.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 22 ++++----
block/block-backend.c | 13 +++--
block/io.c | 36 ++++++++----
block/qapi.c | 2 +-
block/throttle-groups.c | 119 +++++++++++++++++++++-------------------
blockdev.c | 4 +-
include/block/block_int.h | 13 -----
include/block/throttle-groups.h | 2 +-
include/sysemu/block-backend.h | 11 +++-
tests/test-throttle.c | 28 ++++++----
10 files changed, 137 insertions(+), 113 deletions(-)
diff --git a/block.c b/block.c
index 5e907e7..d81fe2e 100644
--- a/block.c
+++ b/block.c
@@ -239,8 +239,6 @@ BlockDriverState *bdrv_new(void)
QLIST_INIT(&bs->op_blockers[i]);
}
notifier_with_return_list_init(&bs->before_write_notifiers);
- qemu_co_queue_init(&bs->throttled_reqs[0]);
- qemu_co_queue_init(&bs->throttled_reqs[1]);
bs->refcnt = 1;
bs->aio_context = qemu_get_aio_context();
@@ -1527,7 +1525,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
return -ENODEV;
}
- if (bs->throttle_state) {
+ if (blk_get_public(bs->blk)->throttle_state) {
error_setg(errp, "Cannot reference an existing block device for "
"which I/O throttling is enabled");
return -EINVAL;
@@ -2126,7 +2124,7 @@ static void bdrv_close(BlockDriverState *bs)
assert(!bs->job);
/* Disable I/O limits and drain all pending throttled requests */
- if (bs->throttle_state) {
+ if (bs->blk && blk_get_public(bs->blk)->throttle_state) {
bdrv_io_limits_disable(bs);
}
@@ -2259,8 +2257,8 @@ static void swap_feature_fields(BlockDriverState *bs_top,
bdrv_move_feature_fields(bs_top, bs_new);
bdrv_move_feature_fields(bs_new, &tmp);
- assert(!bs_new->throttle_state);
- if (bs_top->throttle_state) {
+ assert(!bs_new->blk);
+ if (bs_top->blk && blk_get_public(bs_top->blk)->throttle_state) {
/*
* FIXME Need to break I/O throttling with graph manipulations
* temporarily because of conflicting invariants (3. will go away when
@@ -2302,11 +2300,11 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
assert(!bdrv_requests_pending(bs_new));
bdrv_ref(bs_top);
- change_parent_backing_link(bs_top, bs_new);
/* Some fields always stay on top of the backing file chain */
swap_feature_fields(bs_top, bs_new);
+ change_parent_backing_link(bs_top, bs_new);
bdrv_set_backing_hd(bs_new, bs_top);
bdrv_unref(bs_top);
@@ -3645,8 +3643,9 @@ void bdrv_detach_aio_context(BlockDriverState *bs)
baf->detach_aio_context(baf->opaque);
}
- if (bs->throttle_state) {
- throttle_timers_detach_aio_context(&bs->throttle_timers);
+ if (bs->blk && blk_get_public(bs->blk)->throttle_state) {
+ throttle_timers_detach_aio_context(
+ &blk_get_public(bs->blk)->throttle_timers);
}
if (bs->drv->bdrv_detach_aio_context) {
bs->drv->bdrv_detach_aio_context(bs);
@@ -3681,8 +3680,9 @@ 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->throttle_state) {
- throttle_timers_attach_aio_context(&bs->throttle_timers, new_context);
+ if (bs->blk && blk_get_public(bs->blk)->throttle_state) {
+ throttle_timers_attach_aio_context(
+ &blk_get_public(bs->blk)->throttle_timers, new_context);
}
QLIST_FOREACH(ban, &bs->aio_notifiers, list) {
diff --git a/block/block-backend.c b/block/block-backend.c
index 57cf536..087e60a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -107,8 +107,12 @@ BlockBackend *blk_new(Error **errp)
blk = g_new0(BlockBackend, 1);
blk->refcnt = 1;
+ qemu_co_queue_init(&blk->public.throttled_reqs[0]);
+ qemu_co_queue_init(&blk->public.throttled_reqs[1]);
+
notifier_list_init(&blk->remove_bs_notifiers);
notifier_list_init(&blk->insert_bs_notifiers);
+
QTAILQ_INSERT_TAIL(&block_backends, blk, link);
return blk;
}
@@ -437,7 +441,7 @@ void blk_remove_bs(BlockBackend *blk)
notifier_list_notify(&blk->remove_bs_notifiers, blk);
blk_update_root_state(blk);
- if (blk->root->bs->throttle_state) {
+ if (blk->public.throttle_state) {
bdrv_io_limits_disable(blk->root->bs);
}
@@ -813,7 +817,6 @@ int blk_read(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
int blk_read_unthrottled(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
int nb_sectors)
{
- BlockDriverState *bs = blk_bs(blk);
int ret;
ret = blk_check_request(blk, sector_num, nb_sectors);
@@ -821,9 +824,9 @@ int blk_read_unthrottled(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
return ret;
}
- bdrv_no_throttling_begin(bs);
+ bdrv_no_throttling_begin(blk_bs(blk));
ret = blk_read(blk, sector_num, buf, nb_sectors);
- bdrv_no_throttling_end(bs);
+ bdrv_no_throttling_end(blk_bs(blk));
return ret;
}
@@ -1568,7 +1571,7 @@ void blk_update_root_state(BlockBackend *blk)
g_free(blk->root_state.throttle_group);
throttle_group_unref(blk->root_state.throttle_state);
}
- if (blk->root->bs->throttle_state) {
+ if (blk->public.throttle_state) {
const char *name = throttle_group_get_name(blk);
blk->root_state.throttle_group = g_strdup(name);
blk->root_state.throttle_state = throttle_group_incref(name);
diff --git a/block/io.c b/block/io.c
index fbf7f31..34d1bf6 100644
--- a/block/io.c
+++ b/block/io.c
@@ -67,20 +67,31 @@ void bdrv_set_io_limits(BlockDriverState *bs,
void bdrv_no_throttling_begin(BlockDriverState *bs)
{
- if (bs->io_limits_disabled++ == 0) {
- throttle_group_restart_bs(bs);
+ if (!bs->blk) {
+ return;
+ }
+
+ if (blk_get_public(bs->blk)->io_limits_disabled++ == 0) {
+ throttle_group_restart_blk(bs->blk);
}
}
void bdrv_no_throttling_end(BlockDriverState *bs)
{
- assert(bs->io_limits_disabled);
- --bs->io_limits_disabled;
+ BlockBackendPublic *blkp;
+
+ if (!bs->blk) {
+ return;
+ }
+
+ blkp = blk_get_public(bs->blk);
+ assert(blkp->io_limits_disabled);
+ --blkp->io_limits_disabled;
}
void bdrv_io_limits_disable(BlockDriverState *bs)
{
- assert(bs->throttle_state);
+ assert(blk_get_public(bs->blk)->throttle_state);
bdrv_no_throttling_begin(bs);
throttle_group_unregister_blk(bs->blk);
bdrv_no_throttling_end(bs);
@@ -89,14 +100,16 @@ void bdrv_io_limits_disable(BlockDriverState *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->throttle_state);
+ BlockBackendPublic *blkp = blk_get_public(bs->blk);
+
+ assert(!blkp->throttle_state);
throttle_group_register_blk(bs->blk, group);
}
void bdrv_io_limits_update_group(BlockDriverState *bs, const char *group)
{
/* this bs is not part of any group */
- if (!bs->throttle_state) {
+ if (!blk_get_public(bs->blk)->throttle_state) {
return;
}
@@ -208,14 +221,15 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs)
bool bdrv_requests_pending(BlockDriverState *bs)
{
BdrvChild *child;
+ BlockBackendPublic *blkp = bs->blk ? blk_get_public(bs->blk) : NULL;
if (!QLIST_EMPTY(&bs->tracked_requests)) {
return true;
}
- if (!qemu_co_queue_empty(&bs->throttled_reqs[0])) {
+ if (blkp && !qemu_co_queue_empty(&blkp->throttled_reqs[0])) {
return true;
}
- if (!qemu_co_queue_empty(&bs->throttled_reqs[1])) {
+ if (blkp && !qemu_co_queue_empty(&blkp->throttled_reqs[1])) {
return true;
}
@@ -994,7 +1008,7 @@ int coroutine_fn bdrv_co_do_preadv(BlockDriverState *bs,
}
/* throttling disk I/O */
- if (bs->throttle_state) {
+ if (bs->blk && blk_get_public(bs->blk)->throttle_state) {
throttle_group_co_io_limits_intercept(bs, bytes, false);
}
@@ -1344,7 +1358,7 @@ int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
}
/* throttling disk I/O */
- if (bs->throttle_state) {
+ if (bs->blk && blk_get_public(bs->blk)->throttle_state) {
throttle_group_co_io_limits_intercept(bs, bytes, true);
}
diff --git a/block/qapi.c b/block/qapi.c
index a3e514d..1e4bb8a 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -67,7 +67,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
info->backing_file_depth = bdrv_get_backing_file_depth(bs);
info->detect_zeroes = bs->detect_zeroes;
- if (bs->throttle_state) {
+ if (bs->blk && blk_get_public(bs->blk)->throttle_state) {
ThrottleConfig cfg;
throttle_group_get_config(bs, &cfg);
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index e50ccaa..5bd5c3b 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -30,7 +30,7 @@
#include "sysemu/qtest.h"
/* The ThrottleGroup structure (with its ThrottleState) is shared
- * among different BlockDriverState and it's independent from
+ * among different BlockBackends and it's independent from
* AioContext, so in order to use it from different threads it needs
* its own locking.
*
@@ -40,7 +40,7 @@
* The whole ThrottleGroup structure is private and invisible to
* outside users, that only use it through its ThrottleState.
*
- * In addition to the ThrottleGroup structure, BlockDriverState has
+ * In addition to the ThrottleGroup structure, BlockBackend has
* fields that need to be accessed by other members of the group and
* therefore also need to be protected by this lock. Once a BDS is
* registered in a group those fields can be accessed by other threads
@@ -141,8 +141,8 @@ void throttle_group_unref(ThrottleState *ts)
*/
const char *throttle_group_get_name(BlockBackend *blk)
{
- ThrottleGroup *tg = container_of(blk_bs(blk)->throttle_state,
- ThrottleGroup, ts);
+ BlockBackendPublic *blkp = blk_get_public(blk);
+ ThrottleGroup *tg = container_of(blkp->throttle_state, ThrottleGroup, ts);
return tg->name;
}
@@ -156,10 +156,10 @@ const char *throttle_group_get_name(BlockBackend *blk)
*/
static BlockBackend *throttle_group_next_blk(BlockBackend *blk)
{
- BlockDriverState *bs = blk_bs(blk);
- ThrottleState *ts = bs->throttle_state;
+ BlockBackendPublic *blkp = blk_get_public(blk);
+ ThrottleState *ts = blkp->throttle_state;
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
- BlockBackendPublic *next = QLIST_NEXT(blk_get_public(blk), round_robin);
+ BlockBackendPublic *next = QLIST_NEXT(blkp, round_robin);
if (!next) {
next = QLIST_FIRST(&tg->head);
@@ -180,15 +180,15 @@ static BlockBackend *throttle_group_next_blk(BlockBackend *blk)
*/
static BlockBackend *next_throttle_token(BlockBackend *blk, bool is_write)
{
- ThrottleGroup *tg = container_of(blk_bs(blk)->throttle_state,
- ThrottleGroup, ts);
+ BlockBackendPublic *blkp = blk_get_public(blk);
+ ThrottleGroup *tg = container_of(blkp->throttle_state, ThrottleGroup, ts);
BlockBackend *token, *start;
start = token = tg->tokens[is_write];
/* get next bs round in round robin style */
token = throttle_group_next_blk(token);
- while (token != start && !blk_bs(token)->pending_reqs[is_write]) {
+ while (token != start && !blkp->pending_reqs[is_write]) {
token = throttle_group_next_blk(token);
}
@@ -196,7 +196,7 @@ static BlockBackend *next_throttle_token(BlockBackend *blk, bool is_write)
* then decide the token is the current bs because chances are
* the current bs get the current request queued.
*/
- if (token == start && !blk_bs(token)->pending_reqs[is_write]) {
+ if (token == start && !blkp->pending_reqs[is_write]) {
token = blk;
}
@@ -215,12 +215,13 @@ static BlockBackend *next_throttle_token(BlockBackend *blk, bool is_write)
*/
static bool throttle_group_schedule_timer(BlockBackend *blk, bool is_write)
{
- ThrottleState *ts = blk_bs(blk)->throttle_state;
- ThrottleTimers *tt = &blk_bs(blk)->throttle_timers;
+ BlockBackendPublic *blkp = blk_get_public(blk);
+ ThrottleState *ts = blkp->throttle_state;
+ ThrottleTimers *tt = &blkp->throttle_timers;
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
bool must_wait;
- if (blk_bs(blk)->io_limits_disabled) {
+ if (blkp->io_limits_disabled) {
return false;
}
@@ -249,14 +250,14 @@ static bool throttle_group_schedule_timer(BlockBackend *blk, bool is_write)
*/
static void schedule_next_request(BlockBackend *blk, bool is_write)
{
- BlockDriverState *bs = blk_bs(blk);
- ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
+ BlockBackendPublic *blkp = blk_get_public(blk);
+ ThrottleGroup *tg = container_of(blkp->throttle_state, ThrottleGroup, ts);
bool must_wait;
BlockBackend *token;
/* Check if there's any pending request to schedule next */
token = next_throttle_token(blk, is_write);
- if (!blk_bs(token)->pending_reqs[is_write]) {
+ if (!blkp->pending_reqs[is_write]) {
return;
}
@@ -265,12 +266,12 @@ static void schedule_next_request(BlockBackend *blk, bool is_write)
/* If it doesn't have to wait, queue it for immediate execution */
if (!must_wait) {
- /* Give preference to requests from the current bs */
+ /* Give preference to requests from the current blk */
if (qemu_in_coroutine() &&
- qemu_co_queue_next(&bs->throttled_reqs[is_write])) {
+ qemu_co_queue_next(&blkp->throttled_reqs[is_write])) {
token = blk;
} else {
- ThrottleTimers *tt = &blk_bs(token)->throttle_timers;
+ ThrottleTimers *tt = &blkp->throttle_timers;
int64_t now = qemu_clock_get_ns(tt->clock_type);
timer_mod(tt->timers[is_write], now + 1);
tg->any_timer_armed[is_write] = true;
@@ -294,37 +295,40 @@ void coroutine_fn throttle_group_co_io_limits_intercept(BlockDriverState *bs,
bool must_wait;
BlockBackend *token;
- ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
+ BlockBackend *blk = bs->blk;
+ BlockBackendPublic *blkp = blk_get_public(blk);
+ ThrottleGroup *tg = container_of(blkp->throttle_state, ThrottleGroup, ts);
qemu_mutex_lock(&tg->lock);
/* First we check if this I/O has to be throttled. */
- token = next_throttle_token(bs->blk, is_write);
+ token = next_throttle_token(blk, is_write);
must_wait = throttle_group_schedule_timer(token, is_write);
/* Wait if there's a timer set or queued requests of this type */
- if (must_wait || bs->pending_reqs[is_write]) {
- bs->pending_reqs[is_write]++;
+ if (must_wait || blkp->pending_reqs[is_write]) {
+ blkp->pending_reqs[is_write]++;
qemu_mutex_unlock(&tg->lock);
- qemu_co_queue_wait(&bs->throttled_reqs[is_write]);
+ qemu_co_queue_wait(&blkp->throttled_reqs[is_write]);
qemu_mutex_lock(&tg->lock);
- bs->pending_reqs[is_write]--;
+ blkp->pending_reqs[is_write]--;
}
/* The I/O will be executed, so do the accounting */
- throttle_account(bs->throttle_state, is_write, bytes);
+ throttle_account(blkp->throttle_state, is_write, bytes);
/* Schedule the next request */
- schedule_next_request(bs->blk, is_write);
+ schedule_next_request(blk, is_write);
qemu_mutex_unlock(&tg->lock);
}
-void throttle_group_restart_bs(BlockDriverState *bs)
+void throttle_group_restart_blk(BlockBackend *blk)
{
+ BlockBackendPublic *blkp = blk_get_public(blk);
int i;
for (i = 0; i < 2; i++) {
- while (qemu_co_enter_next(&bs->throttled_reqs[i])) {
+ while (qemu_co_enter_next(&blkp->throttled_reqs[i])) {
;
}
}
@@ -339,8 +343,9 @@ void throttle_group_restart_bs(BlockDriverState *bs)
*/
void throttle_group_config(BlockDriverState *bs, ThrottleConfig *cfg)
{
- ThrottleTimers *tt = &bs->throttle_timers;
- ThrottleState *ts = bs->throttle_state;
+ BlockBackendPublic *blkp = blk_get_public(bs->blk);
+ ThrottleTimers *tt = &blkp->throttle_timers;
+ ThrottleState *ts = blkp->throttle_state;
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
qemu_mutex_lock(&tg->lock);
/* throttle_config() cancels the timers */
@@ -353,8 +358,8 @@ void throttle_group_config(BlockDriverState *bs, ThrottleConfig *cfg)
throttle_config(ts, tt, cfg);
qemu_mutex_unlock(&tg->lock);
- qemu_co_enter_next(&bs->throttled_reqs[0]);
- qemu_co_enter_next(&bs->throttled_reqs[1]);
+ qemu_co_enter_next(&blkp->throttled_reqs[0]);
+ qemu_co_enter_next(&blkp->throttled_reqs[1]);
}
/* Get the throttle configuration from a particular group. Similar to
@@ -366,7 +371,8 @@ void throttle_group_config(BlockDriverState *bs, ThrottleConfig *cfg)
*/
void throttle_group_get_config(BlockDriverState *bs, ThrottleConfig *cfg)
{
- ThrottleState *ts = bs->throttle_state;
+ BlockBackendPublic *blkp = blk_get_public(bs->blk);
+ ThrottleState *ts = blkp->throttle_state;
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
qemu_mutex_lock(&tg->lock);
throttle_get_config(ts, cfg);
@@ -376,12 +382,13 @@ void throttle_group_get_config(BlockDriverState *bs, ThrottleConfig *cfg)
/* ThrottleTimers callback. This wakes up a request that was waiting
* because it had been throttled.
*
- * @bs: the BlockDriverState whose request had been throttled
+ * @blk: the BlockBackend whose request had been throttled
* @is_write: the type of operation (read/write)
*/
-static void timer_cb(BlockDriverState *bs, bool is_write)
+static void timer_cb(BlockBackend *blk, bool is_write)
{
- ThrottleState *ts = bs->throttle_state;
+ BlockBackendPublic *blkp = blk_get_public(blk);
+ ThrottleState *ts = blkp->throttle_state;
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
bool empty_queue;
@@ -391,13 +398,13 @@ static void timer_cb(BlockDriverState *bs, bool is_write)
qemu_mutex_unlock(&tg->lock);
/* Run the request that was waiting for this timer */
- empty_queue = !qemu_co_enter_next(&bs->throttled_reqs[is_write]);
+ empty_queue = !qemu_co_enter_next(&blkp->throttled_reqs[is_write]);
/* If the request queue was empty then we have to take care of
* scheduling the next one */
if (empty_queue) {
qemu_mutex_lock(&tg->lock);
- schedule_next_request(bs->blk, is_write);
+ schedule_next_request(blk, is_write);
qemu_mutex_unlock(&tg->lock);
}
}
@@ -422,7 +429,7 @@ static void write_timer_cb(void *opaque)
void throttle_group_register_blk(BlockBackend *blk, const char *groupname)
{
int i;
- BlockDriverState *bs = blk_bs(blk);
+ BlockBackendPublic *blkp = blk_get_public(blk);
ThrottleState *ts = throttle_group_incref(groupname);
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
int clock_type = QEMU_CLOCK_REALTIME;
@@ -432,7 +439,7 @@ void throttle_group_register_blk(BlockBackend *blk, const char *groupname)
clock_type = QEMU_CLOCK_VIRTUAL;
}
- bs->throttle_state = ts;
+ blkp->throttle_state = ts;
qemu_mutex_lock(&tg->lock);
/* If the ThrottleGroup is new set this BlockBackend as the token */
@@ -442,14 +449,14 @@ void throttle_group_register_blk(BlockBackend *blk, const char *groupname)
}
}
- QLIST_INSERT_HEAD(&tg->head, blk_get_public(blk), round_robin);
+ QLIST_INSERT_HEAD(&tg->head, blkp, round_robin);
- throttle_timers_init(&bs->throttle_timers,
- bdrv_get_aio_context(bs),
+ throttle_timers_init(&blkp->throttle_timers,
+ blk_get_aio_context(blk),
clock_type,
read_timer_cb,
write_timer_cb,
- bs);
+ blk);
qemu_mutex_unlock(&tg->lock);
}
@@ -466,19 +473,19 @@ void throttle_group_register_blk(BlockBackend *blk, const char *groupname)
*/
void throttle_group_unregister_blk(BlockBackend *blk)
{
- BlockDriverState *bs = blk_bs(blk);
- ThrottleGroup *tg = container_of(bs->throttle_state, ThrottleGroup, ts);
+ BlockBackendPublic *blkp = blk_get_public(blk);
+ ThrottleGroup *tg = container_of(blkp->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]));
+ assert(blkp->pending_reqs[0] == 0 && blkp->pending_reqs[1] == 0);
+ assert(qemu_co_queue_empty(&blkp->throttled_reqs[0]));
+ assert(qemu_co_queue_empty(&blkp->throttled_reqs[1]));
qemu_mutex_lock(&tg->lock);
for (i = 0; i < 2; i++) {
if (tg->tokens[i] == blk) {
BlockBackend *token = throttle_group_next_blk(blk);
- /* Take care of the case where this is the last bs in the group */
+ /* Take care of the case where this is the last blk in the group */
if (token == blk) {
token = NULL;
}
@@ -486,13 +493,13 @@ void throttle_group_unregister_blk(BlockBackend *blk)
}
}
- /* remove the current bs from the list */
- QLIST_REMOVE(blk_get_public(blk), round_robin);
- throttle_timers_destroy(&bs->throttle_timers);
+ /* remove the current blk from the list */
+ QLIST_REMOVE(blkp, round_robin);
+ throttle_timers_destroy(&blkp->throttle_timers);
qemu_mutex_unlock(&tg->lock);
throttle_group_unref(&tg->ts);
- bs->throttle_state = NULL;
+ blkp->throttle_state = NULL;
}
static void throttle_groups_init(void)
diff --git a/blockdev.c b/blockdev.c
index 29ded1b..0776edf 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2726,14 +2726,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->throttle_state) {
+ if (!blk_get_public(bs->blk)->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->throttle_state) {
+ } else if (blk_get_public(bs->blk)->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 639165e..8126832 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -26,7 +26,6 @@
#include "block/accounting.h"
#include "block/block.h"
-#include "block/throttle-groups.h"
#include "qemu/option.h"
#include "qemu/queue.h"
#include "qemu/coroutine.h"
@@ -421,18 +420,6 @@ struct BlockDriverState {
/* number of in-flight serialising requests */
unsigned int serialising_in_flight;
- /* I/O throttling.
- * throttle_state tells us if this BDS has I/O limits configured.
- * io_limits_disabled tells us if they are currently being enforced */
- CoQueue throttled_reqs[2];
- unsigned int io_limits_disabled;
-
- /* The following fields are protected by the ThrottleGroup lock.
- * See the ThrottleGroup documentation for details. */
- ThrottleState *throttle_state;
- ThrottleTimers throttle_timers;
- unsigned pending_reqs[2];
-
/* Offset after the highest byte written to */
uint64_t wr_highest_offset;
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index bd55a34..840ba44 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -38,7 +38,7 @@ void throttle_group_get_config(BlockDriverState *bs, ThrottleConfig *cfg);
void throttle_group_register_blk(BlockBackend *blk, const char *groupname);
void throttle_group_unregister_blk(BlockBackend *blk);
-void throttle_group_restart_bs(BlockDriverState *bs);
+void throttle_group_restart_blk(BlockBackend *blk);
void coroutine_fn throttle_group_co_io_limits_intercept(BlockDriverState *bs,
unsigned int bytes,
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index bcd62b0..dd3bb3d 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -14,6 +14,7 @@
#define BLOCK_BACKEND_H
#include "qemu/iov.h"
+#include "block/throttle-groups.h"
/*
* TODO Have to include block/block.h for a bunch of block layer
@@ -63,9 +64,17 @@ typedef struct BlockDevOps {
* fields that must be public. This is in particular for QLIST_ENTRY() and
* friends so that BlockBackends can be kept in lists outside block-backend.c */
typedef struct BlockBackendPublic {
- /* I/O throttling */
+ /* I/O throttling.
+ * throttle_state tells us if this BDS has I/O limits configured.
+ * io_limits_disabled tells us if they are currently being enforced */
+ CoQueue throttled_reqs[2];
+ unsigned int io_limits_disabled;
+
/* The following fields are protected by the ThrottleGroup lock.
* See the ThrottleGroup documentation for details. */
+ ThrottleState *throttle_state;
+ ThrottleTimers throttle_timers;
+ unsigned pending_reqs[2];
QLIST_ENTRY(BlockBackendPublic) round_robin;
} BlockBackendPublic;
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index 1a322f1..a020068 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -576,31 +576,35 @@ static void test_groups(void)
{
ThrottleConfig cfg1, cfg2;
BlockBackend *blk1, *blk2, *blk3;
- BlockDriverState *bdrv1, *bdrv2, *bdrv3;
+ BlockBackendPublic *blkp1, *blkp2, *blkp3;
+ BlockDriverState *bdrv1, *bdrv3;
blk1 = blk_new_with_bs(&error_abort);
blk2 = blk_new_with_bs(&error_abort);
blk3 = blk_new_with_bs(&error_abort);
bdrv1 = blk_bs(blk1);
- bdrv2 = blk_bs(blk2);
bdrv3 = blk_bs(blk3);
- g_assert(bdrv1->throttle_state == NULL);
- g_assert(bdrv2->throttle_state == NULL);
- g_assert(bdrv3->throttle_state == NULL);
+ blkp1 = blk_get_public(blk1);
+ blkp2 = blk_get_public(blk2);
+ blkp3 = blk_get_public(blk3);
+
+ g_assert(blkp1->throttle_state == NULL);
+ g_assert(blkp2->throttle_state == NULL);
+ g_assert(blkp3->throttle_state == NULL);
throttle_group_register_blk(blk1, "bar");
throttle_group_register_blk(blk2, "foo");
throttle_group_register_blk(blk3, "bar");
- g_assert(bdrv1->throttle_state != NULL);
- g_assert(bdrv2->throttle_state != NULL);
- g_assert(bdrv3->throttle_state != NULL);
+ g_assert(blkp1->throttle_state != NULL);
+ g_assert(blkp2->throttle_state != NULL);
+ g_assert(blkp3->throttle_state != NULL);
g_assert(!strcmp(throttle_group_get_name(blk1), "bar"));
g_assert(!strcmp(throttle_group_get_name(blk2), "foo"));
- g_assert(bdrv1->throttle_state == bdrv3->throttle_state);
+ g_assert(blkp1->throttle_state == blkp3->throttle_state);
/* Setting the config of a group member affects the whole group */
throttle_config_init(&cfg1);
@@ -628,9 +632,9 @@ static void test_groups(void)
throttle_group_unregister_blk(blk2);
throttle_group_unregister_blk(blk3);
- g_assert(bdrv1->throttle_state == NULL);
- g_assert(bdrv2->throttle_state == NULL);
- g_assert(bdrv3->throttle_state == NULL);
+ g_assert(blkp1->throttle_state == NULL);
+ g_assert(blkp2->throttle_state == NULL);
+ g_assert(blkp3->throttle_state == NULL);
}
int main(int argc, char **argv)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 05/13] block: Move throttling fields from BDS to BB
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 05/13] block: Move throttling fields from BDS to BB Kevin Wolf
@ 2016-05-04 13:36 ` Alberto Garcia
2016-05-09 12:43 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2016-05-10 12:36 ` [Qemu-devel] " Alberto Garcia
2 siblings, 0 replies; 39+ messages in thread
From: Alberto Garcia @ 2016-05-04 13:36 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: pbonzini, mreitz, eblake, stefanha, qemu-devel
On Fri 22 Apr 2016 07:42:34 PM CEST, Kevin Wolf wrote:
> This patch changes where the throttling state is stored (used to be the
> BlockDriverState, now it is the BlockBackend), but it doesn't actually
> make it a BB level feature yet. For example, throttling is still
> disabled when the BDS is detached from the BB.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 05/13] block: Move throttling fields from BDS to BB
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 05/13] block: Move throttling fields from BDS to BB Kevin Wolf
2016-05-04 13:36 ` Alberto Garcia
@ 2016-05-09 12:43 ` Stefan Hajnoczi
2016-05-11 12:45 ` Kevin Wolf
2016-05-10 12:36 ` [Qemu-devel] " Alberto Garcia
2 siblings, 1 reply; 39+ messages in thread
From: Stefan Hajnoczi @ 2016-05-09 12:43 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, mreitz, stefanha, pbonzini
[-- Attachment #1: Type: text/plain, Size: 395 bytes --]
On Fri, Apr 22, 2016 at 07:42:34PM +0200, Kevin Wolf wrote:
> @@ -1527,7 +1525,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
> return -ENODEV;
> }
>
> - if (bs->throttle_state) {
> + if (blk_get_public(bs->blk)->throttle_state) {
The reference argument can be any nodename. How can you be sure bs->blk
!= NULL?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [Qemu-block] [PATCH v2 05/13] block: Move throttling fields from BDS to BB
2016-05-09 12:43 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
@ 2016-05-11 12:45 ` Kevin Wolf
0 siblings, 0 replies; 39+ messages in thread
From: Kevin Wolf @ 2016-05-11 12:45 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-block, qemu-devel, mreitz, stefanha, pbonzini
[-- Attachment #1: Type: text/plain, Size: 600 bytes --]
Am 09.05.2016 um 14:43 hat Stefan Hajnoczi geschrieben:
> On Fri, Apr 22, 2016 at 07:42:34PM +0200, Kevin Wolf wrote:
> > @@ -1527,7 +1525,7 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
> > return -ENODEV;
> > }
> >
> > - if (bs->throttle_state) {
> > + if (blk_get_public(bs->blk)->throttle_state) {
>
> The reference argument can be any nodename. How can you be sure bs->blk
> != NULL?
Good catch, this should check bs->blk != NULL first. This check goes
away later in series, but I'll fix it anyway.
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v2 05/13] block: Move throttling fields from BDS to BB
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 05/13] block: Move throttling fields from BDS to BB Kevin Wolf
2016-05-04 13:36 ` Alberto Garcia
2016-05-09 12:43 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
@ 2016-05-10 12:36 ` Alberto Garcia
2 siblings, 0 replies; 39+ messages in thread
From: Alberto Garcia @ 2016-05-10 12:36 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: pbonzini, mreitz, eblake, stefanha, qemu-devel
On Fri 22 Apr 2016 07:42:34 PM CEST, Kevin Wolf wrote:
> typedef struct BlockBackendPublic {
> - /* I/O throttling */
> + /* I/O throttling.
> + * throttle_state tells us if this BDS has I/O limits configured.
> + * io_limits_disabled tells us if they are currently being enforced */
I overlooked this earlier... this is now in BlockBackend but the comment
still refers to a BDS. There's also a couple more cases of this in
throttle-groups.c.
Berto
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 06/13] block: Move actual I/O throttling to BlockBackend
2016-04-22 17:42 [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
` (4 preceding siblings ...)
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 05/13] block: Move throttling fields from BDS to BB Kevin Wolf
@ 2016-04-22 17:42 ` Kevin Wolf
2016-05-04 13:05 ` Alberto Garcia
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 07/13] block: Move I/O throttling configuration functions " Kevin Wolf
` (8 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2016-04-22 17:42 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, berto, mreitz, eblake, stefanha, qemu-devel
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/block-backend.c | 10 ++++++++++
block/io.c | 10 ----------
block/throttle-groups.c | 5 ++---
include/block/throttle-groups.h | 2 +-
4 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index 087e60a..b1a176e 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -716,6 +716,11 @@ static int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
return ret;
}
+ /* throttling disk I/O */
+ if (blk->public.throttle_state) {
+ throttle_group_co_io_limits_intercept(blk, bytes, false);
+ }
+
return bdrv_co_do_preadv(blk_bs(blk), offset, bytes, qiov, flags);
}
@@ -730,6 +735,11 @@ static int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
return ret;
}
+ /* throttling disk I/O */
+ if (blk->public.throttle_state) {
+ throttle_group_co_io_limits_intercept(blk, bytes, true);
+ }
+
if (!blk->enable_write_cache) {
flags |= BDRV_REQ_FUA;
}
diff --git a/block/io.c b/block/io.c
index 34d1bf6..ccc6a76 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1007,11 +1007,6 @@ int coroutine_fn bdrv_co_do_preadv(BlockDriverState *bs,
flags |= BDRV_REQ_COPY_ON_READ;
}
- /* throttling disk I/O */
- if (bs->blk && blk_get_public(bs->blk)->throttle_state) {
- throttle_group_co_io_limits_intercept(bs, bytes, false);
- }
-
/* Align read if necessary by padding qiov */
if (offset & (align - 1)) {
head_buf = qemu_blockalign(bs, align);
@@ -1357,11 +1352,6 @@ int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
return ret;
}
- /* throttling disk I/O */
- if (bs->blk && blk_get_public(bs->blk)->throttle_state) {
- throttle_group_co_io_limits_intercept(bs, bytes, true);
- }
-
/*
* Align write if necessary by performing a read-modify-write cycle.
* Pad qiov with the read parts and be sure to have a tracked request not
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 5bd5c3b..6e6939e 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -284,18 +284,17 @@ static void schedule_next_request(BlockBackend *blk, bool is_write)
* if necessary, and schedule the next request using a round robin
* algorithm.
*
- * @bs: the current BlockDriverState
+ * @blk: the current BlockBackend
* @bytes: the number of bytes for this I/O
* @is_write: the type of operation (read/write)
*/
-void coroutine_fn throttle_group_co_io_limits_intercept(BlockDriverState *bs,
+void coroutine_fn throttle_group_co_io_limits_intercept(BlockBackend *blk,
unsigned int bytes,
bool is_write)
{
bool must_wait;
BlockBackend *token;
- BlockBackend *blk = bs->blk;
BlockBackendPublic *blkp = blk_get_public(blk);
ThrottleGroup *tg = container_of(blkp->throttle_state, ThrottleGroup, ts);
qemu_mutex_lock(&tg->lock);
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index 840ba44..ac42248 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -40,7 +40,7 @@ void throttle_group_register_blk(BlockBackend *blk, const char *groupname);
void throttle_group_unregister_blk(BlockBackend *blk);
void throttle_group_restart_blk(BlockBackend *blk);
-void coroutine_fn throttle_group_co_io_limits_intercept(BlockDriverState *bs,
+void coroutine_fn throttle_group_co_io_limits_intercept(BlockBackend *blk,
unsigned int bytes,
bool is_write);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v2 07/13] block: Move I/O throttling configuration functions to BlockBackend
2016-04-22 17:42 [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
` (5 preceding siblings ...)
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 06/13] block: Move actual I/O throttling to BlockBackend Kevin Wolf
@ 2016-04-22 17:42 ` Kevin Wolf
2016-05-04 14:11 ` Alberto Garcia
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 08/13] block: Introduce BdrvChild.opaque Kevin Wolf
` (7 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2016-04-22 17:42 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, berto, mreitz, eblake, stefanha, qemu-devel
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 2 +-
block/block-backend.c | 43 +++++++++++++++++++++++++++++++++++++++--
block/io.c | 41 ---------------------------------------
block/qapi.c | 2 +-
block/throttle-groups.c | 12 ++++++------
blockdev.c | 16 +++++++--------
include/block/block.h | 4 ----
include/block/block_int.h | 3 +--
include/block/throttle-groups.h | 4 ++--
include/sysemu/block-backend.h | 5 +++++
tests/test-throttle.c | 16 ++++++---------
11 files changed, 71 insertions(+), 77 deletions(-)
diff --git a/block.c b/block.c
index d81fe2e..85526ad 100644
--- a/block.c
+++ b/block.c
@@ -2125,7 +2125,7 @@ static void bdrv_close(BlockDriverState *bs)
/* Disable I/O limits and drain all pending throttled requests */
if (bs->blk && blk_get_public(bs->blk)->throttle_state) {
- bdrv_io_limits_disable(bs);
+ blk_io_limits_disable(bs->blk);
}
bdrv_drained_begin(bs); /* complete I/O */
diff --git a/block/block-backend.c b/block/block-backend.c
index b1a176e..7d49d99 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -442,7 +442,7 @@ void blk_remove_bs(BlockBackend *blk)
blk_update_root_state(blk);
if (blk->public.throttle_state) {
- bdrv_io_limits_disable(blk->root->bs);
+ blk_io_limits_disable(blk);
}
blk->root->bs->blk = NULL;
@@ -1600,7 +1600,7 @@ void blk_apply_root_state(BlockBackend *blk, BlockDriverState *bs)
{
bs->detect_zeroes = blk->root_state.detect_zeroes;
if (blk->root_state.throttle_group) {
- bdrv_io_limits_enable(bs, blk->root_state.throttle_group);
+ blk_io_limits_enable(blk, blk->root_state.throttle_group);
}
}
@@ -1664,3 +1664,42 @@ int blk_flush_all(void)
return result;
}
+
+
+/* throttling disk I/O limits */
+void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg)
+{
+ throttle_group_config(blk, cfg);
+}
+
+void blk_io_limits_disable(BlockBackend *blk)
+{
+ assert(blk->public.throttle_state);
+ bdrv_no_throttling_begin(blk_bs(blk));
+ throttle_group_unregister_blk(blk);
+ bdrv_no_throttling_end(blk_bs(blk));
+}
+
+/* should be called before bdrv_set_io_limits if a limit is set */
+void blk_io_limits_enable(BlockBackend *blk, const char *group)
+{
+ assert(!blk->public.throttle_state);
+ throttle_group_register_blk(blk, group);
+}
+
+void blk_io_limits_update_group(BlockBackend *blk, const char *group)
+{
+ /* this BB is not part of any group */
+ if (!blk->public.throttle_state) {
+ return;
+ }
+
+ /* this BB is a part of the same group than the one we want */
+ if (!g_strcmp0(throttle_group_get_name(blk), group)) {
+ return;
+ }
+
+ /* need to change the group this bs belong to */
+ blk_io_limits_disable(blk);
+ blk_io_limits_enable(blk, group);
+}
diff --git a/block/io.c b/block/io.c
index ccc6a76..b892fce 100644
--- a/block/io.c
+++ b/block/io.c
@@ -58,13 +58,6 @@ 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);
-/* throttling disk I/O limits */
-void bdrv_set_io_limits(BlockDriverState *bs,
- ThrottleConfig *cfg)
-{
- throttle_group_config(bs, cfg);
-}
-
void bdrv_no_throttling_begin(BlockDriverState *bs)
{
if (!bs->blk) {
@@ -89,40 +82,6 @@ void bdrv_no_throttling_end(BlockDriverState *bs)
--blkp->io_limits_disabled;
}
-void bdrv_io_limits_disable(BlockDriverState *bs)
-{
- assert(blk_get_public(bs->blk)->throttle_state);
- bdrv_no_throttling_begin(bs);
- throttle_group_unregister_blk(bs->blk);
- 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)
-{
- BlockBackendPublic *blkp = blk_get_public(bs->blk);
-
- assert(!blkp->throttle_state);
- throttle_group_register_blk(bs->blk, group);
-}
-
-void bdrv_io_limits_update_group(BlockDriverState *bs, const char *group)
-{
- /* this bs is not part of any group */
- if (!blk_get_public(bs->blk)->throttle_state) {
- return;
- }
-
- /* this bs is a part of the same group than the one we want */
- if (!g_strcmp0(throttle_group_get_name(bs->blk), group)) {
- return;
- }
-
- /* need to change the group this bs belong to */
- bdrv_io_limits_disable(bs);
- bdrv_io_limits_enable(bs, group);
-}
-
void bdrv_setup_io_funcs(BlockDriver *bdrv)
{
/* Block drivers without coroutine functions need emulation */
diff --git a/block/qapi.c b/block/qapi.c
index 1e4bb8a..b0d8d6b 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -70,7 +70,7 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
if (bs->blk && blk_get_public(bs->blk)->throttle_state) {
ThrottleConfig cfg;
- throttle_group_get_config(bs, &cfg);
+ throttle_group_get_config(bs->blk, &cfg);
info->bps = cfg.buckets[THROTTLE_BPS_TOTAL].avg;
info->bps_rd = cfg.buckets[THROTTLE_BPS_READ].avg;
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 6e6939e..44f6e17 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -337,12 +337,12 @@ void throttle_group_restart_blk(BlockBackend *blk)
* to throttle_config(), but guarantees atomicity within the
* throttling group.
*
- * @bs: a BlockDriverState that is member of the group
+ * @blk: a BlockBackend that is a member of the group
* @cfg: the configuration to set
*/
-void throttle_group_config(BlockDriverState *bs, ThrottleConfig *cfg)
+void throttle_group_config(BlockBackend *blk, ThrottleConfig *cfg)
{
- BlockBackendPublic *blkp = blk_get_public(bs->blk);
+ BlockBackendPublic *blkp = blk_get_public(blk);
ThrottleTimers *tt = &blkp->throttle_timers;
ThrottleState *ts = blkp->throttle_state;
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
@@ -365,12 +365,12 @@ void throttle_group_config(BlockDriverState *bs, ThrottleConfig *cfg)
* throttle_get_config(), but guarantees atomicity within the
* throttling group.
*
- * @bs: a BlockDriverState that is member of the group
+ * @blk: a BlockBackend that is a member of the group
* @cfg: the configuration will be written here
*/
-void throttle_group_get_config(BlockDriverState *bs, ThrottleConfig *cfg)
+void throttle_group_get_config(BlockBackend *blk, ThrottleConfig *cfg)
{
- BlockBackendPublic *blkp = blk_get_public(bs->blk);
+ BlockBackendPublic *blkp = blk_get_public(blk);
ThrottleState *ts = blkp->throttle_state;
ThrottleGroup *tg = container_of(ts, ThrottleGroup, ts);
qemu_mutex_lock(&tg->lock);
diff --git a/blockdev.c b/blockdev.c
index 0776edf..79dd71b 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -616,8 +616,8 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
if (!throttling_group) {
throttling_group = blk_name(blk);
}
- bdrv_io_limits_enable(bs, throttling_group);
- bdrv_set_io_limits(bs, &cfg);
+ blk_io_limits_enable(blk, throttling_group);
+ blk_set_io_limits(blk, &cfg);
}
if (bdrv_key_required(bs)) {
@@ -2726,16 +2726,16 @@ 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 (!blk_get_public(bs->blk)->throttle_state) {
- bdrv_io_limits_enable(bs, has_group ? group : device);
+ if (!blk_get_public(blk)->throttle_state) {
+ blk_io_limits_enable(blk, has_group ? group : device);
} else if (has_group) {
- bdrv_io_limits_update_group(bs, group);
+ blk_io_limits_update_group(blk, group);
}
/* Set the new throttling configuration */
- bdrv_set_io_limits(bs, &cfg);
- } else if (blk_get_public(bs->blk)->throttle_state) {
+ blk_set_io_limits(blk, &cfg);
+ } else if (blk_get_public(blk)->throttle_state) {
/* If all throttling settings are set to 0, disable I/O limits */
- bdrv_io_limits_disable(bs);
+ blk_io_limits_disable(blk);
}
out:
diff --git a/include/block/block.h b/include/block/block.h
index 0e8b4d1..4b04550 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -187,10 +187,6 @@ void bdrv_stats_print(Monitor *mon, const QObject *data);
void bdrv_info_stats(Monitor *mon, QObject **ret_data);
/* disk I/O throttling */
-void bdrv_io_limits_enable(BlockDriverState *bs, const char *group);
-void bdrv_io_limits_disable(BlockDriverState *bs);
-void bdrv_io_limits_update_group(BlockDriverState *bs, const char *group);
-
void bdrv_init(void);
void bdrv_init_with_whitelist(void);
bool bdrv_uses_whitelist(void);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8126832..8f37279 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -517,8 +517,7 @@ int get_tmp_filename(char *filename, int size);
BlockDriver *bdrv_probe_all(const uint8_t *buf, int buf_size,
const char *filename);
-void bdrv_set_io_limits(BlockDriverState *bs,
- ThrottleConfig *cfg);
+bool bdrv_start_throttled_reqs(BlockDriverState *bs);
/**
diff --git a/include/block/throttle-groups.h b/include/block/throttle-groups.h
index ac42248..d983d34 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -33,8 +33,8 @@ const char *throttle_group_get_name(BlockBackend *blk);
ThrottleState *throttle_group_incref(const char *name);
void throttle_group_unref(ThrottleState *ts);
-void throttle_group_config(BlockDriverState *bs, ThrottleConfig *cfg);
-void throttle_group_get_config(BlockDriverState *bs, ThrottleConfig *cfg);
+void throttle_group_config(BlockBackend *blk, ThrottleConfig *cfg);
+void throttle_group_get_config(BlockBackend *blk, ThrottleConfig *cfg);
void throttle_group_register_blk(BlockBackend *blk, const char *groupname);
void throttle_group_unregister_blk(BlockBackend *blk);
diff --git a/include/sysemu/block-backend.h b/include/sysemu/block-backend.h
index dd3bb3d..b79608e 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -215,4 +215,9 @@ BlockAIOCB *blk_abort_aio_request(BlockBackend *blk,
BlockCompletionFunc *cb,
void *opaque, int ret);
+void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg);
+void blk_io_limits_disable(BlockBackend *blk);
+void blk_io_limits_enable(BlockBackend *blk, const char *group);
+void blk_io_limits_update_group(BlockBackend *blk, const char *group);
+
#endif
diff --git a/tests/test-throttle.c b/tests/test-throttle.c
index a020068..5ec966c 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -577,15 +577,11 @@ static void test_groups(void)
ThrottleConfig cfg1, cfg2;
BlockBackend *blk1, *blk2, *blk3;
BlockBackendPublic *blkp1, *blkp2, *blkp3;
- BlockDriverState *bdrv1, *bdrv3;
blk1 = blk_new_with_bs(&error_abort);
blk2 = blk_new_with_bs(&error_abort);
blk3 = blk_new_with_bs(&error_abort);
- bdrv1 = blk_bs(blk1);
- bdrv3 = blk_bs(blk3);
-
blkp1 = blk_get_public(blk1);
blkp2 = blk_get_public(blk2);
blkp3 = blk_get_public(blk3);
@@ -612,20 +608,20 @@ static void test_groups(void)
cfg1.buckets[THROTTLE_BPS_WRITE].avg = 285000;
cfg1.buckets[THROTTLE_OPS_READ].avg = 20000;
cfg1.buckets[THROTTLE_OPS_WRITE].avg = 12000;
- throttle_group_config(bdrv1, &cfg1);
+ throttle_group_config(blk1, &cfg1);
- throttle_group_get_config(bdrv1, &cfg1);
- throttle_group_get_config(bdrv3, &cfg2);
+ throttle_group_get_config(blk1, &cfg1);
+ throttle_group_get_config(blk3, &cfg2);
g_assert(!memcmp(&cfg1, &cfg2, sizeof(cfg1)));
cfg2.buckets[THROTTLE_BPS_READ].avg = 4547;
cfg2.buckets[THROTTLE_BPS_WRITE].avg = 1349;
cfg2.buckets[THROTTLE_OPS_READ].avg = 123;
cfg2.buckets[THROTTLE_OPS_WRITE].avg = 86;
- throttle_group_config(bdrv3, &cfg1);
+ throttle_group_config(blk3, &cfg1);
- throttle_group_get_config(bdrv1, &cfg1);
- throttle_group_get_config(bdrv3, &cfg2);
+ throttle_group_get_config(blk1, &cfg1);
+ throttle_group_get_config(blk3, &cfg2);
g_assert(!memcmp(&cfg1, &cfg2, sizeof(cfg1)));
throttle_group_unregister_blk(blk1);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 07/13] block: Move I/O throttling configuration functions to BlockBackend
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 07/13] block: Move I/O throttling configuration functions " Kevin Wolf
@ 2016-05-04 14:11 ` Alberto Garcia
0 siblings, 0 replies; 39+ messages in thread
From: Alberto Garcia @ 2016-05-04 14:11 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: pbonzini, mreitz, eblake, stefanha, qemu-devel
On Fri 22 Apr 2016 07:42:36 PM CEST, Kevin Wolf wrote:
> +void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg)
> +{
> + throttle_group_config(blk, cfg);
> +}
> +
> +void blk_io_limits_disable(BlockBackend *blk)
> +{
> + assert(blk->public.throttle_state);
> + bdrv_no_throttling_begin(blk_bs(blk));
> + throttle_group_unregister_blk(blk);
> + bdrv_no_throttling_end(blk_bs(blk));
> +}
> +
> +/* should be called before bdrv_set_io_limits if a limit is set */
> +void blk_io_limits_enable(BlockBackend *blk, const char *group)
s/bdrv_set_io_limits/blk_set_io_limits/ in the comment above.
Otherwise the patch looks good to me.
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 08/13] block: Introduce BdrvChild.opaque
2016-04-22 17:42 [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
` (6 preceding siblings ...)
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 07/13] block: Move I/O throttling configuration functions " Kevin Wolf
@ 2016-04-22 17:42 ` Kevin Wolf
2016-05-06 12:54 ` Alberto Garcia
2016-05-06 15:13 ` Alberto Garcia
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 09/13] block: Drain throttling queue with BdrvChild callback Kevin Wolf
` (6 subsequent siblings)
14 siblings, 2 replies; 39+ messages in thread
From: Kevin Wolf @ 2016-04-22 17:42 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, berto, mreitz, eblake, stefanha, qemu-devel
BlockBackends use it to get a back pointer from BdrvChild to
BlockBackend in any BdrvChildRole callbacks.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/block-backend.c | 2 ++
include/block/block_int.h | 1 +
2 files changed, 3 insertions(+)
diff --git a/block/block-backend.c b/block/block-backend.c
index 7d49d99..b72a73d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -133,6 +133,7 @@ BlockBackend *blk_new_with_bs(Error **errp)
bs = bdrv_new_root();
blk->root = bdrv_root_attach_child(bs, "root", &child_root);
+ blk->root->opaque = blk;
bs->blk = blk;
return blk;
}
@@ -458,6 +459,7 @@ void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
assert(!blk->root && !bs->blk);
bdrv_ref(bs);
blk->root = bdrv_root_attach_child(bs, "root", &child_root);
+ blk->root->opaque = blk;
bs->blk = blk;
notifier_list_notify(&blk->insert_bs_notifiers, blk);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 8f37279..ddadb7f 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -370,6 +370,7 @@ struct BdrvChild {
BlockDriverState *bs;
char *name;
const BdrvChildRole *role;
+ void *opaque;
QLIST_ENTRY(BdrvChild) next;
QLIST_ENTRY(BdrvChild) next_parent;
};
--
1.8.3.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 08/13] block: Introduce BdrvChild.opaque
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 08/13] block: Introduce BdrvChild.opaque Kevin Wolf
@ 2016-05-06 12:54 ` Alberto Garcia
2016-05-06 13:19 ` Kevin Wolf
2016-05-06 15:13 ` Alberto Garcia
1 sibling, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2016-05-06 12:54 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: pbonzini, mreitz, eblake, stefanha, qemu-devel
On Fri 22 Apr 2016 07:42:37 PM CEST, Kevin Wolf wrote:
> BlockBackends use it to get a back pointer from BdrvChild to
> BlockBackend in any BdrvChildRole callbacks.
Hmmm... can anyone else use this, other than BlockBackend? If not, why
is it opaque?
Berto
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v2 08/13] block: Introduce BdrvChild.opaque
2016-05-06 12:54 ` Alberto Garcia
@ 2016-05-06 13:19 ` Kevin Wolf
0 siblings, 0 replies; 39+ messages in thread
From: Kevin Wolf @ 2016-05-06 13:19 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-block, pbonzini, mreitz, eblake, stefanha, qemu-devel
Am 06.05.2016 um 14:54 hat Alberto Garcia geschrieben:
> On Fri 22 Apr 2016 07:42:37 PM CEST, Kevin Wolf wrote:
> > BlockBackends use it to get a back pointer from BdrvChild to
> > BlockBackend in any BdrvChildRole callbacks.
>
> Hmmm... can anyone else use this, other than BlockBackend? If not, why
> is it opaque?
If we ever want to call BdrvChild callbacks from somewhere else than the
top of the image, there will be others users of this. For example, I
could imagine .resize() or .change_media() to propagate through the
whole tree when the host_cdrom driver detected a media change.
At the moment we probably don't have another use for it, but it's the
usual style that callbacks get an opaque value, so I just followed the
pattern here. (And I don't want people to abuse this to get back from
BDS to BB outside of such callbacks.)
Kevin
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v2 08/13] block: Introduce BdrvChild.opaque
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 08/13] block: Introduce BdrvChild.opaque Kevin Wolf
2016-05-06 12:54 ` Alberto Garcia
@ 2016-05-06 15:13 ` Alberto Garcia
1 sibling, 0 replies; 39+ messages in thread
From: Alberto Garcia @ 2016-05-06 15:13 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: pbonzini, mreitz, eblake, stefanha, qemu-devel
On Fri 22 Apr 2016 07:42:37 PM CEST, Kevin Wolf wrote:
> BlockBackends use it to get a back pointer from BdrvChild to
> BlockBackend in any BdrvChildRole callbacks.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 09/13] block: Drain throttling queue with BdrvChild callback
2016-04-22 17:42 [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
` (7 preceding siblings ...)
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 08/13] block: Introduce BdrvChild.opaque Kevin Wolf
@ 2016-04-22 17:42 ` Kevin Wolf
2016-05-06 15:13 ` Alberto Garcia
2016-05-09 13:17 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 10/13] block: Decouple throttling from BlockDriverState Kevin Wolf
` (5 subsequent siblings)
14 siblings, 2 replies; 39+ messages in thread
From: Kevin Wolf @ 2016-04-22 17:42 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, berto, mreitz, eblake, stefanha, qemu-devel
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(-)
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);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 09/13] block: Drain throttling queue with BdrvChild callback
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 09/13] block: Drain throttling queue with BdrvChild callback Kevin Wolf
@ 2016-05-06 15:13 ` Alberto Garcia
2016-05-09 13:17 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
1 sibling, 0 replies; 39+ messages in thread
From: Alberto Garcia @ 2016-05-06 15:13 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: pbonzini, mreitz, eblake, stefanha, qemu-devel
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
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 09/13] block: Drain throttling queue with BdrvChild callback
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 09/13] block: Drain throttling queue with BdrvChild callback Kevin Wolf
2016-05-06 15:13 ` Alberto Garcia
@ 2016-05-09 13:17 ` Stefan Hajnoczi
2016-05-09 15:42 ` Kevin Wolf
1 sibling, 1 reply; 39+ messages in thread
From: Stefan Hajnoczi @ 2016-05-09 13:17 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, mreitz, stefanha, pbonzini
[-- Attachment #1: Type: text/plain, Size: 1160 bytes --]
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?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 09/13] block: Drain throttling queue with BdrvChild callback
2016-05-09 13:17 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
@ 2016-05-09 15:42 ` Kevin Wolf
0 siblings, 0 replies; 39+ messages in thread
From: Kevin Wolf @ 2016-05-09 15:42 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: qemu-block, qemu-devel, mreitz, stefanha, pbonzini
[-- Attachment #1: Type: text/plain, Size: 1984 bytes --]
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
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 10/13] block: Decouple throttling from BlockDriverState
2016-04-22 17:42 [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
` (8 preceding siblings ...)
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 09/13] block: Drain throttling queue with BdrvChild callback Kevin Wolf
@ 2016-04-22 17:42 ` Kevin Wolf
2016-05-10 12:38 ` Alberto Garcia
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 11/13] block: Remove bdrv_move_feature_fields() Kevin Wolf
` (4 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2016-04-22 17:42 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, berto, mreitz, eblake, stefanha, qemu-devel
This moves the throttling related part of the BDS life cycle management
to BlockBackend. The throttling group reference is now kept even when no
medium is inserted.
With this commit, throttling isn't disabled and then re-enabled any more
during graph reconfiguration. This fixes the temporary breakage of I/O
throttling when used with live snapshots or block jobs that manipulate
the graph.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 34 ----------------------------------
block/block-backend.c | 37 ++++++++++++++-----------------------
blockdev.c | 27 +++++++++------------------
include/block/block_int.h | 3 ---
4 files changed, 23 insertions(+), 78 deletions(-)
diff --git a/block.c b/block.c
index 85526ad..22703ba 100644
--- a/block.c
+++ b/block.c
@@ -38,7 +38,6 @@
#include "qmp-commands.h"
#include "qemu/timer.h"
#include "qapi-event.h"
-#include "block/throttle-groups.h"
#include "qemu/cutils.h"
#include "qemu/id.h"
@@ -2123,11 +2122,6 @@ static void bdrv_close(BlockDriverState *bs)
assert(!bs->job);
- /* Disable I/O limits and drain all pending throttled requests */
- if (bs->blk && blk_get_public(bs->blk)->throttle_state) {
- blk_io_limits_disable(bs->blk);
- }
-
bdrv_drained_begin(bs); /* complete I/O */
bdrv_flush(bs);
bdrv_drain(bs); /* in case flush left pending I/O */
@@ -2256,26 +2250,6 @@ static void swap_feature_fields(BlockDriverState *bs_top,
bdrv_move_feature_fields(&tmp, bs_top);
bdrv_move_feature_fields(bs_top, bs_new);
bdrv_move_feature_fields(bs_new, &tmp);
-
- assert(!bs_new->blk);
- if (bs_top->blk && blk_get_public(bs_top->blk)->throttle_state) {
- /*
- * FIXME Need to break I/O throttling with graph manipulations
- * temporarily because of conflicting invariants (3. will go away when
- * throttling is fully converted to work on BlockBackends):
- *
- * 1. Every BlockBackend has a single root BDS
- * 2. I/O throttling functions require an attached BlockBackend
- * 3. We need to first enable throttling on the new BDS and then
- * disable it on the old one (because of throttle group refcounts)
- */
-#if 0
- bdrv_io_limits_enable(bs_new, throttle_group_get_name(bs_top));
- bdrv_io_limits_disable(bs_top);
-#else
- abort();
-#endif
- }
}
/*
@@ -3643,10 +3617,6 @@ void bdrv_detach_aio_context(BlockDriverState *bs)
baf->detach_aio_context(baf->opaque);
}
- if (bs->blk && blk_get_public(bs->blk)->throttle_state) {
- throttle_timers_detach_aio_context(
- &blk_get_public(bs->blk)->throttle_timers);
- }
if (bs->drv->bdrv_detach_aio_context) {
bs->drv->bdrv_detach_aio_context(bs);
}
@@ -3680,10 +3650,6 @@ 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->blk && blk_get_public(bs->blk)->throttle_state) {
- throttle_timers_attach_aio_context(
- &blk_get_public(bs->blk)->throttle_timers, new_context);
- }
QLIST_FOREACH(ban, &bs->aio_notifiers, list) {
ban->attached_aio_context(new_context, ban->opaque);
diff --git a/block/block-backend.c b/block/block-backend.c
index 73ff5e6..063024d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -188,10 +188,6 @@ static void blk_delete(BlockBackend *blk)
}
assert(QLIST_EMPTY(&blk->remove_bs_notifiers.notifiers));
assert(QLIST_EMPTY(&blk->insert_bs_notifiers.notifiers));
- if (blk->root_state.throttle_state) {
- g_free(blk->root_state.throttle_group);
- throttle_group_unref(blk->root_state.throttle_state);
- }
QTAILQ_REMOVE(&block_backends, blk, link);
drive_info_del(blk->legacy_dinfo);
block_acct_cleanup(&blk->stats);
@@ -445,12 +441,12 @@ void blk_remove_bs(BlockBackend *blk)
assert(blk->root->bs->blk == blk);
notifier_list_notify(&blk->remove_bs_notifiers, blk);
-
- blk_update_root_state(blk);
if (blk->public.throttle_state) {
- blk_io_limits_disable(blk);
+ throttle_timers_detach_aio_context(&blk->public.throttle_timers);
}
+ blk_update_root_state(blk);
+
blk->root->bs->blk = NULL;
bdrv_root_unref_child(blk->root);
blk->root = NULL;
@@ -468,6 +464,10 @@ void blk_insert_bs(BlockBackend *blk, BlockDriverState *bs)
bs->blk = blk;
notifier_list_notify(&blk->insert_bs_notifiers, blk);
+ if (blk->public.throttle_state) {
+ throttle_timers_attach_aio_context(
+ &blk->public.throttle_timers, bdrv_get_aio_context(bs));
+ }
}
/*
@@ -1413,7 +1413,14 @@ void blk_set_aio_context(BlockBackend *blk, AioContext *new_context)
BlockDriverState *bs = blk_bs(blk);
if (bs) {
+ if (blk->public.throttle_state) {
+ throttle_timers_detach_aio_context(&blk->public.throttle_timers);
+ }
bdrv_set_aio_context(bs, new_context);
+ if (blk->public.throttle_state) {
+ throttle_timers_attach_aio_context(&blk->public.throttle_timers,
+ new_context);
+ }
}
}
@@ -1583,19 +1590,6 @@ void blk_update_root_state(BlockBackend *blk)
blk->root_state.open_flags = blk->root->bs->open_flags;
blk->root_state.read_only = blk->root->bs->read_only;
blk->root_state.detect_zeroes = blk->root->bs->detect_zeroes;
-
- if (blk->root_state.throttle_group) {
- g_free(blk->root_state.throttle_group);
- throttle_group_unref(blk->root_state.throttle_state);
- }
- if (blk->public.throttle_state) {
- const char *name = throttle_group_get_name(blk);
- blk->root_state.throttle_group = g_strdup(name);
- blk->root_state.throttle_state = throttle_group_incref(name);
- } else {
- blk->root_state.throttle_group = NULL;
- blk->root_state.throttle_state = NULL;
- }
}
/*
@@ -1606,9 +1600,6 @@ void blk_update_root_state(BlockBackend *blk)
void blk_apply_root_state(BlockBackend *blk, BlockDriverState *bs)
{
bs->detect_zeroes = blk->root_state.detect_zeroes;
- if (blk->root_state.throttle_group) {
- blk_io_limits_enable(blk, blk->root_state.throttle_group);
- }
}
/*
diff --git a/blockdev.c b/blockdev.c
index 79dd71b..337747e 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -577,15 +577,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
blk_rs->read_only = !(bdrv_flags & BDRV_O_RDWR);
blk_rs->detect_zeroes = detect_zeroes;
- if (throttle_enabled(&cfg)) {
- if (!throttling_group) {
- throttling_group = blk_name(blk);
- }
- blk_rs->throttle_group = g_strdup(throttling_group);
- blk_rs->throttle_state = throttle_group_incref(throttling_group);
- blk_rs->throttle_state->cfg = cfg;
- }
-
QDECREF(bs_opts);
} else {
if (file && !*file) {
@@ -611,15 +602,6 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
bs->detect_zeroes = detect_zeroes;
- /* disk I/O throttling */
- if (throttle_enabled(&cfg)) {
- if (!throttling_group) {
- throttling_group = blk_name(blk);
- }
- blk_io_limits_enable(blk, throttling_group);
- blk_set_io_limits(blk, &cfg);
- }
-
if (bdrv_key_required(bs)) {
autostart = 0;
}
@@ -633,6 +615,15 @@ static BlockBackend *blockdev_init(const char *file, QDict *bs_opts,
}
}
+ /* disk I/O throttling */
+ if (throttle_enabled(&cfg)) {
+ if (!throttling_group) {
+ throttling_group = blk_name(blk);
+ }
+ blk_io_limits_enable(blk, throttling_group);
+ blk_set_io_limits(blk, &cfg);
+ }
+
blk_set_enable_write_cache(blk, !writethrough);
blk_set_on_error(blk, on_read_error, on_write_error);
diff --git a/include/block/block_int.h b/include/block/block_int.h
index cfcdaec..8481e80 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -484,9 +484,6 @@ struct BlockBackendRootState {
int open_flags;
bool read_only;
BlockdevDetectZeroesOptions detect_zeroes;
-
- char *throttle_group;
- ThrottleState *throttle_state;
};
static inline BlockDriverState *backing_bs(BlockDriverState *bs)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 10/13] block: Decouple throttling from BlockDriverState
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 10/13] block: Decouple throttling from BlockDriverState Kevin Wolf
@ 2016-05-10 12:38 ` Alberto Garcia
0 siblings, 0 replies; 39+ messages in thread
From: Alberto Garcia @ 2016-05-10 12:38 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: pbonzini, mreitz, eblake, stefanha, qemu-devel
On Fri 22 Apr 2016 07:42:39 PM CEST, Kevin Wolf wrote:
> This moves the throttling related part of the BDS life cycle management
> to BlockBackend. The throttling group reference is now kept even when no
> medium is inserted.
>
> With this commit, throttling isn't disabled and then re-enabled any more
> during graph reconfiguration. This fixes the temporary breakage of I/O
> throttling when used with live snapshots or block jobs that manipulate
> the graph.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Alberto Garcia <berto@igalia.com>
Berto
^ permalink raw reply [flat|nested] 39+ messages in thread
* [Qemu-devel] [PATCH v2 11/13] block: Remove bdrv_move_feature_fields()
2016-04-22 17:42 [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
` (9 preceding siblings ...)
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 10/13] block: Decouple throttling from BlockDriverState Kevin Wolf
@ 2016-04-22 17:42 ` Kevin Wolf
2016-05-04 14:14 ` Alberto Garcia
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 12/13] Revert "block: Forbid I/O throttling on nodes with multiple parents for 2.6" Kevin Wolf
` (3 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2016-04-22 17:42 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, berto, mreitz, eblake, stefanha, qemu-devel
bdrv_move_feature_fields() and swap_feature_fields() are empty now, they
can be removed.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
block.c | 30 ------------------------------
1 file changed, 30 deletions(-)
diff --git a/block.c b/block.c
index 22703ba..32527dc 100644
--- a/block.c
+++ b/block.c
@@ -2212,13 +2212,6 @@ void bdrv_close_all(void)
}
}
-/* Fields that need to stay with the top-level BDS */
-static void bdrv_move_feature_fields(BlockDriverState *bs_dest,
- BlockDriverState *bs_src)
-{
- /* move some fields that need to stay attached to the device */
-}
-
static void change_parent_backing_link(BlockDriverState *from,
BlockDriverState *to)
{
@@ -2242,16 +2235,6 @@ static void change_parent_backing_link(BlockDriverState *from,
}
}
-static void swap_feature_fields(BlockDriverState *bs_top,
- BlockDriverState *bs_new)
-{
- BlockDriverState tmp;
-
- bdrv_move_feature_fields(&tmp, bs_top);
- bdrv_move_feature_fields(bs_top, bs_new);
- bdrv_move_feature_fields(bs_new, &tmp);
-}
-
/*
* Add new bs contents at the top of an image chain while the chain is
* live, while keeping required fields on the top layer.
@@ -2275,9 +2258,6 @@ void bdrv_append(BlockDriverState *bs_new, BlockDriverState *bs_top)
bdrv_ref(bs_top);
- /* Some fields always stay on top of the backing file chain */
- swap_feature_fields(bs_top, bs_new);
-
change_parent_backing_link(bs_top, bs_new);
bdrv_set_backing_hd(bs_new, bs_top);
bdrv_unref(bs_top);
@@ -2294,16 +2274,6 @@ void bdrv_replace_in_backing_chain(BlockDriverState *old, BlockDriverState *new)
bdrv_ref(old);
- if (old->blk) {
- /* As long as these fields aren't in BlockBackend, but in the top-level
- * BlockDriverState, it's not possible for a BDS to have two BBs.
- *
- * We really want to copy the fields from old to new, but we go for a
- * swap instead so that pointers aren't duplicated and cause trouble.
- * (Also, bdrv_swap() used to do the same.) */
- assert(!new->blk);
- swap_feature_fields(old, new);
- }
change_parent_backing_link(old, new);
/* Change backing files if a previously independent node is added to the
--
1.8.3.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v2 12/13] Revert "block: Forbid I/O throttling on nodes with multiple parents for 2.6"
2016-04-22 17:42 [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
` (10 preceding siblings ...)
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 11/13] block: Remove bdrv_move_feature_fields() Kevin Wolf
@ 2016-04-22 17:42 ` Kevin Wolf
2016-05-09 12:31 ` Alberto Garcia
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 13/13] block: Don't check throttled reqs in bdrv_requests_pending() Kevin Wolf
` (2 subsequent siblings)
14 siblings, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2016-04-22 17:42 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, berto, mreitz, eblake, stefanha, qemu-devel
This reverts commit 76b223200ef4fb09dd87f0e213159795eb68e7a5.
Now that I/O throttling is fully done on the BlockBackend level, there
is no reason any more to block I/O throttling for nodes with multiple
parents as the parents don't influence each other any more.
Conflicts:
block.c
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 6 ------
blockdev.c | 7 -------
2 files changed, 13 deletions(-)
diff --git a/block.c b/block.c
index 32527dc..d8c6e98 100644
--- a/block.c
+++ b/block.c
@@ -1524,12 +1524,6 @@ static int bdrv_open_inherit(BlockDriverState **pbs, const char *filename,
return -ENODEV;
}
- if (blk_get_public(bs->blk)->throttle_state) {
- error_setg(errp, "Cannot reference an existing block device for "
- "which I/O throttling is enabled");
- return -EINVAL;
- }
-
bdrv_ref(bs);
*pbs = bs;
return 0;
diff --git a/blockdev.c b/blockdev.c
index 337747e..7441212 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2652,13 +2652,6 @@ void qmp_block_set_io_throttle(const char *device, int64_t bps, int64_t bps_rd,
goto out;
}
- /* The BlockBackend must be the only parent */
- assert(QLIST_FIRST(&bs->parents));
- if (QLIST_NEXT(QLIST_FIRST(&bs->parents), next_parent)) {
- error_setg(errp, "Cannot throttle device with multiple parents");
- goto out;
- }
-
throttle_config_init(&cfg);
cfg.buckets[THROTTLE_BPS_TOTAL].avg = bps;
cfg.buckets[THROTTLE_BPS_READ].avg = bps_rd;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* [Qemu-devel] [PATCH v2 13/13] block: Don't check throttled reqs in bdrv_requests_pending()
2016-04-22 17:42 [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
` (11 preceding siblings ...)
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 12/13] Revert "block: Forbid I/O throttling on nodes with multiple parents for 2.6" Kevin Wolf
@ 2016-04-22 17:42 ` Kevin Wolf
2016-05-10 12:20 ` Alberto Garcia
2016-04-29 10:01 ` [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
2016-05-09 13:21 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
14 siblings, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2016-04-22 17:42 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, pbonzini, berto, mreitz, eblake, stefanha, qemu-devel
Checking whether there are throttled requests requires going to the
associated BlockBackend, which we want to avoid. All users of
bdrv_requests_pending() already call bdrv_parent_drained_begin() first,
which restarts all throttled requests, so no throttled requests can be
left here and this is removal of dead code.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/io.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/block/io.c b/block/io.c
index b239e97..4224044 100644
--- a/block/io.c
+++ b/block/io.c
@@ -177,17 +177,10 @@ void bdrv_disable_copy_on_read(BlockDriverState *bs)
bool bdrv_requests_pending(BlockDriverState *bs)
{
BdrvChild *child;
- BlockBackendPublic *blkp = bs->blk ? blk_get_public(bs->blk) : NULL;
if (!QLIST_EMPTY(&bs->tracked_requests)) {
return true;
}
- if (blkp && !qemu_co_queue_empty(&blkp->throttled_reqs[0])) {
- return true;
- }
- if (blkp && !qemu_co_queue_empty(&blkp->throttled_reqs[1])) {
- return true;
- }
QLIST_FOREACH(child, &bs->children, next) {
if (bdrv_requests_pending(child->bs)) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 13/13] block: Don't check throttled reqs in bdrv_requests_pending()
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 13/13] block: Don't check throttled reqs in bdrv_requests_pending() Kevin Wolf
@ 2016-05-10 12:20 ` Alberto Garcia
2016-05-11 12:35 ` Kevin Wolf
0 siblings, 1 reply; 39+ messages in thread
From: Alberto Garcia @ 2016-05-10 12:20 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: pbonzini, mreitz, eblake, stefanha, qemu-devel
On Fri 22 Apr 2016 07:42:42 PM CEST, Kevin Wolf wrote:
> Checking whether there are throttled requests requires going to the
> associated BlockBackend, which we want to avoid. All users of
> bdrv_requests_pending() already call bdrv_parent_drained_begin()
> first,
There's a couple of assert(!bdrv_requests_pending()), is it also the
case with them?
Berto
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v2 13/13] block: Don't check throttled reqs in bdrv_requests_pending()
2016-05-10 12:20 ` Alberto Garcia
@ 2016-05-11 12:35 ` Kevin Wolf
0 siblings, 0 replies; 39+ messages in thread
From: Kevin Wolf @ 2016-05-11 12:35 UTC (permalink / raw)
To: Alberto Garcia; +Cc: qemu-block, pbonzini, mreitz, eblake, stefanha, qemu-devel
Am 10.05.2016 um 14:20 hat Alberto Garcia geschrieben:
> On Fri 22 Apr 2016 07:42:42 PM CEST, Kevin Wolf wrote:
> > Checking whether there are throttled requests requires going to the
> > associated BlockBackend, which we want to avoid. All users of
> > bdrv_requests_pending() already call bdrv_parent_drained_begin()
> > first,
>
> There's a couple of assert(!bdrv_requests_pending()), is it also the
> case with them?
I'm changing it into "all users in block/io.c" and adding this paragraph
to the commit message:
The remaining users (assertions during graph manipulation in
block.c) don't care about requests that are still queued in the
BlockBackend and haven't been issued for a BlockDriverState yet.
Makes sense?
Kevin
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend
2016-04-22 17:42 [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
` (12 preceding siblings ...)
2016-04-22 17:42 ` [Qemu-devel] [PATCH v2 13/13] block: Don't check throttled reqs in bdrv_requests_pending() Kevin Wolf
@ 2016-04-29 10:01 ` Kevin Wolf
2016-04-29 11:57 ` Alberto Garcia
2016-05-09 13:21 ` [Qemu-devel] [Qemu-block] " Stefan Hajnoczi
14 siblings, 1 reply; 39+ messages in thread
From: Kevin Wolf @ 2016-04-29 10:01 UTC (permalink / raw)
To: qemu-block; +Cc: pbonzini, berto, mreitz, eblake, stefanha, qemu-devel
Am 22.04.2016 um 19:42 hat Kevin Wolf geschrieben:
> This is another feature that was "logically" part of the BlockBackend, but
> implemented as a BlockDriverState feature. It was always kept on top using
> swap_feature_fields().
>
> This series moves it to be actually implemented in the BlockBackend, removing
> another obstacle for removing bs->blk and allowing multiple BBs per BDS.
>
> Applies to block-next.
Ping?
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend
2016-04-29 10:01 ` [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
@ 2016-04-29 11:57 ` Alberto Garcia
0 siblings, 0 replies; 39+ messages in thread
From: Alberto Garcia @ 2016-04-29 11:57 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: pbonzini, mreitz, eblake, stefanha, qemu-devel
On Fri 29 Apr 2016 12:01:08 PM CEST, Kevin Wolf wrote:
>> This is another feature that was "logically" part of the
>> BlockBackend, but implemented as a BlockDriverState feature. It was
>> always kept on top using swap_feature_fields().
>>
>> This series moves it to be actually implemented in the BlockBackend,
>> removing another obstacle for removing bs->blk and allowing multiple
>> BBs per BDS.
>>
>> Applies to block-next.
>
> Ping?
I had this on my review queue, I'll take a look next week!
Berto
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend
2016-04-22 17:42 [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
` (13 preceding siblings ...)
2016-04-29 10:01 ` [Qemu-devel] [PATCH v2 00/13] block: Move I/O throttling to BlockBackend Kevin Wolf
@ 2016-05-09 13:21 ` Stefan Hajnoczi
14 siblings, 0 replies; 39+ messages in thread
From: Stefan Hajnoczi @ 2016-05-09 13:21 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-block, qemu-devel, mreitz, stefanha, pbonzini
[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]
On Fri, Apr 22, 2016 at 07:42:29PM +0200, Kevin Wolf wrote:
> This is another feature that was "logically" part of the BlockBackend, but
> implemented as a BlockDriverState feature. It was always kept on top using
> swap_feature_fields().
>
> This series moves it to be actually implemented in the BlockBackend, removing
> another obstacle for removing bs->blk and allowing multiple BBs per BDS.
>
> Applies to block-next.
>
> v2:
> - Rebased on top of Paolo's 'bdrv_flush_io_queue removal, shared LinuxAioState'
> Most notable this includes a complete rewrite of patch 9 (was 10): 'block:
> Drain throttling queue with BdrvChild'. Instead of a single drain_queue()
> callback we now have a drained_begin/end() pair.
>
> - Patch 2 (was 3): 'block: Introduce BlockBackendPublic'
> Add int dummy to yet empty struct BlockBackendPublic [Eric]
>
> - Patch 11: 'block: Remove bdrv_move_feature_fields()'
> After the rebase, the function ended up empty, we can remove it now
>
> - Patch 12: 'Revert "block: Forbid I/O throttling on nodes with
> multiple parents for 2.6"'
> This was committed to master after v1 had been posted, so this one is new as
> well. The reason for forbidding this was that patch 6 ('block: Move actual
> I/O throttling to BlockBackend') would change the behaviour of the non-BB
> parents. Now that the final behaviour is implemented, we can allow the setup.
I have posted comments. Looks good overall.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread