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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 1700 bytes --]

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: qemu-stable@nongnu.org
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> ---
>  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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]