qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 02/10] block/dirty-bitmaps: rename frozen pre


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-devel] [PATCH v3 02/10] block/dirty-bitmaps: rename frozen predicate helper
Date: Mon, 25 Feb 2019 07:01:37 +0000

23.02.2019 3:06, John Snow wrote:
> "Frozen" was a good description a long time ago, but it isn't adequate now.
> Rename the frozen predicate to has_successor to make the semantics of the
> predicate more clear to outside callers.
> 
> In the process, remove some calls to frozen() that no longer semantically
> make sense. For bdrv_enable_dirty_bitmap_locked and
> bdrv_disable_dirty_bitmap_locked, it doesn't make sense to prohibit QEMU
> internals from performing this action when we only wished to prohibit QMP
> users from issuing these commands. All of the QMP API commands for bitmap
> manipulation already check against user_locked() to prohibit these actions.
> 
> Several other assertions really want to check that the bitmap isn't in-use
> by another operation -- use the bitmap_user_locked function for this instead,
> which presently also checks for has_successor. This leaves some redundant
> checks of has_sucessor through different helpers that are addressed in
> forthcoming patches.
> 
> Signed-off-by: John Snow <address@hidden>
> ---
>   block/dirty-bitmap.c           | 32 +++++++++++++++++---------------
>   include/block/dirty-bitmap.h   |  2 +-
>   migration/block-dirty-bitmap.c |  2 +-
>   3 files changed, 19 insertions(+), 17 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 101383b3af..aa3f86bb73 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c

[..]

> @@ -285,7 +288,7 @@ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap 
> *bitmap)
>   static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap)
>   {
>       assert(!bitmap->active_iterators);
> -    assert(!bdrv_dirty_bitmap_frozen(bitmap));
> +    assert(!bdrv_dirty_bitmap_user_locked(bitmap));

hmm. I'd prefere to keep a separate assertion for successor: we don't free it 
here,
so we really depend on absence of this object.

>       assert(!bitmap->meta);
>       QLIST_REMOVE(bitmap, list);
>       hbitmap_free(bitmap->bitmap);
> @@ -325,7 +328,7 @@ BdrvDirtyBitmap 
> *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs,
>   /**
>    * In cases of failure where we can no longer safely delete the parent,
>    * we may wish to re-join the parent and child/successor.
> - * The merged parent will be un-frozen, but not explicitly re-enabled.
> + * The merged parent will not be user_locked, nor explicitly re-enabled.
>    * Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken.
>    */
>   BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs,
> @@ -373,7 +376,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, 
> int64_t bytes)
>   
>       bdrv_dirty_bitmaps_lock(bs);
>       QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> -        assert(!bdrv_dirty_bitmap_frozen(bitmap));
> +        assert(!bdrv_dirty_bitmap_user_locked(bitmap));

and here too. As we don't truncate successors.

>           assert(!bitmap->active_iterators);
>           hbitmap_truncate(bitmap->bitmap, bytes);
>           bitmap->size = bytes;
> @@ -391,7 +394,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, 
> BdrvDirtyBitmap *bitmap)
>   
>   /**
>    * Release all named dirty bitmaps attached to a BDS (for use in 
> bdrv_close()).
> - * There must not be any frozen bitmaps attached.
> + * There must not be any user_locked bitmaps attached.
>    * This function does not remove persistent bitmaps from the storage.
>    * Called with BQL taken.
>    */
> @@ -428,7 +431,6 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState 
> *bs,
>   void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap)
>   {
>       bdrv_dirty_bitmap_lock(bitmap);
> -    assert(!bdrv_dirty_bitmap_frozen(bitmap));
>       bitmap->disabled = true;
>       bdrv_dirty_bitmap_unlock(bitmap);
>   }
> diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h
> index 04a117fc81..cdbb4dfefd 100644
> --- a/include/block/dirty-bitmap.h
> +++ b/include/block/dirty-bitmap.h

Could you please add scripts/git.orderfile to your .git/config, like

[diff]
     orderFile = /path/to/scripts/git.orderfile

?

> @@ -36,7 +36,7 @@ BlockDirtyInfoList 
> *bdrv_query_dirty_bitmaps(BlockDriverState *bs);
>   uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs);
>   uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap);
>   bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap);
> -bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap);
> +bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap);
>   const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap);
>   int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap);
>   DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap);
> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
> index 6426151e4f..ac6954142f 100644
> --- a/migration/block-dirty-bitmap.c
> +++ b/migration/block-dirty-bitmap.c
> @@ -542,7 +542,7 @@ static void dirty_bitmap_load_complete(QEMUFile *f, 
> DirtyBitmapLoadState *s)
>           }
>       }
>   
> -    if (bdrv_dirty_bitmap_frozen(s->bitmap)) {
> +    if (bdrv_dirty_bitmap_has_successor(s->bitmap)) {
>           bdrv_dirty_bitmap_lock(s->bitmap);
>           if (enabled_bitmaps == NULL) {
>               /* in postcopy */
> 


With my suggestions or without:
Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>

And thank you for rewriting commit message, it's more clear now!

-- 
Best regards,
Vladimir

reply via email to

[Prev in Thread] Current Thread [Next in Thread]