14.02.2019 2:23, 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 enabled and disabled in particular, it's actually okay for
> the internals to do this but only forbidden for users to invoke them, and
I'm a bit lost in this paragraph.. to this - what?, to invoke them - whom?
I think, it would be simpler for me to read patch itself :)
> all of the QMP entry uses already check against qmp_locked.
>
> Several other assertions really want to check that the bitmap isn't in-use
> by another operation -- use the qmp_locked function for this instead, which
> presently also checks for has_successor.
hm, you mean user_locked, not qmp_locked.
>
> Signed-off-by: John Snow <jsnow@xxxxxxxxxx>
> ---
> 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..639ebc0645 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -50,7 +50,7 @@ struct BdrvDirtyBitmap {
> HBitmap *meta; /* Meta dirty bitmap */
> bool qmp_locked; /* Bitmap is locked, it can't be modified
> through QMP */
> - BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */
> + BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state */
aha, looks like a good moment to fix preexisting misalignment of the comment,
but new line is exactly 80 characters length, so, not a good moment)
> char *name; /* Optional non-empty unique ID */
> int64_t size; /* Size of the bitmap, in bytes */
> bool disabled; /* Bitmap is disabled. It ignores all writes to
> @@ -183,14 +183,14 @@ const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap)
> }
>
> /* Called with BQL taken. */
> -bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap)
> +bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap)
> {
> return bitmap->successor;
> }
>
> /* Both conditions disallow user-modification via QMP. */
> bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) {
> - return bdrv_dirty_bitmap_frozen(bitmap) ||
> + return bdrv_dirty_bitmap_has_successor(bitmap) ||
> bdrv_dirty_bitmap_qmp_locked(bitmap);
> }
>
> @@ -215,7 +215,7 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap)
> /* Called with BQL taken. */
> DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
> {
> - if (bdrv_dirty_bitmap_frozen(bitmap)) {
> + if (bdrv_dirty_bitmap_has_successor(bitmap)) {
> return DIRTY_BITMAP_STATUS_FROZEN;
> } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) {
> return DIRTY_BITMAP_STATUS_LOCKED;
> @@ -235,7 +235,7 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
>
> /**
> * Create a successor bitmap destined to replace this bitmap after an operation.
> - * Requires that the bitmap is not frozen and has no successor.
> + * Requires that the bitmap is not locked and has no successor.
I think, user_locked, to not interfere with bitmaps mutex. And you use user_locked in
other comments in this patch.
> * Called with BQL taken.
> */
> int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> @@ -244,12 +244,16 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
> uint64_t granularity;
> BdrvDirtyBitmap *child;
>
> - if (bdrv_dirty_bitmap_frozen(bitmap)) {
> - error_setg(errp, "Cannot create a successor for a bitmap that is "
> - "currently frozen");
> + if (bdrv_dirty_bitmap_user_locked(bitmap)) {
> + error_setg(errp, "Cannot create a successor for a bitmap that is in-use "
> + "by an operation");
> + return -1;
> + }
> + if (bdrv_dirty_bitmap_has_successor(bitmap)) {
> + error_setg(errp, "Cannot create a successor for a bitmap that already "
> + "has one");
Amm, dead code? _user_locked() implies no successor, so we instead can keep an assertion..
> return -1;
> }
> - assert(!bitmap->successor);
>
> /* Create an anonymous successor */
> granularity = bdrv_dirty_bitmap_granularity(bitmap);
--
Best regards,
Vladimir
