fix persistent bitmaps migration logic
cover fix persistent bitmaps migration logic
This is an updated version of Vladimir's proposal for fixing the handling around migration and persistent dirty bitmaps.
Patches 1, 4, 6, and 7 update the testing for this feature. Patch 2 touches up an error message.
Patch 3 removes dead code.
Patch 5 contains the real fix.
v2:
- Add a new patch 4 as a prerequisite for what's now patch 5
- Rework the fix to be (hopefully) cleaner, see patch 5 notes
- Adjust error message in patch 2 (Eric)
- Adjust test logic slightly (patches 6, 7) to deal with changes in patch 5.
John Snow (2):
iotests: 169: actually test block migration
dirty-bitmaps: clean-up bitmaps loading and migration logic
Vladimir Sementsov-Ogievskiy (5):
iotests: 169: drop deprecated 'autoload' parameter block/qcow2: improve error message in qcow2_inactivate block/qcow2: drop dirty_bitmaps_loaded state variable
iotests: improve 169
iotests: 169: add cases for source vm resuming
block.c | 4 --- block/dirty-bitmap.c | 20 ------------ block/qcow2-bitmap.c | 16 +++++++++ block/qcow2.c | 26 ++++----------- block/qcow2.h | 1 - include/block/dirty-bitmap.h | 2 +- migration/block-dirty-bitmap.c | 11 ++++--- tests/qemu-iotests/169 | 74 ++++++++++++++++++++++++++++++++++++++++-- tests/qemu-iotests/169.out | 4 +-- 9 files changed, 103 insertions(+), 55 deletions(-)
Signature · 1 line
2.14.4
PATCH 1/7 iotests: 169: drop deprecated 'autoload' parameter
From: Vladimir Sementsov-Ogievskiy <[email protected]>
Message-id: [email protected]
--- tests/qemu-iotests/169 | 1 - 1 file changed, 1 deletion(-)
tests/qemu-iotests/169 +0 -1
@@ -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', {});
Signature · 1 line
2.14.4
PATCH 2/7 block/qcow2: improve error message in qcow2_inactivate
In reply to John SnowFrom: Vladimir Sementsov-Ogievskiy <[email protected]>
--- block/qcow2.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
block/qcow2.c +3 -3
@@ -2127,9 +2127,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);
Signature · 1 line
2.14.4
PATCH 3/7 block/qcow2: drop dirty_bitmaps_loaded state variable
In reply to John SnowFrom: Vladimir Sementsov-Ogievskiy <[email protected]>
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.
--- block/qcow2.c | 19 ++----------------- block/qcow2.h | 1 - 2 files changed, 2 insertions(+), 18 deletions(-)
block/qcow2.c +2 -17
@@ -1157,7 +1157,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 ) {
@@ -1496,23 +1495,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 ;
block/qcow2.h +0 -1
@@ -295,7 +295,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;
Signature · 1 line
2.14.4
PATCH 4/7 iotests: 169: actually test block migration
In reply to John SnowPresently, we emulate a block migration by just using a different target file. Update the test to actually request a block migration.
--- tests/qemu-iotests/169 | 3 +++ 1 file changed, 3 insertions(+)
tests/qemu-iotests/169 +3 -0
@@ -89,6 +89,9 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase): mig_caps = [{'capability': 'events', 'state': True}] if migrate_bitmaps: mig_caps.append({'capability': 'dirty-bitmaps', 'state': True}) + if not shared_storage: + mig_caps.append({'capability': 'block', 'state': True}) + result = self.vm_a.qmp('migrate-set-capabilities', capabilities=mig_caps)
Signature · 1 line
2.14.4
PATCH 5/7 dirty-bitmaps: clean-up bitmaps loading and migration logic
In reply to John SnowThis patch aims to bring the following behavior:
1. Bitmaps are not loaded on open if BDRV_O_INACTIVE is set, which occurs for incoming migration cases. We will load these persistent bitmaps on invalidate instead.
2. Regardless of the migration circumstances, persistent bitmaps are always flushed to disk on inactivate or close.
3. Bitmaps are read from disk and loaded into memory on any invalidation:
A) Normal, RW open
B) Migration target invalidation: With or without dirty-bitmaps capability, any bitmaps found on-disk will be loaded.
C) Migration source invalidation: If the migration fails and the source QEMU is resumed, it will re-load bitmaps from disk.
4. When using the dirty-bitmaps migration capability, persistent bitmaps are skipped if we are performing a shared storage migration. The destination QEMU will load them from disk on invalidate. (Transient, non-persistent bitmaps still get migrated.)
For block migrations, persistent bitmaps are both flushed to disk on the source and migrated live across the migration channel.
5. Persistent bitmaps are not unloaded from memory in the inactivate handler. Instead, they are removed on-store which occurs during inactivate. The effect is functionally the same, but keeps bitmap management more inside the module.
--- block.c | 4 ---- block/dirty-bitmap.c | 20 -------------------- block/qcow2-bitmap.c | 16 ++++++++++++++++ block/qcow2.c | 5 +++-- include/block/dirty-bitmap.h | 2 +- migration/block-dirty-bitmap.c | 11 ++++++----- 6 files changed, 26 insertions(+), 32 deletions(-)
block.c +0 -4
@@ -4497,10 +4497,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 ; }
block/dirty-bitmap.c +0 -20
@@ -383,26 +383,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:
block/qcow2-bitmap.c +16 -0
@@ -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 ;
block/qcow2.c +3 -2
@@ -1495,8 +1495,9 @@ 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; +if (!(bdrv_get_flags (bs) &BDRV_O_INACTIVE )) { +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);
include/block/dirty-bitmap.h +1 -1
@@ -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,Error **errp); +void bdrv_dirty_bitmap_set_migration (BdrvDirtyBitmap *bitmap,bool migration);/* Functions that require manual locking. */ void bdrv_dirty_bitmap_lock (BdrvDirtyBitmap *bitmap);
migration/block-dirty-bitmap.c +6 -5
@@ -295,6 +295,12 @@ static int init_dirty_bitmap_migration(void)continue ; } +/* Skip persistant bitmaps, unless it's a block migration: */ +if (bdrv_dirty_bitmap_get_persistance (bitmap) && + !migrate_use_block ()) { +continue ; + } +if (drive_name ==NULL ) {error_report ("Found bitmap '%s' in unnamed node %p. It can't " "be migrated" ,bdrv_dirty_bitmap_name (bitmap), bs);
@@ -335,11 +341,6 @@ static int init_dirty_bitmap_migration(void) } } -/* unset persistance here, to not roll back it */ - QSIMPLEQ_FOREACH(dbms, &dirty_bitmap_mig_state.dbms_list, entry) { -bdrv_dirty_bitmap_set_persistance (dbms->bitmap, false); - } -if (QSIMPLEQ_EMPTY (&dirty_bitmap_mig_state.dbms_list)) { dirty_bitmap_mig_state.no_bitmaps = true; }
Signature · 1 line
2.14.4
PATCH 6/7 iotests: improve 169
In reply to John SnowFrom: Vladimir Sementsov-Ogievskiy <[email protected]>
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.
--- tests/qemu-iotests/169 | 11 +++++++++++ 1 file changed, 11 insertions(+)
tests/qemu-iotests/169 +11 -0
@@ -24,6 +24,7 @@ import time import itertools import operator import new +import re from iotests import qemu_img
@@ -136,6 +137,16 @@ 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'Receiving block device images\n', '', log) + log = re.sub(r'Completed \d+ %\r?\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')
Signature · 1 line
2.14.4
PATCH 7/7 iotests: 169: add cases for source vm resuming
In reply to John SnowFrom: Vladimir Sementsov-Ogievskiy <[email protected]>
Test that we can resume source vm after [failed] migration, and bitmaps are ok.
--- tests/qemu-iotests/169 | 59 +++++++++++++++++++++++++++++++++++++++++++++- tests/qemu-iotests/169.out | 4 ++-- 2 files changed, 60 insertions(+), 3 deletions(-)
tests/qemu-iotests/169 +58 -1
@@ -77,6 +77,57 @@ 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 + self.check_bitmap(self.vm_a, False if persistent 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
@@ -157,7 +208,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_'
@@ -168,6 +219,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'])
tests/qemu-iotests/169.out +2 -2
@@ -1,5 +1,5 @@ -................ +.................... ---------------------------------------------------------------------- -Ran 16 tests +Ran 20 tests OK
Signature · 1 line
2.14.4
Quoted 26 lines · John Snow
This is an updated version of Vladimir's proposal for fixing the handling around migration and persistent dirty bitmaps.
Patches 1, 4, 6, and 7 update the testing for this feature. Patch 2 touches up an error message.
Patch 3 removes dead code.
Patch 5 contains the real fix.v2:
- Add a new patch 4 as a prerequisite for what's now patch 5
- Rework the fix to be (hopefully) cleaner, see patch 5 notes
- Adjust error message in patch 2 (Eric)
- Adjust test logic slightly (patches 6, 7) to deal with changes in patch 5.John Snow (2):
iotests: 169: actually test block migration
dirty-bitmaps: clean-up bitmaps loading and migration logicVladimir Sementsov-Ogievskiy (5):
iotests: 169: drop deprecated 'autoload' parameter block/qcow2: improve error message inqcow2_inactivateblock/qcow2: dropdirty_bitmaps_loadedstate variable
iotests: improve 169
iotests: 169: add cases for source vm resuming
Quoted 10 lines
block.c | 4 --- block/dirty-bitmap.c | 20 ------------ block/qcow2-bitmap.c | 16 +++++++++ block/qcow2.c | 26 ++++----------- block/qcow2.h | 1 - include/block/dirty-bitmap.h | 2 +- migration/block-dirty-bitmap.c | 11 ++++--- tests/qemu-iotests/169 | 74 ++++++++++++++++++++++++++++++++++++++++-- tests/qemu-iotests/169.out | 4 +-- 9 files changed, 103 insertions(+), 55 deletions(-)
Quoting John Snow (2018-07-23 17:22:03)
Quoted 2 lines
This is an updated version of Vladimir's proposal for fixing the handling around migration and persistent dirty bitmaps.
Are these still being considered for 3.0 rc3/rc4? 2.12.1 releases this week and I'm not sure how badly these are needed.
Quoted 39 lines
Patches 1, 4, 6, and 7 update the testing for this feature. Patch 2 touches up an error message.
Patch 3 removes dead code.
Patch 5 contains the real fix.v2:
- Add a new patch 4 as a prerequisite for what's now patch 5
- Rework the fix to be (hopefully) cleaner, see patch 5 notes
- Adjust error message in patch 2 (Eric)
- Adjust test logic slightly (patches 6, 7) to deal with changes in patch 5.John Snow (2):
iotests: 169: actually test block migration
dirty-bitmaps: clean-up bitmaps loading and migration logicVladimir Sementsov-Ogievskiy (5):
iotests: 169: drop deprecated 'autoload' parameter block/qcow2: improve error message inqcow2_inactivateblock/qcow2: dropdirty_bitmaps_loadedstate variable
iotests: improve 169
iotests: 169: add cases for source vm resumingblock.c | 4 --- block/dirty-bitmap.c | 20 ------------ block/qcow2-bitmap.c | 16 +++++++++ block/qcow2.c | 26 ++++----------- block/qcow2.h | 1 - include/block/dirty-bitmap.h | 2 +- migration/block-dirty-bitmap.c | 11 ++++--- tests/qemu-iotests/169 | 74 ++++++++++++++++++++++++++++++++++++++++-- tests/qemu-iotests/169.out | 4 +-- 9 files changed, 103 insertions(+), 55 deletions(-)Signature · 1 line
2.14.4
Quoted 7 lines · Michael Roth
Quoting John Snow (2018-07-23 17:22:03)
Quoted 2 lines
This is an updated version of Vladimir's proposal for fixing the handling around migration and persistent dirty bitmaps.
Are these still being considered for 3.0 rc3/rc4? 2.12.1 releases this week and I'm not sure how badly these are needed.
Good question. I'd still LIKE them in 3.0, but further fixes are actually still needed, and I'm working on this right now.
I think you'll find that you might have trouble applying them to 2.12.1 without a fairly long trail of prerequisites, so it might not be easily plausible.
Quoted 39 lines
Quoted 39 lines
Patches 1, 4, 6, and 7 update the testing for this feature. Patch 2 touches up an error message.
Patch 3 removes dead code.
Patch 5 contains the real fix.v2:
- Add a new patch 4 as a prerequisite for what's now patch 5
- Rework the fix to be (hopefully) cleaner, see patch 5 notes
- Adjust error message in patch 2 (Eric)
- Adjust test logic slightly (patches 6, 7) to deal with changes in patch 5.John Snow (2):
iotests: 169: actually test block migration
dirty-bitmaps: clean-up bitmaps loading and migration logicVladimir Sementsov-Ogievskiy (5):
iotests: 169: drop deprecated 'autoload' parameter block/qcow2: improve error message inqcow2_inactivateblock/qcow2: dropdirty_bitmaps_loadedstate variable
iotests: improve 169
iotests: 169: add cases for source vm resumingblock.c | 4 --- block/dirty-bitmap.c | 20 ------------ block/qcow2-bitmap.c | 16 +++++++++ block/qcow2.c | 26 ++++----------- block/qcow2.h | 1 - include/block/dirty-bitmap.h | 2 +- migration/block-dirty-bitmap.c | 11 ++++--- tests/qemu-iotests/169 | 74 ++++++++++++++++++++++++++++++++++++++++-- tests/qemu-iotests/169.out | 4 +-- 9 files changed, 103 insertions(+), 55 deletions(-)Signature · 1 line
2.14.4