Re: [Qemu-devel] [PATCH 02/42] blockjob: Wrappers for progress counter access

[Kevin Wolf]

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