[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC PATCH 04/33] blockjob: Introduce block_job_ratelim
From: |
Eric Blake |
Subject: |
Re: [Qemu-devel] [RFC PATCH 04/33] blockjob: Introduce block_job_ratelimit_get_delay() |
Date: |
Tue, 24 Apr 2018 16:57:59 -0500 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 |
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 <address@hidden>
> ---
> +++ 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(&job->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(&job->common.limit,
> - job->bytes_read);
> - job->bytes_read = 0;
> - block_job_sleep_ns(&job->common, delay_ns);
> - } else {
> - block_job_sleep_ns(&job->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(&job->common, job->bytes_read);
> + job->bytes_read = 0;
> + block_job_sleep_ns(&job->common, delay_ns);
>
> if (block_job_is_cancelled(&job->common)) {
> return true;
With typo fixes,
Reviewed-by: Eric Blake <address@hidden>
--
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 00/33] Generic background jobs, Kevin Wolf, 2018/04/24
- [Qemu-devel] [RFC PATCH 04/33] blockjob: Introduce block_job_ratelimit_get_delay(), Kevin Wolf, 2018/04/24
- Re: [Qemu-devel] [RFC PATCH 04/33] blockjob: Introduce block_job_ratelimit_get_delay(),
Eric Blake <=
- [Qemu-devel] [RFC PATCH 03/33] blockjob: Implement block_job_set_speed() centrally, Kevin Wolf, 2018/04/24
- [Qemu-devel] [RFC PATCH 02/33] blockjob: Move RateLimit to BlockJob, Kevin Wolf, 2018/04/24
- [Qemu-devel] [RFC PATCH 01/33] blockjob: Wrappers for progress counter access, Kevin Wolf, 2018/04/24
- [Qemu-devel] [RFC PATCH 06/33] blockjob: Remove block_job_pause/resume_all(), Kevin Wolf, 2018/04/24
- [Qemu-devel] [RFC PATCH 05/33] blockjob: Add block_job_driver(), Kevin Wolf, 2018/04/24
- [Qemu-devel] [RFC PATCH 08/33] job: Rename BlockJobType into JobType, Kevin Wolf, 2018/04/24