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
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.
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>
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
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
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
© 2016 - 2024 Red Hat, Inc.