* [Qemu-devel] [PATCH] block/qcow2: fix logic around dirty_bitmaps_loaded @ 2018-06-12 17:26 Vladimir Sementsov-Ogievskiy 2018-06-12 22:11 ` [Qemu-devel] [Qemu-block] " John Snow 2018-06-21 18:26 ` [Qemu-devel] drop " Vladimir Sementsov-Ogievskiy 0 siblings, 2 replies; 6+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2018-06-12 17:26 UTC (permalink / raw) To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, den, vsementsov First: this variable was introduced to handle reopens. We need it on following qcow2_do_open, to don't try loading bitmaps again. So, we are fixing 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 <vsementsov@virtuozzo.com> --- 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; -- 2.11.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block/qcow2: fix logic around dirty_bitmaps_loaded 2018-06-12 17:26 [Qemu-devel] [PATCH] block/qcow2: fix logic around dirty_bitmaps_loaded Vladimir Sementsov-Ogievskiy @ 2018-06-12 22:11 ` John Snow 2018-06-12 22:18 ` John Snow 2018-06-21 18:26 ` [Qemu-devel] drop " Vladimir Sementsov-Ogievskiy 1 sibling, 1 reply; 6+ messages in thread From: John Snow @ 2018-06-12 22:11 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block; +Cc: kwolf, den, mreitz On 06/12/2018 01:26 PM, Vladimir Sementsov-Ogievskiy wrote: > First: this variable was introduced to handle reopens. We need it on > following qcow2_do_open, to don't try loading bitmaps again. So, we are > fixing 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 <vsementsov@virtuozzo.com> > --- > 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. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block/qcow2: fix logic around dirty_bitmaps_loaded 2018-06-12 22:11 ` [Qemu-devel] [Qemu-block] " John Snow @ 2018-06-12 22:18 ` John Snow 2018-06-20 17:17 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 6+ messages in thread From: John Snow @ 2018-06-12 22:18 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block; +Cc: kwolf, den, mreitz 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 on >> following qcow2_do_open, to don't try loading bitmaps again. So, we are >> fixing 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 <vsementsov@virtuozzo.com> >> --- >> 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 <jsnow@redhat.com> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block/qcow2: fix logic around dirty_bitmaps_loaded 2018-06-12 22:18 ` John Snow @ 2018-06-20 17:17 ` Vladimir Sementsov-Ogievskiy 2018-06-20 17:20 ` Vladimir Sementsov-Ogievskiy 0 siblings, 1 reply; 6+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2018-06-20 17:17 UTC (permalink / raw) To: John Snow, qemu-devel, qemu-block; +Cc: kwolf, den, mreitz 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 on >>> following qcow2_do_open, to don't try loading bitmaps again. So, we are >>> fixing 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 <vsementsov@virtuozzo.com> >>> --- >>> 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 <jsnow@redhat.com> 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 see qemu-system-x86_64: Could not reopen qcow2 layer: Bitmap already exists: bitmap0 which leads to failed invalidation and bs->drv = NULL :) I'll improve iotest somehow, to show this failure. -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH] block/qcow2: fix logic around dirty_bitmaps_loaded 2018-06-20 17:17 ` Vladimir Sementsov-Ogievskiy @ 2018-06-20 17:20 ` Vladimir Sementsov-Ogievskiy 0 siblings, 0 replies; 6+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2018-06-20 17:20 UTC (permalink / raw) To: John Snow, qemu-devel, qemu-block; +Cc: kwolf, den, mreitz 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 on >>>> following qcow2_do_open, to don't try loading bitmaps again. So, we >>>> are >>>> fixing 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 <vsementsov@virtuozzo.com> >>>> --- >>>> 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 <jsnow@redhat.com> > > 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 see > > qemu-system-x86_64: Could not reopen qcow2 layer: Bitmap already > exists: bitmap0 > > which 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Qemu-devel] drop Re: [PATCH] block/qcow2: fix logic around dirty_bitmaps_loaded 2018-06-12 17:26 [Qemu-devel] [PATCH] block/qcow2: fix logic around dirty_bitmaps_loaded Vladimir Sementsov-Ogievskiy 2018-06-12 22:11 ` [Qemu-devel] [Qemu-block] " John Snow @ 2018-06-21 18:26 ` Vladimir Sementsov-Ogievskiy 1 sibling, 0 replies; 6+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2018-06-21 18:26 UTC (permalink / raw) To: qemu-devel, qemu-block; +Cc: mreitz, kwolf, den 12.06.2018 20:26, Vladimir Sementsov-Ogievskiy wrote: > First: this variable was introduced to handle reopens. We need it on > following qcow2_do_open, to don't try loading bitmaps again. So, we are > fixing 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 <vsementsov@virtuozzo.com> > --- > 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; Drop this, I'll resend better patch. -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-06-21 18:26 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-12 17:26 [Qemu-devel] [PATCH] block/qcow2: fix logic around dirty_bitmaps_loaded Vladimir Sementsov-Ogievskiy 2018-06-12 22:11 ` [Qemu-devel] [Qemu-block] " John Snow 2018-06-12 22:18 ` John Snow 2018-06-20 17:17 ` Vladimir Sementsov-Ogievskiy 2018-06-20 17:20 ` Vladimir Sementsov-Ogievskiy 2018-06-21 18:26 ` [Qemu-devel] drop " Vladimir Sementsov-Ogievskiy
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.