[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [PATCH v6 02/10] HBitmap: Introduce "meta" bitmap to tr
From: |
Max Reitz |
Subject: |
Re: [Qemu-block] [PATCH v6 02/10] HBitmap: Introduce "meta" bitmap to track bit changes |
Date: |
Mon, 18 Jul 2016 16:54:09 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 |
On 15.07.2016 22:27, John Snow wrote:
> From: Fam Zheng <address@hidden>
>
> Upon each bit toggle, the corresponding bit in the meta bitmap will be
> set.
>
> Signed-off-by: Fam Zheng <address@hidden>
> [Amended text inline. --js]
>
> Signed-off-by: John Snow <address@hidden>
> ---
> include/qemu/hbitmap.h | 21 +++++++++++++++
> util/hbitmap.c | 70
> +++++++++++++++++++++++++++++++++++++++-----------
> 2 files changed, 76 insertions(+), 15 deletions(-)
[...]
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 99fd2ba..5186500 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
[...]
> @@ -256,23 +261,29 @@ static void hb_set_between(HBitmap *hb, int level,
> uint64_t start, uint64_t last
> if (level > 0 && changed) {
> hb_set_between(hb, level - 1, pos, lastpos);
> }
> + return changed;
> }
>
> void hbitmap_set(HBitmap *hb, uint64_t start, uint64_t count)
> {
> /* Compute range in the last layer. */
> + uint64_t first, n;
> uint64_t last = start + count - 1;
>
> trace_hbitmap_set(hb, start, count,
> start >> hb->granularity, last >> hb->granularity);
>
> - start >>= hb->granularity;
> + first = start >> hb->granularity;
> last >>= hb->granularity;
> + assert(last < hb->size);
> count = last - start + 1;
> - assert(last < hb->size);
This looks like a wrong rebase conflict resolution to me. The previous
version did not reorder the assertion and the "count = ..." line, but
just removed the latter.
> + n = last - first + 1;
In this version, we now have count == n. Having two distinct variables
for the same value looks suspicious.
I think we want to have n to be the number of actual bits in the bitmap
(i.e. @count reduced by the granularity), but we still need to retain
the original parameter value because of...
>
> - hb->count += count - hb_count_between(hb, start, last);
> - hb_set_between(hb, HBITMAP_LEVELS - 1, start, last);
> + hb->count += n - hb_count_between(hb, first, last);
> + if (hb_set_between(hb, HBITMAP_LEVELS - 1, first, last) &&
> + hb->meta) {
> + hbitmap_set(hb->meta, start, count);
...this.
With the "count = last - start + 1;" line removed:
Reviewed-by: Max Reitz <address@hidden>
> + }
> }
>
> /* Resetting works the other way round: propagate up if the new
signature.asc
Description: OpenPGP digital signature
- [Qemu-block] [PATCH v6 00/10] Dirty bitmap changes for migration/persistence work, John Snow, 2016/07/15
- [Qemu-block] [PATCH v6 03/10] tests: Add test code for meta bitmap, John Snow, 2016/07/15
- [Qemu-block] [PATCH v6 01/10] block: Hide HBitmap in block dirty bitmap interface, John Snow, 2016/07/15
- [Qemu-block] [PATCH v6 02/10] HBitmap: Introduce "meta" bitmap to track bit changes, John Snow, 2016/07/15
- Re: [Qemu-block] [PATCH v6 02/10] HBitmap: Introduce "meta" bitmap to track bit changes,
Max Reitz <=
- [Qemu-block] [PATCH v6 04/10] block: Support meta dirty bitmap, John Snow, 2016/07/15
- [Qemu-block] [PATCH v6 05/10] block: Add two dirty bitmap getters, John Snow, 2016/07/15
- [Qemu-block] [PATCH v6 06/10] block: Assert that bdrv_release_dirty_bitmap succeeded, John Snow, 2016/07/15
- [Qemu-block] [PATCH v6 08/10] block: BdrvDirtyBitmap serialization interface, John Snow, 2016/07/15
- [Qemu-block] [PATCH v6 07/10] hbitmap: serialization, John Snow, 2016/07/15
- [Qemu-block] [PATCH v6 10/10] block: More operations for meta dirty bitmap, John Snow, 2016/07/15
- [Qemu-block] [PATCH v6 09/10] tests: Add test code for hbitmap serialization, John Snow, 2016/07/15