Re: [Qemu-devel] [PATCH v5 15/21] block: Resize bitmaps on bdrv_truncate

[John Snow]

On 04/17/2015 09:25 AM, Max Reitz wrote:
> On 09.04.2015 00:19, John Snow wrote:
> > Signed-off-by: John Snow <address@hidden>
> > ---
> >   block.c                | 18 ++++++++++++++++++
> >   include/qemu/hbitmap.h | 10 ++++++++++
> >   util/hbitmap.c         | 48
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 76 insertions(+)
> >
> > diff --git a/block.c b/block.c
> > index 16209a2..42839a0 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -113,6 +113,7 @@ static void bdrv_set_dirty(BlockDriverState *bs,
> > int64_t cur_sector,
> >                              int nr_sectors);
> >   static void bdrv_reset_dirty(BlockDriverState *bs, int64_t cur_sector,
> >                                int nr_sectors);
> > +static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs);
> >   /* If non-zero, use only whitelisted block drivers */
> >   static int use_bdrv_whitelist;
> > @@ -3583,6 +3584,7 @@ int bdrv_truncate(BlockDriverState *bs, int64_t
> > offset)
> >       ret = drv->bdrv_truncate(bs, offset);
> >       if (ret == 0) {
> >           ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
> > +        bdrv_dirty_bitmap_truncate(bs);
> >           if (bs->blk) {
> >               blk_dev_resize_cb(bs->blk);
> >           }
> > @@ -5593,6 +5595,22 @@ BdrvDirtyBitmap
> > *bdrv_reclaim_dirty_bitmap(BlockDriverState *bs,
> >       return parent;
> >   }
> > +/**
> > + * Truncates _all_ bitmaps attached to a BDS.
> > + */
> > +static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
> > +{
> > +    BdrvDirtyBitmap *bitmap;
> > +    uint64_t size = bdrv_nb_sectors(bs);
> > +
> > +    QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> > +        if (bdrv_dirty_bitmap_frozen(bitmap)) {
> > +            continue;
> > +        }
> > +        hbitmap_truncate(bitmap->bitmap, size);
> > +    }
> > +}
> > +
> >   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
> > *bitmap)
> >   {
> >       BdrvDirtyBitmap *bm, *next;
> > diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h
> > index c19c1cb..a75157e 100644
> > --- a/include/qemu/hbitmap.h
> > +++ b/include/qemu/hbitmap.h
> > @@ -65,6 +65,16 @@ struct HBitmapIter {
> >   HBitmap *hbitmap_alloc(uint64_t size, int granularity);
> >   /**
> > + * hbitmap_truncate:
> > + * @hb: The bitmap to change the size of.
> > + * @size: The number of elements to change the bitmap to accommodate.
> > + *
> > + * truncate or grow an existing bitmap to accommodate a new number of
> > elements.
> > + * This may invalidate existing HBitmapIterators.
> > + */
> > +void hbitmap_truncate(HBitmap *hb, uint64_t size);
> > +
> > +/**
> >    * hbitmap_merge:
> >    * @a: The bitmap to store the result in.
> >    * @b: The bitmap to merge into @a.
> > diff --git a/util/hbitmap.c b/util/hbitmap.c
> > index ba11fd3..1ad3bf3 100644
> > --- a/util/hbitmap.c
> > +++ b/util/hbitmap.c
> > @@ -400,6 +400,54 @@ HBitmap *hbitmap_alloc(uint64_t size, int
> > granularity)
> >       return hb;
> >   }
> > +void hbitmap_truncate(HBitmap *hb, uint64_t size)
> > +{
> > +    bool shrink;
> > +    unsigned i;
> > +    uint64_t num_elements = size;
> > +    uint64_t old;
> > +
> > +    /* Size comes in as logical elements, adjust for granularity. */
> > +    size = (size + (1ULL << hb->granularity) - 1) >> hb->granularity;
> > +    assert(size <= ((uint64_t)1 << HBITMAP_LOG_MAX_SIZE));
> > +    shrink = size < hb->size;
> > +
> > +    /* bit sizes are identical; nothing to do. */
> > +    if (size == hb->size) {
> > +        return;
> > +    }
> > +
> > +    /* If we're losing bits, let's clear those bits before we
> > invalidate all of
> > +     * our invariants. This helps keep the bitcount consistent, and
> > will prevent
> > +     * us from carrying around garbage bits beyond the end of the map.
> > +     */
> > +    if (shrink) {
> > +        /* Don't clear partial granularity groups;
> > +         * start at the first full one. */
> > +        uint64_t start = QEMU_ALIGN_UP(num_elements, 1 <<
> > hb->granularity);
> > +        uint64_t fix_count = (hb->size << hb->granularity) -
> > num_elements;
>
> Shouldn't this be s/num_elements/start/?
>
> Max

Hmm... Yes, though in practice it doesn't appear to have mattered. Odd.

The effect here is that we will extend the fix count beyond the end of 
the bitmap by an amount less than the granularity.

hbitmap_reset doesn't seem to make any particular effort to ensure the 
virtual bit ranges you give it are valid in any way.

And I suppose due to the extra bits that hbitmap allocates for its own 
use meant that we never ran off the end of the array by a single bit or 
anything.

Well, fixed, regardless. Thanks.

> > +
> > +        assert(fix_count);
> > +        hbitmap_reset(hb, start, fix_count);
> > +    }
> > +
> > +    hb->size = size;
> > +    for (i = HBITMAP_LEVELS; i-- > 0; ) {
> > +        size = MAX(BITS_TO_LONGS(size), 1);
> > +        if (hb->sizes[i] == size) {
> > +            break;
> > +        }
> > +        old = hb->sizes[i];
> > +        hb->sizes[i] = size;
> > +        hb->levels[i] = g_realloc(hb->levels[i], size *
> > sizeof(unsigned long));
> > +        if (!shrink) {
> > +            memset(&hb->levels[i][old], 0x00,
> > +                   (size - old) * sizeof(*hb->levels[i]));
> > +        }
> > +    }
> > +}
> > +
> > +
> >   /**
> >    * Given HBitmaps A and B, let A := A (BITOR) B.
> >    * Bitmap B will not be modified.