fix bitmaps migration through shared storage
v3 cover fix bitmaps migration through shared storage
Hi all.
This fixes bitmaps migration through shared storage. Look at 02 for details.
The bug introduced in 2.10 with the whole qcow2 bitmaps feature, so qemu-stable in CC. However I doubt that someone really suffered from this.
Do we need dirty bitmaps at all in inactive case? - that was a question in v2. And, keeping in mind that we are going to use inactive mode not only for incoming migration, I'm not sure that answer is NO (but, it may be "NO" for 2.10, 2.11), so let's fix it in proposed here manner at least for 2.12.
v3: tiny context changes in 01,02
03: instead of a separate test, enable corresponding case in existent one
v2:
John, thank you for reviewing v1.
changes:
add John's r-bs, change s/timeout=10/timeout=10.0/ in last patch and drop old 03 patch, related to this timeout fix.
Vladimir Sementsov-Ogievskiy (3):
qcow2-bitmap: add qcow2_reopen_bitmaps_rw_hint()
qcow2: handle reopening bitmaps on bdrv_invalidate_cache
iotests: enable shared migration cases in 169
block/qcow2.h | 2 ++ block/qcow2-bitmap.c | 15 ++++++++++++++- block/qcow2.c | 8 +++++++- tests/qemu-iotests/169 | 8 +++----- 4 files changed, 26 insertions(+), 7 deletions(-)
Signature · 1 line
2.11.1
PATCH v3 1/3 qcow2-bitmap: add qcow2_reopen_bitmaps_rw_hint()
Add version of qcow2_reopen_bitmaps_rw, which do the same work but also return a hint about was header updated or not. This will be used in the following fix for bitmaps reloading after migration.
--- block/qcow2.h | 2 ++ block/qcow2-bitmap.c | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-)
block/qcow2.h +2 -0
@@ -671,6 +671,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,void **refcount_table, int64_t *refcount_table_size);bool qcow2_load_dirty_bitmaps (BlockDriverState *bs,Error **errp); +int qcow2_reopen_bitmaps_rw_hint (BlockDriverState *bs,bool *header_updated, +Error **errp);int qcow2_reopen_bitmaps_rw (BlockDriverState *bs,Error **errp);void qcow2_store_persistent_dirty_bitmaps (BlockDriverState *bs,Error **errp);int qcow2_reopen_bitmaps_ro (BlockDriverState *bs,Error **errp);
block/qcow2-bitmap.c +14 -1
@@ -1004,7 +1004,8 @@ fail:return false; } -int qcow2_reopen_bitmaps_rw (BlockDriverState *bs,Error **errp) +int qcow2_reopen_bitmaps_rw_hint (BlockDriverState *bs,bool *header_updated, +Error **errp) {BDRVQcow2State *s = bs->opaque;Qcow2BitmapList *bm_list;
@@ -1012,6 +1013,10 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)GSList *ro_dirty_bitmaps =NULL ;int ret =0 ; +if (header_updated !=NULL ) { + *header_updated = false; + } +if (s->nb_bitmaps ==0 ) {/* No bitmaps - nothing to do */ return 0 ;
@@ -1055,6 +1060,9 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)error_setg_errno (errp, -ret,"Can't update bitmap directory" ); goto out; } +if (header_updated !=NULL ) { + *header_updated = true; + }g_slist_foreach (ro_dirty_bitmaps, set_readonly_helper, false); }
@@ -1065,6 +1073,11 @@ out:return ret; } +int qcow2_reopen_bitmaps_rw (BlockDriverState *bs,Error **errp) +{ +return qcow2_reopen_bitmaps_rw_hint (bs,NULL , errp); +} +/* store_bitmap_data() * Store bitmap to image, filling bitmap table accordingly. */
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH v3 1/3] qcow2-bitmap: add qcow2_reopen_bitmaps_rw_hint()
Quoted 10 lines · Vladimir Sementsov-Ogievskiy
Add version of
qcow2_reopen_bitmaps_rw, which do the same work but also return a hint about was header updated or not. This will be used in the following fix for bitmaps reloading after migration.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>Reviewed-by John Snow <[email protected]>--- block/qcow2.h | 2 ++ block/qcow2-bitmap.c | 15 ++++++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-)
PATCH v3 2/3 qcow2: handle reopening bitmaps on bdrv_invalidate_cache
In reply to Vladimir Sementsov-OgievskiyConsider migration with shared storage. Persistent bitmaps are stored on bdrv_inactivate. Then, on destination process_incoming_migration_bh() calls bdrv_invalidate_cache_all() which leads to qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps are already loaded on destination start. In this case we should call qcow2_reopen_bitmaps_rw instead.
--- block/qcow2.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
block/qcow2.c +7 -1
@@ -1480,7 +1480,13 @@ 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)) { +if (bdrv_has_readonly_bitmaps (bs)) { +if (!bdrv_is_read_only (bs) && !(bdrv_get_flags (bs) &BDRV_O_INACTIVE )) { +bool header_updated = false; +qcow2_reopen_bitmaps_rw_hint (bs, &header_updated, &local_err); + update_header = update_header && !header_updated; + } + }else if (qcow2_load_dirty_bitmaps (bs, &local_err)) { update_header = false; }if (local_err !=NULL ) {
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH v3 2/3] qcow2: handle reopening bitmaps on bdrv_invalidate_cache
Quoted 18 lines · Vladimir Sementsov-Ogievskiy
Consider migration with shared storage. Persistent bitmaps are stored on
bdrv_inactivate. Then, on destinationprocess_incoming_migration_bh()callsbdrv_invalidate_cache_all()which leads toqcow2_load_autoloading_dirty_bitmaps()which fails if bitmaps are already loaded on destination start. In this case we should callqcow2_reopen_bitmaps_rwinstead.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>Reviewed-by John Snow <[email protected]>--- block/qcow2.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)block/qcow2.c +0
-0
block/qcow2.c static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, · line 1483
Quoted 5 lines
block/qcow2.c +1
-1@@ -1480,7 +1480,13 @@ 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)) { +if (bdrv_has_readonly_bitmaps (bs)) {
So if there are any read-only bitmaps, we'll skip qcow2_load_dirty_bitmaps() altogether. That doesn't seem so bad because qcow2_load_dirty_bitmaps() seems to assume no bitmaps have been loaded yet.
But if that's the case, shouldn't we skip that function if any bitmap has been loaded already, RO or R/W? (And we can call qcow2_reopen_bitmaps_rw_hint() even if there aren't any RO bitmaps, it just won't do anything then.)
This doesn't make this patch really wrong, I'm just wondering whether it can do better. (To add to that, I don't even know whether there is a case where qcow2_do_open() would be called with any R/W bitmaps already present.)
Max
Quoted 10 lines
block/qcow2.c +6
-0+if (!bdrv_is_read_only (bs) && !(bdrv_get_flags (bs) &BDRV_O_INACTIVE )) { +bool header_updated = false; +qcow2_reopen_bitmaps_rw_hint (bs, &header_updated, &local_err); + update_header = update_header && !header_updated; + } + }else if (qcow2_load_dirty_bitmaps (bs, &local_err)) { update_header = false; }if (local_err !=NULL ) {
Re: [Qemu-devel] [PATCH v3 2/3] qcow2: handle reopening bitmaps on bdrv_invalidate_cache
Quoted 33 lines · Max Reitz
Quoted 18 lines · Vladimir Sementsov-Ogievskiy
Consider migration with shared storage. Persistent bitmaps are stored on
bdrv_inactivate. Then, on destinationprocess_incoming_migration_bh()callsbdrv_invalidate_cache_all()which leads toqcow2_load_autoloading_dirty_bitmaps()which fails if bitmaps are already loaded on destination start. In this case we should callqcow2_reopen_bitmaps_rwinstead.Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>Reviewed-by John Snow <[email protected]>--- block/qcow2.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)block/qcow2.c +0
-0block/qcow2.c static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options, · line 1483
Quoted 5 lines
block/qcow2.c +1
-1@@ -1480,7 +1480,13 @@ 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)) { +if (bdrv_has_readonly_bitmaps (bs)) {So if there are any read-only bitmaps, we'll skip
qcow2_load_dirty_bitmaps()altogether. That doesn't seem so bad becauseqcow2_load_dirty_bitmaps()seems to assume no bitmaps have been loaded yet.But if that's the case, shouldn't we skip that function if any bitmap has been loaded already, RO or R/W? (And we can call
qcow2_reopen_bitmaps_rw_hint()even if there aren't any RO bitmaps, it just won't do anything then.)
Quoted 6 lines
This doesn't make this patch really wrong, I'm just wondering whether it can do better. (To add to that, I don't even know whether there is a case where
qcow2_do_open()would be called with any R/W bitmaps already present.)Max
hm reasonable. So, we don't know about such cases, it's better to don't allow them.
Quoted 11 lines
Quoted 11 lines
block/qcow2.c +6
-0+if (!bdrv_is_read_only (bs) && !(bdrv_get_flags (bs) &BDRV_O_INACTIVE )) { +bool header_updated = false; +qcow2_reopen_bitmaps_rw_hint (bs, &header_updated, &local_err); + update_header = update_header && !header_updated; + } + }else if (qcow2_load_dirty_bitmaps (bs, &local_err)) { update_header = false; }if (local_err !=NULL ) {
Signature · 2 lines
Best regards, Vladimir
PATCH v3 3/3 iotests: enable shared migration cases in 169
In reply to Vladimir Sementsov-OgievskiyShared migration for dirty bitmaps is fixed by previous patches, so we can enable the test.
--- tests/qemu-iotests/169 | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
tests/qemu-iotests/169 +3 -5
@@ -140,16 +140,14 @@ def inject_test_case(klass, name, method, *args, **kwargs): mc = operator.methodcaller(method, *args, **kwargs) setattr(klass, 'test_' + name, new.instancemethod(mc, None, klass)) -for cmb in list(itertools.product((True, False), repeat=3)): +for cmb in list(itertools.product((True, False), repeat=4)): name = ('_' if cmb[0] else '_not_') + 'persistent_' name += ('_' if cmb[1] else '_not_') + 'migbitmap_' name += '_online' if cmb[2] else '_offline' - - # TODO fix shared-storage bitmap migration and enable cases for it - args = list(cmb) + [False] + name += '_shared' if cmb[3] else '_nonshared' inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration', - *args) + *list(cmb)) if __name__ == '__main__':
Signature · 1 line
2.11.1
Re: [Qemu-devel] [PATCH v3 3/3] iotests: enable shared migration cases in 169
Quoted 7 lines · Vladimir Sementsov-Ogievskiy
Shared migration for dirty bitmaps is fixed by previous patches, so we can enable the test.
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- tests/qemu-iotests/169 | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)
Isn't this missing some changes to 169.out (because the number of tests is doubled)?
Max
Re: [Qemu-devel] [PATCH v3 3/3] iotests: enable shared migration cases in 169
Quoted 4 lines · Max Reitz
Quoted 3 lines · Vladimir Sementsov-Ogievskiy
Shared migration for dirty bitmaps is fixed by previous patches, so we can enable the test.
Quoted 8 lines
Quoted 4 lines
Signed-off-by Vladimir Sementsov-Ogievskiy <[email protected]>--- tests/qemu-iotests/169 | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)Isn't this missing some changes to 169.out (because the number of tests is doubled)?
Max
Oh, yes, forget to add it.
Signature · 2 lines
Best regards, Vladimir