[libvirt] [PATCH v2 0/6] dirty-bitmaps: deprecate @status field
The current internal meanings of "locked", "user_locked", "qmp_locked", "frozen", "enabled", and "disabled" are all a little muddled. Deprecate the @status field in favor of two new booleans that carry very specific meanings. Then, rename and rework some of the internal semantics to help make the API a bit more clear and easier to read. Well, in my opinion. Based on my current bitmaps branch (includes Eric's patch and my documentation update patch.) V2: - All of Eric's suggestions, I hope. - Vladimir's phrasing suggestion on patch 1 - Added a sixth patch that's mostly just motion. I'm on PTO the next two days, so I didn't get to writing a test for the busy bit. I think test 223 is a good candidate, because it uses the NBD functionality. To test with push mode, I need to come up with a blockdebug configuration that will let me pause an incremental backup so I can test race-free. Maybe for V3, sorry. John Snow (6): block/dirty-bitmap: add recording and busy properties block/dirty-bitmaps: rename frozen predicate helper block/dirty-bitmap: change semantics of enabled predicate block/dirty-bitmap: explicitly lock bitmaps with successors block/dirty-bitmaps: unify qmp_locked and user_locked calls block/dirty-bitmaps: move comment block block/dirty-bitmap.c | 110 ++++++++++++++++++--------------- blockdev.c | 18 +++--- include/block/dirty-bitmap.h | 7 +-- migration/block-dirty-bitmap.c | 8 +-- nbd/server.c | 6 +- qapi/block-core.json | 10 ++- qemu-deprecated.texi | 6 ++ tests/qemu-iotests/236.out | 28 +++++++++ 8 files changed, 122 insertions(+), 71 deletions(-) -- 2.17.2
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 <jsnow@redhat.com> --- 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 8f23f2ebb8..5d1d182447 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 80b0702ad5..f7c9f4c101 100644 --- a/qemu-deprecated.texi +++ b/qemu-deprecated.texi @@ -84,6 +84,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 dirty-bitmaps.status parameter (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 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" } ] -- 2.17.2
On 2/13/19 5:23 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 <jsnow@redhat.com> --- 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(-)
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
14.02.2019 2:23, 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 <jsnow@redhat.com> --- 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 8f23f2ebb8..5d1d182447 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 80b0702ad5..f7c9f4c101 100644 --- a/qemu-deprecated.texi +++ b/qemu-deprecated.texi @@ -84,6 +84,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 dirty-bitmaps.status parameter (since 4.0)
It is not a parameter. It's a field of result structure.. maybe @subsection query-block result field 'dirty-bitmaps[i].status' (since 4.0) with or without it: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
+ +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.
Hm, preexisting, but do we have some good alternative to `` '' pair? Maybe a kind of @var{} or something?
+ @subsection query-cpus (since 2.12.0)
The ``query-cpus'' command is replaced by the ``query-cpus-fast'' command.
-- Best regards, Vladimir
"Frozen" was a good description a long time ago, but it isn't adequate now. Rename the frozen predicate to has_successor to make the semantics of the predicate more clear to outside callers. In the process, remove some calls to frozen() that no longer semantically make sense. For enabled and disabled in particular, it's actually okay for the internals to do this but only forbidden for users to invoke them, and all of the QMP entry uses already check against qmp_locked. Several other assertions really want to check that the bitmap isn't in-use by another operation -- use the qmp_locked function for this instead, which presently also checks for has_successor. Signed-off-by: John Snow <jsnow@redhat.com> --- block/dirty-bitmap.c | 32 +++++++++++++++++--------------- include/block/dirty-bitmap.h | 2 +- migration/block-dirty-bitmap.c | 2 +- 3 files changed, 19 insertions(+), 17 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 101383b3af..639ebc0645 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -50,7 +50,7 @@ struct BdrvDirtyBitmap { HBitmap *meta; /* Meta dirty bitmap */ bool qmp_locked; /* Bitmap is locked, it can't be modified through QMP */ - BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */ + BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state */ char *name; /* Optional non-empty unique ID */ int64_t size; /* Size of the bitmap, in bytes */ bool disabled; /* Bitmap is disabled. It ignores all writes to @@ -183,14 +183,14 @@ const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap) } /* Called with BQL taken. */ -bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap) +bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap) { return bitmap->successor; } /* Both conditions disallow user-modification via QMP. */ bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) { - return bdrv_dirty_bitmap_frozen(bitmap) || + return bdrv_dirty_bitmap_has_successor(bitmap) || bdrv_dirty_bitmap_qmp_locked(bitmap); } @@ -215,7 +215,7 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) /* Called with BQL taken. */ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap) { - if (bdrv_dirty_bitmap_frozen(bitmap)) { + if (bdrv_dirty_bitmap_has_successor(bitmap)) { return DIRTY_BITMAP_STATUS_FROZEN; } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) { return DIRTY_BITMAP_STATUS_LOCKED; @@ -235,7 +235,7 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap) /** * Create a successor bitmap destined to replace this bitmap after an operation. - * Requires that the bitmap is not frozen and has no successor. + * Requires that the bitmap is not locked and has no successor. * Called with BQL taken. */ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, @@ -244,12 +244,16 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, uint64_t granularity; BdrvDirtyBitmap *child; - if (bdrv_dirty_bitmap_frozen(bitmap)) { - error_setg(errp, "Cannot create a successor for a bitmap that is " - "currently frozen"); + if (bdrv_dirty_bitmap_user_locked(bitmap)) { + error_setg(errp, "Cannot create a successor for a bitmap that is in-use " + "by an operation"); + return -1; + } + if (bdrv_dirty_bitmap_has_successor(bitmap)) { + error_setg(errp, "Cannot create a successor for a bitmap that already " + "has one"); return -1; } - assert(!bitmap->successor); /* Create an anonymous successor */ granularity = bdrv_dirty_bitmap_granularity(bitmap); @@ -268,7 +272,6 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, void bdrv_enable_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap) { - assert(!bdrv_dirty_bitmap_frozen(bitmap)); bitmap->disabled = false; } @@ -285,7 +288,7 @@ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap) static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap) { assert(!bitmap->active_iterators); - assert(!bdrv_dirty_bitmap_frozen(bitmap)); + assert(!bdrv_dirty_bitmap_user_locked(bitmap)); assert(!bitmap->meta); QLIST_REMOVE(bitmap, list); hbitmap_free(bitmap->bitmap); @@ -325,7 +328,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, /** * In cases of failure where we can no longer safely delete the parent, * we may wish to re-join the parent and child/successor. - * The merged parent will be un-frozen, but not explicitly re-enabled. + * The merged parent will not be user_locked, nor explicitly re-enabled. * Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken. */ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, @@ -373,7 +376,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes) bdrv_dirty_bitmaps_lock(bs); QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { - assert(!bdrv_dirty_bitmap_frozen(bitmap)); + assert(!bdrv_dirty_bitmap_user_locked(bitmap)); assert(!bitmap->active_iterators); hbitmap_truncate(bitmap->bitmap, bytes); bitmap->size = bytes; @@ -391,7 +394,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) /** * Release all named dirty bitmaps attached to a BDS (for use in bdrv_close()). - * There must not be any frozen bitmaps attached. + * There must not be any user_locked bitmaps attached. * This function does not remove persistent bitmaps from the storage. * Called with BQL taken. */ @@ -428,7 +431,6 @@ void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, void bdrv_disable_dirty_bitmap(BdrvDirtyBitmap *bitmap) { bdrv_dirty_bitmap_lock(bitmap); - assert(!bdrv_dirty_bitmap_frozen(bitmap)); bitmap->disabled = true; bdrv_dirty_bitmap_unlock(bitmap); } diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 04a117fc81..cdbb4dfefd 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -36,7 +36,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs); uint32_t bdrv_get_default_bitmap_granularity(BlockDriverState *bs); uint32_t bdrv_dirty_bitmap_granularity(const BdrvDirtyBitmap *bitmap); bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap); -bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap); +bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap); const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap); int64_t bdrv_dirty_bitmap_size(const BdrvDirtyBitmap *bitmap); DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap); diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index 6426151e4f..ac6954142f 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -542,7 +542,7 @@ static void dirty_bitmap_load_complete(QEMUFile *f, DirtyBitmapLoadState *s) } } - if (bdrv_dirty_bitmap_frozen(s->bitmap)) { + if (bdrv_dirty_bitmap_has_successor(s->bitmap)) { bdrv_dirty_bitmap_lock(s->bitmap); if (enabled_bitmaps == NULL) { /* in postcopy */ -- 2.17.2
On 2/13/19 5:23 PM, John Snow wrote:
"Frozen" was a good description a long time ago, but it isn't adequate now. Rename the frozen predicate to has_successor to make the semantics of the predicate more clear to outside callers.
In the process, remove some calls to frozen() that no longer semantically make sense. For enabled and disabled in particular, it's actually okay for the internals to do this but only forbidden for users to invoke them, and all of the QMP entry uses already check against qmp_locked.
Several other assertions really want to check that the bitmap isn't in-use by another operation -- use the qmp_locked function for this instead, which presently also checks for has_successor.
Signed-off-by: John Snow <jsnow@redhat.com> --- block/dirty-bitmap.c | 32 +++++++++++++++++--------------- include/block/dirty-bitmap.h | 2 +- migration/block-dirty-bitmap.c | 2 +- 3 files changed, 19 insertions(+), 17 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
14.02.2019 2:23, John Snow wrote:
"Frozen" was a good description a long time ago, but it isn't adequate now. Rename the frozen predicate to has_successor to make the semantics of the predicate more clear to outside callers.
In the process, remove some calls to frozen() that no longer semantically make sense. For enabled and disabled in particular, it's actually okay for the internals to do this but only forbidden for users to invoke them, and
I'm a bit lost in this paragraph.. to this - what?, to invoke them - whom? I think, it would be simpler for me to read patch itself :)
all of the QMP entry uses already check against qmp_locked.
Several other assertions really want to check that the bitmap isn't in-use by another operation -- use the qmp_locked function for this instead, which presently also checks for has_successor.
hm, you mean user_locked, not qmp_locked.
Signed-off-by: John Snow <jsnow@redhat.com> --- block/dirty-bitmap.c | 32 +++++++++++++++++--------------- include/block/dirty-bitmap.h | 2 +- migration/block-dirty-bitmap.c | 2 +- 3 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 101383b3af..639ebc0645 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -50,7 +50,7 @@ struct BdrvDirtyBitmap { HBitmap *meta; /* Meta dirty bitmap */ bool qmp_locked; /* Bitmap is locked, it can't be modified through QMP */ - BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */ + BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state */
aha, looks like a good moment to fix preexisting misalignment of the comment, but new line is exactly 80 characters length, so, not a good moment)
char *name; /* Optional non-empty unique ID */ int64_t size; /* Size of the bitmap, in bytes */ bool disabled; /* Bitmap is disabled. It ignores all writes to @@ -183,14 +183,14 @@ const char *bdrv_dirty_bitmap_name(const BdrvDirtyBitmap *bitmap) }
/* Called with BQL taken. */ -bool bdrv_dirty_bitmap_frozen(BdrvDirtyBitmap *bitmap) +bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap) { return bitmap->successor; }
/* Both conditions disallow user-modification via QMP. */ bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) { - return bdrv_dirty_bitmap_frozen(bitmap) || + return bdrv_dirty_bitmap_has_successor(bitmap) || bdrv_dirty_bitmap_qmp_locked(bitmap); }
@@ -215,7 +215,7 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) /* Called with BQL taken. */ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap) { - if (bdrv_dirty_bitmap_frozen(bitmap)) { + if (bdrv_dirty_bitmap_has_successor(bitmap)) { return DIRTY_BITMAP_STATUS_FROZEN; } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) { return DIRTY_BITMAP_STATUS_LOCKED; @@ -235,7 +235,7 @@ static bool bdrv_dirty_bitmap_recording(BdrvDirtyBitmap *bitmap)
/** * Create a successor bitmap destined to replace this bitmap after an operation. - * Requires that the bitmap is not frozen and has no successor. + * Requires that the bitmap is not locked and has no successor.
I think, user_locked, to not interfere with bitmaps mutex. And you use user_locked in other comments in this patch.
* Called with BQL taken. */ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, @@ -244,12 +244,16 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, uint64_t granularity; BdrvDirtyBitmap *child;
- if (bdrv_dirty_bitmap_frozen(bitmap)) { - error_setg(errp, "Cannot create a successor for a bitmap that is " - "currently frozen"); + if (bdrv_dirty_bitmap_user_locked(bitmap)) { + error_setg(errp, "Cannot create a successor for a bitmap that is in-use " + "by an operation"); + return -1; + } + if (bdrv_dirty_bitmap_has_successor(bitmap)) { + error_setg(errp, "Cannot create a successor for a bitmap that already " + "has one");
Amm, dead code? _user_locked() implies no successor, so we instead can keep an assertion..
return -1; } - assert(!bitmap->successor);
/* Create an anonymous successor */ granularity = bdrv_dirty_bitmap_granularity(bitmap);
-- Best regards, Vladimir
18.02.2019 16:57, Vladimir Sementsov-Ogievskiy wrote:
14.02.2019 2:23, John Snow wrote:
"Frozen" was a good description a long time ago, but it isn't adequate now. Rename the frozen predicate to has_successor to make the semantics of the predicate more clear to outside callers.
In the process, remove some calls to frozen() that no longer semantically make sense. For enabled and disabled in particular, it's actually okay for the internals to do this but only forbidden for users to invoke them, and
I'm a bit lost in this paragraph.. to this - what?, to invoke them - whom? I think, it would be simpler for me to read patch itself :)
all of the QMP entry uses already check against qmp_locked.
Several other assertions really want to check that the bitmap isn't in-use by another operation -- use the qmp_locked function for this instead, which presently also checks for has_successor.
hm, you mean user_locked, not qmp_locked.
Signed-off-by: John Snow <jsnow@redhat.com> --- block/dirty-bitmap.c | 32 +++++++++++++++++--------------- include/block/dirty-bitmap.h | 2 +- migration/block-dirty-bitmap.c | 2 +- 3 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 101383b3af..639ebc0645 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -50,7 +50,7 @@ struct BdrvDirtyBitmap { HBitmap *meta; /* Meta dirty bitmap */ bool qmp_locked; /* Bitmap is locked, it can't be modified through QMP */ - BdrvDirtyBitmap *successor; /* Anonymous child; implies frozen status */ + BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state */
aha, looks like a good moment to fix preexisting misalignment of the comment, but new line is exactly 80 characters length, so, not a good moment)
and there was no any misalignment, only thunderbird bug... -- Best regards, Vladimir
On 2/18/19 8:57 AM, Vladimir Sementsov-Ogievskiy wrote:
14.02.2019 2:23, John Snow wrote:
"Frozen" was a good description a long time ago, but it isn't adequate now. Rename the frozen predicate to has_successor to make the semantics of the predicate more clear to outside callers.
In the process, remove some calls to frozen() that no longer semantically make sense. For enabled and disabled in particular, it's actually okay for the internals to do this but only forbidden for users to invoke them, and
I'm a bit lost in this paragraph.. to this - what?, to invoke them - whom? I think, it would be simpler for me to read patch itself :)
Touched this up. I meant enable and disable, not enabled and disabled.
all of the QMP entry uses already check against qmp_locked.
Several other assertions really want to check that the bitmap isn't in-use by another operation -- use the qmp_locked function for this instead, which presently also checks for has_successor.
hm, you mean user_locked, not qmp_locked.
Yes. [...]
/** * Create a successor bitmap destined to replace this bitmap after an operation. - * Requires that the bitmap is not frozen and has no successor. + * Requires that the bitmap is not locked and has no successor.
I think, user_locked, to not interfere with bitmaps mutex. And you use user_locked in other comments in this patch.
You're right. It gets changed again later, but I didn't make this easy to read.
* Called with BQL taken. */ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, @@ -244,12 +244,16 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, uint64_t granularity; BdrvDirtyBitmap *child;
- if (bdrv_dirty_bitmap_frozen(bitmap)) { - error_setg(errp, "Cannot create a successor for a bitmap that is " - "currently frozen"); + if (bdrv_dirty_bitmap_user_locked(bitmap)) { + error_setg(errp, "Cannot create a successor for a bitmap that is in-use " + "by an operation"); + return -1; + } + if (bdrv_dirty_bitmap_has_successor(bitmap)) { + error_setg(errp, "Cannot create a successor for a bitmap that already " + "has one");
Amm, dead code? _user_locked() implies no successor, so we instead can keep an assertion..
It gets changed later in the series, but I didn't do a great job of explaining that in advance. I'll amend the commit message to explain what I'm trying to do. I tried to hint at this with: "which presently also checks for has_successor" as an admission that it was redundant, but I need to call it out in stronger language.
19.02.2019 1:32, John Snow wrote:
On 2/18/19 8:57 AM, Vladimir Sementsov-Ogievskiy wrote:
14.02.2019 2:23, John Snow wrote:
"Frozen" was a good description a long time ago, but it isn't adequate now. Rename the frozen predicate to has_successor to make the semantics of the predicate more clear to outside callers.
In the process, remove some calls to frozen() that no longer semantically make sense. For enabled and disabled in particular, it's actually okay for the internals to do this but only forbidden for users to invoke them, and
I'm a bit lost in this paragraph.. to this - what?, to invoke them - whom? I think, it would be simpler for me to read patch itself :)
Touched this up. I meant enable and disable, not enabled and disabled.
all of the QMP entry uses already check against qmp_locked.
Several other assertions really want to check that the bitmap isn't in-use by another operation -- use the qmp_locked function for this instead, which presently also checks for has_successor.
hm, you mean user_locked, not qmp_locked.
Yes.
[...]
/** * Create a successor bitmap destined to replace this bitmap after an operation. - * Requires that the bitmap is not frozen and has no successor. + * Requires that the bitmap is not locked and has no successor.
I think, user_locked, to not interfere with bitmaps mutex. And you use user_locked in other comments in this patch.
You're right. It gets changed again later, but I didn't make this easy to read.
* Called with BQL taken. */ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, @@ -244,12 +244,16 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, uint64_t granularity; BdrvDirtyBitmap *child;
- if (bdrv_dirty_bitmap_frozen(bitmap)) { - error_setg(errp, "Cannot create a successor for a bitmap that is " - "currently frozen"); + if (bdrv_dirty_bitmap_user_locked(bitmap)) { + error_setg(errp, "Cannot create a successor for a bitmap that is in-use " + "by an operation"); + return -1; + } + if (bdrv_dirty_bitmap_has_successor(bitmap)) { + error_setg(errp, "Cannot create a successor for a bitmap that already " + "has one");
Amm, dead code? _user_locked() implies no successor, so we instead can keep an assertion..
It gets changed later in the series, but I didn't do a great job of explaining that in advance. I'll amend the commit message to explain what I'm trying to do.
I tried to hint at this with: "which presently also checks for has_successor" as an admission that it was redundant, but I need to call it out in stronger language.
Hmm, isn't it better to keep an assertion, than add dead code, to be fixed in later commits? -- Best regards, Vladimir
On 2/19/19 5:17 AM, Vladimir Sementsov-Ogievskiy wrote:
19.02.2019 1:32, John Snow wrote:
On 2/18/19 8:57 AM, Vladimir Sementsov-Ogievskiy wrote:
14.02.2019 2:23, John Snow wrote:
"Frozen" was a good description a long time ago, but it isn't adequate now. Rename the frozen predicate to has_successor to make the semantics of the predicate more clear to outside callers.
In the process, remove some calls to frozen() that no longer semantically make sense. For enabled and disabled in particular, it's actually okay for the internals to do this but only forbidden for users to invoke them, and
I'm a bit lost in this paragraph.. to this - what?, to invoke them - whom? I think, it would be simpler for me to read patch itself :)
Touched this up. I meant enable and disable, not enabled and disabled.
all of the QMP entry uses already check against qmp_locked.
Several other assertions really want to check that the bitmap isn't in-use by another operation -- use the qmp_locked function for this instead, which presently also checks for has_successor.
hm, you mean user_locked, not qmp_locked.
Yes.
[...]
/** * Create a successor bitmap destined to replace this bitmap after an operation. - * Requires that the bitmap is not frozen and has no successor. + * Requires that the bitmap is not locked and has no successor.
I think, user_locked, to not interfere with bitmaps mutex. And you use user_locked in other comments in this patch.
You're right. It gets changed again later, but I didn't make this easy to read.
* Called with BQL taken. */ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, @@ -244,12 +244,16 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, uint64_t granularity; BdrvDirtyBitmap *child;
- if (bdrv_dirty_bitmap_frozen(bitmap)) { - error_setg(errp, "Cannot create a successor for a bitmap that is " - "currently frozen"); + if (bdrv_dirty_bitmap_user_locked(bitmap)) { + error_setg(errp, "Cannot create a successor for a bitmap that is in-use " + "by an operation"); + return -1; + } + if (bdrv_dirty_bitmap_has_successor(bitmap)) { + error_setg(errp, "Cannot create a successor for a bitmap that already " + "has one");
Amm, dead code? _user_locked() implies no successor, so we instead can keep an assertion..
It gets changed later in the series, but I didn't do a great job of explaining that in advance. I'll amend the commit message to explain what I'm trying to do.
I tried to hint at this with: "which presently also checks for has_successor" as an admission that it was redundant, but I need to call it out in stronger language.
Hmm, isn't it better to keep an assertion, than add dead code, to be fixed in later commits?
Eh. I wrote code that looked semantically correct without worrying about what the calls actually do: - We want to make sure this bitmap is not in use (user_locked, qmp_locked, or busy -- however you want to spell it), and - We want to make sure this bitmap doesn't have a successor. Now, you and I happen to know that these two conditions aren't actually independent, but that's not necessarily obvious from this one function to a new reader. Adding the second check gives some assurance to the reader. In my mind, the concept of having a successor is now distinct from that of being busy, and create_successor actually wants to check both things: (1) That is able to create a successor, because it doesn't have one, and (2) That it is not modifying a bitmap in use by some operation. But, you're right, there's no way to have a bitmap with a successor that isn't busy, so an assertion will suffice for the instructional purpose. I'll change it. --js
Currently, enabled means something like "the status of the bitmap is ACTIVE." After this patch, it should mean exclusively: "This bitmap is recording guest writes, and is allowed to do so." In many places, this is how this predicate was already used. We'll allow users to call user_locked if they're really curious about finding out if the bitmap is in use by an operation. To accommodate this, modify the create_successor routine to now explicitly disable the parent bitmap at creation time. Justifications: 1. bdrv_dirty_bitmap_status suffers no change from the lack of 1:1 parity with the new predicates because of the order in which the predicates are checked. This is now only for compatibility. 2. bdrv_set_dirty_bitmap is only used by mirror, which does not use disabled bitmaps -- all of these writes are internal usages. Therefore, we should allow writes even in the disabled state. The condition is removed. 3. bdrv_reset_dirty_bitmap Similarly, this is only used internally by mirror and migration. In these contexts it is always enabled anyway, but our API does not need to enforce this. 4. bdrv_set_dirty() is unchanged: pre-patch, it was skipping bitmaps that were disabled or had a successor, while post-patch it is only skipping bitmaps that are disabled. To accommodate this, create_successor now ensures that any bitmap with a successor is explicitly disabled. 5. qcow2_store_persistent_dirty_bitmaps: This only ever wanted to check if the bitmap was enabled or not. Theoretically if we save during an operation, this now gets set as enabled instead of disabled, but this cannot happen in practice because jobs must be finished before we close the disk. 6. block_dirty_bitmap_enable_prepare only ever cared about the literal bit, and already checked for user_locked beforehand. 7. block_dirty_bitmap_disable_prepare ditto as above. 8. init_dirty_bitmap_migration also already checks user_locked, so this call can be a simple enabled/disabled check. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- block/dirty-bitmap.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 639ebc0645..bb2e19e0d8 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -209,7 +209,7 @@ bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap) /* Called with BQL taken. */ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) { - return !(bitmap->disabled || bitmap->successor); + return !bitmap->disabled; } /* Called with BQL taken. */ @@ -264,6 +264,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, /* Successor will be on or off based on our current state. */ child->disabled = bitmap->disabled; + bitmap->disabled = true; /* Install the successor and freeze the parent */ bitmap->successor = child; @@ -346,6 +347,8 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, error_setg(errp, "Merging of parent and successor bitmap failed"); return NULL; } + + parent->disabled = successor->disabled; bdrv_release_dirty_bitmap_locked(successor); parent->successor = NULL; @@ -542,7 +545,6 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter) void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, int64_t offset, int64_t bytes) { - assert(bdrv_dirty_bitmap_enabled(bitmap)); assert(!bdrv_dirty_bitmap_readonly(bitmap)); hbitmap_set(bitmap->bitmap, offset, bytes); } @@ -559,7 +561,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, int64_t offset, int64_t bytes) { - assert(bdrv_dirty_bitmap_enabled(bitmap)); assert(!bdrv_dirty_bitmap_readonly(bitmap)); hbitmap_reset(bitmap->bitmap, offset, bytes); } -- 2.17.2
14.02.2019 2:23, John Snow wrote:
Currently, enabled means something like "the status of the bitmap is ACTIVE." After this patch, it should mean exclusively: "This bitmap is recording guest writes, and is allowed to do so."
In many places, this is how this predicate was already used. We'll allow users to call user_locked if they're really curious about finding out if the bitmap is in use by an operation.
To accommodate this, modify the create_successor routine to now explicitly disable the parent bitmap at creation time.
Justifications:
1. bdrv_dirty_bitmap_status suffers no change from the lack of 1:1 parity with the new predicates because of the order in which the predicates are checked. This is now only for compatibility.
2. bdrv_set_dirty_bitmap is only used by mirror, which does not use disabled bitmaps -- all of these writes are internal usages. Therefore, we should allow writes even in the disabled state. The condition is removed.
3. bdrv_reset_dirty_bitmap Similarly, this is only used internally by mirror and migration. In these contexts it is always enabled anyway, but our API does not need to enforce this.
4. bdrv_set_dirty() is unchanged: pre-patch, it was skipping bitmaps that were disabled or had a successor, while post-patch it is only skipping bitmaps that are disabled. To accommodate this, create_successor now ensures that any bitmap with a successor is explicitly disabled.
5-8 are examples of "this is how this predicate was already used"
5. qcow2_store_persistent_dirty_bitmaps: This only ever wanted to check if the bitmap was enabled or not. Theoretically if we save during an operation, this now gets set as enabled instead of disabled,
No, as you explicitly disable bitmap in create_successor, so bitmaps with successor will be disabled anyway. Hmm, and this shows, that actually, you don't need this big description for all calls, as actually nothing changed and all calls may be described like (4.). Except (2. and 3.), as these calls are removed (so, is it worth to split them into separate previous patch?) but this cannot happen
in practice because jobs must be finished before we close the disk.
6. block_dirty_bitmap_enable_prepare only ever cared about the literal bit, and already checked for user_locked beforehand.
7. block_dirty_bitmap_disable_prepare ditto as above.
8. init_dirty_bitmap_migration also already checks user_locked, so this call can be a simple enabled/disabled check.
hmmm 9. nbd_export_new, which too checks bdrv_dirty_bitmap_user_locked but _after_ call to bdrv_dirty_bitmap_enabled. Anyway it's not changed as described in (4.), I think it is better to check _user_locked first.
Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- block/dirty-bitmap.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 639ebc0645..bb2e19e0d8 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -209,7 +209,7 @@ bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap) /* Called with BQL taken. */ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) { - return !(bitmap->disabled || bitmap->successor); + return !bitmap->disabled; }
/* Called with BQL taken. */ @@ -264,6 +264,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
/* Successor will be on or off based on our current state. */ child->disabled = bitmap->disabled; + bitmap->disabled = true;
/* Install the successor and freeze the parent */ bitmap->successor = child; @@ -346,6 +347,8 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, error_setg(errp, "Merging of parent and successor bitmap failed"); return NULL; } + + parent->disabled = successor->disabled;
at this point comment to the function "The merged parent will not be user_locked, nor explicitly re-enabled." becomes really weird. Need to reword it somehow..
bdrv_release_dirty_bitmap_locked(successor); parent->successor = NULL;
@@ -542,7 +545,6 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter) void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, int64_t offset, int64_t bytes) { - assert(bdrv_dirty_bitmap_enabled(bitmap)); assert(!bdrv_dirty_bitmap_readonly(bitmap)); hbitmap_set(bitmap->bitmap, offset, bytes); } @@ -559,7 +561,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, int64_t offset, int64_t bytes) { - assert(bdrv_dirty_bitmap_enabled(bitmap)); assert(!bdrv_dirty_bitmap_readonly(bitmap)); hbitmap_reset(bitmap->bitmap, offset, bytes); }
-- Best regards, Vladimir
On 2/18/19 11:39 AM, Vladimir Sementsov-Ogievskiy wrote:
14.02.2019 2:23, John Snow wrote:
Currently, enabled means something like "the status of the bitmap is ACTIVE." After this patch, it should mean exclusively: "This bitmap is recording guest writes, and is allowed to do so."
In many places, this is how this predicate was already used. We'll allow users to call user_locked if they're really curious about finding out if the bitmap is in use by an operation.
To accommodate this, modify the create_successor routine to now explicitly disable the parent bitmap at creation time.
Justifications:
1. bdrv_dirty_bitmap_status suffers no change from the lack of 1:1 parity with the new predicates because of the order in which the predicates are checked. This is now only for compatibility.
2. bdrv_set_dirty_bitmap is only used by mirror, which does not use disabled bitmaps -- all of these writes are internal usages. Therefore, we should allow writes even in the disabled state. The condition is removed.
3. bdrv_reset_dirty_bitmap Similarly, this is only used internally by mirror and migration. In these contexts it is always enabled anyway, but our API does not need to enforce this.
4. bdrv_set_dirty() is unchanged: pre-patch, it was skipping bitmaps that were disabled or had a successor, while post-patch it is only skipping bitmaps that are disabled. To accommodate this, create_successor now ensures that any bitmap with a successor is explicitly disabled.
5-8 are examples of "this is how this predicate was already used"
5. qcow2_store_persistent_dirty_bitmaps: This only ever wanted to check if the bitmap was enabled or not. Theoretically if we save during an operation, this now gets set as enabled instead of disabled,
No, as you explicitly disable bitmap in create_successor, so bitmaps with successor will be disabled anyway.
Well, yeah. There's no way it happens in practice currently. It's just "theoretically" from the viewpoint of the API call itself. There's nothing stopping a developer from making that call, and this is a potential change in behavior that we don't expect to observe. Just noting it down.
Hmm, and this shows, that actually, you don't need this big description for all calls, as actually nothing changed and all calls may be described like (4.). Except (2. and 3.), as these calls are removed (so, is it worth to split them into separate previous patch?)
I could, to at least have its own justification in a commit message apart from these -- but at this point it's primarily a benefit for Eric, You, and myself.
but this cannot happen
in practice because jobs must be finished before we close the disk.
6. block_dirty_bitmap_enable_prepare only ever cared about the literal bit, and already checked for user_locked beforehand.
7. block_dirty_bitmap_disable_prepare ditto as above.
8. init_dirty_bitmap_migration also already checks user_locked, so this call can be a simple enabled/disabled check.
hmmm 9. nbd_export_new, which too checks bdrv_dirty_bitmap_user_locked but _after_ call to bdrv_dirty_bitmap_enabled. Anyway it's not changed as described in (4.), I think it is better to check _user_locked first.
You're right, and Eric left a similar feedback elsewhere. user_locked is the more obvious disqualifier. I think this ought to be its own small patch because it has nothing much to do with this one.
Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- block/dirty-bitmap.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 639ebc0645..bb2e19e0d8 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -209,7 +209,7 @@ bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap) /* Called with BQL taken. */ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) { - return !(bitmap->disabled || bitmap->successor); + return !bitmap->disabled; }
/* Called with BQL taken. */ @@ -264,6 +264,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs,
/* Successor will be on or off based on our current state. */ child->disabled = bitmap->disabled; + bitmap->disabled = true;
/* Install the successor and freeze the parent */ bitmap->successor = child; @@ -346,6 +347,8 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, error_setg(errp, "Merging of parent and successor bitmap failed"); return NULL; } + + parent->disabled = successor->disabled;
at this point comment to the function "The merged parent will not be user_locked, nor explicitly re-enabled." becomes really weird. Need to reword it somehow..
It is a pretty awkward comment. I'll try to touch it up here.
bdrv_release_dirty_bitmap_locked(successor); parent->successor = NULL;
@@ -542,7 +545,6 @@ int64_t bdrv_dirty_iter_next(BdrvDirtyBitmapIter *iter) void bdrv_set_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, int64_t offset, int64_t bytes) { - assert(bdrv_dirty_bitmap_enabled(bitmap)); assert(!bdrv_dirty_bitmap_readonly(bitmap)); hbitmap_set(bitmap->bitmap, offset, bytes); } @@ -559,7 +561,6 @@ void bdrv_set_dirty_bitmap(BdrvDirtyBitmap *bitmap, void bdrv_reset_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap, int64_t offset, int64_t bytes) { - assert(bdrv_dirty_bitmap_enabled(bitmap)); assert(!bdrv_dirty_bitmap_readonly(bitmap)); hbitmap_reset(bitmap->bitmap, offset, bytes); }
Instead of implying a locked status, make it explicit. Now, bitmaps in use by migration, NBD or backup operations are all treated the same way with the same code paths. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- block/dirty-bitmap.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index bb2e19e0d8..2042c62602 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -188,10 +188,8 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap) return bitmap->successor; } -/* Both conditions disallow user-modification via QMP. */ bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) { - return bdrv_dirty_bitmap_has_successor(bitmap) || - bdrv_dirty_bitmap_qmp_locked(bitmap); + return bdrv_dirty_bitmap_qmp_locked(bitmap); } void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked) @@ -266,8 +264,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, child->disabled = bitmap->disabled; bitmap->disabled = true; - /* Install the successor and freeze the parent */ + /* Install the successor and lock the parent */ bitmap->successor = child; + bitmap->qmp_locked = true; return 0; } @@ -321,6 +320,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, bitmap->successor = NULL; successor->persistent = bitmap->persistent; bitmap->persistent = false; + bitmap->qmp_locked = false; bdrv_release_dirty_bitmap(bs, bitmap); return successor; @@ -349,6 +349,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, } parent->disabled = successor->disabled; + parent->qmp_locked = false; bdrv_release_dirty_bitmap_locked(successor); parent->successor = NULL; -- 2.17.2
14.02.2019 2:23, John Snow wrote:
Instead of implying a locked status, make it explicit.
locked interferes with bitmap mutex, so may be better "qmp_locked state", but not sure.
Now, bitmaps in use by migration, NBD or backup operations are all treated the same way with the same code paths.
Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> Hmm. Isn't it better to make successor-related staff unrelated to locking at all? So, backup will call set_qmp_locked like others? And then do create_successor, abdicate, reclaim, whatever it wants, and finally set_qmp_locked(false) ? To make it work even more in the same path. But it may be done separately, if we want.
--- block/dirty-bitmap.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index bb2e19e0d8..2042c62602 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -188,10 +188,8 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap) return bitmap->successor; }
-/* Both conditions disallow user-modification via QMP. */ bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) { - return bdrv_dirty_bitmap_has_successor(bitmap) || - bdrv_dirty_bitmap_qmp_locked(bitmap); + return bdrv_dirty_bitmap_qmp_locked(bitmap); }
void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked) @@ -266,8 +264,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, child->disabled = bitmap->disabled; bitmap->disabled = true;
- /* Install the successor and freeze the parent */ + /* Install the successor and lock the parent */ bitmap->successor = child; + bitmap->qmp_locked = true; return 0; }
@@ -321,6 +320,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, bitmap->successor = NULL; successor->persistent = bitmap->persistent; bitmap->persistent = false; + bitmap->qmp_locked = false; bdrv_release_dirty_bitmap(bs, bitmap);
return successor; @@ -349,6 +349,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, }
parent->disabled = successor->disabled; + parent->qmp_locked = false; bdrv_release_dirty_bitmap_locked(successor); parent->successor = NULL;
-- Best regards, Vladimir
On 2/18/19 11:52 AM, Vladimir Sementsov-Ogievskiy wrote:
14.02.2019 2:23, John Snow wrote:
Instead of implying a locked status, make it explicit.
locked interferes with bitmap mutex, so may be better "qmp_locked state", but not sure.
I agree that "locked" has too many meanings, so in patch 5 I start using the term "busy" instead.
Now, bitmaps in use by migration, NBD or backup operations are all treated the same way with the same code paths.
Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Hmm. Isn't it better to make successor-related staff unrelated to locking at all?
Maybe -- but it doesn't make sense to allow users to modify bitmaps that have a successor because we know it's definitely busy. I'll take a further cleanup patch if you think it's better -- just be careful to make sure that any interface calls will work gracefully with a bitmap with a successor.
So, backup will call set_qmp_locked like others? And then do create_successor, abdicate, reclaim, whatever it wants, and finally set_qmp_locked(false) ? To make it work even more in the same path. But it may be done separately, if we want.
These mean the same thing now. Unify them and rename the merged call bdrv_dirty_bitmap_busy to indicate semantically what we are describing, as well as help disambiguate from the various _locked and _unlocked versions of bitmap helpers that refer to mutex locks. Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- block/dirty-bitmap.c | 41 +++++++++++++++------------------- blockdev.c | 18 +++++++-------- include/block/dirty-bitmap.h | 5 ++--- migration/block-dirty-bitmap.c | 6 ++--- nbd/server.c | 6 ++--- 5 files changed, 35 insertions(+), 41 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 2042c62602..8ab048385a 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -48,9 +48,9 @@ struct BdrvDirtyBitmap { QemuMutex *mutex; HBitmap *bitmap; /* Dirty bitmap implementation */ HBitmap *meta; /* Meta dirty bitmap */ - bool qmp_locked; /* Bitmap is locked, it can't be modified - through QMP */ - BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state */ + bool busy; /* Bitmap is busy, it can't be modified through + QMP */ + BdrvDirtyBitmap *successor; /* Anonymous child, if any. */ char *name; /* Optional non-empty unique ID */ int64_t size; /* Size of the bitmap, in bytes */ bool disabled; /* Bitmap is disabled. It ignores all writes to @@ -188,22 +188,17 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap) return bitmap->successor; } -bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap) { - return bdrv_dirty_bitmap_qmp_locked(bitmap); +bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap) { + return bitmap->busy; } -void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked) +void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy) { qemu_mutex_lock(bitmap->mutex); - bitmap->qmp_locked = qmp_locked; + bitmap->busy = busy; qemu_mutex_unlock(bitmap->mutex); } -bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap) -{ - return bitmap->qmp_locked; -} - /* Called with BQL taken. */ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) { @@ -215,7 +210,7 @@ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap) { if (bdrv_dirty_bitmap_has_successor(bitmap)) { return DIRTY_BITMAP_STATUS_FROZEN; - } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) { + } else if (bdrv_dirty_bitmap_busy(bitmap)) { return DIRTY_BITMAP_STATUS_LOCKED; } else if (!bdrv_dirty_bitmap_enabled(bitmap)) { return DIRTY_BITMAP_STATUS_DISABLED; @@ -242,7 +237,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, uint64_t granularity; BdrvDirtyBitmap *child; - if (bdrv_dirty_bitmap_user_locked(bitmap)) { + if (bdrv_dirty_bitmap_busy(bitmap)) { error_setg(errp, "Cannot create a successor for a bitmap that is in-use " "by an operation"); return -1; @@ -266,7 +261,7 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, /* Install the successor and lock the parent */ bitmap->successor = child; - bitmap->qmp_locked = true; + bitmap->busy = true; return 0; } @@ -288,7 +283,7 @@ void bdrv_dirty_bitmap_enable_successor(BdrvDirtyBitmap *bitmap) static void bdrv_release_dirty_bitmap_locked(BdrvDirtyBitmap *bitmap) { assert(!bitmap->active_iterators); - assert(!bdrv_dirty_bitmap_user_locked(bitmap)); + assert(!bdrv_dirty_bitmap_busy(bitmap)); assert(!bitmap->meta); QLIST_REMOVE(bitmap, list); hbitmap_free(bitmap->bitmap); @@ -320,7 +315,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, bitmap->successor = NULL; successor->persistent = bitmap->persistent; bitmap->persistent = false; - bitmap->qmp_locked = false; + bitmap->busy = false; bdrv_release_dirty_bitmap(bs, bitmap); return successor; @@ -329,7 +324,7 @@ BdrvDirtyBitmap *bdrv_dirty_bitmap_abdicate(BlockDriverState *bs, /** * In cases of failure where we can no longer safely delete the parent, * we may wish to re-join the parent and child/successor. - * The merged parent will not be user_locked, nor explicitly re-enabled. + * The merged parent will not be busy, nor explicitly re-enabled. * Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken. */ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, @@ -349,7 +344,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, } parent->disabled = successor->disabled; - parent->qmp_locked = false; + parent->busy = false; bdrv_release_dirty_bitmap_locked(successor); parent->successor = NULL; @@ -380,7 +375,7 @@ void bdrv_dirty_bitmap_truncate(BlockDriverState *bs, int64_t bytes) bdrv_dirty_bitmaps_lock(bs); QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) { - assert(!bdrv_dirty_bitmap_user_locked(bitmap)); + assert(!bdrv_dirty_bitmap_busy(bitmap)); assert(!bitmap->active_iterators); hbitmap_truncate(bitmap->bitmap, bytes); bitmap->size = bytes; @@ -398,7 +393,7 @@ void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap) /** * Release all named dirty bitmaps attached to a BDS (for use in bdrv_close()). - * There must not be any user_locked bitmaps attached. + * There must not be any busy bitmaps attached. * This function does not remove persistent bitmaps from the storage. * Called with BQL taken. */ @@ -462,7 +457,7 @@ BlockDirtyInfoList *bdrv_query_dirty_bitmaps(BlockDriverState *bs) 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->busy = bdrv_dirty_bitmap_busy(bm); info->persistent = bm->persistent; entry->value = info; *plist = entry; @@ -770,7 +765,7 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, qemu_mutex_lock(dest->mutex); - if (bdrv_dirty_bitmap_user_locked(dest)) { + if (bdrv_dirty_bitmap_busy(dest)) { error_setg(errp, "Bitmap '%s' is currently in use by another" " operation and cannot be modified", dest->name); goto out; diff --git a/blockdev.c b/blockdev.c index fb18e9c975..23a4bf136e 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2008,7 +2008,7 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common, return; } - if (bdrv_dirty_bitmap_user_locked(state->bitmap)) { + if (bdrv_dirty_bitmap_busy(state->bitmap)) { error_setg(errp, "Cannot modify a bitmap in use by another operation"); return; } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) { @@ -2057,7 +2057,7 @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common, return; } - if (bdrv_dirty_bitmap_user_locked(state->bitmap)) { + if (bdrv_dirty_bitmap_busy(state->bitmap)) { error_setg(errp, "Bitmap '%s' is currently in use by another operation" " and cannot be enabled", action->name); @@ -2098,7 +2098,7 @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common, return; } - if (bdrv_dirty_bitmap_user_locked(state->bitmap)) { + if (bdrv_dirty_bitmap_busy(state->bitmap)) { error_setg(errp, "Bitmap '%s' is currently in use by another operation" " and cannot be disabled", action->name); @@ -2884,7 +2884,7 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name, return; } - if (bdrv_dirty_bitmap_user_locked(bitmap)) { + if (bdrv_dirty_bitmap_busy(bitmap)) { error_setg(errp, "Bitmap '%s' is currently in use by another operation and" " cannot be removed", name); @@ -2917,7 +2917,7 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name, return; } - if (bdrv_dirty_bitmap_user_locked(bitmap)) { + if (bdrv_dirty_bitmap_busy(bitmap)) { error_setg(errp, "Bitmap '%s' is currently in use by another operation" " and cannot be cleared", name); @@ -2941,7 +2941,7 @@ void qmp_block_dirty_bitmap_enable(const char *node, const char *name, return; } - if (bdrv_dirty_bitmap_user_locked(bitmap)) { + if (bdrv_dirty_bitmap_busy(bitmap)) { error_setg(errp, "Bitmap '%s' is currently in use by another operation" " and cannot be enabled", name); @@ -2962,7 +2962,7 @@ void qmp_block_dirty_bitmap_disable(const char *node, const char *name, return; } - if (bdrv_dirty_bitmap_user_locked(bitmap)) { + if (bdrv_dirty_bitmap_busy(bitmap)) { error_setg(errp, "Bitmap '%s' is currently in use by another operation" " and cannot be disabled", name); @@ -3538,7 +3538,7 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, bdrv_unref(target_bs); goto out; } - if (bdrv_dirty_bitmap_user_locked(bmap)) { + if (bdrv_dirty_bitmap_busy(bmap)) { error_setg(errp, "Bitmap '%s' is currently in use by another operation" " and cannot be used for backup", backup->bitmap); @@ -3651,7 +3651,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn, error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap); goto out; } - if (bdrv_dirty_bitmap_user_locked(bmap)) { + if (bdrv_dirty_bitmap_busy(bmap)) { error_setg(errp, "Bitmap '%s' is currently in use by another operation" " and cannot be used for backup", backup->bitmap); diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index cdbb4dfefd..ba8477b73f 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -68,7 +68,7 @@ void bdrv_dirty_bitmap_deserialize_finish(BdrvDirtyBitmap *bitmap); void bdrv_dirty_bitmap_set_readonly(BdrvDirtyBitmap *bitmap, bool value); void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent); -void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked); +void bdrv_dirty_bitmap_set_busy(BdrvDirtyBitmap *bitmap, bool busy); void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, HBitmap **backup, Error **errp); void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration); @@ -91,8 +91,7 @@ bool bdrv_dirty_bitmap_readonly(const BdrvDirtyBitmap *bitmap); bool bdrv_has_readonly_bitmaps(BlockDriverState *bs); bool bdrv_dirty_bitmap_get_autoload(const BdrvDirtyBitmap *bitmap); bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap); -bool bdrv_dirty_bitmap_qmp_locked(BdrvDirtyBitmap *bitmap); -bool bdrv_dirty_bitmap_user_locked(BdrvDirtyBitmap *bitmap); +bool bdrv_dirty_bitmap_busy(BdrvDirtyBitmap *bitmap); bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs); BdrvDirtyBitmap *bdrv_dirty_bitmap_next(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index ac6954142f..7057fff242 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -261,7 +261,7 @@ static void dirty_bitmap_mig_cleanup(void) while ((dbms = QSIMPLEQ_FIRST(&dirty_bitmap_mig_state.dbms_list)) != NULL) { QSIMPLEQ_REMOVE_HEAD(&dirty_bitmap_mig_state.dbms_list, entry); - bdrv_dirty_bitmap_set_qmp_locked(dbms->bitmap, false); + bdrv_dirty_bitmap_set_busy(dbms->bitmap, false); bdrv_unref(dbms->bs); g_free(dbms); } @@ -301,7 +301,7 @@ static int init_dirty_bitmap_migration(void) goto fail; } - if (bdrv_dirty_bitmap_user_locked(bitmap)) { + if (bdrv_dirty_bitmap_busy(bitmap)) { error_report("Can't migrate a bitmap that is in use by another operation: '%s'", bdrv_dirty_bitmap_name(bitmap)); goto fail; @@ -314,7 +314,7 @@ static int init_dirty_bitmap_migration(void) } bdrv_ref(bs); - bdrv_dirty_bitmap_set_qmp_locked(bitmap, true); + bdrv_dirty_bitmap_set_busy(bitmap, true); dbms = g_new0(DirtyBitmapMigBitmapState, 1); dbms->bs = bs; diff --git a/nbd/server.c b/nbd/server.c index 0910d09a6d..5021239f7d 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1518,12 +1518,12 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, goto fail; } - if (bdrv_dirty_bitmap_user_locked(bm)) { + if (bdrv_dirty_bitmap_busy(bm)) { error_setg(errp, "Bitmap '%s' is in use", bitmap); goto fail; } - bdrv_dirty_bitmap_set_qmp_locked(bm, true); + bdrv_dirty_bitmap_set_busy(bm, true); exp->export_bitmap = bm; exp->export_bitmap_context = g_strdup_printf("qemu:dirty-bitmap:%s", bitmap); @@ -1641,7 +1641,7 @@ void nbd_export_put(NBDExport *exp) } if (exp->export_bitmap) { - bdrv_dirty_bitmap_set_qmp_locked(exp->export_bitmap, false); + bdrv_dirty_bitmap_set_busy(exp->export_bitmap, false); g_free(exp->export_bitmap_context); } -- 2.17.2
14.02.2019 2:23, John Snow wrote:
These mean the same thing now. Unify them and rename the merged call bdrv_dirty_bitmap_busy to indicate semantically what we are describing, as well as help disambiguate from the various _locked and _unlocked versions of bitmap helpers that refer to mutex locks.
Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- block/dirty-bitmap.c | 41 +++++++++++++++------------------- blockdev.c | 18 +++++++-------- include/block/dirty-bitmap.h | 5 ++--- migration/block-dirty-bitmap.c | 6 ++--- nbd/server.c | 6 ++--- 5 files changed, 35 insertions(+), 41 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 2042c62602..8ab048385a 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -48,9 +48,9 @@ struct BdrvDirtyBitmap { QemuMutex *mutex; HBitmap *bitmap; /* Dirty bitmap implementation */ HBitmap *meta; /* Meta dirty bitmap */ - bool qmp_locked; /* Bitmap is locked, it can't be modified - through QMP */ - BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state */ + bool busy; /* Bitmap is busy, it can't be modified through + QMP */
better not "modified" but "used".. for example, export through NBD is not a modification.
+ BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
hm this comment change about successor relates more to previous patch, but I don't really care.
char *name; /* Optional non-empty unique ID */ int64_t size; /* Size of the bitmap, in bytes */ bool disabled; /* Bitmap is disabled. It ignores all writes to @@ -188,22 +188,17 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap) return bitmap->successor; }
In comment for bdrv_dirty_bitmap_create_successor, there is "locked" word, which you forget to fix to "busy" with at least this fixed: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> -- Best regards, Vladimir
On 2/18/19 12:27 PM, Vladimir Sementsov-Ogievskiy wrote:
14.02.2019 2:23, John Snow wrote:
These mean the same thing now. Unify them and rename the merged call bdrv_dirty_bitmap_busy to indicate semantically what we are describing, as well as help disambiguate from the various _locked and _unlocked versions of bitmap helpers that refer to mutex locks.
Signed-off-by: John Snow <jsnow@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- block/dirty-bitmap.c | 41 +++++++++++++++------------------- blockdev.c | 18 +++++++-------- include/block/dirty-bitmap.h | 5 ++--- migration/block-dirty-bitmap.c | 6 ++--- nbd/server.c | 6 ++--- 5 files changed, 35 insertions(+), 41 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 2042c62602..8ab048385a 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -48,9 +48,9 @@ struct BdrvDirtyBitmap { QemuMutex *mutex; HBitmap *bitmap; /* Dirty bitmap implementation */ HBitmap *meta; /* Meta dirty bitmap */ - bool qmp_locked; /* Bitmap is locked, it can't be modified - through QMP */ - BdrvDirtyBitmap *successor; /* Anonymous child; implies user_locked state */ + bool busy; /* Bitmap is busy, it can't be modified through + QMP */
better not "modified" but "used".. for example, export through NBD is not a modification.
True.
+ BdrvDirtyBitmap *successor; /* Anonymous child, if any. */
hm this comment change about successor relates more to previous patch, but I don't really care.
Oh, true.
char *name; /* Optional non-empty unique ID */ int64_t size; /* Size of the bitmap, in bytes */ bool disabled; /* Bitmap is disabled. It ignores all writes to @@ -188,22 +188,17 @@ bool bdrv_dirty_bitmap_has_successor(BdrvDirtyBitmap *bitmap) return bitmap->successor; }
In comment for bdrv_dirty_bitmap_create_successor, there is "locked" word, which you forget to fix to "busy" with at least this fixed:
Good spot. Too many occurrences of the word "lock" to have looked for them all.
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Simply move the big status enum comment block to above the status function, and document it as being deprecated. The whole confusing block can get deleted in three releases time. Signed-off-by: John Snow <jsnow@redhat.com> --- block/dirty-bitmap.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 8ab048385a..fc390cae94 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -28,22 +28,6 @@ #include "block/block_int.h" #include "block/blockjob.h" -/** - * 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 anonymous successor bitmap may be either - * Active and recording writes from the guest (e.g. backup jobs), - * but it can be Disabled and not recording writes. - * (4) Locked: Whether Active or Disabled, the user cannot modify this bitmap - * in any way from the monitor. - */ struct BdrvDirtyBitmap { QemuMutex *mutex; HBitmap *bitmap; /* Dirty bitmap implementation */ @@ -205,7 +189,25 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) return !bitmap->disabled; } -/* Called with BQL taken. */ +/** + * bdrv_dirty_bitmap_status: This API is now deprecated. + * Called with BQL taken. + * + * 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 anonymous successor bitmap may be either + * Active and recording writes from the guest (e.g. backup jobs), + * but it can be Disabled and not recording writes. + * (4) Locked: Whether Active or Disabled, the user cannot modify this bitmap + * in any way from the monitor. + */ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap) { if (bdrv_dirty_bitmap_has_successor(bitmap)) { -- 2.17.2
On 2/13/19 5:23 PM, John Snow wrote:
Simply move the big status enum comment block to above the status function, and document it as being deprecated. The whole confusing block can get deleted in three releases time.
Signed-off-by: John Snow <jsnow@redhat.com> --- block/dirty-bitmap.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-)
-/* Called with BQL taken. */ +/** + * bdrv_dirty_bitmap_status: This API is now deprecated. + * Called with BQL taken. + * + * 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 anonymous successor bitmap may be either + * Active and recording writes from the guest (e.g. backup jobs), + * but it can be Disabled and not recording writes.
Pre-existing due to code motion, but you could improve the grammar with s/but/or/
+ * (4) Locked: Whether Active or Disabled, the user cannot modify this bitmap + * in any way from the monitor. + */
Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
14.02.2019 2:23, John Snow wrote:
Simply move the big status enum comment block to above the status function, and document it as being deprecated. The whole confusing block can get deleted in three releases time.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> But I think, it's OK to remove it now. It mostly unrelated to code now, so, is it needed? And it is unrelated to deprecation. As I understand, user-seen information is DirtyBitmapStatus declaration in qapi, and it is deprecated and to be removed in three releases.
--- block/dirty-bitmap.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-)
diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 8ab048385a..fc390cae94 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -28,22 +28,6 @@ #include "block/block_int.h" #include "block/blockjob.h"
-/** - * 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 anonymous successor bitmap may be either - * Active and recording writes from the guest (e.g. backup jobs), - * but it can be Disabled and not recording writes. - * (4) Locked: Whether Active or Disabled, the user cannot modify this bitmap - * in any way from the monitor. - */ struct BdrvDirtyBitmap { QemuMutex *mutex; HBitmap *bitmap; /* Dirty bitmap implementation */ @@ -205,7 +189,25 @@ bool bdrv_dirty_bitmap_enabled(BdrvDirtyBitmap *bitmap) return !bitmap->disabled; }
-/* Called with BQL taken. */ +/** + * bdrv_dirty_bitmap_status: This API is now deprecated. + * Called with BQL taken. + * + * 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 anonymous successor bitmap may be either + * Active and recording writes from the guest (e.g. backup jobs), + * but it can be Disabled and not recording writes. + * (4) Locked: Whether Active or Disabled, the user cannot modify this bitmap + * in any way from the monitor. + */ DirtyBitmapStatus bdrv_dirty_bitmap_status(BdrvDirtyBitmap *bitmap) { if (bdrv_dirty_bitmap_has_successor(bitmap)) {
-- Best regards, Vladimir
On 2/18/19 12:39 PM, Vladimir Sementsov-Ogievskiy wrote:
14.02.2019 2:23, John Snow wrote:
Simply move the big status enum comment block to above the status function, and document it as being deprecated. The whole confusing block can get deleted in three releases time.
Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
But I think, it's OK to remove it now. It mostly unrelated to code now, so, is it needed? And it is unrelated to deprecation. As I understand, user-seen information is DirtyBitmapStatus declaration in qapi, and it is deprecated and to be removed in three releases.
I decided to keep it because I wanted to document the particulars of what the fields meant internally before we finally remove it some future release, in case we need to change this function around to maintain backwards compatibility. It'll get removed at that point in time. --js
participants (3)
-
Eric Blake -
John Snow -
Vladimir Sementsov-Ogievskiy