block: add the support for draining the throttled request queue
PATCH 1/2 block: add the support for draining the throttled request queue
From: Zhi Yong Wu <[email protected]>
If one guest has multiple disks with enabling I/O throttling function separately, when draining activities are done, some requests maybe are in the throttled queue; So we need to restart them at first.
Moreover, when only one disk need to be drained such as hotplug out, if another disk still has some requests in its throttled queue, these request should not be effected.
--- block.c | 29 ++++++++++++++++++++++------- block_int.h | 1 + 2 files changed, 23 insertions(+), 7 deletions(-)
block.c +22 -7
@@ -853,25 +853,40 @@ void bdrv_close_all(void) } } -/* -* Wait for pending requests to complete across all BlockDriverStates -* -* This function does not flush data to disk, use bdrv_flush_all() for that -* after calling this function. -*/ -void bdrv_drain_all (void ) +void bdrv_drain_request (BlockDriverState *throttled_bs) {BlockDriverState *bs; + QTAILQ_FOREACH(bs, &bdrv_states, list) { +if (throttled_bs && throttled_bs != bs) { +continue ; + } +qemu_co_queue_restart_all (&bs->throttled_reqs); + } +qemu_aio_flush ();/* If requests are still pending there is a bug somewhere */ QTAILQ_FOREACH(bs, &bdrv_states, list) {assert (QLIST_EMPTY (&bs->tracked_requests)); +if (throttled_bs && throttled_bs != bs) { +continue ; + }assert (qemu_co_queue_empty (&bs->throttled_reqs)); } } +/* +* Wait for pending requests to complete across all BlockDriverStates +* +* This function does not flush data to disk, use bdrv_flush_all() for that +* after calling this function. +*/ +void bdrv_drain_all (void ) +{ +bdrv_drain_request (NULL ); +} +/* make a BlockDriverState anonymous by removing from bdrv_state list. Also, NULL terminate the device_name to prevent double remove */ void bdrv_make_anon (BlockDriverState *bs)
block_int.h +1 -0
@@ -323,6 +323,7 @@ void qemu_aio_release(void *p);void bdrv_set_io_limits (BlockDriverState *bs,BlockIOLimit *io_limits); +void bdrv_drain_request (BlockDriverState *bs);#ifdef _WIN32int is_windows_drive (const char *filename);
Signature · 1 line
1.7.6
Am 20.02.2012 05:50, schrieb [email protected]:
Quoted 29 lines
From: Zhi Yong Wu <[email protected]>
If one guest has multiple disks with enabling I/O throttling function separately, when draining activities are done, some requests maybe are in the throttled queue; So we need to restart them at first.
Moreover, when only one disk need to be drained such as hotplug out, if another disk still has some requests in its throttled queue, these request should not be effected.
Signed-off-by Zhi Yong Wu <[email protected]>--- block.c | 29 ++++++++++++++++++++++------- block_int.h | 1 + 2 files changed, 23 insertions(+), 7 deletions(-)block.c +1
-7@@ -853,25 +853,40 @@ void bdrv_close_all(void) } } -/* -* Wait for pending requests to complete across all BlockDriverStates -* -* This function does not flush data to disk, use bdrv_flush_all() for that -* after calling this function. -*/ -void bdrv_drain_all (void ) +void bdrv_drain_request (BlockDriverState *throttled_bs) {
block.c void bdrv_close_all(void) · line 867
Quoted 10 lines
block.c +7
-0BlockDriverState *bs; + QTAILQ_FOREACH(bs, &bdrv_states, list) { +if (throttled_bs && throttled_bs != bs) { +continue ; + } +qemu_co_queue_restart_all (&bs->throttled_reqs); + } +qemu_aio_flush ();
Why doesn't qemu_aio_flush() invoke whatever is needed? I think this is the real bug that should be fixed.
Kevin
Quoted 32 lines · Kevin Wolf
Am 20.02.2012 05:50, schrieb [email protected]:
Quoted 31 lines
From: Zhi Yong Wu <[email protected]>
If one guest has multiple disks with enabling I/O throttling function separately, when draining activities are done, some requests maybe are in the throttled queue; So we need to restart them at first.
Moreover, when only one disk need to be drained such as hotplug out, if another disk still has some requests in its throttled queue, these request should not be effected.
Signed-off-by Zhi Yong Wu <[email protected]>---
block.c | 29 ++++++++++++++++++++++-------
block_int.h | 1 +2 files changed, 23 insertions(+), 7 deletions(-)block.c +1
-7@@ -853,25 +853,40 @@ void bdrv_close_all(void) } } -/* -* Wait for pending requests to complete across all BlockDriverStates -* -* This function does not flush data to disk, use bdrv_flush_all() for that -* after calling this function. -*/ -void bdrv_drain_all (void ) +void bdrv_drain_request (BlockDriverState *throttled_bs) { BlockDriverState *bs;
Quoted 11 lines
block.c void bdrv_close_all(void) · line 867
Quoted 8 lines
block.c +7
-0+ QTAILQ_FOREACH(bs, &bdrv_states, list) { + if (throttled_bs && throttled_bs != bs) { + continue; + } + qemu_co_queue_restart_all(&bs->throttled_reqs); + } + qemu_aio_flush();Why doesn't
qemu_aio_flush()invoke whatever is needed? I think this is the real bug that should be fixed.
Do you mean that why qemu_aio_flush doesn't invoke those lines of codes above it? As what i said in above comments, when one VM has 2 multiple disks with enabling I/O throttling, if only one disk need to be flushed, only this disk's throttled request need to be drained, and another disk's throttled requests don't need.
Quoted 2 lines
Kevin
Signature · 3 lines
Regards, Zhi Yong Wu
Am 20.02.2012 10:29, schrieb Zhi Yong Wu:
Quoted 44 lines · Kevin Wolf
Quoted 32 lines
Am 20.02.2012 05:50, schrieb [email protected]:
Quoted 31 lines
From: Zhi Yong Wu <[email protected]>
If one guest has multiple disks with enabling I/O throttling function separately, when draining activities are done, some requests maybe are in the throttled queue; So we need to restart them at first.
Moreover, when only one disk need to be drained such as hotplug out, if another disk still has some requests in its throttled queue, these request should not be effected.
Signed-off-by Zhi Yong Wu <[email protected]>--- block.c | 29 ++++++++++++++++++++++------- block_int.h | 1 + 2 files changed, 23 insertions(+), 7 deletions(-)block.c +1
-7@@ -853,25 +853,40 @@ void bdrv_close_all(void) } } -/* -* Wait for pending requests to complete across all BlockDriverStates -* -* This function does not flush data to disk, use bdrv_flush_all() for that -* after calling this function. -*/ -void bdrv_drain_all (void ) +void bdrv_drain_request (BlockDriverState *throttled_bs) {BlockDriverState *bs;Quoted 11 lines
block.c void bdrv_close_all(void) · line 867
Quoted 8 lines
block.c +7
-0+ QTAILQ_FOREACH(bs, &bdrv_states, list) { +if (throttled_bs && throttled_bs != bs) { +continue ; + } +qemu_co_queue_restart_all (&bs->throttled_reqs); + } +qemu_aio_flush ();Why doesn't
qemu_aio_flush()invoke whatever is needed? I think this is the real bug that should be fixed.
Quoted 5 lines
Do you mean that why
qemu_aio_flushdoesn't invoke those lines of codes above it? As what i said in above comments, when one VM has 2 multiple disks with enabling I/O throttling, if only one disk need to be flushed, only this disk's throttled request need to be drained, and another disk's throttled requests don't need.
So this is an optimisation rather than a fix? Would the right optimisation be a qemu_aio_flush_disk() that flushes the requests for a single disk?
Kevin
Quoted 45 lines · Kevin Wolf
Am 20.02.2012 10:29, schrieb Zhi Yong Wu:
Quoted 44 lines · Kevin Wolf
Quoted 43 lines
Am 20.02.2012 05:50, schrieb [email protected]:
Quoted 29 lines
From: Zhi Yong Wu <[email protected]>
If one guest has multiple disks with enabling I/O throttling function separately, when draining activities are done, some requests maybe are in the throttled queue; So we need to restart them at first.
Moreover, when only one disk need to be drained such as hotplug out, if another disk still has some requests in its throttled queue, these request should not be effected.
Signed-off-by Zhi Yong Wu <[email protected]>---
block.c | 29 ++++++++++++++++++++++-------
block_int.h | 1 +2 files changed, 23 insertions(+), 7 deletions(-)block.c +1
-7@@ -853,25 +853,40 @@ void bdrv_close_all(void) } } -/* -* Wait for pending requests to complete across all BlockDriverStates -* -* This function does not flush data to disk, use bdrv_flush_all() for that -* after calling this function. -*/ -void bdrv_drain_all (void ) +void bdrv_drain_request (BlockDriverState *throttled_bs) {block.c void bdrv_close_all(void) · line 867
Quoted 10 lines
block.c +7
-0BlockDriverState *bs; + QTAILQ_FOREACH(bs, &bdrv_states, list) { + if (throttled_bs && throttled_bs != bs) { + continue; + } + qemu_co_queue_restart_all(&bs->throttled_reqs); + } + qemu_aio_flush();Why doesn't
qemu_aio_flush()invoke whatever is needed? I think this is the real bug that should be fixed.
Quoted 7 lines
Quoted 5 lines
Do you mean that why
qemu_aio_flushdoesn't invoke those lines of codes above it? As what i said in above comments, when one VM has 2 multiple disks with enabling I/O throttling, if only one disk need to be flushed, only this disk's throttled request need to be drained, and another disk's throttled requests don't need.So this is an optimisation rather than a fix? Would the right
It is a fix. i think that current handling is wrong when I/O throttling is enabled
Quoted 2 lines
optimisation be a
qemu_aio_flush_disk()that flushes the requests for a single disk?
No. If bdrv_drain_request is invoked with specified bs, it will flush All the throttled requests in the throttled queue for this specific bs
+ pending emulated aio for all disks (in aio engine)
But if bdrv_drain_request(NULL) is invoked, it will flush All the throttled request in the throttled queue for all disks + pending emulated aio for all disks (in aio engine)
Quoted 2 lines
Kevin
Signature · 3 lines
Regards, Zhi Yong Wu
For example, one disk of one guest is hot plugout, its other disks should not be affected if those disks also enable I/O throttling. But if one whole VM need to be stored, all throttled requests for all disks need to be drained.
Quoted 45 lines · Kevin Wolf
Am 20.02.2012 05:50, schrieb [email protected]:
Quoted 29 lines
From: Zhi Yong Wu <[email protected]>
If one guest has multiple disks with enabling I/O throttling function separately, when draining activities are done, some requests maybe are in the throttled queue; So we need to restart them at first.
Moreover, when only one disk need to be drained such as hotplug out, if another disk still has some requests in its throttled queue, these request should not be effected.
Signed-off-by Zhi Yong Wu <[email protected]>---
block.c | 29 ++++++++++++++++++++++-------
block_int.h | 1 +2 files changed, 23 insertions(+), 7 deletions(-)block.c +1
-7@@ -853,25 +853,40 @@ void bdrv_close_all(void) } } -/* -* Wait for pending requests to complete across all BlockDriverStates -* -* This function does not flush data to disk, use bdrv_flush_all() for that -* after calling this function. -*/ -void bdrv_drain_all (void ) +void bdrv_drain_request (BlockDriverState *throttled_bs) {block.c void bdrv_close_all(void) · line 867
Quoted 10 lines
block.c +7
-0BlockDriverState *bs; + QTAILQ_FOREACH(bs, &bdrv_states, list) { + if (throttled_bs && throttled_bs != bs) { + continue; + } + qemu_co_queue_restart_all(&bs->throttled_reqs); + } + qemu_aio_flush();Why doesn't
qemu_aio_flush()invoke whatever is needed? I think this is the real bug that should be fixed.Kevin
Signature · 3 lines
Regards, Zhi Yong Wu
Quoted 29 lines · On Mon, Feb 20, 2012 at 12:50:30PM +0800, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
If one guest has multiple disks with enabling I/O throttling function separately, when draining activities are done, some requests maybe are in the throttled queue; So we need to restart them at first.
Moreover, when only one disk need to be drained such as hotplug out, if another disk still has some requests in its throttled queue, these request should not be effected.
Signed-off-by Zhi Yong Wu <[email protected]>--- block.c | 29 ++++++++++++++++++++++------- block_int.h | 1 + 2 files changed, 23 insertions(+), 7 deletions(-)block.c +1
-7@@ -853,25 +853,40 @@ void bdrv_close_all(void) } } -/* -* Wait for pending requests to complete across all BlockDriverStates -* -* This function does not flush data to disk, use bdrv_flush_all() for that -* after calling this function. -*/ -void bdrv_drain_all (void ) +void bdrv_drain_request (BlockDriverState *throttled_bs) {
block.c void bdrv_close_all(void) · line 867
Quoted 10 lines
block.c +7
-0BlockDriverState *bs; + QTAILQ_FOREACH(bs, &bdrv_states, list) { +if (throttled_bs && throttled_bs != bs) { +continue ; + } +qemu_co_queue_restart_all (&bs->throttled_reqs); + } +qemu_aio_flush ();
Since I/O throttling is still enabled, the restarted requests could enqueue again if they exceed the limit. We could still hit the assert.
If the semantics of bdrv_drain_request() are that no requests are pending when it returns then we need a loop here.
BTW bdrv_drain() would be a shorter name for this function.
Stefan
Quoted 32 lines · Stefan Hajnoczi
Quoted 31 lines · On Mon, Feb 20, 2012 at 12:50:30PM +0800, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
If one guest has multiple disks with enabling I/O throttling function separately, when draining activities are done, some requests maybe are in the throttled queue; So we need to restart them at first.
Moreover, when only one disk need to be drained such as hotplug out, if another disk still has some requests in its throttled queue, these request should not be effected.
Signed-off-by Zhi Yong Wu <[email protected]>---
block.c | 29 ++++++++++++++++++++++-------
block_int.h | 1 +2 files changed, 23 insertions(+), 7 deletions(-)block.c +1
-7@@ -853,25 +853,40 @@ void bdrv_close_all(void) } } -/* -* Wait for pending requests to complete across all BlockDriverStates -* -* This function does not flush data to disk, use bdrv_flush_all() for that -* after calling this function. -*/ -void bdrv_drain_all (void ) +void bdrv_drain_request (BlockDriverState *throttled_bs) { BlockDriverState *bs;
Quoted 11 lines
block.c void bdrv_close_all(void) · line 867
Quoted 8 lines
block.c +7
-0+ QTAILQ_FOREACH(bs, &bdrv_states, list) { + if (throttled_bs && throttled_bs != bs) { + continue; + } + qemu_co_queue_restart_all(&bs->throttled_reqs); + } + qemu_aio_flush();Since I/O throttling is still enabled, the restarted requests could enqueue again if they exceed the limit. We could still hit the assert.
good catch.
Quoted 3 lines
If the semantics of
bdrv_drain_request()are that no requests are pending when it returns then we need a loop here.
OK.
Quoted 2 lines
BTW
bdrv_drain()would be a shorter name for this function.
OK
Quoted 2 lines
Stefan
Signature · 3 lines
Regards, Zhi Yong Wu
Quoted 44 lines · Stefan Hajnoczi
Quoted 29 lines · On Mon, Feb 20, 2012 at 12:50:30PM +0800, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
If one guest has multiple disks with enabling I/O throttling function separately, when draining activities are done, some requests maybe are in the throttled queue; So we need to restart them at first.
Moreover, when only one disk need to be drained such as hotplug out, if another disk still has some requests in its throttled queue, these request should not be effected.
Signed-off-by Zhi Yong Wu <[email protected]>---
block.c | 29 ++++++++++++++++++++++-------
block_int.h | 1 +2 files changed, 23 insertions(+), 7 deletions(-)block.c +1
-7@@ -853,25 +853,40 @@ void bdrv_close_all(void) } } -/* -* Wait for pending requests to complete across all BlockDriverStates -* -* This function does not flush data to disk, use bdrv_flush_all() for that -* after calling this function. -*/ -void bdrv_drain_all (void ) +void bdrv_drain_request (BlockDriverState *throttled_bs) {block.c void bdrv_close_all(void) · line 867
Quoted 10 lines
block.c +7
-0BlockDriverState *bs; + QTAILQ_FOREACH(bs, &bdrv_states, list) { + if (throttled_bs && throttled_bs != bs) { + continue; + } + qemu_co_queue_restart_all(&bs->throttled_reqs); + } + qemu_aio_flush();Since I/O throttling is still enabled, the restarted requests could enqueue again if they exceed the limit. We could still hit the assert.
Quoted 4 lines
If the semantics of
bdrv_drain_request()are that no requests are pending when it returns then we need a loop here.BTW
bdrv_drain()would be a shorter name for this function.
For this function's semantics, i have some concerns.
Is it used to drain all requests of one single disk or all disks for one guest? which is more suitable?
Quoted 2 lines
Stefan
Signature · 3 lines
Regards, Zhi Yong Wu
Quoted 45 lines · Zhi Yong Wu
Quoted 44 lines · Stefan Hajnoczi
Quoted 29 lines · On Mon, Feb 20, 2012 at 12:50:30PM +0800, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
If one guest has multiple disks with enabling I/O throttling function separately, when draining activities are done, some requests maybe are in the throttled queue; So we need to restart them at first.
Moreover, when only one disk need to be drained such as hotplug out, if another disk still has some requests in its throttled queue, these request should not be effected.
Signed-off-by Zhi Yong Wu <[email protected]>---
block.c | 29 ++++++++++++++++++++++-------
block_int.h | 1 +2 files changed, 23 insertions(+), 7 deletions(-)block.c +1
-7@@ -853,25 +853,40 @@ void bdrv_close_all(void) } } -/* -* Wait for pending requests to complete across all BlockDriverStates -* -* This function does not flush data to disk, use bdrv_flush_all() for that -* after calling this function. -*/ -void bdrv_drain_all (void ) +void bdrv_drain_request (BlockDriverState *throttled_bs) {block.c void bdrv_close_all(void) · line 867
Quoted 10 lines
block.c +7
-0BlockDriverState *bs; + QTAILQ_FOREACH(bs, &bdrv_states, list) { + if (throttled_bs && throttled_bs != bs) { + continue; + } + qemu_co_queue_restart_all(&bs->throttled_reqs); + } + qemu_aio_flush();Since I/O throttling is still enabled, the restarted requests could enqueue again if they exceed the limit. We could still hit the assert.
Quoted 7 lines
Quoted 4 lines
If the semantics of
bdrv_drain_request()are that no requests are pending when it returns then we need a loop here.BTW
bdrv_drain()would be a shorter name for this function.For this function's semantics, i have some concerns.
Is it used to drain all requests of one single disk or all disks for one guest? which is more suitable?
Both are useful:
/**
* Complete all pending requests for a block device */
void bdrv_drain(BlockDriverState *bs);
/**
* Complete pending requests for all block devices */
void bdrv_drain_all(void);
Stefan
Quoted 61 lines · Stefan Hajnoczi
Quoted 45 lines · Zhi Yong Wu
Quoted 44 lines · Stefan Hajnoczi
Quoted 29 lines · On Mon, Feb 20, 2012 at 12:50:30PM +0800, [email protected] wrote:
From: Zhi Yong Wu <[email protected]>
If one guest has multiple disks with enabling I/O throttling function separately, when draining activities are done, some requests maybe are in the throttled queue; So we need to restart them at first.
Moreover, when only one disk need to be drained such as hotplug out, if another disk still has some requests in its throttled queue, these request should not be effected.
Signed-off-by Zhi Yong Wu <[email protected]>---
block.c | 29 ++++++++++++++++++++++-------
block_int.h | 1 +2 files changed, 23 insertions(+), 7 deletions(-)block.c +1
-7@@ -853,25 +853,40 @@ void bdrv_close_all(void) } } -/* -* Wait for pending requests to complete across all BlockDriverStates -* -* This function does not flush data to disk, use bdrv_flush_all() for that -* after calling this function. -*/ -void bdrv_drain_all (void ) +void bdrv_drain_request (BlockDriverState *throttled_bs) {block.c void bdrv_close_all(void) · line 867
Quoted 10 lines
block.c +7
-0BlockDriverState *bs; + QTAILQ_FOREACH(bs, &bdrv_states, list) { + if (throttled_bs && throttled_bs != bs) { + continue; + } + qemu_co_queue_restart_all(&bs->throttled_reqs); + } + qemu_aio_flush();Since I/O throttling is still enabled, the restarted requests could enqueue again if they exceed the limit. We could still hit the assert.
Quoted 7 lines
Quoted 4 lines
If the semantics of
bdrv_drain_request()are that no requests are pending when it returns then we need a loop here.BTW
bdrv_drain()would be a shorter name for this function.For this function's semantics, i have some concerns.
Is it used to drain all requests of one single disk or all disks for one guest? which is more suitable?Both are useful:
/**
* Complete all pending requests for a block device */
voidbdrv_drain(BlockDriverState *bs);
Quoted 4 lines
/**
* Complete pending requests for all block devices */
voidbdrv_drain_all(void);
Great, thanks.
Quoted 3 lines
Stefan
Signature · 3 lines
Regards, Zhi Yong Wu
Quoted 4 lines · Stefan Hajnoczi
Quoted 4 lines
Quoted 4 lines
+void
bdrv_drain_request(BlockDriverState *throttled_bs){ BlockDriverState *bs;
Quoted 10 lines
Quoted 8 lines
block.c void bdrv_close_all(void) · line 867
Quoted 8 lines
block.c +7
-0+ QTAILQ_FOREACH(bs, &bdrv_states, list) { +if (throttled_bs && throttled_bs != bs) { +continue ; + } +qemu_co_queue_restart_all (&bs->throttled_reqs); + } +qemu_aio_flush ();Since I/O throttling is still enabled, the restarted requests could enqueue again if they exceed the limit. We could still hit the assert.
Yes, and qemu_aio_flush() rightly doesn't know that there are pending requests, because these are "not there" until the timer fires.
Perhaps the bug is simply that the assert is bogus. If there are throttled requests, we need to invoke qemu_aio_flush() again. The problem with this is that timers don't run inside qemu_aio_wait() so we have to restart them all and we're effectively busy waiting. Not a huge problem since qemu_aio_flush() is rare, but not too nice either.
Paolo