Re: [Qemu-devel] [PATCH v3 01/10] block/dirty-bitmap: add recording and busy properties
On 2/25/19 9:01 AM, Vladimir Sementsov-Ogievskiy wrote: > 23.02.2019 3:06, 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. >> >> Signed-off-by: John Snow >> --- >> +++ b/qapi/block-core.json >> @@ -458,7 +458,14 @@ >> # >> # @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 2.4) >> +# >> +# @recording: true if the bitmap is recording new writes from the guest. >> +# Replaces `active` and `disabled` statuses. (since 4.0) >> +# >> +# @busy: true if the bitmap is in-use by some operation (NBD or jobs) >> +#and cannot be modified via QMP or used by another operation. >> +#Replaces `locked` and `frozen` statuses. (since 4.0) > > > Don't we want instead an array of flags? Which will include also persistent > and > inconsistent? No, I don't think an array of flags is worth the extra complications in generation and parsing of the JSON. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v3 01/10] block/dirty-bitmap: add recording and busy properties
23.02.2019 3:06, 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.
>
> Signed-off-by: John Snow
> ---
> block/dirty-bitmap.c | 9 +
> qapi/block-core.json | 10 +-
> qemu-deprecated.texi | 6 ++
> tests/qemu-iotests/236.out | 28
> 4 files changed, 52 insertions(+), 1 deletion(-)
>
> 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 2b8afbb924..6e543594b3 100644
> --- a/qapi/block-core.json
> +++ b/qapi/block-core.json
> @@ -458,7 +458,14 @@
> #
> # @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 2.4)
> +#
> +# @recording: true if the bitmap is recording new writes from the guest.
> +# Replaces `active` and `disabled` statuses. (since 4.0)
> +#
> +# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
> +#and cannot be modified via QMP or used by another operation.
> +#Replaces `locked` and `frozen` statuses. (since 4.0)
Don't we want instead an array of flags? Which will include also persistent and
inconsistent?
> #
> # @persistent: true if the bitmap will eventually be flushed to persistent
> # storage (since 4.0)
> @@ -467,6 +474,7 @@
> ##
> { 'struct': 'BlockDirtyInfo',
> 'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
> + 'recording': 'bool', 'busy': 'bool',
> 'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
>
> ##
> diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
> index 45c57952da..4c7ae8180f 100644
> --- a/qemu-deprecated.texi
> +++ b/qemu-deprecated.texi
> @@ -67,6 +67,12 @@ topologies described with -smp include all possible cpus,
> i.e.
> "autoload" parameter is now ignored. All bitmaps are automatically loaded
> from qcow2 images.
>
> +@subsection query-block result field(s) dirty-bitmaps[i].status (since 4.0)
> +
> +The ``status'' field of the ``BlockDirtyInfo'' structure, returned by
> +the query-block command is deprecated. Two new boolean fields,
> +``recording'' and ``busy'' effectively replace it.
> +
> @subsection query-cpus (since 2.12.0)
>
> The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
> 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 0x3ff 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 0x3ff 64k
> "bitmaps": {
> "drive0": [
> {
> +"busy": false,
> "count": 262144,
>
Re: [Qemu-devel] [PATCH v3 01/10] block/dirty-bitmap: add recording and busy properties
23.02.2019 3:06, 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. > > Signed-off-by: John Snow Reviewed-by: Vladimir Sementsov-Ogievskiy -- Best regards, Vladimir
Re: [Qemu-devel] [PATCH v3 01/10] block/dirty-bitmap: add recording and busy properties
On 2/22/19 6:06 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. > > Signed-off-by: John Snow > --- > +++ b/qemu-deprecated.texi > @@ -67,6 +67,12 @@ topologies described with -smp include all possible cpus, > i.e. > "autoload" parameter is now ignored. All bitmaps are automatically loaded > from qcow2 images. > > +@subsection query-block result field(s) dirty-bitmaps[i].status (since 4.0) I suspect you wrote "field(s)" since there can be more than one dirty-bitmaps[i]. But it still sound funny; within a single dirty-bitmaps[i], there is only one field being deprecated. Dropping the '(s)' makes this subsection read better to me. That's minor enough that a maintainer could fix it, if you don't have any other reason to spin v4. Reviewed-by: Eric Blake -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
[Qemu-devel] [PATCH v3 01/10] block/dirty-bitmap: add recording and busy properties
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.
Signed-off-by: John Snow
---
block/dirty-bitmap.c | 9 +
qapi/block-core.json | 10 +-
qemu-deprecated.texi | 6 ++
tests/qemu-iotests/236.out | 28
4 files changed, 52 insertions(+), 1 deletion(-)
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 2b8afbb924..6e543594b3 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -458,7 +458,14 @@
#
# @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 2.4)
+#
+# @recording: true if the bitmap is recording new writes from the guest.
+# Replaces `active` and `disabled` statuses. (since 4.0)
+#
+# @busy: true if the bitmap is in-use by some operation (NBD or jobs)
+#and cannot be modified via QMP or used by another operation.
+#Replaces `locked` and `frozen` statuses. (since 4.0)
#
# @persistent: true if the bitmap will eventually be flushed to persistent
# storage (since 4.0)
@@ -467,6 +474,7 @@
##
{ 'struct': 'BlockDirtyInfo',
'data': {'*name': 'str', 'count': 'int', 'granularity': 'uint32',
+ 'recording': 'bool', 'busy': 'bool',
'status': 'DirtyBitmapStatus', 'persistent': 'bool' } }
##
diff --git a/qemu-deprecated.texi b/qemu-deprecated.texi
index 45c57952da..4c7ae8180f 100644
--- a/qemu-deprecated.texi
+++ b/qemu-deprecated.texi
@@ -67,6 +67,12 @@ topologies described with -smp include all possible cpus,
i.e.
"autoload" parameter is now ignored. All bitmaps are automatically loaded
from qcow2 images.
+@subsection query-block result field(s) dirty-bitmaps[i].status (since 4.0)
+
+The ``status'' field of the ``BlockDirtyInfo'' structure, returned by
+the query-block command is deprecated. Two new boolean fields,
+``recording'' and ``busy'' effectively replace it.
+
@subsection query-cpus (since 2.12.0)
The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
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 0x3ff 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 0x3ff 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 0x3fe 64k
"bitmaps": {
"drive0": [
{
+
