Re: [Qemu-block] [Qemu-devel] [PATCH v4 05/17] dirty-bitmap: Change bdrv_dirty_bitmap_size() to report bytes

[John Snow]

On 07/10/2017 05:19 PM, Eric Blake wrote:
> On 07/10/2017 04:09 PM, John Snow wrote:
> > On 07/03/2017 11:10 AM, Eric Blake wrote:
> > > 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.
> > >
> > > 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.
> > >
> > > Signed-off-by: Eric Blake <address@hidden>
>
> > > @@ -305,8 +307,10 @@ 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);
> > > +    size = DIV_ROUND_UP(size, BDRV_SECTOR_SIZE);
> >
> > Do we need a TODO here as well, or are we going to track these in terms
> > of "sectors" permanently?
>
> The rounding goes away in patch 17/17 when I flip the internals to
> byte-based.  If a TODO comment here (that goes away in patch 17) makes
> review easier, I can add that.

Email confirmation is enough for me, personally.

--js