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

[John Snow]

On 07/15/2016 08:04 AM, 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 <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 :-))
> 

https://media.giphy.com/media/waG6HzLWKIkhi/giphy.gif

> 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.
> 

Aha, I understand exactly now, thanks.

> 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
> 

I'll work on a followup patch to improve it.

>>
>>> 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;
>>>> +}
>>>
>>
> 
>