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(a)redhat.com&gt; --- 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 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/25/19 9:01 AM, Vladimir Sementsov-Ogievskiy wrote: 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

On 2/25/19 2:01 AM, Vladimir Sementsov-Ogievskiy wrote: Reasonable. Hm, we should add this to the .gitpublish profile, but yes, let me re-add this. Thanks!

Currently, the enabled predicate means something like: "the QAPI 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. Internal usages of the bitmap QPI can call user_locked to find 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() 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. 3. qcow2_store_persistent_dirty_bitmaps: No functional change. This function cares only about the literal enabled bit, and makes no effort to check if the bitmap is in-use or not. After this patch there are still no ways to produce an enabled bitmap with a successor. 4. block_dirty_bitmap_enable_prepare block_dirty_bitmap_disable_prepare init_dirty_bitmap_migration nbd_export_new These functions care about the literal enabled bit, and already check user_locked separately. Signed-off-by: John Snow <jsnow(a)redhat.com&gt; --- block/dirty-bitmap.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 9ea5738420..976831e765 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. */ @@ -236,6 +236,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 user_locked and has no successor. + * The successor will be enabled if the parent bitmap was. * Called with BQL taken. */ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, @@ -264,6 +265,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; @@ -328,7 +330,8 @@ 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 user_locked. + * The marged parent will be enabled if and only if the successor was enabled. * Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken. */ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, @@ -346,6 +349,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; -- 2.17.2

Instead of implying a user_locked/busy 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(a)redhat.com&gt; Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; --- block/dirty-bitmap.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 976831e765..d92a269753 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 user_locked state */ + 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,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) @@ -267,8 +265,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; } @@ -322,6 +321,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; @@ -351,6 +351,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

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(a)redhat.com&gt; --- block/dirty-bitmap.c | 40 +++++++++++++++------------------- blockdev.c | 18 +++++++-------- include/block/dirty-bitmap.h | 5 ++--- migration/block-dirty-bitmap.c | 6 ++--- nbd/server.c | 6 ++--- 5 files changed, 34 insertions(+), 41 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index d92a269753..b3627b0d8c 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -48,8 +48,7 @@ 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 */ + bool busy; /* Bitmap is busy, it can't be used via QMP */ BdrvDirtyBitmap *successor; /* Anonymous child, if any. */ char *name; /* Optional non-empty unique ID */ int64_t size; /* Size of the bitmap, in bytes */ @@ -188,22 +187,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 +209,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; @@ -243,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; @@ -265,9 +259,9 @@ int bdrv_dirty_bitmap_create_successor(BlockDriverState *bs, child->disabled = bitmap->disabled; bitmap->disabled = true; - /* Install the successor and lock the parent */ + /* Install the successor and mark the parent as busy */ bitmap->successor = child; - bitmap->qmp_locked = true; + bitmap->busy = true; return 0; } @@ -289,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); @@ -321,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; @@ -330,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. + * The merged parent will be marked as not busy. * The marged parent will be enabled if and only if the successor was enabled. * Called within bdrv_dirty_bitmap_lock..unlock and with BQL taken. */ @@ -351,7 +345,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; @@ -382,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_user_locked(bitmap)); + assert(!bdrv_dirty_bitmap_busy(bitmap)); assert(!bitmap->active_iterators); hbitmap_truncate(bitmap->bitmap, bytes); bitmap->size = bytes; @@ -400,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 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. */ @@ -464,7 +458,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; @@ -772,7 +766,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 8714ad2702..1aaadb6128 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); @@ -2892,7 +2892,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); @@ -2931,7 +2931,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); @@ -2955,7 +2955,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); @@ -2976,7 +2976,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); @@ -3552,7 +3552,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); @@ -3665,7 +3665,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 de21c64ce3..02773e2836 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -1510,7 +1510,7 @@ 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; } @@ -1523,7 +1523,7 @@ NBDExport *nbd_export_new(BlockDriverState *bs, uint64_t dev_offset, 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

23.02.2019 3:06, John Snow wrote: Hmm, and here, you directly fixed what I've noticed, so my r-b, of course, still applies. Ha, but I noticed funny thing: [..] I asked you to fix comment for bdrv_dirty_bitmap_create_successor(), but I didn't noticed this one, I meant the comment to the whole function, placed above it, which now have "Requires that the bitmap is not user_locked and has no successor." So, it should be updated too. and with it: Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov(a)virtuozzo.com&gt; -- Best regards, Vladimir

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(a)redhat.com&gt; --- 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 b3627b0d8c..86c3b87ab9 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 */ @@ -204,7 +188,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), + * or 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/22/19 6:06 PM, John Snow wrote: Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org

This adds a simple test that ensures the busy bit works for push backups, as well as doubling as bonus test for incremental backups that get interrupted by EIO errors. Recording bit tests are already handled sufficiently by 236. Signed-off-by: John Snow <jsnow(a)redhat.com&gt; --- tests/qemu-iotests/124 | 110 +++++++++++++++++++++++++++++++++++++ tests/qemu-iotests/124.out | 4 +- 2 files changed, 112 insertions(+), 2 deletions(-) diff --git a/tests/qemu-iotests/124 b/tests/qemu-iotests/124 index 5aa1bf1bd6..30f12a2202 100755 --- a/tests/qemu-iotests/124 +++ b/tests/qemu-iotests/124 @@ -634,6 +634,116 @@ class TestIncrementalBackupBlkdebug(TestIncrementalBackupBase): self.vm.shutdown() self.check_backups() + def test_incremental_pause(self): + """ + Test an incremental backup that errors into a pause and is resumed. + """ + + drive0 = self.drives[0] + result = self.vm.qmp('blockdev-add', + node_name=drive0['id'], + driver=drive0['fmt'], + file={ + 'driver': 'blkdebug', + 'image': { + 'driver': 'file', + 'filename': drive0['file'] + }, + 'set-state': [{ + 'event': 'flush_to_disk', + 'state': 1, + 'new_state': 2 + },{ + 'event': 'read_aio', + 'state': 2, + 'new_state': 3 + }], + 'inject-error': [{ + 'event': 'read_aio', + 'errno': 5, + 'state': 3, + 'immediately': False, + 'once': True + }], + }) + self.assert_qmp(result, 'return', {}) + self.create_anchor_backup(drive0) + bitmap = self.add_bitmap('bitmap0', drive0) + + # Emulate guest activity + self.hmp_io_writes(drive0['id'], (('0xab', 0, 512), + ('0xfe', '16M', '256k'), + ('0x64', '32736k', '64k'))) + + # For the purposes of query-block visibility of bitmaps, add a drive + # frontend after we've written data; otherwise we can't use hmp-io + result = self.vm.qmp("device_add", + id="device0", + drive=drive0['id'], + driver="virtio-blk") + self.assert_qmp(result, 'return', {}) + + # Bitmap Status Check + query = self.vm.qmp('query-block') + ret = [bmap for bmap in query['return'][0]['dirty-bitmaps'] + if bmap.get('name') == bitmap.name][0] + self.assert_qmp(ret, 'count', 458752) + self.assert_qmp(ret, 'granularity', 65536) + self.assert_qmp(ret, 'status', 'active') + self.assert_qmp(ret, 'busy', False) + self.assert_qmp(ret, 'recording', True) + + # Start backup + parent, _ = bitmap.last_target() + target = self.prepare_backup(bitmap, parent) + res = self.vm.qmp('drive-backup', + job_id=bitmap.drive['id'], + device=bitmap.drive['id'], + sync='incremental', + bitmap=bitmap.name, + format=bitmap.drive['fmt'], + target=target, + mode='existing', + on_source_error='stop') + self.assert_qmp(res, 'return', {}) + + # Wait for the error + event = self.vm.event_wait(name="BLOCK_JOB_ERROR", + match={"data":{"device":bitmap.drive['id']}}) + self.assert_qmp(event, 'data', {'device': bitmap.drive['id'], + 'action': 'stop', + 'operation': 'read'}) + + # Bitmap Status Check + query = self.vm.qmp('query-block') + ret = [bmap for bmap in query['return'][0]['dirty-bitmaps'] + if bmap.get('name') == bitmap.name][0] + self.assert_qmp(ret, 'count', 458752) + self.assert_qmp(ret, 'granularity', 65536) + self.assert_qmp(ret, 'status', 'frozen') + self.assert_qmp(ret, 'busy', True) + self.assert_qmp(ret, 'recording', True) + + # Resume and check incremental backup for consistency + res = self.vm.qmp('block-job-resume', device=bitmap.drive['id']) + self.assert_qmp(res, 'return', {}) + self.wait_qmp_backup(bitmap.drive['id']) + + # Bitmap Status Check + query = self.vm.qmp('query-block') + ret = [bmap for bmap in query['return'][0]['dirty-bitmaps'] + if bmap.get('name') == bitmap.name][0] + self.assert_qmp(ret, 'count', 0) + self.assert_qmp(ret, 'granularity', 65536) + self.assert_qmp(ret, 'status', 'active') + self.assert_qmp(ret, 'busy', False) + self.assert_qmp(ret, 'recording', True) + + # Finalize / Cleanup + self.make_reference_backup(bitmap) + self.vm.shutdown() + self.check_backups() + if __name__ == '__main__': iotests.main(supported_fmts=['qcow2']) diff --git a/tests/qemu-iotests/124.out b/tests/qemu-iotests/124.out index e56cae021b..281b69efea 100644 --- a/tests/qemu-iotests/124.out +++ b/tests/qemu-iotests/124.out @@ -1,5 +1,5 @@ -........... +............ ---------------------------------------------------------------------- -Ran 11 tests +Ran 12 tests OK -- 2.17.2

On 2/23/19 5:06 PM, Eric Blake wrote: The problem in this case was that the drive_add itself provokes a read_aio. I didn't dig too far to see why, but I needed an extra read_aio state in the sequence there so that the error would occur during the actual backup job. I think I'll document this in the test, with at least a comment. I wonder if there's a syntactical sugar we could write that makes this easier to read, like: 'inject-error': [{ 'event': 'flush_to_disk->read_aio->read_aio' }] or some such, but that's probably at odds with the existing interface and not worth my time to go on a crusade about. It also tests that you can query a bitmap during an error-paused backup job which is new. And that the job can successfully finish, the bitmap is correct, etc. It does not, sadly, test further guest writes because of the thorny issue with the drive frontend prohibiting writes from HMP. Thanks!