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 > Reviewed-by: Eric Blake > --- > 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 > + 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.