migration/block-dirty-bitmap.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
We shouldn't fail, if found unnamed bitmap in a unnamed node or node
with auto-generated node name, as bitmap migration ignores such bitmaps
at all.
Fixes: 82640edb88faa
Fixes: 4ff5cc121b089
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
migration/block-dirty-bitmap.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c
index 47bc0f650c..b0dbf9eeed 100644
--- a/migration/block-dirty-bitmap.c
+++ b/migration/block-dirty-bitmap.c
@@ -274,7 +274,11 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name)
DirtyBitmapMigBitmapState *dbms;
Error *local_err = NULL;
- bitmap = bdrv_dirty_bitmap_first(bs);
+ FOR_EACH_DIRTY_BITMAP(bs, bitmap) {
+ if (bdrv_dirty_bitmap_name(bitmap)) {
+ break;
+ }
+ }
if (!bitmap) {
return 0;
}
--
2.18.0
On 6/26/20 8:06 AM, Vladimir Sementsov-Ogievskiy wrote: > We shouldn't fail, if found unnamed bitmap in a unnamed node or node We shouldn't fail when finding an unnamed > with auto-generated node name, as bitmap migration ignores such bitmaps > at all. such bitmaps in the first place. > > Fixes: 82640edb88faa > Fixes: 4ff5cc121b089 > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > migration/block-dirty-bitmap.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > Reviewed-by: Eric Blake <eblake@redhat.com> Will queue through the bitmaps tree. Is this easy enough to reproduce that it would be worth having iotest coverage? > diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c > index 47bc0f650c..b0dbf9eeed 100644 > --- a/migration/block-dirty-bitmap.c > +++ b/migration/block-dirty-bitmap.c > @@ -274,7 +274,11 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name) > DirtyBitmapMigBitmapState *dbms; > Error *local_err = NULL; > > - bitmap = bdrv_dirty_bitmap_first(bs); > + FOR_EACH_DIRTY_BITMAP(bs, bitmap) { > + if (bdrv_dirty_bitmap_name(bitmap)) { > + break; > + } > + } > if (!bitmap) { > return 0; > } > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3226 Virtualization: qemu.org | libvirt.org
26.06.2020 17:34, Eric Blake wrote: > On 6/26/20 8:06 AM, Vladimir Sementsov-Ogievskiy wrote: >> We shouldn't fail, if found unnamed bitmap in a unnamed node or node > > We shouldn't fail when finding an unnamed > >> with auto-generated node name, as bitmap migration ignores such bitmaps >> at all. > > such bitmaps in the first place. > >> >> Fixes: 82640edb88faa >> Fixes: 4ff5cc121b089 >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> migration/block-dirty-bitmap.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> > > Reviewed-by: Eric Blake <eblake@redhat.com> > > Will queue through the bitmaps tree. > > Is this easy enough to reproduce that it would be worth having iotest coverage? I don't think it worth it. This all is so bad, I only hope for the future of explicit mapping. This patch just fixes wrong code. Actually, today I've committed to downstream also a change, which when see auto-generated node-name a few lines above just returns 0 instead of an error, otherwise, in some specific case, bitmaps which were in backing files break migrations (and when skipped, they migrate through storage, as our specific case is storage migration).. Don't know, how much it applies to upstream. I've already said about my idea of "lazy" disabled bitmap: we shouldn't load them all on start, but on demand. Actually same thing may be said about bitmaps in read-only nodes. And of course migrating bitmaps from r-o backing chain, when it is shared-migration is nonsense. > >> diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c >> index 47bc0f650c..b0dbf9eeed 100644 >> --- a/migration/block-dirty-bitmap.c >> +++ b/migration/block-dirty-bitmap.c >> @@ -274,7 +274,11 @@ static int add_bitmaps_to_list(BlockDriverState *bs, const char *bs_name) >> DirtyBitmapMigBitmapState *dbms; >> Error *local_err = NULL; >> - bitmap = bdrv_dirty_bitmap_first(bs); >> + FOR_EACH_DIRTY_BITMAP(bs, bitmap) { >> + if (bdrv_dirty_bitmap_name(bitmap)) { >> + break; >> + } >> + } >> if (!bitmap) { >> return 0; >> } >> > Here is my today's downstream fix: diff --git a/migration/block-dirty-bitmap.c b/migration/block-dirty-bitmap.c index e8ce791278..66f5d6365a 100644 --- a/migration/block-dirty-bitmap.c +++ b/migration/block-dirty-bitmap.c @@ -312,10 +312,12 @@ static int add_bitmaps_to_list(DBMSaveState *s, } if (bs_name[0] == '#') { - error_report("Found bitmap '%s' in a node with auto-generated " - "name: %s. It can't be migrated", - bdrv_dirty_bitmap_name(bitmap), bs_name); - return -1; + /* + * Reporting error here breaks shared migration of dispatcher-created + * bitmaps. In u13 we just ignored them (ignored all bitmaps in backing + * files). So, just ignore the node. + */ + return 0; } for ( ; bitmap; bitmap = bdrv_dirty_bitmap_next(bs, bitmap)) { I can send a patch, if you think it worth it. For me it seems too hacky and too specific. With this change, bitmaps in backing chain can be migrated though shared-storage when dirty-bitmaps capability is enabled. (for clarity: u13 didn't have patch 592203e7cfbd1a "migration/dirty-bitmaps: change bitmap enumeration method", and in u14 we've backported it) -- Best regards, Vladimir
© 2016 - 2024 Red Hat, Inc.