[Qemu-devel] [PATCH] qapi: add dirty-bitmaps to query-named-block-nodes result

[Qemu-devel] [PATCH] qapi: add dirty-bitmaps to query-named-block-nodes result

[Vladimir Sementsov-Ogievskiy]

Let's add a possibility to query dirty-bitmaps not only on root nodes.
It is useful when dealing both with snapshots and incremental backups.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 qapi/block-core.json | 5 ++++-
 block/qapi.c         | 5 +++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 1defcde048..64ae1ab812 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -360,6 +360,9 @@
 # @write_threshold: configured write threshold for the device.
 #                   0 if disabled. (Since 2.3)
 #
+# @dirty-bitmaps: dirty bitmaps information (only present if node
+#                 has one or more dirty bitmaps) (Since 4.1)
+#
 # Since: 0.14.0
 #
 ##
@@ -378,7 +381,7 @@
             '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
             '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
             '*iops_size': 'int', '*group': 'str', 'cache': 'BlockdevCacheInfo',
-            'write_threshold': 'int' } }
+            'write_threshold': 'int', '*dirty-bitmaps': ['BlockDirtyInfo'] } }
 
 ##
 # @BlockDeviceIoStatus:
diff --git a/block/qapi.c b/block/qapi.c
index 0c13c86f4e..7eefdecb29 100644
--- a/block/qapi.c
+++ b/block/qapi.c
@@ -78,6 +78,11 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
         info->backing_file = g_strdup(bs->backing_file);
     }
 
+    if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
+        info->has_dirty_bitmaps = true;
+        info->dirty_bitmaps = bdrv_query_dirty_bitmaps(bs);
+    }
+
     info->detect_zeroes = bs->detect_zeroes;
 
     if (blk && blk_get_public(blk)->throttle_group_member.throttle_state) {
-- 
2.18.0

Re: [Qemu-devel] [PATCH] qapi: add dirty-bitmaps to query-named-block-nodes result

[Eric Blake]

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

On 5/30/19 9:39 AM, Vladimir Sementsov-Ogievskiy wrote:
> Let's add a possibility to query dirty-bitmaps not only on root nodes.
> It is useful when dealing both with snapshots and incremental backups.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block-core.json | 5 ++++-
>  block/qapi.c         | 5 +++++
>  2 files changed, 9 insertions(+), 1 deletion(-)

Indeed useful.

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] qapi: add dirty-bitmaps to query-named-block-nodes result

[John Snow]

On 5/30/19 10:39 AM, Vladimir Sementsov-Ogievskiy wrote:
> Let's add a possibility to query dirty-bitmaps not only on root nodes.
> It is useful when dealing both with snapshots and incremental backups.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  qapi/block-core.json | 5 ++++-
>  block/qapi.c         | 5 +++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 1defcde048..64ae1ab812 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -360,6 +360,9 @@
>  # @write_threshold: configured write threshold for the device.
>  #                   0 if disabled. (Since 2.3)
>  #
> +# @dirty-bitmaps: dirty bitmaps information (only present if node
> +#                 has one or more dirty bitmaps) (Since 4.1)
> +#
>  # Since: 0.14.0
>  #
>  ##
> @@ -378,7 +381,7 @@
>              '*bps_wr_max_length': 'int', '*iops_max_length': 'int',
>              '*iops_rd_max_length': 'int', '*iops_wr_max_length': 'int',
>              '*iops_size': 'int', '*group': 'str', 'cache': 'BlockdevCacheInfo',
> -            'write_threshold': 'int' } }
> +            'write_threshold': 'int', '*dirty-bitmaps': ['BlockDirtyInfo'] } }
>  
>  ##
>  # @BlockDeviceIoStatus:
> diff --git a/block/qapi.c b/block/qapi.c
> index 0c13c86f4e..7eefdecb29 100644
> --- a/block/qapi.c
> +++ b/block/qapi.c
> @@ -78,6 +78,11 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>          info->backing_file = g_strdup(bs->backing_file);
>      }
>  
> +    if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
> +        info->has_dirty_bitmaps = true;
> +        info->dirty_bitmaps = bdrv_query_dirty_bitmaps(bs);
> +    }
> +
>      info->detect_zeroes = bs->detect_zeroes;
>  
>      if (blk && blk_get_public(blk)->throttle_group_member.throttle_state) {
> 

So query-block uses bdrv_query_info, which calls bdrv_block_device_info,
so we'll duplicate the bitmap output when doing the old-fashioned block
query, but that's probably harmless overall.

We can continue to support the output in both places, or we could opt to
deprecate the older interface; I think this is one of the last chances
we'd get to do so before libvirt and wider adoption.

I think that's probably Eric's choice.

Reviewed-by: John Snow <jsnow@redhat.com>

Re: [Qemu-devel] [PATCH] qapi: add dirty-bitmaps to query-named-block-nodes result

[Eric Blake]

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

On 5/30/19 11:26 AM, John Snow wrote:
> 
> 
> On 5/30/19 10:39 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Let's add a possibility to query dirty-bitmaps not only on root nodes.
>> It is useful when dealing both with snapshots and incremental backups.
>>

>> +++ b/block/qapi.c
>> @@ -78,6 +78,11 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>          info->backing_file = g_strdup(bs->backing_file);
>>      }
>>  
>> +    if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
>> +        info->has_dirty_bitmaps = true;
>> +        info->dirty_bitmaps = bdrv_query_dirty_bitmaps(bs);
>> +    }
>> +
>>      info->detect_zeroes = bs->detect_zeroes;
>>  
>>      if (blk && blk_get_public(blk)->throttle_group_member.throttle_state) {
>>
> 
> So query-block uses bdrv_query_info, which calls bdrv_block_device_info,
> so we'll duplicate the bitmap output when doing the old-fashioned block
> query, but that's probably harmless overall.

We already know that none of our existing query- interfaces are sane
(either too little information, or too much).  Duplication starts to
push an interface towards too much (it takes processor time to bundle up
the extra JSON, especially if the other end is not going to care if it
was present). I know Kevin still has somewhere on his to-do list the
implementation of a saner query- command for the information we really
want (about each block, without redundant information, and where we
don't repeat information in a nested manner, but where we also don't
omit information that would otherwise require multiple existing query-
to reconstruct).

> 
> We can continue to support the output in both places, or we could opt to
> deprecate the older interface; I think this is one of the last chances
> we'd get to do so before libvirt and wider adoption.
> 
> I think that's probably Eric's choice.

If you want to try to deprecate the old location, introspection at least
works to allow libvirt to know which place to look for it on a given
qemu. If you don't think deprecation is necessary, the duplication is
probably tolerable for now (as ideally we'd be deprecating ALL of our
not-quite-perfect query- block interfaces in favor of whatever sane
interface Kevin comes up with).

-- 
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] qapi: add dirty-bitmaps to query-named-block-nodes result

[John Snow]

On 5/31/19 10:55 AM, Eric Blake wrote:
> On 5/30/19 11:26 AM, John Snow wrote:
>>
>>
>> On 5/30/19 10:39 AM, Vladimir Sementsov-Ogievskiy wrote:
>>> Let's add a possibility to query dirty-bitmaps not only on root nodes.
>>> It is useful when dealing both with snapshots and incremental backups.
>>>
> 
>>> +++ b/block/qapi.c
>>> @@ -78,6 +78,11 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>>          info->backing_file = g_strdup(bs->backing_file);
>>>      }
>>>  
>>> +    if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
>>> +        info->has_dirty_bitmaps = true;
>>> +        info->dirty_bitmaps = bdrv_query_dirty_bitmaps(bs);
>>> +    }
>>> +
>>>      info->detect_zeroes = bs->detect_zeroes;
>>>  
>>>      if (blk && blk_get_public(blk)->throttle_group_member.throttle_state) {
>>>
>>
>> So query-block uses bdrv_query_info, which calls bdrv_block_device_info,
>> so we'll duplicate the bitmap output when doing the old-fashioned block
>> query, but that's probably harmless overall.
> 
> We already know that none of our existing query- interfaces are sane
> (either too little information, or too much).  Duplication starts to
> push an interface towards too much (it takes processor time to bundle up
> the extra JSON, especially if the other end is not going to care if it
> was present). I know Kevin still has somewhere on his to-do list the
> implementation of a saner query- command for the information we really
> want (about each block, without redundant information, and where we
> don't repeat information in a nested manner, but where we also don't
> omit information that would otherwise require multiple existing query-
> to reconstruct).
> 
>>
>> We can continue to support the output in both places, or we could opt to
>> deprecate the older interface; I think this is one of the last chances
>> we'd get to do so before libvirt and wider adoption.
>>
>> I think that's probably Eric's choice.
> 
> If you want to try to deprecate the old location, introspection at least
> works to allow libvirt to know which place to look for it on a given
> qemu. If you don't think deprecation is necessary, the duplication is
> probably tolerable for now (as ideally we'd be deprecating ALL of our
> not-quite-perfect query- block interfaces in favor of whatever sane
> interface Kevin comes up with).
> 

It sounds like it's probably the right move to deprecate the entire
legacy interface, but still... If you have 20 or 30 bitmaps on a root
node, you will see 40 or 60 entries.

What's the smart way to deprecate it? We're not adding new flags or
showing new arguments or anything. There might not be bitmaps, so you
can't rely on that field being present or absent.

Recommendations?

Re: [Qemu-devel] [PATCH] qapi: add dirty-bitmaps to query-named-block-nodes result

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 5/31/19 10:55 AM, Eric Blake wrote:
>> On 5/30/19 11:26 AM, John Snow wrote:
>>>
>>>
>>> On 5/30/19 10:39 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Let's add a possibility to query dirty-bitmaps not only on root nodes.
>>>> It is useful when dealing both with snapshots and incremental backups.
>>>>
>> 
>>>> +++ b/block/qapi.c
>>>> @@ -78,6 +78,11 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>>>          info->backing_file = g_strdup(bs->backing_file);
>>>>      }
>>>>  
>>>> +    if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
>>>> +        info->has_dirty_bitmaps = true;
>>>> +        info->dirty_bitmaps = bdrv_query_dirty_bitmaps(bs);
>>>> +    }
>>>> +
>>>>      info->detect_zeroes = bs->detect_zeroes;
>>>>  
>>>>      if (blk && blk_get_public(blk)->throttle_group_member.throttle_state) {
>>>>
>>>
>>> So query-block uses bdrv_query_info, which calls bdrv_block_device_info,
>>> so we'll duplicate the bitmap output when doing the old-fashioned block
>>> query, but that's probably harmless overall.
>> 
>> We already know that none of our existing query- interfaces are sane
>> (either too little information, or too much).  Duplication starts to
>> push an interface towards too much (it takes processor time to bundle up
>> the extra JSON, especially if the other end is not going to care if it
>> was present). I know Kevin still has somewhere on his to-do list the
>> implementation of a saner query- command for the information we really
>> want (about each block, without redundant information, and where we
>> don't repeat information in a nested manner, but where we also don't
>> omit information that would otherwise require multiple existing query-
>> to reconstruct).
>> 
>>>
>>> We can continue to support the output in both places, or we could opt to
>>> deprecate the older interface; I think this is one of the last chances
>>> we'd get to do so before libvirt and wider adoption.
>>>
>>> I think that's probably Eric's choice.
>> 
>> If you want to try to deprecate the old location, introspection at least
>> works to allow libvirt to know which place to look for it on a given
>> qemu. If you don't think deprecation is necessary, the duplication is
>> probably tolerable for now (as ideally we'd be deprecating ALL of our
>> not-quite-perfect query- block interfaces in favor of whatever sane
>> interface Kevin comes up with).
>> 
>
> It sounds like it's probably the right move to deprecate the entire
> legacy interface, but still... If you have 20 or 30 bitmaps on a root
> node, you will see 40 or 60 entries.
>
> What's the smart way to deprecate it? We're not adding new flags or
> showing new arguments or anything. There might not be bitmaps, so you
> can't rely on that field being present or absent.
>
> Recommendations?

Kevin's "[PATCH v4 0/6] file-posix: Add dynamic-auto-read-only QAPI
feature" adds "feature flags" to the QAPI schema language, limited to
struct types, because that's what he needs.  They're visible in
introspection.  I intend to complete his work, so we can tack
"deprecated" feature flags to pretty much anything

Could that address your need?

Re: [Qemu-devel] [PATCH] qapi: add dirty-bitmaps to query-named-block-nodes result

[John Snow]

On 6/5/19 8:46 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 5/31/19 10:55 AM, Eric Blake wrote:
>>> On 5/30/19 11:26 AM, John Snow wrote:
>>>>
>>>>
>>>> On 5/30/19 10:39 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Let's add a possibility to query dirty-bitmaps not only on root nodes.
>>>>> It is useful when dealing both with snapshots and incremental backups.
>>>>>
>>>
>>>>> +++ b/block/qapi.c
>>>>> @@ -78,6 +78,11 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>>>>          info->backing_file = g_strdup(bs->backing_file);
>>>>>      }
>>>>>  
>>>>> +    if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
>>>>> +        info->has_dirty_bitmaps = true;
>>>>> +        info->dirty_bitmaps = bdrv_query_dirty_bitmaps(bs);
>>>>> +    }
>>>>> +
>>>>>      info->detect_zeroes = bs->detect_zeroes;
>>>>>  
>>>>>      if (blk && blk_get_public(blk)->throttle_group_member.throttle_state) {
>>>>>
>>>>
>>>> So query-block uses bdrv_query_info, which calls bdrv_block_device_info,
>>>> so we'll duplicate the bitmap output when doing the old-fashioned block
>>>> query, but that's probably harmless overall.
>>>
>>> We already know that none of our existing query- interfaces are sane
>>> (either too little information, or too much).  Duplication starts to
>>> push an interface towards too much (it takes processor time to bundle up
>>> the extra JSON, especially if the other end is not going to care if it
>>> was present). I know Kevin still has somewhere on his to-do list the
>>> implementation of a saner query- command for the information we really
>>> want (about each block, without redundant information, and where we
>>> don't repeat information in a nested manner, but where we also don't
>>> omit information that would otherwise require multiple existing query-
>>> to reconstruct).
>>>
>>>>
>>>> We can continue to support the output in both places, or we could opt to
>>>> deprecate the older interface; I think this is one of the last chances
>>>> we'd get to do so before libvirt and wider adoption.
>>>>
>>>> I think that's probably Eric's choice.
>>>
>>> If you want to try to deprecate the old location, introspection at least
>>> works to allow libvirt to know which place to look for it on a given
>>> qemu. If you don't think deprecation is necessary, the duplication is
>>> probably tolerable for now (as ideally we'd be deprecating ALL of our
>>> not-quite-perfect query- block interfaces in favor of whatever sane
>>> interface Kevin comes up with).
>>>
>>
>> It sounds like it's probably the right move to deprecate the entire
>> legacy interface, but still... If you have 20 or 30 bitmaps on a root
>> node, you will see 40 or 60 entries.
>>
>> What's the smart way to deprecate it? We're not adding new flags or
>> showing new arguments or anything. There might not be bitmaps, so you
>> can't rely on that field being present or absent.
>>
>> Recommendations?
> 
> Kevin's "[PATCH v4 0/6] file-posix: Add dynamic-auto-read-only QAPI
> feature" adds "feature flags" to the QAPI schema language, limited to
> struct types, because that's what he needs.  They're visible in
> introspection.  I intend to complete his work, so we can tack
> "deprecated" feature flags to pretty much anything
> 
> Could that address your need?
> 

Hi Markus, digging this up again.

In brief, we are displaying bitmap info in the "wrong" part of the query
result (attached to drive instead of node) and would like to change it.
I'd like to avoid reporting bitmaps in both locations permanently, so if
we have a plan to deprecate reporting bitmaps in the old location, I
will tolerate the duplicated output temporarily.

Keeping in mind the bitmap fields are optional (so they can be absent
from both the new and old locations), what plan can we implement?

Perhaps I can add a feature flag "has-node-bitmaps" for 4.2. Then, for
the next three versions, I will report bitmaps from both locations.
Then, in 5.2+ I will remove the old location.

A client knows it can find bitmaps (if there are any) in the new
location if the feature flag is set. Otherwise, it should look in the
old location.

I think I've convinced myself that this is correct, so correct me if I
am wrong.

--js

Re: [Qemu-devel] [PATCH] qapi: add dirty-bitmaps to query-named-block-nodes result

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 6/5/19 8:46 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> On 5/31/19 10:55 AM, Eric Blake wrote:
>>>> On 5/30/19 11:26 AM, John Snow wrote:
>>>>>
>>>>>
>>>>> On 5/30/19 10:39 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> Let's add a possibility to query dirty-bitmaps not only on root nodes.
>>>>>> It is useful when dealing both with snapshots and incremental backups.
>>>>>>
>>>>
>>>>>> +++ b/block/qapi.c
>>>>>> @@ -78,6 +78,11 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>>>>>          info->backing_file = g_strdup(bs->backing_file);
>>>>>>      }
>>>>>>  
>>>>>> +    if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
>>>>>> +        info->has_dirty_bitmaps = true;
>>>>>> +        info->dirty_bitmaps = bdrv_query_dirty_bitmaps(bs);
>>>>>> +    }
>>>>>> +
>>>>>>      info->detect_zeroes = bs->detect_zeroes;
>>>>>>  
>>>>>>      if (blk && blk_get_public(blk)->throttle_group_member.throttle_state) {
>>>>>>
>>>>>
>>>>> So query-block uses bdrv_query_info, which calls bdrv_block_device_info,
>>>>> so we'll duplicate the bitmap output when doing the old-fashioned block
>>>>> query, but that's probably harmless overall.
>>>>
>>>> We already know that none of our existing query- interfaces are sane
>>>> (either too little information, or too much).  Duplication starts to
>>>> push an interface towards too much (it takes processor time to bundle up
>>>> the extra JSON, especially if the other end is not going to care if it
>>>> was present). I know Kevin still has somewhere on his to-do list the
>>>> implementation of a saner query- command for the information we really
>>>> want (about each block, without redundant information, and where we
>>>> don't repeat information in a nested manner, but where we also don't
>>>> omit information that would otherwise require multiple existing query-
>>>> to reconstruct).
>>>>
>>>>>
>>>>> We can continue to support the output in both places, or we could opt to
>>>>> deprecate the older interface; I think this is one of the last chances
>>>>> we'd get to do so before libvirt and wider adoption.
>>>>>
>>>>> I think that's probably Eric's choice.
>>>>
>>>> If you want to try to deprecate the old location, introspection at least
>>>> works to allow libvirt to know which place to look for it on a given
>>>> qemu. If you don't think deprecation is necessary, the duplication is
>>>> probably tolerable for now (as ideally we'd be deprecating ALL of our
>>>> not-quite-perfect query- block interfaces in favor of whatever sane
>>>> interface Kevin comes up with).
>>>>
>>>
>>> It sounds like it's probably the right move to deprecate the entire
>>> legacy interface, but still... If you have 20 or 30 bitmaps on a root
>>> node, you will see 40 or 60 entries.
>>>
>>> What's the smart way to deprecate it? We're not adding new flags or
>>> showing new arguments or anything. There might not be bitmaps, so you
>>> can't rely on that field being present or absent.
>>>
>>> Recommendations?
>> 
>> Kevin's "[PATCH v4 0/6] file-posix: Add dynamic-auto-read-only QAPI
>> feature" adds "feature flags" to the QAPI schema language, limited to
>> struct types, because that's what he needs.  They're visible in
>> introspection.  I intend to complete his work, so we can tack
>> "deprecated" feature flags to pretty much anything
>> 
>> Could that address your need?
>> 
>
> Hi Markus, digging this up again.
>
> In brief, we are displaying bitmap info in the "wrong" part of the query
> result (attached to drive instead of node) and would like to change it.

I lack context: which query command, which part of its result?

> I'd like to avoid reporting bitmaps in both locations permanently, so if
> we have a plan to deprecate reporting bitmaps in the old location, I
> will tolerate the duplicated output temporarily.

How bulky is the bitmap report?

> Keeping in mind the bitmap fields are optional (so they can be absent
> from both the new and old locations), what plan can we implement?

"Optional" is a syntactic thing, which ought to be backed by a semantic
"present iff" condition.  Have we specified such conditions?

> Perhaps I can add a feature flag "has-node-bitmaps" for 4.2. Then, for
> the next three versions, I will report bitmaps from both locations.
> Then, in 5.2+ I will remove the old location.

For how long has the bitmap been in the "wrong" place?  Any known
consumers?

> A client knows it can find bitmaps (if there are any) in the new
> location if the feature flag is set. Otherwise, it should look in the
> old location.
>
> I think I've convinced myself that this is correct, so correct me if I
> am wrong.

Sounds like a valid use of feature flags to me.  However, feature flags
are best used as a last resort.  With answers to my questions, I should
be able to compare the feature flags solution to possible alternatives.

Re: [Qemu-devel] [PATCH] qapi: add dirty-bitmaps to query-named-block-nodes result

[John Snow]

On 7/16/19 2:32 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> On 6/5/19 8:46 AM, Markus Armbruster wrote:
>>> John Snow <jsnow@redhat.com> writes:
>>>
>>>> On 5/31/19 10:55 AM, Eric Blake wrote:
>>>>> On 5/30/19 11:26 AM, John Snow wrote:
>>>>>>
>>>>>>
>>>>>> On 5/30/19 10:39 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>> Let's add a possibility to query dirty-bitmaps not only on root nodes.
>>>>>>> It is useful when dealing both with snapshots and incremental backups.
>>>>>>>
>>>>>
>>>>>>> +++ b/block/qapi.c
>>>>>>> @@ -78,6 +78,11 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>>>>>>          info->backing_file = g_strdup(bs->backing_file);
>>>>>>>      }
>>>>>>>  
>>>>>>> +    if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
>>>>>>> +        info->has_dirty_bitmaps = true;
>>>>>>> +        info->dirty_bitmaps = bdrv_query_dirty_bitmaps(bs);
>>>>>>> +    }
>>>>>>> +
>>>>>>>      info->detect_zeroes = bs->detect_zeroes;
>>>>>>>  
>>>>>>>      if (blk && blk_get_public(blk)->throttle_group_member.throttle_state) {
>>>>>>>
>>>>>>
>>>>>> So query-block uses bdrv_query_info, which calls bdrv_block_device_info,
>>>>>> so we'll duplicate the bitmap output when doing the old-fashioned block
>>>>>> query, but that's probably harmless overall.
>>>>>
>>>>> We already know that none of our existing query- interfaces are sane
>>>>> (either too little information, or too much).  Duplication starts to
>>>>> push an interface towards too much (it takes processor time to bundle up
>>>>> the extra JSON, especially if the other end is not going to care if it
>>>>> was present). I know Kevin still has somewhere on his to-do list the
>>>>> implementation of a saner query- command for the information we really
>>>>> want (about each block, without redundant information, and where we
>>>>> don't repeat information in a nested manner, but where we also don't
>>>>> omit information that would otherwise require multiple existing query-
>>>>> to reconstruct).
>>>>>
>>>>>>
>>>>>> We can continue to support the output in both places, or we could opt to
>>>>>> deprecate the older interface; I think this is one of the last chances
>>>>>> we'd get to do so before libvirt and wider adoption.
>>>>>>
>>>>>> I think that's probably Eric's choice.
>>>>>
>>>>> If you want to try to deprecate the old location, introspection at least
>>>>> works to allow libvirt to know which place to look for it on a given
>>>>> qemu. If you don't think deprecation is necessary, the duplication is
>>>>> probably tolerable for now (as ideally we'd be deprecating ALL of our
>>>>> not-quite-perfect query- block interfaces in favor of whatever sane
>>>>> interface Kevin comes up with).
>>>>>
>>>>
>>>> It sounds like it's probably the right move to deprecate the entire
>>>> legacy interface, but still... If you have 20 or 30 bitmaps on a root
>>>> node, you will see 40 or 60 entries.
>>>>
>>>> What's the smart way to deprecate it? We're not adding new flags or
>>>> showing new arguments or anything. There might not be bitmaps, so you
>>>> can't rely on that field being present or absent.
>>>>
>>>> Recommendations?
>>>
>>> Kevin's "[PATCH v4 0/6] file-posix: Add dynamic-auto-read-only QAPI
>>> feature" adds "feature flags" to the QAPI schema language, limited to
>>> struct types, because that's what he needs.  They're visible in
>>> introspection.  I intend to complete his work, so we can tack
>>> "deprecated" feature flags to pretty much anything
>>>
>>> Could that address your need?
>>>
>>
>> Hi Markus, digging this up again.
>>
>> In brief, we are displaying bitmap info in the "wrong" part of the query
>> result (attached to drive instead of node) and would like to change it.
> 
> I lack context: which query command, which part of its result?
> 

query-block (or is it block-query? Well, you know the one.)

It's the optional *dirty-bitmaps field. It's present when there are
bitmaps attached to the root of the device.

>> I'd like to avoid reporting bitmaps in both locations permanently, so if
>> we have a plan to deprecate reporting bitmaps in the old location, I
>> will tolerate the duplicated output temporarily.
> 
> How bulky is the bitmap report?
> 

@BlockDirtyInfo structure, four bools, a deprecated enum, uint32 and an in.

However, there can be any number of them. Possibly very many. If you
have 30 of them on the root node, adding their output to the correct
node means you will now see 60 bitmaps reported. (Augh.)

However, see below, if you add them to a node that doesn't qualify for
this top-level output, you'll only see them once.

[Incremental backup paradigm: only one per backup chain.
 Pull-mode checkpoint paradigm, at least n+1 bitmaps for n checkpoints.]

>> Keeping in mind the bitmap fields are optional (so they can be absent
>> from both the new and old locations), what plan can we implement?
> 
> "Optional" is a syntactic thing, which ought to be backed by a semantic
> "present iff" condition.  Have we specified such conditions?
> 

The BlockDirtyInfo is present when there are dirty bitmaps attached to
the root node of the "device".

(This special case, bitmaps attached to the "root" of a "device", may or
may not work completely correctly with anonymous BlockBackend structures
and other non-sugared syntaxes which seemingly create "special" trees
that cannot be replicated precisely with QMP commands. I forget the
particulars at the present moment.)

>> Perhaps I can add a feature flag "has-node-bitmaps" for 4.2. Then, for
>> the next three versions, I will report bitmaps from both locations.
>> Then, in 5.2+ I will remove the old location.
> 
> For how long has the bitmap been in the "wrong" place?  Any known
> consumers?
> 

1. Since it was introduced.

It's just that most of our use cases revolved around bitmaps being
attached in this way. With blockdev-backup and more flexible backup
modes, we might be creating bitmaps on nodes that aren't traditional
drives, and we need to be able to query those. We have no way to do so
in 4.1.

So we have had the ability to create mysterious, unqueriable bitmaps for
quite some time; since 2.4 at least.


2. Harder to answer. Yes, iotests 124 and 256 and 257 and others look
for bitmaps via block-query. These are all the "named" style bitmaps,
which I added in 2.4-ish timeframe. They've always been in this weird place.

However, dirty-bitmaps field has existed even before then, and was used
for communicating information about the implicit dirty bitmap used by
the mirror job. Paolo added that in 1.x, a long long time ago.

21b56835086 (Nov 13 2013) changed this field from "dirty" to
"dirty-bitmaps" with no justification for the break in compatibility.
The "dirty" field was the one used exclusively for mirror job's dirty
information, and this commit and series added support for named bitmaps,
and is now used primarily user-created, named bitmap objects. (But it
still reports the old, anonymous implicit type for mirror.)

Are there any users for this other kind of bitmap? Completely unknown.

I don't think we use it in iotests, and I don't see evidence of libvirt
using it. (I just checked with pkrempa and he doesn't seem to think it's
used that way either.)

>> A client knows it can find bitmaps (if there are any) in the new
>> location if the feature flag is set. Otherwise, it should look in the
>> old location.
>>
>> I think I've convinced myself that this is correct, so correct me if I
>> am wrong.
> 
> Sounds like a valid use of feature flags to me.  However, feature flags
> are best used as a last resort.  With answers to my questions, I should
> be able to compare the feature flags solution to possible alternatives.
> 

Re: [Qemu-devel] [PATCH] qapi: add dirty-bitmaps to query-named-block-nodes result

[John Snow]

On 7/16/19 11:26 AM, John Snow wrote:
> 
> 
> On 7/16/19 2:32 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>>
>>> On 6/5/19 8:46 AM, Markus Armbruster wrote:
>>>> John Snow <jsnow@redhat.com> writes:
>>>>
>>>>> On 5/31/19 10:55 AM, Eric Blake wrote:
>>>>>> On 5/30/19 11:26 AM, John Snow wrote:
>>>>>>>
>>>>>>>
>>>>>>> On 5/30/19 10:39 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>>>>>> Let's add a possibility to query dirty-bitmaps not only on root nodes.
>>>>>>>> It is useful when dealing both with snapshots and incremental backups.
>>>>>>>>
>>>>>>
>>>>>>>> +++ b/block/qapi.c
>>>>>>>> @@ -78,6 +78,11 @@ BlockDeviceInfo *bdrv_block_device_info(BlockBackend *blk,
>>>>>>>>          info->backing_file = g_strdup(bs->backing_file);
>>>>>>>>      }
>>>>>>>>  
>>>>>>>> +    if (!QLIST_EMPTY(&bs->dirty_bitmaps)) {
>>>>>>>> +        info->has_dirty_bitmaps = true;
>>>>>>>> +        info->dirty_bitmaps = bdrv_query_dirty_bitmaps(bs);
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>>      info->detect_zeroes = bs->detect_zeroes;
>>>>>>>>  
>>>>>>>>      if (blk && blk_get_public(blk)->throttle_group_member.throttle_state) {
>>>>>>>>
>>>>>>>
>>>>>>> So query-block uses bdrv_query_info, which calls bdrv_block_device_info,
>>>>>>> so we'll duplicate the bitmap output when doing the old-fashioned block
>>>>>>> query, but that's probably harmless overall.
>>>>>>
>>>>>> We already know that none of our existing query- interfaces are sane
>>>>>> (either too little information, or too much).  Duplication starts to
>>>>>> push an interface towards too much (it takes processor time to bundle up
>>>>>> the extra JSON, especially if the other end is not going to care if it
>>>>>> was present). I know Kevin still has somewhere on his to-do list the
>>>>>> implementation of a saner query- command for the information we really
>>>>>> want (about each block, without redundant information, and where we
>>>>>> don't repeat information in a nested manner, but where we also don't
>>>>>> omit information that would otherwise require multiple existing query-
>>>>>> to reconstruct).
>>>>>>
>>>>>>>
>>>>>>> We can continue to support the output in both places, or we could opt to
>>>>>>> deprecate the older interface; I think this is one of the last chances
>>>>>>> we'd get to do so before libvirt and wider adoption.
>>>>>>>
>>>>>>> I think that's probably Eric's choice.
>>>>>>
>>>>>> If you want to try to deprecate the old location, introspection at least
>>>>>> works to allow libvirt to know which place to look for it on a given
>>>>>> qemu. If you don't think deprecation is necessary, the duplication is
>>>>>> probably tolerable for now (as ideally we'd be deprecating ALL of our
>>>>>> not-quite-perfect query- block interfaces in favor of whatever sane
>>>>>> interface Kevin comes up with).
>>>>>>
>>>>>
>>>>> It sounds like it's probably the right move to deprecate the entire
>>>>> legacy interface, but still... If you have 20 or 30 bitmaps on a root
>>>>> node, you will see 40 or 60 entries.
>>>>>
>>>>> What's the smart way to deprecate it? We're not adding new flags or
>>>>> showing new arguments or anything. There might not be bitmaps, so you
>>>>> can't rely on that field being present or absent.
>>>>>
>>>>> Recommendations?
>>>>
>>>> Kevin's "[PATCH v4 0/6] file-posix: Add dynamic-auto-read-only QAPI
>>>> feature" adds "feature flags" to the QAPI schema language, limited to
>>>> struct types, because that's what he needs.  They're visible in
>>>> introspection.  I intend to complete his work, so we can tack
>>>> "deprecated" feature flags to pretty much anything
>>>>
>>>> Could that address your need?
>>>>
>>>
>>> Hi Markus, digging this up again.
>>>
>>> In brief, we are displaying bitmap info in the "wrong" part of the query
>>> result (attached to drive instead of node) and would like to change it.
>>
>> I lack context: which query command, which part of its result?
>>
> 
> query-block (or is it block-query? Well, you know the one.)
> 
> It's the optional *dirty-bitmaps field. It's present when there are
> bitmaps attached to the root of the device.
> 
>>> I'd like to avoid reporting bitmaps in both locations permanently, so if
>>> we have a plan to deprecate reporting bitmaps in the old location, I
>>> will tolerate the duplicated output temporarily.
>>
>> How bulky is the bitmap report?
>>
> 
> @BlockDirtyInfo structure, four bools, a deprecated enum, uint32 and an in.
> 
> However, there can be any number of them. Possibly very many. If you
> have 30 of them on the root node, adding their output to the correct
> node means you will now see 60 bitmaps reported. (Augh.)
> 
> However, see below, if you add them to a node that doesn't qualify for
> this top-level output, you'll only see them once.
> 
> [Incremental backup paradigm: only one per backup chain.
>  Pull-mode checkpoint paradigm, at least n+1 bitmaps for n checkpoints.]
> 
>>> Keeping in mind the bitmap fields are optional (so they can be absent
>>> from both the new and old locations), what plan can we implement?
>>
>> "Optional" is a syntactic thing, which ought to be backed by a semantic
>> "present iff" condition.  Have we specified such conditions?
>>
> 
> The BlockDirtyInfo is present when there are dirty bitmaps attached to
> the root node of the "device".
> 
> (This special case, bitmaps attached to the "root" of a "device", may or
> may not work completely correctly with anonymous BlockBackend structures
> and other non-sugared syntaxes which seemingly create "special" trees
> that cannot be replicated precisely with QMP commands. I forget the
> particulars at the present moment.)
> 
>>> Perhaps I can add a feature flag "has-node-bitmaps" for 4.2. Then, for
>>> the next three versions, I will report bitmaps from both locations.
>>> Then, in 5.2+ I will remove the old location.
>>
>> For how long has the bitmap been in the "wrong" place?  Any known
>> consumers?
>>
> 
> 1. Since it was introduced.
> 
> It's just that most of our use cases revolved around bitmaps being
> attached in this way. With blockdev-backup and more flexible backup
> modes, we might be creating bitmaps on nodes that aren't traditional
> drives, and we need to be able to query those. We have no way to do so
> in 4.1.
> 
> So we have had the ability to create mysterious, unqueriable bitmaps for
> quite some time; since 2.4 at least.
> 
> 
> 2. Harder to answer. Yes, iotests 124 and 256 and 257 and others look
> for bitmaps via block-query. These are all the "named" style bitmaps,
> which I added in 2.4-ish timeframe. They've always been in this weird place.
> 
> However, dirty-bitmaps field has existed even before then, and was used
> for communicating information about the implicit dirty bitmap used by
> the mirror job. Paolo added that in 1.x, a long long time ago.
> 
> 21b56835086 (Nov 13 2013) changed this field from "dirty" to
> "dirty-bitmaps" with no justification for the break in compatibility.
> The "dirty" field was the one used exclusively for mirror job's dirty
> information, and this commit and series added support for named bitmaps,
> and is now used primarily user-created, named bitmap objects. (But it
> still reports the old, anonymous implicit type for mirror.)
> 
> Are there any users for this other kind of bitmap? Completely unknown.
> 
> I don't think we use it in iotests, and I don't see evidence of libvirt
> using it. (I just checked with pkrempa and he doesn't seem to think it's
> used that way either.)
> 
>>> A client knows it can find bitmaps (if there are any) in the new
>>> location if the feature flag is set. Otherwise, it should look in the
>>> old location.
>>>
>>> I think I've convinced myself that this is correct, so correct me if I
>>> am wrong.
>>
>> Sounds like a valid use of feature flags to me.  However, feature flags
>> are best used as a last resort.  With answers to my questions, I should
>> be able to compare the feature flags solution to possible alternatives.
>>
> 

Update: I convinced myself I'm wrong, because even if the *output* isn't
always present, it's enough that the field is present via introspection.

So I'll resend my other version of this patch without the feature flag.

--js