Re: [Qemu-block] [PATCH] block/dirty-bitmap: Documentation and Comment fixups

[Vladimir Sementsov-Ogievskiy]

31.01.2019 4:01, John Snow wrote:
> The meaning of the states has changed subtly over time,
> this should bring the understanding more in-line with the
> current, actual usages.
> 
> Reported-by: Eric Blake <address@hidden>
> Signed-off-by: John Snow <address@hidden>
> ---
>   block/dirty-bitmap.c | 19 +++++++++++++------
>   qapi/block-core.json | 17 ++++++++++++-----
>   2 files changed, 25 insertions(+), 11 deletions(-)
> 
> diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
> index 00ea36f554..e2adf54dd3 100644
> --- a/block/dirty-bitmap.c
> +++ b/block/dirty-bitmap.c
> @@ -29,12 +29,19 @@
>   #include "block/blockjob.h"
>   
>   /**
> - * A BdrvDirtyBitmap can be in three possible states:
> - * (1) successor is NULL and disabled is false: full r/w mode
> - * (2) successor is NULL and disabled is true: read only mode ("disabled")
> - * (3) successor is set: frozen mode.
> - *     A frozen bitmap cannot be renamed, deleted, anonymized, cleared, set,
> - *     or enabled. A frozen bitmap can only abdicate() or reclaim().
> + * A BdrvDirtyBitmap can be in four possible user-visible states:
> + * (1) Active:   successor is NULL, and disabled is false: full r/w mode
> + * (2) Disabled: successor is NULL, and disabled is true: qualified r/w mode,
> + *               guest writes are dropped, but monitor writes are possible,
> + *               through commands like merge and clear.
> + * (3) Frozen:   successor is not null.
> + *               A frozen bitmap cannot be renamed, deleted, cleared, set,
> + *               enabled, merged to, etc. A frozen bitmap can only abdicate()
> + *               or reclaim().
> + *               In this state, the successor bitmap is Active and will
> + *               generally be recording writes from the guest for us.

Not necessarily. Frozen bitmap may be disabled in the same time, and successor 
is then
disabled too.

So, disabled/enabled is orthogonal to normal/frozen/locked..

Hm, while looking at this, I see that we don't check in _create_successor for 
locked
state, so we theoretically could create frozen-locked..

Also, I remember there was an idea of deprecating Frozen and reporting Locked 
for
backup too, didn't you think about it? So, successors becomes internal and 
invisible by
user in any sense, and we just say "Locked".

Anyway patch is good, I'd just prefer to fix it here, to show that Frozen may 
be disabled too.


> + * (4) Locked:   Whether Active or Disabled, the user cannot modify this 
> bitmap
> + *               in any way from the monitor.
>    */
>   struct BdrvDirtyBitmap {
>       QemuMutex *mutex;
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 91685be6c2..eba126c76e 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -418,10 +418,12 @@
>   # An enumeration of possible states that a dirty bitmap can report to the 
> user.
>   #
>   # @frozen: The bitmap is currently in-use by a backup operation or block 
> job,
> -#          and is immutable.
> +#          and is immutable. New writes by the guest are being recorded in a
> +#          cache, and are not lost.
>   #
> -# @disabled: The bitmap is currently in-use by an internal operation and is
> -#            read-only. It can still be deleted.
> +# @disabled: The bitmap is not currently recording new writes by the guest.
> +#            This is requested explicitly via @block-dirty-bitmap-disable.
> +#            It can still be cleared, deleted, or used for backup operations.
>   #
>   # @active: The bitmap is actively monitoring for new writes, and can be 
> cleared,
>   #          deleted, or used for backup operations.
> @@ -1944,9 +1946,14 @@
>   # @block-dirty-bitmap-merge:
>   #
>   # Merge dirty bitmaps listed in @bitmaps to the @target dirty bitmap.
> -# The @bitmaps dirty bitmaps are unchanged.
> +# Dirty bitmaps in @bitmaps will be unchanged.

except the case when @target is in @bitmaps too? Not sure about should we 
mention this.

About @frozen and @locked, we can also note that they can't be exported through 
NBD.
We can summarize, that @frozen and @locked means that user can't use them in any
command, and the only option is to list them. However even info->count 
information
should not be considered as something meaningful, as bitmaps are under some 
operations.
And we can also use same paragraph for @frozen and @locked, as a first step to 
@frozen
deprecation.

> +# Any bits already set in @target will still be set after the merge.
>   # On error, @target is unchanged.
>   #
> +# The resulting bitmap will count as dirty any clusters that were dirty in 
> any
> +# of the source bitmaps. This can be used to achieve backup checkpoints, or 
> in
> +# simpler usages, to copy bitmaps.
> +#
>   # Returns: nothing on success
>   #          If @node is not a valid block device, DeviceNotFound
>   #          If any bitmap in @bitmaps or @target is not found, GenericError
> @@ -1981,7 +1988,7 @@
>   ##
>   # @x-debug-block-dirty-bitmap-sha256:
>   #
> -# Get bitmap SHA256
> +# Get bitmap SHA256.
>   #
>   # Returns: BlockDirtyBitmapSha256 on success
>   #          If @node is not a valid block device, DeviceNotFound
> 


-- 
Best regards,
Vladimir