| Message ID | 20190620010356.19164-7-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | bitmaps: introduce 'bitmap' sync mode | expand |
On 20.06.19 03:03, John Snow wrote: > This function can claim an hbitmap to replace its own current hbitmap. > In the case that the granularities do not match, it will use > hbitmap_merge to copy the bit data instead. I really do not like this name because to me it implies a relationship to bdrv_reclaim_dirty_bitmap(). Maybe just bdrv_dirty_bitmap_take()? Or, if you want to get more fancy, bdrv_dirty_dirty_bitmap_steal(). (Because references are taken or stolen.) The latter might fit in nicely with the abdication theme. We’d just need to rename bdrv_reclaim_dirty_bitmap() to bdrv_dirty_bitmap_backstab(), and it’d be perfect. (On another note: bdrv_restore_dirty_bitmap() vs. bdrv_dirty_bitmap_restore() – really? :-/) > Signed-off-by: John Snow <jsnow@redhat.com> > --- > include/block/block_int.h | 1 + > include/qemu/hbitmap.h | 8 ++++++++ > block/dirty-bitmap.c | 14 ++++++++++++++ > util/hbitmap.c | 5 +++++ > 4 files changed, 28 insertions(+) The implementation looks good to me. Max
On 6/20/19 12:02 PM, Max Reitz wrote: > On 20.06.19 03:03, John Snow wrote: >> This function can claim an hbitmap to replace its own current hbitmap. >> In the case that the granularities do not match, it will use >> hbitmap_merge to copy the bit data instead. > > I really do not like this name because to me it implies a relationship > to bdrv_reclaim_dirty_bitmap(). Maybe just bdrv_dirty_bitmap_take()? > Or, if you want to get more fancy, bdrv_dirty_dirty_bitmap_steal(). > (Because references are taken or stolen.) > take or steal is good. I just wanted to highlight that it truly takes ownership. The double-pointer and erasing the caller's reference for emphasis of this idea. > The latter might fit in nicely with the abdication theme. We’d just > need to rename bdrv_reclaim_dirty_bitmap() to > bdrv_dirty_bitmap_backstab(), and it’d be perfect. > Don't tempt me; I do like my silly function names. You are lucky I don't call > (On another note: bdrv_restore_dirty_bitmap() vs. > bdrv_dirty_bitmap_restore() – really? :-/) > I have done a terrible job at enforcing any kind of consistency here, and it gets me all the time too. I had a big series that re-arranged and re-named a ton of stuff just to make things a little more nicer, but I let it bitrot because I didn't want to deal with the bike-shedding. I do agree I am in desperate need of a spring cleaning in here. One thing that does upset me quite often is that the canonical name for the structure is "bdrv dirty bitmap", which makes the function names all quite long. >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> include/block/block_int.h | 1 + >> include/qemu/hbitmap.h | 8 ++++++++ >> block/dirty-bitmap.c | 14 ++++++++++++++ >> util/hbitmap.c | 5 +++++ >> 4 files changed, 28 insertions(+) > > The implementation looks good to me. > > Max >
20.06.2019 19:36, John Snow wrote: > > > On 6/20/19 12:02 PM, Max Reitz wrote: >> On 20.06.19 03:03, John Snow wrote: >>> This function can claim an hbitmap to replace its own current hbitmap. >>> In the case that the granularities do not match, it will use >>> hbitmap_merge to copy the bit data instead. >> >> I really do not like this name because to me it implies a relationship >> to bdrv_reclaim_dirty_bitmap(). Maybe just bdrv_dirty_bitmap_take()? >> Or, if you want to get more fancy, bdrv_dirty_dirty_bitmap_steal(). >> (Because references are taken or stolen.) >> > > take or steal is good. I just wanted to highlight that it truly takes > ownership. The double-pointer and erasing the caller's reference for > emphasis of this idea. Didn't you consider bdrv_dirty_bitmap_set_hbitmap? Hmm, but your function actually eats pointer, so 'set' is not enough.. bdrv_dirty_bitmap_eat_hbitmap? And to be serious: it is the point where we definitely should drop HBitmap:meta field, as it in bad relation with parent hbitmap stealing. > >> The latter might fit in nicely with the abdication theme. We’d just >> need to rename bdrv_reclaim_dirty_bitmap() to >> bdrv_dirty_bitmap_backstab(), and it’d be perfect. >> > > Don't tempt me; I do like my silly function names. You are lucky I don't > call > >> (On another note: bdrv_restore_dirty_bitmap() vs. >> bdrv_dirty_bitmap_restore() – really? :-/) >> > > I have done a terrible job at enforcing any kind of consistency here, > and it gets me all the time too. I had a big series that re-arranged and > re-named a ton of stuff just to make things a little more nicer, but I > let it bitrot because I didn't want to deal with the bike-shedding. > > I do agree I am in desperate need of a spring cleaning in here. > > One thing that does upset me quite often is that the canonical name for > the structure is "bdrv dirty bitmap", which makes the function names all > quite long. > >>> Signed-off-by: John Snow <jsnow@redhat.com> >>> --- >>> include/block/block_int.h | 1 + >>> include/qemu/hbitmap.h | 8 ++++++++ >>> block/dirty-bitmap.c | 14 ++++++++++++++ >>> util/hbitmap.c | 5 +++++ >>> 4 files changed, 28 insertions(+) >> >> The implementation looks good to me. >> >> Max >> >
On 6/21/19 7:58 AM, Vladimir Sementsov-Ogievskiy wrote: > 20.06.2019 19:36, John Snow wrote: >> >> >> On 6/20/19 12:02 PM, Max Reitz wrote: >>> On 20.06.19 03:03, John Snow wrote: >>>> This function can claim an hbitmap to replace its own current hbitmap. >>>> In the case that the granularities do not match, it will use >>>> hbitmap_merge to copy the bit data instead. >>> >>> I really do not like this name because to me it implies a relationship >>> to bdrv_reclaim_dirty_bitmap(). Maybe just bdrv_dirty_bitmap_take()? >>> Or, if you want to get more fancy, bdrv_dirty_dirty_bitmap_steal(). >>> (Because references are taken or stolen.) >>> >> >> take or steal is good. I just wanted to highlight that it truly takes >> ownership. The double-pointer and erasing the caller's reference for >> emphasis of this idea. > > Didn't you consider bdrv_dirty_bitmap_set_hbitmap? Hmm, but your function > actually eats pointer, so 'set' is not enough.. bdrv_dirty_bitmap_eat_hbitmap? > :) > And to be serious: it is the point where we definitely should drop HBitmap:meta > field, as it in bad relation with parent hbitmap stealing. > You're right, I didn't consider how this would interact with that. Allow me the time to re-audit how this feature works, there's clearly a lot of problems with what I've proposed for cross-granularity merging. >> >>> The latter might fit in nicely with the abdication theme. We’d just >>> need to rename bdrv_reclaim_dirty_bitmap() to >>> bdrv_dirty_bitmap_backstab(), and it’d be perfect. >>> >> >> Don't tempt me; I do like my silly function names. You are lucky I don't >> call >> >>> (On another note: bdrv_restore_dirty_bitmap() vs. >>> bdrv_dirty_bitmap_restore() – really? :-/) >>> >> >> I have done a terrible job at enforcing any kind of consistency here, >> and it gets me all the time too. I had a big series that re-arranged and >> re-named a ton of stuff just to make things a little more nicer, but I >> let it bitrot because I didn't want to deal with the bike-shedding. >> >> I do agree I am in desperate need of a spring cleaning in here. >> >> One thing that does upset me quite often is that the canonical name for >> the structure is "bdrv dirty bitmap", which makes the function names all >> quite long. >> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> include/block/block_int.h | 1 + >>>> include/qemu/hbitmap.h | 8 ++++++++ >>>> block/dirty-bitmap.c | 14 ++++++++++++++ >>>> util/hbitmap.c | 5 +++++ >>>> 4 files changed, 28 insertions(+) >>> >>> The implementation looks good to me. >>> >>> Max >>> >> > >
diff --git a/include/block/block_int.h b/include/block/block_int.h index 89370c1b9b..7348ea8e78 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1240,6 +1240,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes); void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out); void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup); +void bdrv_dirty_bitmap_claim(BdrvDirtyBitmap *bitmap, HBitmap **hbitmap); void bdrv_inc_in_flight(BlockDriverState *bs); void bdrv_dec_in_flight(BlockDriverState *bs); diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h index 4afbe6292e..c74519042a 100644 --- a/include/qemu/hbitmap.h +++ b/include/qemu/hbitmap.h @@ -82,6 +82,14 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size); */ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result); +/** + * hbitmap_same_conf: + * + * Compares the configuration for HBitmaps A and B. + * Return true if they are identical, false otherwise. + */ +bool hbitmap_same_conf(const HBitmap *a, const HBitmap *b); + /** * hbitmap_can_merge: * diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 95a9c2a5d8..15c857e445 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -632,6 +632,20 @@ void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup) hbitmap_free(tmp); } +/* claim ownership of an hbitmap */ +void bdrv_dirty_bitmap_claim(BdrvDirtyBitmap *bitmap, HBitmap **hbitmap) +{ + if (hbitmap_same_conf(bitmap->bitmap, *hbitmap)) { + bdrv_restore_dirty_bitmap(bitmap, *hbitmap); + } else { + assert(hbitmap_can_merge(bitmap->bitmap, *hbitmap)); + bdrv_clear_dirty_bitmap(bitmap, NULL); + hbitmap_merge(bitmap->bitmap, *hbitmap, bitmap->bitmap); + hbitmap_free(*hbitmap); + } + *hbitmap = NULL; +} + uint64_t bdrv_dirty_bitmap_serialization_size(const BdrvDirtyBitmap *bitmap, uint64_t offset, uint64_t bytes) { diff --git a/util/hbitmap.c b/util/hbitmap.c index 0d6724b7bc..a2abd425b5 100644 --- a/util/hbitmap.c +++ b/util/hbitmap.c @@ -775,6 +775,11 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size) } } +bool hbitmap_same_conf(const HBitmap *a, const HBitmap *b) +{ + return (a->size == b->size) && (a->granularity == b->granularity); +} + bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b) { return (a->size == b->size);
This function can claim an hbitmap to replace its own current hbitmap. In the case that the granularities do not match, it will use hbitmap_merge to copy the bit data instead. Signed-off-by: John Snow <jsnow@redhat.com> --- include/block/block_int.h | 1 + include/qemu/hbitmap.h | 8 ++++++++ block/dirty-bitmap.c | 14 ++++++++++++++ util/hbitmap.c | 5 +++++ 4 files changed, 28 insertions(+)