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

[Stefan Hajnoczi]

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 <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.

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