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

[John Snow <jsnow@redhat.com>]

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.

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?

¯\_(ツ)_/¯

>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>  block/backup.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/block/backup.c b/block/backup.c
>> index a64b768e24..38c4a688c6 100644
>> --- a/block/backup.c
>> +++ b/block/backup.c
>> @@ -417,6 +417,7 @@ static int coroutine_fn backup_loop(BackupBlockJob *job)
>>          if (job->sync_mode == MIRROR_SYNC_MODE_TOP &&
>>              bdrv_is_unallocated_range(bs, offset, job->cluster_size))
>>          {
>> +            job_progress_update(&job->common.job, job->cluster_size);
>>              bdrv_reset_dirty_bitmap(job->copy_bitmap, offset,
>>                                      job->cluster_size);
>>              continue;
>>
> 
>