Re: [Qemu-devel] [PATCH 09/22] block: introduce persistent dirty bitmaps

[Vladimir Sementsov-Ogievskiy]

On 07.10.2016 20:54, Max Reitz wrote:
> On 30.09.2016 12:53, Vladimir Sementsov-Ogievskiy wrote:
> > New field BdrvDirtyBitmap.persistent means, that bitmap should be saved
> > on bdrv_close, using format driver. Format driver should maintain bitmap
> > storing.
> >
> > Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> > ---
> >   block.c                      | 30 ++++++++++++++++++++++++++++++
> >   block/dirty-bitmap.c         | 27 +++++++++++++++++++++++++++
> >   block/qcow2-bitmap.c         |  1 +
> >   include/block/block.h        |  2 ++
> >   include/block/block_int.h    |  2 ++
> >   include/block/dirty-bitmap.h |  6 ++++++
> >   6 files changed, 68 insertions(+)
> >
> > diff --git a/block.c b/block.c
> > index 804e3d4..1cde03a 100644
> > --- a/block.c
> > +++ b/block.c
> > @@ -2196,6 +2196,7 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
> >   static void bdrv_close(BlockDriverState *bs)
> >   {
> >       BdrvAioNotifier *ban, *ban_next;
> > +    Error *local_err = NULL;
> >   
> >       assert(!bs->job);
> >       assert(!bs->refcnt);
> > @@ -2204,6 +2205,10 @@ static void bdrv_close(BlockDriverState *bs)
> >       bdrv_flush(bs);
> >       bdrv_drain(bs); /* in case flush left pending I/O */
> >   
> > +    bdrv_store_persistent_bitmaps(bs, &local_err);
> > +    if (local_err != NULL) {
> > +        error_report_err(local_err);
> > +    }
> That seems pretty wrong to me. If the persistent bitmaps cannot be
> stored, the node should not be closed to avoid loss of data.
>
> >       bdrv_release_named_dirty_bitmaps(bs);
> Especially since the next function will just drop all the dirty bitmaps.
>
> I see the issue that bdrv_close() is only called by bdrv_delete() which
> in turn is only called by bdrv_unref(); and how are you supposed to
> react to bdrv_unref() failing?
>
> So I'm not sure how this issue should be addressed, but this is most
> certainly not ideal. You should not just drop supposedly persistent
> dirty bitmaps if they cannot be saved.
>
> We really should to have some way to keep the bitmap around if it cannot
> be saved, but I don't know how to do that either.
>
> In any case, we should make sure that the node supports saving
> persistent dirty bitmaps, because having persistent dirty bitmaps at a
> node that does not support them is something we can and must prevent
> beforehand.
>
> But I don't know how to handle failure if writing the dirty bitmap
> fails. I guess one could argue that it's the same as bdrv_flush()
> failing and thus can be handled in the same way, i.e. ignore it. I'm not
> happy with that, but I'd accept it if there's no other way.

For now, the only usage of these bitmaps is incremental backup and 
bitmaps are not critical data. If we lost them we will just do full 
backup. If there will be some critical persistent bdrv dirty bitmaps in 
future, we can introduce a callback BdrvDirtyBitmap.store_failed for 
them, which will somehow handle that case.. Detach bitmap from bs and 
save it in memory, add qmp commands to raw-dump them, etc.. I

> >       assert(QLIST_EMPTY(&bs->dirty_bitmaps));
> >   
> > @@ -3969,3 +3974,28 @@ void bdrv_del_child(BlockDriverState *parent_bs, BdrvChild *child, Error **errp)
> >   
> >       parent_bs->drv->bdrv_del_child(parent_bs, child, errp);
> >   }
> > +
> > +void bdrv_store_persistent_bitmaps(BlockDriverState *bs, Error **errp)
> > +{
> > +    BlockDriver *drv = bs->drv;
> > +
> > +    if (!bdrv_has_persistent_bitmaps(bs)) {
> > +        return;
> > +    }
> > +
> > +    if (!drv) {
> > +        error_setg_errno(errp, ENOMEDIUM,
> > +                         "Can't store persistent bitmaps to %s",
> > +                         bdrv_get_device_or_node_name(bs));
> > +        return;
> > +    }
> > +
> > +    if (!drv->bdrv_store_persistent_bitmaps) {
> > +        error_setg_errno(errp, ENOTSUP,
> > +                         "Can't store persistent bitmaps to %s",
> > +                         bdrv_get_device_or_node_name(bs));
> > +        return;
> > +    }
> > +
> > +    drv->bdrv_store_persistent_bitmaps(bs, errp);
> > +}
> > diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> > index 623e1d1..0314581 100644
> > --- a/block/dirty-bitmap.c
> > +++ b/block/dirty-bitmap.c
> > @@ -44,6 +44,7 @@ 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 persistent;            /* bitmap must be saved to owner disk image */
> >       bool autoload;              /* bitmap must be autoloaded on image opening 
> > */
> >       QLIST_ENTRY(BdrvDirtyBitmap) list;
> >   };
> > @@ -72,6 +73,7 @@ void bdrv_dirty_bitmap_make_anon(BdrvDirtyBitmap *bitmap)
> >       g_free(bitmap->name);
> >       bitmap->name = NULL;
> >   
> > +    bitmap->persistent = false;
> >       bitmap->autoload = false;
> >   }
> >   
> > @@ -241,6 +243,8 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
> >       bitmap->name = NULL;
> >       successor->name = name;
> >       bitmap->successor = NULL;
> > +    successor->persistent = bitmap->persistent;
> > +    bitmap->persistent = false;
> >       successor->autoload = bitmap->autoload;
> >       bitmap->autoload = false;
> >       bdrv_release_dirty_bitmap(bs, bitmap);
> > @@ -555,3 +559,26 @@ bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap 
> > *bitmap)
> >   {
> >       return bitmap->autoload;
> >   }
> > +
> > +void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap,
> > +                                                bool persistent)
> The second parameter should be aligned to the opening parenthesis.
>
> Max
>
> > +{
> > +    bitmap->persistent = persistent;
> > +}
> > +
> > +bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap)
> > +{
> > +    return bitmap->persistent;
> > +}
> > +
> > +bool bdrv_has_persistent_bitmaps(BlockDriverState *bs)
> > +{
> > +    BdrvDirtyBitmap *bm;
> > +    QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) {
> > +        if (bm->persistent) {
> > +            return true;
> > +        }
> > +    }
> > +
> > +    return false;
> > +}


--
Best regards,
Vladimir