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 <kw...@redhat.com> > > Reviewed-by: Eric Blake <ebl...@redhat.com> > > --- > > 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
