Patchwork [1/5] block/dirty-bitmap: add recording and busy properties

login
register
mail settings
Submitter John Snow
Date Feb. 12, 2019, 1:02 a.m.
Message ID <20190212010248.11056-2-jsnow@redhat.com>
Download mbox | patch
Permalink /patch/723461/
State New
Headers show

Comments

John Snow - Feb. 12, 2019, 1:02 a.m.
The current API allows us to report a single status, which we've defined as:

Frozen: has a successor, treated as qmp_locked, may or may not be enabled.
Locked: no successor, qmp_locked. may or may not be enabled.
Disabled: Not frozen or locked, disabled.
Active: Not frozen, locked, or disabled.

The problem is that both "Frozen" and "Locked" mean nearly the same thing,
and that both of them do not intuit whether they are recording guest writes
or not.

This patch deprecates that status field and introduces two orthogonal
properties instead to replace it.
---
 block/dirty-bitmap.c       |  9 +++++++++
 qapi/block-core.json       |  9 ++++++++-
 tests/qemu-iotests/236.out | 28 ++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+), 1 deletion(-)
Eric Blake - Feb. 12, 2019, 6:17 p.m.
On 2/11/19 7:02 PM, John Snow wrote:
> The current API allows us to report a single status, which we've defined as:
> 
> Frozen: has a successor, treated as qmp_locked, may or may not be enabled.
> Locked: no successor, qmp_locked. may or may not be enabled.
> Disabled: Not frozen or locked, disabled.
> Active: Not frozen, locked, or disabled.
> 
> The problem is that both "Frozen" and "Locked" mean nearly the same thing,
> and that both of them do not intuit whether they are recording guest writes
> or not.
> 
> This patch deprecates that status field and introduces two orthogonal
> properties instead to replace it.
> ---

No S-o-b?


> +++ b/qapi/block-core.json
> @@ -455,7 +455,13 @@
>  #
>  # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
>  #
> -# @status: current status of the dirty bitmap (since 2.4)
> +# @status: Deprecated in favor of @recording and @locked. (since 4.0)

I'd leave this one as (since 2.4). The deprecation clock is starting in
4.0, but the field has been present longer, and it is still obvious to
readers...

> +#
> +# @recording: true if the bitmap is recording new writes from the guest.
> +#             Replaces `active` status. (since 4.0)

...that the replacement fields are only usable in 4.0 and newer.

> +#
> +# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
> +#        and cannot be modified via QMP right now. (since 4.0)
>  #
>  # @persistent: true if the bitmap will eventually be flushed to persistent
>  #              storage (since 4.0)
> @@ -464,6 +470,7 @@
>  ##
>  { 'struct': 'BlockDirtyInfo',
>    'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
> +           'recording': 'bool', 'busy': 'bool',
>             'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }

The UI changes look reasonable.

No iotests coverage of "busy":true?
John Snow - Feb. 12, 2019, 6:23 p.m.
On 2/12/19 1:17 PM, Eric Blake wrote:
> On 2/11/19 7:02 PM, John Snow wrote:
>> The current API allows us to report a single status, which we've defined as:
>>
>> Frozen: has a successor, treated as qmp_locked, may or may not be enabled.
>> Locked: no successor, qmp_locked. may or may not be enabled.
>> Disabled: Not frozen or locked, disabled.
>> Active: Not frozen, locked, or disabled.
>>
>> The problem is that both "Frozen" and "Locked" mean nearly the same thing,
>> and that both of them do not intuit whether they are recording guest writes
>> or not.
>>
>> This patch deprecates that status field and introduces two orthogonal
>> properties instead to replace it.
>> ---
> 
> No S-o-b?
> 

Ah, heck, what's wrong with my script that it keeps dropping this? sorry.

> 
>> +++ b/qapi/block-core.json
>> @@ -455,7 +455,13 @@
>>  #
>>  # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
>>  #
>> -# @status: current status of the dirty bitmap (since 2.4)
>> +# @status: Deprecated in favor of @recording and @locked. (since 4.0)
> 
> I'd leave this one as (since 2.4). The deprecation clock is starting in
> 4.0, but the field has been present longer, and it is still obvious to
> readers...
> >> +#
>> +# @recording: true if the bitmap is recording new writes from the guest.
>> +#             Replaces `active` status. (since 4.0)
> 
> ...that the replacement fields are only usable in 4.0 and newer.
> 

OK. I'll just add a small note for what it was deprecated in favor of.

>> +#
>> +# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
>> +#        and cannot be modified via QMP right now. (since 4.0)
>>  #
>>  # @persistent: true if the bitmap will eventually be flushed to persistent
>>  #              storage (since 4.0)
>> @@ -464,6 +470,7 @@
>>  ##
>>  { 'struct': 'BlockDirtyInfo',
>>    'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
>> +           'recording': 'bool', 'busy': 'bool',
>>             'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
> 
> The UI changes look reasonable.
> 
> No iotests coverage of "busy":true?
> 

I'll add it to the list. I wrote test 124 when I was pretty new at
python and it's a dense monster to add things to, but I can probably
work it in without much problem.
Vladimir Sementsov-Ogievskiy - Feb. 13, 2019, 9:31 a.m.
On 12.02.2019 1:02, John Snow wrote:
> The current API allows us to report a single status, which we've defined as:
> 
> Frozen: has a successor, treated as qmp_locked, may or may not be enabled.
> Locked: no successor, qmp_locked. may or may not be enabled.
> Disabled: Not frozen or locked, disabled.
> Active: Not frozen, locked, or disabled.
> 
> The problem is that both "Frozen" and "Locked" mean nearly the same thing,
> and that both of them do not intuit whether they are recording guest writes
> or not.
> 
> This patch deprecates that status field and introduces two orthogonal
> properties instead to replace it.
> ---
>   block/dirty-bitmap.c       |  9 +++++++++
>   qapi/block-core.json       |  9 ++++++++-
>   tests/qemu-iotests/236.out | 28 ++++++++++++++++++++++++++++
>   3 files changed, 45 insertions(+), 1 deletion(-)
> 

> diff --git a/qapi/block-core.json b/qapi/block-core.json
> index 5a0f5f5a95..275f0d573c 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -455,7 +455,13 @@
>   #
>   # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
>   #
> -# @status: current status of the dirty bitmap (since 2.4)
> +# @status: Deprecated in favor of @recording and @locked. (since 4.0)
> +#
> +# @recording: true if the bitmap is recording new writes from the guest.
> +#             Replaces `active` status. (since 4.0)
> +#
> +# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
> +#        and cannot be modified via QMP right now. (since 4.0)

not only modified, but also cannot be used somehow for any other operation.

>   #
>   # @persistent: true if the bitmap will eventually be flushed to persistent
>   #              storage (since 4.0)
> @@ -464,6 +470,7 @@
>   ##
>   { 'struct': 'BlockDirtyInfo',
>     'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
> +           'recording': 'bool', 'busy': 'bool',
>              'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
>

Patch

diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c
index c6d4acebfa..101383b3af 100644
--- a/block/dirty-bitmap.c
+++ b/block/dirty-bitmap.c
@@ -226,6 +226,13 @@  DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap)
     }
 }
 
+/* Called with BQL taken.  */
+static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
+{
+    return !bitmap->disabled || (bitmap->successor &&
+                                 !bitmap->successor->disabled);
+}
+
 /**
  * Create a successor bitmap destined to replace this bitmap after an operation.
  * Requires that the bitmap is not frozen and has no successor.
@@ -448,6 +455,8 @@  BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs)
         info->has_name = !!bm->name;
         info->name = g_strdup(bm->name);
         info->status = bdrv_dirty_bitmap_status(bm);
+        info->recording = bdrv_dirty_bitmap_recording(bm);
+        info->busy = bdrv_dirty_bitmap_user_locked(bm);
         info->persistent = bm->persistent;
         entry->value = info;
         *plist = entry;
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5a0f5f5a95..275f0d573c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -455,7 +455,13 @@ 
 #
 # @granularity: granularity of the dirty bitmap in bytes (since 1.4)
 #
-# @status: current status of the dirty bitmap (since 2.4)
+# @status: Deprecated in favor of @recording and @locked. (since 4.0)
+#
+# @recording: true if the bitmap is recording new writes from the guest.
+#             Replaces `active` status. (since 4.0)
+#
+# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
+#        and cannot be modified via QMP right now. (since 4.0)
 #
 # @persistent: true if the bitmap will eventually be flushed to persistent
 #              storage (since 4.0)
@@ -464,6 +470,7 @@ 
 ##
 { 'struct': 'BlockDirtyInfo',
   'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
+           'recording': 'bool', 'busy': 'bool',
            'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
 
 ##
diff --git a/tests/qemu-iotests/236.out b/tests/qemu-iotests/236.out
index 5006f7bca1..815cd053f0 100644
--- a/tests/qemu-iotests/236.out
+++ b/tests/qemu-iotests/236.out
@@ -22,17 +22,21 @@  write -P0xcd 0x3ff0000 64k
   "bitmaps": {
     "drive0": [
       {
+        "busy": false,
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapB",
         "persistent": false,
+        "recording": true,
         "status": "active"
       },
       {
+        "busy": false,
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapA",
         "persistent": false,
+        "recording": true,
         "status": "active"
       }
     ]
@@ -84,17 +88,21 @@  write -P0xcd 0x3ff0000 64k
   "bitmaps": {
     "drive0": [
       {
+        "busy": false,
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapB",
         "persistent": false,
+        "recording": true,
         "status": "active"
       },
       {
+        "busy": false,
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapA",
         "persistent": false,
+        "recording": true,
         "status": "active"
       }
     ]
@@ -184,24 +192,30 @@  write -P0xea 0x3fe0000 64k
   "bitmaps": {
     "drive0": [
       {
+        "busy": false,
         "count": 393216,
         "granularity": 65536,
         "name": "bitmapC",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       },
       {
+        "busy": false,
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapB",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       },
       {
+        "busy": false,
         "count": 458752,
         "granularity": 65536,
         "name": "bitmapA",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       }
     ]
@@ -251,24 +265,30 @@  write -P0xea 0x3fe0000 64k
   "bitmaps": {
     "drive0": [
       {
+        "busy": false,
         "count": 393216,
         "granularity": 65536,
         "name": "bitmapC",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       },
       {
+        "busy": false,
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapB",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       },
       {
+        "busy": false,
         "count": 458752,
         "granularity": 65536,
         "name": "bitmapA",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       }
     ]
@@ -311,31 +331,39 @@  write -P0xea 0x3fe0000 64k
   "bitmaps": {
     "drive0": [
       {
+        "busy": false,
         "count": 458752,
         "granularity": 65536,
         "name": "bitmapD",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       },
       {
+        "busy": false,
         "count": 393216,
         "granularity": 65536,
         "name": "bitmapC",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       },
       {
+        "busy": false,
         "count": 262144,
         "granularity": 65536,
         "name": "bitmapB",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       },
       {
+        "busy": false,
         "count": 458752,
         "granularity": 65536,
         "name": "bitmapA",
         "persistent": false,
+        "recording": false,
         "status": "disabled"
       }
     ]