Re: [Qemu-block] [PATCH v6 02/10] HBitmap: Introduce "meta" bitmap to track bit changes

[Max Reitz]

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