| Message ID | 20211222174018.257550-8-vsementsov@virtuozzo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make image fleecing more usable | expand |
On 12/22/21 20:40, Vladimir Sementsov-Ogievskiy wrote: > Add a convenient function similar with bdrv_block_status() to get > status of dirty bitmap. > > Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com> > --- > include/block/dirty-bitmap.h | 2 ++ > include/qemu/hbitmap.h | 11 +++++++++++ > block/dirty-bitmap.c | 6 ++++++ > util/hbitmap.c | 36 ++++++++++++++++++++++++++++++++++++ > 4 files changed, 55 insertions(+) > > diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h > index f95d350b70..2ae7dc3d1d 100644 > --- a/include/block/dirty-bitmap.h > +++ b/include/block/dirty-bitmap.h > @@ -115,6 +115,8 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, int64_t offset, > bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap, > int64_t start, int64_t end, int64_t max_dirty_count, > int64_t *dirty_start, int64_t *dirty_count); > +void bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap, int64_t offset, > + int64_t bytes, bool *is_dirty, int64_t *count); > BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, > Error **errp); > > diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h > index 5e71b6d6f7..845fda12db 100644 > --- a/include/qemu/hbitmap.h > +++ b/include/qemu/hbitmap.h > @@ -340,6 +340,17 @@ bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t start, int64_t end, > int64_t max_dirty_count, > int64_t *dirty_start, int64_t *dirty_count); > > +/* > + * bdrv_dirty_bitmap_status: > + * @hb: The HBitmap to operate on > + * @start: the offset to start from > + * @end: end of requested area > + * @is_dirty: is bitmap dirty at @offset > + * @pnum: how many bits has same value starting from @offset > + */ > +void hbitmap_status(const HBitmap *hb, int64_t offset, int64_t bytes, > + bool *is_dirty, int64_t *pnum); > + I think description should be changed, there is no start and no end arguments in function. > /** > * hbitmap_iter_next: > * @hbi: HBitmapIter to operate on. > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c > index 94a0276833..e4a836749a 100644 > --- a/block/dirty-bitmap.c > +++ b/block/dirty-bitmap.c > @@ -875,6 +875,12 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap, > dirty_start, dirty_count); > } > > +void bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap, int64_t offset, > + int64_t bytes, bool *is_dirty, int64_t *count) > +{ > + hbitmap_status(bitmap->bitmap, offset, bytes, is_dirty, count); > +} > + > /** > * bdrv_merge_dirty_bitmap: merge src into dest. > * Ensures permissions on bitmaps are reasonable; use for public API. > diff --git a/util/hbitmap.c b/util/hbitmap.c > index 305b894a63..ae8d0eb4d2 100644 > --- a/util/hbitmap.c > +++ b/util/hbitmap.c > @@ -301,6 +301,42 @@ bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t start, int64_t end, > return true; > } > > +void hbitmap_status(const HBitmap *hb, int64_t start, int64_t count, > + bool *is_dirty, int64_t *pnum) > +{ > + int64_t next_dirty, next_zero; > + > + assert(start >= 0); > + assert(count > 0); > + assert(start + count <= hb->orig_size); > + > + next_dirty = hbitmap_next_dirty(hb, start, count); > + if (next_dirty == -1) { > + *pnum = count; > + *is_dirty = false; > + return; > + } > + > + if (next_dirty > start) { > + *pnum = next_dirty - start; > + *is_dirty = false; > + return; > + } > + > + assert(next_dirty == start); > + > + next_zero = hbitmap_next_zero(hb, start, count); > + if (next_zero == -1) { > + *pnum = count; > + *is_dirty = true; > + return; > + } > + > + assert(next_zero > start); > + *pnum = next_zero - start; > + *is_dirty = false; > +} > + This function finds if this bitmap is dirty and also counts first bits. I don't think that this is a problem, but may be it should be divided? > bool hbitmap_empty(const HBitmap *hb) > { > return hb->count == 0; With corrected description Reviewed-by: Nikita Lapshin<nikita.lapshin@virtuozzo.com>
17.01.2022 13:06, Nikta Lapshin wrote: > On 12/22/21 20:40, Vladimir Sementsov-Ogievskiy wrote: > >> Add a convenient function similar with bdrv_block_status() to get >> status of dirty bitmap. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy<vsementsov@virtuozzo.com> >> --- >> include/block/dirty-bitmap.h | 2 ++ >> include/qemu/hbitmap.h | 11 +++++++++++ >> block/dirty-bitmap.c | 6 ++++++ >> util/hbitmap.c | 36 ++++++++++++++++++++++++++++++++++++ >> 4 files changed, 55 insertions(+) >> >> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h >> index f95d350b70..2ae7dc3d1d 100644 >> --- a/include/block/dirty-bitmap.h >> +++ b/include/block/dirty-bitmap.h >> @@ -115,6 +115,8 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, int64_t offset, >> bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap, >> int64_t start, int64_t end, int64_t max_dirty_count, >> int64_t *dirty_start, int64_t *dirty_count); >> +void bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap, int64_t offset, >> + int64_t bytes, bool *is_dirty, int64_t *count); >> BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, >> Error **errp); >> >> diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h >> index 5e71b6d6f7..845fda12db 100644 >> --- a/include/qemu/hbitmap.h >> +++ b/include/qemu/hbitmap.h >> @@ -340,6 +340,17 @@ bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t start, int64_t end, >> int64_t max_dirty_count, >> int64_t *dirty_start, int64_t *dirty_count); >> >> +/* >> + * bdrv_dirty_bitmap_status: >> + * @hb: The HBitmap to operate on >> + * @start: the offset to start from >> + * @end: end of requested area >> + * @is_dirty: is bitmap dirty at @offset >> + * @pnum: how many bits has same value starting from @offset >> + */ >> +void hbitmap_status(const HBitmap *hb, int64_t offset, int64_t bytes, >> + bool *is_dirty, int64_t *pnum); >> + > > I think description should be changed, there is no start and no end > arguments in function. > >> /** >> * hbitmap_iter_next: >> * @hbi: HBitmapIter to operate on. >> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c >> index 94a0276833..e4a836749a 100644 >> --- a/block/dirty-bitmap.c >> +++ b/block/dirty-bitmap.c >> @@ -875,6 +875,12 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap, >> dirty_start, dirty_count); >> } >> >> +void bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap, int64_t offset, >> + int64_t bytes, bool *is_dirty, int64_t *count) >> +{ >> + hbitmap_status(bitmap->bitmap, offset, bytes, is_dirty, count); >> +} >> + >> /** >> * bdrv_merge_dirty_bitmap: merge src into dest. >> * Ensures permissions on bitmaps are reasonable; use for public API. >> diff --git a/util/hbitmap.c b/util/hbitmap.c >> index 305b894a63..ae8d0eb4d2 100644 >> --- a/util/hbitmap.c >> +++ b/util/hbitmap.c >> @@ -301,6 +301,42 @@ bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t start, int64_t end, >> return true; >> } >> >> +void hbitmap_status(const HBitmap *hb, int64_t start, int64_t count, >> + bool *is_dirty, int64_t *pnum) >> +{ >> + int64_t next_dirty, next_zero; >> + >> + assert(start >= 0); >> + assert(count > 0); >> + assert(start + count <= hb->orig_size); >> + >> + next_dirty = hbitmap_next_dirty(hb, start, count); >> + if (next_dirty == -1) { >> + *pnum = count; >> + *is_dirty = false; >> + return; >> + } >> + >> + if (next_dirty > start) { >> + *pnum = next_dirty - start; >> + *is_dirty = false; >> + return; >> + } >> + >> + assert(next_dirty == start); >> + >> + next_zero = hbitmap_next_zero(hb, start, count); >> + if (next_zero == -1) { >> + *pnum = count; >> + *is_dirty = true; >> + return; >> + } >> + >> + assert(next_zero > start); >> + *pnum = next_zero - start; >> + *is_dirty = false; >> +} >> + > > This function finds if this bitmap is dirty and also counts first bits. Not exactly. The idea was to have one function, that works like block_status: it return status of bit at offset and count how many bits are of the same status after it. > I don't think that this is a problem, but may be it should be divided? No, I need it as one function, for further commits. > >> bool hbitmap_empty(const HBitmap *hb) >> { >> return hb->count == 0; > > With corrected description > Reviewed-by: Nikita Lapshin<nikita.lapshin@virtuozzo.com> > thanks!
On 22.12.21 18:40, Vladimir Sementsov-Ogievskiy wrote: > Add a convenient function similar with bdrv_block_status() to get > status of dirty bitmap. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > include/block/dirty-bitmap.h | 2 ++ > include/qemu/hbitmap.h | 11 +++++++++++ > block/dirty-bitmap.c | 6 ++++++ > util/hbitmap.c | 36 ++++++++++++++++++++++++++++++++++++ > 4 files changed, 55 insertions(+) [...] > diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h > index 5e71b6d6f7..845fda12db 100644 > --- a/include/qemu/hbitmap.h > +++ b/include/qemu/hbitmap.h > @@ -340,6 +340,17 @@ bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t start, int64_t end, > int64_t max_dirty_count, > int64_t *dirty_start, int64_t *dirty_count); > > +/* > + * bdrv_dirty_bitmap_status: > + * @hb: The HBitmap to operate on > + * @start: the offset to start from > + * @end: end of requested area > + * @is_dirty: is bitmap dirty at @offset > + * @pnum: how many bits has same value starting from @offset > + */ > +void hbitmap_status(const HBitmap *hb, int64_t offset, int64_t bytes, In addition to the comment not fitting the parameter names, I also don’t find it ideal that the parameter names here don’t match the ones in the function’s definition. I don’t have a preference between `start` or `offset` (although most other bitmap functions seem to prefer `start`), but I do prefer `count` over `bytes`, because... Well, it’s a bit count, not a byte count, right? (And from the bitmap user’s perspective, those bits might stand for any arbitrary unit.) Apart from that, looks nice to me. I am wondering a bit why this function doesn’t simply return the dirty bit status (like, well, the block-status functions do it), but I presume you simply found this interface to be better suited for its callers. > + bool *is_dirty, int64_t *pnum); > + > /** > * hbitmap_iter_next: > * @hbi: HBitmapIter to operate on.
18.01.2022 16:31, Hanna Reitz wrote: >> +/* >> + * bdrv_dirty_bitmap_status: >> + * @hb: The HBitmap to operate on >> + * @start: the offset to start from >> + * @end: end of requested area >> + * @is_dirty: is bitmap dirty at @offset >> + * @pnum: how many bits has same value starting from @offset >> + */ >> +void hbitmap_status(const HBitmap *hb, int64_t offset, int64_t bytes, > > In addition to the comment not fitting the parameter names, I also don’t find it ideal that the parameter names here don’t match the ones in the function’s definition. > > I don’t have a preference between `start` or `offset` (although most other bitmap functions seem to prefer `start`), but I do prefer `count` over `bytes`, because... Well, it’s a bit count, not a byte count, right? (And from the bitmap user’s perspective, those bits might stand for any arbitrary unit.) > > Apart from that, looks nice to me. I am wondering a bit why this function doesn’t simply return the dirty bit status (like, well, the block-status functions do it), but I presume you simply found this interface to be better suited for its callers. Hmm, seems, no reason for it actually. Will change to use normal return value.
diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index f95d350b70..2ae7dc3d1d 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -115,6 +115,8 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, int64_t offset, bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap, int64_t start, int64_t end, int64_t max_dirty_count, int64_t *dirty_start, int64_t *dirty_count); +void bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap, int64_t offset, + int64_t bytes, bool *is_dirty, int64_t *count); BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, Error **errp); diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h index 5e71b6d6f7..845fda12db 100644 --- a/include/qemu/hbitmap.h +++ b/include/qemu/hbitmap.h @@ -340,6 +340,17 @@ bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t start, int64_t end, int64_t max_dirty_count, int64_t *dirty_start, int64_t *dirty_count); +/* + * bdrv_dirty_bitmap_status: + * @hb: The HBitmap to operate on + * @start: the offset to start from + * @end: end of requested area + * @is_dirty: is bitmap dirty at @offset + * @pnum: how many bits has same value starting from @offset + */ +void hbitmap_status(const HBitmap *hb, int64_t offset, int64_t bytes, + bool *is_dirty, int64_t *pnum); + /** * hbitmap_iter_next: * @hbi: HBitmapIter to operate on. diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 94a0276833..e4a836749a 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -875,6 +875,12 @@ bool bdrv_dirty_bitmap_next_dirty_area(BdrvDirtyBitmap *bitmap, dirty_start, dirty_count); } +void bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap, int64_t offset, + int64_t bytes, bool *is_dirty, int64_t *count) +{ + hbitmap_status(bitmap->bitmap, offset, bytes, is_dirty, count); +} + /** * bdrv_merge_dirty_bitmap: merge src into dest. * Ensures permissions on bitmaps are reasonable; use for public API. diff --git a/util/hbitmap.c b/util/hbitmap.c index 305b894a63..ae8d0eb4d2 100644 --- a/util/hbitmap.c +++ b/util/hbitmap.c @@ -301,6 +301,42 @@ bool hbitmap_next_dirty_area(const HBitmap *hb, int64_t start, int64_t end, return true; } +void hbitmap_status(const HBitmap *hb, int64_t start, int64_t count, + bool *is_dirty, int64_t *pnum) +{ + int64_t next_dirty, next_zero; + + assert(start >= 0); + assert(count > 0); + assert(start + count <= hb->orig_size); + + next_dirty = hbitmap_next_dirty(hb, start, count); + if (next_dirty == -1) { + *pnum = count; + *is_dirty = false; + return; + } + + if (next_dirty > start) { + *pnum = next_dirty - start; + *is_dirty = false; + return; + } + + assert(next_dirty == start); + + next_zero = hbitmap_next_zero(hb, start, count); + if (next_zero == -1) { + *pnum = count; + *is_dirty = true; + return; + } + + assert(next_zero > start); + *pnum = next_zero - start; + *is_dirty = false; +} + bool hbitmap_empty(const HBitmap *hb) { return hb->count == 0;
Add a convenient function similar with bdrv_block_status() to get status of dirty bitmap. Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- include/block/dirty-bitmap.h | 2 ++ include/qemu/hbitmap.h | 11 +++++++++++ block/dirty-bitmap.c | 6 ++++++ util/hbitmap.c | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 55 insertions(+)