|
From: | Vladimir Sementsov-Ogievskiy |
Subject: | Re: [Qemu-block] [PATCH] block/qcow2: fix logic around dirty_bitmaps_loaded |
Date: | Wed, 20 Jun 2018 20:20:58 +0300 |
User-agent: | Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 |
20.06.2018 20:17, Vladimir Sementsov-Ogievskiy wrote:
13.06.2018 01:18, John Snow wrote:On 06/12/2018 06:11 PM, John Snow wrote:On 06/12/2018 01:26 PM, Vladimir Sementsov-Ogievskiy wrote:First: this variable was introduced to handle reopens. We need it onfollowing qcow2_do_open, to don't try loading bitmaps again. So, we arefixing qcow2_invalidate_cache(). However, if we fix only qcow2_invalidate_cache, iotest 169 fails on case test__persistent__not_migbitmap__online_shared, because on first target open, source is not yet closed, and is not yet stored bitmaps, so, we are load nothing. And on second open, dirty_bitmaps_loaded is already true, but we have no bitmaps to reopen with qcow2_reopen_bitmaps_rw_hint(). So, we are fixing qcow2_do_open() too. Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden> --- block/qcow2.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/block/qcow2.c b/block/qcow2.c index 6fa5e1d71a..b4216a5830 100644 --- a/block/qcow2.c +++ b/block/qcow2.c@@ -1499,7 +1499,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,{qcow2_reopen_bitmaps_rw_hint(bs, &header_updated, &local_err);} - } else { + } else if (s->nb_bitmaps > 0) { header_updated = qcow2_load_dirty_bitmaps(bs, &local_err); s->dirty_bitmaps_loaded = true; }@@ -2178,6 +2178,7 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,QDict *options; Error *local_err = NULL; int ret; + bool dirty_bitmaps_loaded = s->dirty_bitmaps_loaded; /** Backing files are read-only which makes all of their metadata immutable, @@ -2190,6 +2191,7 @@ static void coroutine_fn qcow2_co_invalidate_cache(BlockDriverState *bs,qcow2_close(bs); memset(s, 0, sizeof(BDRVQcow2State)); + s->dirty_bitmaps_loaded = dirty_bitmaps_loaded; options = qdict_clone_shallow(bs->options); flags &= ~BDRV_O_INACTIVE;Ah, tch... I didn't realize that invalidate completely wipes the qcow2 metadata. I guess the other three fields, nb_bitmaps, bitmap_directory_size and bitmap_directory_offset get initialized again on re-open. (I suppose this means I need to rethink caching bm_list more carefully, too...) I think this looks correct mechanically.Oh, I meant: Reviewed-by: John Snow <address@hidden>It is interesting, this patch also fixes iotest 169. It's not broken formally, but if we look at vm output of vm_b after migration, we'll seeqemu-system-x86_64: Could not reopen qcow2 layer: Bitmap already exists: bitmap0which leads to failed invalidation and bs->drv = NULL :) I'll improve iotest somehow, to show this failure.
the bug can be shown with the following workaround: diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169 index f243db9955..6dce24d29f 100755 --- a/tests/qemu-iotests/169 +++ b/tests/qemu-iotests/169 @@ -131,9 +131,13 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase): break self.check_bitmap(self.vm_b, sha256 if should_migrate else False) + self.vm_b.hmp_qemu_io('drive0', 'write 0 512') if should_migrate: self.vm_b.shutdown() + print '====' + print self.vm_b.get_log() + print '===='# 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')@@ -152,7 +156,8 @@ for cmb in list(itertools.product((True, False), repeat=4)):
name += '_online' if cmb[2] else '_offline' name += '_shared' if cmb[3] else '_nonshared' - inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration', + if name == '_persistent__not_migbitmap__offline_shared':+ inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
*list(cmb)) -- Best regards, Vladimir
[Prev in Thread] | Current Thread | [Next in Thread] |