Re: [Qemu-devel] [PATCH 3/5] backup: init copy_bitmap from sync_bitmap for incremental

[John Snow <jsnow@redhat.com>]

On 10/02/2017 10:39 AM, Vladimir Sementsov-Ogievskiy wrote:
> We should not copy non-dirty clusters in write notifiers. So,
> initialize copy_bitmap from sync_bitmap.
> 

...! Duh, good find!!

So, previously we'd copy extra sectors if they just so happened to be
written to during the backup process. These would be totally redundant
compared to the previous backups in the chain, by definition.

Now, we allow the write to go through to the source image and let it
dirty the bdrv dirty bitmap we have on standby.

Good fix.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/backup.c | 45 ++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 44 insertions(+), 1 deletion(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 08efec639f..07f4ae846b 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -422,6 +422,43 @@ out:
>      return ret;
>  }
>  
> +/* init copy_bitmap from sync_bitmap */
> +static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
> +{
> +    BdrvDirtyBitmapIter *dbi;
> +    int64_t offset;
> +    int64_t end = DIV_ROUND_UP(bdrv_dirty_bitmap_size(job->sync_bitmap),
> +                               job->cluster_size);
> +
> +    dbi = bdrv_dirty_iter_new(job->sync_bitmap);
> +    while ((offset = bdrv_dirty_iter_next(dbi)) != -1) {
> +        int64_t cluster = offset / job->cluster_size;
> +        int64_t next_cluster;
> +
> +        offset += bdrv_dirty_bitmap_granularity(job->sync_bitmap);
> +        if (offset >= bdrv_dirty_bitmap_size(job->sync_bitmap)) {
> +            hbitmap_set(job->copy_bitmap, cluster, end - cluster);
> +            break;
> +        }
> +
> +        offset = bdrv_dirty_bitmap_next_zero(job->sync_bitmap, offset);
> +        if (offset == -1) {
> +            hbitmap_set(job->copy_bitmap, cluster, end - cluster);
> +            break;
> +        }
> +
> +        next_cluster = DIV_ROUND_UP(offset, job->cluster_size);
> +        hbitmap_set(job->copy_bitmap, cluster, next_cluster - cluster);
> +        if (next_cluster >= end) {
> +            break;
> +        }
> +

Did you look into initializing this from a lower layer, e.g. a call to
initialize-an-hbitmap-from-another-hbitmap?

The loop might look a little cleaner that way and we might possibly get
more utility out of it than a custom coded loop here in the backup code.

> +        bdrv_set_dirty_iter(dbi, next_cluster * job->cluster_size);
> +    }
> +
> +    bdrv_dirty_iter_free(dbi);
> +}
> +
>  static void coroutine_fn backup_run(void *opaque)
>  {
>      BackupBlockJob *job = opaque;
> @@ -435,20 +472,26 @@ static void coroutine_fn backup_run(void *opaque)
>  
>      nb_clusters = DIV_ROUND_UP(job->common.len, job->cluster_size);
>      job->copy_bitmap = hbitmap_alloc(nb_clusters, 0);
> -    hbitmap_set(job->copy_bitmap, 0, nb_clusters);

Alright, so the last patch introduced a blunt initialization, and you've
replaced it by more granular initializations.

>  
>      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) {
> +        /* Here we set all bits in job->copy_bitmap to allow copying clusters
> +         * which are going to be changed (none-mode semantics). It doesn't mean
> +         * that we want to copy _all_ clusters.
> +         */

Right, this is more like ALLOWING all sectors to be copied.

Perhaps:

"All bits are set to allow any cluster to be copied. This does not
actually require them to be copied."

> +        hbitmap_set(job->copy_bitmap, 0, nb_clusters);
>          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);
>          ret = backup_run_incremental(job);
>      } else {
> +        hbitmap_set(job->copy_bitmap, 0, nb_clusters);

Maybe you could just factor these things out entirely:

if (job->sync_mode == MIRROR_SYNC_MODE_INCREMENTAL) {
  backup_incremental_init_copy_bitmap(job);
} else {
  hbitmap_set(job->copy_bitmap, 0, nb_clusters);
}

prior to the big switch statement.

>          /* Both FULL and TOP SYNC_MODE's require copying.. */
>          for (offset = 0; offset < job->common.len;
>               offset += job->cluster_size) {
>