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

[Vladimir Sementsov-Ogievskiy]

24.01.2017 12:46, Fam Zheng wrote:
> On Tue, 01/24 12:00, Vladimir Sementsov-Ogievskiy wrote:
> > > >    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.
> Yes, that's how I understand it too, what I dislike is this bit of inconsistency
> with it: "allowed to copy bitmap" here versus "planned to copy" in other modes.
> I don't know how to improve that, but I think at least the specialty of none
> mode could be mentioned in the comment of copy_bitmap.

Ok, comment will be good. I'll add it.


--
Best regards,
Vladimir