Re: [Qemu-block] [PATCH v5 04/10] block: Support meta dirty bitmap

[Max Reitz]

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 <address@hidden>
>>> ---
>>>  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 :-))

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 <address@hidden>
>>
>>> +    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