[01/29] hbitmap: fix dirty iter

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[...]).

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Denis V. Lunev <den@openvz.org>
---
 util/hbitmap.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

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 <vsementsov@virtuozzo.com>
> Signed-off-by: Denis V. Lunev <den@openvz.org>
> ---
>  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

On 10.08.2016 16:41, Kevin Wolf wrote:
> 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.

Something like this, yes.

>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> ---
>>   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.

Aha, cool. We've missed this. So this patch is a new feature but not a 
bug fix. I will update this comment.

>
> Kevin