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.
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 <vsementsov(a)virtuozzo.com>
And thank you for rewriting commit message, it's more clear now!
--
Best regards,
Vladimir