diff mbox

[v5,04/10] block: Support meta dirty bitmap

Message ID 1464928382-9650-5-git-send-email-famz@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Fam Zheng June 3, 2016, 4:32 a.m. UTC
The added group of operations enables tracking of the changed bits in
the dirty bitmap.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block/dirty-bitmap.c         | 52 ++++++++++++++++++++++++++++++++++++++++++++
 include/block/dirty-bitmap.h |  9 ++++++++
 2 files changed, 61 insertions(+)

Comments

Max Reitz June 22, 2016, 3:53 p.m. UTC | #1
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 <famz@redhat.com>
> ---
>  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.

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 <mreitz@redhat.com>

> +    for (i = sector; i < sector + nb_sectors; i += gran) {
> +        if (hbitmap_get(bitmap->meta, i)) {
> +            return true;
> +        }
> +    }
> +    return false;
> +}
John Snow July 14, 2016, 8 p.m. UTC | #2
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 <famz@redhat.com>
>> ---
>>  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?

> 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 <mreitz@redhat.com>
> 
>> +    for (i = sector; i < sector + nb_sectors; i += gran) {
>> +        if (hbitmap_get(bitmap->meta, i)) {
>> +            return true;
>> +        }
>> +    }
>> +    return false;
>> +}
>
Max Reitz July 15, 2016, 12:04 p.m. UTC | #3
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 <famz@redhat.com>
>>> ---
>>>  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 <mreitz@redhat.com>
>>
>>> +    for (i = sector; i < sector + nb_sectors; i += gran) {
>>> +        if (hbitmap_get(bitmap->meta, i)) {
>>> +            return true;
>>> +        }
>>> +    }
>>> +    return false;
>>> +}
>>
>
Max Reitz July 15, 2016, 12:10 p.m. UTC | #4
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 <famz@redhat.com>
>>>> ---
>>>>  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 <mreitz@redhat.com>
>>>
>>>> +    for (i = sector; i < sector + nb_sectors; i += gran) {
>>>> +        if (hbitmap_get(bitmap->meta, i)) {
>>>> +            return true;
>>>> +        }
>>>> +    }
>>>> +    return false;
>>>> +}
>>>
>>
> 
>
John Snow July 15, 2016, 6:02 p.m. UTC | #5
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 <famz@redhat.com>
>>>> ---
>>>>  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 <mreitz@redhat.com>
>>>
>>>> +    for (i = sector; i < sector + nb_sectors; i += gran) {
>>>> +        if (hbitmap_get(bitmap->meta, i)) {
>>>> +            return true;
>>>> +        }
>>>> +    }
>>>> +    return false;
>>>> +}
>>>
>>
> 
>
Fam Zheng July 18, 2016, 6:59 a.m. UTC | #6
On Fri, 07/15 14:10, Max Reitz wrote:
> 
> 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 :-)

This is my fault, the delays were mostly because the motivation of this series
went away from my hands (QBM) and I handed it off to John not quite promptedly.

Fam
diff mbox

Patch

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.
+ *
+ * @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. */
+    for (i = sector; i < sector + nb_sectors; i += gran) {
+        if (hbitmap_get(bitmap->meta, i)) {
+            return true;
+        }
+    }
+    return false;
+}
+
+void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
+                                  BdrvDirtyBitmap *bitmap, int64_t sector,
+                                  int nb_sectors)
+{
+    hbitmap_reset(bitmap->meta, sector, nb_sectors);
+}
+
 bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
 {
     return bitmap->successor;
@@ -233,6 +284,7 @@  static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
         if ((!bitmap || bm == bitmap) && (!only_named || bm->name)) {
             assert(!bm->active_iterators);
             assert(!bdrv_dirty_bitmap_frozen(bm));
+            assert(!bm->meta);
             QLIST_REMOVE(bm, list);
             hbitmap_free(bm->bitmap);
             g_free(bm->name);
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
index 2ea601b..50e0fca 100644
--- a/include/block/dirty-bitmap.h
+++ b/include/block/dirty-bitmap.h
@@ -8,6 +8,9 @@  BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
                                           uint32_t granularity,
                                           const char *name,
                                           Error **errp);
+void bdrv_create_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap,
+                                   int chunk_size);
+void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap);
 int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
                                        BdrvDirtyBitmap *bitmap,
                                        Error **errp);
@@ -36,6 +39,12 @@  void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                            int64_t cur_sector, int nr_sectors);
 void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
                              int64_t cur_sector, int nr_sectors);
+int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
+                               BdrvDirtyBitmap *bitmap, int64_t sector,
+                               int nb_sectors);
+void bdrv_dirty_bitmap_reset_meta(BlockDriverState *bs,
+                                  BdrvDirtyBitmap *bitmap, int64_t sector,
+                                  int nb_sectors);
 BdrvDirtyBitmapIter *bdrv_dirty_iter_new(BdrvDirtyBitmap *bitmap,
                                          uint64_t first_sector);
 void bdrv_dirty_iter_free(BdrvDirtyBitmapIter *iter);