Re: [PATCH 17/21] block/block-copy: switch to fully set bitmap by default

[Vladimir Sementsov-Ogievskiy]

18.05.2021 17:22, Max Reitz wrote:
> On 17.05.21 08:44, Vladimir Sementsov-Ogievskiy wrote:
> > block-copy has a bit inconvenient interface around dirty bitmap: user
> > should get pointer to it and than set by hand. We do need a possibility
> > to share the bitmap with backup job.
> >
> > But default of empty bitmap is strange.
>
> I don’t know, I don’t find it strange.  It expects its user to specify what 
> data to copy, so clearly it gives said user a blank slate.
>
> > Switch to full-set bitmap by
> > default. This way we will not care about setting dirty bitmap in
> > copy-before-write filter when publish it so that it can be used in
> > separate of backup job.
>
> That’s a valid reason, though, so:
>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
>
> Still, I find it stranger this way, because I’m more used to “initialization to 
> 0 by default”.

When I started to test this series it didn't work, because bitmap was not 
initialized. I fixed it and call original behavior strange :)

Probably better to keep block-copy as is and set bitmap in cbw_open. Backup 
code should be modified anyway, as it will get the bitmap after initialization 
in cbw_open.

Next step would be support for bitmap in copy-before-write filter anyway, and 
this place will be changed again.. So, I don't know, but it's not important 
thing.

> > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> > ---
> >   block/backup.c     | 16 +++++++---------
> >   block/block-copy.c |  1 +
> >   2 files changed, 8 insertions(+), 9 deletions(-)
> >
> > diff --git a/block/backup.c b/block/backup.c
> > index 90cca1ca30..706c54fea1 100644
> > --- a/block/backup.c
> > +++ b/block/backup.c
> > @@ -233,18 +233,16 @@ static void backup_init_bcs_bitmap(BackupBlockJob *job)
> >       BdrvDirtyBitmap *bcs_bitmap = block_copy_dirty_bitmap(job->bcs);
> >       if (job->sync_mode == MIRROR_SYNC_MODE_BITMAP) {
> > +        bdrv_clear_dirty_bitmap(bcs_bitmap, NULL);
> >           ret = bdrv_dirty_bitmap_merge_internal(bcs_bitmap, job->sync_bitmap,
> >                                                  NULL, true);
> >           assert(ret);
> > -    } else {
> > -        if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
> > -            /*
> > -             * We can't hog the coroutine to initialize this thoroughly.
> > -             * Set a flag and resume work when we are able to yield safely.
> > -             */
> > -            block_copy_set_skip_unallocated(job->bcs, true);
> > -        }
> > -        bdrv_set_dirty_bitmap(bcs_bitmap, 0, job->len);
> > +    } else if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
> > +        /*
> > +         * We can't hog the coroutine to initialize this thoroughly.
> > +         * Set a flag and resume work when we are able to yield safely.
> > +         */
> > +        block_copy_set_skip_unallocated(job->bcs, true);
> >       }
> >       estimate = bdrv_get_dirty_count(bcs_bitmap);
> > diff --git a/block/block-copy.c b/block/block-copy.c
> > index 9020234c6e..4126f7e8cc 100644
> > --- a/block/block-copy.c
> > +++ b/block/block-copy.c
> > @@ -296,6 +296,7 @@ BlockCopyState *block_copy_state_new(BdrvChild *source, 
> > BdrvChild *target,
> >           return NULL;
> >       }
> >       bdrv_disable_dirty_bitmap(copy_bitmap);
> > +    bdrv_set_dirty_bitmap(copy_bitmap, 0, bdrv_dirty_bitmap_size(copy_bitmap));
> >       /*
> >        * Why we always set BDRV_REQ_SERIALISING write flag:


--
Best regards,
Vladimir