Re: [Qemu-block] [Qemu-devel] [PATCH 03/21] backup: improve non-dirty bits progress processing

[Vladimir Sementsov-Ogievskiy]

24.01.2017 10:17, Fam Zheng wrote:

On Fri, 12/23 17:28, Vladimir Sementsov-Ogievskiy wrote:

Set fake progress for non-dirty clusters in copy_bitmap initialization,
to:
1. set progress in the same place where corresponding clusters are
consumed from copy_bitmap (or not initialized, as here)
2. earlier progress information for user
3. simplify the code

Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
 block/backup.c | 18 +++---------------
 1 file changed, 3 insertions(+), 15 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 621b1c0..f1f87f6 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -383,7 +383,6 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
     int64_t sector;
     int64_t cluster;
     int64_t end;
-    int64_t last_cluster = -1;
     int64_t sectors_per_cluster = cluster_size_sectors(job);
     BdrvDirtyBitmapIter *dbi;
 
@@ -395,12 +394,6 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
     while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
         cluster = sector / sectors_per_cluster;
 
-        /* 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)) {
@@ -422,14 +415,6 @@ static int coroutine_fn backup_run_incremental(BackupBlockJob *job)
         if (granularity < job->cluster_size) {
             bdrv_set_dirty_iter(dbi, cluster * sectors_per_cluster);
         }
-
-        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:
@@ -462,6 +447,9 @@ static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
         bdrv_set_dirty_iter(dbi, (cluster + cl_gran) * sectors_per_cluster);
     }
 
+    job->common.offset = job->common.len -
+                         hbitmap_count(job->copy_bitmap) * job->cluster_size;
+
     bdrv_dirty_iter_free(dbi);
 }

Is this effectively moving the progress reporting from cluster copying to dirty
bitmap initialization? If so the progress will stay "100%" once
backup_incremental_init_copy_bitmap returns, but the backup has merely started.
I don't think this is a good idea.

Fam

Currently progress tracking for incremental backup is bad too. Holes are bad for progress in any way. To make good progress we should calculate it like (cluters _physically_ copied) / (full amount of clusters to be _physically_ copied). And with current qapi scheme it is not possible. This formula may be approximated by (offset - skipped_clusters) / (len - skipped_clusters), where offset and len are old good len, and skipped_clusters should be added to query_block_jobs. And with such approximation it is obvious that it will be more presize if we skip all clusters that should be skipped earlier. The best way of course is to skip them in initialization. It is not possible (if I understand things right) for full mode, as it skips clusters using get_block_status which may be long operation, so we should start notifier handling before skipping operation is finished...

Any way, it is work of management tool to show beautiful progress, qemu only provides information for it, and with current scheme, the only way to provide information about cluster skipping in copying progress is by offset field. And it is not bad to provide this information earlier. And again, to produce really beautiful progress we at least need one more field - skipped_clusters.

-- 
Best regards,
Vladimir