Re: [Qemu-devel] [PATCH 4/5] backup: simplify non-dirty bits progress processing

[John Snow]

On 10/02/2017 10:39 AM, Vladimir Sementsov-Ogievskiy wrote:
> Set fake progress for non-dirty clusters in copy_bitmap initialization,
> to. It simplifies code and allows further refactoring.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
> 
> Motivation (some of these paragraphs may be needed in commit message...)
> 

lol! "pick and choose which paragraphs look good, stick some up there,
maybe?"

> 1. Current behavior is not good: setting fake progress leads to progress
> hopping, so actually for sparse disk progress say nothing. We just move
> all these hops to the beginning.
> 

To be fair, if it's sparse, the first update will be very soon.

> 2. Big hop in the beginning is possible now too: if there is a hole at
> the beginning of virtual disk.
> 
> 3. Now, there are some excuses for old behavior in comparison to new one:
> backup progress is linear, cluster by cluster. But in future cluster
> copying will be done by several coroutine-workers, several requests in
> parallel, so it will make no sense to publish fake progress by parts in
> parallel with non-sequential requests.
> 

I agree.

> 4. Actually it's just a point of view: when we are actually skip clusters?
> If go through disk sequentially, it's logical to say, that we skip clusters
> between copied portions to the left and to the right of them. But even know
> copying progress is not sequential because of write notifiers.
> If copying is async, non-sequential, we can say in the very beginning, that
> we skip these clusters and do not think about them later.
> 

Good observation.

> So, I don't want to maintain portions of fake progress in the new backup
> architecture. I understand, that this patch will change user's view
> of backup progress, but formally it doesn't change: progress hops are just
> moved to the beginning. Progress was meaningless for incremental backup and
> it remains meaningless.
> 

Haha. Not meaningless, but it does carry less information than other
progress meters. It is effectively a % complete meter, with a really bad
estimate.

> But what should we do with progress, really? It's obvious, that we need at
> least one more field for block job status, for example, "skipped", which
> would be amount of bytes which are not really processed but skipped by the
> block job. So, more real progress may be calculated (by management tool) like
> 
> (job.offset - job.skipped) / (job.len - job.skipped)
> 

I'm not sure we actually need a new field... let's just say that the job
length is the number of bytes described by the incremental backup. Every
time we copy some, move offset forward. This would give a more
appropriately linear/accurate sense of the progress.

I think we are allowed to do this as I think we promise that these
progress numbers mean essentially nothing...

New fields that actually *do* report meaningful information, however,
could be added. bytesCopied would be a prime candidate.

> And this progress will be more precise if information about skipped bytes
> is provided earlier, and current patch is close to this idea.
> 
> So, if you agree, I can make a patch for 'skipped' too, but it actually not
> related to the series, so I hope that current patch will be merged anyway.
> (I need it for backup refactoring, not for accurate progress)
> 

>  block/backup.c | 18 +++---------------
>  1 file changed, 3 insertions(+), 15 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 07f4ae846b..52df7bb19e 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -369,7 +369,6 @@ static int coroutine_fn 
> backup_run_incremental(BackupBlockJob *job)
>      int64_t offset;
>      int64_t cluster;
>      int64_t end;
> -    int64_t last_cluster = -1;
>      BdrvDirtyBitmapIter *dbi;
>  
>      granularity = bdrv_dirty_bitmap_granularity(job->sync_bitmap);
> @@ -380,12 +379,6 @@ static int coroutine_fn 
> backup_run_incremental(BackupBlockJob *job)
>      while ((offset = bdrv_dirty_iter_next(dbi)) >= 0) {
>          cluster = offset / job->cluster_size;
>  
> -        /* Fake progress updates for any clusters we skipped */
> -        if (cluster != last_cluster + 1) {
> -            job->common.offset += ((cluster - last_cluster - 1) *
> -                                   job->cluster_size);
> -        }
> -
>          for (end = cluster + clusters_per_iter; cluster < end; cluster++) {
>              do {
>                  if (yield_and_check(job)) {
> @@ -407,14 +400,6 @@ static int coroutine_fn 
> backup_run_incremental(BackupBlockJob *job)
>          if (granularity < job->cluster_size) {
>              bdrv_set_dirty_iter(dbi, cluster * job->cluster_size);
>          }
> -
> -        last_cluster = cluster - 1;
> -    }
> -
> -    /* Play some final catchup with the progress meter */
> -    end = DIV_ROUND_UP(job->common.len, job->cluster_size);
> -    if (last_cluster + 1 < end) {
> -        job->common.offset += ((end - last_cluster - 1) * job->cluster_size);
>      }
>  
>  out:
> @@ -456,6 +441,9 @@ static void 
> backup_incremental_init_copy_bitmap(BackupBlockJob *job)
>          bdrv_set_dirty_iter(dbi, next_cluster * job->cluster_size);
>      }
>  
> +    job->common.offset = job->common.len -
> +                         hbitmap_count(job->copy_bitmap) * job->cluster_size;
> +
>      bdrv_dirty_iter_free(dbi);
>  }
>  
> 

Anything that makes less lines of code is nice, though, so if we cannot
reduce the length of the job:

Reviewed-by: John Snow <address@hidden>