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

[Max Reitz]

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