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 <[email protected]>
---
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
