qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH v2 2/6] block/dirty-bitmaps: rename frozen predi


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [Qemu-devel] [PATCH v2 2/6] block/dirty-bitmaps: rename frozen predicate helper
Date: Mon, 18 Feb 2019 13:57:53 +0000

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 <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..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

reply via email to

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