* [Qemu-devel] [PATCH 01/12] block: Don't disable I/O throttling on sync requests
2016-03-22 15:33 [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Kevin Wolf
@ 2016-03-22 15:33 ` Kevin Wolf
2016-03-22 21:40 ` Eric Blake
2016-03-22 15:33 ` [Qemu-devel] [PATCH 02/12] block: Make sure throttled BDSes always have a BB Kevin Wolf
` (11 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Kevin Wolf @ 2016-03-22 15:33 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, berto, qemu-devel
We had to disable I/O throttling with synchronous requests because we
didn't use to run timers in nested event loops when the code was
introduced. This isn't true any more, and throttling works just fine
even when using the synchronous API.
The removed code is in fact dead code since commit a8823a3b ('block: Use
blk_co_pwritev() for blk_write()') because I/O throttling can only be
set on the top layer, but BlockBackend always uses the coroutine
interface now instead of using the sync API emulation in block.c.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/io.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/block/io.c b/block/io.c
index cce508a..e4438da 100644
--- a/block/io.c
+++ b/block/io.c
@@ -561,17 +561,6 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t offset,
.flags = flags,
};
- /**
- * In sync call context, when the vcpu is blocked, this throttling timer
- * will not fire; so the I/O throttling function has to be disabled here
- * if it has been enabled.
- */
- if (bs->io_limits_enabled) {
- fprintf(stderr, "Disabling I/O throttling on '%s' due "
- "to synchronous I/O.\n", bdrv_get_device_name(bs));
- bdrv_io_limits_disable(bs);
- }
-
if (qemu_in_coroutine()) {
/* Fast-path if already in coroutine context */
bdrv_rw_co_entry(&rwco);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH 01/12] block: Don't disable I/O throttling on sync requests
2016-03-22 15:33 ` [Qemu-devel] [PATCH 01/12] block: Don't disable I/O throttling on sync requests Kevin Wolf
@ 2016-03-22 21:40 ` Eric Blake
0 siblings, 0 replies; 27+ messages in thread
From: Eric Blake @ 2016-03-22 21:40 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: berto, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1572 bytes --]
On 03/22/2016 09:33 AM, Kevin Wolf wrote:
> We had to disable I/O throttling with synchronous requests because we
> didn't use to run timers in nested event loops when the code was
> introduced. This isn't true any more, and throttling works just fine
> even when using the synchronous API.
>
> The removed code is in fact dead code since commit a8823a3b ('block: Use
> blk_co_pwritev() for blk_write()') because I/O throttling can only be
> set on the top layer, but BlockBackend always uses the coroutine
> interface now instead of using the sync API emulation in block.c.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> block/io.c | 11 -----------
> 1 file changed, 11 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index cce508a..e4438da 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -561,17 +561,6 @@ static int bdrv_prwv_co(BlockDriverState *bs, int64_t offset,
> .flags = flags,
> };
>
> - /**
> - * In sync call context, when the vcpu is blocked, this throttling timer
> - * will not fire; so the I/O throttling function has to be disabled here
> - * if it has been enabled.
> - */
> - if (bs->io_limits_enabled) {
> - fprintf(stderr, "Disabling I/O throttling on '%s' due "
> - "to synchronous I/O.\n", bdrv_get_device_name(bs));
And we get rid of an fprintf(). Nice bonus.
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] 27+ messages in thread
* [Qemu-devel] [PATCH 02/12] block: Make sure throttled BDSes always have a BB
2016-03-22 15:33 [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Kevin Wolf
2016-03-22 15:33 ` [Qemu-devel] [PATCH 01/12] block: Don't disable I/O throttling on sync requests Kevin Wolf
@ 2016-03-22 15:33 ` Kevin Wolf
2016-03-22 21:46 ` Eric Blake
2016-03-22 15:33 ` [Qemu-devel] [PATCH 03/12] block: Introduce BlockBackendPublic Kevin Wolf
` (10 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Kevin Wolf @ 2016-03-22 15:33 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, berto, 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, but quite cumbersome, to keep things
working with some temporary hacks, so it's not worth the hassle. We'll
fix things again in a minute.
Signed-off-by: Kevin Wolf <kwolf@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 c22cb3f..4f05586 100644
--- a/block.c
+++ b/block.c
@@ -2255,8 +2255,22 @@ static void swap_feature_fields(BlockDriverState *bs_top,
assert(!bs_new->throttle_state);
if (bs_top->throttle_state) {
assert(bs_top->io_limits_enabled);
+ /*
+ * 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 a528674..df8f717 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -419,6 +419,9 @@ void blk_remove_bs(BlockBackend *blk)
notifier_list_notify(&blk->remove_bs_notifiers, blk);
blk_update_root_state(blk);
+ if (blk->root->bs->io_limits_enabled) {
+ 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 5a53f59..cac9afd 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2568,8 +2568,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);
@@ -2594,6 +2592,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 59675fa..d0b3c86 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -573,11 +573,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] 27+ messages in thread* Re: [Qemu-devel] [PATCH 02/12] block: Make sure throttled BDSes always have a BB
2016-03-22 15:33 ` [Qemu-devel] [PATCH 02/12] block: Make sure throttled BDSes always have a BB Kevin Wolf
@ 2016-03-22 21:46 ` Eric Blake
0 siblings, 0 replies; 27+ messages in thread
From: Eric Blake @ 2016-03-22 21:46 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: berto, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]
On 03/22/2016 09:33 AM, 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, but quite cumbersome, to keep things
> working with some temporary hacks, so it's not worth the hassle. We'll
> fix things again in a minute.
Might read slightly better as:
It would have been possible to keep things working with some temporary
hacks, but quite cumbersome, so it's not worth the hassle.
>
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
> +#if 0
> bdrv_io_limits_enable(bs_new, throttle_group_get_name(bs_top));
> bdrv_io_limits_disable(bs_top);
> +#else
> + abort();
If it weren't fixed in the same series, then I'd want some text to go
along with the abort. But since it's temporary, this is fine.
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] 27+ messages in thread
* [Qemu-devel] [PATCH 03/12] block: Introduce BlockBackendPublic
2016-03-22 15:33 [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Kevin Wolf
2016-03-22 15:33 ` [Qemu-devel] [PATCH 01/12] block: Don't disable I/O throttling on sync requests Kevin Wolf
2016-03-22 15:33 ` [Qemu-devel] [PATCH 02/12] block: Make sure throttled BDSes always have a BB Kevin Wolf
@ 2016-03-22 15:33 ` Kevin Wolf
2016-03-22 21:53 ` Eric Blake
2016-03-22 15:33 ` [Qemu-devel] [PATCH 04/12] block: throttle-groups: Use BlockBackend pointers internally Kevin Wolf
` (9 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Kevin Wolf @ 2016-03-22 15:33 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, berto, qemu-devel
Some features, like I/O throttling, are implemented outside
block-backend.c, but still want to keep BlockBackends in a list. In
order to avoid exposing the whole struct layout in the public header
file, this patch introduces an embedded public struct where list entry
structs 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 | 9 +++++++++
2 files changed, 26 insertions(+)
diff --git a/block/block-backend.c b/block/block-backend.c
index df8f717..4394950 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -33,6 +33,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 */
@@ -410,6 +411,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 d839bff..bb4ff6f 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -59,6 +59,12 @@ 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 {
+} 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 +80,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] 27+ messages in thread* Re: [Qemu-devel] [PATCH 03/12] block: Introduce BlockBackendPublic
2016-03-22 15:33 ` [Qemu-devel] [PATCH 03/12] block: Introduce BlockBackendPublic Kevin Wolf
@ 2016-03-22 21:53 ` Eric Blake
2016-03-23 9:09 ` Kevin Wolf
0 siblings, 1 reply; 27+ messages in thread
From: Eric Blake @ 2016-03-22 21:53 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: berto, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 2051 bytes --]
On 03/22/2016 09:33 AM, Kevin Wolf wrote:
> Some features, like I/O throttling, are implemented outside
> block-backend.c, but still want to keep BlockBackends in a list. In
> order to avoid exposing the whole struct layout in the public header
> file, this patch introduces an embedded public struct where list entry
> structs 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 | 9 +++++++++
> 2 files changed, 26 insertions(+)
>
> diff --git a/block/block-backend.c b/block/block-backend.c
> index df8f717..4394950 100644
> --- a/block/block-backend.c
> +++ b/block/block-backend.c
> @@ -33,6 +33,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;
Any reason to not put the public struct at offset 0?
> +
> +/*
> + * Returns a BlockBackend given the associated @public fields.
> + */
> +BlockBackend *blk_by_public(BlockBackendPublic *public)
> +{
> + return container_of(public, BlockBackend, public);
> +}
At least container_of() doesn't care, so I guess it doesn't matter.
> +/* 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 {
> +} BlockBackendPublic;
No fields? So really all we need this for is so that we can have an
address of a member of the larger struct, so that we can do list
operations based on that address?
--
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] 27+ messages in thread* Re: [Qemu-devel] [PATCH 03/12] block: Introduce BlockBackendPublic
2016-03-22 21:53 ` Eric Blake
@ 2016-03-23 9:09 ` Kevin Wolf
2016-03-23 21:35 ` Eric Blake
0 siblings, 1 reply; 27+ messages in thread
From: Kevin Wolf @ 2016-03-23 9:09 UTC (permalink / raw)
To: Eric Blake; +Cc: berto, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 2526 bytes --]
Am 22.03.2016 um 22:53 hat Eric Blake geschrieben:
> On 03/22/2016 09:33 AM, Kevin Wolf wrote:
> > Some features, like I/O throttling, are implemented outside
> > block-backend.c, but still want to keep BlockBackends in a list. In
> > order to avoid exposing the whole struct layout in the public header
> > file, this patch introduces an embedded public struct where list entry
> > structs 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 | 9 +++++++++
> > 2 files changed, 26 insertions(+)
> >
> > diff --git a/block/block-backend.c b/block/block-backend.c
> > index df8f717..4394950 100644
> > --- a/block/block-backend.c
> > +++ b/block/block-backend.c
> > @@ -33,6 +33,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;
>
> Any reason to not put the public struct at offset 0?
No, but also no reason to put it there. :-)
> > +
> > +/*
> > + * Returns a BlockBackend given the associated @public fields.
> > + */
> > +BlockBackend *blk_by_public(BlockBackendPublic *public)
> > +{
> > + return container_of(public, BlockBackend, public);
> > +}
>
> At least container_of() doesn't care, so I guess it doesn't matter.
>
> > +/* 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 {
> > +} BlockBackendPublic;
>
> No fields? So really all we need this for is so that we can have an
> address of a member of the larger struct, so that we can do list
> operations based on that address?
Well, a list still needs a QLIST_ENTRY, otherwise I could have directly
used BlockBackend. I just tried to keep the introduction of .public
separate from the addition of a specific list.
I think it's strictly speaking invalid C to have an empty struct, but
gcc compiles it and so I thought it should be acceptable to have one for
a single commit until something is added there.
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH 03/12] block: Introduce BlockBackendPublic
2016-03-23 9:09 ` Kevin Wolf
@ 2016-03-23 21:35 ` Eric Blake
2016-03-24 8:06 ` Kevin Wolf
0 siblings, 1 reply; 27+ messages in thread
From: Eric Blake @ 2016-03-23 21:35 UTC (permalink / raw)
To: Kevin Wolf; +Cc: berto, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1482 bytes --]
On 03/23/2016 03:09 AM, Kevin Wolf wrote:
>>> +++ b/block/block-backend.c
>>> @@ -33,6 +33,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;
>>
>> Any reason to not put the public struct at offset 0?
>
> No, but also no reason to put it there. :-)
True.
>>> +typedef struct BlockBackendPublic {
>>> +} BlockBackendPublic;
>>
>> No fields? So really all we need this for is so that we can have an
>> address of a member of the larger struct, so that we can do list
>> operations based on that address?
>
> Well, a list still needs a QLIST_ENTRY, otherwise I could have directly
> used BlockBackend. I just tried to keep the introduction of .public
> separate from the addition of a specific list.
>
> I think it's strictly speaking invalid C to have an empty struct, but
> gcc compiles it and so I thought it should be acceptable to have one for
> a single commit until something is added there.
clang gripes; see commit 83ecb22ba.
But breaking bisection for one patch on clang-only is different than
breaking bisection on all compilers, so up to you what you want to do
about it.
--
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] 27+ messages in thread* Re: [Qemu-devel] [PATCH 03/12] block: Introduce BlockBackendPublic
2016-03-23 21:35 ` Eric Blake
@ 2016-03-24 8:06 ` Kevin Wolf
0 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2016-03-24 8:06 UTC (permalink / raw)
To: Eric Blake; +Cc: berto, qemu-devel, qemu-block
[-- Attachment #1: Type: text/plain, Size: 1547 bytes --]
Am 23.03.2016 um 22:35 hat Eric Blake geschrieben:
> On 03/23/2016 03:09 AM, Kevin Wolf wrote:
> >>> +++ b/block/block-backend.c
> >>> @@ -33,6 +33,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;
> >>
> >> Any reason to not put the public struct at offset 0?
> >
> > No, but also no reason to put it there. :-)
>
> True.
>
> >>> +typedef struct BlockBackendPublic {
> >>> +} BlockBackendPublic;
> >>
> >> No fields? So really all we need this for is so that we can have an
> >> address of a member of the larger struct, so that we can do list
> >> operations based on that address?
> >
> > Well, a list still needs a QLIST_ENTRY, otherwise I could have directly
> > used BlockBackend. I just tried to keep the introduction of .public
> > separate from the addition of a specific list.
> >
> > I think it's strictly speaking invalid C to have an empty struct, but
> > gcc compiles it and so I thought it should be acceptable to have one for
> > a single commit until something is added there.
>
> clang gripes; see commit 83ecb22ba.
>
> But breaking bisection for one patch on clang-only is different than
> breaking bisection on all compilers, so up to you what you want to do
> about it.
We can always have an int dummy in there...
Kevin
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 04/12] block: throttle-groups: Use BlockBackend pointers internally
2016-03-22 15:33 [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Kevin Wolf
` (2 preceding siblings ...)
2016-03-22 15:33 ` [Qemu-devel] [PATCH 03/12] block: Introduce BlockBackendPublic Kevin Wolf
@ 2016-03-22 15:33 ` Kevin Wolf
2016-03-22 15:33 ` [Qemu-devel] [PATCH 05/12] block: Convert throttle_group_get_name() to BlockBackend Kevin Wolf
` (8 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2016-03-22 15:33 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, berto, 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 | 132 ++++++++++++++++++++--------------------
include/block/block_int.h | 1 -
include/block/throttle-groups.h | 4 +-
include/sysemu/block-backend.h | 4 ++
tests/test-throttle.c | 13 ++--
6 files changed, 82 insertions(+), 76 deletions(-)
diff --git a/block/io.c b/block/io.c
index e4438da..fbfbbb2 100644
--- a/block/io.c
+++ b/block/io.c
@@ -93,14 +93,14 @@ void bdrv_io_limits_disable(BlockDriverState *bs)
{
bs->io_limits_enabled = false;
bdrv_start_throttled_reqs(bs);
- throttle_group_unregister_bs(bs);
+ throttle_group_unregister_blk(bs->blk);
}
/* should be called before bdrv_set_io_limits if a limit is set */
void bdrv_io_limits_enable(BlockDriverState *bs, const char *group)
{
assert(!bs->io_limits_enabled);
- throttle_group_register_bs(bs, group);
+ throttle_group_register_blk(bs->blk, group);
bs->io_limits_enabled = true;
}
diff --git a/block/throttle-groups.c b/block/throttle-groups.c
index 4920e09..4ba0fa3 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,77 +146,77 @@ 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;
@@ -226,9 +227,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;
}
@@ -239,18 +240,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;
}
@@ -262,9 +264,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;
@@ -286,13 +288,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 */
@@ -308,7 +310,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);
}
@@ -377,7 +379,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);
}
}
@@ -392,17 +394,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;
@@ -415,14 +417,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),
@@ -434,19 +436,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;
@@ -456,10 +458,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;
@@ -467,7 +469,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 9c56bd9..ade6276 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -426,7 +426,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 aba28f3..46b843e 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 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 bb4ff6f..af2abf3 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -63,6 +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 {
+ /* 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 d0b3c86..f03f71a 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -19,6 +19,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;
@@ -588,9 +589,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);
@@ -622,9 +623,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] 27+ messages in thread* [Qemu-devel] [PATCH 05/12] block: Convert throttle_group_get_name() to BlockBackend
2016-03-22 15:33 [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Kevin Wolf
` (3 preceding siblings ...)
2016-03-22 15:33 ` [Qemu-devel] [PATCH 04/12] block: throttle-groups: Use BlockBackend pointers internally Kevin Wolf
@ 2016-03-22 15:33 ` Kevin Wolf
2016-03-22 15:33 ` [Qemu-devel] [PATCH 06/12] block: Move throttling fields from BDS to BB Kevin Wolf
` (7 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2016-03-22 15:33 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, berto, 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 4394950..ed8550f 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -1563,7 +1563,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 fbfbbb2..b589857 100644
--- a/block/io.c
+++ b/block/io.c
@@ -112,7 +112,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 f01894a..e908757 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -117,7 +117,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 4ba0fa3..295bed0 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 46b843e..61e84f3 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 f03f71a..85d3de2 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -597,8 +597,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] 27+ messages in thread* [Qemu-devel] [PATCH 06/12] block: Move throttling fields from BDS to BB
2016-03-22 15:33 [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Kevin Wolf
` (4 preceding siblings ...)
2016-03-22 15:33 ` [Qemu-devel] [PATCH 05/12] block: Convert throttle_group_get_name() to BlockBackend Kevin Wolf
@ 2016-03-22 15:33 ` Kevin Wolf
2016-03-22 15:33 ` [Qemu-devel] [PATCH 07/12] block: Move actual I/O throttling to BlockBackend Kevin Wolf
` (6 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2016-03-22 15:33 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, berto, 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 | 15 +++---
block/io.c | 32 +++++++-----
block/qapi.c | 2 +-
block/throttle-groups.c | 108 ++++++++++++++++++++++-------------------
blockdev.c | 4 +-
include/block/block_int.h | 13 -----
include/sysemu/block-backend.h | 11 ++++-
tests/test-throttle.c | 28 ++++++-----
9 files changed, 125 insertions(+), 110 deletions(-)
diff --git a/block.c b/block.c
index 4f05586..be06564 100644
--- a/block.c
+++ b/block.c
@@ -238,8 +238,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();
@@ -2119,7 +2117,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);
}
@@ -2252,9 +2250,9 @@ 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_top->io_limits_enabled);
+ assert(!bs_new->blk);
+ if (bs_top->blk && blk_get_public(bs_top->blk)->throttle_state) {
+ assert(blk_get_public(bs_top->blk)->io_limits_enabled);
/*
* FIXME Need to break I/O throttling with graph manipulations
* temporarily because of conflicting invariants (3. will go away when
@@ -2296,11 +2294,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);
@@ -3639,8 +3637,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);
@@ -3675,8 +3674,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 ed8550f..8e8dbb2 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -106,8 +106,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;
}
@@ -436,7 +440,7 @@ void blk_remove_bs(BlockBackend *blk)
notifier_list_notify(&blk->remove_bs_notifiers, blk);
blk_update_root_state(blk);
- if (blk->root->bs->io_limits_enabled) {
+ if (blk->public.io_limits_enabled) {
bdrv_io_limits_disable(blk->root->bs);
}
@@ -812,7 +816,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);
bool enabled;
int ret;
@@ -821,10 +824,10 @@ int blk_read_unthrottled(BlockBackend *blk, int64_t sector_num, uint8_t *buf,
return ret;
}
- enabled = bs->io_limits_enabled;
- bs->io_limits_enabled = false;
+ enabled = blk->public.io_limits_enabled;
+ blk->public.io_limits_enabled = false;
ret = blk_read(blk, sector_num, buf, nb_sectors);
- bs->io_limits_enabled = enabled;
+ blk->public.io_limits_enabled = enabled;
return ret;
}
@@ -1562,7 +1565,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 b589857..bfafdfa 100644
--- a/block/io.c
+++ b/block/io.c
@@ -65,33 +65,38 @@ void bdrv_set_io_limits(BlockDriverState *bs,
throttle_group_config(bs, cfg);
for (i = 0; i < 2; i++) {
- qemu_co_enter_next(&bs->throttled_reqs[i]);
+ qemu_co_enter_next(&blk_get_public(bs->blk)->throttled_reqs[i]);
}
}
/* this function drain all the throttled IOs */
static bool bdrv_start_throttled_reqs(BlockDriverState *bs)
{
+ if (!bs->blk) {
+ return false;
+ }
+
+ BlockBackendPublic *blkp = blk_get_public(bs->blk);
bool drained = false;
- bool enabled = bs->io_limits_enabled;
+ bool enabled = blk_get_public(bs->blk)->io_limits_enabled;
int i;
- bs->io_limits_enabled = false;
+ blkp->io_limits_enabled = false;
for (i = 0; i < 2; i++) {
- while (qemu_co_enter_next(&bs->throttled_reqs[i])) {
+ while (qemu_co_enter_next(&blkp->throttled_reqs[i])) {
drained = true;
}
}
- bs->io_limits_enabled = enabled;
+ blkp->io_limits_enabled = enabled;
return drained;
}
void bdrv_io_limits_disable(BlockDriverState *bs)
{
- bs->io_limits_enabled = false;
+ blk_get_public(bs->blk)->io_limits_enabled = false;
bdrv_start_throttled_reqs(bs);
throttle_group_unregister_blk(bs->blk);
}
@@ -99,15 +104,15 @@ 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->io_limits_enabled);
+ assert(!blk_get_public(bs->blk)->io_limits_enabled);
throttle_group_register_blk(bs->blk, group);
- bs->io_limits_enabled = true;
+ blk_get_public(bs->blk)->io_limits_enabled = true;
}
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;
}
@@ -219,14 +224,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;
}
@@ -938,7 +944,7 @@ int coroutine_fn bdrv_co_do_preadv(BlockDriverState *bs,
}
/* throttling disk I/O */
- if (bs->io_limits_enabled) {
+ if (bs->blk && blk_get_public(bs->blk)->io_limits_enabled) {
throttle_group_co_io_limits_intercept(bs, bytes, false);
}
@@ -1287,7 +1293,7 @@ int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
}
/* throttling disk I/O */
- if (bs->io_limits_enabled) {
+ if (bs->blk && blk_get_public(bs->blk)->io_limits_enabled) {
throttle_group_co_io_limits_intercept(bs, bytes, true);
}
diff --git a/block/qapi.c b/block/qapi.c
index e908757..274f359 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -66,7 +66,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 295bed0..af74f76 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,8 +215,9 @@ 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;
@@ -245,14 +246,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;
}
@@ -261,12 +262,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;
@@ -290,27 +291,29 @@ 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);
}
@@ -324,8 +327,9 @@ void coroutine_fn throttle_group_co_io_limits_intercept(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 */
@@ -348,7 +352,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);
@@ -358,12 +363,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;
@@ -373,13 +379,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);
}
}
@@ -404,7 +410,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;
@@ -414,7 +420,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 */
@@ -424,14 +430,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);
}
@@ -448,19 +454,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;
}
@@ -468,13 +474,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 cac9afd..87fe931 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -2717,14 +2717,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 ade6276..1266dc7 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"
@@ -415,18 +414,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_enabled tells us if they are currently being
- * enforced, but it can be temporarily set to false */
- CoQueue throttled_reqs[2];
- bool io_limits_enabled;
- /* The following fields are protected by the ThrottleGroup lock.
- * 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/sysemu/block-backend.h b/include/sysemu/block-backend.h
index af2abf3..e48f706 100644
--- a/include/sysemu/block-backend.h
+++ b/include/sysemu/block-backend.h
@@ -14,6 +14,7 @@
#define BLOCK_BACKEND_H
#include "qemu/typedefs.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_enabled tells us if they are currently being
+ * enforced, but it can be temporarily set to false */
+ CoQueue throttled_reqs[2];
+ bool io_limits_enabled;
/* The following fields are protected by the ThrottleGroup lock.
* 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 85d3de2..53becd7 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -575,31 +575,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);
@@ -627,9 +631,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] 27+ messages in thread* [Qemu-devel] [PATCH 07/12] block: Move actual I/O throttling to BlockBackend
2016-03-22 15:33 [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Kevin Wolf
` (5 preceding siblings ...)
2016-03-22 15:33 ` [Qemu-devel] [PATCH 06/12] block: Move throttling fields from BDS to BB Kevin Wolf
@ 2016-03-22 15:33 ` Kevin Wolf
2016-03-22 15:33 ` [Qemu-devel] [PATCH 08/12] block: Move I/O throttling configuration functions " Kevin Wolf
` (5 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2016-03-22 15:33 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, berto, 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 8e8dbb2..1567d8b 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -715,6 +715,11 @@ static int coroutine_fn blk_co_preadv(BlockBackend *blk, int64_t offset,
return ret;
}
+ /* throttling disk I/O */
+ if (blk->public.io_limits_enabled) {
+ throttle_group_co_io_limits_intercept(blk, bytes, false);
+ }
+
return bdrv_co_do_preadv(blk_bs(blk), offset, bytes, qiov, flags);
}
@@ -729,6 +734,11 @@ static int coroutine_fn blk_co_pwritev(BlockBackend *blk, int64_t offset,
return ret;
}
+ /* throttling disk I/O */
+ if (blk->public.io_limits_enabled) {
+ 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 bfafdfa..84370ae 100644
--- a/block/io.c
+++ b/block/io.c
@@ -943,11 +943,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)->io_limits_enabled) {
- 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);
@@ -1292,11 +1287,6 @@ int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
return ret;
}
- /* throttling disk I/O */
- if (bs->blk && blk_get_public(bs->blk)->io_limits_enabled) {
- 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 af74f76..46e888c 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -280,18 +280,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 61e84f3..7019d04 100644
--- a/include/block/throttle-groups.h
+++ b/include/block/throttle-groups.h
@@ -39,7 +39,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 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] 27+ messages in thread* [Qemu-devel] [PATCH 08/12] block: Move I/O throttling configuration functions to BlockBackend
2016-03-22 15:33 [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Kevin Wolf
` (6 preceding siblings ...)
2016-03-22 15:33 ` [Qemu-devel] [PATCH 07/12] block: Move actual I/O throttling to BlockBackend Kevin Wolf
@ 2016-03-22 15:33 ` Kevin Wolf
2016-03-22 15:33 ` [Qemu-devel] [PATCH 09/12] block: Introduce BdrvChild.opaque Kevin Wolf
` (4 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2016-03-22 15:33 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, berto, qemu-devel
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block.c | 2 +-
block/block-backend.c | 49 +++++++++++++++++++++++++++++++++++++++--
block/io.c | 47 +--------------------------------------
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, 78 insertions(+), 82 deletions(-)
diff --git a/block.c b/block.c
index be06564..1fe35cd 100644
--- a/block.c
+++ b/block.c
@@ -2118,7 +2118,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 1567d8b..2309672 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -441,7 +441,7 @@ void blk_remove_bs(BlockBackend *blk)
blk_update_root_state(blk);
if (blk->public.io_limits_enabled) {
- bdrv_io_limits_disable(blk->root->bs);
+ blk_io_limits_disable(blk);
}
blk->root->bs->blk = NULL;
@@ -1594,7 +1594,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);
}
}
@@ -1658,3 +1658,48 @@ int blk_flush_all(void)
return result;
}
+
+
+/* throttling disk I/O limits */
+void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg)
+{
+ int i;
+
+ throttle_group_config(blk, cfg);
+
+ for (i = 0; i < 2; i++) {
+ qemu_co_enter_next(&blk->public.throttled_reqs[i]);
+ }
+}
+
+void blk_io_limits_disable(BlockBackend *blk)
+{
+ blk->public.io_limits_enabled = false;
+ bdrv_start_throttled_reqs(blk_bs(blk));
+ throttle_group_unregister_blk(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.io_limits_enabled);
+ throttle_group_register_blk(blk, group);
+ blk->public.io_limits_enabled = true;
+}
+
+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 BB belong to */
+ blk_io_limits_disable(blk);
+ blk_io_limits_enable(blk, group);
+}
diff --git a/block/io.c b/block/io.c
index 84370ae..22dbb43 100644
--- a/block/io.c
+++ b/block/io.c
@@ -56,21 +56,8 @@ 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)
-{
- int i;
-
- throttle_group_config(bs, cfg);
-
- for (i = 0; i < 2; i++) {
- qemu_co_enter_next(&blk_get_public(bs->blk)->throttled_reqs[i]);
- }
-}
-
/* this function drain all the throttled IOs */
-static bool bdrv_start_throttled_reqs(BlockDriverState *bs)
+bool bdrv_start_throttled_reqs(BlockDriverState *bs)
{
if (!bs->blk) {
return false;
@@ -94,38 +81,6 @@ static bool bdrv_start_throttled_reqs(BlockDriverState *bs)
return drained;
}
-void bdrv_io_limits_disable(BlockDriverState *bs)
-{
- blk_get_public(bs->blk)->io_limits_enabled = false;
- bdrv_start_throttled_reqs(bs);
- throttle_group_unregister_blk(bs->blk);
-}
-
-/* should be called before bdrv_set_io_limits if a limit is set */
-void bdrv_io_limits_enable(BlockDriverState *bs, const char *group)
-{
- assert(!blk_get_public(bs->blk)->io_limits_enabled);
- throttle_group_register_blk(bs->blk, group);
- blk_get_public(bs->blk)->io_limits_enabled = true;
-}
-
-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 274f359..0a222f6 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -69,7 +69,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 46e888c..bbf9e58 100644
--- a/block/throttle-groups.c
+++ b/block/throttle-groups.c
@@ -321,12 +321,12 @@ void coroutine_fn throttle_group_co_io_limits_intercept(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);
@@ -346,12 +346,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 87fe931..bb51ef2 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -614,8 +614,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)) {
@@ -2717,16 +2717,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 3eb281a..80ece12 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -186,10 +186,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);
BlockDriver *bdrv_find_protocol(const char *filename,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1266dc7..0cc38d5 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -507,8 +507,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 7019d04..f3b0fa7 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 e48f706..a2f5d97d 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 53becd7..fe6619b 100644
--- a/tests/test-throttle.c
+++ b/tests/test-throttle.c
@@ -576,15 +576,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);
@@ -611,20 +607,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] 27+ messages in thread* [Qemu-devel] [PATCH 09/12] block: Introduce BdrvChild.opaque
2016-03-22 15:33 [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Kevin Wolf
` (7 preceding siblings ...)
2016-03-22 15:33 ` [Qemu-devel] [PATCH 08/12] block: Move I/O throttling configuration functions " Kevin Wolf
@ 2016-03-22 15:33 ` Kevin Wolf
2016-03-22 15:33 ` [Qemu-devel] [PATCH 10/12] block: Drain throttling queue with BdrvChild callback Kevin Wolf
` (3 subsequent siblings)
12 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2016-03-22 15:33 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, berto, 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 2309672..d2bd268 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -132,6 +132,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;
}
@@ -457,6 +458,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 0cc38d5..1b3c310 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -364,6 +364,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] 27+ messages in thread* [Qemu-devel] [PATCH 10/12] block: Drain throttling queue with BdrvChild callback
2016-03-22 15:33 [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Kevin Wolf
` (8 preceding siblings ...)
2016-03-22 15:33 ` [Qemu-devel] [PATCH 09/12] block: Introduce BdrvChild.opaque Kevin Wolf
@ 2016-03-22 15:33 ` Kevin Wolf
2016-03-23 21:29 ` Paolo Bonzini
2016-03-22 15:33 ` [Qemu-devel] [PATCH 11/12] block: Decouple throttling from BlockDriverState Kevin Wolf
` (2 subsequent siblings)
12 siblings, 1 reply; 27+ messages in thread
From: Kevin Wolf @ 2016-03-22 15:33 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, berto, qemu-devel
This removes the last part of I/O throttling from block/io.c and moves
it to the BlockBackend.
When draining the queue of a BlockDriverState, we must make sure that no
new requests can come in for it. Request sources from outside the block
layer are disabled with aio_disable_external(), but the throttling queue
must be handled separately.
The two obvious options we have are either to implement a similar
mechanism in BlockBackend that queues requests and avoids to pass them
to the BDS, or to flush the whole queue. The first option seems nicer
and could prevent bypassing the I/O limit, but the second is closer to
what we're already doing on the BDS level, so this patch keeps it this
way for now.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/block-backend.c | 29 +++++++++++++++++++++++++++--
block/io.c | 35 ++++++++---------------------------
include/block/block_int.h | 4 ++--
3 files changed, 37 insertions(+), 31 deletions(-)
diff --git a/block/block-backend.c b/block/block-backend.c
index d2bd268..c71ce4d 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -90,9 +90,12 @@ 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 bool blk_drain_throttling_queue(BdrvChild *child);
static const BdrvChildRole child_root = {
- .inherit_options = blk_root_inherit_options,
+ .inherit_options = blk_root_inherit_options,
+
+ .drain_queue = blk_drain_throttling_queue,
};
/*
@@ -1677,7 +1680,7 @@ void blk_set_io_limits(BlockBackend *blk, ThrottleConfig *cfg)
void blk_io_limits_disable(BlockBackend *blk)
{
blk->public.io_limits_enabled = false;
- bdrv_start_throttled_reqs(blk_bs(blk));
+ blk_drain_throttling_queue(blk->root);
throttle_group_unregister_blk(blk);
}
@@ -1705,3 +1708,25 @@ void blk_io_limits_update_group(BlockBackend *blk, const char *group)
blk_io_limits_disable(blk);
blk_io_limits_enable(blk, group);
}
+
+/* this function drain all the throttled IOs */
+static bool blk_drain_throttling_queue(BdrvChild *child)
+{
+ BlockBackend *blk = child->opaque;
+ BlockBackendPublic *blkp = &blk->public;
+ bool drained = false;
+ bool enabled = blkp->io_limits_enabled;
+ int i;
+
+ blkp->io_limits_enabled = false;
+
+ for (i = 0; i < 2; i++) {
+ while (qemu_co_enter_next(&blkp->throttled_reqs[i])) {
+ drained = true;
+ }
+ }
+
+ blkp->io_limits_enabled = enabled;
+
+ return drained;
+}
diff --git a/block/io.c b/block/io.c
index 22dbb43..f6edab8 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/error-report.h"
#define NOT_DONE 0x7fffffff /* used while emulated sync operation in progress */
@@ -56,31 +55,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);
-/* this function drain all the throttled IOs */
-bool bdrv_start_throttled_reqs(BlockDriverState *bs)
-{
- if (!bs->blk) {
- return false;
- }
-
- BlockBackendPublic *blkp = blk_get_public(bs->blk);
- bool drained = false;
- bool enabled = blk_get_public(bs->blk)->io_limits_enabled;
- int i;
-
- blkp->io_limits_enabled = false;
-
- for (i = 0; i < 2; i++) {
- while (qemu_co_enter_next(&blkp->throttled_reqs[i])) {
- drained = true;
- }
- }
-
- blkp->io_limits_enabled = enabled;
-
- return drained;
-}
-
void bdrv_setup_io_funcs(BlockDriver *bdrv)
{
/* Block drivers without coroutine functions need emulation */
@@ -2668,12 +2642,19 @@ void bdrv_io_unplug(BlockDriverState *bs)
void bdrv_flush_io_queue(BlockDriverState *bs)
{
BlockDriver *drv = bs->drv;
+ BdrvChild *c;
+
if (drv && drv->bdrv_flush_io_queue) {
drv->bdrv_flush_io_queue(bs);
} else if (bs->file) {
bdrv_flush_io_queue(bs->file->bs);
}
- bdrv_start_throttled_reqs(bs);
+
+ QLIST_FOREACH(c, &bs->parents, next_parent) {
+ if (c->role->drain_queue) {
+ c->role->drain_queue(c);
+ }
+ }
}
void bdrv_drained_begin(BlockDriverState *bs)
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 1b3c310..e064d9d 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -355,6 +355,8 @@ typedef struct BdrvAioNotifier {
struct BdrvChildRole {
void (*inherit_options)(int *child_flags, QDict *child_options,
int parent_flags, QDict *parent_options);
+
+ bool (*drain_queue)(BdrvChild *child);
};
extern const BdrvChildRole child_file;
@@ -508,8 +510,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:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH 10/12] block: Drain throttling queue with BdrvChild callback
2016-03-22 15:33 ` [Qemu-devel] [PATCH 10/12] block: Drain throttling queue with BdrvChild callback Kevin Wolf
@ 2016-03-23 21:29 ` Paolo Bonzini
2016-03-24 8:25 ` Kevin Wolf
0 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2016-03-23 21:29 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
On 22/03/2016 16:33, Kevin Wolf wrote:
> This removes the last part of I/O throttling from block/io.c and moves
> it to the BlockBackend.
>
> When draining the queue of a BlockDriverState, we must make sure that no
> new requests can come in for it. Request sources from outside the block
> layer are disabled with aio_disable_external(), but the throttling queue
> must be handled separately.
I have looked at the strategy we talked about today to implement request
cancellation (so that e.g. system reset doesn't take ages because of
throttled requests). While that may be a worthwhile addition anyway, I
think throttling bdrv_drain() may impose an excessive cost for cases
such as live migration. The risk of the guest using bdrv_drain() to
game throttling is low enough that we can keep on disabling throttling
during bdrv_drain().
So for now I think we can merge the two series just fine. The strategy
I used in my patch, adding bdrv_no_throttling_begin and
bdrv_no_throttling_end around the bdrv_drain loop, can be adapted just
as use BdrvChildRole callbacks ->drained_begin and ->drained_end.
I will post v3 of my series tomorrow, adopting your patch 1/12 of this
series and removing the recursion on bdrv_no_throttling_begin and
bdrv_no_throttling_end, which is unnecessary.
Paolo
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 10/12] block: Drain throttling queue with BdrvChild callback
2016-03-23 21:29 ` Paolo Bonzini
@ 2016-03-24 8:25 ` Kevin Wolf
2016-03-24 9:32 ` Paolo Bonzini
0 siblings, 1 reply; 27+ messages in thread
From: Kevin Wolf @ 2016-03-24 8:25 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: qemu-devel, qemu-block
Am 23.03.2016 um 22:29 hat Paolo Bonzini geschrieben:
>
>
> On 22/03/2016 16:33, Kevin Wolf wrote:
> > This removes the last part of I/O throttling from block/io.c and moves
> > it to the BlockBackend.
> >
> > When draining the queue of a BlockDriverState, we must make sure that no
> > new requests can come in for it. Request sources from outside the block
> > layer are disabled with aio_disable_external(), but the throttling queue
> > must be handled separately.
>
> I have looked at the strategy we talked about today to implement request
> cancellation (so that e.g. system reset doesn't take ages because of
> throttled requests). While that may be a worthwhile addition anyway, I
> think throttling bdrv_drain() may impose an excessive cost for cases
> such as live migration. The risk of the guest using bdrv_drain() to
> game throttling is low enough that we can keep on disabling throttling
> during bdrv_drain().
I think your cancellation series (allows to) gets rid of most if not all
blk_drain() callers in the device emulation, so it becomes harder for
guests to trigger one. Ideally only the monitor should allow triggering
a drain.
On the other hand, your other series introduces bdrv_drain() calls where
we have open-coded nested event loops waiting for a single request
today. I'm pretty sure that these can be triggered by the guest and that
throttling the drain would be desirable therefore. Maybe we need a
different function there, and maybe we can even retain the behaviour
that it doesn't unnecessarily flush everything instead of just waiting
for the completion of a single request.
> So for now I think we can merge the two series just fine. The strategy
> I used in my patch, adding bdrv_no_throttling_begin and
> bdrv_no_throttling_end around the bdrv_drain loop, can be adapted just
> as use BdrvChildRole callbacks ->drained_begin and ->drained_end.
Okay. Actually, such a pair of callbacks - not only into the
BlockBackend, but from there into the guest device - was a thought
already when we introduced aio_disable_external(). Do you think it would
make sense to change things in the mid term so that the users of a
BlockBackend just get drain_begin/end callbacks?
> I will post v3 of my series tomorrow, adopting your patch 1/12 of this
> series and removing the recursion on bdrv_no_throttling_begin and
> bdrv_no_throttling_end, which is unnecessary.
Okay, I'll try to rebase then.
Kevin
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 10/12] block: Drain throttling queue with BdrvChild callback
2016-03-24 8:25 ` Kevin Wolf
@ 2016-03-24 9:32 ` Paolo Bonzini
0 siblings, 0 replies; 27+ messages in thread
From: Paolo Bonzini @ 2016-03-24 9:32 UTC (permalink / raw)
To: Kevin Wolf; +Cc: qemu-devel, qemu-block
On 24/03/2016 09:25, Kevin Wolf wrote:
> I think your cancellation series (allows to) gets rid of most if not all
> blk_drain() callers in the device emulation, so it becomes harder for
> guests to trigger one. Ideally only the monitor should allow triggering
> a drain.
More precesely you still need to call drain, but indeed they won't be
able to game throttling.
> On the other hand, your other series introduces bdrv_drain() calls where
> we have open-coded nested event loops waiting for a single request
> today. I'm pretty sure that these can be triggered by the guest and that
> throttling the drain would be desirable therefore.
The open-coded nested event loops are typically triggered only from
qemu-io, bdrv_create, etc. They shouldn't be worse than the "disable
throttling for sync I/O" that we used to have.
> Okay. Actually, such a pair of callbacks - not only into the
> BlockBackend, but from there into the guest device - was a thought
> already when we introduced aio_disable_external(). Do you think it would
> make sense to change things in the mid term so that the users of a
> BlockBackend just get drain_begin/end callbacks?
Yes, aio_disable_external and aio_disable_internal can be moved to the
BdrvChildRole callbacks too.
Paolo
^ permalink raw reply [flat|nested] 27+ messages in thread
* [Qemu-devel] [PATCH 11/12] block: Decouple throttling from BlockDriverState
2016-03-22 15:33 [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Kevin Wolf
` (9 preceding siblings ...)
2016-03-22 15:33 ` [Qemu-devel] [PATCH 10/12] block: Drain throttling queue with BdrvChild callback Kevin Wolf
@ 2016-03-22 15:33 ` Kevin Wolf
2016-03-22 15:33 ` [Qemu-devel] [PATCH 12/12] block: Don't check throttled reqs in bdrv_requests_pending() Kevin Wolf
2016-03-22 21:33 ` [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Paolo Bonzini
12 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2016-03-22 15:33 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, berto, 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 | 35 -----------------------------------
block/block-backend.c | 37 ++++++++++++++-----------------------
blockdev.c | 27 +++++++++------------------
include/block/block_int.h | 3 ---
4 files changed, 23 insertions(+), 79 deletions(-)
diff --git a/block.c b/block.c
index 1fe35cd..fb37b91 100644
--- a/block.c
+++ b/block.c
@@ -39,7 +39,6 @@
#include "qmp-commands.h"
#include "qemu/timer.h"
#include "qapi-event.h"
-#include "block/throttle-groups.h"
#ifdef CONFIG_BSD
#include <sys/ioctl.h>
@@ -2116,11 +2115,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 */
@@ -2249,27 +2243,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) {
- assert(blk_get_public(bs_top->blk)->io_limits_enabled);
- /*
- * 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
- }
}
/*
@@ -3637,10 +3610,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);
}
@@ -3674,10 +3643,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 c71ce4d..8b4eb1a 100644
--- a/block/block-backend.c
+++ b/block/block-backend.c
@@ -185,10 +185,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);
@@ -442,11 +438,11 @@ void blk_remove_bs(BlockBackend *blk)
assert(blk->root->bs->blk == blk);
notifier_list_notify(&blk->remove_bs_notifiers, blk);
+ if (blk->public.throttle_state) {
+ throttle_timers_detach_aio_context(&blk->public.throttle_timers);
+ }
blk_update_root_state(blk);
- if (blk->public.io_limits_enabled) {
- blk_io_limits_disable(blk);
- }
blk->root->bs->blk = NULL;
bdrv_root_unref_child(blk->root);
@@ -465,6 +461,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));
+ }
}
/*
@@ -1405,7 +1405,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);
+ }
}
}
@@ -1575,19 +1582,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;
- }
}
/*
@@ -1598,9 +1592,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 bb51ef2..99657d0 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -575,15 +575,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) {
@@ -609,15 +600,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;
}
@@ -631,6 +613,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 e064d9d..000d2c0 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -473,9 +473,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] 27+ messages in thread* [Qemu-devel] [PATCH 12/12] block: Don't check throttled reqs in bdrv_requests_pending()
2016-03-22 15:33 [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Kevin Wolf
` (10 preceding siblings ...)
2016-03-22 15:33 ` [Qemu-devel] [PATCH 11/12] block: Decouple throttling from BlockDriverState Kevin Wolf
@ 2016-03-22 15:33 ` Kevin Wolf
2016-03-22 21:33 ` [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Paolo Bonzini
12 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2016-03-22 15:33 UTC (permalink / raw)
To: qemu-block; +Cc: kwolf, berto, 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_flush_io_queue() first, which
restarts throttled requests. We just have to use the return value of
that callback (which tells us whether any requests have been restarted)
instead of ignoring it.
Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
block/io.c | 22 ++++++++++------------
include/block/block.h | 2 +-
2 files changed, 11 insertions(+), 13 deletions(-)
diff --git a/block/io.c b/block/io.c
index f6edab8..24bdd6c 100644
--- a/block/io.c
+++ b/block/io.c
@@ -153,17 +153,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)) {
@@ -204,8 +197,8 @@ void bdrv_drain(BlockDriverState *bs)
bdrv_drain_recurse(bs);
while (busy) {
/* Keep iterating */
- bdrv_flush_io_queue(bs);
- busy = bdrv_requests_pending(bs);
+ busy = bdrv_flush_io_queue(bs);
+ busy |= bdrv_requests_pending(bs);
busy |= aio_poll(bdrv_get_aio_context(bs), busy);
}
}
@@ -254,7 +247,9 @@ void bdrv_drain_all(void)
aio_context_acquire(aio_context);
while ((bs = bdrv_next(bs))) {
if (aio_context == bdrv_get_aio_context(bs)) {
- bdrv_flush_io_queue(bs);
+ if (bdrv_flush_io_queue(bs)) {
+ busy = true;
+ }
if (bdrv_requests_pending(bs)) {
busy = true;
aio_poll(aio_context, busy);
@@ -2639,10 +2634,11 @@ void bdrv_io_unplug(BlockDriverState *bs)
}
}
-void bdrv_flush_io_queue(BlockDriverState *bs)
+bool bdrv_flush_io_queue(BlockDriverState *bs)
{
BlockDriver *drv = bs->drv;
BdrvChild *c;
+ bool new_request = false;
if (drv && drv->bdrv_flush_io_queue) {
drv->bdrv_flush_io_queue(bs);
@@ -2652,9 +2648,11 @@ void bdrv_flush_io_queue(BlockDriverState *bs)
QLIST_FOREACH(c, &bs->parents, next_parent) {
if (c->role->drain_queue) {
- c->role->drain_queue(c);
+ new_request |= c->role->drain_queue(c);
}
}
+
+ return new_request;
}
void bdrv_drained_begin(BlockDriverState *bs)
diff --git a/include/block/block.h b/include/block/block.h
index 80ece12..46cee8e 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -513,7 +513,7 @@ int bdrv_probe_geometry(BlockDriverState *bs, HDGeometry *geo);
void bdrv_io_plug(BlockDriverState *bs);
void bdrv_io_unplug(BlockDriverState *bs);
-void bdrv_flush_io_queue(BlockDriverState *bs);
+bool bdrv_flush_io_queue(BlockDriverState *bs);
/**
* bdrv_drained_begin:
--
1.8.3.1
^ permalink raw reply related [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend
2016-03-22 15:33 [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Kevin Wolf
` (11 preceding siblings ...)
2016-03-22 15:33 ` [Qemu-devel] [PATCH 12/12] block: Don't check throttled reqs in bdrv_requests_pending() Kevin Wolf
@ 2016-03-22 21:33 ` Paolo Bonzini
2016-03-23 9:03 ` Kevin Wolf
12 siblings, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2016-03-22 21:33 UTC (permalink / raw)
To: Kevin Wolf, qemu-block; +Cc: qemu-devel
On 22/03/2016 16:33, 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.
>
> Depends on 'block: Implement writethrough in BlockBackend'.
This series would mess up my own I/O throttling cleanups that have been
posted in February and have hardly seen a review for one month.
I expect the rules for soft freeze to apply to maintainers as well.
These patches and the removal of bs->blk are about one month late and
shouldn't be included in 2.6.
Thanks,
Paolo
> Kevin Wolf (12):
> block: Don't disable I/O throttling on sync requests
> block: Make sure throttled BDSes always have a BB
> block: Introduce BlockBackendPublic
> block: throttle-groups: Use BlockBackend pointers internally
> block: Convert throttle_group_get_name() to BlockBackend
> block: Move throttling fields from BDS to BB
> block: Move actual I/O throttling to BlockBackend
> block: Move I/O throttling configuration functions to BlockBackend
> block: Introduce BdrvChild.opaque
> block: Drain throttling queue with BdrvChild callback
> block: Decouple throttling from BlockDriverState
> block: Don't check throttled reqs in bdrv_requests_pending()
>
> block.c | 23 +----
> block/block-backend.c | 146 +++++++++++++++++++++-----
> block/io.c | 115 +++------------------
> block/qapi.c | 6 +-
> block/throttle-groups.c | 223 +++++++++++++++++++++-------------------
> blockdev.c | 43 +++-----
> include/block/block.h | 6 +-
> include/block/block_int.h | 23 +----
> include/block/throttle-groups.h | 12 +--
> include/sysemu/block-backend.h | 27 +++++
> tests/test-throttle.c | 62 ++++++-----
> 11 files changed, 345 insertions(+), 341 deletions(-)
>
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend
2016-03-22 21:33 ` [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend Paolo Bonzini
@ 2016-03-23 9:03 ` Kevin Wolf
2016-03-23 9:28 ` Paolo Bonzini
2016-03-23 10:05 ` Alberto Garcia
0 siblings, 2 replies; 27+ messages in thread
From: Kevin Wolf @ 2016-03-23 9:03 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: berto, qemu-devel, qemu-block
Am 22.03.2016 um 22:33 hat Paolo Bonzini geschrieben:
> On 22/03/2016 16:33, 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.
> >
> > Depends on 'block: Implement writethrough in BlockBackend'.
>
> This series would mess up my own I/O throttling cleanups that have been
> posted in February and have hardly seen a review for one month.
Which cleanups? The ones that you hid in an I/O path locking series?
Whose v2 didn't even include qemu-block? I noticed only now that they
exist.
I can take a look, but nobody told me (or Berto, for that matter) that
this is a series we should have a look at. block/io.c is maintained by
Stefan, throttling isn't.
> I expect the rules for soft freeze to apply to maintainers as well.
> These patches and the removal of bs->blk are about one month late and
> shouldn't be included in 2.6.
This is not a feature. It's a series that brings actual behaviour in
line with the promised API, in other words a bug fix.
Admittedly, it's a very heavy fix, but if we decide that we can't do a
massive bug fix that late in the cycle (I admit that it got a bit late,
even though the initial patches were on time before the soft freeze), we
need to carefully check which other features we must revert in order to
keep the API changes in 2.7 minimal. At least, I think, we'd have to
disallow referencing backing files by node-name in 2.6.
Kevin
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend
2016-03-23 9:03 ` Kevin Wolf
@ 2016-03-23 9:28 ` Paolo Bonzini
2016-03-23 10:02 ` Kevin Wolf
2016-03-23 10:05 ` Alberto Garcia
1 sibling, 1 reply; 27+ messages in thread
From: Paolo Bonzini @ 2016-03-23 9:28 UTC (permalink / raw)
To: Kevin Wolf; +Cc: berto, qemu-devel, qemu-block
On 23/03/2016 10:03, Kevin Wolf wrote:
> Am 22.03.2016 um 22:33 hat Paolo Bonzini geschrieben:
>> On 22/03/2016 16:33, 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.
>>>
>>> Depends on 'block: Implement writethrough in BlockBackend'.
>>
>> This series would mess up my own I/O throttling cleanups that have been
>> posted in February and have hardly seen a review for one month.
>
> Which cleanups? The ones that you hid in an I/O path locking series?
> Whose v2 didn't even include qemu-block? I noticed only now that they
> exist.
>
> I can take a look, but nobody told me (or Berto, for that matter) that
> this is a series we should have a look at. block/io.c is maintained by
> Stefan, throttling isn't.
If you actually look at the series, all of the changes are in block/io.c
except for:
- moving some code from block/io.c to block/throttle-groups.c, with zero
semantic change.
- adding exactly three lines of code to block/throttle-groups.c:
if (bs->io_limits_disabled) {
return false;
}
I think it's okay to assume that the block/io.c maintainer can make a
fair judgment on these changes.
On the other hand, this series touches block/io.c much more heavily,
adding code to a function that we want to remove altogether
(bdrv_flush_io_queue). Yet, Stefan wasn't even CCed. If you intend to
override the block/io.c maintainer, you are _expected_ to take a look at
pending block/io.c patches and see how they interact with yours.
Sure, v2 of my series was not CCed to qemu-block, but v1 was. Even just
reading the 00/16 part of the thread would have provided the status of
the series without any doubt. Again, I'd only expect you to take this
additional burden because you didn't CC the submaintainer on this
series. The alternative is to do that, explain him why you are sending
such an intrusive fix close to soft freeze, and letting him make a decision.
>> I expect the rules for soft freeze to apply to maintainers as well.
>> These patches and the removal of bs->blk are about one month late and
>> shouldn't be included in 2.6.
>
> This is not a feature. It's a series that brings actual behaviour in
> line with the promised API, in other words a bug fix.
>
> Admittedly, it's a very heavy fix, but if we decide that we can't do a
> massive bug fix that late in the cycle (I admit that it got a bit late,
> even though the initial patches were on time before the soft freeze), we
> need to carefully check which other features we must revert in order to
> keep the API changes in 2.7 minimal. At least, I think, we'd have to
> disallow referencing backing files by node-name in 2.6.
Let me rephrase that. There is a feature, "referencing backing files by
node-name", which should have had patches on the list by the soft freeze
date; you committed the API without having everything else posted or
even written, and now everything else qualifies as bug fix.
Since the damage is done, I guess you actually *should* go on and commit
these patches during hard freeze. But please give priority to patches
that were sent according to the rules.
Paolo
^ permalink raw reply [flat|nested] 27+ messages in thread* Re: [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend
2016-03-23 9:28 ` Paolo Bonzini
@ 2016-03-23 10:02 ` Kevin Wolf
0 siblings, 0 replies; 27+ messages in thread
From: Kevin Wolf @ 2016-03-23 10:02 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: stefanha, berto, qemu-devel, qemu-block
[ Cc: Stefan ]
Am 23.03.2016 um 10:28 hat Paolo Bonzini geschrieben:
> On 23/03/2016 10:03, Kevin Wolf wrote:
> > Am 22.03.2016 um 22:33 hat Paolo Bonzini geschrieben:
> >> On 22/03/2016 16:33, 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.
> >>>
> >>> Depends on 'block: Implement writethrough in BlockBackend'.
> >>
> >> This series would mess up my own I/O throttling cleanups that have been
> >> posted in February and have hardly seen a review for one month.
> >
> > Which cleanups? The ones that you hid in an I/O path locking series?
> > Whose v2 didn't even include qemu-block? I noticed only now that they
> > exist.
> >
> > I can take a look, but nobody told me (or Berto, for that matter) that
> > this is a series we should have a look at. block/io.c is maintained by
> > Stefan, throttling isn't.
>
> If you actually look at the series, all of the changes are in block/io.c
> except for:
>
> - moving some code from block/io.c to block/throttle-groups.c, with zero
> semantic change.
>
> - adding exactly three lines of code to block/throttle-groups.c:
>
> if (bs->io_limits_disabled) {
> return false;
> }
>
> I think it's okay to assume that the block/io.c maintainer can make a
> fair judgment on these changes.
I'm less worried about the actual code, but about the architectural
part. This example shows that dividing maintainership at source file
boundaries doesn't always make sense.
> On the other hand, this series touches block/io.c much more heavily,
> adding code to a function that we want to remove altogether
> (bdrv_flush_io_queue). Yet, Stefan wasn't even CCed. If you intend to
> override the block/io.c maintainer, you are _expected_ to take a look at
> pending block/io.c patches and see how they interact with yours.
>
> Sure, v2 of my series was not CCed to qemu-block, but v1 was. Even just
> reading the 00/16 part of the thread would have provided the status of
> the series without any doubt. Again, I'd only expect you to take this
> additional burden because you didn't CC the submaintainer on this
> series. The alternative is to do that, explain him why you are sending
> such an intrusive fix close to soft freeze, and letting him make a decision.
Yes, both of us messed up their CC list, thanks for pointing it out.
Adding Stefan to the thread now.
> >> I expect the rules for soft freeze to apply to maintainers as well.
> >> These patches and the removal of bs->blk are about one month late and
> >> shouldn't be included in 2.6.
> >
> > This is not a feature. It's a series that brings actual behaviour in
> > line with the promised API, in other words a bug fix.
> >
> > Admittedly, it's a very heavy fix, but if we decide that we can't do a
> > massive bug fix that late in the cycle (I admit that it got a bit late,
> > even though the initial patches were on time before the soft freeze), we
> > need to carefully check which other features we must revert in order to
> > keep the API changes in 2.7 minimal. At least, I think, we'd have to
> > disallow referencing backing files by node-name in 2.6.
>
> Let me rephrase that. There is a feature, "referencing backing files by
> node-name", which should have had patches on the list by the soft freeze
> date; you committed the API without having everything else posted or
> even written, and now everything else qualifies as bug fix.
More like the feature was merged without noticing that this has even
more implications that we saw. This was at the very beginning of the 2.6
cycle, posted during the freeze of 2.5 (don't remember whether soft or
hard). So at least we're lucky enough that we can still revert it if we
need. It had been almost a year before we decided that it can finally go
in, so I don't think I have to take blame for rushing things there.
The problem is that it allows you to create configurations where it
becomes visible that things are implemented different from the design
that we envision and for which the QMP APIs are tailored (essentially
implemented in BDS + bdrv_swap() instead of implemented in BB).
Features that are implemented in the BB can only ever be enabled on the
top level. As long as they are implemented in the BDS, however, you can
also enable them in intermediate layers, which will break as soon as we
implement the real thing. So in order to avoid API breakage we either
need to restrict user control over intermediate layers or we do the real
thing now.
User control over backing files is new in 2.6, so we can revert that if
we have to, but doing the real thing might be preferable.
User control over bs->file is older, though, and in principle affected
by the same problem. Of course, it's less likely that people actually
use the problematic features there and removing them now is already an
API change, so it's a less good reason for fixing things in 2.6 rathe
than 2.7.
> Since the damage is done, I guess you actually *should* go on and commit
> these patches during hard freeze. But please give priority to patches
> that were sent according to the rules.
I'll have a look at your series and check how serious the conflicts are.
At the first sight I'm afraid it's not obvious and we need to discuss in
more detail how we can achieve both goals before any of the series can
be merged.
Kevin
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [Qemu-devel] [PATCH 00/12] block: Move I/O throttling to BlockBackend
2016-03-23 9:03 ` Kevin Wolf
2016-03-23 9:28 ` Paolo Bonzini
@ 2016-03-23 10:05 ` Alberto Garcia
1 sibling, 0 replies; 27+ messages in thread
From: Alberto Garcia @ 2016-03-23 10:05 UTC (permalink / raw)
To: Kevin Wolf, Paolo Bonzini; +Cc: qemu-devel, qemu-block
On Wed 23 Mar 2016 10:03:41 AM CET, Kevin Wolf <kwolf@redhat.com> wrote:
>> This series would mess up my own I/O throttling cleanups that have
>> been posted in February and have hardly seen a review for one month.
>
> Which cleanups? The ones that you hid in an I/O path locking series?
> Whose v2 didn't even include qemu-block? I noticed only now that they
> exist.
>
> I can take a look, but nobody told me (or Berto, for that matter) that
> this is a series we should have a look at. block/io.c is maintained by
> Stefan, throttling isn't.
I also didn't notice the changes to the throttling code in that series,
sorry!
Berto
^ permalink raw reply [flat|nested] 27+ messages in thread