Re: [Qemu-devel] [PATCH 1/2] block: add the support for draining the throttled request queue
On Mon, Feb 20, 2012 at 12:50:30PM +0800, zwu.ker...@gmail.com wrote:
From: Zhi Yong Wu wu...@linux.vnet.ibm.com
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 wu...@linux.vnet.ibm.com
---
block.c | 29 ++---
block_int.h |1 +
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index ae297bb..f78df78 100644
--- a/block.c
+++ b/block.c
@@ -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();
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
Re: [Qemu-devel] [PATCH 1/2] block: add the support for draining the throttled request queue
On Fri, Feb 24, 2012 at 4:49 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
On Mon, Feb 20, 2012 at 12:50:30PM +0800, zwu.ker...@gmail.com wrote:
From: Zhi Yong Wu wu...@linux.vnet.ibm.com
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 wu...@linux.vnet.ibm.com
---
block.c | 29 ++---
block_int.h | 1 +
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index ae297bb..f78df78 100644
--- a/block.c
+++ b/block.c
@@ -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();
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.
If the semantics of bdrv_drain_request() are that no requests are
pending when it returns then we need a loop here.
OK.
BTW bdrv_drain() would be a shorter name for this function.
OK
Stefan
--
Regards,
Zhi Yong Wu
Re: [Qemu-devel] [PATCH 1/2] block: add the support for draining the throttled request queue
On Fri, Feb 24, 2012 at 4:49 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
On Mon, Feb 20, 2012 at 12:50:30PM +0800, zwu.ker...@gmail.com wrote:
From: Zhi Yong Wu wu...@linux.vnet.ibm.com
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 wu...@linux.vnet.ibm.com
---
block.c | 29 ++---
block_int.h | 1 +
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index ae297bb..f78df78 100644
--- a/block.c
+++ b/block.c
@@ -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();
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.
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?
Stefan
--
Regards,
Zhi Yong Wu
Re: [Qemu-devel] [PATCH 1/2] block: add the support for draining the throttled request queue
On Fri, Feb 24, 2012 at 05:25:08PM +0800, Zhi Yong Wu wrote:
On Fri, Feb 24, 2012 at 4:49 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
On Mon, Feb 20, 2012 at 12:50:30PM +0800, zwu.ker...@gmail.com wrote:
From: Zhi Yong Wu wu...@linux.vnet.ibm.com
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 wu...@linux.vnet.ibm.com
---
block.c | 29 ++---
block_int.h | 1 +
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index ae297bb..f78df78 100644
--- a/block.c
+++ b/block.c
@@ -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();
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.
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
Re: [Qemu-devel] [PATCH 1/2] block: add the support for draining the throttled request queue
On 02/24/2012 09:49 AM, Stefan Hajnoczi wrote:
+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();
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
Re: [Qemu-devel] [PATCH 1/2] block: add the support for draining the throttled request queue
On Fri, Feb 24, 2012 at 7:18 PM, Stefan Hajnoczi
stefa...@linux.vnet.ibm.com wrote:
On Fri, Feb 24, 2012 at 05:25:08PM +0800, Zhi Yong Wu wrote:
On Fri, Feb 24, 2012 at 4:49 PM, Stefan Hajnoczi stefa...@gmail.com wrote:
On Mon, Feb 20, 2012 at 12:50:30PM +0800, zwu.ker...@gmail.com wrote:
From: Zhi Yong Wu wu...@linux.vnet.ibm.com
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 wu...@linux.vnet.ibm.com
---
block.c | 29 ++---
block_int.h | 1 +
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index ae297bb..f78df78 100644
--- a/block.c
+++ b/block.c
@@ -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();
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.
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);
Great, thanks.
Stefan
--
Regards,
Zhi Yong Wu
Re: [Qemu-devel] [PATCH 1/2] block: add the support for draining the throttled request queue
Am 20.02.2012 05:50, schrieb zwu.ker...@gmail.com:
From: Zhi Yong Wu wu...@linux.vnet.ibm.com
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 wu...@linux.vnet.ibm.com
---
block.c | 29 ++---
block_int.h |1 +
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index ae297bb..f78df78 100644
--- a/block.c
+++ b/block.c
@@ -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();
Why doesn't qemu_aio_flush() invoke whatever is needed? I think this is
the real bug that should be fixed.
Kevin
Re: [Qemu-devel] [PATCH 1/2] block: add the support for draining the throttled request queue
On Mon, Feb 20, 2012 at 5:26 PM, Kevin Wolf kw...@redhat.com wrote:
Am 20.02.2012 05:50, schrieb zwu.ker...@gmail.com:
From: Zhi Yong Wu wu...@linux.vnet.ibm.com
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 wu...@linux.vnet.ibm.com
---
block.c | 29 ++---
block_int.h | 1 +
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index ae297bb..f78df78 100644
--- a/block.c
+++ b/block.c
@@ -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();
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.
Kevin
--
Regards,
Zhi Yong Wu
Re: [Qemu-devel] [PATCH 1/2] block: add the support for draining the throttled request queue
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.
On Mon, Feb 20, 2012 at 5:26 PM, Kevin Wolf kw...@redhat.com wrote:
Am 20.02.2012 05:50, schrieb zwu.ker...@gmail.com:
From: Zhi Yong Wu wu...@linux.vnet.ibm.com
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 wu...@linux.vnet.ibm.com
---
block.c | 29 ++---
block_int.h | 1 +
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index ae297bb..f78df78 100644
--- a/block.c
+++ b/block.c
@@ -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();
Why doesn't qemu_aio_flush() invoke whatever is needed? I think this is
the real bug that should be fixed.
Kevin
--
Regards,
Zhi Yong Wu
Re: [Qemu-devel] [PATCH 1/2] block: add the support for draining the throttled request queue
Am 20.02.2012 10:29, schrieb Zhi Yong Wu:
On Mon, Feb 20, 2012 at 5:26 PM, Kevin Wolf kw...@redhat.com wrote:
Am 20.02.2012 05:50, schrieb zwu.ker...@gmail.com:
From: Zhi Yong Wu wu...@linux.vnet.ibm.com
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 wu...@linux.vnet.ibm.com
---
block.c | 29 ++---
block_int.h |1 +
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index ae297bb..f78df78 100644
--- a/block.c
+++ b/block.c
@@ -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();
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.
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
Re: [Qemu-devel] [PATCH 1/2] block: add the support for draining the throttled request queue
On Mon, Feb 20, 2012 at 5:39 PM, Kevin Wolf kw...@redhat.com wrote:
Am 20.02.2012 10:29, schrieb Zhi Yong Wu:
On Mon, Feb 20, 2012 at 5:26 PM, Kevin Wolf kw...@redhat.com wrote:
Am 20.02.2012 05:50, schrieb zwu.ker...@gmail.com:
From: Zhi Yong Wu wu...@linux.vnet.ibm.com
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 wu...@linux.vnet.ibm.com
---
block.c | 29 ++---
block_int.h | 1 +
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index ae297bb..f78df78 100644
--- a/block.c
+++ b/block.c
@@ -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();
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.
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
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)
Kevin
--
Regards,
Zhi Yong Wu
[Qemu-devel] [PATCH 1/2] block: add the support for draining the throttled request queue
From: Zhi Yong Wu wu...@linux.vnet.ibm.com
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 wu...@linux.vnet.ibm.com
---
block.c | 29 ++---
block_int.h |1 +
2 files changed, 23 insertions(+), 7 deletions(-)
diff --git a/block.c b/block.c
index ae297bb..f78df78 100644
--- a/block.c
+++ b/block.c
@@ -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)
diff --git a/block_int.h b/block_int.h
index 7946cf6..1311288 100644
--- a/block_int.h
+++ b/block_int.h
@@ -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 _WIN32
int is_windows_drive(const char *filename);
--
1.7.6
