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 <js...@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:
>
