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 <js...@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 <vsement...@virtuozzo.com>
>
> And thank you for rewriting commit message, it's more clear now!
>
Thanks!
