Re: [Qemu-devel] [PATCH 6/8] block/backup: issue progress updates for skipped regions

[Max Reitz]

On 10.07.19 20:20, John Snow wrote:
> 
> 
> On 7/10/19 12:36 PM, Max Reitz wrote:
>> On 10.07.19 03:05, John Snow wrote:
>>> The way bitmap backups work is by starting at 75% if it needs
>>> to copy just 25% of the disk.
>>
>> Although there is this comment:
>>
>>> /* TODO job_progress_set_remaining() would make more sense */
>>
>> So...
>>
>>> The way sync=top currently works, however, is to start at 0% and then
>>> never update the progress if it doesn't copy a region. If it needs to
>>> copy 25% of the disk, we'll finish at 25%.
>>>
>>> Update the progress when we skip regions.
>>
>> Wouldn’t it be more correct to decrease the job length?
>>
>> Max
>>
> 
> Admittedly I have precisely no idea. Maybe? As far as I understand it,
> we guarantee only:
> 
> (1) Progress monotonically increases
> (2) Upon completion, progress will equal the total work estimate.
>     [Trying to fix that to be true here.]
> 
> This means we can do stuff like:
> 
> - Total work estimate can increase or decrease arbitrarily
> - Neither value has to mean anything in particular
> 
> 
> Bitmap sync works by artificially increasing progress for NOP regions,
> seen in init_copy_bitmap.

Yes, and it has a TODO comment that says it should be done differently.

> Full sync also tends to increase progress regardless of it actually did
> a copy or not; offload support also counts as progress here. So if you
> full sync an empty image, you'll see it increasing the progress as it
> doesn't actually do anything.
> 
> Top sync is the odd one out, which just omits progress for regions it skips.
> 
> My only motivation here was to make them consistent. Can I do it the
> other way? Yeah, probably. Is one way better than the other? I
> legitimately have no idea. I guess whoever wrote the last comment felt
> that it should all be the other way instead. Why'd they not do that?

If you look at the commit (05df8a6a2b4), I suppose it was because that
commit simply did not intend to change behavior.  It just touched that
piece of code and noted that maybe there should be a follow-up commit to
change it.

But yeah, whatever.

Reviewed-by: Max Reitz <address@hidden>

signature.asc
Description: OpenPGP digital signature