Re: [PATCH] migration/block-dirty-bitmap: fix add_bitmaps_to_list

[Vladimir Sementsov-Ogievskiy]

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