Re: [Qemu-devel] [PATCH v2 11/18] block/backup: upgrade copy_bitmap to BdrvDirtyBitmap

[Max Reitz]

On 03.07.19 23:55, John Snow wrote:
> This simplifies some interface matters; namely the initialization and
> (later) merging the manifest back into the sync_bitmap if it was
> provided.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>  block/backup.c | 76 ++++++++++++++++++++++++--------------------------
>  1 file changed, 37 insertions(+), 39 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index d7fdafebda..9cc5a7f6ca 100644
> --- a/block/backup.c
> +++ b/block/backup.c

[...]

> @@ -202,7 +204,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
>      cow_request_begin(&cow_request, job, start, end);
>  
>      while (start < end) {
> -        if (!hbitmap_get(job->copy_bitmap, start)) {
> +        if (!bdrv_dirty_bitmap_get(job->copy_bitmap, start)) {
>              trace_backup_do_cow_skip(job, start);
>              start += job->cluster_size;
>              continue; /* already copied */
> @@ -296,14 +298,16 @@ static void backup_abort(Job *job)
>  static void backup_clean(Job *job)
>  {
>      BackupBlockJob *s = container_of(job, BackupBlockJob, common.job);
> +    BlockDriverState *bs = blk_bs(s->target);

I’d prefer to call it target_bs, because “bs” is usually the source node.

Which brings me to a question: Why is the copy bitmap assigned to the
target in the first place?  Wouldn’t it make more sense on the source?

> +
> +    if (s->copy_bitmap) {
> +        bdrv_release_dirty_bitmap(bs, s->copy_bitmap);
> +        s->copy_bitmap = NULL;
> +    }
> +
>      assert(s->target);
>      blk_unref(s->target);
>      s->target = NULL;
> -
> -    if (s->copy_bitmap) {
> -        hbitmap_free(s->copy_bitmap);
> -        s->copy_bitmap = NULL;
> -    }
>  }
>  
>  void backup_do_checkpoint(BlockJob *job, Error **errp)

[...]

> @@ -387,59 +391,49 @@ static bool bdrv_is_unallocated_range(BlockDriverState 
> *bs,
>  
>  static int coroutine_fn backup_loop(BackupBlockJob *job)
>  {
> -    int ret;
>      bool error_is_read;
>      int64_t offset;
> -    HBitmapIter hbi;
> +    BdrvDirtyBitmapIter *bdbi;
>      BlockDriverState *bs = blk_bs(job->common.blk);
> +    int ret = 0;
>  
> -    hbitmap_iter_init(&hbi, job->copy_bitmap, 0);
> -    while ((offset = hbitmap_iter_next(&hbi)) != -1) {
> +    bdbi = bdrv_dirty_iter_new(job->copy_bitmap);
> +    while ((offset = bdrv_dirty_iter_next(bdbi)) != -1) {
>          if (job->sync_mode == MIRROR_SYNC_MODE_TOP &&
>              bdrv_is_unallocated_range(bs, offset, job->cluster_size))
>          {
> -            hbitmap_reset(job->copy_bitmap, offset, job->cluster_size);
> +            bdrv_set_dirty_bitmap(job->copy_bitmap, offset, 
> job->cluster_size);

Should be reset.

>              continue;
>          }
>  
>          do {
>              if (yield_and_check(job)) {
> -                return 0;
> +                goto out;
>              }
>              ret = backup_do_cow(job, offset,
>                                  job->cluster_size, &error_is_read, false);
>              if (ret < 0 && backup_error_action(job, error_is_read, -ret) ==
>                             BLOCK_ERROR_ACTION_REPORT)
>              {
> -                return ret;
> +                goto out;
>              }
>          } while (ret < 0);
>      }
>  
> -    return 0;
> + out:
> +    bdrv_dirty_iter_free(bdbi);
> +    return ret;
>  }
>  
>  /* init copy_bitmap from sync_bitmap */
>  static void backup_incremental_init_copy_bitmap(BackupBlockJob *job)
>  {
> -    uint64_t offset = 0;
> -    uint64_t bytes = job->len;
> -
> -    while (bdrv_dirty_bitmap_next_dirty_area(job->sync_bitmap,
> -                                             &offset, &bytes))
> -    {
> -        hbitmap_set(job->copy_bitmap, offset, bytes);
> -
> -        offset += bytes;
> -        if (offset >= job->len) {
> -            break;
> -        }
> -        bytes = job->len - offset;
> -    }
> +    bdrv_dirty_bitmap_merge_internal(job->copy_bitmap, job->sync_bitmap,
> +                                     NULL, true);

How about asserting that this was successful?

>  
>      /* TODO job_progress_set_remaining() would make more sense */
>      job_progress_update(&job->common.job,
> -        job->len - hbitmap_count(job->copy_bitmap));
> +                        job->len - bdrv_get_dirty_count(job->copy_bitmap));
>  }
>  
>  static int coroutine_fn backup_run(Job *job, Error **errp)

[...]

> @@ -670,7 +668,7 @@ BlockJob *backup_job_create(const char *job_id, 
> BlockDriverState *bs,
>   error:
>      if (copy_bitmap) {
>          assert(!job || !job->copy_bitmap);
> -        hbitmap_free(copy_bitmap);
> +        bdrv_release_dirty_bitmap(bs, copy_bitmap);

If you decide to keep the copy_bitmap on the target, s/bs/target/.

Max

signature.asc
Description: OpenPGP digital signature