[Qemu-devel] [PATCH 0/7] blockjob: refactor mirror_throttle

[Qemu-devel] [PATCH 0/7] blockjob: refactor mirror_throttle

[John Snow]

mirror_throttle attempts to make sure we yield every so often when we're
doing operations that may not otherwise be as cooperative as we'd like.

This pattern is useful to other jobs like commit as well; if commit
continuously decides not to copy data (and calculates delay_ns to be 0),
we'll frequently and aggressively yield, only to be rescheduled immediately.

This causes those "WARNING: I/O thread spun for 1000 iterations"
warnings that everyone loves so much.

Split out the mirror_throttle function to become a new internal
BlockJob API function, block_job_throttle; then use it for all
the other jobs in their runtime loops.

I decided to use it in stream and backup as well, so that regardless of if the
loop structure has the chance to be greedy or not (I did not audit this), the
BlockJob has some kind of failsafe that will occasionally perform a 0ns sleep
every SLICE_TIME.

________________________________________________________________________________

For convenience, this branch is available at:
https://github.com/jnsnow/qemu.git branch block-job-yield
https://github.com/jnsnow/qemu/tree/block-job-yield

This version is tagged block-job-yield-v1:
https://github.com/jnsnow/qemu/releases/tag/block-job-yield-v1

John Snow (7):
  blockjob: record time of last yield
  blockjob: consolidate SLICE_TIME definition
  blockjob: create block_job_throttle
  blockjob: allow block_job_throttle to take delay_ns
  block/commit: use block_job_throttle
  block/stream: use block_job_throttle
  block/backup: use block_job_throttle

 block/backup.c               | 12 ++++++------
 block/commit.c               |  5 ++---
 block/mirror.c               | 22 +++-------------------
 block/stream.c               |  4 +---
 blockjob.c                   | 12 ++++++++++++
 include/block/blockjob.h     |  5 +++++
 include/block/blockjob_int.h | 15 +++++++++++++++
 7 files changed, 44 insertions(+), 31 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH 1/7] blockjob: record time of last yield

[John Snow]

The mirror job makes a semi-inaccurate record of the last time we left
a "pause", but this doesn't always correlate to the time we actually
last successfully yielded control.

Record the time we last left a yield centrally.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/mirror.c           | 8 ++------
 blockjob.c               | 2 ++
 include/block/blockjob.h | 5 +++++
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index c9badc1203..d35c688faa 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -63,7 +63,6 @@ typedef struct MirrorBlockJob {
     QSIMPLEQ_HEAD(, MirrorBuffer) buf_free;
     int buf_free_count;
 
-    uint64_t last_pause_ns;
     unsigned long *in_flight_bitmap;
     int in_flight;
     int64_t bytes_in_flight;
@@ -596,8 +595,7 @@ static void mirror_throttle(MirrorBlockJob *s)
 {
     int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 
-    if (now - s->last_pause_ns > SLICE_TIME) {
-        s->last_pause_ns = now;
+    if (now - s->common.last_yield_ns > SLICE_TIME) {
         block_job_sleep_ns(&s->common, 0);
     } else {
         block_job_pause_point(&s->common);
@@ -769,7 +767,6 @@ static void coroutine_fn mirror_run(void *opaque)
 
     mirror_free_init(s);
 
-    s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     if (!s->is_none_mode) {
         ret = mirror_dirty_init(s);
         if (ret < 0 || block_job_is_cancelled(&s->common)) {
@@ -803,7 +800,7 @@ static void coroutine_fn mirror_run(void *opaque)
          * We do so every SLICE_TIME nanoseconds, or when there is an error,
          * or when the source is clean, whichever comes first.
          */
-        delta = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - s->last_pause_ns;
+        delta = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - s->common.last_yield_ns;
         if (delta < SLICE_TIME &&
             s->common.iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
             if (s->in_flight >= MAX_IN_FLIGHT || s->buf_free_count == 0 ||
@@ -878,7 +875,6 @@ static void coroutine_fn mirror_run(void *opaque)
             delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
             block_job_sleep_ns(&s->common, delay_ns);
         }
-        s->last_pause_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     }
 
 immediate_exit:
diff --git a/blockjob.c b/blockjob.c
index 715c2c2680..8cbc142f57 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -692,6 +692,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
     job->paused        = true;
     job->pause_count   = 1;
     job->refcnt        = 1;
+    job->last_yield_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     aio_timer_init(qemu_get_aio_context(), &job->sleep_timer,
                    QEMU_CLOCK_REALTIME, SCALE_NS,
                    block_job_sleep_timer_cb, job);
@@ -776,6 +777,7 @@ static void block_job_do_yield(BlockJob *job, uint64_t ns)
     job->busy = false;
     block_job_unlock();
     qemu_coroutine_yield();
+    job->last_yield_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
 
     /* Set by block_job_enter before re-entering the coroutine.  */
     assert(job->busy);
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 00403d9482..2712e2fd4e 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -141,6 +141,11 @@ typedef struct BlockJob {
      */
     QEMUTimer sleep_timer;
 
+    /**
+     * Timestamp of the last yield
+     */
+    uint64_t last_yield_ns;
+
     /** Non-NULL if this job is part of a transaction */
     BlockJobTxn *txn;
     QLIST_ENTRY(BlockJob) txn_list;
-- 
2.14.3

Re: [Qemu-devel] [PATCH 1/7] blockjob: record time of last yield

[Paolo Bonzini]

On 14/12/2017 01:59, John Snow wrote:
>      qemu_coroutine_yield();
> +    job->last_yield_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);

This is not the time the job has yielded control, but the time the job
has gotten it back.  Is it intended?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 1/7] blockjob: record time of last yield

[John Snow]

On 12/14/2017 03:38 AM, Paolo Bonzini wrote:
> On 14/12/2017 01:59, John Snow wrote:
>>      qemu_coroutine_yield();
>> +    job->last_yield_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> 
> This is not the time the job has yielded control, but the time the job
> has gotten it back.  Is it intended?
> 
> Thanks,
> 
> Paolo
> 

Yes, since that matches how mirror recorded last_sleep_ns, except for
where it records the 0ns sleep, then it records it beforehand (close
enough.)

I intended for this valuable to count how long we haven't yielded,
basically. Thought being that any time we are yielded, we're being
cooperative.

This is the selfishness counter :)

Re: [Qemu-devel] [PATCH 1/7] blockjob: record time of last yield

[Stefan Hajnoczi]

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

On Thu, Dec 14, 2017 at 10:55:22AM -0500, John Snow wrote:
> 
> 
> On 12/14/2017 03:38 AM, Paolo Bonzini wrote:
> > On 14/12/2017 01:59, John Snow wrote:
> >>      qemu_coroutine_yield();
> >> +    job->last_yield_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> > 
> > This is not the time the job has yielded control, but the time the job
> > has gotten it back.  Is it intended?
> > 
> > Thanks,
> > 
> > Paolo
> > 
> 
> Yes, since that matches how mirror recorded last_sleep_ns, except for
> where it records the 0ns sleep, then it records it beforehand (close
> enough.)
> 
> I intended for this valuable to count how long we haven't yielded,
> basically. Thought being that any time we are yielded, we're being
> cooperative.
> 
> This is the selfishness counter :)

Maybe last_enter_ns is a clearer name.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 2/7] blockjob: consolidate SLICE_TIME definition

[John Snow]

They're all the same. If it actually becomes important to configure it,
it can become a job or driver property.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/backup.c               | 1 -
 block/commit.c               | 2 --
 block/mirror.c               | 1 -
 block/stream.c               | 2 --
 include/block/blockjob_int.h | 2 ++
 5 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 99e6bcc748..d71b25c017 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -27,7 +27,6 @@
 #include "qemu/error-report.h"
 
 #define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16)
-#define SLICE_TIME 100000000ULL /* ns */
 
 typedef struct BackupBlockJob {
     BlockJob common;
diff --git a/block/commit.c b/block/commit.c
index c5327551ce..873e749d50 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -31,8 +31,6 @@ enum {
     COMMIT_BUFFER_SIZE = 512 * 1024, /* in bytes */
 };
 
-#define SLICE_TIME 100000000ULL /* ns */
-
 typedef struct CommitBlockJob {
     BlockJob common;
     RateLimit limit;
diff --git a/block/mirror.c b/block/mirror.c
index d35c688faa..eef5b598f5 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -22,7 +22,6 @@
 #include "qemu/ratelimit.h"
 #include "qemu/bitmap.h"
 
-#define SLICE_TIME    100000000ULL /* ns */
 #define MAX_IN_FLIGHT 16
 #define MAX_IO_BYTES (1 << 20) /* 1 Mb */
 #define DEFAULT_MIRROR_BUF_SIZE (MAX_IN_FLIGHT * MAX_IO_BYTES)
diff --git a/block/stream.c b/block/stream.c
index 499cdacdb0..e85af18c54 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -29,8 +29,6 @@ enum {
     STREAM_BUFFER_SIZE = 512 * 1024, /* in bytes */
 };
 
-#define SLICE_TIME 100000000ULL /* ns */
-
 typedef struct StreamBlockJob {
     BlockJob common;
     RateLimit limit;
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index c9b23b0cc9..209fa1bb3e 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -29,6 +29,8 @@
 #include "block/blockjob.h"
 #include "block/block.h"
 
+#define SLICE_TIME 100000000ULL /* ns */
+
 /**
  * BlockJobDriver:
  *
-- 
2.14.3

Re: [Qemu-devel] [PATCH 2/7] blockjob: consolidate SLICE_TIME definition

[Paolo Bonzini]

On 14/12/2017 01:59, John Snow wrote:
> They're all the same. If it actually becomes important to configure it,
> it can become a job or driver property.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/backup.c               | 1 -
>  block/commit.c               | 2 --
>  block/mirror.c               | 1 -
>  block/stream.c               | 2 --
>  include/block/blockjob_int.h | 2 ++
>  5 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 99e6bcc748..d71b25c017 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -27,7 +27,6 @@
>  #include "qemu/error-report.h"
>  
>  #define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16)
> -#define SLICE_TIME 100000000ULL /* ns */
>  
>  typedef struct BackupBlockJob {
>      BlockJob common;
> diff --git a/block/commit.c b/block/commit.c
> index c5327551ce..873e749d50 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -31,8 +31,6 @@ enum {
>      COMMIT_BUFFER_SIZE = 512 * 1024, /* in bytes */
>  };
>  
> -#define SLICE_TIME 100000000ULL /* ns */
> -
>  typedef struct CommitBlockJob {
>      BlockJob common;
>      RateLimit limit;
> diff --git a/block/mirror.c b/block/mirror.c
> index d35c688faa..eef5b598f5 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -22,7 +22,6 @@
>  #include "qemu/ratelimit.h"
>  #include "qemu/bitmap.h"
>  
> -#define SLICE_TIME    100000000ULL /* ns */
>  #define MAX_IN_FLIGHT 16
>  #define MAX_IO_BYTES (1 << 20) /* 1 Mb */
>  #define DEFAULT_MIRROR_BUF_SIZE (MAX_IN_FLIGHT * MAX_IO_BYTES)
> diff --git a/block/stream.c b/block/stream.c
> index 499cdacdb0..e85af18c54 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -29,8 +29,6 @@ enum {
>      STREAM_BUFFER_SIZE = 512 * 1024, /* in bytes */
>  };
>  
> -#define SLICE_TIME 100000000ULL /* ns */
> -
>  typedef struct StreamBlockJob {
>      BlockJob common;
>      RateLimit limit;
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index c9b23b0cc9..209fa1bb3e 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -29,6 +29,8 @@
>  #include "block/blockjob.h"
>  #include "block/block.h"
>  
> +#define SLICE_TIME 100000000ULL /* ns */
> +
>  /**
>   * BlockJobDriver:
>   *
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 2/7] blockjob: consolidate SLICE_TIME definition

[Stefan Hajnoczi]

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

On Wed, Dec 13, 2017 at 07:59:48PM -0500, John Snow wrote:
> They're all the same. If it actually becomes important to configure it,
> it can become a job or driver property.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/backup.c               | 1 -
>  block/commit.c               | 2 --
>  block/mirror.c               | 1 -
>  block/stream.c               | 2 --
>  include/block/blockjob_int.h | 2 ++
>  5 files changed, 2 insertions(+), 6 deletions(-)

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 2/7] blockjob: consolidate SLICE_TIME definition

[Jeff Cody]

On Wed, Dec 13, 2017 at 07:59:48PM -0500, John Snow wrote:
> They're all the same. If it actually becomes important to configure it,
> it can become a job or driver property.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/backup.c               | 1 -
>  block/commit.c               | 2 --
>  block/mirror.c               | 1 -
>  block/stream.c               | 2 --
>  include/block/blockjob_int.h | 2 ++
>  5 files changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 99e6bcc748..d71b25c017 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -27,7 +27,6 @@
>  #include "qemu/error-report.h"
>  
>  #define BACKUP_CLUSTER_SIZE_DEFAULT (1 << 16)
> -#define SLICE_TIME 100000000ULL /* ns */
>  
>  typedef struct BackupBlockJob {
>      BlockJob common;
> diff --git a/block/commit.c b/block/commit.c
> index c5327551ce..873e749d50 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -31,8 +31,6 @@ enum {
>      COMMIT_BUFFER_SIZE = 512 * 1024, /* in bytes */
>  };
>  
> -#define SLICE_TIME 100000000ULL /* ns */
> -
>  typedef struct CommitBlockJob {
>      BlockJob common;
>      RateLimit limit;
> diff --git a/block/mirror.c b/block/mirror.c
> index d35c688faa..eef5b598f5 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -22,7 +22,6 @@
>  #include "qemu/ratelimit.h"
>  #include "qemu/bitmap.h"
>  
> -#define SLICE_TIME    100000000ULL /* ns */
>  #define MAX_IN_FLIGHT 16
>  #define MAX_IO_BYTES (1 << 20) /* 1 Mb */
>  #define DEFAULT_MIRROR_BUF_SIZE (MAX_IN_FLIGHT * MAX_IO_BYTES)
> diff --git a/block/stream.c b/block/stream.c
> index 499cdacdb0..e85af18c54 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -29,8 +29,6 @@ enum {
>      STREAM_BUFFER_SIZE = 512 * 1024, /* in bytes */
>  };
>  
> -#define SLICE_TIME 100000000ULL /* ns */
> -
>  typedef struct StreamBlockJob {
>      BlockJob common;
>      RateLimit limit;
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index c9b23b0cc9..209fa1bb3e 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -29,6 +29,8 @@
>  #include "block/blockjob.h"
>  #include "block/block.h"
>  
> +#define SLICE_TIME 100000000ULL /* ns */
> +
>  /**
>   * BlockJobDriver:
>   *
> -- 
> 2.14.3
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>

[Qemu-devel] [PATCH 3/7] blockjob: create block_job_throttle

[John Snow]

This will replace mirror_throttle, for reuse in other jobs.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/mirror.c               | 15 ++-------------
 blockjob.c                   | 11 +++++++++++
 include/block/blockjob_int.h |  9 +++++++++
 3 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index eef5b598f5..60b52cfb19 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -590,17 +590,6 @@ static void mirror_exit(BlockJob *job, void *opaque)
     bdrv_unref(src);
 }
 
-static void mirror_throttle(MirrorBlockJob *s)
-{
-    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-
-    if (now - s->common.last_yield_ns > SLICE_TIME) {
-        block_job_sleep_ns(&s->common, 0);
-    } else {
-        block_job_pause_point(&s->common);
-    }
-}
-
 static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
 {
     int64_t offset;
@@ -621,7 +610,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
             int bytes = MIN(s->bdev_length - offset,
                             QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
 
-            mirror_throttle(s);
+            block_job_throttle(&s->common);
 
             if (block_job_is_cancelled(&s->common)) {
                 s->initial_zeroing_ongoing = false;
@@ -649,7 +638,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
         int bytes = MIN(s->bdev_length - offset,
                         QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
 
-        mirror_throttle(s);
+        block_job_throttle(&s->common);
 
         if (block_job_is_cancelled(&s->common)) {
             return 0;
diff --git a/blockjob.c b/blockjob.c
index 8cbc142f57..8d0c89a813 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -882,6 +882,17 @@ void block_job_yield(BlockJob *job)
     block_job_pause_point(job);
 }
 
+void block_job_throttle(BlockJob *job)
+{
+    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+
+    if (now - job->last_yield_ns > SLICE_TIME) {
+        block_job_sleep_ns(job, 0);
+    } else {
+        block_job_pause_point(job);
+    }
+}
+
 void block_job_iostatus_reset(BlockJob *job)
 {
     if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 209fa1bb3e..1a771b1e2e 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -157,6 +157,15 @@ void block_job_sleep_ns(BlockJob *job, int64_t ns);
  */
 void block_job_yield(BlockJob *job);
 
+/**
+ * block_job_throttle:
+ * @job: The job that calls the function.
+ *
+ * Yield if it has been SLICE_TIME nanoseconds since the last yield.
+ * Otherwise, check if we need to pause (and update the yield counter).
+ */
+void block_job_throttle(BlockJob *job);
+
 /**
  * block_job_pause_all:
  *
-- 
2.14.3

Re: [Qemu-devel] [PATCH 3/7] blockjob: create block_job_throttle

[Paolo Bonzini]

On 14/12/2017 01:59, John Snow wrote:
> + * Yield if it has been SLICE_TIME nanoseconds since the last yield.
> + * Otherwise, check if we need to pause (and update the yield counter).

What is the yield counter?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 3/7] blockjob: create block_job_throttle

[John Snow]

On 12/14/2017 03:39 AM, Paolo Bonzini wrote:
> On 14/12/2017 01:59, John Snow wrote:
>> + * Yield if it has been SLICE_TIME nanoseconds since the last yield.
>> + * Otherwise, check if we need to pause (and update the yield counter).
> 
> What is the yield counter?
> 
> Thanks,
> 
> Paolo
> 

Fuzzy brain talk.

I mean to refer to the last_yield_ns field, which gets updated by both
pause and sleep commands. I'm trying to document that no matter what
happens when you call this function (either a scheduled 0ns sleep, the
sleep requested, or a pause was requested) that it will update the
last_yield_ns variable.

Re: [Qemu-devel] [PATCH 3/7] blockjob: create block_job_throttle

[Stefan Hajnoczi]

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

On Wed, Dec 13, 2017 at 07:59:49PM -0500, John Snow wrote:
> +/**
> + * block_job_throttle:
> + * @job: The job that calls the function.
> + *
> + * Yield if it has been SLICE_TIME nanoseconds since the last yield.
> + * Otherwise, check if we need to pause (and update the yield counter).
> + */
> +void block_job_throttle(BlockJob *job);

This name is easily confused with the block-job-set-speed
ratelimit/throttling feature.

I suggest block_job_cpu_relax() or just block_job_relax() to make it
clear we're giving up our CPU time voluntarily - but this isn't
"throttling".

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

Re: [Qemu-devel] [PATCH 3/7] blockjob: create block_job_throttle

[Jeff Cody]

On Wed, Dec 13, 2017 at 07:59:49PM -0500, John Snow wrote:
> This will replace mirror_throttle, for reuse in other jobs.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/mirror.c               | 15 ++-------------
>  blockjob.c                   | 11 +++++++++++
>  include/block/blockjob_int.h |  9 +++++++++
>  3 files changed, 22 insertions(+), 13 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index eef5b598f5..60b52cfb19 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -590,17 +590,6 @@ static void mirror_exit(BlockJob *job, void *opaque)
>      bdrv_unref(src);
>  }
>  
> -static void mirror_throttle(MirrorBlockJob *s)
> -{
> -    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> -
> -    if (now - s->common.last_yield_ns > SLICE_TIME) {
> -        block_job_sleep_ns(&s->common, 0);
> -    } else {
> -        block_job_pause_point(&s->common);
> -    }
> -}
> -
>  static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>  {
>      int64_t offset;
> @@ -621,7 +610,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>              int bytes = MIN(s->bdev_length - offset,
>                              QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
>  
> -            mirror_throttle(s);
> +            block_job_throttle(&s->common);
>  
>              if (block_job_is_cancelled(&s->common)) {
>                  s->initial_zeroing_ongoing = false;
> @@ -649,7 +638,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>          int bytes = MIN(s->bdev_length - offset,
>                          QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
>  
> -        mirror_throttle(s);
> +        block_job_throttle(&s->common);
>  
>          if (block_job_is_cancelled(&s->common)) {
>              return 0;
> diff --git a/blockjob.c b/blockjob.c
> index 8cbc142f57..8d0c89a813 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -882,6 +882,17 @@ void block_job_yield(BlockJob *job)
>      block_job_pause_point(job);
>  }
>  
> +void block_job_throttle(BlockJob *job)
> +{
> +    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> +
> +    if (now - job->last_yield_ns > SLICE_TIME) {
> +        block_job_sleep_ns(job, 0);
> +    } else {
> +        block_job_pause_point(job);
> +    }
> +}
> +
>  void block_job_iostatus_reset(BlockJob *job)
>  {
>      if (job->iostatus == BLOCK_DEVICE_IO_STATUS_OK) {
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index 209fa1bb3e..1a771b1e2e 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -157,6 +157,15 @@ void block_job_sleep_ns(BlockJob *job, int64_t ns);
>   */
>  void block_job_yield(BlockJob *job);
>  
> +/**
> + * block_job_throttle:
> + * @job: The job that calls the function.
> + *
> + * Yield if it has been SLICE_TIME nanoseconds since the last yield.
> + * Otherwise, check if we need to pause (and update the yield counter).
> + */
> +void block_job_throttle(BlockJob *job);
> +
>  /**
>   * block_job_pause_all:
>   *
> -- 
> 2.14.3
> 

Reviewed-by: Jeff Cody <jcody@redhat.com>

[Qemu-devel] [PATCH 4/7] blockjob: allow block_job_throttle to take delay_ns

[John Snow]

Instead of only sleeping for 0ms when we've hit a timeout, optionally
take a longer more explicit delay_ns that always forces the sleep.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/mirror.c               |  4 ++--
 blockjob.c                   |  9 ++++-----
 include/block/blockjob_int.h | 10 +++++++---
 3 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index 60b52cfb19..81450e6ac4 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -610,7 +610,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
             int bytes = MIN(s->bdev_length - offset,
                             QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
 
-            block_job_throttle(&s->common);
+            block_job_throttle(&s->common, 0);
 
             if (block_job_is_cancelled(&s->common)) {
                 s->initial_zeroing_ongoing = false;
@@ -638,7 +638,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
         int bytes = MIN(s->bdev_length - offset,
                         QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
 
-        block_job_throttle(&s->common);
+        block_job_throttle(&s->common, 0);
 
         if (block_job_is_cancelled(&s->common)) {
             return 0;
diff --git a/blockjob.c b/blockjob.c
index 8d0c89a813..b0868c3ed5 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -882,12 +882,11 @@ void block_job_yield(BlockJob *job)
     block_job_pause_point(job);
 }
 
-void block_job_throttle(BlockJob *job)
+void block_job_throttle(BlockJob *job, int64_t delay_ns)
 {
-    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-
-    if (now - job->last_yield_ns > SLICE_TIME) {
-        block_job_sleep_ns(job, 0);
+    if (delay_ns || (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - \
+                     job->last_yield_ns > SLICE_TIME)) {
+        block_job_sleep_ns(job, delay_ns);
     } else {
         block_job_pause_point(job);
     }
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 1a771b1e2e..8faec3f5e0 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -160,11 +160,15 @@ void block_job_yield(BlockJob *job);
 /**
  * block_job_throttle:
  * @job: The job that calls the function.
+ * @delay_ns: The amount of time to sleep for
  *
- * Yield if it has been SLICE_TIME nanoseconds since the last yield.
- * Otherwise, check if we need to pause (and update the yield counter).
+ * Sleep for delay_ns nanoseconds.
+ *
+ * If delay_ns is 0, yield if it has been SLICE_TIME
+ * nanoseconds since the last yield. Otherwise, check
+ * if we need to yield for a pause event.
  */
-void block_job_throttle(BlockJob *job);
+void block_job_throttle(BlockJob *job, int64_t delay_ns);
 
 /**
  * block_job_pause_all:
-- 
2.14.3

Re: [Qemu-devel] [PATCH 4/7] blockjob: allow block_job_throttle to take delay_ns

[Paolo Bonzini]

On 14/12/2017 01:59, John Snow wrote:
> Instead of only sleeping for 0ms when we've hit a timeout, optionally
> take a longer more explicit delay_ns that always forces the sleep.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/mirror.c               |  4 ++--
>  blockjob.c                   |  9 ++++-----
>  include/block/blockjob_int.h | 10 +++++++---
>  3 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 60b52cfb19..81450e6ac4 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -610,7 +610,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>              int bytes = MIN(s->bdev_length - offset,
>                              QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
>  
> -            block_job_throttle(&s->common);
> +            block_job_throttle(&s->common, 0);
>  
>              if (block_job_is_cancelled(&s->common)) {
>                  s->initial_zeroing_ongoing = false;
> @@ -638,7 +638,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>          int bytes = MIN(s->bdev_length - offset,
>                          QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
>  
> -        block_job_throttle(&s->common);
> +        block_job_throttle(&s->common, 0);
>  
>          if (block_job_is_cancelled(&s->common)) {
>              return 0;
> diff --git a/blockjob.c b/blockjob.c
> index 8d0c89a813..b0868c3ed5 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -882,12 +882,11 @@ void block_job_yield(BlockJob *job)
>      block_job_pause_point(job);
>  }
>  
> -void block_job_throttle(BlockJob *job)
> +void block_job_throttle(BlockJob *job, int64_t delay_ns)
>  {
> -    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> -
> -    if (now - job->last_yield_ns > SLICE_TIME) {
> -        block_job_sleep_ns(job, 0);
> +    if (delay_ns || (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - \
> +                     job->last_yield_ns > SLICE_TIME)) {
> +        block_job_sleep_ns(job, delay_ns);
>      } else {
>          block_job_pause_point(job);
>      }
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index 1a771b1e2e..8faec3f5e0 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -160,11 +160,15 @@ void block_job_yield(BlockJob *job);
>  /**
>   * block_job_throttle:
>   * @job: The job that calls the function.
> + * @delay_ns: The amount of time to sleep for
>   *
> - * Yield if it has been SLICE_TIME nanoseconds since the last yield.
> - * Otherwise, check if we need to pause (and update the yield counter).

Okay, the yield counter goes away. :)

> + * Sleep for delay_ns nanoseconds.
> + *
> + * If delay_ns is 0, yield if it has been SLICE_TIME
> + * nanoseconds since the last yield. Otherwise, check
> + * if we need to yield for a pause event.

There are two meanings of yield here; one is just letting other events
run, the other is forever.  Can we rephrase it?  Perhaps, since the
check for pauses always holds (either directly via
block_job_pause_point, or via block_job_sleep_ns's call), something like:


   /* Sleep for delay_ns nanoseconds, and check if the block jobs
    * was requested to pause.
    *
    * If delay_ns is 0, block_job_throttle will also yield momentarily
    * if it has been SLICE_TIME nanoseconds since the last yield,
    * letting the main loop run.
    */

And another question.  After this series there is exactly one
block_job_sleep_ns call (in block/mirror.c).  Perhaps instead of
block_job_throttle, you should refine block_job_sleep_ns?

There are also two remaining calls to block_job_pause_point outside
block_job_throttle and block_job_sleep_ns (which however might be
unified according to the previous point).  Perhaps they should become
block_job_sleep_ns(job, 0), and block_job_pause_point can be made static?

Thanks,

Paolo

> -void block_job_throttle(BlockJob *job);
> +void block_job_throttle(BlockJob *job, int64_t delay_ns);
>  
>  /**
>   * block_job_pause_all:
> 

Re: [Qemu-devel] [PATCH 4/7] blockjob: allow block_job_throttle to take delay_ns

[John Snow]

On 12/14/2017 03:49 AM, Paolo Bonzini wrote:
> On 14/12/2017 01:59, John Snow wrote:
>> Instead of only sleeping for 0ms when we've hit a timeout, optionally
>> take a longer more explicit delay_ns that always forces the sleep.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/mirror.c               |  4 ++--
>>  blockjob.c                   |  9 ++++-----
>>  include/block/blockjob_int.h | 10 +++++++---
>>  3 files changed, 13 insertions(+), 10 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index 60b52cfb19..81450e6ac4 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -610,7 +610,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>>              int bytes = MIN(s->bdev_length - offset,
>>                              QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
>>  
>> -            block_job_throttle(&s->common);
>> +            block_job_throttle(&s->common, 0);
>>  
>>              if (block_job_is_cancelled(&s->common)) {
>>                  s->initial_zeroing_ongoing = false;
>> @@ -638,7 +638,7 @@ static int coroutine_fn mirror_dirty_init(MirrorBlockJob *s)
>>          int bytes = MIN(s->bdev_length - offset,
>>                          QEMU_ALIGN_DOWN(INT_MAX, s->granularity));
>>  
>> -        block_job_throttle(&s->common);
>> +        block_job_throttle(&s->common, 0);
>>  
>>          if (block_job_is_cancelled(&s->common)) {
>>              return 0;
>> diff --git a/blockjob.c b/blockjob.c
>> index 8d0c89a813..b0868c3ed5 100644
>> --- a/blockjob.c
>> +++ b/blockjob.c
>> @@ -882,12 +882,11 @@ void block_job_yield(BlockJob *job)
>>      block_job_pause_point(job);
>>  }
>>  
>> -void block_job_throttle(BlockJob *job)
>> +void block_job_throttle(BlockJob *job, int64_t delay_ns)
>>  {
>> -    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>> -
>> -    if (now - job->last_yield_ns > SLICE_TIME) {
>> -        block_job_sleep_ns(job, 0);
>> +    if (delay_ns || (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - \
>> +                     job->last_yield_ns > SLICE_TIME)) {
>> +        block_job_sleep_ns(job, delay_ns);
>>      } else {
>>          block_job_pause_point(job);
>>      }
>> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
>> index 1a771b1e2e..8faec3f5e0 100644
>> --- a/include/block/blockjob_int.h
>> +++ b/include/block/blockjob_int.h
>> @@ -160,11 +160,15 @@ void block_job_yield(BlockJob *job);
>>  /**
>>   * block_job_throttle:
>>   * @job: The job that calls the function.
>> + * @delay_ns: The amount of time to sleep for
>>   *
>> - * Yield if it has been SLICE_TIME nanoseconds since the last yield.
>> - * Otherwise, check if we need to pause (and update the yield counter).
> 
> Okay, the yield counter goes away. :)
> 

Yeah, I guess I wrote the documentation twice and in one that phrase
lost out. Not any conscious decision, actually ... see my reply to your
earlier question.

>> + * Sleep for delay_ns nanoseconds.
>> + *
>> + * If delay_ns is 0, yield if it has been SLICE_TIME
>> + * nanoseconds since the last yield. Otherwise, check
>> + * if we need to yield for a pause event.
> 
> There are two meanings of yield here; one is just letting other events
> run, the other is forever.  Can we rephrase it?  Perhaps, since the
> check for pauses always holds (either directly via
> block_job_pause_point, or via block_job_sleep_ns's call), something like:
> 
> 

Yes, I see what you mean. I was trying to avoid ambiguity over exactly
which primitive we were measuring and as "yield" was presently the most
primitive before we disappear into coroutines, I opted for that. I
didn't want other readers to confuse this with:

- Sleep: We also track indefinite yields, like with pauses or user
pauses. It's not just a counter to keep track of sleep_ns calls, for
instance.
- Pause: The counter keeps track of more than just pauses or user pauses.

Since everything boils down to do_yield calls, I opted for that one to
try to be explicit about what I'm tracking. I can see that it's also
silly because of course "block_job_yield" is also a call, and of course
we don't track JUST that, either...


>    /* Sleep for delay_ns nanoseconds, and check if the block jobs
>     * was requested to pause.
>     *
>     * If delay_ns is 0, block_job_throttle will also yield momentarily
>     * if it has been SLICE_TIME nanoseconds since the last yield,
>     * letting the main loop run.
>     */
> 
> And another question.  After this series there is exactly one
> block_job_sleep_ns call (in block/mirror.c).  Perhaps instead of
> block_job_throttle, you should refine block_job_sleep_ns?
> 

Yeah, maybe? "A rose by any other name," though -- I think I might be
coming for the block/mirror call next because I have one more downstream
BZ that references this as a job that can cause the warning print.

So maybe we'll just have throttle calls instead of sleep calls from here
on out.

> There are also two remaining calls to block_job_pause_point outside
> block_job_throttle and block_job_sleep_ns (which however might be
> unified according to the previous point).  Perhaps they should become
> block_job_sleep_ns(job, 0), and block_job_pause_point can be made static?
> 
> Thanks,
> 
> Paolo
> 

Yeah, I am heading in that direction but couldn't prove to myself it was
safe yesterday; but unifying the way in which the jobs handle
cooperative scheduling is on the docket.

Of course, maybe this is just a small baby step towards unifying all the
jobs into one hideous super mega job, as per Kevin.

>> -void block_job_throttle(BlockJob *job);
>> +void block_job_throttle(BlockJob *job, int64_t delay_ns);
>>  
>>  /**
>>   * block_job_pause_all:
>>
> 
> 

Thanks,

John

Re: [Qemu-devel] [PATCH 4/7] blockjob: allow block_job_throttle to take delay_ns

[Paolo Bonzini]

On 14/12/2017 17:06, John Snow wrote:
>>
>> And another question.  After this series there is exactly one
>> block_job_sleep_ns call (in block/mirror.c).  Perhaps instead of
>> block_job_throttle, you should refine block_job_sleep_ns?
>>
> Yeah, maybe? "A rose by any other name," though -- I think I might be
> coming for the block/mirror call next because I have one more downstream
> BZ that references this as a job that can cause the warning print.
> 
> So maybe we'll just have throttle calls instead of sleep calls from here
> on out.

Ok, shall we wait for v2 where you look at that BZ as well?

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 4/7] blockjob: allow block_job_throttle to take delay_ns

[John Snow]

On 12/14/2017 12:21 PM, Paolo Bonzini wrote:
> On 14/12/2017 17:06, John Snow wrote:
>>>
>>> And another question.  After this series there is exactly one
>>> block_job_sleep_ns call (in block/mirror.c).  Perhaps instead of
>>> block_job_throttle, you should refine block_job_sleep_ns?
>>>
>> Yeah, maybe? "A rose by any other name," though -- I think I might be
>> coming for the block/mirror call next because I have one more downstream
>> BZ that references this as a job that can cause the warning print.
>>
>> So maybe we'll just have throttle calls instead of sleep calls from here
>> on out.
> 
> Ok, shall we wait for v2 where you look at that BZ as well?
> 
> Thanks,
> 
> Paolo
> 

Sure -- if the only feedback you have on this series is primarily style
and "maybe there are some more wins" I can spin a v2 to try to broaden
the scope if it looks good so far.

--js

Re: [Qemu-devel] [PATCH 4/7] blockjob: allow block_job_throttle to take delay_ns

[Paolo Bonzini]

On 14/12/2017 18:22, John Snow wrote:
>> Ok, shall we wait for v2 where you look at that BZ as well?
>
> Sure -- if the only feedback you have on this series is primarily style
> and "maybe there are some more wins" I can spin a v2 to try to broaden
> the scope if it looks good so far.

Yeah, that's pretty much it (except that it isn't "maybe there are some
more wins", but rather "going from two to three functions isn't exactly
what I was hoping for" :)).

Paolo

[Qemu-devel] [PATCH 5/7] block/commit: use block_job_throttle

[John Snow]

Depending on the value of `speed` and how fast our backends are,
delay_ns might be 0 very, very often. This creates some warning
messages that spook users, but it's also pretty inefficient.

Use block_job_throttle instead to yield a little more intelligently.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/commit.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/block/commit.c b/block/commit.c
index 873e749d50..567064215b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -172,7 +172,8 @@ static void coroutine_fn commit_run(void *opaque)
         /* Note that even when no rate limit is applied we need to yield
          * with no pending I/O here so that bdrv_drain_all() returns.
          */
-        block_job_sleep_ns(&s->common, delay_ns);
+        block_job_throttle(&s->common, delay_ns);
+
         if (block_job_is_cancelled(&s->common)) {
             break;
         }
-- 
2.14.3

Re: [Qemu-devel] [PATCH 5/7] block/commit: use block_job_throttle

[Paolo Bonzini]

On 14/12/2017 01:59, John Snow wrote:
> Depending on the value of `speed` and how fast our backends are,
> delay_ns might be 0 very, very often. This creates some warning
> messages that spook users, but it's also pretty inefficient.
> 
> Use block_job_throttle instead to yield a little more intelligently.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/commit.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/block/commit.c b/block/commit.c
> index 873e749d50..567064215b 100644
> --- a/block/commit.c
> +++ b/block/commit.c
> @@ -172,7 +172,8 @@ static void coroutine_fn commit_run(void *opaque)
>          /* Note that even when no rate limit is applied we need to yield
>           * with no pending I/O here so that bdrv_drain_all() returns.
>           */
> -        block_job_sleep_ns(&s->common, delay_ns);
> +        block_job_throttle(&s->common, delay_ns);
> +
>          if (block_job_is_cancelled(&s->common)) {
>              break;
>          }
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 5/7] block/commit: use block_job_throttle

[Stefan Hajnoczi]

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

On Wed, Dec 13, 2017 at 07:59:51PM -0500, John Snow wrote:
> Depending on the value of `speed` and how fast our backends are,
> delay_ns might be 0 very, very often. This creates some warning
> messages that spook users, but it's also pretty inefficient.
> 
> Use block_job_throttle instead to yield a little more intelligently.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/commit.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 6/7] block/stream: use block_job_throttle

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/stream.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/stream.c b/block/stream.c
index e85af18c54..3ad3190387 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -139,7 +139,7 @@ static void coroutine_fn stream_run(void *opaque)
         /* Note that even when no rate limit is applied we need to yield
          * with no pending I/O here so that bdrv_drain_all() returns.
          */
-        block_job_sleep_ns(&s->common, delay_ns);
+        block_job_throttle(&s->common, delay_ns);
         if (block_job_is_cancelled(&s->common)) {
             break;
         }
-- 
2.14.3

Re: [Qemu-devel] [PATCH 6/7] block/stream: use block_job_throttle

[Paolo Bonzini]

On 14/12/2017 01:59, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/stream.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/block/stream.c b/block/stream.c
> index e85af18c54..3ad3190387 100644
> --- a/block/stream.c
> +++ b/block/stream.c
> @@ -139,7 +139,7 @@ static void coroutine_fn stream_run(void *opaque)
>          /* Note that even when no rate limit is applied we need to yield
>           * with no pending I/O here so that bdrv_drain_all() returns.
>           */
> -        block_job_sleep_ns(&s->common, delay_ns);
> +        block_job_throttle(&s->common, delay_ns);
>          if (block_job_is_cancelled(&s->common)) {
>              break;
>          }
> 

Nit, the previous patch is adding a blank line here, but this one isn't.

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 6/7] block/stream: use block_job_throttle

[Stefan Hajnoczi]

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

On Wed, Dec 13, 2017 at 07:59:52PM -0500, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/stream.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 7/7] block/backup: use block_job_throttle

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/backup.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index d71b25c017..a5ede5f643 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -334,6 +334,8 @@ static void backup_complete(BlockJob *job, void *opaque)
 
 static bool coroutine_fn yield_and_check(BackupBlockJob *job)
 {
+    uint64_t delay_ns = 0;
+
     if (block_job_is_cancelled(&job->common)) {
         return true;
     }
@@ -342,14 +344,13 @@ static bool coroutine_fn yield_and_check(BackupBlockJob *job)
      * (without, VM does not reboot)
      */
     if (job->common.speed) {
-        uint64_t delay_ns = ratelimit_calculate_delay(&job->limit,
-                                                      job->bytes_read);
+        delay_ns = ratelimit_calculate_delay(&job->limit,
+                                             job->bytes_read);
         job->bytes_read = 0;
-        block_job_sleep_ns(&job->common, delay_ns);
-    } else {
-        block_job_sleep_ns(&job->common, 0);
     }
 
+    block_job_throttle(&job->common, delay_ns);
+
     if (block_job_is_cancelled(&job->common)) {
         return true;
     }
-- 
2.14.3

Re: [Qemu-devel] [PATCH 7/7] block/backup: use block_job_throttle

[Paolo Bonzini]

On 14/12/2017 01:59, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/backup.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index d71b25c017..a5ede5f643 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -334,6 +334,8 @@ static void backup_complete(BlockJob *job, void *opaque)
>  
>  static bool coroutine_fn yield_and_check(BackupBlockJob *job)
>  {
> +    uint64_t delay_ns = 0;
> +
>      if (block_job_is_cancelled(&job->common)) {
>          return true;
>      }
> @@ -342,14 +344,13 @@ static bool coroutine_fn yield_and_check(BackupBlockJob *job)
>       * (without, VM does not reboot)
>       */
>      if (job->common.speed) {
> -        uint64_t delay_ns = ratelimit_calculate_delay(&job->limit,
> -                                                      job->bytes_read);
> +        delay_ns = ratelimit_calculate_delay(&job->limit,
> +                                             job->bytes_read);
>          job->bytes_read = 0;
> -        block_job_sleep_ns(&job->common, delay_ns);
> -    } else {
> -        block_job_sleep_ns(&job->common, 0);
>      }
>  
> +    block_job_throttle(&job->common, delay_ns);
> +
>      if (block_job_is_cancelled(&job->common)) {
>          return true;
>      }
> 

This one is. :)

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 7/7] block/backup: use block_job_throttle

[Stefan Hajnoczi]

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

On Wed, Dec 13, 2017 at 07:59:53PM -0500, John Snow wrote:
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/backup.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]