Re: [Qemu-devel] [PATCH for-4.1 1/2] backup: Copy only dirty areas

[Vladimir Sementsov-Ogievskiy]

01.08.2019 18:12, Max Reitz wrote:
> The backup job must only copy areas that the copy_bitmap reports as
> dirty.  This is always the case when using traditional non-offloading
> backup, because it copies each cluster separately.  When offloading the
> copy operation, we sometimes copy more than one cluster at a time, but
> we only check whether the first one is dirty.
> 
> Therefore, whenever copy offloading is possible, the backup job
> currently produces wrong output when the guest writes to an area of
> which an inner part has already been backed up, because that inner part
> will be re-copied.
> 
> Fixes: 9ded4a0114968e98b41494fc035ba14f84cdf700
> Signed-off-by: Max Reitz <address@hidden>
> ---
>   block/backup.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/block/backup.c b/block/backup.c
> index 715e1d3be8..1ee271f9f1 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -202,22 +202,31 @@ static int coroutine_fn backup_do_cow(BackupBlockJob 
> *job,
>       cow_request_begin(&cow_request, job, start, end);
>   
>       while (start < end) {
> +        int64_t dirty_end;
> +
>           if (!hbitmap_get(job->copy_bitmap, start)) {
>               trace_backup_do_cow_skip(job, start);
>               start += job->cluster_size;
>               continue; /* already copied */
>           }
>   
> +        dirty_end = hbitmap_next_zero(job->copy_bitmap, start, (end - 
> start));
> +        if (dirty_end < 0) {
> +            dirty_end = end;
> +        }

1. hbitmap_next_dirty_area is better I think
2. we can do it only in use_copy_range case, as backup_cow_with_bounce_buffer 
copies
    only one cluster anyway

But this all should be refactored anyway, so:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

> +
>           trace_backup_do_cow_process(job, start);
>   
>           if (job->use_copy_range) {
> -            ret = backup_cow_with_offload(job, start, end, 
> is_write_notifier);
> +            ret = backup_cow_with_offload(job, start, dirty_end,
> +                                          is_write_notifier);
>               if (ret < 0) {
>                   job->use_copy_range = false;
>               }
>           }
>           if (!job->use_copy_range) {
> -            ret = backup_cow_with_bounce_buffer(job, start, end, 
> is_write_notifier,
> +            ret = backup_cow_with_bounce_buffer(job, start, dirty_end,
> +                                                is_write_notifier,
>                                                   error_is_read, 
> &bounce_buffer);
>           }
>           if (ret < 0) {
> 


-- 
Best regards,
Vladimir