On 10.07.19 22:47, John Snow wrote:
> 
> 
> 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 <mreitz@redhat.com>
>>
> 
> 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?

If you’d flip a coin, I can say that I’d find it a bit more meaningful
to reduce the length.  Especially if you change sync=top to calculate
beforehand how much we need to copy, so the length isn’t even reduced
while the job is running.

And it’s easier to remove the TODO comment this way.  If we decide
against it, you’d have to remove the TODO comment aso (because we
decided against it), but you’d need to justify it in the commit message.
 And we all know that writing explanations and documentation is the
hardest thing of all.

Max