On 09/08/2017 07:22 AM, Kevin Wolf wrote: > Am 30.08.2017 um 23:05 hat Eric Blake geschrieben: >> We are still using an internal hbitmap that tracks a size in sectors, >> with the granularity scaled down accordingly, because it lets us >> use a shortcut for our iterators which are currently sector-based. >> But there's no reason we can't track the dirty bitmap size in bytes, >> since it is (mostly) an internal-only variable (remember, the size >> is how many bytes are covered by the bitmap, not how many bytes the >> bitmap occupies). Furthermore, we're already reporting bytes for >> bdrv_dirty_bitmap_granularity(); mixing bytes and sectors in our >> return values is a recipe for confusion. A later cleanup will >> convert dirty bitmap internals to be entirely byte-based, >> eliminating the intermediate sector rounding added here; and >> technically, since bdrv_getlength() already rounds up to sectors, >> our use of DIV_ROUND_UP is more for theoretical completeness than >> for any actual rounding. >> >> The only external caller in qcow2-bitmap.c is temporarily more verbose >> (because it is still using sector-based math), but will later be >> switched to track progress by bytes instead of sectors. >> >> Use is_power_of_2() while at it, instead of open-coding that, and >> add an assertion where bdrv_getlength() should not fail. >> >> Signed-off-by: Eric Blake <ebl...@redhat.com> >> Reviewed-by: John Snow <js...@redhat.com> > > I think I would have preferred to change the unit of > BdrvDirtyBitmap.size in one patch and the unit of the return value of > bdrv_dirty_bitmap_size() in another one to keep review a bit easier.
I can split on respin, if there's still enough reason for a respin. >> @@ -305,13 +307,14 @@ BdrvDirtyBitmap >> *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs, >> void bdrv_dirty_bitmap_truncate(BlockDriverState *bs) >> { >> BdrvDirtyBitmap *bitmap; >> - uint64_t size = bdrv_nb_sectors(bs); >> + int64_t size = bdrv_getlength(bs); >> >> + assert(size >= 0); > > How can you assert that there will never be an error? Even if it's > correct (I don't know whether you can have dirty bitmaps on devices that > don't use the cached value), this needs at least a comment. The old code wasn't checking for errors; if an error occurs, we have no way to report it. So I indeed need to audit whether all callers have a cached length at this point in time (it can't fail), or else change bdrv_dirty_bitmap_truncate() to be able to fail (pass failure along) and update all callers. This may indeed be reason for a respin, depending on what I find. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
signature.asc
Description: OpenPGP digital signature