Re: [PATCH v3 2/9] block/block-copy: fix progress calculation

[Max Reitz]

On 06.03.20 08:38, Vladimir Sementsov-Ogievskiy wrote:
> Assume we have two regions, A and B, and region B is in-flight now,
> region A is not yet touched, but it is unallocated and should be
> skipped.
> 
> Correspondingly, as progress we have
> 
>   total = A + B
>   current = 0
> 
> If we reset unallocated region A and call progress_reset_callback,
> it will calculate 0 bytes dirty in the bitmap and call
> job_progress_set_remaining, which will set
> 
>    total = current + 0 = 0 + 0 = 0
> 
> So, B bytes are actually removed from total accounting. When job
> finishes we'll have
> 
>    total = 0
>    current = B
> 
> , which doesn't sound good.
> 
> This is because we didn't considered in-flight bytes, actually when
> calculating remaining, we should have set (in_flight + dirty_bytes)
> as remaining, not only dirty_bytes.
> 
> To fix it, let's refactor progress calculation, moving it to block-copy
> itself instead of fixing callback. And, of course, track in_flight
> bytes count.
> 
> We still have to keep one callback, to maintain backup job bytes_read
> calculation, but it will go on soon, when we turn the whole backup
> process into one block_copy call.
> 
> Cc: address@hidden
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Reviewed-by: Andrey Shinkevich <address@hidden>
> ---
>  include/block/block-copy.h | 14 +++++---------
>  block/backup.c             | 13 ++-----------
>  block/block-copy.c         | 16 ++++++++++++----
>  3 files changed, 19 insertions(+), 24 deletions(-)

Looks good, but I suppose we should also drop the
ProgressResetCallbackFunc type.

Max

signature.asc
Description: OpenPGP digital signature