qemu-block
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-block] [PATCH 01/29] hbitmap: fix dirty iter


From: Kevin Wolf
Subject: Re: [Qemu-block] [PATCH 01/29] hbitmap: fix dirty iter
Date: Wed, 10 Aug 2016 15:41:06 +0200
User-agent: Mutt/1.5.21 (2010-09-15)

Am 08.08.2016 um 17:04 hat Vladimir Sementsov-Ogievskiy geschrieben:
> If dirty bitmap was cleared during iterator life, we can went to zero
> current in hbitmap_iter_skip_words, starting from saved (and currently
> wrong hbi->cur[...]).

I was going to suggest improved grammar, but actually I find this
description hard to understand even with correct grammar.

If I understand correctly, hbi->cur contains a copy of hb->levels that
isn't updated after the iterator entered the subtree, because this way
we avoid going backwards if concurrent operations set a new bit.
However, in the opposite case of clearing a bit, we actually want to use
the new value because we wouldn't find anything to return when going to
the lower levels. This is what the & does in your patch.

Is this explanation reasonably correct? If so, maybe you can try to make
these points in the commit message in the next version.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> Signed-off-by: Denis V. Lunev <address@hidden>
> ---
>  util/hbitmap.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/util/hbitmap.c b/util/hbitmap.c
> index 6a13c12..f807f64 100644
> --- a/util/hbitmap.c
> +++ b/util/hbitmap.c
> @@ -106,8 +106,9 @@ unsigned long hbitmap_iter_skip_words(HBitmapIter *hbi)
>  
>      unsigned long cur;
>      do {
> -        cur = hbi->cur[--i];
> +        i--;
>          pos >>= BITS_PER_LEVEL;
> +        cur = hbi->cur[i] & hb->levels[i][pos];
>      } while (cur == 0);

I think you will want to update the comment for hbitmap_iter_init. This
is what it says today:

 * Concurrent setting of bits is acceptable, and will at worst cause the
 * iteration to miss some of those bits.  Resetting bits before the current
 * position of the iterator is also okay.  However, concurrent resetting of
 * bits can lead to unexpected behavior if the iterator has not yet reached
 * those bits.

Kevin



reply via email to

[Prev in Thread] Current Thread [Next in Thread]