Re: [Qemu-devel] [PATCH v10 06/13] block: Add bdrv_copy_dirty_bitmap and bdrv_clear_dirty_bitmap

[John Snow]

On 12/30/2014 08:47 AM, Vladimir Sementsov-Ogievskiy wrote:
> I'm sorry if it was already discussed, but I think it is inconsistent to
> have "size" in sectors and "granularity" in bytes in one structure. I've
> misused these fields because of this in my current work.
>
> At least, I think there should be comments about this.
>
> Best regards,
> Vladimir

Looking at it now, I think it'd be worse to change it, because it 
represents the "size" of the bitmap, as in the number of logical bits 
for the interface of the bitmap. Since it is a sector bitmap, I think 
this should remain in sectors, for now.

I really want to keep the granularity in bytes in this case, because I 
want to match the existing interface, and size makes sense to me in sectors.

What I will do instead is just to document this quirk. Look out for V11 :)

--John

> On 23.12.2014 04:12, John Snow wrote:
> > Signed-off-by: Fam Zheng <address@hidden>
> > Signed-off-by: John Snow <address@hidden>
> > ---
> >   block.c               | 39 +++++++++++++++++++++++++++++++++++----
> >   include/block/block.h |  4 ++++
> >   2 files changed, 39 insertions(+), 4 deletions(-)
> >
> > diff --git a/block.c b/block.c
> > index a1d9e88..f9e0767 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -53,6 +53,8 @@
> >   struct BdrvDirtyBitmap {
> >       HBitmap *bitmap;
> > +    int64_t size;
> > +    int64_t granularity;
> >       char *name;
> >       QLIST_ENTRY(BdrvDirtyBitmap) list;
> >   };
> > @@ -5343,6 +5345,21 @@ void
> > bdrv_dirty_bitmap_make_anon(BlockDriverState *bs, BdrvDirtyBitmap
> > *bitmap)
> >       bitmap->name = NULL;
> >   }
> > +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
> > +                                        BdrvDirtyBitmap *bitmap,
> > +                                        const char *name)
> > +{
> > +    BdrvDirtyBitmap *new_bitmap;
> > +
> > +    new_bitmap = g_malloc0(sizeof(BdrvDirtyBitmap));
> > +    new_bitmap->bitmap = hbitmap_copy(bitmap->bitmap);
> > +    new_bitmap->size = bitmap->size;
> > +    new_bitmap->granularity = bitmap->granularity;
> > +    new_bitmap->name = g_strdup(name);
> > +    QLIST_INSERT_HEAD(&bs->dirty_bitmaps, new_bitmap, list);
> > +    return new_bitmap;
> > +}
> > +
> >   BdrvDirtyBitmap *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> >                                             int granularity,
> >                                             const char *name,
> > @@ -5350,6 +5367,7 @@ BdrvDirtyBitmap
> > *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> >   {
> >       int64_t bitmap_size;
> >       BdrvDirtyBitmap *bitmap;
> > +    int sector_granularity;
> >       assert((granularity & (granularity - 1)) == 0);
> > @@ -5357,8 +5375,8 @@ BdrvDirtyBitmap
> > *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> >           error_setg(errp, "Bitmap already exists: %s", name);
> >           return NULL;
> >       }
> > -    granularity >>= BDRV_SECTOR_BITS;
> > -    assert(granularity);
> > +    sector_granularity = granularity >> BDRV_SECTOR_BITS;
> > +    assert(sector_granularity);
> >       bitmap_size = bdrv_nb_sectors(bs);
> >       if (bitmap_size < 0) {
> >           error_setg_errno(errp, -bitmap_size, "could not get length
> > of device");
> > @@ -5366,7 +5384,9 @@ BdrvDirtyBitmap
> > *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> >           return NULL;
> >       }
> >       bitmap = g_new0(BdrvDirtyBitmap, 1);
> > -    bitmap->bitmap = hbitmap_alloc(bitmap_size, ffs(granularity) - 1);
> > +    bitmap->size = bitmap_size;
> > +    bitmap->granularity = granularity;
> > +    bitmap->bitmap = hbitmap_alloc(bitmap->size,
> > ffs(sector_granularity) - 1);
> >       bitmap->name = g_strdup(name);
> >       QLIST_INSERT_HEAD(&bs->dirty_bitmaps, bitmap, list);
> >       return bitmap;
> > @@ -5439,7 +5459,9 @@ uint64_t
> > bdrv_get_default_bitmap_granularity(BlockDriverState *bs)
> >   uint64_t bdrv_dirty_bitmap_granularity(BlockDriverState *bs,
> >                                          BdrvDirtyBitmap *bitmap)
> >   {
> > -    return BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap);
> > +    g_assert(BDRV_SECTOR_SIZE << hbitmap_granularity(bitmap->bitmap) ==
> > +             bitmap->granularity);
> > +    return bitmap->granularity;
> >   }
> >   void bdrv_dirty_iter_init(BlockDriverState *bs,
> > @@ -5460,6 +5482,15 @@ void bdrv_reset_dirty_bitmap(BlockDriverState
> > *bs, BdrvDirtyBitmap *bitmap,
> >       hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
> >   }
> > +/**
> > + * Effectively, reset the hbitmap from bits [0, size)
> > + * Synonymous with bdrv_reset_dirty_bitmap(bs, bitmap, 0, bitmap->size)
> > + */
> > +void bdrv_clear_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
> > *bitmap)
> > +{
> > +    hbitmap_reset(bitmap->bitmap, 0, bitmap->size);
> > +}
> > +
> >   static void bdrv_set_dirty(BlockDriverState *bs, int64_t cur_sector,
> >                              int nr_sectors)
> >   {
> > diff --git a/include/block/block.h b/include/block/block.h
> > index c7402e7..e964abd 100644
> > --- a/include/block/block.h
> > +++ b/include/block/block.h
> > @@ -436,6 +436,9 @@ BdrvDirtyBitmap
> > *bdrv_create_dirty_bitmap(BlockDriverState *bs,
> >   BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs,
> >                                           const char *name);
> >   void bdrv_dirty_bitmap_make_anon(BlockDriverState *bs,
> > BdrvDirtyBitmap *bitmap);
> > +BdrvDirtyBitmap *bdrv_copy_dirty_bitmap(BlockDriverState *bs,
> > +                                        BdrvDirtyBitmap *bitmap,
> > +                                        const char *name);
> >   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
> > *bitmap);
> >   BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
> >   uint64_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
> > @@ -446,6 +449,7 @@ void bdrv_set_dirty_bitmap(BlockDriverState *bs,
> > BdrvDirtyBitmap *bitmap,
> >                              int64_t cur_sector, int nr_sectors);
> >   void bdrv_reset_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
> > *bitmap,
> >                                int64_t cur_sector, int nr_sectors);
> > +void bdrv_clear_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap
> > *bitmap);
> >   void bdrv_dirty_iter_init(BlockDriverState *bs,
> >                             BdrvDirtyBitmap *bitmap, struct
> > HBitmapIter *hbi);
> >   int64_t bdrv_get_dirty_count(BlockDriverState *bs, BdrvDirtyBitmap
> > *bitmap);

--
—js