Re: [Qemu-devel] [PATCH 08/13] block: Support meta dirty bitmap

[Fam Zheng <famz@redhat.com>]

On Thu, 01/07 14:30, John Snow wrote:
> > +void bdrv_release_meta_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> > +{
> > +    assert(bitmap->meta);
> > +    hbitmap_free(bitmap->meta);
> 
> This leaves a dangling pointer inside the Hbitmap, no?

Yes, will fix.

> 
> > +    bitmap->meta = NULL;
> > +}
> > +
> > +int bdrv_dirty_bitmap_get_meta(BlockDriverState *bs,
> > +                               BdrvDirtyBitmap *bitmap, int64_t sector,
> > +                               int nb_sectors)
> > +{
> > +    uint64_t i;
> > +    int gran = bdrv_dirty_bitmap_granularity(bitmap) >> BDRV_SECTOR_BITS;
> > +
> > +    /* To optimize: we can make hbitmap to internally check the range in a
> > +     * coarse level, or at least do it word by word. */
> > +    for (i = sector; i < sector + nb_sectors; i += gran) {
> > +        if (hbitmap_get(bitmap->meta, i)) {
> > +            return true;
> > +        }
> > +    }
> > +    return false;
> > +}
> > +
> 
> In essence get_meta() is a greedy algorithm that simply returns true if
> anything is set between [sector, sector + nb_sectors], yes?
> 
> Is this more useful than just using an iterator directly on the
> meta-bitmap?
> 
> I haven't finished reading but, I imagine that:
> 
> - If we need to check to see what is dirty specifically, we can just use
> the iterator. If the iterator doesn't return anything, we know it's
> empty. If it does return, we know exactly what's dirty.
> - If we need to explicitly check for emptiness in general, we can use
> the internal popcount.
> 
> 
> I'm not sure when a 'dirty range bool' will be explicitly useful all by
> itself, but maybe that becomes obvious later.

It's for the meta bitmap user to decide. In the case of persistent dirty bitmap
driver, I simply check whether the range of write request is meta-dirty, and
write the corresponding dirty bitmap range accordingly, rather than splitting
one write req into potentially multiple bit ranges that are meta-dirty. I think
this is reasonable, hence the interface.