Re: [Qemu-devel] [PATCH 22/24] block/dirty-bitmap: deep release dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

29.01.2017 19:54, Max Reitz wrote:
> On 23.01.2017 13:10, Vladimir Sementsov-Ogievskiy wrote:
> > Remove persistent bitmap from its storage on bdrv_release_dirty_bitmap.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   block/dirty-bitmap.c      | 21 ++++++++++++++++++---
> >   include/block/block_int.h |  3 +++
> >   2 files changed, 21 insertions(+), 3 deletions(-)
> >
> > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> > index 775181c9ab..0977df6309 100644
> > --- a/block/dirty-bitmap.c
> > +++ b/block/dirty-bitmap.c
> > @@ -297,7 +297,8 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
> >   
> >   static void bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
> >                                                     BdrvDirtyBitmap *bitmap,
> > -                                                  bool only_named)
> > +                                                  bool only_named,
> > +                                                  bool deep)
> I'd rather call it "remove_persistent" or something, which is bad
> grammar, but it's better at getting the point across.
>
> >   {
> >       BdrvDirtyBitmap *bm, *next;
> >       QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) {
> > @@ -305,6 +306,19 @@ static void 
> > bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
> >               assert(!bm->active_iterators);
> >               assert(!bdrv_dirty_bitmap_frozen(bm));
> >               assert(!bm->meta);
> > +
> > +            if (deep && bm->persistent && bs->drv &&
> > +                bs->drv->bdrv_remove_persistent_dirty_bitmap)
> > +            {
> > +                Error *local_err = NULL;
> > +                bs->drv->bdrv_remove_persistent_dirty_bitmap(bs, bm->name,
> > +                                                             &local_err);
> > +                if (local_err != NULL) {
> > +                    error_report_err(local_err);
> This looks like maybe it's the wrong thing to do... I do agree that
> sometimes it may not be fatal, but sometimes it probably is.
>
> > +                }
> > +            }
> > +
> > +
> >               QLIST_REMOVE(bm, list);
> >               hbitmap_free(bm->bitmap);
> >               g_free(bm->name);
> > @@ -322,16 +336,17 @@ static void 
> > bdrv_do_release_matching_dirty_bitmap(BlockDriverState *bs,
> >   
> >   void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap)
> >   {
> > -    bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false);
> > +    bdrv_do_release_matching_dirty_bitmap(bs, bitmap, false, true);
> The @deep parameter (or however you decide to call it) should be
> available to this function's caller, too. I don't believe qcow2's
> load_bitmap() wants to delete the persistent bitmap in the error path;
> and block-dirty-bitmap-remove should probably allow the user to decide
> what to do.
>
> Which brings me to the error thing above: If the user (or management
> tool) decides that block-dirty-bitmap-remove should remove the
> persistent bitmap, I believe that the operation should be aborted if the
> persistent bitmap cannot be removed. It should not just go on and
> release the in-memory bitmap but abort altogether. If the user just
> wants to release that in-memory bitmap then, they can still go on an
> call block-dirty-bitmap-remove with deep=false.

I've started to rewrite it in that way and it seems like deep almost 
always will be false. What about just move call to
bs->drv->bdrv_remove_persistent_dirty_bitmap() directly to 
qmp_block_dirty_bitmap_remove and add parameter
'remove_persistent' (or 'remove_from_storage', to make it maximum 
descriptive) only to this qmp command?

Or even without that parameter, as leaving inconsistent version of 
bitmap in the storage doesn't seem useful.

> >   }
> >   
> >   /**
> >    * Release all named dirty bitmaps attached to a BDS (for use in 
> > bdrv_close()).
> >    * There must not be any frozen bitmaps attached.
> > + * This function do not remove persistent bitmaps from the storage.
> *does
>
> Max
>
> >    */
> >   void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs)
> >   {
> > -    bdrv_do_release_matching_dirty_bitmap(bs, NULL, true);
> > +    bdrv_do_release_matching_dirty_bitmap(bs, NULL, true, false);
> >   }
> >   
> >   void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
> > diff --git a/include/block/block_int.h b/include/block/block_int.h
> > index b38b3aa198..3e080d6d68 100644
> > --- a/include/block/block_int.h
> > +++ b/include/block/block_int.h
> > @@ -326,6 +326,9 @@ struct BlockDriver {
> >                                                   Error **errp);
> >       bool (*bdrv_can_store_new_dirty_bitmap)(BlockDriverState *bs, const char 
> > *name,
> >                                               uint32_t granularity, Error 
> > **errp);
> > +    void (*bdrv_remove_persistent_dirty_bitmap)(BlockDriverState *bs,
> > +                                                const char *name,
> > +                                                Error **errp);
> >   
> >       QLIST_ENTRY(BlockDriver) list;
> >   };


--
Best regards,
Vladimir