* [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
@ 2013-01-10 9:21 Peter Lieven
2013-01-10 10:19 ` Paolo Bonzini
2013-01-10 10:45 ` Markus Armbruster
0 siblings, 2 replies; 17+ messages in thread
From: Peter Lieven @ 2013-01-10 9:21 UTC (permalink / raw)
To: qemu-devel@nongnu.org; +Cc: kwolf, Paolo Bonzini
If io_limits are specified during runtime that exceed the number of operations in flight
bs->io_base is not initialized in the else statement in bdrv_exceed_io_limits().
The wait time calculated in bdrv_exceed_{bps,iops}_limits is thus totally wrong
and the machine locks.
Signed-off-by: Peter Lieven <pl@kamp.de>
---
block.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/block.c b/block.c
index 4e28c55..309aa85 100644
--- a/block.c
+++ b/block.c
@@ -159,6 +159,10 @@ void bdrv_io_limits_enable(BlockDriverState *bs)
bs->slice_start = qemu_get_clock_ns(vm_clock);
bs->slice_end = bs->slice_start + bs->slice_time;
memset(&bs->io_base, 0, sizeof(bs->io_base));
+ bs->io_base.bytes[0] = bs->nr_bytes[0];
+ bs->io_base.bytes[1] = bs->nr_bytes[1];
+ bs->io_base.ios[0] = bs->nr_ops[0];
+ bs->io_base.ios[1] = bs->nr_ops[1];
bs->io_limits_enabled = true;
}
--
1.7.9.5
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 9:21 [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking Peter Lieven
@ 2013-01-10 10:19 ` Paolo Bonzini
2013-01-10 10:52 ` Peter Lieven
2013-01-10 10:45 ` Markus Armbruster
1 sibling, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2013-01-10 10:19 UTC (permalink / raw)
To: Peter Lieven; +Cc: kwolf, qemu-devel@nongnu.org
Il 10/01/2013 10:21, Peter Lieven ha scritto:
> If io_limits are specified during runtime that exceed the number of
> operations in flight
> bs->io_base is not initialized in the else statement in
> bdrv_exceed_io_limits().
> The wait time calculated in bdrv_exceed_{bps,iops}_limits is thus
> totally wrong
> and the machine locks.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
> block.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/block.c b/block.c
> index 4e28c55..309aa85 100644
> --- a/block.c
> +++ b/block.c
> @@ -159,6 +159,10 @@ void bdrv_io_limits_enable(BlockDriverState *bs)
> bs->slice_start = qemu_get_clock_ns(vm_clock);
> bs->slice_end = bs->slice_start + bs->slice_time;
> memset(&bs->io_base, 0, sizeof(bs->io_base));
Please remove this memset.
> + bs->io_base.bytes[0] = bs->nr_bytes[0];
> + bs->io_base.bytes[1] = bs->nr_bytes[1];
> + bs->io_base.ios[0] = bs->nr_ops[0];
> + bs->io_base.ios[1] = bs->nr_ops[1];
> bs->io_limits_enabled = true;
> }
>
Also, perhaps you can just call
bdrv_exceed_io_limits(bs, 0, 0, NULL);
(which also subsumes the setting of slice_time, slice_start, slice_end).
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 9:21 [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking Peter Lieven
2013-01-10 10:19 ` Paolo Bonzini
@ 2013-01-10 10:45 ` Markus Armbruster
2013-01-10 10:55 ` Paolo Bonzini
2013-01-10 10:59 ` Peter Lieven
1 sibling, 2 replies; 17+ messages in thread
From: Markus Armbruster @ 2013-01-10 10:45 UTC (permalink / raw)
To: Peter Lieven; +Cc: kwolf, Paolo Bonzini, qemu-devel@nongnu.org
Subject line made me go "huh?" until I realized that you mean "Guest
locks up", not "QEMU uses locks to synchronize". Suggest to add the
preposition :)
Peter Lieven <pl@dlhnet.de> writes:
> If io_limits are specified during runtime that exceed the number of operations in flight
> bs->io_base is not initialized in the else statement in bdrv_exceed_io_limits().
I'm confused.
if ((bs->slice_start < now)
&& (bs->slice_end > now)) {
bs->slice_end = now + bs->slice_time;
} else {
bs->slice_time = 5 * BLOCK_IO_SLICE_TIME;
bs->slice_start = now;
bs->slice_end = now + bs->slice_time;
bs->io_base.bytes[is_write] = bs->nr_bytes[is_write];
bs->io_base.bytes[!is_write] = bs->nr_bytes[!is_write];
bs->io_base.ios[is_write] = bs->nr_ops[is_write];
bs->io_base.ios[!is_write] = bs->nr_ops[!is_write];
}
It's set in the else. Could you explain what goes wrong in a bit more
detail?
> The wait time calculated in bdrv_exceed_{bps,iops}_limits is thus totally wrong
> and the machine locks.
Please wrap lines around column 70.
>
> Signed-off-by: Peter Lieven <pl@kamp.de>
> ---
> block.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/block.c b/block.c
> index 4e28c55..309aa85 100644
> --- a/block.c
> +++ b/block.c
> @@ -159,6 +159,10 @@ void bdrv_io_limits_enable(BlockDriverState *bs)
> bs->slice_start = qemu_get_clock_ns(vm_clock);
> bs->slice_end = bs->slice_start + bs->slice_time;
> memset(&bs->io_base, 0, sizeof(bs->io_base));
> + bs->io_base.bytes[0] = bs->nr_bytes[0];
> + bs->io_base.bytes[1] = bs->nr_bytes[1];
> + bs->io_base.ios[0] = bs->nr_ops[0];
> + bs->io_base.ios[1] = bs->nr_ops[1];
> bs->io_limits_enabled = true;
> }
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 10:19 ` Paolo Bonzini
@ 2013-01-10 10:52 ` Peter Lieven
2013-01-10 10:55 ` Paolo Bonzini
0 siblings, 1 reply; 17+ messages in thread
From: Peter Lieven @ 2013-01-10 10:52 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel@nongnu.org
Am 10.01.2013 um 11:19 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> Il 10/01/2013 10:21, Peter Lieven ha scritto:
>> If io_limits are specified during runtime that exceed the number of
>> operations in flight
>> bs->io_base is not initialized in the else statement in
>> bdrv_exceed_io_limits().
>> The wait time calculated in bdrv_exceed_{bps,iops}_limits is thus
>> totally wrong
>> and the machine locks.
>>
>> Signed-off-by: Peter Lieven <pl@kamp.de>
>> ---
>> block.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/block.c b/block.c
>> index 4e28c55..309aa85 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -159,6 +159,10 @@ void bdrv_io_limits_enable(BlockDriverState *bs)
>> bs->slice_start = qemu_get_clock_ns(vm_clock);
>> bs->slice_end = bs->slice_start + bs->slice_time;
>> memset(&bs->io_base, 0, sizeof(bs->io_base));
>
> Please remove this memset.
>
>> + bs->io_base.bytes[0] = bs->nr_bytes[0];
>> + bs->io_base.bytes[1] = bs->nr_bytes[1];
>> + bs->io_base.ios[0] = bs->nr_ops[0];
>> + bs->io_base.ios[1] = bs->nr_ops[1];
>> bs->io_limits_enabled = true;
>> }
>>
>
> Also, perhaps you can just call
>
> bdrv_exceed_io_limits(bs, 0, 0, NULL);
won`t this segfault if only one of bps or ions limit is set?
in this case it reads wait before returning false in
bdrv_exceed_{bps,iops}_limits().
>
> (which also subsumes the setting of slice_time, slice_start, slice_end).
>
> Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 10:52 ` Peter Lieven
@ 2013-01-10 10:55 ` Paolo Bonzini
2013-01-10 11:02 ` Peter Lieven
0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2013-01-10 10:55 UTC (permalink / raw)
To: Peter Lieven; +Cc: kwolf, qemu-devel@nongnu.org
Il 10/01/2013 11:52, Peter Lieven ha scritto:
>> > Also, perhaps you can just call
>> >
>> > bdrv_exceed_io_limits(bs, 0, 0, NULL);
> won`t this segfault if only one of bps or ions limit is set?
> in this case it reads wait before returning false in
> bdrv_exceed_{bps,iops}_limits().
I didn't see any accesses that aren't checked with "if (wait)". Am I
blind? :)
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 10:45 ` Markus Armbruster
@ 2013-01-10 10:55 ` Paolo Bonzini
2013-01-10 11:52 ` Markus Armbruster
2013-01-10 10:59 ` Peter Lieven
1 sibling, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2013-01-10 10:55 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, Peter Lieven, qemu-devel@nongnu.org
Il 10/01/2013 11:45, Markus Armbruster ha scritto:
>
>> > If io_limits are specified during runtime that exceed the number of operations in flight
>> > bs->io_base is not initialized in the else statement in bdrv_exceed_io_limits().
> I'm confused.
>
> if ((bs->slice_start < now)
> && (bs->slice_end > now)) {
> bs->slice_end = now + bs->slice_time;
> } else {
> bs->slice_time = 5 * BLOCK_IO_SLICE_TIME;
> bs->slice_start = now;
> bs->slice_end = now + bs->slice_time;
>
> bs->io_base.bytes[is_write] = bs->nr_bytes[is_write];
> bs->io_base.bytes[!is_write] = bs->nr_bytes[!is_write];
>
> bs->io_base.ios[is_write] = bs->nr_ops[is_write];
> bs->io_base.ios[!is_write] = bs->nr_ops[!is_write];
> }
bdrv_io_limits_enable correctly starts a new slice (the first three
lines of the else), but does not set io_base correctly for that slice.
Here is how io_base is used:
bytes_base = bs->nr_bytes[is_write] - bs->io_base.bytes[is_write];
bytes_res = (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
if (bytes_base + bytes_res <= bytes_limit) {
/* no wait */
} else {
/* operation needs to be throttled */
}
As a result, any I/O operations that are triggered between now and
bs->slice_end are incorrectly limited. If 10 MB of data has been
written since the VM was started, QEMU thinks that 10 MB of data has
been written in this slice.
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 10:45 ` Markus Armbruster
2013-01-10 10:55 ` Paolo Bonzini
@ 2013-01-10 10:59 ` Peter Lieven
2013-01-10 11:17 ` Paolo Bonzini
1 sibling, 1 reply; 17+ messages in thread
From: Peter Lieven @ 2013-01-10 10:59 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, Paolo Bonzini, qemu-devel@nongnu.org
Am 10.01.2013 um 11:45 schrieb Markus Armbruster <armbru@redhat.com>:
> Subject line made me go "huh?" until I realized that you mean "Guest
> locks up", not "QEMU uses locks to synchronize". Suggest to add the
> preposition :)
>
> Peter Lieven <pl@dlhnet.de> writes:
>
>> If io_limits are specified during runtime that exceed the number of operations in flight
>> bs->io_base is not initialized in the else statement in bdrv_exceed_io_limits().
>
> I'm confused.
>
> if ((bs->slice_start < now)
> && (bs->slice_end > now)) {
> bs->slice_end = now + bs->slice_time;
> } else {
> bs->slice_time = 5 * BLOCK_IO_SLICE_TIME;
> bs->slice_start = now;
> bs->slice_end = now + bs->slice_time;
>
> bs->io_base.bytes[is_write] = bs->nr_bytes[is_write];
> bs->io_base.bytes[!is_write] = bs->nr_bytes[!is_write];
>
> bs->io_base.ios[is_write] = bs->nr_ops[is_write];
> bs->io_base.ios[!is_write] = bs->nr_ops[!is_write];
> }
>
> It's set in the else. Could you explain what goes wrong in a bit more
> detail?
If io_limits are enabled while there are I/Os in flight, we do not enter
the else tree as bs->slice_start and bs->slice_end are initialized
in bdrv_io_limits_enable().
In bdrv_exceed_iops_limits() ios_base is then initialized as
ios_base = bs->nr_ops[is_write] - bs->io_base.ios[is_write];
ios_base is then a very high value if the machine runs for some time.
wait_time is then also a very high value as it is calculated as:
wait_time = (ios_base + 1) / iops_limit;
I hope my fix is correct. Maybe there is sth else wrong, but at least this
fixes the locking of the guest for me.
My intention is not to limit I/O in general, just watch it and shape guests
that behave inappropriate.
Peter
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 10:55 ` Paolo Bonzini
@ 2013-01-10 11:02 ` Peter Lieven
0 siblings, 0 replies; 17+ messages in thread
From: Peter Lieven @ 2013-01-10 11:02 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, qemu-devel@nongnu.org
Am 10.01.2013 um 11:55 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> Il 10/01/2013 11:52, Peter Lieven ha scritto:
>>>> Also, perhaps you can just call
>>>>
>>>> bdrv_exceed_io_limits(bs, 0, 0, NULL);
>> won`t this segfault if only one of bps or ions limit is set?
>> in this case it reads wait before returning false in
>> bdrv_exceed_{bps,iops}_limits().
>
> I didn't see any accesses that aren't checked with "if (wait)". Am I
> blind? :)
No, I am ;-)
Peter
>
> Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 10:59 ` Peter Lieven
@ 2013-01-10 11:17 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2013-01-10 11:17 UTC (permalink / raw)
To: Peter Lieven; +Cc: kwolf, Markus Armbruster, qemu-devel@nongnu.org
Il 10/01/2013 11:59, Peter Lieven ha scritto:
> I hope my fix is correct. Maybe there is sth else wrong, but at least this
> fixes the locking of the guest for me.
Yes, I also believe youre fix is correct (only aesthetic remarks).
Paolo
> My intention is not to limit I/O in general, just watch it and shape guests
> that behave inappropriate.
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 10:55 ` Paolo Bonzini
@ 2013-01-10 11:52 ` Markus Armbruster
2013-01-10 11:57 ` Peter Lieven
0 siblings, 1 reply; 17+ messages in thread
From: Markus Armbruster @ 2013-01-10 11:52 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, Peter Lieven, qemu-devel@nongnu.org
Paolo Bonzini <pbonzini@redhat.com> writes:
> Il 10/01/2013 11:45, Markus Armbruster ha scritto:
>>
>>> > If io_limits are specified during runtime that exceed the number
>>> > of operations in flight
>>> > bs->io_base is not initialized in the else statement in
>>> > bdrv_exceed_io_limits().
>> I'm confused.
>>
>> if ((bs->slice_start < now)
>> && (bs->slice_end > now)) {
>> bs->slice_end = now + bs->slice_time;
>> } else {
>> bs->slice_time = 5 * BLOCK_IO_SLICE_TIME;
>> bs->slice_start = now;
>> bs->slice_end = now + bs->slice_time;
>>
>> bs->io_base.bytes[is_write] = bs->nr_bytes[is_write];
>> bs->io_base.bytes[!is_write] = bs->nr_bytes[!is_write];
>>
>> bs->io_base.ios[is_write] = bs->nr_ops[is_write];
>> bs->io_base.ios[!is_write] = bs->nr_ops[!is_write];
>> }
>
> bdrv_io_limits_enable correctly starts a new slice (the first three
> lines of the else), but does not set io_base correctly for that slice.
> Here is how io_base is used:
>
> bytes_base = bs->nr_bytes[is_write] - bs->io_base.bytes[is_write];
> bytes_res = (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
>
> if (bytes_base + bytes_res <= bytes_limit) {
> /* no wait */
> } else {
> /* operation needs to be throttled */
> }
>
> As a result, any I/O operations that are triggered between now and
> bs->slice_end are incorrectly limited. If 10 MB of data has been
> written since the VM was started, QEMU thinks that 10 MB of data has
> been written in this slice.
Work that into the commit message, and I'm happy :)
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 11:52 ` Markus Armbruster
@ 2013-01-10 11:57 ` Peter Lieven
2013-01-10 12:09 ` Paolo Bonzini
0 siblings, 1 reply; 17+ messages in thread
From: Peter Lieven @ 2013-01-10 11:57 UTC (permalink / raw)
To: Markus Armbruster; +Cc: kwolf, Paolo Bonzini, qemu-devel@nongnu.org
Am 10.01.2013 um 12:52 schrieb Markus Armbruster <armbru@redhat.com>:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>
>> Il 10/01/2013 11:45, Markus Armbruster ha scritto:
>>>
>>>>> If io_limits are specified during runtime that exceed the number
>>>>> of operations in flight
>>>>> bs->io_base is not initialized in the else statement in
>>>>> bdrv_exceed_io_limits().
>>> I'm confused.
>>>
>>> if ((bs->slice_start < now)
>>> && (bs->slice_end > now)) {
>>> bs->slice_end = now + bs->slice_time;
>>> } else {
>>> bs->slice_time = 5 * BLOCK_IO_SLICE_TIME;
>>> bs->slice_start = now;
>>> bs->slice_end = now + bs->slice_time;
>>>
>>> bs->io_base.bytes[is_write] = bs->nr_bytes[is_write];
>>> bs->io_base.bytes[!is_write] = bs->nr_bytes[!is_write];
>>>
>>> bs->io_base.ios[is_write] = bs->nr_ops[is_write];
>>> bs->io_base.ios[!is_write] = bs->nr_ops[!is_write];
>>> }
>>
>> bdrv_io_limits_enable correctly starts a new slice (the first three
>> lines of the else), but does not set io_base correctly for that slice.
>> Here is how io_base is used:
>>
>> bytes_base = bs->nr_bytes[is_write] - bs->io_base.bytes[is_write];
>> bytes_res = (unsigned) nb_sectors * BDRV_SECTOR_SIZE;
>>
>> if (bytes_base + bytes_res <= bytes_limit) {
>> /* no wait */
>> } else {
>> /* operation needs to be throttled */
>> }
>>
>> As a result, any I/O operations that are triggered between now and
>> bs->slice_end are incorrectly limited. If 10 MB of data has been
>> written since the VM was started, QEMU thinks that 10 MB of data has
>> been written in this slice.
>
> Work that into the commit message, and I'm happy :)
Paolo, if you agree I would resubmit the patch (using your description).
I would not directly collapse the code to as its not obvious what
bdrv_exceed_io_limits(bs, 0, 0, NULL);
is doing. Maybe this could be done in a later patch.
Peter
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 11:57 ` Peter Lieven
@ 2013-01-10 12:09 ` Paolo Bonzini
2013-01-10 12:12 ` Peter Lieven
0 siblings, 1 reply; 17+ messages in thread
From: Paolo Bonzini @ 2013-01-10 12:09 UTC (permalink / raw)
To: Peter Lieven; +Cc: kwolf, Markus Armbruster, qemu-devel@nongnu.org
Il 10/01/2013 12:57, Peter Lieven ha scritto:
> Paolo, if you agree I would resubmit the patch (using your description).
Using the description is fine. Also at least remove the memset.
> I would not directly collapse the code to as its not obvious what
> bdrv_exceed_io_limits(bs, 0, 0, NULL);
> is doing. Maybe this could be done in a later patch.
You're right that it's not obvious.
But perhaps we do not need to start a slice at all when iolimits are
set. That is, do
bs->slice_start = bs->slice_end = bs->slice_time = 0;
or perhaps even nothing at all since bdrv_io_limits_disable should have
written those exact values.
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 12:09 ` Paolo Bonzini
@ 2013-01-10 12:12 ` Peter Lieven
2013-01-10 12:15 ` Paolo Bonzini
0 siblings, 1 reply; 17+ messages in thread
From: Peter Lieven @ 2013-01-10 12:12 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, Markus Armbruster, qemu-devel@nongnu.org
Am 10.01.2013 um 13:09 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> Il 10/01/2013 12:57, Peter Lieven ha scritto:
>> Paolo, if you agree I would resubmit the patch (using your description).
>
> Using the description is fine. Also at least remove the memset.
>
>> I would not directly collapse the code to as its not obvious what
>> bdrv_exceed_io_limits(bs, 0, 0, NULL);
>> is doing. Maybe this could be done in a later patch.
>
> You're right that it's not obvious.
>
> But perhaps we do not need to start a slice at all when iolimits are
> set. That is, do
>
> bs->slice_start = bs->slice_end = bs->slice_time = 0;
>
> or perhaps even nothing at all since bdrv_io_limits_disable should have
> written those exact values.
Or it was set when the BlockDriverState was initialized.
I am not familiar enough with the io limits code to decide if not starting a slice
is also correct.
Peter
>
> Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 12:12 ` Peter Lieven
@ 2013-01-10 12:15 ` Paolo Bonzini
2013-01-10 12:26 ` Peter Lieven
2013-01-10 12:58 ` Peter Lieven
0 siblings, 2 replies; 17+ messages in thread
From: Paolo Bonzini @ 2013-01-10 12:15 UTC (permalink / raw)
To: Peter Lieven; +Cc: kwolf, Markus Armbruster, qemu-devel@nongnu.org
Il 10/01/2013 13:12, Peter Lieven ha scritto:
>> >
>> > But perhaps we do not need to start a slice at all when iolimits are
>> > set. That is, do
>> >
>> > bs->slice_start = bs->slice_end = bs->slice_time = 0;
>> >
>> > or perhaps even nothing at all since bdrv_io_limits_disable should have
>> > written those exact values.
> Or it was set when the BlockDriverState was initialized.
>
> I am not familiar enough with the io limits code to decide if not starting a slice
> is also correct.
I haven't tested it, but if it works, I think it is better.
Think of it this way: it doesn't matter whether the first I/O operation
comes immediately after limits are set, or 10 seconds later. In the
latter case, bdrv_exceed_io_limits will _already_ start a new slice. It
is better to be consistent and always delay the start of the slice.
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 12:15 ` Paolo Bonzini
@ 2013-01-10 12:26 ` Peter Lieven
2013-01-10 12:58 ` Peter Lieven
1 sibling, 0 replies; 17+ messages in thread
From: Peter Lieven @ 2013-01-10 12:26 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, Markus Armbruster, qemu-devel@nongnu.org
Am 10.01.2013 um 13:15 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> Il 10/01/2013 13:12, Peter Lieven ha scritto:
>>>>
>>>> But perhaps we do not need to start a slice at all when iolimits are
>>>> set. That is, do
>>>>
>>>> bs->slice_start = bs->slice_end = bs->slice_time = 0;
>>>>
>>>> or perhaps even nothing at all since bdrv_io_limits_disable should have
>>>> written those exact values.
>> Or it was set when the BlockDriverState was initialized.
>>
>> I am not familiar enough with the io limits code to decide if not starting a slice
>> is also correct.
>
> I haven't tested it, but if it works, I think it is better.
i will test and report.
Peter
>
> Think of it this way: it doesn't matter whether the first I/O operation
> comes immediately after limits are set, or 10 seconds later. In the
> latter case, bdrv_exceed_io_limits will _already_ start a new slice. It
> is better to be consistent and always delay the start of the slice.
>
> Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 12:15 ` Paolo Bonzini
2013-01-10 12:26 ` Peter Lieven
@ 2013-01-10 12:58 ` Peter Lieven
2013-01-10 13:29 ` Paolo Bonzini
1 sibling, 1 reply; 17+ messages in thread
From: Peter Lieven @ 2013-01-10 12:58 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: kwolf, Markus Armbruster, qemu-devel@nongnu.org
Am 10.01.2013 um 13:15 schrieb Paolo Bonzini <pbonzini@redhat.com>:
> Il 10/01/2013 13:12, Peter Lieven ha scritto:
>>>>
>>>> But perhaps we do not need to start a slice at all when iolimits are
>>>> set. That is, do
>>>>
>>>> bs->slice_start = bs->slice_end = bs->slice_time = 0;
>>>>
>>>> or perhaps even nothing at all since bdrv_io_limits_disable should have
>>>> written those exact values.
>> Or it was set when the BlockDriverState was initialized.
>>
>> I am not familiar enough with the io limits code to decide if not starting a slice
>> is also correct.
>
> I haven't tested it, but if it works, I think it is better.
>
> Think of it this way: it doesn't matter whether the first I/O operation
> comes immediately after limits are set, or 10 seconds later. In the
> latter case, bdrv_exceed_io_limits will _already_ start a new slice. It
> is better to be consistent and always delay the start of the slice.
>
seems to be working as well.
are you happy with:
block: fix initialization in bdrv_io_limits_enable()
Peter
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking
2013-01-10 12:58 ` Peter Lieven
@ 2013-01-10 13:29 ` Paolo Bonzini
0 siblings, 0 replies; 17+ messages in thread
From: Paolo Bonzini @ 2013-01-10 13:29 UTC (permalink / raw)
To: Peter Lieven; +Cc: kwolf, Markus Armbruster, qemu-devel@nongnu.org
Il 10/01/2013 13:58, Peter Lieven ha scritto:
>
> Am 10.01.2013 um 13:15 schrieb Paolo Bonzini <pbonzini@redhat.com>:
>
>> Il 10/01/2013 13:12, Peter Lieven ha scritto:
>>>>>
>>>>> But perhaps we do not need to start a slice at all when iolimits are
>>>>> set. That is, do
>>>>>
>>>>> bs->slice_start = bs->slice_end = bs->slice_time = 0;
>>>>>
>>>>> or perhaps even nothing at all since bdrv_io_limits_disable should have
>>>>> written those exact values.
>>> Or it was set when the BlockDriverState was initialized.
>>>
>>> I am not familiar enough with the io limits code to decide if not starting a slice
>>> is also correct.
>>
>> I haven't tested it, but if it works, I think it is better.
>>
>> Think of it this way: it doesn't matter whether the first I/O operation
>> comes immediately after limits are set, or 10 seconds later. In the
>> latter case, bdrv_exceed_io_limits will _already_ start a new slice. It
>> is better to be consistent and always delay the start of the slice.
>>
>
> seems to be working as well.
>
> are you happy with:
>
> block: fix initialization in bdrv_io_limits_enable()
Sure.
Paolo
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2013-01-10 13:29 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-01-10 9:21 [Qemu-devel] [PATCH] block: init bs->io_base correctly to avoid locking Peter Lieven
2013-01-10 10:19 ` Paolo Bonzini
2013-01-10 10:52 ` Peter Lieven
2013-01-10 10:55 ` Paolo Bonzini
2013-01-10 11:02 ` Peter Lieven
2013-01-10 10:45 ` Markus Armbruster
2013-01-10 10:55 ` Paolo Bonzini
2013-01-10 11:52 ` Markus Armbruster
2013-01-10 11:57 ` Peter Lieven
2013-01-10 12:09 ` Paolo Bonzini
2013-01-10 12:12 ` Peter Lieven
2013-01-10 12:15 ` Paolo Bonzini
2013-01-10 12:26 ` Peter Lieven
2013-01-10 12:58 ` Peter Lieven
2013-01-10 13:29 ` Paolo Bonzini
2013-01-10 10:59 ` Peter Lieven
2013-01-10 11:17 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).