Re: [PATCH 7/8] blockjob: Adhere to rate limit even when reentered early

[Eric Blake]

On Wed, May 10, 2023 at 10:36:00PM +0200, Kevin Wolf wrote:
> 
> When jobs are sleeping, for example to enforce a given rate limit, they
> can be reentered early, in particular in order to get paused, to update
> the rate limit or to get cancelled.
> 
> Before this patch, they behave in this case as if they had fully
> completed their rate limiting delay. This means that requests are sped
> up beyond their limit, violating the constraints that the user gave us.

Aha - our tests ARE non-deterministic!  Good find.

> 
> Change the block jobs to sleep in a loop until the necessary delay is
> completed, while still allowing cancelling them immediately as well
> pausing (handled by the pause point in job_sleep_ns()) and updating the
> rate limit.
> 
> This change is also motivated by iotests cases being prone to fail
> because drain operations pause and unpause them so often that block jobs
> complete earlier than they are supposed to. In particular, the next
> commit would fail iotests 030 without this change.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  include/block/blockjob_int.h | 14 ++++++++++----
>  block/commit.c               |  7 ++-----
>  block/mirror.c               | 23 ++++++++++-------------
>  block/stream.c               |  7 ++-----
>  blockjob.c                   | 22 ++++++++++++++++++++--
>  5 files changed, 44 insertions(+), 29 deletions(-)
> 
> +++ b/blockjob.c
> @@ -319,10 +319,28 @@ static bool block_job_set_speed(BlockJob *job, int64_t 
> speed, Error **errp)
>      return block_job_set_speed_locked(job, speed, errp);
>  }
>  
> -int64_t block_job_ratelimit_get_delay(BlockJob *job, uint64_t n)
> +void block_job_ratelimit_processed_bytes(BlockJob *job, uint64_t n)
>  {
>      IO_CODE();
> -    return ratelimit_calculate_delay(&job->limit, n);
> +    ratelimit_calculate_delay(&job->limit, n);
> +}
> +
> +void block_job_ratelimit_sleep(BlockJob *job)
> +{
> +    uint64_t delay_ns;
> +
> +    /*
> +     * Sleep at least once. If the job is reentered early, keep waiting until
> +     * we've waited for the full time that is necessary to keep the job at 
> the
> +     * right speed.
> +     *
> +     * Make sure to recalculate the delay after each (possibly interrupted)
> +     * sleep because the speed can change while the job has yielded.
> +     */
> +    do {
> +        delay_ns = ratelimit_calculate_delay(&job->limit, 0);
> +        job_sleep_ns(&job->job, delay_ns);
> +    } while (delay_ns && !job_is_cancelled(&job->job));
>  }

Yes, that looks more robust.

Reviewed-by: Eric Blake <eblake@redhat.com>

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