[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH 02/42] blockjob: Wrappers for progress counter a
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH 02/42] blockjob: Wrappers for progress counter access |
Date: |
Sat, 12 May 2018 00:12:19 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 |
On 2018-05-09 18:25, Kevin Wolf wrote:
> Block job drivers are not expected to mess with the internals of the
> BlockJob object, so provide wrapper functions for one of the cases where
> they still do it: Updating the progress counter.
>
> Signed-off-by: Kevin Wolf <address@hidden>
> Reviewed-by: Eric Blake <address@hidden>
> ---
> include/block/blockjob.h | 19 +++++++++++++++++++
> block/backup.c | 22 +++++++++++++---------
> block/commit.c | 16 ++++++++--------
> block/mirror.c | 11 +++++------
> block/stream.c | 14 ++++++++------
> blockjob.c | 10 ++++++++++
> 6 files changed, 63 insertions(+), 29 deletions(-)
>
[...]
> diff --git a/block/backup.c b/block/backup.c
> index 453cd62c24..5d95805472 100644
> --- a/block/backup.c
> +++ b/block/backup.c
>
[...]
> @@ -420,8 +421,9 @@ static void
> backup_incremental_init_copy_bitmap(BackupBlockJob *job)
> bdrv_set_dirty_iter(dbi, next_cluster * job->cluster_size);
> }
>
> - job->common.offset = job->common.len -
> - hbitmap_count(job->copy_bitmap) * job->cluster_size;
> + /* TODO block_job_progress_set_remaining() would make more sense */
Extremely true, especially considering that at least there was an
assignment before.
> + block_job_progress_update(&job->common,
> + job->len - hbitmap_count(job->copy_bitmap) * job->cluster_size);
Now, with an incremental update, you have to know that the progress was
0 before this call to make any sense of it.
I could ask: Why don't you just resolve the TODO immediately with
block_job_progress_set_remaining(&job->common,
hbitmap_count(job->copy_bitmap) * job->cluster_size);
?
I suppose one possible answer is that this series has 42 patches as it
is, but I have to say that it took me more time to figure this hunk out
than it would have taken me to acknowledge the above change.
Considering that job->len and job->common.len are now separate after
this patch, and that there is only a single other
block_job_progress_update() call in this file, I can't see any side effects.
>
> bdrv_dirty_iter_free(dbi);
> }
[...]
> diff --git a/block/mirror.c b/block/mirror.c
> index 99da9c0858..77ee9b1791 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
[...]
> @@ -792,11 +792,10 @@ static void coroutine_fn mirror_run(void *opaque)
> block_job_pause_point(&s->common);
>
> cnt = bdrv_get_dirty_count(s->dirty_bitmap);
> - /* 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
> - * processed; together those are the current total operation length
> */
> - s->common.len = s->common.offset + s->bytes_in_flight + cnt;
> + /* cnt is the number of dirty bytes remaining and s->bytes_in_flight
> is
> + * the number of bytes currently being processed; together those are
> + * the current total operation length */
No, together, those are the current remaining operation length.
With that fixed:
Reviewed-by: Max Reitz <address@hidden>
> + block_job_progress_set_remaining(&s->common, s->bytes_in_flight +
> cnt);
>
> /* Note that even when no rate limit is applied we need to yield
> * periodically with no pending I/O so that bdrv_drain_all() returns.
signature.asc
Description: OpenPGP digital signature