On Tue, Jan 24, 2017 at 12:12:47PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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 <vsement...@virtuozzo.com>
> > > ---
> > > 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.If you want to change the semantics of the progress indicator, please send it as a separate series. I'm not convinced by this patch but it shouldn't block the rest of the series from being merged. Stefan
signature.asc
Description: PGP signature
