2:09 p.m.
On 2/25/19 2:01 AM, Vladimir Sementsov-Ogievskiy wrote:
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 <jsnow(a)redhat.com>
> ---
> 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.
Reasonable.
> 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
?
Hm, we should add this to the .gitpublish profile, but yes, let me
re-add this.
> @@ -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 <vsementsov(a)virtuozzo.com>
And thank you for rewriting commit message, it's more clear now!
Thanks!