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

[Vladimir Sementsov-Ogievskiy]

23.12.2016 17:28, 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.

This sentence is unrelated. It actually relates to old my mistake (not 
published versions), which is fixed here. Really, in case of error I set 
bit in copy_bitmap back, to correspond to old behavior with done_bitmap.

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


--
Best regards,
Vladimir