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

[John Snow]

On 7/10/19 4:30 PM, Max Reitz wrote:
> 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>
> 

I mean. I'll make it consistent either way, but I actually don't know
which way I should make it go. I just think that all the modes should
work the same if we can help it.

Flip a coin?

--js