On 15.07.2016 14:04, Max Reitz wrote: > On 14.07.2016 22:00, John Snow wrote: >> On 06/22/2016 11:53 AM, Max Reitz wrote: >>> On 03.06.2016 06:32, Fam Zheng wrote: >>>> The added group of operations enables tracking of the changed bits in >>>> the dirty bitmap. >>>> >>>> Signed-off-by: Fam Zheng <[email protected]> >>>> --- >>>> block/dirty-bitmap.c | 52 >>>> ++++++++++++++++++++++++++++++++++++++++++++ >>>> include/block/dirty-bitmap.h | 9 ++++++++ >>>> 2 files changed, 61 insertions(+) >>>> >>>> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >>>> index 628b77c..9c53c56 100644 >>>> --- a/block/dirty-bitmap.c >>>> +++ b/block/dirty-bitmap.c >>>> @@ -38,6 +38,7 @@ >>>> */ >>>> struct BdrvDirtyBitmap { >>>> HBitmap *bitmap; /* Dirty sector bitmap implementation */ >>>> + HBitmap *meta; /* Meta dirty bitmap */ >>>> BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status >>>> */ >>>> char *name; /* Optional non-empty unique ID */ >>>> int64_t size; /* Size of the bitmap (Number of sectors) >>>> */ >>>> @@ -103,6 +104,56 @@ BdrvDirtyBitmap >>>> *bdrv_create_dirty_bitmap(BlockDriverState *bs, >>>> return bitmap; >>>> } >>>> >>>> +/* bdrv_create_meta_dirty_bitmap >>>> + * >>>> + * Create a meta dirty bitmap that tracks the changes of bits in @bitmap. >>>> I.e. >>>> + * when a dirty status bit in @bitmap is changed (either from reset to >>>> set or >>>> + * the other way around), its respective meta dirty bitmap bit will be >>>> marked >>>> + * dirty as well. >>> >>> Not wrong, but I'd like a note here that this is not an >>> when-and-only-when relationship, i.e. that bits in the meta bitmap may >>> be set even without the tracked bits in the dirty bitmap having changed. >>> >> >> How? >> >> You mean, if the caller manually starts setting things in the meta >> bitmap object? >> >> That's their fault then, isn't it? > > No, I mean something that I mentioned in a reply to some previous > version (the patch adding the test): > > http://lists.nongnu.org/archive/html/qemu-block/2016-03/msg00332.html > > Fam's reply is here: > > http://lists.nongnu.org/archive/html/qemu-block/2016-06/msg00097.html > > (Interesting how that reply took nearly three months and yours took > nearly one, there most be something about this series that makes > replying to replies very cumbersome :-))
I just remembered that it's very much justified now, as you have only
recently adopted this series.
It's just always funny to get a “What are you talking about?” reply to
some nagging I sent out long enough in the past that I can't even
remember myself, so I have to look it up, too.
Sorry :-)
Max
> What I meant by “then it would update meta” is that it would update *all
> of the range* even though only a single bit has actually been changed.
>
> So the answer to your “how” is: See patch 2, the changes to
> hbitmap_set() (and hbitmap_reset()). If any of the bits in the given
> range is changed, all of the range is marked as having changed in the
> meta bitmap.
>
> So all we guarantee is that every time a bit is changed, the
> corresponding bit in the meta bitmap will be set. But we do not
> guarantee that a bit in the meta bitmap stays cleared as long as the
> corresponding range of the underlying bitmap stays the same.
>
> Max
>
>>
>>> Maybe this should be mentioned somewhere in patch 2, too. Or maybe only
>>> in patch 2.
>>>
>>>> + *
>>>> + * @bitmap: the block dirty bitmap for which to create a meta dirty
>>>> bitmap.
>>>> + * @chunk_size: how many bytes of bitmap data does each bit in the meta
>>>> bitmap
>>>> + * track.
>>>> + */
>>>> +void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
>>>> + int chunk_size)
>>>> +{
>>>> + assert(!bitmap->meta);
>>>> + bitmap->meta = hbitmap_create_meta(bitmap->bitmap,
>>>> + chunk_size * BITS_PER_BYTE);
>>>> +}
>>>> +
>>>> +void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>>>> +{
>>>> + assert(bitmap->meta);
>>>> + hbitmap_free_meta(bitmap->bitmap);
>>>> + bitmap->meta = NULL;
>>>> +}
>>>> +
>>>> +int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
>>>> + BdrvDirtyBitmap *bitmap, int64_t sector,
>>>> + int nb_sectors)
>>>> +{
>>>> + uint64_t i;
>>>> + int gran = bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
>>>> +
>>>> + /* To optimize: we can make hbitmap to internally check the range in a
>>>> + * coarse level, or at least do it word by word. */
>>>
>>> We could also multiply gran by the granularity of the meta bitmap. Each
>>> bit of the meta bitmap tracks at least eight bits of the dirty bitmap,
>>> so we're calling hbitmap_get() at least eight times as often as
>>> necessary here.
>>>
>>> Or we just use int gran = hbitmap_granularity(bitmap->meta);.
>>>
>>> Not exactly wrong, though, so:
>>>
>>> Reviewed-by: Max Reitz <[email protected]>
>>>
>>>> + for (i = sector; i < sector + nb_sectors; i += gran) {
>>>> + if (hbitmap_get(bitmap->meta, i)) {
>>>> + return true;
>>>> + }
>>>> + }
>>>> + return false;
>>>> +}
>>>
>>
>
>
signature.asc
Description: OpenPGP digital signature
