Re: [Qemu-block] [PATCH 01/21] backup: move from done_bitmap to copy_bitmap

[Jeff Cody]

On Fri, Dec 23, 2016 at 05:28:44PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Use HBitmap copy_bitmap instead of done_bitmap. This is needed to unify
> backup loop for full/incremental modes in future patches.
> 
> We reset bit of the copy_bitmap immediately after checking it in
> backup_do_cow(). It is safe, because all other intersecting requests
> will wait for our request finish anyway.
> 
> The other difference is that in case of error we will have zeroed bit in
> copy_bitmap, when in done_bitmap we have not set bit.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> ---
>  block/backup.c | 17 ++++++++++-------
>  1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index ea38733..6b27e55 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -39,11 +39,12 @@ typedef struct BackupBlockJob {
>      BlockdevOnError on_target_error;
>      CoRwlock flush_rwlock;
>      uint64_t sectors_read;
> -    unsigned long *done_bitmap;
>      int64_t cluster_size;
>      bool compress;
>      NotifierWithReturn before_write;
>      QLIST_HEAD(, CowRequest) inflight_reqs;
> +
> +    HBitmap *copy_bitmap;
>  } BackupBlockJob;
>  
>  /* Size of a cluster in sectors, instead of bytes. */
> @@ -115,10 +116,11 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
>      cow_request_begin(&cow_request, job, start, end);
>  
>      for (; start < end; start++) {
> -        if (test_bit(start, job->done_bitmap)) {
> +        if (!hbitmap_get(job->copy_bitmap, start)) {
>              trace_backup_do_cow_skip(job, start);
>              continue; /* already copied */
>          }
> +        hbitmap_reset(job->copy_bitmap, start, 1);
>  
>          trace_backup_do_cow_process(job, start);
>  
> @@ -141,6 +143,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob *job,
>              if (error_is_read) {
>                  *error_is_read = true;
>              }
> +            hbitmap_set(job->copy_bitmap, start, 1);
>              goto out;
>          }
>  
> @@ -157,11 +160,10 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
>              if (error_is_read) {
>                  *error_is_read = false;
>              }
> +            hbitmap_set(job->copy_bitmap, start, 1);
>              goto out;
>          }
>  
> -        set_bit(start, job->done_bitmap);
> -
>          /* Publish progress, guest I/O counts as progress too.  Note that the
>           * offset field is an opaque progress value, it is not a disk offset.
>           */
> @@ -271,7 +273,7 @@ void backup_do_checkpoint(BlockJob *job, Error **errp)
>      }
>  
>      len = DIV_ROUND_UP(backup_job->common.len, backup_job->cluster_size);
> -    bitmap_zero(backup_job->done_bitmap, len);
> +    hbitmap_set(backup_job->copy_bitmap, 0, len);
>  }
>  
>  void backup_wait_for_overlapping_requests(BlockJob *job, int64_t sector_num,
> @@ -450,7 +452,8 @@ static void coroutine_fn backup_run(void *opaque)
>      start = 0;
>      end = DIV_ROUND_UP(job->common.len, job->cluster_size);
>  
> -    job->done_bitmap = bitmap_new(end);
> +    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);
> @@ -524,7 +527,7 @@ static void coroutine_fn backup_run(void *opaque)
>      /* wait until pending backup_do_cow() calls have completed */
>      qemu_co_rwlock_wrlock(&job->flush_rwlock);
>      qemu_co_rwlock_unlock(&job->flush_rwlock);
> -    g_free(job->done_bitmap);
> +    hbitmap_free(job->copy_bitmap);
>  
>      data = g_malloc(sizeof(*data));
>      data->ret = ret;
> -- 
> 1.8.3.1
>

Reviewed-by: Jeff Cody <address@hidden>