[Qemu-devel] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

[Qemu-devel] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

[Fam Zheng]

This supersedes:

http://patchwork.ozlabs.org/patch/491415/

and [1] which is currently in Jeff's tree.

Although [1] fixed the QMP responsiveness, Alexandre DERUMIER reported that
guest responsiveness still suffers when we are busy in the initial dirty bitmap
scanning loop of mirror job. That is because 1) we issue too many lseeks; 2) we
only sleep for 0 ns which turns out quite ineffective in yielding BQL to vcpu
threads.  Both are fixed.

To reproduce: start a guest, attach a 10G raw image, then mirror it.  Your
guest will immediately start to stutter (with patch [1] testing on a local ext4
raw image, and "while echo -n .; do sleep 0.05; done" in guest console).

This series adds block_job_relax_cpu as suggested by Stefan Hajnoczi and uses
it in mirror job; and lets bdrv_is_allocated_above return a larger p_num as
suggested by Paolo Bonzini (although it's done without changing the API).

[1]: http://patchwork.ozlabs.org/patch/471656/ "block/mirror: Sleep
     periodically during bitmap scanning"

Fam Zheng (3):
  blockjob: Introduce block_job_relax_cpu
  mirror: Use block_job_relax_cpu during bitmap scanning
  mirror: Speed up bitmap initial scanning

 block/mirror.c           | 17 +++++++----------
 include/block/blockjob.h | 16 ++++++++++++++++
 2 files changed, 23 insertions(+), 10 deletions(-)

-- 
2.4.3

[Qemu-devel] [PATCH 1/3] blockjob: Introduce block_job_relax_cpu

[Fam Zheng]

block_job_sleep_ns is called by block job coroutines to yield the
execution to VCPU threads and monitor etc. It is pointless to sleep for
0 or a few nanoseconds, because that equals to a "yield + enter" with no
intermission in between (the timer fires immediately in the same
iteration of event loop), which means other code still doesn't get a
fair share of main loop / BQL.

Introduce block_job_relax_cpu which will at least for
BLOCK_JOB_RELAX_CPU_NS. Existing block_job_sleep_ns(job, 0) callers can
be replaced by this later.

Reported-by: Alexandre DERUMIER <aderumier@odiso.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 include/block/blockjob.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 57d8ef1..53ac4f4 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -157,6 +157,22 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
  */
 void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns);
 
+#define BLOCK_JOB_RELAX_CPU_NS 10000000L
+
+/**
+ * block_job_relax_cpu:
+ * @job: The job that calls the function.
+ *
+ * Sleep a little to avoid intensive cpu time occupation. Block jobs should
+ * call this or block_job_sleep_ns (for more precision, but note that 0 ns is
+ * usually not enought) periodically, otherwise the QMP and VCPU could starve
+ * on CPU and/or BQL.
+ */
+static inline void block_job_relax_cpu(BlockJob *job)
+{
+    return block_job_sleep_ns(job, QEMU_CLOCK_REALTIME, BLOCK_JOB_RELAX_CPU_NS);
+}
+
 /**
  * block_job_yield:
  * @job: The job that calls the function.
-- 
2.4.3

Re: [Qemu-devel] [Qemu-block] [PATCH 1/3] blockjob: Introduce block_job_relax_cpu

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1914 bytes --]

On Thu, Jul 09, 2015 at 11:47:56AM +0800, Fam Zheng wrote:
> block_job_sleep_ns is called by block job coroutines to yield the
> execution to VCPU threads and monitor etc. It is pointless to sleep for
> 0 or a few nanoseconds, because that equals to a "yield + enter" with no
> intermission in between (the timer fires immediately in the same
> iteration of event loop), which means other code still doesn't get a
> fair share of main loop / BQL.
> 
> Introduce block_job_relax_cpu which will at least for
> BLOCK_JOB_RELAX_CPU_NS. Existing block_job_sleep_ns(job, 0) callers can
> be replaced by this later.
> 
> Reported-by: Alexandre DERUMIER <aderumier@odiso.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  include/block/blockjob.h | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h
> index 57d8ef1..53ac4f4 100644
> --- a/include/block/blockjob.h
> +++ b/include/block/blockjob.h
> @@ -157,6 +157,22 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
>   */
>  void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns);
>  
> +#define BLOCK_JOB_RELAX_CPU_NS 10000000L

By the way, why did you choose 10 milliseconds?  That is quite long.

If this function is called once per 10 ms disk I/O operations then we
lose 50% utilization.  1 ms or less would be reasonable.

> +
> +/**
> + * block_job_relax_cpu:
> + * @job: The job that calls the function.
> + *
> + * Sleep a little to avoid intensive cpu time occupation. Block jobs should
> + * call this or block_job_sleep_ns (for more precision, but note that 0 ns is
> + * usually not enought) periodically, otherwise the QMP and VCPU could starve

s/enought/enough/

> + * on CPU and/or BQL.
> + */
> +static inline void block_job_relax_cpu(BlockJob *job)

coroutine_fn is missing.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 1/3] blockjob: Introduce block_job_relax_cpu

[Alexandre DERUMIER]

Hi Stefan,

>>By the way, why did you choose 10 milliseconds?  That is quite long.
>>
>>If this function is called once per 10 ms disk I/O operations then we
>>lose 50% utilization.  1 ms or less would be reasonable.

>From my tests, 1ms is not enough, It still hanging in guest or qmp queries.
10ms give me optimal balance between bitmap scan speed and guest responsiveness.

I don't known if this can be compute automaticaly ? (maybe it's depend of disk lseek speed and cpu speed).


----- Mail original -----
De: "Stefan Hajnoczi" <stefanha@gmail.com>
À: "Fam Zheng" <famz@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, qemu-block@nongnu.org
Envoyé: Jeudi 9 Juillet 2015 14:54:55
Objet: Re: [Qemu-devel] [Qemu-block] [PATCH 1/3] blockjob: Introduce	block_job_relax_cpu

On Thu, Jul 09, 2015 at 11:47:56AM +0800, Fam Zheng wrote: 
> block_job_sleep_ns is called by block job coroutines to yield the 
> execution to VCPU threads and monitor etc. It is pointless to sleep for 
> 0 or a few nanoseconds, because that equals to a "yield + enter" with no 
> intermission in between (the timer fires immediately in the same 
> iteration of event loop), which means other code still doesn't get a 
> fair share of main loop / BQL. 
> 
> Introduce block_job_relax_cpu which will at least for 
> BLOCK_JOB_RELAX_CPU_NS. Existing block_job_sleep_ns(job, 0) callers can 
> be replaced by this later. 
> 
> Reported-by: Alexandre DERUMIER <aderumier@odiso.com> 
> Signed-off-by: Fam Zheng <famz@redhat.com> 
> --- 
> include/block/blockjob.h | 16 ++++++++++++++++ 
> 1 file changed, 16 insertions(+) 
> 
> diff --git a/include/block/blockjob.h b/include/block/blockjob.h 
> index 57d8ef1..53ac4f4 100644 
> --- a/include/block/blockjob.h 
> +++ b/include/block/blockjob.h 
> @@ -157,6 +157,22 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs, 
> */ 
> void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns); 
> 
> +#define BLOCK_JOB_RELAX_CPU_NS 10000000L 

By the way, why did you choose 10 milliseconds? That is quite long. 

If this function is called once per 10 ms disk I/O operations then we 
lose 50% utilization. 1 ms or less would be reasonable. 

> + 
> +/** 
> + * block_job_relax_cpu: 
> + * @job: The job that calls the function. 
> + * 
> + * Sleep a little to avoid intensive cpu time occupation. Block jobs should 
> + * call this or block_job_sleep_ns (for more precision, but note that 0 ns is 
> + * usually not enought) periodically, otherwise the QMP and VCPU could starve 

s/enought/enough/ 

> + * on CPU and/or BQL. 
> + */ 
> +static inline void block_job_relax_cpu(BlockJob *job) 

coroutine_fn is missing. 

Re: [Qemu-devel] [Qemu-block] [PATCH 1/3] blockjob: Introduce block_job_relax_cpu

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 541 bytes --]

On Fri, Jul 10, 2015 at 05:42:48AM +0200, Alexandre DERUMIER wrote:
> >>By the way, why did you choose 10 milliseconds?  That is quite long.
> >>
> >>If this function is called once per 10 ms disk I/O operations then we
> >>lose 50% utilization.  1 ms or less would be reasonable.
> 
> From my tests, 1ms is not enough, It still hanging in guest or qmp queries.
> 10ms give me optimal balance between bitmap scan speed and guest responsiveness.

Then I don't fully understand the bug.

Fam: can you explain why 1ms isn't enough?

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 1/3] blockjob: Introduce block_job_relax_cpu

[Fam Zheng]

On Tue, 07/14 13:31, Stefan Hajnoczi wrote:
> On Fri, Jul 10, 2015 at 05:42:48AM +0200, Alexandre DERUMIER wrote:
> > >>By the way, why did you choose 10 milliseconds?  That is quite long.
> > >>
> > >>If this function is called once per 10 ms disk I/O operations then we
> > >>lose 50% utilization.  1 ms or less would be reasonable.
> > 
> > From my tests, 1ms is not enough, It still hanging in guest or qmp queries.
> > 10ms give me optimal balance between bitmap scan speed and guest responsiveness.
> 
> Then I don't fully understand the bug.
> 
> Fam: can you explain why 1ms isn't enough?

In Alexandre's case, I suppose it's because the lseek is so slow that sleeping
for 1ms would still let mirror coroutine to occupy, say, 90% of CPU time, so
guest IO stutters. Perhaps we could move lseek to thread pool in the future.

Anyway, 10ms wasn't a deliberate choice, because I didn't have one. I agree in
other cases, 1ms or less should be enough.

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH 1/3] blockjob: Introduce block_job_relax_cpu

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1174 bytes --]

On Wed, Jul 15, 2015 at 06:32:54PM +0800, Fam Zheng wrote:
> On Tue, 07/14 13:31, Stefan Hajnoczi wrote:
> > On Fri, Jul 10, 2015 at 05:42:48AM +0200, Alexandre DERUMIER wrote:
> > > >>By the way, why did you choose 10 milliseconds?  That is quite long.
> > > >>
> > > >>If this function is called once per 10 ms disk I/O operations then we
> > > >>lose 50% utilization.  1 ms or less would be reasonable.
> > > 
> > > From my tests, 1ms is not enough, It still hanging in guest or qmp queries.
> > > 10ms give me optimal balance between bitmap scan speed and guest responsiveness.
> > 
> > Then I don't fully understand the bug.
> > 
> > Fam: can you explain why 1ms isn't enough?
> 
> In Alexandre's case, I suppose it's because the lseek is so slow that sleeping
> for 1ms would still let mirror coroutine to occupy, say, 90% of CPU time, so
> guest IO stutters. Perhaps we could move lseek to thread pool in the future.

Right, that's the real problem here.  If lseek is done in a worker
thread than the coroutine yields in the meantime and the responsiveness
problem is solved.

This sounds like an important fix in the early 2.5 release cycle.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH 2/3] mirror: Use block_job_relax_cpu during bitmap scanning

[Fam Zheng]

Sleeping for 0 second may not be as effective as we want, use
block_job_relax_cpu.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/mirror.c b/block/mirror.c
index 62db031..ca55578 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -438,7 +438,7 @@ static void coroutine_fn mirror_run(void *opaque)
 
             if (now - last_pause_ns > SLICE_TIME) {
                 last_pause_ns = now;
-                block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, 0);
+                block_job_relax_cpu(&s->common);
             }
 
             if (block_job_is_cancelled(&s->common)) {
-- 
2.4.3

Re: [Qemu-devel] [Qemu-block] [PATCH 2/3] mirror: Use block_job_relax_cpu during bitmap scanning

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 336 bytes --]

On Thu, Jul 09, 2015 at 11:47:57AM +0800, Fam Zheng wrote:
> Sleeping for 0 second may not be as effective as we want, use
> block_job_relax_cpu.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/mirror.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

[Qemu-devel] [PATCH 3/3] mirror: Speed up bitmap initial scanning

[Fam Zheng]

Limiting to sectors_per_chunk for each bdrv_is_allocated_above is slow,
because the underlying protocol driver would issue much more queries
than necessary. We should coalesce the query.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/mirror.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index ca55578..e8cb592 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -371,7 +371,7 @@ static void coroutine_fn mirror_run(void *opaque)
     MirrorBlockJob *s = opaque;
     MirrorExitData *data;
     BlockDriverState *bs = s->common.bs;
-    int64_t sector_num, end, sectors_per_chunk, length;
+    int64_t sector_num, end, length;
     uint64_t last_pause_ns;
     BlockDriverInfo bdi;
     char backing_filename[2]; /* we only need 2 characters because we are only
@@ -425,7 +425,6 @@ static void coroutine_fn mirror_run(void *opaque)
         goto immediate_exit;
     }
 
-    sectors_per_chunk = s->granularity >> BDRV_SECTOR_BITS;
     mirror_free_init(s);
 
     last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
@@ -433,7 +432,9 @@ static void coroutine_fn mirror_run(void *opaque)
         /* First part, loop on the sectors and initialize the dirty bitmap.  */
         BlockDriverState *base = s->base;
         for (sector_num = 0; sector_num < end; ) {
-            int64_t next = (sector_num | (sectors_per_chunk - 1)) + 1;
+            /* Just to make sure we are not exceeding int limit. */
+            int nb_sectors = MIN(INT_MAX >> BDRV_SECTOR_BITS,
+                                 end - sector_num);
             int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 
             if (now - last_pause_ns > SLICE_TIME) {
@@ -444,9 +445,7 @@ static void coroutine_fn mirror_run(void *opaque)
             if (block_job_is_cancelled(&s->common)) {
                 goto immediate_exit;
             }
-
-            ret = bdrv_is_allocated_above(bs, base,
-                                          sector_num, next - sector_num, &n);
+            ret = bdrv_is_allocated_above(bs, base, sector_num, nb_sectors, &n);
 
             if (ret < 0) {
                 goto immediate_exit;
@@ -455,10 +454,8 @@ static void coroutine_fn mirror_run(void *opaque)
             assert(n > 0);
             if (ret == 1) {
                 bdrv_set_dirty_bitmap(s->dirty_bitmap, sector_num, n);
-                sector_num = next;
-            } else {
-                sector_num += n;
             }
+            sector_num += n;
         }
     }
 
-- 
2.4.3

Re: [Qemu-devel] [PATCH 3/3] mirror: Speed up bitmap initial scanning

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 459 bytes --]

On Thu, Jul 09, 2015 at 11:47:58AM +0800, Fam Zheng wrote:
> Limiting to sectors_per_chunk for each bdrv_is_allocated_above is slow,
> because the underlying protocol driver would issue much more queries
> than necessary. We should coalesce the query.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/mirror.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 3/3] mirror: Speed up bitmap initial scanning

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

On Thu, Jul 09, 2015 at 11:47:58AM +0800, Fam Zheng wrote:
> Limiting to sectors_per_chunk for each bdrv_is_allocated_above is slow,
> because the underlying protocol driver would issue much more queries
> than necessary. We should coalesce the query.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block/mirror.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)

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

Stefan

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

[Alexandre DERUMIER]

Hi, 

I have tested them, they works fine here (tested with source :raw on nfs, raw on ext3, and ceph/rbd device).

Patch 3 give me a big boost with an empty 10GB source raw on ext3. (take some seconds vs some minutes)


----- Mail original -----
De: "Fam Zheng" <famz@redhat.com>
À: "qemu-devel" <qemu-devel@nongnu.org>
Cc: "Kevin Wolf" <kwolf@redhat.com>, "Jeff Cody" <jcody@redhat.com>, qemu-block@nongnu.org
Envoyé: Jeudi 9 Juillet 2015 05:47:55
Objet: [Qemu-devel] [PATCH 0/3] mirror: Fix guest responsiveness during	bitmap scan

This supersedes: 

http://patchwork.ozlabs.org/patch/491415/ 

and [1] which is currently in Jeff's tree. 

Although [1] fixed the QMP responsiveness, Alexandre DERUMIER reported that 
guest responsiveness still suffers when we are busy in the initial dirty bitmap 
scanning loop of mirror job. That is because 1) we issue too many lseeks; 2) we 
only sleep for 0 ns which turns out quite ineffective in yielding BQL to vcpu 
threads. Both are fixed. 

To reproduce: start a guest, attach a 10G raw image, then mirror it. Your 
guest will immediately start to stutter (with patch [1] testing on a local ext4 
raw image, and "while echo -n .; do sleep 0.05; done" in guest console). 

This series adds block_job_relax_cpu as suggested by Stefan Hajnoczi and uses 
it in mirror job; and lets bdrv_is_allocated_above return a larger p_num as 
suggested by Paolo Bonzini (although it's done without changing the API). 

[1]: http://patchwork.ozlabs.org/patch/471656/ "block/mirror: Sleep 
periodically during bitmap scanning" 

Fam Zheng (3): 
blockjob: Introduce block_job_relax_cpu 
mirror: Use block_job_relax_cpu during bitmap scanning 
mirror: Speed up bitmap initial scanning 

block/mirror.c | 17 +++++++---------- 
include/block/blockjob.h | 16 ++++++++++++++++ 
2 files changed, 23 insertions(+), 10 deletions(-) 

-- 
2.4.3 

Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1715 bytes --]

On Thu, Jul 09, 2015 at 11:47:55AM +0800, Fam Zheng wrote:
> This supersedes:
> 
> http://patchwork.ozlabs.org/patch/491415/
> 
> and [1] which is currently in Jeff's tree.
> 
> Although [1] fixed the QMP responsiveness, Alexandre DERUMIER reported that
> guest responsiveness still suffers when we are busy in the initial dirty bitmap
> scanning loop of mirror job. That is because 1) we issue too many lseeks; 2) we
> only sleep for 0 ns which turns out quite ineffective in yielding BQL to vcpu
> threads.  Both are fixed.
> 
> To reproduce: start a guest, attach a 10G raw image, then mirror it.  Your
> guest will immediately start to stutter (with patch [1] testing on a local ext4
> raw image, and "while echo -n .; do sleep 0.05; done" in guest console).
> 
> This series adds block_job_relax_cpu as suggested by Stefan Hajnoczi and uses
> it in mirror job; and lets bdrv_is_allocated_above return a larger p_num as
> suggested by Paolo Bonzini (although it's done without changing the API).
> 
> [1]: http://patchwork.ozlabs.org/patch/471656/ "block/mirror: Sleep
>      periodically during bitmap scanning"
> 
> Fam Zheng (3):
>   blockjob: Introduce block_job_relax_cpu
>   mirror: Use block_job_relax_cpu during bitmap scanning
>   mirror: Speed up bitmap initial scanning
> 
>  block/mirror.c           | 17 +++++++----------
>  include/block/blockjob.h | 16 ++++++++++++++++
>  2 files changed, 23 insertions(+), 10 deletions(-)

This patch only converts the mirror block job to use the new relax
function.  The other block jobs (stream, backup, commit) are still using
a 0 ns delay and are therefore broken.  They should probably be
converted in the same series.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

[Alexandre DERUMIER]

>>The other block jobs (stream, backup, commit) are still using 
>>a 0 ns delay and are therefore broken. 

Also in mirror.c, they are block_job_sleep_ns with delay_ns, where delay_ns can be 0.

I have seen sometime,some server/qmp hangs, on a pretty old server/slow disks.
(no more in bitmap scan with theses last patches, but on the block mirror itself)


       if (!s->synced) {
            block_job_sleep_ns(&s->common, QEMU_CLOCK_REALTIME, delay_ns);
            if (block_job_is_cancelled(&s->common)) {
                break;
            }
        } else if (!should_complete) {
            delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);


----- Mail original -----
De: "Stefan Hajnoczi" <stefanha@gmail.com>
À: "Fam Zheng" <famz@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, qemu-block@nongnu.org
Envoyé: Jeudi 9 Juillet 2015 15:02:08
Objet: Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

On Thu, Jul 09, 2015 at 11:47:55AM +0800, Fam Zheng wrote: 
> This supersedes: 
> 
> http://patchwork.ozlabs.org/patch/491415/ 
> 
> and [1] which is currently in Jeff's tree. 
> 
> Although [1] fixed the QMP responsiveness, Alexandre DERUMIER reported that 
> guest responsiveness still suffers when we are busy in the initial dirty bitmap 
> scanning loop of mirror job. That is because 1) we issue too many lseeks; 2) we 
> only sleep for 0 ns which turns out quite ineffective in yielding BQL to vcpu 
> threads. Both are fixed. 
> 
> To reproduce: start a guest, attach a 10G raw image, then mirror it. Your 
> guest will immediately start to stutter (with patch [1] testing on a local ext4 
> raw image, and "while echo -n .; do sleep 0.05; done" in guest console). 
> 
> This series adds block_job_relax_cpu as suggested by Stefan Hajnoczi and uses 
> it in mirror job; and lets bdrv_is_allocated_above return a larger p_num as 
> suggested by Paolo Bonzini (although it's done without changing the API). 
> 
> [1]: http://patchwork.ozlabs.org/patch/471656/ "block/mirror: Sleep 
> periodically during bitmap scanning" 
> 
> Fam Zheng (3): 
> blockjob: Introduce block_job_relax_cpu 
> mirror: Use block_job_relax_cpu during bitmap scanning 
> mirror: Speed up bitmap initial scanning 
> 
> block/mirror.c | 17 +++++++---------- 
> include/block/blockjob.h | 16 ++++++++++++++++ 
> 2 files changed, 23 insertions(+), 10 deletions(-) 

This patch only converts the mirror block job to use the new relax 
function. The other block jobs (stream, backup, commit) are still using 
a 0 ns delay and are therefore broken. They should probably be 
converted in the same series. 

Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

[Fam Zheng]

On Thu, 07/09 14:02, Stefan Hajnoczi wrote:
> This patch only converts the mirror block job to use the new relax
> function.  The other block jobs (stream, backup, commit) are still using
> a 0 ns delay and are therefore broken.  They should probably be
> converted in the same series.

That's because they all can be perfectly mitigated by setting a reasonable
"speed" that matchs the host horsepower. Thinking about this again, I doubt
that lengthening the duration with a hardcoded value benifits everyone; and
before Alexandre's reply on old server/slow disks, I don't recall any report,
because the coroutines would already yield often enough, when waiting for IO to
complete. So I am not sure whether they're broken in practice.

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

[Alexandre DERUMIER]

>>Thinking about this again, I doubt
>>that lengthening the duration with a hardcoded value benifits everyone; and
>>before Alexandre's reply on old server/slow disks

With 1ms sleep, I can reproduce the hang 100% with a fast cpu (xeon e5 v3 3,1ghz) and source raw file on nfs.


----- Mail original -----
De: "Fam Zheng" <famz@redhat.com>
À: "Stefan Hajnoczi" <stefanha@gmail.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, qemu-block@nongnu.org
Envoyé: Vendredi 10 Juillet 2015 08:43:50
Objet: Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

On Thu, 07/09 14:02, Stefan Hajnoczi wrote: 
> This patch only converts the mirror block job to use the new relax 
> function. The other block jobs (stream, backup, commit) are still using 
> a 0 ns delay and are therefore broken. They should probably be 
> converted in the same series. 

That's because they all can be perfectly mitigated by setting a reasonable 
"speed" that matchs the host horsepower. Thinking about this again, I doubt 
that lengthening the duration with a hardcoded value benifits everyone; and 
before Alexandre's reply on old server/slow disks, I don't recall any report, 
because the coroutines would already yield often enough, when waiting for IO to 
complete. So I am not sure whether they're broken in practice. 

Fam 

Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

[Fam Zheng]

On Fri, 07/10 08:54, Alexandre DERUMIER wrote:
> >>Thinking about this again, I doubt
> >>that lengthening the duration with a hardcoded value benifits everyone; and
> >>before Alexandre's reply on old server/slow disks
> 
> With 1ms sleep, I can reproduce the hang 100% with a fast cpu (xeon e5 v3 3,1ghz) and source raw file on nfs.

Does it completely hang? I can't reproduce this with my machine mirroring from
nfs to local: the guest runs smoothly.  What does "perf top" show?

Fam

> 
> 
> ----- Mail original -----
> De: "Fam Zheng" <famz@redhat.com>
> À: "Stefan Hajnoczi" <stefanha@gmail.com>
> Cc: "Kevin Wolf" <kwolf@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, qemu-block@nongnu.org
> Envoyé: Vendredi 10 Juillet 2015 08:43:50
> Objet: Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan
> 
> On Thu, 07/09 14:02, Stefan Hajnoczi wrote: 
> > This patch only converts the mirror block job to use the new relax 
> > function. The other block jobs (stream, backup, commit) are still using 
> > a 0 ns delay and are therefore broken. They should probably be 
> > converted in the same series. 
> 
> That's because they all can be perfectly mitigated by setting a reasonable 
> "speed" that matchs the host horsepower. Thinking about this again, I doubt 
> that lengthening the duration with a hardcoded value benifits everyone; and 
> before Alexandre's reply on old server/slow disks, I don't recall any report, 
> because the coroutines would already yield often enough, when waiting for IO to 
> complete. So I am not sure whether they're broken in practice. 
> 
> Fam 

Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

[Alexandre DERUMIER]

>>Does it completely hang? 
yes, can't ping network, and vnc console is frozen.


>>What does "perf top" show? 
I'll do test today . (I'm going to vacation this night,I'll try to send results before that)

----- Mail original -----
De: "Fam Zheng" <famz@redhat.com>
À: "aderumier" <aderumier@odiso.com>
Cc: "Stefan Hajnoczi" <stefanha@gmail.com>, "Kevin Wolf" <kwolf@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, qemu-block@nongnu.org
Envoyé: Vendredi 10 Juillet 2015 09:13:33
Objet: Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

On Fri, 07/10 08:54, Alexandre DERUMIER wrote: 
> >>Thinking about this again, I doubt 
> >>that lengthening the duration with a hardcoded value benifits everyone; and 
> >>before Alexandre's reply on old server/slow disks 
> 
> With 1ms sleep, I can reproduce the hang 100% with a fast cpu (xeon e5 v3 3,1ghz) and source raw file on nfs. 

Does it completely hang? I can't reproduce this with my machine mirroring from 
nfs to local: the guest runs smoothly. What does "perf top" show? 

Fam 

> 
> 
> ----- Mail original ----- 
> De: "Fam Zheng" <famz@redhat.com> 
> À: "Stefan Hajnoczi" <stefanha@gmail.com> 
> Cc: "Kevin Wolf" <kwolf@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, qemu-block@nongnu.org 
> Envoyé: Vendredi 10 Juillet 2015 08:43:50 
> Objet: Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan 
> 
> On Thu, 07/09 14:02, Stefan Hajnoczi wrote: 
> > This patch only converts the mirror block job to use the new relax 
> > function. The other block jobs (stream, backup, commit) are still using 
> > a 0 ns delay and are therefore broken. They should probably be 
> > converted in the same series. 
> 
> That's because they all can be perfectly mitigated by setting a reasonable 
> "speed" that matchs the host horsepower. Thinking about this again, I doubt 
> that lengthening the duration with a hardcoded value benifits everyone; and 
> before Alexandre's reply on old server/slow disks, I don't recall any report, 
> because the coroutines would already yield often enough, when waiting for IO to 
> complete. So I am not sure whether they're broken in practice. 
> 
> Fam 

Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

[Alexandre DERUMIER]

Hi again,

I have redone a lot of tests,

with raw on nfs
---------------

Patch 3/3, fix my problem (not apply patch 1/3 and patch 2/3).

without patch 3/3, I'm seeing a lot of lseek, can take some minutes with guest hang.

with patch 3/3, it almost take no time to generate the bitmap, no guest hang . (I have tested with differents raw files, different sizes)


with raw on ext3
----------------
Here, this hurt a lot. Not with every disk, but with some disks is totaly hanging the guest

-----------------------
  PID USER      PR  NI    VIRT    RES    SHR S  %CPU %MEM     TIME+ COMMAND    
 4387 root      20   0 8985788 288900  12532 R  99.3  1.8   2:50.82 qemu    



  10.27%  [kernel]                  [k] ext4_es_find_delayed_extent_range
   9.64%  [kernel]                  [k] _raw_read_lock
   9.17%  [kernel]                  [k] ext4_es_lookup_extent
   6.63%  [kernel]                  [k] down_read
   5.49%  [kernel]                  [k] ext4_map_blocks
   4.82%  [kernel]                  [k] up_read
   3.09%  [kernel]                  [k] ext4_ind_map_blocks
   2.58%  [kernel]                  [k] ext4_block_to_path.isra.9
   2.18%  [kernel]                  [k] kvm_arch_vcpu_ioctl_run
   1.99%  [kernel]                  [k] vmx_get_segment
   1.99%  [kernel]                  [k] ext4_llseek
   1.62%  [kernel]                  [k] ext4_get_branch
   1.43%  [kernel]                  [k] vmx_segment_access_rights.isra.28.part.29
   1.23%  [kernel]                  [k] x86_decode_insn
   1.00%  [kernel]                  [k] copy_user_generic_string
   0.83%  [kernel]                  [k] x86_emulate_instruction
   0.81%  [kernel]                  [k] rmode_segment_valid
   0.78%  [kernel]                  [k] gfn_to_hva_prot
   0.74%  [kernel]                  [k] __es_tree_search
   0.73%  [kernel]                  [k] do_insn_fetch
   0.70%  [kernel]                  [k] vmx_vcpu_run
   0.69%  [kernel]                  [k] vmx_read_guest_seg_selector
   0.67%  [kernel]                  [k] vmcs_writel
   0.64%  [kernel]                  [k] init_emulate_ctxt
   0.62%  [kernel]                  [k] native_write_msr_safe
   0.61%  [kernel]                  [k] kvm_apic_has_interrupt
   0.57%  [kernel]                  [k] vmx_handle_external_intr
   0.57%  [kernel]                  [k] x86_emulate_insn
   0.56%  [kernel]                  [k] vmx_handle_exit
   0.50%  [kernel]                  [k] native_read_tsc
   0.47%  [kernel]                  [k] _cond_resched
   0.46%  [kernel]                  [k] decode_operand
   0.44%  [kernel]                  [k] __srcu_read_lock
   0.41%  [kernel]                  [k] __linearize.isra.25
   0.41%  [kernel]                  [k] vmx_read_l1_tsc
   0.39%  [kernel]                  [k] clear_page_c
   0.39%  [kernel]                  [k] vmx_get_interrupt_shadow
   0.36%  [kernel]                  [k] vmx_set_interrupt_shadow


Seem to be a known kernel bug:

http://qemu.11.n7.nabble.com/Re-2-Strange-problems-with-lseek-in-qemu-img-map-td334414.html

http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=14516bb7bb6ffbd49f35389f9ece3b2045ba5815




So, I think patch 3/3 is enough. (could be great to have it for qemu 2.4)

Thanks again Fam for your hard work.

Regards,

Alexandre


----- Mail original -----
De: "aderumier" <aderumier@odiso.com>
À: "Fam Zheng" <famz@redhat.com>
Cc: "Kevin Wolf" <kwolf@redhat.com>, "Stefan Hajnoczi" <stefanha@gmail.com>, "qemu-devel" <qemu-devel@nongnu.org>, qemu-block@nongnu.org
Envoyé: Vendredi 10 Juillet 2015 12:36:40
Objet: Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

>>Does it completely hang? 
yes, can't ping network, and vnc console is frozen. 


>>What does "perf top" show? 
I'll do test today . (I'm going to vacation this night,I'll try to send results before that) 

----- Mail original ----- 
De: "Fam Zheng" <famz@redhat.com> 
À: "aderumier" <aderumier@odiso.com> 
Cc: "Stefan Hajnoczi" <stefanha@gmail.com>, "Kevin Wolf" <kwolf@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, qemu-block@nongnu.org 
Envoyé: Vendredi 10 Juillet 2015 09:13:33 
Objet: Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan 

On Fri, 07/10 08:54, Alexandre DERUMIER wrote: 
> >>Thinking about this again, I doubt 
> >>that lengthening the duration with a hardcoded value benifits everyone; and 
> >>before Alexandre's reply on old server/slow disks 
> 
> With 1ms sleep, I can reproduce the hang 100% with a fast cpu (xeon e5 v3 3,1ghz) and source raw file on nfs. 

Does it completely hang? I can't reproduce this with my machine mirroring from 
nfs to local: the guest runs smoothly. What does "perf top" show? 

Fam 

> 
> 
> ----- Mail original ----- 
> De: "Fam Zheng" <famz@redhat.com> 
> À: "Stefan Hajnoczi" <stefanha@gmail.com> 
> Cc: "Kevin Wolf" <kwolf@redhat.com>, "qemu-devel" <qemu-devel@nongnu.org>, qemu-block@nongnu.org 
> Envoyé: Vendredi 10 Juillet 2015 08:43:50 
> Objet: Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan 
> 
> On Thu, 07/09 14:02, Stefan Hajnoczi wrote: 
> > This patch only converts the mirror block job to use the new relax 
> > function. The other block jobs (stream, backup, commit) are still using 
> > a 0 ns delay and are therefore broken. They should probably be 
> > converted in the same series. 
> 
> That's because they all can be perfectly mitigated by setting a reasonable 
> "speed" that matchs the host horsepower. Thinking about this again, I doubt 
> that lengthening the duration with a hardcoded value benifits everyone; and 
> before Alexandre's reply on old server/slow disks, I don't recall any report, 
> because the coroutines would already yield often enough, when waiting for IO to 
> complete. So I am not sure whether they're broken in practice. 
> 
> Fam 

Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

[Fam Zheng]

On Fri, 07/10 14:16, Alexandre DERUMIER wrote:
> So, I think patch 3/3 is enough. (could be great to have it for qemu 2.4)

Stefan, are you happy with this series?

Fam

Re: [Qemu-devel] [Qemu-block] [PATCH 0/3] mirror: Fix guest responsiveness during bitmap scan

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 477 bytes --]

On Mon, Jul 13, 2015 at 01:08:37PM +0800, Fam Zheng wrote:
> On Fri, 07/10 14:16, Alexandre DERUMIER wrote:
> > So, I think patch 3/3 is enough. (could be great to have it for qemu 2.4)
> 
> Stefan, are you happy with this series?

I'll take Patch 3 for QEMU 2.4.

This series should go via Jeff Cody (blockjobs maintainer) but he is/was
on vacation.

Patches 1 & 2 need more justification.  It seems like they might be
working around or masking a deeper problem.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]