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

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

[John Snow]

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 <eblake@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 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.
+ * (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.
+# 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
-- 
2.17.2

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

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 624 bytes --]

On 1/30/19 7:01 PM, 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 <eblake@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block/dirty-bitmap.c | 19 +++++++++++++------
>  qapi/block-core.json | 17 ++++++++++++-----
>  2 files changed, 25 insertions(+), 11 deletions(-)
> 
Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Qemu-devel] [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 <eblake@redhat.com>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>   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

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

[John Snow]

On 1/31/19 3:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> 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 <eblake@redhat.com>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   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..
> 

Ah, yeah, I was trying to communicate this but I failed. I shouldn't use
wiggly language like "generally." I'll clean this up to be more explicit.

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

That would be better. Would it cause any harm to clients that know about
"Frozen" but not about "Locked"? This might have to be a 3-releases
deprecation change.

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

Oh, right. I guess I'll try to be more careful about the phrasing.

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

Good suggestion.

>> +# 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
>>
> 
> 

Thanks!

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

[John Snow]

On 1/31/19 3:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> 
> 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.

There's a subtle difference in the two, namely that:

(1) Frozen may or may not record new writes, but they don't "show up"
until after the operation is over, because it's using a second bitmap as
a temporary buffer.

(2) Locked may or may not record new writes *immediately*.

Regardless, I'm realizing that for both Frozen and Locked bitmaps we
want to allow users to know if the bitmap is recording or not. (And
possibly if there is a temporary write cache or not, but maybe it's not
important.)

Maybe we want two new fields on query:

Recording/Enabled: [True | False]
Busy/Locked:       [True | False] (or "Locked)

which will then deprecate the status field. This doesn't manage to
communicate the write cache nuance, but maybe we don't need to.

(It also doesn't help elucidate whether or not the successor is
active/disabled, but even for migration once the guest has started, the
successors are always enabled, right?)

Thoughts?

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

[Vladimir Sementsov-Ogievskiy]

02.02.2019 4:01, John Snow wrote:
> 
> 
> On 1/31/19 3:29 AM, Vladimir Sementsov-Ogievskiy wrote:
>>
>> 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.
> 
> There's a subtle difference in the two, namely that:
> 
> (1) Frozen may or may not record new writes, but they don't "show up"
> until after the operation is over, because it's using a second bitmap as
> a temporary buffer.
> 
> (2) Locked may or may not record new writes *immediately*.
> 
> Regardless, I'm realizing that for both Frozen and Locked bitmaps we
> want to allow users to know if the bitmap is recording or not. (And
> possibly if there is a temporary write cache or not, but maybe it's not
> important.)
> 
> Maybe we want two new fields on query:
> 
> Recording/Enabled: [True | False]
> Busy/Locked:       [True | False] (or "Locked)
> 
> which will then deprecate the status field. This doesn't manage to
> communicate the write cache nuance, but maybe we don't need to.

Agree.

> 
> (It also doesn't help elucidate whether or not the successor is
> active/disabled, but even for migration once the guest has started, the
> successors are always enabled, right?)
> 
> Thoughts?
> 

Hmmm, it's all really questionable.

What we want to exporot about bitmaps?

A. For not-locked, not-frozen:

It's obvious enough: we want to say that bitmap is not locked and available for
any operation, and all information we have, is it disabled or not, dirty count, etc.

So we have two possible states here:
(not-locked, disabled)
(not-locked, enabled)

B. For locked of frozen:

Firsty, dirty-count seems not useful and not reliable.

Disabled/enabled.. what it means for locked bitmaps?

1. for frozen (used only in backups), enabled means, that writes are tracked in context
of this bitmaps, and after operation finish information about dirtiness will be available
somehow. But details about reclaim/abdicate are related to the operation, not to the bitmap
status.

2. for locked bitmap, again, enabled means that it tracks dirtiness.

...

So, yes, [recording, not-recording] x [busy, not-busy] works. And we may document
recording for busy case like "guest writes are tracked in context of the bitmap, but
availability of tracked information as well as bitmap status after holding operation
depends on the operation itself"


-- 
Best regards,
Vladimir