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

[Max Reitz]

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

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