[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] bitmaps branch conflict resolution
From: |
Max Reitz |
Subject: |
Re: [Qemu-devel] bitmaps branch conflict resolution |
Date: |
Sat, 17 Aug 2019 00:20:31 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.8.0 |
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 <address@hidden>
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) {
>
signature.asc
Description: OpenPGP digital signature
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [Qemu-devel] bitmaps branch conflict resolution,
Max Reitz <=