On 17.08.19 00:07, John Snow wrote:
> Hi Max, I took your patch and adjusted it slightly: I don't like
> "skip_bytes" anymore because it's clear now that we don't only read that
> value when we're skipping bytes, so now it's just status_bytes.

Yep, sure.

> Since this is based on your fixup, would you like to offer an
> Ack/S-o-b/R-B/whichever here?

Sure:

Reviewed-by: Max Reitz <mreitz@redhat.com>

Additional explanation for others:

The conflict resolution in itself is just a matter of the
“backup_bitmap_reset_unallocated” block and the
“bdrv_dirty_bitmap_next_zero” block introduced in the same place in two
separate patches (one went to master, the other to bitmaps-next).

So the question is how to order them.  On the first glance, it doesn’t
matter, it can go both ways.

On a second glance, it turns out we need to combine the results, hence
the new MIN() here.

If we are initializing the bitmap, bdrv_dirty_bitmap_next_zero() does
not necessarily return the correct result.  It is only accurate insofar
we have actually initialized the bitmap.  We can get that information
from backup_bitmap_reset_unallocated(): It ensures that the bitmap is
accurate in the [start, start + status_bytes) range.

Therefore, we have to limit dirty_end by start + status_bytes.

I don’t think it really matters whether we do the
backup_bitmap_reset_allocated() or the bdrv_dirty_bitmap_next_zero()
first.  It’s just that it’s slightly simpler to do the latter first,
because the former is in a conditional block, so we can put the MIN()
right there.  Hence the order change here.

(If we did it the other way around, we’d need another conditional block
“if (job->initializing_bitmap) { dirty_end = MIN(...) }” after we have
both dirty_end and status_bytes.)

Max

> diff --git a/block/backup.c b/block/backup.c
> index ee4d5598986..9e1382ec5c6 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -266,7 +266,7 @@ static int coroutine_fn backup_do_cow(BackupBlockJob
> *job,
>      int ret = 0;
>      int64_t start, end; /* bytes */
>      void *bounce_buffer = NULL;
> -    int64_t skip_bytes;
> +    int64_t status_bytes;
> 
>      qemu_co_rwlock_rdlock(&job->flush_rwlock);
> 
> @@ -287,21 +287,23 @@ static int coroutine_fn
> backup_do_cow(BackupBlockJob *job,
>              continue; /* already copied */
>          }
> 
> -        if (job->initializing_bitmap) {
> -            ret = backup_bitmap_reset_unallocated(job, start, &skip_bytes);
> -            if (ret == 0) {
> -                trace_backup_do_cow_skip_range(job, start, skip_bytes);
> -                start += skip_bytes;
> -                continue;
> -            }
> -        }
> -
>          dirty_end = bdrv_dirty_bitmap_next_zero(job->copy_bitmap, start,
>                                                  (end - start));
>          if (dirty_end < 0) {
>              dirty_end = end;
>          }
> 
> +        if (job->initializing_bitmap) {
> +            ret = backup_bitmap_reset_unallocated(job, start,
> &status_bytes);
> +            if (ret == 0) {
> +                trace_backup_do_cow_skip_range(job, start, status_bytes);
> +                start += status_bytes;
> +                continue;
> +            }
> +            /* Clamp to known allocated region */
> +            dirty_end = MIN(dirty_end, start + status_bytes);
> +        }
> +
>          trace_backup_do_cow_process(job, start);
> 
>          if (job->use_copy_range) {
>