Re: [Qemu-block] [PATCH 02/21] backup: init copy_bitmap from sync_bitmap for incremental

[Vladimir Sementsov-Ogievskiy]

24.01.2017 10:09, Fam Zheng wrote:
> On Fri, 12/23 17:28, Vladimir Sementsov-Ogievskiy wrote:
> > We should not copy non-dirty clusters in write notifiers. So,
> > initialize copy_bitmap from sync_bitmap.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   block/backup.c | 32 +++++++++++++++++++++++++++++++-
> >   1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/block/backup.c b/block/backup.c
> > index 6b27e55..621b1c0 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -437,6 +437,34 @@ out:
> >       return ret;
> >   }
> >   
> > +/* init copy_bitmap from sync_bitmap */
> > +static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
> > +{
> > +    int64_t sector;
> > +    BdrvDirtyBitmapIter *dbi;
> > +    uint32_t sect_gran =
> > +        bdrv_dirty_bitmap_granularity(job->sync_bitmap) >> BDRV_SECTOR_BITS;
> > +    int64_t sz = bdrv_dirty_bitmap_size(job->sync_bitmap);
> > +    int64_t sectors_per_cluster = cluster_size_sectors(job);
> > +    uint32_t cl_gran = MAX(1, sect_gran / sectors_per_cluster);
> > +
> > +    dbi = bdrv_dirty_iter_new(job->sync_bitmap, 0);
> > +    while ((sector = bdrv_dirty_iter_next(dbi)) != -1) {
> > +        int64_t cluster = sector / sectors_per_cluster;
> > +        int64_t next_sector = (cluster + cl_gran) * sectors_per_cluster;
> > +
> > +        hbitmap_set(job->copy_bitmap, cluster, cl_gran);
> > +
> > +        if (next_sector >= sz) {
> > +            break;
> > +        }
> > +
> > +        bdrv_set_dirty_iter(dbi, (cluster + cl_gran) * sectors_per_cluster);
> > +    }
> > +
> > +    bdrv_dirty_iter_free(dbi);
> > +}
> > +
> >   static void coroutine_fn backup_run(void *opaque)
> >   {
> >       BackupBlockJob *job = opaque;
> > @@ -453,20 +481,22 @@ static void coroutine_fn backup_run(void *opaque)
> >       end = DIV_ROUND_UP(job->common.len, job->cluster_size);
> >   
> >       job->copy_bitmap = hbitmap_alloc(end, 0);
> > -    hbitmap_set(job->copy_bitmap, 0, end);
> >   
> >       job->before_write.notify = backup_before_write_notify;
> >       bdrv_add_before_write_notifier(bs, &job->before_write);
> >   
> >       if (job->sync_mode == MIRROR_SYNC_MODE_NONE) {
> > +        hbitmap_set(job->copy_bitmap, 0, end);
> This is confusing. It seems job->copy_bitmap is actually a superset of clusters
> to copy instead of the exact one?  Because "none" mode doesn't need blanket copy
> - only overwritten clusters are copied...

We can't guess, what clusters should be copied finally in none mode. 
None mode is done by allowing only notifier handling and no linear 
copying. But notifier handling will copy only clusters marked in 
copy_bitmap, so I set it from 0 to end.

> >           while (!block_job_is_cancelled(&job->common)) {
> >               /* Yield until the job is cancelled.  We just let our before_write
> >                * notify callback service CoW requests. */
> >               block_job_yield(&job->common);
> >           }
> >       } else if (job->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
> > +        backup_incremental_init_copy_bitmap(job);
> ... But here you are only marking bits in sync_bitmap.
>
> Fam
>
> >           ret = backup_run_incremental(job);
> >       } else {
> > +        hbitmap_set(job->copy_bitmap, 0, end);
> >           /* Both FULL and TOP SYNC_MODE's require copying.. */
> >           for (; start < end; start++) {
> >               bool error_is_read;
> > --
> > 1.8.3.1


--
Best regards,
Vladimir