| Message ID | 20190212010248.11056-2-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | dirty-bitmaps: deprecate @status field | expand |
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?
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.
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' } } >
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" } ]