From: John Snow <jsnow@redhat.com>
To: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>,
qemu-block@nongnu.org
Cc: fam@euphon.net, kwolf@redhat.com, den@openvz.org,
qemu-devel@nongnu.org, mreitz@redhat.com
Subject: Re: [Qemu-devel] [PATCH] util/hbitmap: fix unaligned reset
Date: Fri, 2 Aug 2019 15:21:57 -0400 [thread overview]
Message-ID: <c784af61-663d-a633-657e-9f7541c9e97b@redhat.com> (raw)
In-Reply-To: <20190802185830.74648-1-vsementsov@virtuozzo.com>
On 8/2/19 2:58 PM, Vladimir Sementsov-Ogievskiy wrote:
> hbitmap_reset is broken: it rounds up the requested region. It leads to
> the following bug, which is shown by fixed test:
>
> assume granularity = 2
> set(0, 3) # count becomes 4
> reset(0, 1) # count becomes 2
>
> But user of the interface assume that virtual bit 1 should be still
> dirty, so hbitmap should report count to be 4!
>
> In other words, because of granularity, when we set one "virtual" bit,
> yes, we make all "virtual" bits in same chunk to be dirty. But this
> should not be so for reset.
>
> Fix this, aligning bound correctly.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> Hi all!
>
> Hmm, is it a bug or feature? :)
Very, very good question.
> I don't have a test for mirror yet, but I think that sync mirror may be broken
> because of this, as do_sync_target_write() seems to be using unaligned reset.
>
Honestly I was worried about this -- if you take a look at my patches
where I add new bitmap sync modes, I bent over backwards to align
requests for the sync=top bitmap initialization methods because I was
worried about this possibly being the case.
I'm not sure what the "right" behavior ought to be.
Let's say you have a granularity of 8 bytes:
if you reset 0-3 in one call, and then 4-7 in the next, what happens? If
the caller naively thinks there's a 1:1 relationship, it might actually
expect that to reflect a cleared bit. With alignment protection, we'll
just fail to clear it both times and it remains set.
On the other hand, if you do allow partial clears, the first reset for
0-3 will toggle off 4-7 too, where we might rely on the fact that it's
actually still dirty.
Whether or not that's dangerous depends on the context, and only the
caller knows the context. I think we need to make the semantic effect of
the reset "obvious" to the caller.
I envision this:
- hbitmap_reset(bitmap, start, length)
returns -EINVAL if the range is not properly aligned
- hbitmap_reset_flags(bitmap, flags, start, length)
if (flags & HBITMAP_ALIGN_DOWN) align request to only full bits
if (flags & HBITMAP_ALIGN_UP) align request to cover any bit even
partially touched by the specified range
otherwise, pass range through as-is to hbitmap_reset (and possibly get
-EINVAL if caller did not align the request.)
That way the semantics are always clear to the caller.
--js
> tests/test-hbitmap.c | 2 +-
> util/hbitmap.c | 24 +++++++++++++++++++-----
> 2 files changed, 20 insertions(+), 6 deletions(-)
>
> diff --git a/tests/test-hbitmap.c b/tests/test-hbitmap.c
> index 592d8219db..0008025a9f 100644
> --- a/tests/test-hbitmap.c
> +++ b/tests/test-hbitmap.c
> @@ -424,7 +424,7 @@ static void test_hbitmap_granularity(TestHBitmapData *data,
> hbitmap_test_set(data, 0, 3);
> g_assert_cmpint(hbitmap_count(data->hb), ==, 4);
> hbitmap_test_reset(data, 0, 1);
> - g_assert_cmpint(hbitmap_count(data->hb), ==, 2);
> + g_assert_cmpint(hbitmap_count(data->hb), ==, 4);
> }
>
> static void test_hbitmap_iter_granularity(TestHBitmapData *data,
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 7905212a8b..61a813994a 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -473,15 +473,29 @@ void hbitmap_reset(HBitmap *hb, uint64_t start, uint64_t count)
> {
> /* Compute range in the last layer. */
> uint64_t first;
> - uint64_t last = start + count - 1;
> + uint64_t last;
> + uint64_t end = start + count;
> + uint64_t gran = UINT64_C(1) << hb->granularity;
>
> - trace_hbitmap_reset(hb, start, count,
> - start >> hb->granularity, last >> hb->granularity);
> + /*
> + * We should clear only bits, fully covered by requested region. Otherwise
> + * we may clear something that is actually still dirty.
> + */
> + first = DIV_ROUND_UP(start, gran);
>
> - first = start >> hb->granularity;
> - last >>= hb->granularity;
> + if (end == hb->orig_size) {
> + end = DIV_ROUND_UP(end, gran);
> + } else {
> + end = end >> hb->granularity;
> + }
> + if (end <= first) {
> + return;
> + }
> + last = end - 1;
> assert(last < hb->size);
>
> + trace_hbitmap_reset(hb, start, count, first, last);
> +
> hb->count -= hb_count_between(hb, first, last);
> if (hb_reset_between(hb, HBITMAP_LEVELS - 1, first, last) &&
> hb->meta) {
>
next prev parent reply other threads:[~2019-08-02 19:24 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-02 18:58 [Qemu-devel] [PATCH] util/hbitmap: fix unaligned reset Vladimir Sementsov-Ogievskiy
2019-08-02 19:21 ` John Snow [this message]
2019-08-05 9:26 ` Vladimir Sementsov-Ogievskiy
2019-08-05 9:48 ` Vladimir Sementsov-Ogievskiy
2019-08-05 20:03 ` John Snow
2019-08-02 21:19 ` Max Reitz
2019-08-05 9:45 ` Vladimir Sementsov-Ogievskiy
2019-08-05 11:26 ` Max Reitz
2019-08-05 11:43 ` Max Reitz
2019-08-05 9:56 ` Kevin Wolf
2019-08-05 11:30 ` Max Reitz
2019-08-05 23:31 ` Paolo Bonzini
2019-08-06 12:39 ` Vladimir Sementsov-Ogievskiy
2019-08-06 13:30 ` John Snow
2019-08-06 13:47 ` Vladimir Sementsov-Ogievskiy
2019-08-05 11:32 ` Max Reitz
2019-08-05 11:37 ` Vladimir Sementsov-Ogievskiy
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=c784af61-663d-a633-657e-9f7541c9e97b@redhat.com \
--to=jsnow@redhat.com \
--cc=den@openvz.org \
--cc=fam@euphon.net \
--cc=kwolf@redhat.com \
--cc=mreitz@redhat.com \
--cc=qemu-block@nongnu.org \
--cc=qemu-devel@nongnu.org \
--cc=vsementsov@virtuozzo.com \
/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).