| Message ID | 20170628120530.31251-7-vsementsov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
On 06/28/2017 07:05 AM, Vladimir Sementsov-Ogievskiy wrote: > Add bdrv_dirty_bitmap_deserialize_ones() function, which is needed for > qcow2 bitmap loading, to handle unallocated bitmap parts, marked as > all-ones. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > Reviewed-by: Kevin Wolf <kwolf@redhat.com> > Reviewed-by: John Snow <jsnow@redhat.com> > --- > block/dirty-bitmap.c | 7 +++++++ > include/block/dirty-bitmap.h | 3 +++ > include/qemu/hbitmap.h | 15 +++++++++++++++ > util/hbitmap.c | 17 +++++++++++++++++ > 4 files changed, 42 insertions(+) > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index df0110cf9f..f502c45a70 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -586,6 +586,13 @@ void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap, > hbitmap_deserialize_zeroes(bitmap->bitmap, start, count, finish); > } > > +void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap, > + uint64_t start, uint64_t count, > + bool finish) > +{ > + hbitmap_deserialize_ones(bitmap->bitmap, start, count, finish); > +} What unit is 'count' in? /me reads the code, finding serialization_chunk() Oh, it's in terms of the underlying bitmap->bitmap->granularity (currently the number of sectors). Eww - I'd rather we name the parameter 'bytes' rather than 'count', and then scale it appropriately when passing to the underlying bitmap->bitmap, rather than making the user guess what scale to use. But as you are just copying from the existing deserialize_zeroes right above, I'd rather keep the semantics the same between the two, so my series will take care of fixing that up, and your series can go in as-is for this patch. > +++ b/include/qemu/hbitmap.h > @@ -229,6 +229,21 @@ void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count, > bool finish); > > /** > + * hbitmap_deserialize_ones > + * @hb: HBitmap to operate on. > + * @start: First bit to restore. > + * @count: Number of bits to restore. This part is accurate (the dirty-bitmap is using an underlying bitmap with "one bit per sector" before my series, afterwards it will be "one bit per byte", remembering that hbitmap really stores only one bit per granularity multiple of the underlying unit), if incomplete (the code asserts that things are aligned, but doesn't document that the caller must pass in aligned values); but again, that's matching the pre-existing deserialize_zeroes code.
On 06/29/2017 08:55 PM, Eric Blake wrote: > On 06/28/2017 07:05 AM, Vladimir Sementsov-Ogievskiy wrote: >> Add bdrv_dirty_bitmap_deserialize_ones() function, which is needed for >> qcow2 bitmap loading, to handle unallocated bitmap parts, marked as >> all-ones. >> >> + * hbitmap_deserialize_ones >> + * @hb: HBitmap to operate on. >> + * @start: First bit to restore. >> + * @count: Number of bits to restore. > > This part is accurate (the dirty-bitmap is using an underlying bitmap > with "one bit per sector" before my series, afterwards it will be "one > bit per byte", remembering that hbitmap really stores only one bit per > granularity multiple of the underlying unit), if incomplete (the code > asserts that things are aligned, but doesn't document that the caller > must pass in aligned values); but again, that's matching the > pre-existing deserialize_zeroes code. Okay, I looked again; the documentation for hbitmap_serialization_granularity() has a blanket statement that all other hbitmap_serialization_* functions taking a start and count must be aligned. Indirect, but at least documented, so I retract my statement about the docs being incomplete.
On 06/29/2017 10:01 PM, Eric Blake wrote: > On 06/29/2017 08:55 PM, Eric Blake wrote: >> On 06/28/2017 07:05 AM, Vladimir Sementsov-Ogievskiy wrote: >>> Add bdrv_dirty_bitmap_deserialize_ones() function, which is needed for >>> qcow2 bitmap loading, to handle unallocated bitmap parts, marked as >>> all-ones. >>> > >>> + * hbitmap_deserialize_ones >>> + * @hb: HBitmap to operate on. >>> + * @start: First bit to restore. >>> + * @count: Number of bits to restore. >> >> This part is accurate (the dirty-bitmap is using an underlying bitmap >> with "one bit per sector" before my series, afterwards it will be "one >> bit per byte", remembering that hbitmap really stores only one bit per >> granularity multiple of the underlying unit), if incomplete (the code >> asserts that things are aligned, but doesn't document that the caller >> must pass in aligned values); but again, that's matching the >> pre-existing deserialize_zeroes code. > > Okay, I looked again; the documentation for > hbitmap_serialization_granularity() has a blanket statement that all > other hbitmap_serialization_* functions taking a start and count must be > aligned. Indirect, but at least documented, so I retract my statement > about the docs being incomplete. > If the docs are confusing, please send patches to amend them; the bitmap code is definitely very confusing at times and I appreciate any and all insight to make the names, variable and documentation read better to be more intuitive. Thanks!
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index df0110cf9f..f502c45a70 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -586,6 +586,13 @@ void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap, hbitmap_deserialize_zeroes(bitmap->bitmap, start, count, finish); } +void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap, + uint64_t start, uint64_t count, + bool finish) +{ + hbitmap_deserialize_ones(bitmap->bitmap, start, count, finish); +} + void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap) { hbitmap_deserialize_finish(bitmap->bitmap); diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index ab89f08e3d..05451c727d 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -66,6 +66,9 @@ void bdrv_dirty_bitmap_deserialize_part(BdrvDirtyBitmap *bitmap, void bdrv_dirty_bitmap_deserialize_zeroes(BdrvDirtyBitmap *bitmap, uint64_t start, uint64_t count, bool finish); +void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap *bitmap, + uint64_t start, uint64_t count, + bool finish); void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap); /* Functions that require manual locking. */ diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h index 6b04391266..b52304ac29 100644 --- a/include/qemu/hbitmap.h +++ b/include/qemu/hbitmap.h @@ -229,6 +229,21 @@ void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count, bool finish); /** + * hbitmap_deserialize_ones + * @hb: HBitmap to operate on. + * @start: First bit to restore. + * @count: Number of bits to restore. + * @finish: Whether to call hbitmap_deserialize_finish automatically. + * + * Fills the bitmap with ones. + * + * If @finish is false, caller must call hbitmap_serialize_finish before using + * the bitmap. + */ +void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count, + bool finish); + +/** * hbitmap_deserialize_finish * @hb: HBitmap to operate on. * diff --git a/util/hbitmap.c b/util/hbitmap.c index 0b38817505..0c1591a594 100644 --- a/util/hbitmap.c +++ b/util/hbitmap.c @@ -551,6 +551,23 @@ void hbitmap_deserialize_zeroes(HBitmap *hb, uint64_t start, uint64_t count, } } +void hbitmap_deserialize_ones(HBitmap *hb, uint64_t start, uint64_t count, + bool finish) +{ + uint64_t el_count; + unsigned long *first; + + if (!count) { + return; + } + serialization_chunk(hb, start, count, &first, &el_count); + + memset(first, 0xff, el_count * sizeof(unsigned long)); + if (finish) { + hbitmap_deserialize_finish(hb); + } +} + void hbitmap_deserialize_finish(HBitmap *bitmap) { int64_t i, size, prev_size;