From: Max Reitz <mreitz@redhat.com>
To: John Snow <jsnow@redhat.com>
Cc: "qemu-devel@nongnu.org" <qemu-devel@nongnu.org>,
Qemu-block <qemu-block@nongnu.org>
Subject: Re: [Qemu-devel] bitmaps branch conflict resolution
Date: Sat, 17 Aug 2019 00:20:31 +0200 [thread overview]
Message-ID: <52b4cd48-93f2-c001-fa56-2fbd8485ee1d@redhat.com> (raw)
In-Reply-To: <c8b0ab36-79c8-0b4b-3193-4e12ed8c848b@redhat.com>
[-- Attachment #1.1: Type: text/plain, Size: 3563 bytes --]
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) {
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
parent reply other threads:[~2019-08-16 22:21 UTC|newest]
Thread overview: expand[flat|nested] mbox.gz Atom feed
[parent not found: <c8b0ab36-79c8-0b4b-3193-4e12ed8c848b@redhat.com>]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=52b4cd48-93f2-c001-fa56-2fbd8485ee1d@redhat.com \
--to=mreitz@redhat.com \
--cc=jsnow@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).