Re: [Qemu-devel] [RFC PATCH 04/33] blockjob: Introduce block_job_ratelimit_get_delay()

[Eric Blake]

On 04/24/2018 10:24 AM, Kevin Wolf wrote:
> This gets us rid of more direct accesses to BlockJob fields from the
> job drivers.
> 
> Signed-off-by: Kevin Wolf 
> ---

> +++ b/include/block/blockjob_int.h
> @@ -166,6 +166,14 @@ void block_job_sleep_ns(BlockJob *job, int64_t ns);
>  void block_job_yield(BlockJob *job);
>  
>  /**
> + * block_job_ratelimit_get_delay:
> + *
> + * Calculate and return delay for the next request in ns. See the 
> docuemntation

s/docuemntation/documentation/

> + * of ratelimit_calculate_delay() for details.
> + */
> +int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n);

Signed return type matches ratelimit_calculate_delay, but what does a
negative return really mean?  Hmm, it looks like it can't actually
return a negative, other than if qemu_clock_get_ns() returns a negative
on some sort of error (which appears to be unlikely - we don't even
bother checking for it, even though it would mess things up if it did
happen) - all other return paths are 0 or a subtraction between time
values that can't overflow a 64-bit integer in our lifetime.  Should we
audit the source to flip the callers to use an unsigned value, perhaps
as a separate cleanup?

> +
> +/**
>   * block_job_pause_all:
>   *
>   * Asynchronously pause all jobs.
> diff --git a/block/backup.c b/block/backup.c
> index 8468fd9f94..3f3ec6e408 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -325,21 +325,17 @@ static void backup_complete(BlockJob *job, void *opaque)
>  
>  static bool coroutine_fn yield_and_check(BackupBlockJob *job)
>  {
> +uint64_t delay_ns;
> +
>  if (block_job_is_cancelled(>common)) {
>  return true;
>  }
>  
> -/* we need to yield so that bdrv_drain_all() returns.
> - * (without, VM does not reboot)
> - */
> -if (job->common.speed) {
> -uint64_t delay_ns = ratelimit_calculate_delay(>common.limit,
> -  job->bytes_read);
> -job->bytes_read = 0;
> -block_job_sleep_ns(>common, delay_ns);
> -} else {
> -block_job_sleep_ns(>common, 0);
> -}
> +/* we need to yield even for delay_ns = 0 so that bdrv_drain_all() 
> returns.
> + * (without it, the VM does not reboot) */

As long as you are touching this, is it worth further improving the grammar?

/* We need to yield even for delay_ns = 0 so that bdrv_drain_all() can
return; without a yield, the VM would not reboot. */

> +delay_ns = block_job_ratelimit_get_delay(>common, job->bytes_read);
> +job->bytes_read = 0;
> +block_job_sleep_ns(>common, delay_ns);
>  
>  if (block_job_is_cancelled(>common)) {
>  return true;

With typo fixes,
Reviewed-by: Eric Blake 

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

[Qemu-devel] [RFC PATCH 04/33] blockjob: Introduce block_job_ratelimit_get_delay()

[Kevin Wolf]

This gets us rid of more direct accesses to BlockJob fields from the
job drivers.

Signed-off-by: Kevin Wolf 
---
 include/block/blockjob_int.h |  8 
 block/backup.c   | 18 +++---
 block/commit.c   |  4 ++--
 block/mirror.c   |  5 +
 block/stream.c   |  4 ++--
 blockjob.c   |  9 +
 6 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/include/block/blockjob_int.h b/include/block/blockjob_int.h
index 870bd346a8..d26115207b 100644
--- a/include/block/blockjob_int.h
+++ b/include/block/blockjob_int.h
@@ -166,6 +166,14 @@ void block_job_sleep_ns(BlockJob *job, int64_t ns);
 void block_job_yield(BlockJob *job);
 
 /**
+ * block_job_ratelimit_get_delay:
+ *
+ * Calculate and return delay for the next request in ns. See the docuemntation
+ * of ratelimit_calculate_delay() for details.
+ */
+int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n);
+
+/**
  * block_job_pause_all:
  *
  * Asynchronously pause all jobs.
diff --git a/block/backup.c b/block/backup.c
index 8468fd9f94..3f3ec6e408 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -325,21 +325,17 @@ static void backup_complete(BlockJob *job, void *opaque)
 
 static bool coroutine_fn yield_and_check(BackupBlockJob *job)
 {
+uint64_t delay_ns;
+
 if (block_job_is_cancelled(>common)) {
 return true;
 }
 
-/* we need to yield so that bdrv_drain_all() returns.
- * (without, VM does not reboot)
- */
-if (job->common.speed) {
-uint64_t delay_ns = ratelimit_calculate_delay(>common.limit,
-  job->bytes_read);
-job->bytes_read = 0;
-block_job_sleep_ns(>common, delay_ns);
-} else {
-block_job_sleep_ns(>common, 0);
-}
+/* we need to yield even for delay_ns = 0 so that bdrv_drain_all() returns.
+ * (without it, the VM does not reboot) */
+delay_ns = block_job_ratelimit_get_delay(>common, job->bytes_read);
+job->bytes_read = 0;
+block_job_sleep_ns(>common, delay_ns);
 
 if (block_job_is_cancelled(>common)) {
 return true;
diff --git a/block/commit.c b/block/commit.c
index 46cbeaec3e..ba5df6aa0a 100644
--- a/block/commit.c
+++ b/block/commit.c
@@ -197,8 +197,8 @@ static void coroutine_fn commit_run(void *opaque)
 /* Publish progress */
 block_job_progress_update(>common, n);
 
-if (copy && s->common.speed) {
-delay_ns = ratelimit_calculate_delay(>common.limit, n);
+if (copy) {
+delay_ns = block_job_ratelimit_get_delay(>common, n);
 } else {
 delay_ns = 0;
 }
diff --git a/block/mirror.c b/block/mirror.c
index de495fddc4..1cbdb1e0d8 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -447,10 +447,7 @@ static uint64_t coroutine_fn 
mirror_iteration(MirrorBlockJob *s)
 assert(io_bytes);
 offset += io_bytes;
 nb_chunks -= DIV_ROUND_UP(io_bytes, s->granularity);
-if (s->common.speed) {
-delay_ns = ratelimit_calculate_delay(>common.limit,
- io_bytes_acct);
-}
+delay_ns = block_job_ratelimit_get_delay(>common, io_bytes_acct);
 }
 return delay_ns;
 }
diff --git a/block/stream.c b/block/stream.c
index 797d7c4f21..df9660d2fc 100644
--- a/block/stream.c
+++ b/block/stream.c
@@ -185,8 +185,8 @@ static void coroutine_fn stream_run(void *opaque)
 
 /* Publish progress */
 block_job_progress_update(>common, n);
-if (copy && s->common.speed) {
-delay_ns = ratelimit_calculate_delay(>common.limit, n);
+if (copy) {
+delay_ns = block_job_ratelimit_get_delay(>common, n);
 } else {
 delay_ns = 0;
 }
diff --git a/blockjob.c b/blockjob.c
index 9b79abc821..31130d87cc 100644
--- a/blockjob.c
+++ b/blockjob.c
@@ -680,6 +680,15 @@ void block_job_set_speed(BlockJob *job, int64_t speed, 
Error **errp)
 block_job_enter_cond(job, block_job_timer_pending);
 }
 
+int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n)
+{
+if (!job->speed) {
+return 0;
+}
+
+return ratelimit_calculate_delay(>limit, n);
+}
+
 void block_job_complete(BlockJob *job, Error **errp)
 {
 /* Should not be reachable via external interface for internal jobs */
-- 
2.13.6