[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
- [Qemu-block] [PATCH 28/29] qcow2-bitmap: do not try reloading bitmaps, (continued)
- [Qemu-block] [PATCH 28/29] qcow2-bitmap: do not try reloading bitmaps, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 07/29] qcow2-bitmap: add qcow2_bitmap_load(), Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 26/29] iotests: test qcow2 persistent dirty bitmap, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 21/29] qcow2-bitmap: add EXTRA_DATA_COMPATIBLE flag, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 13/29] qcow2: add dirty bitmaps extension, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 27/29] qcow2-bitmap: delete in_use bitmaps on image load, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 22/29] qmp: add persistent flag to block-dirty-bitmap-add, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 24/29] qcow2-bitmap: maintian BlockDirtyBitmap.autoload, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 01/29] hbitmap: fix dirty iter, Vladimir Sementsov-Ogievskiy, 2016/08/08
- Re: [Qemu-block] [PATCH 01/29] hbitmap: fix dirty iter,
Kevin Wolf <=
- [Qemu-block] [PATCH 14/29] qcow2-bitmap: add qcow2_bitmap_load_check(), Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 15/29] block/dirty-bitmap: introduce persistent bitmaps, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 11/29] qcow2-bitmap: check constraints, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 12/29] qcow2: add qcow2_delete_bitmaps, Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 06/29] qcow2-bitmap: add qcow2_read_bitmaps(), Vladimir Sementsov-Ogievskiy, 2016/08/08
- [Qemu-block] [PATCH 17/29] qcow2-bitmap: add autoclear bit, Vladimir Sementsov-Ogievskiy, 2016/08/08