[Qemu-devel] [PATCH v2 00/13] blockjob: refactor mirror_throttle

[Qemu-devel] [PATCH v2 00/13] 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.

v2:
 - Fixed spacing consistency (Paolo)
 - Renamed last_yield_ns to last_enter_ns (Stefan)
 - Renamed block_job_throttle to block_job_relax (Stefan)
 - Removed external usages of block_job_pause_point and
   block_job_sleep_ns (Paolo)
 - Further cut down block/backup code to use block_job_relax

________________________________________________________________________________

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-v2:
https://github.com/jnsnow/qemu/releases/tag/block-job-yield-v2

John Snow (13):
  blockjob: record time of last entrance
  blockjob: consolidate SLICE_TIME definition
  blockjob: create block_job_relax
  blockjob: allow block_job_throttle to take delay_ns
  block/commit: use block_job_relax
  block/stream: use block_job_relax
  block/backup: use block_job_relax
  allow block_job_relax to return -ECANCELED
  block/backup: remove yield_and_check
  block/mirror: condense cancellation and relax calls
  block/mirror: remove block_job_sleep_ns calls
  blockjob: privatize block_job_sleep_ns
  blockjob: remove block_job_pause_point from interface

 block/backup.c               | 27 ++++++-----------------
 block/commit.c               |  4 +---
 block/mirror.c               | 52 +++++++++++++++-----------------------------
 block/stream.c               |  4 +---
 block/trace-events           |  2 +-
 blockjob.c                   | 51 ++++++++++++++++++++++++++++++++++++-------
 include/block/blockjob.h     |  5 +++++
 include/block/blockjob_int.h | 37 +++++++++++++++----------------
 tests/test-bdrv-drain.c      |  2 +-
 tests/test-blockjob-txn.c    |  2 +-
 10 files changed, 94 insertions(+), 92 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v2 01/13] blockjob: record time of last entrance

[John Snow]

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

Record the time we last *exited* a yield centrally. In other words, record
the time we began execution of this job to know how long we have been
selfish for.

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..88f4e8964d 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_enter_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_enter_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 f5cea84e73..2a9ff66b95 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -321,6 +321,7 @@ void block_job_start(BlockJob *job)
     job->pause_count--;
     job->busy = true;
     job->paused = false;
+    job->last_enter_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
     bdrv_coroutine_enter(blk_bs(job->blk), job->co);
 }
 
@@ -786,6 +787,7 @@ static void block_job_do_yield(BlockJob *job, uint64_t ns)
     job->busy = false;
     block_job_unlock();
     qemu_coroutine_yield();
+    job->last_enter_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..e965845c94 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_enter_ns;
+
     /** Non-NULL if this job is part of a transaction */
     BlockJobTxn *txn;
     QLIST_ENTRY(BlockJob) txn_list;
-- 
2.14.3

[Qemu-devel] [PATCH v2 02/13] 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>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Jeff Cody <jcody@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 4a16a37229..7b1cdd038a 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 bb6c904704..898545b318 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 88f4e8964d..1fb5fc0cb8 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

[Qemu-devel] [PATCH v2 03/13] blockjob: create block_job_relax

[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 1fb5fc0cb8..36c681c7a1 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_enter_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_relax(&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_relax(&s->common);
 
         if (block_job_is_cancelled(&s->common)) {
             return 0;
diff --git a/blockjob.c b/blockjob.c
index 2a9ff66b95..6f2e709b51 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -906,6 +906,17 @@ void block_job_yield(BlockJob *job)
     block_job_pause_point(job);
 }
 
+void block_job_relax(BlockJob *job)
+{
+    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
+
+    if (now - job->last_enter_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..553784d86f 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_relax:
+ * @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 yield if so.
+ */
+void block_job_relax(BlockJob *job);
+
 /**
  * block_job_pause_all:
  *
-- 
2.14.3

[Qemu-devel] [PATCH v2 04/13] 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 36c681c7a1..3c73caed5e 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_relax(&s->common);
+            block_job_relax(&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_relax(&s->common);
+        block_job_relax(&s->common, 0);
 
         if (block_job_is_cancelled(&s->common)) {
             return 0;
diff --git a/blockjob.c b/blockjob.c
index 6f2e709b51..51c0eb5d9e 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -906,12 +906,11 @@ void block_job_yield(BlockJob *job)
     block_job_pause_point(job);
 }
 
-void block_job_relax(BlockJob *job)
+void block_job_relax(BlockJob *job, int64_t delay_ns)
 {
-    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
-
-    if (now - job->last_enter_ns > SLICE_TIME) {
-        block_job_sleep_ns(job, 0);
+    if (delay_ns || (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - \
+                     job->last_enter_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 553784d86f..5f1520fab7 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_relax:
  * @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 yield if so.
+ * 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_relax(BlockJob *job);
+void block_job_relax(BlockJob *job, int64_t delay_ns);
 
 /**
  * block_job_pause_all:
-- 
2.14.3

[Qemu-devel] [PATCH v2 05/13] block/commit: use block_job_relax

[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_relax instead to yield a little more intelligently.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/commit.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/commit.c b/block/commit.c
index 898545b318..9cfe3ed76b 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -172,7 +172,7 @@ 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_relax(&s->common, delay_ns);
         if (block_job_is_cancelled(&s->common)) {
             break;
         }
-- 
2.14.3

[Qemu-devel] [PATCH v2 06/13] block/stream: use block_job_relax

[John Snow]

See prior commit for justification.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@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..49dc102565 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_relax(&s->common, delay_ns);
         if (block_job_is_cancelled(&s->common)) {
             break;
         }
-- 
2.14.3

[Qemu-devel] [PATCH v2 07/13] block/backup: use block_job_relax

[John Snow]

See two commits back for justification.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/backup.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 7b1cdd038a..b4204c0ee4 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -336,6 +336,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;
     }
@@ -344,14 +346,12 @@ 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_relax(&job->common, delay_ns);
     if (block_job_is_cancelled(&job->common)) {
         return true;
     }
-- 
2.14.3

[Qemu-devel] [PATCH v2 08/13] allow block_job_relax to return -ECANCELED

[John Snow]

This is just an optimization for callers who are likely going to
want to check quite close to this call if the job was canceled or
not anyway.

Along the same lines, add the return to block_job_pause_point and
block_job_sleep_ns, so we don't have to re-check it quite so
excessively.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockjob.c                   | 28 +++++++++++++++++-----------
 include/block/blockjob_int.h |  8 +++++---
 2 files changed, 22 insertions(+), 14 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index 51c0eb5d9e..b5a0cda412 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -793,15 +793,15 @@ static void block_job_do_yield(BlockJob *job, uint64_t ns)
     assert(job->busy);
 }
 
-void coroutine_fn block_job_pause_point(BlockJob *job)
+int coroutine_fn block_job_pause_point(BlockJob *job)
 {
     assert(job && block_job_started(job));
 
-    if (!block_job_should_pause(job)) {
-        return;
-    }
     if (block_job_is_cancelled(job)) {
-        return;
+        return -ECANCELED;
+    }
+    if (!block_job_should_pause(job)) {
+        return 0;
     }
 
     if (job->driver->pause) {
@@ -817,6 +817,8 @@ void coroutine_fn block_job_pause_point(BlockJob *job)
     if (job->driver->resume) {
         job->driver->resume(job);
     }
+
+    return 0;
 }
 
 void block_job_resume_all(void)
@@ -874,20 +876,20 @@ bool block_job_is_cancelled(BlockJob *job)
     return job->cancelled;
 }
 
-void block_job_sleep_ns(BlockJob *job, int64_t ns)
+int block_job_sleep_ns(BlockJob *job, int64_t ns)
 {
     assert(job->busy);
 
     /* Check cancellation *before* setting busy = false, too!  */
     if (block_job_is_cancelled(job)) {
-        return;
+        return -ECANCELED;
     }
 
     if (!block_job_should_pause(job)) {
         block_job_do_yield(job, qemu_clock_get_ns(QEMU_CLOCK_REALTIME) + ns);
     }
 
-    block_job_pause_point(job);
+    return block_job_pause_point(job);
 }
 
 void block_job_yield(BlockJob *job)
@@ -906,13 +908,17 @@ void block_job_yield(BlockJob *job)
     block_job_pause_point(job);
 }
 
-void block_job_relax(BlockJob *job, int64_t delay_ns)
+int block_job_relax(BlockJob *job, int64_t delay_ns)
 {
+    if (block_job_is_cancelled(job)) {
+        return -ECANCELED;
+    }
+
     if (delay_ns || (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - \
                      job->last_enter_ns > SLICE_TIME)) {
-        block_job_sleep_ns(job, delay_ns);
+        return block_job_sleep_ns(job, delay_ns);
     } else {
-        block_job_pause_point(job);
+        return block_job_pause_point(job);
     }
 }
 
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 5f1520fab7..1ceb47e1e6 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -147,7 +147,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
  * %QEMU_CLOCK_REALTIME nanoseconds.  Canceling the job will immediately
  * interrupt the wait.
  */
-void block_job_sleep_ns(BlockJob *job, int64_t ns);
+int block_job_sleep_ns(BlockJob *job, int64_t ns);
 
 /**
  * block_job_yield:
@@ -167,8 +167,10 @@ void block_job_yield(BlockJob *job);
  * 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.
+ *
+ * returns ECANCELED if the job has been canceled.
  */
-void block_job_relax(BlockJob *job, int64_t delay_ns);
+int block_job_relax(BlockJob *job, int64_t delay_ns);
 
 /**
  * block_job_pause_all:
@@ -217,7 +219,7 @@ bool block_job_is_cancelled(BlockJob *job);
  * Pause now if block_job_pause() has been called.  Block jobs that perform
  * lots of I/O must call this between requests so that the job can be paused.
  */
-void coroutine_fn block_job_pause_point(BlockJob *job);
+int coroutine_fn block_job_pause_point(BlockJob *job);
 
 /**
  * block_job_enter:
-- 
2.14.3

[Qemu-devel] [PATCH v2 09/13] block/backup: remove yield_and_check

[John Snow]

This is a respin of the same functionality as mirror_throttle,
so trash this and replace it with the generic version.

yield_and_check returned true if canceled, false otherwise.
block_job_relax returns -ECANCELED if canceled, 0 otherwise.

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

diff --git a/block/backup.c b/block/backup.c
index b4204c0ee4..0624c3b322 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -334,29 +334,17 @@ static void backup_complete(BlockJob *job, void *opaque)
     g_free(data);
 }
 
-static bool coroutine_fn yield_and_check(BackupBlockJob *job)
+static uint64_t get_delay_ns(BackupBlockJob *job)
 {
     uint64_t delay_ns = 0;
 
-    if (block_job_is_cancelled(&job->common)) {
-        return true;
-    }
-
-    /* we need to yield so that bdrv_drain_all() returns.
-     * (without, VM does not reboot)
-     */
     if (job->common.speed) {
         delay_ns = ratelimit_calculate_delay(&job->limit,
                                              job->bytes_read);
         job->bytes_read = 0;
     }
 
-    block_job_relax(&job->common, delay_ns);
-    if (block_job_is_cancelled(&job->common)) {
-        return true;
-    }
-
-    return false;
+    return delay_ns;
 }
 
 static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
@@ -369,7 +357,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
     hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
     while ((cluster = hbitmap_iter_next(&hbi)) != -1) {
         do {
-            if (yield_and_check(job)) {
+            if (block_job_relax(&job->common, get_delay_ns(job))) {
                 return 0;
             }
             ret = backup_do_cow(job, cluster * job->cluster_size,
@@ -465,7 +453,7 @@ static void coroutine_fn backup_run(void *opaque)
             bool error_is_read;
             int alloced = 0;
 
-            if (yield_and_check(job)) {
+            if (block_job_relax(&job->common, get_delay_ns(job))) {
                 break;
             }
 
-- 
2.14.3

[Qemu-devel] [PATCH v2 10/13] block/mirror: condense cancellation and relax calls

[John Snow]

We can count on the relax call to check cancellation for us, so
condense these concurrent calls.

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

diff --git a/block/mirror.c b/block/mirror.c
index 3c73caed5e..a0e0044de2 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -610,9 +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_relax(&s->common, 0);
-
-            if (block_job_is_cancelled(&s->common)) {
+            if (block_job_relax(&s->common, 0)) {
                 s->initial_zeroing_ongoing = false;
                 return 0;
             }
@@ -638,9 +636,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_relax(&s->common, 0);
-
-        if (block_job_is_cancelled(&s->common)) {
+        if (block_job_relax(&s->common, 0)) {
             return 0;
         }
 
-- 
2.14.3

[Qemu-devel] [PATCH v2 11/13] block/mirror: remove block_job_sleep_ns calls

[John Snow]

We're attempting to slacken the mirror loop in three different places,
but we can combine these three attempts. Combine the early loop call to
block_job_pause_point with the two late-loop calls to block_job_sleep_ns.

When delay_ns is 0 and it has not been SLICE_TIME since the last yield,
block_job_relax is merely a call to block_job_pause_point, so this should
be equivalent with the exception that if we have managed to not yield at
all in the last SLICE_TIME ns, we will now do so.

I am not sure that condition was possible,
so this loop should be equivalent.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block/mirror.c     | 22 +++++++++++-----------
 block/trace-events |  2 +-
 2 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/block/mirror.c b/block/mirror.c
index a0e0044de2..192e03694f 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -761,7 +761,7 @@ static void coroutine_fn mirror_run(void *opaque)
     assert(!s->dbi);
     s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap);
     for (;;) {
-        uint64_t delay_ns = 0;
+        static uint64_t delay_ns = 0;
         int64_t cnt, delta;
         bool should_complete;
 
@@ -770,9 +770,16 @@ static void coroutine_fn mirror_run(void *opaque)
             goto immediate_exit;
         }
 
-        block_job_pause_point(&s->common);
-
         cnt = bdrv_get_dirty_count(s->dirty_bitmap);
+
+        trace_mirror_before_relax(s, cnt, s->synced, delay_ns);
+        if (block_job_relax(&s->common, delay_ns)) {
+            if (!s->synced) {
+                goto immediate_exit;
+            }
+        }
+        delay_ns = 0;
+
         /* s->common.offset contains the number of bytes already processed so
          * far, cnt is the number of dirty bytes remaining and
          * s->bytes_in_flight is the number of bytes currently being
@@ -849,15 +856,8 @@ static void coroutine_fn mirror_run(void *opaque)
         }
 
         ret = 0;
-        trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
-        if (!s->synced) {
-            block_job_sleep_ns(&s->common, delay_ns);
-            if (block_job_is_cancelled(&s->common)) {
-                break;
-            }
-        } else if (!should_complete) {
+        if (s->synced && !should_complete) {
             delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
-            block_job_sleep_ns(&s->common, delay_ns);
         }
     }
 
diff --git a/block/trace-events b/block/trace-events
index 11c8d5f590..60f36f929a 100644
--- a/block/trace-events
+++ b/block/trace-events
@@ -27,7 +27,7 @@ mirror_start(void *bs, void *s, void *opaque) "bs %p s %p opaque %p"
 mirror_restart_iter(void *s, int64_t cnt) "s %p dirty count %"PRId64
 mirror_before_flush(void *s) "s %p"
 mirror_before_drain(void *s, int64_t cnt) "s %p dirty count %"PRId64
-mirror_before_sleep(void *s, int64_t cnt, int synced, uint64_t delay_ns) "s %p dirty count %"PRId64" synced %d delay %"PRIu64"ns"
+mirror_before_relax(void *s, int64_t cnt, int synced, uint64_t delay_ns) "s %p dirty count %"PRId64" synced %d delay %"PRIu64"ns"
 mirror_one_iteration(void *s, int64_t offset, uint64_t bytes) "s %p offset %" PRId64 " bytes %" PRIu64
 mirror_iteration_done(void *s, int64_t offset, uint64_t bytes, int ret) "s %p offset %" PRId64 " bytes %" PRIu64 " ret %d"
 mirror_yield(void *s, int64_t cnt, int buf_free_count, int in_flight) "s %p dirty count %"PRId64" free buffers %d in_flight %d"
-- 
2.14.3

[Qemu-devel] [PATCH v2 12/13] blockjob: privatize block_job_sleep_ns

[John Snow]

There's not currently any external caller of it.

Except in tests, but we'll fix that here too.

Replace usages in test cases with block_job_relax, which functions
similarly enough to be used as a drop-in replacement.

Very technically block_job_sleep_ns(job, 0) behaves differently
from block_job_relax(job, 0) in that relax may resolve to a no-op,
but this makes no difference in the test in which it is used.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 blockjob.c                   | 11 ++++++++++-
 include/block/blockjob_int.h | 11 -----------
 tests/test-bdrv-drain.c      |  2 +-
 tests/test-blockjob-txn.c    |  2 +-
 4 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/blockjob.c b/blockjob.c
index b5a0cda412..40167d6896 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -876,7 +876,16 @@ bool block_job_is_cancelled(BlockJob *job)
     return job->cancelled;
 }
 
-int block_job_sleep_ns(BlockJob *job, int64_t ns)
+/**
+ * block_job_sleep_ns:
+ * @job: The job that calls the function.
+ * @ns: How many nanoseconds to stop for.
+ *
+ * Put the job to sleep (assuming that it wasn't canceled) for @ns
+ * %QEMU_CLOCK_REALTIME nanoseconds.  Canceling the job will immediately
+ * interrupt the wait.
+ */
+static int block_job_sleep_ns(BlockJob *job, int64_t ns)
 {
     assert(job->busy);
 
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 1ceb47e1e6..c4891a5a9b 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -138,17 +138,6 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
                        uint64_t shared_perm, int64_t speed, int flags,
                        BlockCompletionFunc *cb, void *opaque, Error **errp);
 
-/**
- * block_job_sleep_ns:
- * @job: The job that calls the function.
- * @ns: How many nanoseconds to stop for.
- *
- * Put the job to sleep (assuming that it wasn't canceled) for @ns
- * %QEMU_CLOCK_REALTIME nanoseconds.  Canceling the job will immediately
- * interrupt the wait.
- */
-int block_job_sleep_ns(BlockJob *job, int64_t ns);
-
 /**
  * block_job_yield:
  * @job: The job that calls the function.
diff --git a/tests/test-bdrv-drain.c b/tests/test-bdrv-drain.c
index d760e2b243..5d47541b4c 100644
--- a/tests/test-bdrv-drain.c
+++ b/tests/test-bdrv-drain.c
@@ -506,7 +506,7 @@ static void coroutine_fn test_job_start(void *opaque)
     TestBlockJob *s = opaque;
 
     while (!s->should_complete) {
-        block_job_sleep_ns(&s->common, 100000);
+        block_job_relax(&s->common, 100000);
     }
 
     block_job_defer_to_main_loop(&s->common, test_job_completed, NULL);
diff --git a/tests/test-blockjob-txn.c b/tests/test-blockjob-txn.c
index 3591c9617f..edcf0de6bd 100644
--- a/tests/test-blockjob-txn.c
+++ b/tests/test-blockjob-txn.c
@@ -44,7 +44,7 @@ static void coroutine_fn test_block_job_run(void *opaque)
 
     while (s->iterations--) {
         if (s->use_timer) {
-            block_job_sleep_ns(job, 0);
+            block_job_relax(job, 0);
         } else {
             block_job_yield(job);
         }
-- 
2.14.3

[Qemu-devel] [PATCH v2 13/13] blockjob: remove block_job_pause_point from interface

[John Snow]

Remove the last call in block/mirror, using relax instead.
relax may do nothing if we are canceled, so allow iteration to return
prematurely and allow mirror_run to handle the cancellation logic.

This is a functional change to mirror that should have the effect of
cancelled mirror jobs being able to respond to that request a little
sooner instead of launching new requests.

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

diff --git a/block/mirror.c b/block/mirror.c
index 192e03694f..8e6b5b25a9 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -345,7 +345,9 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
         mirror_wait_for_io(s);
     }
 
-    block_job_pause_point(&s->common);
+    if (block_job_relax(&s->common, 0)) {
+        return 0;
+    }
 
     /* Find the number of consective dirty chunks following the first dirty
      * one, and wait for in flight requests in them. */
diff --git a/blockjob.c b/blockjob.c
index 40167d6896..27c13fdd08 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -60,6 +60,7 @@ static void __attribute__((__constructor__)) block_job_init(void)
 static void block_job_event_cancelled(BlockJob *job);
 static void block_job_event_completed(BlockJob *job, const char *msg);
 static void block_job_enter_cond(BlockJob *job, bool(*fn)(BlockJob *job));
+static int coroutine_fn block_job_pause_point(BlockJob *job);
 
 /* Transactional group of block jobs */
 struct BlockJobTxn {
@@ -793,7 +794,14 @@ static void block_job_do_yield(BlockJob *job, uint64_t ns)
     assert(job->busy);
 }
 
-int coroutine_fn block_job_pause_point(BlockJob *job)
+/**
+ * block_job_pause_point:
+ * @job: The job that is ready to pause.
+ *
+ * Pause now if block_job_pause() has been called.  Block jobs that perform
+ * lots of I/O must call this between requests so that the job can be paused.
+ */
+static int coroutine_fn block_job_pause_point(BlockJob *job)
 {
     assert(job && block_job_started(job));
 
diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index c4891a5a9b..57327cbc5a 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -201,15 +201,6 @@ void block_job_completed(BlockJob *job, int ret);
  */
 bool block_job_is_cancelled(BlockJob *job);
 
-/**
- * block_job_pause_point:
- * @job: The job that is ready to pause.
- *
- * Pause now if block_job_pause() has been called.  Block jobs that perform
- * lots of I/O must call this between requests so that the job can be paused.
- */
-int coroutine_fn block_job_pause_point(BlockJob *job);
-
 /**
  * block_job_enter:
  * @job: The job to enter.
-- 
2.14.3

Re: [Qemu-devel] [PATCH v2 00/13] blockjob: refactor mirror_throttle

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180119205847.7141-1-jsnow@redhat.com
Subject: [Qemu-devel] [PATCH v2 00/13] blockjob: refactor mirror_throttle

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 t [tag update]            patchew/20180119162554.27270-1-marcandre.lureau@redhat.com -> patchew/20180119162554.27270-1-marcandre.lureau@redhat.com
 * [new tag]               patchew/20180119205847.7141-1-jsnow@redhat.com -> patchew/20180119205847.7141-1-jsnow@redhat.com
Switched to a new branch 'test'
969dddc4fd blockjob: remove block_job_pause_point from interface
792539a07e blockjob: privatize block_job_sleep_ns
ea6f8f2359 block/mirror: remove block_job_sleep_ns calls
7f2ee60c8b block/mirror: condense cancellation and relax calls
9a16d6778e block/backup: remove yield_and_check
bbfcfe9c89 allow block_job_relax to return -ECANCELED
b1324b111c block/backup: use block_job_relax
d208db6fca block/stream: use block_job_relax
788ece436a block/commit: use block_job_relax
4e209e7aed blockjob: allow block_job_throttle to take delay_ns
76ce42710b blockjob: create block_job_relax
9771e84ab8 blockjob: consolidate SLICE_TIME definition
2a1eec68ac blockjob: record time of last entrance

=== OUTPUT BEGIN ===
Checking PATCH 1/13: blockjob: record time of last entrance...
WARNING: line over 80 characters
#52: FILE: block/mirror.c:803:
+        delta = qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - s->common.last_enter_ns;

total: 0 errors, 1 warnings, 63 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 2/13: blockjob: consolidate SLICE_TIME definition...
Checking PATCH 3/13: blockjob: create block_job_relax...
Checking PATCH 4/13: blockjob: allow block_job_throttle to take delay_ns...
Checking PATCH 5/13: block/commit: use block_job_relax...
Checking PATCH 6/13: block/stream: use block_job_relax...
Checking PATCH 7/13: block/backup: use block_job_relax...
Checking PATCH 8/13: allow block_job_relax to return -ECANCELED...
Checking PATCH 9/13: block/backup: remove yield_and_check...
Checking PATCH 10/13: block/mirror: condense cancellation and relax calls...
Checking PATCH 11/13: block/mirror: remove block_job_sleep_ns calls...
ERROR: do not initialise statics to 0 or NULL
#30: FILE: block/mirror.c:764:
+        static uint64_t delay_ns = 0;

total: 1 errors, 0 warnings, 50 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 12/13: blockjob: privatize block_job_sleep_ns...
Checking PATCH 13/13: blockjob: remove block_job_pause_point from interface...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org

Re: [Qemu-devel] [PATCH v2 00/13] blockjob: refactor mirror_throttle

[John Snow]

ping;

I won't respin for patchew until this gets looked over again, sorry :)

On 01/19/2018 03:58 PM, John Snow wrote:
> 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.
> 
> v2:
>  - Fixed spacing consistency (Paolo)
>  - Renamed last_yield_ns to last_enter_ns (Stefan)
>  - Renamed block_job_throttle to block_job_relax (Stefan)
>  - Removed external usages of block_job_pause_point and
>    block_job_sleep_ns (Paolo)
>  - Further cut down block/backup code to use block_job_relax
> 
> ________________________________________________________________________________
> 
> 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-v2:
> https://github.com/jnsnow/qemu/releases/tag/block-job-yield-v2
> 
> John Snow (13):
>   blockjob: record time of last entrance
>   blockjob: consolidate SLICE_TIME definition
>   blockjob: create block_job_relax
>   blockjob: allow block_job_throttle to take delay_ns
>   block/commit: use block_job_relax
>   block/stream: use block_job_relax
>   block/backup: use block_job_relax
>   allow block_job_relax to return -ECANCELED
>   block/backup: remove yield_and_check
>   block/mirror: condense cancellation and relax calls
>   block/mirror: remove block_job_sleep_ns calls
>   blockjob: privatize block_job_sleep_ns
>   blockjob: remove block_job_pause_point from interface
> 
>  block/backup.c               | 27 ++++++-----------------
>  block/commit.c               |  4 +---
>  block/mirror.c               | 52 +++++++++++++++-----------------------------
>  block/stream.c               |  4 +---
>  block/trace-events           |  2 +-
>  blockjob.c                   | 51 ++++++++++++++++++++++++++++++++++++-------
>  include/block/blockjob.h     |  5 +++++
>  include/block/blockjob_int.h | 37 +++++++++++++++----------------
>  tests/test-bdrv-drain.c      |  2 +-
>  tests/test-blockjob-txn.c    |  2 +-
>  10 files changed, 94 insertions(+), 92 deletions(-)
> 

Re: [Qemu-devel] [PATCH v2 01/13] blockjob: record time of last entrance

[Max Reitz]

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

On 2018-01-19 21:58, John Snow wrote:
> The mirror job makes a semi-inaccurate record of the last time we yielded
> by recording the last time we left a "pause", but this doesn't always
> correlate to the time we actually last successfully ceded control.
> 
> Record the time we last *exited* a yield centrally. In other words, record
> the time we began execution of this job to know how long we have been
> selfish for.
> 
> 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..88f4e8964d 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_enter_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_enter_ns;

The horror.

>          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);
>      }

Hmmm.  So you're now relying on block_job_sleep_ns() updating
last_enter_ns.  But it will only do so if we actually do sleep, which we
will not if the block job has been cancelled.

Then, last_enter_ns will stay at its current value indefinitely because
neither block_job_sleep_ns() nor block_job_pause_point() will yield.
OK, so at this point we should leave the mirror loop.  There are three
points where this is done:

(1) If s->ret < 0.  OK, let's hope it isn't.
(2) If cnt == 0 && should_complete.  We'll come back to that.
(3) If !s->synced && block_job_is_cancelled(...).  So basically now, if
the job has not emitted a READY event yet.

But if we have emitted the READY, we have to wait for cnt == 0 &&
should_complete (note that should_complete in turn will only be set if
s->in_flight == 0 && cnt == 0).  But unless delta < SLICE_TIME, we will
never do another mirror_iteration(), so unless we have already started
the necessary requests, they will never be started and we will loop forever.

So try this on master (I prefixed the QMP lines with in/out depending on
the direction -- note that it's important to have the block-job-cancel
on the same line the human-monitor-command finishes on so they are
executed right after each other!):

$ ./qemu-img create -f qcow2 foo.qcow2 64M
$ x86_64-softmmu/qemu-system-x86_64 -qmp stdio
In: {"QMP": {"version": {"qemu": {"micro": 50, "minor": 11, "major": 2},
"package": " (v2.9.0-632-g4a52d43-dirty)"}, "capabilities": []}}
Out: {"execute":"qmp_capabilities"}
In: {"return": {}}
Out: {"execute":"object-add","arguments":
      {"qom-type":"throttle-group","id":"tg0","props":{"limits":
         {"bps-total":16777216}}}}
In: {"return": {}}
Out: {"execute":"blockdev-add","arguments":
      {"node-name":"source","driver":"qcow2","file":
       {"driver":"file","filename":"foo.qcow2"}}}
In: {"return": {}}
Out: {"execute":"blockdev-add","arguments":
      {"node-name":"target","driver":"throttle",
       "throttle-group":"tg0","file":
       {"driver":"null-co","size":67108864}}}
In: {"return": {}}
Out: {"execute":"blockdev-mirror","arguments":
     {"job-id":"mirror","device":"source","target":"target",
      "sync":"full"}}
In: {"return": {}}
In: {"timestamp": {"seconds": 1518040566, "microseconds": 658111},
     "event": "BLOCK_JOB_READY", "data":
     {"device": "mirror", "len": 67108864, "offset": 67108864,
      "speed": 0, "type": "mirror"}}
Out: {"execute":"human-monitor-command","arguments":
      {"command-line":"qemu-io source \"write 0 64M\""}
     }{"execute":"block-job-cancel","arguments":{"device":"mirror"}}
In: wrote 67108864/67108864 bytes at offset 0
In: 64 MiB, 1 ops; 0.0310 sec (2.011 GiB/sec and 32.1823 ops/sec)
In: {"return": ""}
In: {"return": {}}
In: {"timestamp": {"seconds": 1518040578, "microseconds": 626669},
     "event": "BLOCK_JOB_COMPLETED", "data":
     {"device": "mirror", "len": 134217728, "offset": 134217728,
      "speed": 0, "type": "mirror"}}

But with your patch, the BLOCK_JOB_COMPLETED never appears (it should
take four seconds).

>  
>  immediate_exit:
> diff --git a/blockjob.c b/blockjob.c
> index f5cea84e73..2a9ff66b95 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -321,6 +321,7 @@ void block_job_start(BlockJob *job)
>      job->pause_count--;
>      job->busy = true;
>      job->paused = false;
> +    job->last_enter_ns = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
>      bdrv_coroutine_enter(blk_bs(job->blk), job->co);
>  }
>  
> @@ -786,6 +787,7 @@ static void block_job_do_yield(BlockJob *job, uint64_t ns)
>      job->busy = false;
>      block_job_unlock();
>      qemu_coroutine_yield();
> +    job->last_enter_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..e965845c94 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

A yield has two timestamps, one pre-yield, one post-yield.  Maybe it
should be noted that this records the post-yield one because that's not
what I'd assume just based on the comment, but OTOH the name of the
variable should be enough to clear that up.

Max

> +     */
> +    uint64_t last_enter_ns;
> +
>      /** Non-NULL if this job is part of a transaction */
>      BlockJobTxn *txn;
>      QLIST_ENTRY(BlockJob) txn_list;
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v2 03/13] blockjob: create block_job_relax

[Max Reitz]

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

On 2018-01-19 21:58, 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(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v2 04/13] blockjob: allow block_job_throttle to take delay_ns

[Max Reitz]

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

On 2018-01-19 21:58, 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/blockjob.c b/blockjob.c
> index 6f2e709b51..51c0eb5d9e 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -906,12 +906,11 @@ void block_job_yield(BlockJob *job)
>      block_job_pause_point(job);
>  }
>  
> -void block_job_relax(BlockJob *job)
> +void block_job_relax(BlockJob *job, int64_t delay_ns)
>  {
> -    int64_t now = qemu_clock_get_ns(QEMU_CLOCK_REALTIME);
> -
> -    if (now - job->last_enter_ns > SLICE_TIME) {
> -        block_job_sleep_ns(job, 0);
> +    if (delay_ns || (qemu_clock_get_ns(QEMU_CLOCK_REALTIME) - \
> +                     job->last_enter_ns > SLICE_TIME)) {
> +        block_job_sleep_ns(job, delay_ns);

I can't say I like the readability of that any better...

(And I'd argue that if delay_ns > 0, the one superfluous call to
qemu_clock_get_ns() isn't going to harm performance.)

>      } else {
>          block_job_pause_point(job);
>      }
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index 553784d86f..5f1520fab7 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_relax:
>   * @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 yield if so.
> + * 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.

That "Otherwise" now sounds as if it refers to the "If delay_ns is 0".

Code change is OK, but this comment needs some fixing.

Max

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v2 07/13] block/backup: use block_job_relax

[Max Reitz]

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

On 2018-01-19 21:58, John Snow wrote:
> See two commits back for justification.

When will git-log support hyperlinks so you can write "HEAD^^" here? ^^

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v2 08/13] allow block_job_relax to return -ECANCELED

[Max Reitz]

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

On 2018-01-19 21:58, John Snow wrote:
> This is just an optimization for callers who are likely going to
> want to check quite close to this call if the job was canceled or
> not anyway.

But jobs are “cancelled” and not “canceled”.

!!!

> 
> Along the same lines, add the return to block_job_pause_point and
> block_job_sleep_ns, so we don't have to re-check it quite so
> excessively.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockjob.c                   | 28 +++++++++++++++++-----------
>  include/block/blockjob_int.h |  8 +++++---
>  2 files changed, 22 insertions(+), 14 deletions(-)

[...]

> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index 5f1520fab7..1ceb47e1e6 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -147,7 +147,7 @@ void *block_job_create(const char *job_id, const BlockJobDriver *driver,
>   * %QEMU_CLOCK_REALTIME nanoseconds.  Canceling the job will immediately
>   * interrupt the wait.
>   */
> -void block_job_sleep_ns(BlockJob *job, int64_t ns);
> +int block_job_sleep_ns(BlockJob *job, int64_t ns);
>  
>  /**
>   * block_job_yield:
> @@ -167,8 +167,10 @@ void block_job_yield(BlockJob *job);
>   * 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.
> + *
> + * returns ECANCELED if the job has been canceled.

-ECANCELED, please.

With that fixed:

Reviewed-by: Max Reitz <mreitz@redhat.com>

>   */
> -void block_job_relax(BlockJob *job, int64_t delay_ns);
> +int block_job_relax(BlockJob *job, int64_t delay_ns);
>  
>  /**
>   * block_job_pause_all:
> @@ -217,7 +219,7 @@ bool block_job_is_cancelled(BlockJob *job);
>   * Pause now if block_job_pause() has been called.  Block jobs that perform
>   * lots of I/O must call this between requests so that the job can be paused.
>   */
> -void coroutine_fn block_job_pause_point(BlockJob *job);
> +int coroutine_fn block_job_pause_point(BlockJob *job);
>  
>  /**
>   * block_job_enter:
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v2 09/13] block/backup: remove yield_and_check

[Max Reitz]

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

On 2018-01-19 21:58, John Snow wrote:
> This is a respin of the same functionality as mirror_throttle,
> so trash this and replace it with the generic version.
> 
> yield_and_check returned true if canceled, false otherwise.
> block_job_relax returns -ECANCELED if canceled, 0 otherwise.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/backup.c | 20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index b4204c0ee4..0624c3b322 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -334,29 +334,17 @@ static void backup_complete(BlockJob *job, void *opaque)
>      g_free(data);
>  }
>  
> -static bool coroutine_fn yield_and_check(BackupBlockJob *job)
> +static uint64_t get_delay_ns(BackupBlockJob *job)
>  {
>      uint64_t delay_ns = 0;
>  
> -    if (block_job_is_cancelled(&job->common)) {
> -        return true;
> -    }
> -
> -    /* we need to yield so that bdrv_drain_all() returns.
> -     * (without, VM does not reboot)
> -     */
>      if (job->common.speed) {
>          delay_ns = ratelimit_calculate_delay(&job->limit,
>                                               job->bytes_read);
>          job->bytes_read = 0;
>      }
>  
> -    block_job_relax(&job->common, delay_ns);
> -    if (block_job_is_cancelled(&job->common)) {
> -        return true;
> -    }
> -
> -    return false;
> +    return delay_ns;
>  }
>  
>  static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
> @@ -369,7 +357,7 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
>      hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
>      while ((cluster = hbitmap_iter_next(&hbi)) != -1) {
>          do {
> -            if (yield_and_check(job)) {
> +            if (block_job_relax(&job->common, get_delay_ns(job))) {
>                  return 0;
>              }
>              ret = backup_do_cow(job, cluster * job->cluster_size,
> @@ -465,7 +453,7 @@ static void coroutine_fn backup_run(void *opaque)
>              bool error_is_read;
>              int alloced = 0;
>  
> -            if (yield_and_check(job)) {
> +            if (block_job_relax(&job->common, get_delay_ns(job))) {
>                  break;
>              }
>  
> 

I'd very much prefer an explicit block_job_relax(...) == -ECANCELED, I
have to say.

If you coerce me, I can give an R-b, but you'll have to wrest it from me
by force!

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v2 10/13] block/mirror: condense cancellation and relax calls

[Max Reitz]

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

On 2018-01-19 21:58, John Snow wrote:
> We can count on the relax call to check cancellation for us, so
> condense these concurrent calls.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/mirror.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 3c73caed5e..a0e0044de2 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -610,9 +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_relax(&s->common, 0);
> -
> -            if (block_job_is_cancelled(&s->common)) {
> +            if (block_job_relax(&s->common, 0)) {
>                  s->initial_zeroing_ongoing = false;
>                  return 0;
>              }
> @@ -638,9 +636,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_relax(&s->common, 0);
> -
> -        if (block_job_is_cancelled(&s->common)) {
> +        if (block_job_relax(&s->common, 0)) {
>              return 0;
>          }

“See <a href="HEAD^">last patch</a>.”


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v2 11/13] block/mirror: remove block_job_sleep_ns calls

[Max Reitz]

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

On 2018-01-19 21:58, John Snow wrote:
> We're attempting to slacken the mirror loop in three different places,
> but we can combine these three attempts. Combine the early loop call to
> block_job_pause_point with the two late-loop calls to block_job_sleep_ns.
> 
> When delay_ns is 0 and it has not been SLICE_TIME since the last yield,
> block_job_relax is merely a call to block_job_pause_point, so this should
> be equivalent with the exception that if we have managed to not yield at
> all in the last SLICE_TIME ns, we will now do so.
> 
> I am not sure that condition was possible,
> so this loop should be equivalent.

Well, to me it even sounds like a positive change if it was a change.
We want the job to yield after SLICE_TIME ns, after all, and I don't
think it matters where that happens, exactly.

> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/mirror.c     | 22 +++++++++++-----------
>  block/trace-events |  2 +-
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index a0e0044de2..192e03694f 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -761,7 +761,7 @@ static void coroutine_fn mirror_run(void *opaque)
>      assert(!s->dbi);
>      s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap);
>      for (;;) {
> -        uint64_t delay_ns = 0;
> +        static uint64_t delay_ns = 0;

Errr.  Are you sure about that?

Now every mirror job in the qeny process will share this single
variable.  Was that your intention?

>          int64_t cnt, delta;
>          bool should_complete;
>  
> @@ -770,9 +770,16 @@ static void coroutine_fn mirror_run(void *opaque)
>              goto immediate_exit;
>          }
>  
> -        block_job_pause_point(&s->common);
> -
>          cnt = bdrv_get_dirty_count(s->dirty_bitmap);
> +
> +        trace_mirror_before_relax(s, cnt, s->synced, delay_ns);
> +        if (block_job_relax(&s->common, delay_ns)) {

See the reply to <a href="HEAD^^">that patch</a>.

> +            if (!s->synced) {
> +                goto immediate_exit;
> +            }
> +        }
> +        delay_ns = 0;
> +
>          /* s->common.offset contains the number of bytes already processed so
>           * far, cnt is the number of dirty bytes remaining and
>           * s->bytes_in_flight is the number of bytes currently being
> @@ -849,15 +856,8 @@ static void coroutine_fn mirror_run(void *opaque)
>          }
>  
>          ret = 0;
> -        trace_mirror_before_sleep(s, cnt, s->synced, delay_ns);
> -        if (!s->synced) {
> -            block_job_sleep_ns(&s->common, delay_ns);
> -            if (block_job_is_cancelled(&s->common)) {
> -                break;
> -            }
> -        } else if (!should_complete) {
> +        if (s->synced && !should_complete) {
>              delay_ns = (s->in_flight == 0 && cnt == 0 ? SLICE_TIME : 0);
> -            block_job_sleep_ns(&s->common, delay_ns);
>          }
>      }

Basic idea looks good to me (apart from the static delay_ns), but, well,
block-job-cancel on a busy job still breaks.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v2 12/13] blockjob: privatize block_job_sleep_ns

[Max Reitz]

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

On 2018-01-19 21:58, John Snow wrote:
> There's not currently any external caller of it.
> 
> Except in tests, but we'll fix that here too.
> 
> Replace usages in test cases with block_job_relax, which functions
> similarly enough to be used as a drop-in replacement.
> 
> Very technically block_job_sleep_ns(job, 0) behaves differently
> from block_job_relax(job, 0) in that relax may resolve to a no-op,
> but this makes no difference in the test in which it is used.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  blockjob.c                   | 11 ++++++++++-
>  include/block/blockjob_int.h | 11 -----------
>  tests/test-bdrv-drain.c      |  2 +-
>  tests/test-blockjob-txn.c    |  2 +-
>  4 files changed, 12 insertions(+), 14 deletions(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v2 13/13] blockjob: remove block_job_pause_point from interface

[Max Reitz]

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

On 2018-01-19 21:58, John Snow wrote:
> Remove the last call in block/mirror, using relax instead.
> relax may do nothing if we are canceled, so allow iteration to return
> prematurely and allow mirror_run to handle the cancellation logic.

Ah, now you write it with two l? ;-)

> 
> This is a functional change to mirror that should have the effect of
> cancelled mirror jobs being able to respond to that request a little

??!??!  Such inconsistency.  Many l.

> sooner instead of launching new requests.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/mirror.c               |  4 +++-
>  blockjob.c                   | 10 +++++++++-
>  include/block/blockjob_int.h |  9 ---------
>  3 files changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/block/mirror.c b/block/mirror.c
> index 192e03694f..8e6b5b25a9 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -345,7 +345,9 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s)
>          mirror_wait_for_io(s);
>      }
>  
> -    block_job_pause_point(&s->common);
> +    if (block_job_relax(&s->common, 0)) {
> +        return 0;
> +    }

:c

>  
>      /* Find the number of consective dirty chunks following the first dirty
>       * one, and wait for in flight requests in them. */
> diff --git a/blockjob.c b/blockjob.c
> index 40167d6896..27c13fdd08 100644
> --- a/blockjob.c
> +++ b/blockjob.c
> @@ -60,6 +60,7 @@ static void __attribute__((__constructor__)) block_job_init(void)
>  static void block_job_event_cancelled(BlockJob *job);
>  static void block_job_event_completed(BlockJob *job, const char *msg);
>  static void block_job_enter_cond(BlockJob *job, bool(*fn)(BlockJob *job));
> +static int coroutine_fn block_job_pause_point(BlockJob *job);
>  
>  /* Transactional group of block jobs */
>  struct BlockJobTxn {
> @@ -793,7 +794,14 @@ static void block_job_do_yield(BlockJob *job, uint64_t ns)
>      assert(job->busy);
>  }
>  
> -int coroutine_fn block_job_pause_point(BlockJob *job)
> +/**
> + * block_job_pause_point:
> + * @job: The job that is ready to pause.
> + *
> + * Pause now if block_job_pause() has been called.  Block jobs that perform
> + * lots of I/O must call this between requests so that the job can be paused.

But jobs can't call this anymore, now.  This part of the comment should
either mention block_job_relax() instead or should be moved there
altogether.

Max

> + */
> +static int coroutine_fn block_job_pause_point(BlockJob *job)
>  {
>      assert(job && block_job_started(job));
>  
> diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
> index c4891a5a9b..57327cbc5a 100644
> --- a/include/block/blockjob_int.h
> +++ b/include/block/blockjob_int.h
> @@ -201,15 +201,6 @@ void block_job_completed(BlockJob *job, int ret);
>   */
>  bool block_job_is_cancelled(BlockJob *job);
>  
> -/**
> - * block_job_pause_point:
> - * @job: The job that is ready to pause.
> - *
> - * Pause now if block_job_pause() has been called.  Block jobs that perform
> - * lots of I/O must call this between requests so that the job can be paused.
> - */
> -int coroutine_fn block_job_pause_point(BlockJob *job);
> -
>  /**
>   * block_job_enter:
>   * @job: The job to enter.
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v2 11/13] block/mirror: remove block_job_sleep_ns calls

[Max Reitz]

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

On 2018-02-07 23:46, Max Reitz wrote:
> On 2018-01-19 21:58, John Snow wrote:
>> We're attempting to slacken the mirror loop in three different places,
>> but we can combine these three attempts. Combine the early loop call to
>> block_job_pause_point with the two late-loop calls to block_job_sleep_ns.
>>
>> When delay_ns is 0 and it has not been SLICE_TIME since the last yield,
>> block_job_relax is merely a call to block_job_pause_point, so this should
>> be equivalent with the exception that if we have managed to not yield at
>> all in the last SLICE_TIME ns, we will now do so.
>>
>> I am not sure that condition was possible,
>> so this loop should be equivalent.
> 
> Well, to me it even sounds like a positive change if it was a change.
> We want the job to yield after SLICE_TIME ns, after all, and I don't
> think it matters where that happens, exactly.
> 
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/mirror.c     | 22 +++++++++++-----------
>>  block/trace-events |  2 +-
>>  2 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index a0e0044de2..192e03694f 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -761,7 +761,7 @@ static void coroutine_fn mirror_run(void *opaque)
>>      assert(!s->dbi);
>>      s->dbi = bdrv_dirty_iter_new(s->dirty_bitmap);
>>      for (;;) {
>> -        uint64_t delay_ns = 0;
>> +        static uint64_t delay_ns = 0;
> 
> Errr.  Are you sure about that?
> 
> Now every mirror job in the qeny process will share this single
> variable.  Was that your intention?

("Errr" @myself for "qeny")


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v2 11/13] block/mirror: remove block_job_sleep_ns calls

[John Snow]

On 02/07/2018 05:46 PM, Max Reitz wrote:
> Errr.  Are you sure about that?
> 
> Now every mirror job in the qeny process will share this single
> variable.  Was that your intention?
> 

Duh, no.

Thanks for the review, also. I'm currently sidetracked on something
else, so I'll get back to this soon.