[Qemu-devel] [PATCH v3 for 2.12 0/3] fix bitmaps migration through shared storage

[Qemu-devel] [PATCH v3 for 2.12 0/3] fix bitmaps migration through shared storage

[Vladimir Sementsov-Ogievskiy]

Hi all.

This fixes bitmaps migration through shared storage. Look at 02 for
details.

The bug introduced in 2.10 with the whole qcow2 bitmaps feature, so
qemu-stable in CC. However I doubt that someone really suffered from this.

Do we need dirty bitmaps at all in inactive case? - that was a question in v2.
And, keeping in mind that we are going to use inactive mode not only for
incoming migration, I'm not sure that answer is NO (but, it may be "NO" for 
2.10, 2.11), so let's fix it in proposed here manner at least for 2.12.

v3: tiny context changes in 01,02
    03: instead of a separate test, enable corresponding case in existent one

v2:
   John, thank you for reviewing v1.
   changes:
    add John's r-bs, change s/timeout=10/timeout=10.0/ in last patch
    and drop old 03 patch, related to this timeout fix.

Vladimir Sementsov-Ogievskiy (3):
  qcow2-bitmap: add qcow2_reopen_bitmaps_rw_hint()
  qcow2: handle reopening bitmaps on bdrv_invalidate_cache
  iotests: enable shared migration cases in 169

 block/qcow2.h          |  2 ++
 block/qcow2-bitmap.c   | 15 ++++++++++++++-
 block/qcow2.c          |  8 +++++++-
 tests/qemu-iotests/169 |  8 +++-----
 4 files changed, 26 insertions(+), 7 deletions(-)

-- 
2.11.1

[Qemu-devel] [PATCH v3 1/3] qcow2-bitmap: add qcow2_reopen_bitmaps_rw_hint()

[Vladimir Sementsov-Ogievskiy]

Add version of qcow2_reopen_bitmaps_rw, which do the same work but
also return a hint about was header updated or not. This will be
used in the following fix for bitmaps reloading after migration.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 block/qcow2.h        |  2 ++
 block/qcow2-bitmap.c | 15 ++++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index ccb92a9696..d301f77cea 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -671,6 +671,8 @@ int qcow2_check_bitmaps_refcounts(BlockDriverState *bs, BdrvCheckResult *res,
                                   void **refcount_table,
                                   int64_t *refcount_table_size);
 bool qcow2_load_dirty_bitmaps(BlockDriverState *bs, Error **errp);
+int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
+                                 Error **errp);
 int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp);
 void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs, Error **errp);
 int qcow2_reopen_bitmaps_ro(BlockDriverState *bs, Error **errp);
diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 3010adb909..6e93ec43e1 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1004,7 +1004,8 @@ fail:
     return false;
 }
 
-int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
+int qcow2_reopen_bitmaps_rw_hint(BlockDriverState *bs, bool *header_updated,
+                                 Error **errp)
 {
     BDRVQcow2State *s = bs->opaque;
     Qcow2BitmapList *bm_list;
@@ -1012,6 +1013,10 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
     GSList *ro_dirty_bitmaps = NULL;
     int ret = 0;
 
+    if (header_updated != NULL) {
+        *header_updated = false;
+    }
+
     if (s->nb_bitmaps == 0) {
         /* No bitmaps - nothing to do */
         return 0;
@@ -1055,6 +1060,9 @@ int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
             error_setg_errno(errp, -ret, "Can't update bitmap directory");
             goto out;
         }
+        if (header_updated != NULL) {
+            *header_updated = true;
+        }
         g_slist_foreach(ro_dirty_bitmaps, set_readonly_helper, false);
     }
 
@@ -1065,6 +1073,11 @@ out:
     return ret;
 }
 
+int qcow2_reopen_bitmaps_rw(BlockDriverState *bs, Error **errp)
+{
+    return qcow2_reopen_bitmaps_rw_hint(bs, NULL, errp);
+}
+
 /* store_bitmap_data()
  * Store bitmap to image, filling bitmap table accordingly.
  */
-- 
2.11.1

Re: [Qemu-devel] [PATCH v3 1/3] qcow2-bitmap: add qcow2_reopen_bitmaps_rw_hint()

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 574 bytes --]

On 2018-03-19 10:02, Vladimir Sementsov-Ogievskiy wrote:
> Add version of qcow2_reopen_bitmaps_rw, which do the same work but
> also return a hint about was header updated or not. This will be
> used in the following fix for bitmaps reloading after migration.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
>  block/qcow2.h        |  2 ++
>  block/qcow2-bitmap.c | 15 ++++++++++++++-
>  2 files changed, 16 insertions(+), 1 deletion(-)

Reviewed-by: Max Reitz <mreitz@redhat.com>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

[Qemu-devel] [PATCH v3 2/3] qcow2: handle reopening bitmaps on bdrv_invalidate_cache

[Vladimir Sementsov-Ogievskiy]

Consider migration with shared storage. Persistent bitmaps are stored
on bdrv_inactivate. Then, on destination
process_incoming_migration_bh() calls bdrv_invalidate_cache_all() which
leads to qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps
are already loaded on destination start. In this case we should call
qcow2_reopen_bitmaps_rw instead.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Reviewed-by: John Snow <jsnow@redhat.com>
---
 block/qcow2.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/block/qcow2.c b/block/qcow2.c
index 7472af6931..6219666d4a 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1480,7 +1480,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
         s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
     }
 
-    if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
+    if (bdrv_has_readonly_bitmaps(bs)) {
+        if (!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
+            bool header_updated = false;
+            qcow2_reopen_bitmaps_rw_hint(bs, &header_updated, &local_err);
+            update_header = update_header && !header_updated;
+        }
+    } else if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
         update_header = false;
     }
     if (local_err != NULL) {
-- 
2.11.1

Re: [Qemu-devel] [PATCH v3 2/3] qcow2: handle reopening bitmaps on bdrv_invalidate_cache

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 2169 bytes --]

On 2018-03-19 10:02, Vladimir Sementsov-Ogievskiy wrote:
> Consider migration with shared storage. Persistent bitmaps are stored
> on bdrv_inactivate. Then, on destination
> process_incoming_migration_bh() calls bdrv_invalidate_cache_all() which
> leads to qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps
> are already loaded on destination start. In this case we should call
> qcow2_reopen_bitmaps_rw instead.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> Reviewed-by: John Snow <jsnow@redhat.com>
> ---
>  block/qcow2.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7472af6931..6219666d4a 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1480,7 +1480,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>          s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
>      }
>  
> -    if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
> +    if (bdrv_has_readonly_bitmaps(bs)) {

So if there are any read-only bitmaps, we'll skip
qcow2_load_dirty_bitmaps() altogether.  That doesn't seem so bad because
qcow2_load_dirty_bitmaps() seems to assume no bitmaps have been loaded yet.

But if that's the case, shouldn't we skip that function if any bitmap
has been loaded already, RO or R/W?  (And we can call
qcow2_reopen_bitmaps_rw_hint() even if there aren't any RO bitmaps, it
just won't do anything then.)

This doesn't make this patch really wrong, I'm just wondering whether it
can do better.  (To add to that, I don't even know whether there is a
case where qcow2_do_open() would be called with any R/W bitmaps already
present.)

Max

> +        if (!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
> +            bool header_updated = false;
> +            qcow2_reopen_bitmaps_rw_hint(bs, &header_updated, &local_err);
> +            update_header = update_header && !header_updated;
> +        }
> +    } else if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
>          update_header = false;
>      }
>      if (local_err != NULL) {
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v3 2/3] qcow2: handle reopening bitmaps on bdrv_invalidate_cache

[Vladimir Sementsov-Ogievskiy]

20.03.2018 16:44, Max Reitz wrote:
> On 2018-03-19 10:02, Vladimir Sementsov-Ogievskiy wrote:
>> Consider migration with shared storage. Persistent bitmaps are stored
>> on bdrv_inactivate. Then, on destination
>> process_incoming_migration_bh() calls bdrv_invalidate_cache_all() which
>> leads to qcow2_load_autoloading_dirty_bitmaps() which fails if bitmaps
>> are already loaded on destination start. In this case we should call
>> qcow2_reopen_bitmaps_rw instead.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> Reviewed-by: John Snow <jsnow@redhat.com>
>> ---
>>   block/qcow2.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/qcow2.c b/block/qcow2.c
>> index 7472af6931..6219666d4a 100644
>> --- a/block/qcow2.c
>> +++ b/block/qcow2.c
>> @@ -1480,7 +1480,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>>           s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
>>       }
>>   
>> -    if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
>> +    if (bdrv_has_readonly_bitmaps(bs)) {
> So if there are any read-only bitmaps, we'll skip
> qcow2_load_dirty_bitmaps() altogether.  That doesn't seem so bad because
> qcow2_load_dirty_bitmaps() seems to assume no bitmaps have been loaded yet.
>
> But if that's the case, shouldn't we skip that function if any bitmap
> has been loaded already, RO or R/W?  (And we can call
> qcow2_reopen_bitmaps_rw_hint() even if there aren't any RO bitmaps, it
> just won't do anything then.)
>
> This doesn't make this patch really wrong, I'm just wondering whether it
> can do better.  (To add to that, I don't even know whether there is a
> case where qcow2_do_open() would be called with any R/W bitmaps already
> present.)
>
> Max

hm reasonable. So, we don't know about such cases, it's better to don't 
allow them.

>
>> +        if (!bdrv_is_read_only(bs) && !(bdrv_get_flags(bs) & BDRV_O_INACTIVE)) {
>> +            bool header_updated = false;
>> +            qcow2_reopen_bitmaps_rw_hint(bs, &header_updated, &local_err);
>> +            update_header = update_header && !header_updated;
>> +        }
>> +    } else if (qcow2_load_dirty_bitmaps(bs, &local_err)) {
>>           update_header = false;
>>       }
>>       if (local_err != NULL) {
>>


-- 
Best regards,
Vladimir

[Qemu-devel] [PATCH v3 3/3] iotests: enable shared migration cases in 169

[Vladimir Sementsov-Ogievskiy]

Shared migration for dirty bitmaps is fixed by previous patches,
so we can enable the test.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 tests/qemu-iotests/169 | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
index 3a8db91f6f..153b10b6e7 100755
--- a/tests/qemu-iotests/169
+++ b/tests/qemu-iotests/169
@@ -140,16 +140,14 @@ def inject_test_case(klass, name, method, *args, **kwargs):
     mc = operator.methodcaller(method, *args, **kwargs)
     setattr(klass, 'test_' + name, new.instancemethod(mc, None, klass))
 
-for cmb in list(itertools.product((True, False), repeat=3)):
+for cmb in list(itertools.product((True, False), repeat=4)):
     name = ('_' if cmb[0] else '_not_') + 'persistent_'
     name += ('_' if cmb[1] else '_not_') + 'migbitmap_'
     name += '_online' if cmb[2] else '_offline'
-
-    # TODO fix shared-storage bitmap migration and enable cases for it
-    args = list(cmb) + [False]
+    name += '_shared' if cmb[3] else '_nonshared'
 
     inject_test_case(TestDirtyBitmapMigration, name, 'do_test_migration',
-                     *args)
+                     *list(cmb))
 
 
 if __name__ == '__main__':
-- 
2.11.1

Re: [Qemu-devel] [PATCH v3 3/3] iotests: enable shared migration cases in 169

[Max Reitz]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]

On 2018-03-19 10:02, Vladimir Sementsov-Ogievskiy wrote:
> Shared migration for dirty bitmaps is fixed by previous patches,
> so we can enable the test.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/169 | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)

Isn't this missing some changes to 169.out (because the number of tests
is doubled)?

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]

Re: [Qemu-devel] [PATCH v3 3/3] iotests: enable shared migration cases in 169

[Vladimir Sementsov-Ogievskiy]

20.03.2018 17:01, Max Reitz wrote:
> On 2018-03-19 10:02, Vladimir Sementsov-Ogievskiy wrote:
>> Shared migration for dirty bitmaps is fixed by previous patches,
>> so we can enable the test.
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/169 | 8 +++-----
>>   1 file changed, 3 insertions(+), 5 deletions(-)
> Isn't this missing some changes to 169.out (because the number of tests
> is doubled)?
>
> Max

Oh, yes, forget to add it.

-- 
Best regards,
Vladimir