From 945c1ee0cb7d29f2fd0fece2cd2b5329802de5e9 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 29 Oct 2018 16:23:14 -0400 Subject: [PATCH 01/19] blockdev-backup: add bitmap argument It is only an oversight that we don't allow incremental backup with blockdev-backup. Add the bitmap argument which enables this. Signed-off-by: John Snow Message-id: 20180830211605.13683-2-jsnow@redhat.com Signed-off-by: John Snow --- blockdev.c | 18 +++++++++++++++++- qapi/block-core.json | 7 ++++++- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/blockdev.c b/blockdev.c index 574adbcb7f..1869f9aaf3 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3545,6 +3545,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn, BlockDriverState *bs; BlockDriverState *target_bs; Error *local_err = NULL; + BdrvDirtyBitmap *bmap = NULL; AioContext *aio_context; BlockJob *job = NULL; int job_flags = JOB_DEFAULT; @@ -3595,6 +3596,21 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn, goto out; } } + + if (backup->has_bitmap) { + bmap = bdrv_find_dirty_bitmap(bs, backup->bitmap); + if (!bmap) { + error_setg(errp, "Bitmap '%s' could not be found", backup->bitmap); + goto out; + } + if (bdrv_dirty_bitmap_qmp_locked(bmap)) { + error_setg(errp, + "Bitmap '%s' is currently locked and cannot be used for " + "backup", backup->bitmap); + goto out; + } + } + if (!backup->auto_finalize) { job_flags |= JOB_MANUAL_FINALIZE; } @@ -3602,7 +3618,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn, job_flags |= JOB_MANUAL_DISMISS; } job = backup_job_create(backup->job_id, bs, target_bs, backup->speed, - backup->sync, NULL, backup->compress, + backup->sync, bmap, backup->compress, backup->on_source_error, backup->on_target_error, job_flags, NULL, NULL, txn, &local_err); if (local_err != NULL) { diff --git a/qapi/block-core.json b/qapi/block-core.json index cfb37f8c1d..0fc1590c1b 100644 --- a/qapi/block-core.json +++ b/qapi/block-core.json @@ -1316,6 +1316,10 @@ # @speed: the maximum speed, in bytes per second. The default is 0, # for unlimited. # +# @bitmap: the name of dirty bitmap if sync is "incremental". +# Must be present if sync is "incremental", must NOT be present +# otherwise. (Since 3.1) +# # @compress: true to compress data, if the target format supports it. # (default: false) (since 2.8) # @@ -1348,7 +1352,8 @@ ## { 'struct': 'BlockdevBackup', 'data': { '*job-id': 'str', 'device': 'str', 'target': 'str', - 'sync': 'MirrorSyncMode', '*speed': 'int', '*compress': 'bool', + 'sync': 'MirrorSyncMode', '*speed': 'int', + '*bitmap': 'str', '*compress': 'bool', '*on-source-error': 'BlockdevOnError', '*on-target-error': 'BlockdevOnError', '*auto-finalize': 'bool', '*auto-dismiss': 'bool' } } From 06bf50068a7e952afff8c4f6470ec54a712570f7 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Mon, 29 Oct 2018 16:23:14 -0400 Subject: [PATCH 02/19] dirty-bitmap: switch assert-fails to errors in bdrv_merge_dirty_bitmap Move checks from qmp_x_block_dirty_bitmap_merge() to bdrv_merge_dirty_bitmap(), to share them with dirty bitmap merge transaction action in future commit. Note: for now, only qmp_x_block_dirty_bitmap_merge() calls bdrv_merge_dirty_bitmap(). Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: John Snow --- block/dirty-bitmap.c | 15 +++++++++++++-- blockdev.c | 10 ---------- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index c9b8a6fd52..6c8761e027 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -798,12 +798,23 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, qemu_mutex_lock(dest->mutex); - assert(bdrv_dirty_bitmap_enabled(dest)); - assert(!bdrv_dirty_bitmap_readonly(dest)); + if (bdrv_dirty_bitmap_frozen(dest)) { + error_setg(errp, "Bitmap '%s' is frozen and cannot be modified", + dest->name); + goto out; + } + + if (bdrv_dirty_bitmap_readonly(dest)) { + error_setg(errp, "Bitmap '%s' is readonly and cannot be modified", + dest->name); + goto out; + } if (!hbitmap_merge(dest->bitmap, src->bitmap)) { error_setg(errp, "Bitmaps are incompatible and can't be merged"); + goto out; } +out: qemu_mutex_unlock(dest->mutex); } diff --git a/blockdev.c b/blockdev.c index 1869f9aaf3..e3398e141a 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2962,16 +2962,6 @@ void qmp_x_block_dirty_bitmap_merge(const char *node, const char *dst_name, return; } - if (bdrv_dirty_bitmap_frozen(dst)) { - error_setg(errp, "Bitmap '%s' is frozen and cannot be modified", - dst_name); - return; - } else if (bdrv_dirty_bitmap_readonly(dst)) { - error_setg(errp, "Bitmap '%s' is readonly and cannot be modified", - dst_name); - return; - } - src = bdrv_find_dirty_bitmap(bs, src_name); if (!src) { error_setg(errp, "Dirty bitmap '%s' not found", src_name); From 56bd662497259400b7c9f155aaebaddde4450028 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Mon, 29 Oct 2018 16:23:14 -0400 Subject: [PATCH 03/19] dirty-bitmap: rename bdrv_undo_clear_dirty_bitmap Use more generic names to reuse the function for bitmap merge in the following commit. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: John Snow --- block/dirty-bitmap.c | 4 ++-- blockdev.c | 2 +- include/block/block_int.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 6c8761e027..017ee9db46 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -633,12 +633,12 @@ void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) bdrv_dirty_bitmap_unlock(bitmap); } -void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in) +void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup) { HBitmap *tmp = bitmap->bitmap; assert(bdrv_dirty_bitmap_enabled(bitmap)); assert(!bdrv_dirty_bitmap_readonly(bitmap)); - bitmap->bitmap = in; + bitmap->bitmap = backup; hbitmap_free(tmp); } diff --git a/blockdev.c b/blockdev.c index e3398e141a..598ff87519 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2033,7 +2033,7 @@ static void block_dirty_bitmap_clear_abort(BlkActionState *common) common, common); if (state->backup) { - bdrv_undo_clear_dirty_bitmap(state->bitmap, state->backup); + bdrv_restore_dirty_bitmap(state->bitmap, state->backup); } } diff --git a/include/block/block_int.h b/include/block/block_int.h index 92ecbd866e..f605622216 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -1155,7 +1155,7 @@ bool blk_dev_is_medium_locked(BlockBackend *blk); void bdrv_set_dirty(BlockDriverState *bs, int64_t offset, int64_t bytes); void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out); -void bdrv_undo_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *in); +void bdrv_restore_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap *backup); void bdrv_inc_in_flight(BlockDriverState *bs); void bdrv_dec_in_flight(BlockDriverState *bs); From fa000f2f9fd96a75a0a33d50ead247fce11da92a Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Mon, 29 Oct 2018 16:23:15 -0400 Subject: [PATCH 04/19] dirty-bitmap: make it possible to restore bitmap after merge Add backup parameter to bdrv_merge_dirty_bitmap() to be used then with bdrv_restore_dirty_bitmap() if it needed to restore the bitmap after merge operation. This is needed to implement bitmap merge transaction action in further commit. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: John Snow --- block/dirty-bitmap.c | 17 ++++++++++++++--- blockdev.c | 2 +- include/block/dirty-bitmap.h | 2 +- include/qemu/hbitmap.h | 23 +++++++++++++++-------- util/hbitmap.c | 11 ++++++++--- 5 files changed, 39 insertions(+), 16 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 017ee9db46..8ac933cf1c 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -314,7 +314,7 @@ BdrvDirtyBitmap *bdrv_reclaim_dirty_bitmap_locked(BlockDriverState *bs, return NULL; } - if (!hbitmap_merge(parent->bitmap, successor->bitmap)) { + if (!hbitmap_merge(parent->bitmap, successor->bitmap, parent->bitmap)) { error_setg(errp, "Merging of parent and successor bitmap failed"); return NULL; } @@ -791,8 +791,10 @@ int64_t bdrv_dirty_bitmap_next_zero(BdrvDirtyBitmap *bitmap, uint64_t offset) } void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, - Error **errp) + HBitmap **backup, Error **errp) { + bool ret; + /* only bitmaps from one bds are supported */ assert(dest->mutex == src->mutex); @@ -810,11 +812,20 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, goto out; } - if (!hbitmap_merge(dest->bitmap, src->bitmap)) { + if (!hbitmap_can_merge(dest->bitmap, src->bitmap)) { error_setg(errp, "Bitmaps are incompatible and can't be merged"); goto out; } + if (backup) { + *backup = dest->bitmap; + dest->bitmap = hbitmap_alloc(dest->size, hbitmap_granularity(*backup)); + ret = hbitmap_merge(*backup, src->bitmap, dest->bitmap); + } else { + ret = hbitmap_merge(dest->bitmap, src->bitmap, dest->bitmap); + } + assert(ret); + out: qemu_mutex_unlock(dest->mutex); } diff --git a/blockdev.c b/blockdev.c index 598ff87519..b8a854fba5 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2968,7 +2968,7 @@ void qmp_x_block_dirty_bitmap_merge(const char *node, const char *dst_name, return; } - bdrv_merge_dirty_bitmap(dst, src, errp); + bdrv_merge_dirty_bitmap(dst, src, NULL, errp); } BlockDirtyBitmapSha256 *qmp_x_debug_block_dirty_bitmap_sha256(const char *node, diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 259bd27c40..201ff7f20b 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -71,7 +71,7 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent); void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked); void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, - Error **errp); + HBitmap **backup, Error **errp); /* Functions that require manual locking. */ void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap); diff --git a/include/qemu/hbitmap.h b/include/qemu/hbitmap.h index ddca52c48e..a7cb780592 100644 --- a/include/qemu/hbitmap.h +++ b/include/qemu/hbitmap.h @@ -73,16 +73,23 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size); /** * hbitmap_merge: - * @a: The bitmap to store the result in. - * @b: The bitmap to merge into @a. - * @return true if the merge was successful, - * false if it was not attempted. * - * Merge two bitmaps together. - * A := A (BITOR) B. - * B is left unmodified. + * Store result of merging @a and @b into @result. + * @result is allowed to be equal to @a or @b. + * + * Return true if the merge was successful, + * false if it was not attempted. */ -bool hbitmap_merge(HBitmap *a, const HBitmap *b); +bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result); + +/** + * hbitmap_can_merge: + * + * hbitmap_can_merge(a, b) && hbitmap_can_merge(a, result) is sufficient and + * necessary for hbitmap_merge will not fail. + * + */ +bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b); /** * hbitmap_empty: diff --git a/util/hbitmap.c b/util/hbitmap.c index bcd304041a..d5aca5159f 100644 --- a/util/hbitmap.c +++ b/util/hbitmap.c @@ -723,6 +723,10 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size) } } +bool hbitmap_can_merge(const HBitmap *a, const HBitmap *b) +{ + return (a->size == b->size) && (a->granularity == b->granularity); +} /** * Given HBitmaps A and B, let A := A (BITOR) B. @@ -731,14 +735,15 @@ void hbitmap_truncate(HBitmap *hb, uint64_t size) * @return true if the merge was successful, * false if it was not attempted. */ -bool hbitmap_merge(HBitmap *a, const HBitmap *b) +bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result) { int i; uint64_t j; - if ((a->size != b->size) || (a->granularity != b->granularity)) { + if (!hbitmap_can_merge(a, b) || !hbitmap_can_merge(a, result)) { return false; } + assert(hbitmap_can_merge(b, result)); if (hbitmap_count(b) == 0) { return true; @@ -750,7 +755,7 @@ bool hbitmap_merge(HBitmap *a, const HBitmap *b) */ for (i = HBITMAP_LEVELS - 1; i >= 0; i--) { for (j = 0; j < a->sizes[i]; j++) { - a->levels[i][j] |= b->levels[i][j]; + result->levels[i][j] = a->levels[i][j] | b->levels[i][j]; } } From 5c4cf8b294ee65c049d6c40f5f6ff7c1befdb3d9 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Mon, 29 Oct 2018 16:23:15 -0400 Subject: [PATCH 05/19] blockdev: rename block-dirty-bitmap-clear transaction handlers Rename block-dirty-bitmap-clear transaction handlers to reuse them for x-block-dirty-bitmap-merge transaction in the following patch. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: John Snow --- blockdev.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/blockdev.c b/blockdev.c index b8a854fba5..d7776cbd4c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2027,7 +2027,7 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common, bdrv_clear_dirty_bitmap(state->bitmap, &state->backup); } -static void block_dirty_bitmap_clear_abort(BlkActionState *common) +static void block_dirty_bitmap_restore(BlkActionState *common) { BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, common, common); @@ -2037,7 +2037,7 @@ static void block_dirty_bitmap_clear_abort(BlkActionState *common) } } -static void block_dirty_bitmap_clear_commit(BlkActionState *common) +static void block_dirty_bitmap_free_backup(BlkActionState *common) { BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, common, common); @@ -2171,8 +2171,8 @@ static const BlkActionOps actions[] = { [TRANSACTION_ACTION_KIND_BLOCK_DIRTY_BITMAP_CLEAR] = { .instance_size = sizeof(BlockDirtyBitmapState), .prepare = block_dirty_bitmap_clear_prepare, - .commit = block_dirty_bitmap_clear_commit, - .abort = block_dirty_bitmap_clear_abort, + .commit = block_dirty_bitmap_free_backup, + .abort = block_dirty_bitmap_restore, }, [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_ENABLE] = { .instance_size = sizeof(BlockDirtyBitmapState), From 6fd2e40789ef7389b17c5fff93b0bf82d4352cb3 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Mon, 29 Oct 2018 16:23:15 -0400 Subject: [PATCH 06/19] qapi: add transaction support for x-block-dirty-bitmap-merge New action is like clean action: do the whole thing in .prepare and undo in .abort. This behavior for bitmap-changing actions is needed because backup job actions use bitmap in .prepare. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: John Snow --- blockdev.c | 35 +++++++++++++++++++++++++++++++++++ qapi/transaction.json | 2 ++ 2 files changed, 37 insertions(+) diff --git a/blockdev.c b/blockdev.c index d7776cbd4c..d685bb7746 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2113,6 +2113,35 @@ static void block_dirty_bitmap_disable_abort(BlkActionState *common) } } +static void block_dirty_bitmap_merge_prepare(BlkActionState *common, + Error **errp) +{ + BlockDirtyBitmapMerge *action; + BlockDirtyBitmapState *state = DO_UPCAST(BlockDirtyBitmapState, + common, common); + BdrvDirtyBitmap *merge_source; + + if (action_check_completion_mode(common, errp) < 0) { + return; + } + + action = common->action->u.x_block_dirty_bitmap_merge.data; + state->bitmap = block_dirty_bitmap_lookup(action->node, + action->dst_name, + &state->bs, + errp); + if (!state->bitmap) { + return; + } + + merge_source = bdrv_find_dirty_bitmap(state->bs, action->src_name); + if (!merge_source) { + return; + } + + bdrv_merge_dirty_bitmap(state->bitmap, merge_source, &state->backup, errp); +} + static void abort_prepare(BlkActionState *common, Error **errp) { error_setg(errp, "Transaction aborted using Abort action"); @@ -2184,6 +2213,12 @@ static const BlkActionOps actions[] = { .prepare = block_dirty_bitmap_disable_prepare, .abort = block_dirty_bitmap_disable_abort, }, + [TRANSACTION_ACTION_KIND_X_BLOCK_DIRTY_BITMAP_MERGE] = { + .instance_size = sizeof(BlockDirtyBitmapState), + .prepare = block_dirty_bitmap_merge_prepare, + .commit = block_dirty_bitmap_free_backup, + .abort = block_dirty_bitmap_restore, + }, /* Where are transactions for MIRROR, COMMIT and STREAM? * Although these blockjobs use transaction callbacks like the backup job, * these jobs do not necessarily adhere to transaction semantics. diff --git a/qapi/transaction.json b/qapi/transaction.json index d7e4274550..5875cdb16c 100644 --- a/qapi/transaction.json +++ b/qapi/transaction.json @@ -48,6 +48,7 @@ # - @block-dirty-bitmap-clear: since 2.5 # - @x-block-dirty-bitmap-enable: since 3.0 # - @x-block-dirty-bitmap-disable: since 3.0 +# - @x-block-dirty-bitmap-merge: since 3.1 # - @blockdev-backup: since 2.3 # - @blockdev-snapshot: since 2.5 # - @blockdev-snapshot-internal-sync: since 1.7 @@ -63,6 +64,7 @@ 'block-dirty-bitmap-clear': 'BlockDirtyBitmap', 'x-block-dirty-bitmap-enable': 'BlockDirtyBitmap', 'x-block-dirty-bitmap-disable': 'BlockDirtyBitmap', + 'x-block-dirty-bitmap-merge': 'BlockDirtyBitmapMerge', 'blockdev-backup': 'BlockdevBackup', 'blockdev-snapshot': 'BlockdevSnapshot', 'blockdev-snapshot-internal-sync': 'BlockdevSnapshotInternal', From 304cc429a07eb6601020212a478050ebbe87df88 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Mon, 29 Oct 2018 16:23:15 -0400 Subject: [PATCH 07/19] iotests: 169: drop deprecated 'autoload' parameter Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: John Snow Signed-off-by: John Snow --- tests/qemu-iotests/169 | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169 index f243db9955..df408f8367 100755 --- a/tests/qemu-iotests/169 +++ b/tests/qemu-iotests/169 @@ -58,7 +58,6 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase): 'granularity': granularity} if persistent: params['persistent'] = True - params['autoload'] = True result = vm.qmp('block-dirty-bitmap-add', **params) self.assert_qmp(result, 'return', {}); From 132adb682098e9af40a2132ec4feec6850fce8cd Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Mon, 29 Oct 2018 16:23:15 -0400 Subject: [PATCH 08/19] block/qcow2: improve error message in qcow2_inactivate Signed-off-by: Vladimir Sementsov-Ogievskiy [Maintainer edit -- touched up error message. --js] Reviewed-by: John Snow Signed-off-by: John Snow --- block/qcow2.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index 4f8d2fa7bd..eeb72ac362 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2123,9 +2123,9 @@ static int qcow2_inactivate(BlockDriverState *bs) qcow2_store_persistent_dirty_bitmaps(bs, &local_err); if (local_err != NULL) { result = -EINVAL; - error_report_err(local_err); - error_report("Persistent bitmaps are lost for node '%s'", - bdrv_get_device_or_node_name(bs)); + error_reportf_err(local_err, "Lost persistent bitmaps during " + "inactivation of node '%s': ", + bdrv_get_device_or_node_name(bs)); } ret = qcow2_cache_flush(bs, s->l2_table_cache); From 2ea427effff61efa5d0dc69f9cae126d13879617 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Mon, 29 Oct 2018 16:23:15 -0400 Subject: [PATCH 09/19] bloc/qcow2: drop dirty_bitmaps_loaded state variable This variable doesn't work as it should, because it is actually cleared in qcow2_co_invalidate_cache() by memset(). Drop it, as the following patch will introduce new behavior. Signed-off-by: Vladimir Sementsov-Ogievskiy Reviewed-by: John Snow Signed-off-by: John Snow --- block/qcow2.c | 19 ++----------------- block/qcow2.h | 1 - 2 files changed, 2 insertions(+), 18 deletions(-) diff --git a/block/qcow2.c b/block/qcow2.c index eeb72ac362..3110bb0e20 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1153,7 +1153,6 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, uint64_t ext_end; uint64_t l1_vm_state_index; bool update_header = false; - bool header_updated = false; ret = bdrv_pread(bs->file, 0, &header, sizeof(header)); if (ret < 0) { @@ -1492,23 +1491,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, s->autoclear_features &= QCOW2_AUTOCLEAR_MASK; } - if (s->dirty_bitmaps_loaded) { - /* It's some kind of reopen. There are no known cases where we need to - * reload bitmaps in such a situation, so it's safer to skip them. - * - * Moreover, if we have some readonly bitmaps and we are reopening for - * rw we should reopen bitmaps correspondingly. - */ - if (bdrv_has_readonly_bitmaps(bs) && - !bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) - { - qcow2_reopen_bitmaps_rw_hint(bs, &header_updated, &local_err); - } - } else { - header_updated = qcow2_load_dirty_bitmaps(bs, &local_err); - s->dirty_bitmaps_loaded = true; + if (qcow2_load_dirty_bitmaps(bs, &local_err)) { + update_header = false; } - update_header = update_header && !header_updated; if (local_err != NULL) { error_propagate(errp, local_err); ret = -EINVAL; diff --git a/block/qcow2.h b/block/qcow2.h index ba430316b9..29c98d87a0 100644 --- a/block/qcow2.h +++ b/block/qcow2.h @@ -300,7 +300,6 @@ typedef struct BDRVQcow2State { uint32_t nb_bitmaps; uint64_t bitmap_directory_size; uint64_t bitmap_directory_offset; - bool dirty_bitmaps_loaded; int flags; int qcow_version; From 993edc0ce0c6f44deb8272a7a857e419417f5f84 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 29 Oct 2018 16:23:16 -0400 Subject: [PATCH 10/19] block/dirty-bitmaps: add user_locked status checker Instead of both frozen and qmp_locked checks, wrap it into one check. frozen implies the bitmap is split in two (for backup), and shouldn't be modified. qmp_locked implies it's being used by another operation, like being exported over NBD. In both cases it means we shouldn't allow the user to modify it in any meaningful way. Replace any usages where we check both frozen and qmp_locked with the new check. Signed-off-by: John Snow Reviewed-by: Eric Blake Message-id: 20181002230218.13949-2-jsnow@redhat.com [w/edits Suggested-By: Vladimir Sementsov-Ogievskiy ] Signed-off-by: John Snow --- block/dirty-bitmap.c | 6 ++++++ blockdev.c | 29 ++++++++--------------------- include/block/dirty-bitmap.h | 1 + migration/block-dirty-bitmap.c | 10 ++-------- 4 files changed, 17 insertions(+), 29 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 8ac933cf1c..9603cdd29b 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -176,6 +176,12 @@ bool bdrv_dirty_bitmap_frozen(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) || + bdrv_dirty_bitmap_qmp_locked(bitmap); +} + void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked) { qemu_mutex_lock(bitmap->mutex); diff --git a/blockdev.c b/blockdev.c index d685bb7746..9da0cf1a72 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2010,11 +2010,8 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common, return; } - if (bdrv_dirty_bitmap_frozen(state->bitmap)) { - error_setg(errp, "Cannot modify a frozen bitmap"); - return; - } else if (bdrv_dirty_bitmap_qmp_locked(state->bitmap)) { - error_setg(errp, "Cannot modify a locked bitmap"); + if (bdrv_dirty_bitmap_user_locked(state->bitmap)) { + error_setg(errp, "Cannot modify a bitmap in use by another operation"); return; } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) { error_setg(errp, "Cannot clear a disabled bitmap"); @@ -2883,15 +2880,10 @@ void qmp_block_dirty_bitmap_remove(const char *node, const char *name, return; } - if (bdrv_dirty_bitmap_frozen(bitmap)) { + if (bdrv_dirty_bitmap_user_locked(bitmap)) { error_setg(errp, - "Bitmap '%s' is currently frozen and cannot be removed", - name); - return; - } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) { - error_setg(errp, - "Bitmap '%s' is currently locked and cannot be removed", - name); + "Bitmap '%s' is currently in use by another operation and" + " cannot be removed", name); return; } @@ -2921,15 +2913,10 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name, return; } - if (bdrv_dirty_bitmap_frozen(bitmap)) { + if (bdrv_dirty_bitmap_user_locked(bitmap)) { error_setg(errp, - "Bitmap '%s' is currently frozen and cannot be modified", - name); - return; - } else if (bdrv_dirty_bitmap_qmp_locked(bitmap)) { - error_setg(errp, - "Bitmap '%s' is currently locked and cannot be modified", - name); + "Bitmap '%s' is currently in use by another operation" + " and cannot be cleared", name); return; } else if (!bdrv_dirty_bitmap_enabled(bitmap)) { error_setg(errp, diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 201ff7f20b..14639439a2 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -94,6 +94,7 @@ 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_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 477826330c..dfbfb853b7 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -301,14 +301,8 @@ static int init_dirty_bitmap_migration(void) goto fail; } - if (bdrv_dirty_bitmap_frozen(bitmap)) { - error_report("Can't migrate frozen dirty bitmap: '%s", - bdrv_dirty_bitmap_name(bitmap)); - goto fail; - } - - if (bdrv_dirty_bitmap_qmp_locked(bitmap)) { - error_report("Can't migrate locked dirty bitmap: '%s", + if (bdrv_dirty_bitmap_user_locked(bitmap)) { + error_report("Can't migrate a bitmap that is in use by another operation: '%s'", bdrv_dirty_bitmap_name(bitmap)); goto fail; } From 283d7a04f2addcc51468635300208b60c19a0db3 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 29 Oct 2018 16:23:16 -0400 Subject: [PATCH 11/19] block/dirty-bitmaps: fix merge permissions In prior commits that made merge transactionable, we removed the assertion that merge cannot operate on disabled bitmaps. In addition, we want to make sure that we are prohibiting merges to "locked" bitmaps. Use the new user_locked function to check. Reported-by: Eric Blake Signed-off-by: John Snow Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-id: 20181002230218.13949-3-jsnow@redhat.com Signed-off-by: John Snow --- block/dirty-bitmap.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 9603cdd29b..bfccb0ea15 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -806,9 +806,9 @@ void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, qemu_mutex_lock(dest->mutex); - if (bdrv_dirty_bitmap_frozen(dest)) { - error_setg(errp, "Bitmap '%s' is frozen and cannot be modified", - dest->name); + if (bdrv_dirty_bitmap_user_locked(dest)) { + error_setg(errp, "Bitmap '%s' is currently in use by another" + " operation and cannot be modified", dest->name); goto out; } From 0be37c9e19f541643ef407bdafe0282b667ec23c Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 29 Oct 2018 16:23:16 -0400 Subject: [PATCH 12/19] block/dirty-bitmaps: allow clear on disabled bitmaps Similarly to merge, it's OK to allow clear operations on disabled bitmaps, as this condition only means that they are not recording new writes. We are free to clear it if the user requests it. Signed-off-by: John Snow Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-id: 20181002230218.13949-4-jsnow@redhat.com Signed-off-by: John Snow --- block/dirty-bitmap.c | 1 - blockdev.c | 8 -------- 2 files changed, 9 deletions(-) diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index bfccb0ea15..9b9ebd7142 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -625,7 +625,6 @@ void bdrv_reset_dirty_bitmap(BdrvDirtyBitmap *bitmap, void bdrv_clear_dirty_bitmap(BdrvDirtyBitmap *bitmap, HBitmap **out) { - assert(bdrv_dirty_bitmap_enabled(bitmap)); assert(!bdrv_dirty_bitmap_readonly(bitmap)); bdrv_dirty_bitmap_lock(bitmap); if (!out) { diff --git a/blockdev.c b/blockdev.c index 9da0cf1a72..8970f699b9 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2013,9 +2013,6 @@ static void block_dirty_bitmap_clear_prepare(BlkActionState *common, if (bdrv_dirty_bitmap_user_locked(state->bitmap)) { error_setg(errp, "Cannot modify a bitmap in use by another operation"); return; - } else if (!bdrv_dirty_bitmap_enabled(state->bitmap)) { - error_setg(errp, "Cannot clear a disabled bitmap"); - return; } else if (bdrv_dirty_bitmap_readonly(state->bitmap)) { error_setg(errp, "Cannot clear a readonly bitmap"); return; @@ -2918,11 +2915,6 @@ void qmp_block_dirty_bitmap_clear(const char *node, const char *name, "Bitmap '%s' is currently in use by another operation" " and cannot be cleared", name); return; - } else if (!bdrv_dirty_bitmap_enabled(bitmap)) { - error_setg(errp, - "Bitmap '%s' is currently disabled and cannot be cleared", - name); - return; } else if (bdrv_dirty_bitmap_readonly(bitmap)) { error_setg(errp, "Bitmap '%s' is readonly and cannot be cleared", name); return; From b053bb55738f35832f3d6472b12277a75c32a038 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 29 Oct 2018 16:23:16 -0400 Subject: [PATCH 13/19] block/dirty-bitmaps: prohibit enable/disable on locked/frozen bitmaps We're not being consistent about this. If it's in use by an operation, the user should not be able to change the behavior of that bitmap. Signed-off-by: John Snow Reviewed-by: Eric Blake Reviewed-by: Vladimir Sementsov-Ogievskiy Message-id: 20181002230218.13949-5-jsnow@redhat.com Signed-off-by: John Snow --- blockdev.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/blockdev.c b/blockdev.c index 8970f699b9..35097b92cc 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2059,6 +2059,13 @@ static void block_dirty_bitmap_enable_prepare(BlkActionState *common, return; } + if (bdrv_dirty_bitmap_user_locked(state->bitmap)) { + error_setg(errp, + "Bitmap '%s' is currently in use by another operation" + " and cannot be enabled", action->name); + return; + } + state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap); bdrv_enable_dirty_bitmap(state->bitmap); } @@ -2093,6 +2100,13 @@ static void block_dirty_bitmap_disable_prepare(BlkActionState *common, return; } + if (bdrv_dirty_bitmap_user_locked(state->bitmap)) { + error_setg(errp, + "Bitmap '%s' is currently in use by another operation" + " and cannot be disabled", action->name); + return; + } + state->was_enabled = bdrv_dirty_bitmap_enabled(state->bitmap); bdrv_disable_dirty_bitmap(state->bitmap); } @@ -2934,10 +2948,10 @@ void qmp_x_block_dirty_bitmap_enable(const char *node, const char *name, return; } - if (bdrv_dirty_bitmap_frozen(bitmap)) { + if (bdrv_dirty_bitmap_user_locked(bitmap)) { error_setg(errp, - "Bitmap '%s' is currently frozen and cannot be enabled", - name); + "Bitmap '%s' is currently in use by another operation" + " and cannot be enabled", name); return; } @@ -2955,10 +2969,10 @@ void qmp_x_block_dirty_bitmap_disable(const char *node, const char *name, return; } - if (bdrv_dirty_bitmap_frozen(bitmap)) { + if (bdrv_dirty_bitmap_user_locked(bitmap)) { error_setg(errp, - "Bitmap '%s' is currently frozen and cannot be disabled", - name); + "Bitmap '%s' is currently in use by another operation" + " and cannot be disabled", name); return; } From b27a6b8b329a8dcbab9dc1af45586f7585f3d47b Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 29 Oct 2018 16:23:16 -0400 Subject: [PATCH 14/19] block/backup: prohibit backup from using in use bitmaps If the bitmap is frozen, we shouldn't touch it. Signed-off-by: John Snow Reviewed-by: Vladimir Sementsov-Ogievskiy Message-id: 20181002230218.13949-6-jsnow@redhat.com Signed-off-by: John Snow --- blockdev.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/blockdev.c b/blockdev.c index 35097b92cc..c30495d035 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3513,10 +3513,10 @@ static BlockJob *do_drive_backup(DriveBackup *backup, JobTxn *txn, bdrv_unref(target_bs); goto out; } - if (bdrv_dirty_bitmap_qmp_locked(bmap)) { + if (bdrv_dirty_bitmap_user_locked(bmap)) { error_setg(errp, - "Bitmap '%s' is currently locked and cannot be used for " - "backup", backup->bitmap); + "Bitmap '%s' is currently in use by another operation" + " and cannot be used for backup", backup->bitmap); goto out; } } @@ -3621,10 +3621,10 @@ 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_qmp_locked(bmap)) { + if (bdrv_dirty_bitmap_user_locked(bmap)) { error_setg(errp, - "Bitmap '%s' is currently locked and cannot be used for " - "backup", backup->bitmap); + "Bitmap '%s' is currently in use by another operation" + " and cannot be used for backup", backup->bitmap); goto out; } } From d9782022bda7f8eccaf961044e9efe980dc90c04 Mon Sep 17 00:00:00 2001 From: John Snow Date: Mon, 29 Oct 2018 16:23:16 -0400 Subject: [PATCH 15/19] nbd: forbid use of frozen bitmaps Whether it's "locked" or "frozen", it's in use and should not be allowed for the purposes of this operation. Signed-off-by: John Snow Reviewed-by: Vladimir Sementsov-Ogievskiy Message-id: 20181002230218.13949-7-jsnow@redhat.com Signed-off-by: John Snow --- nbd/server.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nbd/server.c b/nbd/server.c index a1eda0114f..4e8f5ae51b 100644 --- a/nbd/server.c +++ b/nbd/server.c @@ -2456,8 +2456,8 @@ void nbd_export_bitmap(NBDExport *exp, const char *bitmap, return; } - if (bdrv_dirty_bitmap_qmp_locked(bm)) { - error_setg(errp, "Bitmap '%s' is locked", bitmap); + if (bdrv_dirty_bitmap_user_locked(bm)) { + error_setg(errp, "Bitmap '%s' is in use", bitmap); return; } From d1dde7149e376d72b422a529ec4bf3ed47f3ba30 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 29 Oct 2018 16:23:17 -0400 Subject: [PATCH 16/19] bitmap: Update count after a merge We need an accurate count of the number of bits set in a bitmap after a merge. In particular, since the merge operation short-circuits a merge from an empty source, if you have bitmaps A, B, and C where B started empty, then merge C into B, and B into A, an inaccurate count meant that A did not get the contents of C. In the worst case, we may falsely regard the bitmap as empty when it has had new writes merged into it. Fixes: be58721db CC: qemu-stable@nongnu.org Signed-off-by: Eric Blake Signed-off-by: John Snow Reviewed-by: Vladimir Sementsov-Ogievskiy Message-id: 20181002233314.30159-1-jsnow@redhat.com Signed-off-by: John Snow --- util/hbitmap.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/util/hbitmap.c b/util/hbitmap.c index d5aca5159f..8d402c59d9 100644 --- a/util/hbitmap.c +++ b/util/hbitmap.c @@ -759,6 +759,9 @@ bool hbitmap_merge(const HBitmap *a, const HBitmap *b, HBitmap *result) } } + /* Recompute the dirty count */ + result->count = hb_count_between(result, 0, result->size - 1); + return true; } From 9c98f145dfb994e1e9d68a4d606ee5693891280d Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Mon, 29 Oct 2018 16:23:17 -0400 Subject: [PATCH 17/19] dirty-bitmaps: clean-up bitmaps loading and migration logic This patch aims to bring the following behavior: 1. We don't load bitmaps, when started in inactive mode. It's the case of incoming migration. In this case we wait for bitmaps migration through migration channel (if 'dirty-bitmaps' capability is enabled) or for invalidation (to load bitmaps from the image). 2. We don't remove persistent bitmaps on inactivation. Instead, we only remove bitmaps after storing. This is the only way to restore bitmaps, if we decided to resume source after [failed] migration with 'dirty-bitmaps' capability enabled (which means, that bitmaps were not stored). 3. We load bitmaps on open and any invalidation, it's ok for all cases: - normal open - migration target invalidation with dirty-bitmaps capability (bitmaps are migrating through migration channel, the are not stored, so they should have IN_USE flag set and will be skipped when loading. However, it would fail if bitmaps are read-only[1]) - migration target invalidation without dirty-bitmaps capability (normal load of the bitmaps, if migrated with shared storage) - source invalidation with dirty-bitmaps capability (skip because IN_USE) - source invalidation without dirty-bitmaps capability (bitmaps were dropped, reload them) [1]: to accurately handle this, migration of read-only bitmaps is explicitly forbidden in this patch. New mechanism for not storing bitmaps when migrate with dirty-bitmaps capability is introduced: migration filed in BdrvDirtyBitmap. Signed-off-by: Vladimir Sementsov-Ogievskiy Signed-off-by: John Snow --- block.c | 11 +++--- block/dirty-bitmap.c | 36 ++++++++----------- block/qcow2-bitmap.c | 16 +++++++++ block/qcow2.c | 65 ++++++++++++++++++++++++++++++++-- include/block/dirty-bitmap.h | 2 +- migration/block-dirty-bitmap.c | 10 ++++-- 6 files changed, 109 insertions(+), 31 deletions(-) diff --git a/block.c b/block.c index 08d64cdc61..95d8635aa1 100644 --- a/block.c +++ b/block.c @@ -4403,6 +4403,7 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, uint64_t perm, shared_perm; Error *local_err = NULL; int ret; + BdrvDirtyBitmap *bm; if (!bs->drv) { return; @@ -4452,6 +4453,12 @@ static void coroutine_fn bdrv_co_invalidate_cache(BlockDriverState *bs, } } + for (bm = bdrv_dirty_bitmap_next(bs, NULL); bm; + bm = bdrv_dirty_bitmap_next(bs, bm)) + { + bdrv_dirty_bitmap_set_migration(bm, false); + } + ret = refresh_total_sectors(bs, bs->total_sectors); if (ret < 0) { bs->open_flags |= BDRV_O_INACTIVE; @@ -4566,10 +4573,6 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs, } } - /* At this point persistent bitmaps should be already stored by the format - * driver */ - bdrv_release_persistent_dirty_bitmaps(bs); - return 0; } diff --git a/block/dirty-bitmap.c b/block/dirty-bitmap.c index 9b9ebd7142..89fd1d7f8b 100644 --- a/block/dirty-bitmap.c +++ b/block/dirty-bitmap.c @@ -55,6 +55,10 @@ struct BdrvDirtyBitmap { and this bitmap must remain unchanged while this flag is set. */ bool persistent; /* bitmap must be saved to owner disk image */ + bool migration; /* Bitmap is selected for migration, it should + not be stored on the next inactivation + (persistent flag doesn't matter until next + invalidation).*/ QLIST_ENTRY(BdrvDirtyBitmap) list; }; @@ -389,26 +393,6 @@ void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs) bdrv_dirty_bitmaps_unlock(bs); } -/** - * Release all persistent dirty bitmaps attached to a BDS (for use in - * bdrv_inactivate_recurse()). - * There must not be any frozen bitmaps attached. - * This function does not remove persistent bitmaps from the storage. - * Called with BQL taken. - */ -void bdrv_release_persistent_dirty_bitmaps(BlockDriverState *bs) -{ - BdrvDirtyBitmap *bm, *next; - - bdrv_dirty_bitmaps_lock(bs); - QLIST_FOREACH_SAFE(bm, &bs->dirty_bitmaps, list, next) { - if (bdrv_dirty_bitmap_get_persistance(bm)) { - bdrv_release_dirty_bitmap_locked(bm); - } - } - bdrv_dirty_bitmaps_unlock(bs); -} - /** * Remove persistent dirty bitmap from the storage if it exists. * Absence of bitmap is not an error, because we have the following scenario: @@ -761,16 +745,24 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, bool persistent) qemu_mutex_unlock(bitmap->mutex); } +/* Called with BQL taken. */ +void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration) +{ + qemu_mutex_lock(bitmap->mutex); + bitmap->migration = migration; + qemu_mutex_unlock(bitmap->mutex); +} + bool bdrv_dirty_bitmap_get_persistance(BdrvDirtyBitmap *bitmap) { - return bitmap->persistent; + return bitmap->persistent && !bitmap->migration; } bool bdrv_has_changed_persistent_bitmaps(BlockDriverState *bs) { BdrvDirtyBitmap *bm; QLIST_FOREACH(bm, &bs->dirty_bitmaps, list) { - if (bm->persistent && !bm->readonly) { + if (bm->persistent && !bm->readonly && !bm->migration) { return true; } } diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c index ba978ad2aa..b5f1b3563d 100644 --- a/block/qcow2-bitmap.c +++ b/block/qcow2-bitmap.c @@ -1418,6 +1418,22 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp) g_free(tb); } + QSIMPLEQ_FOREACH(bm, bm_list, entry) { + /* For safety, we remove bitmap after storing. + * We may be here in two cases: + * 1. bdrv_close. It's ok to drop bitmap. + * 2. inactivation. It means migration without 'dirty-bitmaps' + * capability, so bitmaps are not marked with + * BdrvDirtyBitmap.migration flags. It's not bad to drop them too, + * and reload on invalidation. + */ + if (bm->dirty_bitmap == NULL) { + continue; + } + + bdrv_release_dirty_bitmap(bs, bm->dirty_bitmap); + } + bitmap_list_free(bm_list); return; diff --git a/block/qcow2.c b/block/qcow2.c index 3110bb0e20..30689b7688 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -1491,8 +1491,69 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, s->autoclear_features &= QCOW2_AUTOCLEAR_MASK; } - if (qcow2_load_dirty_bitmaps(bs, &local_err)) { - update_header = false; + /* == Handle persistent dirty bitmaps == + * + * We want load dirty bitmaps in three cases: + * + * 1. Normal open of the disk in active mode, not related to invalidation + * after migration. + * + * 2. Invalidation of the target vm after pre-copy phase of migration, if + * bitmaps are _not_ migrating through migration channel, i.e. + * 'dirty-bitmaps' capability is disabled. + * + * 3. Invalidation of source vm after failed or canceled migration. + * This is a very interesting case. There are two possible types of + * bitmaps: + * + * A. Stored on inactivation and removed. They should be loaded from the + * image. + * + * B. Not stored: not-persistent bitmaps and bitmaps, migrated through + * the migration channel (with dirty-bitmaps capability). + * + * On the other hand, there are two possible sub-cases: + * + * 3.1 disk was changed by somebody else while were inactive. In this + * case all in-RAM dirty bitmaps (both persistent and not) are + * definitely invalid. And we don't have any method to determine + * this. + * + * Simple and safe thing is to just drop all the bitmaps of type B on + * inactivation. But in this case we lose bitmaps in valid 4.2 case. + * + * On the other hand, resuming source vm, if disk was already changed + * is a bad thing anyway: not only bitmaps, the whole vm state is + * out of sync with disk. + * + * This means, that user or management tool, who for some reason + * decided to resume source vm, after disk was already changed by + * target vm, should at least drop all dirty bitmaps by hand. + * + * So, we can ignore this case for now, but TODO: "generation" + * extension for qcow2, to determine, that image was changed after + * last inactivation. And if it is changed, we will drop (or at least + * mark as 'invalid' all the bitmaps of type B, both persistent + * and not). + * + * 3.2 disk was _not_ changed while were inactive. Bitmaps may be saved + * to disk ('dirty-bitmaps' capability disabled), or not saved + * ('dirty-bitmaps' capability enabled), but we don't need to care + * of: let's load bitmaps as always: stored bitmaps will be loaded, + * and not stored has flag IN_USE=1 in the image and will be skipped + * on loading. + * + * One remaining possible case when we don't want load bitmaps: + * + * 4. Open disk in inactive mode in target vm (bitmaps are migrating or + * will be loaded on invalidation, no needs try loading them before) + */ + + if (!(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) { + /* It's case 1, 2 or 3.2. Or 3.1 which is BUG in management layer. */ + bool header_updated = qcow2_load_dirty_bitmaps(bs, &local_err); + + update_header = update_header && !header_updated; } if (local_err != NULL) { error_propagate(errp, local_err); diff --git a/include/block/dirty-bitmap.h b/include/block/dirty-bitmap.h index 14639439a2..8f38a3dec1 100644 --- a/include/block/dirty-bitmap.h +++ b/include/block/dirty-bitmap.h @@ -26,7 +26,6 @@ BdrvDirtyBitmap *bdrv_find_dirty_bitmap(BlockDriverState *bs, const char *name); void bdrv_release_dirty_bitmap(BlockDriverState *bs, BdrvDirtyBitmap *bitmap); void bdrv_release_named_dirty_bitmaps(BlockDriverState *bs); -void bdrv_release_persistent_dirty_bitmaps(BlockDriverState *bs); void bdrv_remove_persistent_dirty_bitmap(BlockDriverState *bs, const char *name, Error **errp); @@ -72,6 +71,7 @@ void bdrv_dirty_bitmap_set_persistance(BdrvDirtyBitmap *bitmap, void bdrv_dirty_bitmap_set_qmp_locked(BdrvDirtyBitmap *bitmap, bool qmp_locked); void bdrv_merge_dirty_bitmap(BdrvDirtyBitmap *dest, const BdrvDirtyBitmap *src, HBitmap **backup, Error **errp); +void bdrv_dirty_bitmap_set_migration(BdrvDirtyBitmap *bitmap, bool migration); /* Functions that require manual locking. */ void bdrv_dirty_bitmap_lock(BdrvDirtyBitmap *bitmap); diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index dfbfb853b7..5e90f44c2f 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -307,6 +307,12 @@ static int init_dirty_bitmap_migration(void) goto fail; } + if (bdrv_dirty_bitmap_readonly(bitmap)) { + error_report("Can't migrate read-only dirty bitmap: '%s", + bdrv_dirty_bitmap_name(bitmap)); + goto fail; + } + bdrv_ref(bs); bdrv_dirty_bitmap_set_qmp_locked(bitmap, true); @@ -329,9 +335,9 @@ static int init_dirty_bitmap_migration(void) } } - /* unset persistance here, to not roll back it */ + /* unset migration flags here, to not roll back it */ QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) { - bdrv_dirty_bitmap_set_persistance(dbms->bitmap, false); + bdrv_dirty_bitmap_set_migration(dbms->bitmap, true); } if (QSIMPLEQ_EMPTY(&dirty_bitmap_mig_state.dbms_list)) { From b9247fc1a8ffe5c367fa049f295fbb58c8ca9d05 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Mon, 29 Oct 2018 16:23:17 -0400 Subject: [PATCH 18/19] iotests: improve 169 Before previous patch, iotest 169 was actually broken for the case test_persistent__not_migbitmap__offline_shared, while formally passing. After migration log of vm_b had message: qemu-system-x86_64: Could not reopen qcow2 layer: Bitmap already exists: bitmap0 which means that invalidation failed and bs->drv = NULL. It was because we've loaded bitmap twice: on open and on invalidation. Add code to 169, to catch such fails. Signed-off-by: Vladimir Sementsov-Ogievskiy Signed-off-by: John Snow --- tests/qemu-iotests/169 | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169 index df408f8367..8b7947d650 100755 --- a/tests/qemu-iotests/169 +++ b/tests/qemu-iotests/169 @@ -24,6 +24,7 @@ import time import itertools import operator import new +import re from iotests import qemu_img @@ -133,6 +134,14 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase): if should_migrate: self.vm_b.shutdown() + + # catch 'Could not reopen qcow2 layer: Bitmap already exists' + # possible error + log = self.vm_b.get_log() + log = re.sub(r'^\[I \d+\.\d+\] OPENED\n', '', log) + log = re.sub(r'\[I \+\d+\.\d+\] CLOSED\n?$', '', log) + self.assertEqual(log, '') + # recreate vm_b, as we don't want -incoming option (this will lead # to "cat" process left alive after test finish) self.vm_b = iotests.VM(path_suffix='b') From 3e6d88f280a53b5b399e73b1f80efe4c3db306f1 Mon Sep 17 00:00:00 2001 From: Vladimir Sementsov-Ogievskiy Date: Mon, 29 Oct 2018 16:23:17 -0400 Subject: [PATCH 19/19] iotests: 169: add cases for source vm resuming Test that we can resume source vm after [failed] migration, and bitmaps are ok. Signed-off-by: Vladimir Sementsov-Ogievskiy Signed-off-by: John Snow --- tests/qemu-iotests/169 | 60 +++++++++++++++++++++++++++++++++++++- tests/qemu-iotests/169.out | 4 +-- 2 files changed, 61 insertions(+), 3 deletions(-) diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169 index 8b7947d650..69850c4c67 100755 --- a/tests/qemu-iotests/169 +++ b/tests/qemu-iotests/169 @@ -77,6 +77,58 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase): self.assert_qmp(result, 'error/desc', "Dirty bitmap 'bitmap0' not found"); + def do_test_migration_resume_source(self, persistent, migrate_bitmaps): + granularity = 512 + + # regions = ((start, count), ...) + regions = ((0, 0x10000), + (0xf0000, 0x10000), + (0xa0201, 0x1000)) + + mig_caps = [{'capability': 'events', 'state': True}] + if migrate_bitmaps: + mig_caps.append({'capability': 'dirty-bitmaps', 'state': True}) + + result = self.vm_a.qmp('migrate-set-capabilities', + capabilities=mig_caps) + self.assert_qmp(result, 'return', {}) + + self.add_bitmap(self.vm_a, granularity, persistent) + for r in regions: + self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % r) + sha256 = self.get_bitmap_hash(self.vm_a) + + result = self.vm_a.qmp('migrate', uri=mig_cmd) + while True: + event = self.vm_a.event_wait('MIGRATION') + if event['data']['status'] == 'completed': + break + + # test that bitmap is still here + removed = (not migrate_bitmaps) and persistent + self.check_bitmap(self.vm_a, False if removed else sha256) + + self.vm_a.qmp('cont') + + # test that bitmap is still here after invalidation + self.check_bitmap(self.vm_a, sha256) + + # shutdown and check that invalidation didn't fail + self.vm_a.shutdown() + + # catch 'Could not reopen qcow2 layer: Bitmap already exists' + # possible error + log = self.vm_a.get_log() + log = re.sub(r'^\[I \d+\.\d+\] OPENED\n', '', log) + log = re.sub(r'^(wrote .* bytes at offset .*\n.*KiB.*ops.*sec.*\n){3}', + '', log) + log = re.sub(r'\[I \+\d+\.\d+\] CLOSED\n?$', '', log) + self.assertEqual(log, '') + + # test that bitmap is still persistent + self.vm_a.launch() + self.check_bitmap(self.vm_a, sha256 if persistent else False) + def do_test_migration(self, persistent, migrate_bitmaps, online, shared_storage): granularity = 512 @@ -152,7 +204,7 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase): def inject_test_case(klass, name, method, *args, **kwargs): mc = operator.methodcaller(method, *args, **kwargs) - setattr(klass, 'test_' + name, new.instancemethod(mc, None, klass)) + setattr(klass, 'test_' + method + name, new.instancemethod(mc, None, klass)) for cmb in list(itertools.product((True, False), repeat=4)): name = ('_' if cmb[0] else '_not_') + 'persistent_' @@ -163,6 +215,12 @@ for cmb in list(itertools.product((True, False), repeat=4)): inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration', *list(cmb)) +for cmb in list(itertools.product((True, False), repeat=2)): + name = ('_' if cmb[0] else '_not_') + 'persistent_' + name += ('_' if cmb[1] else '_not_') + 'migbitmap' + + inject_test_case(TestDirtyBitmapMigration, name, + 'do_test_migration_resume_source', *list(cmb)) if __name__ == '__main__': iotests.main(supported_fmts=['qcow2']) diff --git a/tests/qemu-iotests/169.out b/tests/qemu-iotests/169.out index b6f257674e..3a89159833 100644 --- a/tests/qemu-iotests/169.out +++ b/tests/qemu-iotests/169.out @@ -1,5 +1,5 @@ -................ +.................... ---------------------------------------------------------------------- -Ran 16 tests +Ran 20 tests OK