[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: |
Kevin Wolf |
Subject: |
Re: [Qemu-block] [PATCH 02/42] blockjob: Wrappers for progress counter access |
Date: |
Mon, 14 May 2018 12:16:36 +0200 |
User-agent: |
Mutt/1.9.1 (2017-09-22) |
Am 12.05.2018 um 00:12 hat Max Reitz geschrieben:
> 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.
Basically just because I tried to make the naive change whenever I had
to touch something that isn't what the patch changes as its main
purpose. The old code changed offset rather than len, so I used the
function that does the same.
If I reduced len instead of increasing offset, I suppose that at least a
few test cases would have to be updated etc. and who knows what else
(QMP clients shouldn't rely on the current way, but do they?).
I'd rather not make such a change as a side effect of a patch that tries
to do something quite different.
> >
> > 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.
Thanks, will fix.
Kevin
signature.asc
Description: PGP signature
- [Qemu-block] [PATCH 00/42] Generic background jobs, Kevin Wolf, 2018/05/09
- [Qemu-block] [PATCH 01/42] blockjob: Fix assertion in block_job_finalize(), Kevin Wolf, 2018/05/09
- [Qemu-block] [PATCH 02/42] blockjob: Wrappers for progress counter access, Kevin Wolf, 2018/05/09
- [Qemu-block] [PATCH 07/42] blockjob: Remove block_job_pause/resume_all(), Kevin Wolf, 2018/05/09
- [Qemu-block] [PATCH 03/42] blockjob: Move RateLimit to BlockJob, Kevin Wolf, 2018/05/09
- [Qemu-block] [PATCH 04/42] blockjob: Implement block_job_set_speed() centrally, Kevin Wolf, 2018/05/09