block: fix initialization in bdrv_io_limits_enable()

PATCH Applied Peter Lieven 3 participants 3 messages Jan 11, 2013 – Jan 15, 2013

lore

Share & tools

lore

Peter Lieven

author

2013-01-11 12:30 UTC #0 lore raw

PATCH 1/1 block: fix initialization in bdrv_io_limits_enable()

bdrv_io_limits_enable() starts a new slice, 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. This leads to a I/O lockup in the guest.

We fix this by delaying the start of a new slice to the next call of bdrv_exceed_io_limits().

Signed-off-by Peter Lieven <[email protected]>

---
 block.c |    4 ----
 1 file changed, 4 deletions(-)

block.c +0 -4

index 4e28c55..a40a389 100644
@@ -155,10 +155,6 @@ void bdrv_io_limits_enable(BlockDriverState *bs)
 {
     qemu_co_queue_init(&bs->throttled_reqs);
     bs->block_timer = qemu_new_timer_ns(vm_clock, bdrv_block_timer, bs);
-    bs->slice_time  = 5 * BLOCK_IO_SLICE_TIME;
-    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_limits_enabled = true;
 }

Signature · 1 line

1.7.9.5

Paolo Bonzini

Reviewed-by on v1 1/1

2013-01-11 12:57 UTC #1 lore raw

Il 11/01/2013 13:29, Peter Lieven ha scritto:

Quoted 32 lines

bdrv_io_limits_enable() starts a new slice, 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. This leads to a I/O lockup in the guest.
We fix this by delaying the start of a new slice to the next call of bdrv_exceed_io_limits().
Signed-off-by Peter Lieven <[email protected]>
---
 block.c |    4 ----
 1 file changed, 4 deletions(-)
block.c +0 -0
index 4e28c55..a40a389 100644

block.c void bdrv_io_limits_enable(BlockDriverState *bs) · line 160

Quoted 10 lines

block.c +0 -4

@@ -155,10 +155,6 @@ void bdrv_io_limits_enable(BlockDriverState *bs)
 {
     qemu_co_queue_init(&bs->throttled_reqs);
     bs->block_timer = qemu_new_timer_ns(vm_clock, bdrv_block_timer, bs);
-    bs->slice_time  = 5 * BLOCK_IO_SLICE_TIME;
-    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_limits_enabled = true;
 }

Thanks!

Reviewed-by Paolo Bonzini <[email protected]>

Paolo

Stefan Hajnoczi

Applied

2013-01-15 08:27 UTC #2 lore raw

In reply to Peter Lieven

Quoted 19 lines · Peter Lieven

bdrv_io_limits_enable() starts a new slice, 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. This leads to a I/O lockup in the guest.

Quoted 7 lines

We fix this by delaying the start of a new slice to the next call of bdrv_exceed_io_limits().
Signed-off-by Peter Lieven <[email protected]>
---
 block.c |    4 ----
 1 file changed, 4 deletions(-)

I resolved a merge conflict due to whitespace issues with your patch. Perhaps you copy-pasted from a terminal into an email client. Please try git-send-email(1) or double-check that spaces haven't changed before sending.

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan