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

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

[Vladimir Sementsov-Ogievskiy]

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

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

[Eric Blake]

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

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