Re: [Qemu-block] [Qemu-devel] [PATCH 09/25] block/dirty-bitmap: add readonly field to BdrvDirtyBitmap

[Sementsov-Ogievskiy Vladimir]

On 01.06.2017 01:58, John Snow wrote:
> On 05/19/2017 07:02 PM, John Snow wrote:
> > On 05/03/2017 08:25 AM, Vladimir Sementsov-Ogievskiy wrote:
> > > It will be needed in following commits for persistent bitmaps.
> > > If bitmap is loaded from read-only storage (and we can't mark it
> > > "in use" in this storage) corresponding BdrvDirtyBitmap should be
> > > read-only.
> > >
> > > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > > ---
> > >   block/dirty-bitmap.c         | 16 ++++++++++++++++
> > >   include/block/dirty-bitmap.h |  3 +++
> > >   2 files changed, 19 insertions(+)
> > >
> > > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> > > index 90af37287f..ab6a95cf41 100644
> > > --- a/block/dirty-bitmap.c
> > > +++ b/block/dirty-bitmap.c
> > > @@ -44,6 +44,8 @@ struct BdrvDirtyBitmap {
> > >       int64_t size;               /* Size of the bitmap (Number of sectors) */
> > >       bool disabled;              /* Bitmap is read-only */
> > >       int active_iterators;       /* How many iterators are active */
> > > +    bool readonly;              /* Bitmap is read-only and may be changed only
> > > +                                   by deserialize* functions */
> > >       QLIST_ENTRY(BdrvDirtyBitmap) list;
> > >   };
> > >   
> > > @@ -436,6 +438,7 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> > >                              int64_t cur_sector, int64_t nr_sectors)
> > >   {
> > >       assert(bdrv_dirty_bitmap_enabled(bitmap));
> > > +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
> > Doesn't this change the nature of the assertion?
> >
> > We're going from
> >      return !(bitmap->disabled || bitmap->successor);
> > to
> >         !bitmap->readonly
> >
> > That makes me a little nervous to ACK this patch, because the
> > correctness depends on how readonly is updated and manipulated in the
> > future, which makes it more prone to error.
> I must have been *REALLY* tired when I sent this; I had thought you were
> replacing an assertion instead of just adding one.
>
> I'm sorry about that.

I thought so,  don't worry)

> > >       hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
> > >   }
> > >   
> > > @@ -443,12 +446,14 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap,
> > >                                int64_t cur_sector, int64_t nr_sectors)
> > >   {
> > >       assert(bdrv_dirty_bitmap_enabled(bitmap));
> > > +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
> > >       hbitmap_reset(bitmap->bitmap, cur_sector, nr_sectors);
> > >   }
> > >   
> > >   void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out)
> > >   {
> > >       assert(bdrv_dirty_bitmap_enabled(bitmap));
> > > +    assert(!bdrv_dirty_bitmap_readonly(bitmap));
> > >       if (!out) {
> > >           hbitmap_reset_all(bitmap->bitmap);
> > >       } else {
> > > @@ -519,6 +524,7 @@ void bdrv_set_dirty(BlockDriverState *bs, int64_t 
> > > cur_sector,
> > >           if (!bdrv_dirty_bitmap_enabled(bitmap)) {
> > >               continue;
> > >           }
> > > +        assert(!bdrv_dirty_bitmap_readonly(bitmap));
> > >           hbitmap_set(bitmap->bitmap, cur_sector, nr_sectors);
> > >       }
> > >   }
> > > @@ -540,3 +546,13 @@ int64_t bdrv_get_meta_dirty_count(BdrvDirtyBitmap *bitmap)
> > >   {
> > >       return hbitmap_count(bitmap->meta);
> > >   }
> > > +
> > > +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap)
> > > +{
> > > +    return bitmap->readonly;
> > > +}
> > > +
> > > +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap)
> > > +{
> > > +    bitmap->readonly = true;
> > > +}
> > > diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> > > index 1e17729ac2..0aab5841f5 100644
> > > --- a/include/block/dirty-bitmap.h
> > > +++ b/include/block/dirty-bitmap.h
> > > @@ -75,4 +75,7 @@ void bdrv_dirty_bitmap_deserialize_ones(BdrvDirtyBitmap 
> > > *bitmap,
> > >                                           bool finish);
> > >   void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap);
> > >   
> > > +bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap);
> > > +void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap);
> > > +
> > >   #endif

--
Best regards,
Vladimir.