[Qemu-devel] [PATCH v2 0/2] bitmaps persistent and migration fixes

[Qemu-devel] [PATCH v2 0/2] bitmaps persistent and migration fixes

[Vladimir Sementsov-Ogievskiy]

v2:

01: new, proposed by Max
02: fix leaving cat processes

Vladimir Sementsov-Ogievskiy (2):
  qcow2: try load bitmaps only once
  iotests: fix 169

 block/qcow2.h          |  1 +
 block/qcow2.c          | 16 ++++++++--------
 tests/qemu-iotests/169 | 48 +++++++++++++++++++++++++++---------------------
 3 files changed, 36 insertions(+), 29 deletions(-)

-- 
2.11.1

[Qemu-devel] [PATCH v2 1/2] qcow2: try load bitmaps only once

[Vladimir Sementsov-Ogievskiy]

Checking reopen by existence of some bitmaps is wrong, as it may be
some other bitmaps, or on the other hand, user may remove bitmaps. This
criteria is bad. To simplify things and make behavior more predictable
let's just add a flag to remember, that we've already tried to load
bitmaps on open and do not want do it again.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2.h |  1 +
 block/qcow2.c | 16 ++++++++--------
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index d301f77cea..adf5c3950f 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -298,6 +298,7 @@ typedef struct BDRVQcow2State {
     uint32_t nb_bitmaps;
     uint64_t bitmap_directory_size;
     uint64_t bitmap_directory_offset;
+    bool dirty_bitmaps_loaded;
 
     int flags;
     int qcow_version;
diff --git a/block/qcow2.c b/block/qcow2.c
index 486f3e83b7..4242e99f1e 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -1142,6 +1142,7 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
     uint64_t ext_end;
     uint64_t l1_vm_state_index;
     bool update_header = false;
+    bool header_updated = false;
 
     ret = bdrv_pread(bs->file, 0, &header, sizeof(header));
     if (ret < 0) {
@@ -1480,10 +1481,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
         s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
     }
 
-    if (bdrv_dirty_bitmap_next(bs, NULL)) {
-        /* It's some kind of reopen with already existing dirty bitmaps. There
-         * are no known cases where we need loading bitmaps in such situation,
-         * so it's safer don't load them.
+    if (s->dirty_bitmaps_loaded) {
+        /* It's some kind of reopen. There are no known cases where we need
+         * loading bitmaps in such situation, so it's safer don't load them.
          *
          * Moreover, if we have some readonly bitmaps and we are reopening for
          * rw we should reopen bitmaps correspondingly.
@@ -1491,13 +1491,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
         if (bdrv_has_readonly_bitmaps(bs) &&
             !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;
+    } else {
+        header_updated = qcow2_load_dirty_bitmaps(bs, &local_err);
+        s->dirty_bitmaps_loaded = true;
     }
+    update_header = update_header && !header_updated;
     if (local_err != NULL) {
         error_propagate(errp, local_err);
         ret = -EINVAL;
-- 
2.11.1

Re: [Qemu-devel] [PATCH v2 1/2] qcow2: try load bitmaps only once

[Eric Blake]

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

On 04/11/2018 07:26 AM, Vladimir Sementsov-Ogievskiy wrote:
> Checking reopen by existence of some bitmaps is wrong, as it may be
> some other bitmaps, or on the other hand, user may remove bitmaps. This
> criteria is bad. To simplify things and make behavior more predictable
> let's just add a flag to remember, that we've already tried to load
> bitmaps on open and do not want do it again.

Wording suggestion:

Checking whether we are reopening an image based on the existence of
some bitmaps is wrong, as the set of bitmaps may have changed (for
example, the user may have added or removed bitmaps in the meantime).
To simplify things and make behavior more predictable, let's just add a
flag to remember whether we've already tried to load bitmaps, so that we
only attempt it on the initial open.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2.h |  1 +
>  block/qcow2.c | 16 ++++++++--------
>  2 files changed, 9 insertions(+), 8 deletions(-)
> 
> @@ -1480,10 +1481,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>          s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
>      }
>  
> -    if (bdrv_dirty_bitmap_next(bs, NULL)) {
> -        /* It's some kind of reopen with already existing dirty bitmaps. There
> -         * are no known cases where we need loading bitmaps in such situation,
> -         * so it's safer don't load them.
> +    if (s->dirty_bitmaps_loaded) {
> +        /* It's some kind of reopen. There are no known cases where we need
> +         * loading bitmaps in such situation, so it's safer don't load them.

Pre-existing wording, but sounds better as:

There are no known cases where we need to reload bitmaps in such a
situation, so it's safer to skip them.

>           *
>           * Moreover, if we have some readonly bitmaps and we are reopening for
>           * rw we should reopen bitmaps correspondingly.
> @@ -1491,13 +1491,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>          if (bdrv_has_readonly_bitmaps(bs) &&
>              !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;
> +    } else {
> +        header_updated = qcow2_load_dirty_bitmaps(bs, &local_err);
> +        s->dirty_bitmaps_loaded = true;
>      }
> +    update_header = update_header && !header_updated;

Could write this as 'update_header &= !header_updated;'

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org


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

Re: [Qemu-devel] [PATCH v2 1/2] qcow2: try load bitmaps only once

[Vladimir Sementsov-Ogievskiy]

11.04.2018 16:22, Eric Blake wrote:
> On 04/11/2018 07:26 AM, Vladimir Sementsov-Ogievskiy wrote:
>> Checking reopen by existence of some bitmaps is wrong, as it may be
>> some other bitmaps, or on the other hand, user may remove bitmaps. This
>> criteria is bad. To simplify things and make behavior more predictable
>> let's just add a flag to remember, that we've already tried to load
>> bitmaps on open and do not want do it again.
> Wording suggestion:
>
> Checking whether we are reopening an image based on the existence of
> some bitmaps is wrong, as the set of bitmaps may have changed (for
> example, the user may have added or removed bitmaps in the meantime).
> To simplify things and make behavior more predictable, let's just add a
> flag to remember whether we've already tried to load bitmaps, so that we
> only attempt it on the initial open.

You've dropped an idea supposed by Max, about migration related bitmaps,
I said "some other bitmaps", because now I doubt is it possible: we have 
migration related
bitmaps on source, not on target..

Anyway, I'm ok with all your suggestions, thank you. I can respin, or 
they may be squashed-in inflight.

>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2.h |  1 +
>>   block/qcow2.c | 16 ++++++++--------
>>   2 files changed, 9 insertions(+), 8 deletions(-)
>>
>> @@ -1480,10 +1481,9 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>>           s->autoclear_features &= QCOW2_AUTOCLEAR_MASK;
>>       }
>>   
>> -    if (bdrv_dirty_bitmap_next(bs, NULL)) {
>> -        /* It's some kind of reopen with already existing dirty bitmaps. There
>> -         * are no known cases where we need loading bitmaps in such situation,
>> -         * so it's safer don't load them.
>> +    if (s->dirty_bitmaps_loaded) {
>> +        /* It's some kind of reopen. There are no known cases where we need
>> +         * loading bitmaps in such situation, so it's safer don't load them.
> Pre-existing wording, but sounds better as:
>
> There are no known cases where we need to reload bitmaps in such a
> situation, so it's safer to skip them.
>
>>            *
>>            * Moreover, if we have some readonly bitmaps and we are reopening for
>>            * rw we should reopen bitmaps correspondingly.
>> @@ -1491,13 +1491,13 @@ static int coroutine_fn qcow2_do_open(BlockDriverState *bs, QDict *options,
>>           if (bdrv_has_readonly_bitmaps(bs) &&
>>               !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;
>> +    } else {
>> +        header_updated = qcow2_load_dirty_bitmaps(bs, &local_err);
>> +        s->dirty_bitmaps_loaded = true;
>>       }
>> +    update_header = update_header && !header_updated;
> Could write this as 'update_header &= !header_updated;'
>


-- 
Best regards,
Vladimir

Re: [Qemu-devel] [PATCH v2 1/2] qcow2: try load bitmaps only once

[Max Reitz]

On 2018-04-11 14:26, Vladimir Sementsov-Ogievskiy wrote:
> Checking reopen by existence of some bitmaps is wrong, as it may be
> some other bitmaps, or on the other hand, user may remove bitmaps. This
> criteria is bad. To simplify things and make behavior more predictable
> let's just add a flag to remember, that we've already tried to load
> bitmaps on open and do not want do it again.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2.h |  1 +
>  block/qcow2.c | 16 ++++++++--------
>  2 files changed, 9 insertions(+), 8 deletions(-)

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

[Qemu-devel] [PATCH v2 2/2] iotests: fix 169

[Vladimir Sementsov-Ogievskiy]

Improve and fix 169:
    - use MIGRATION events instead of RESUME
    - make a TODO: enable dirty-bitmaps capability for offline case
    - recreate vm_b without -incoming near test end

This (likely) fixes racy faults at least of the following types:

    - timeout on waiting for RESUME event
    - sha256 mismatch on line 136 (142 after this patch)
    - fail to self.vm_b.launch() on line 135 (141 now after this patch)

And surely fixes cat processes, left after test finish.

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

diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
index 153b10b6e7..f243db9955 100755
--- a/tests/qemu-iotests/169
+++ b/tests/qemu-iotests/169
@@ -31,6 +31,8 @@ disk_a = os.path.join(iotests.test_dir, 'disk_a')
 disk_b = os.path.join(iotests.test_dir, 'disk_b')
 size = '1M'
 mig_file = os.path.join(iotests.test_dir, 'mig_file')
+mig_cmd = 'exec: cat > ' + mig_file
+incoming_cmd = 'exec: cat ' + mig_file
 
 
 class TestDirtyBitmapMigration(iotests.QMPTestCase):
@@ -49,7 +51,6 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
         self.vm_a.launch()
 
         self.vm_b = iotests.VM(path_suffix='b')
-        self.vm_b.add_incoming("exec: cat '" + mig_file + "'")
 
     def add_bitmap(self, vm, granularity, persistent):
         params = {'node': 'drive0',
@@ -86,36 +87,30 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
                    (0xa0201, 0x1000))
 
         should_migrate = migrate_bitmaps or persistent and shared_storage
+        mig_caps = [{'capability': 'events', 'state': True}]
+        if migrate_bitmaps:
+            mig_caps.append({'capability': 'dirty-bitmaps', 'state': True})
 
+        result = self.vm_a.qmp('migrate-set-capabilities',
+                               capabilities=mig_caps)
+        self.assert_qmp(result, 'return', {})
+
+        self.vm_b.add_incoming(incoming_cmd if online else "defer")
         self.vm_b.add_drive(disk_a if shared_storage else disk_b)
 
         if online:
             os.mkfifo(mig_file)
             self.vm_b.launch()
+            result = self.vm_b.qmp('migrate-set-capabilities',
+                                   capabilities=mig_caps)
+            self.assert_qmp(result, 'return', {})
 
         self.add_bitmap(self.vm_a, granularity, persistent)
         for r in regions:
             self.vm_a.hmp_qemu_io('drive0', 'write %d %d' % r)
         sha256 = self.get_bitmap_hash(self.vm_a)
 
-        if migrate_bitmaps:
-            capabilities = [{'capability': 'dirty-bitmaps', 'state': True}]
-
-            result = self.vm_a.qmp('migrate-set-capabilities',
-                                   capabilities=capabilities)
-            self.assert_qmp(result, 'return', {})
-
-            if online:
-                result = self.vm_b.qmp('migrate-set-capabilities',
-                                       capabilities=capabilities)
-                self.assert_qmp(result, 'return', {})
-
-        result = self.vm_a.qmp('migrate-set-capabilities',
-                               capabilities=[{'capability': 'events',
-                                              'state': True}])
-        self.assert_qmp(result, 'return', {})
-
-        result = self.vm_a.qmp('migrate', uri='exec:cat>' + mig_file)
+        result = self.vm_a.qmp('migrate', uri=mig_cmd)
         while True:
             event = self.vm_a.event_wait('MIGRATION')
             if event['data']['status'] == 'completed':
@@ -124,14 +119,25 @@ class TestDirtyBitmapMigration(iotests.QMPTestCase):
         if not online:
             self.vm_a.shutdown()
             self.vm_b.launch()
-            # TODO enable bitmap capability for vm_b in this case
+            result = self.vm_b.qmp('migrate-set-capabilities',
+                                   capabilities=mig_caps)
+            self.assert_qmp(result, 'return', {})
+            result = self.vm_b.qmp('migrate-incoming', uri=incoming_cmd)
+            self.assert_qmp(result, 'return', {})
 
-        self.vm_b.event_wait("RESUME", timeout=10.0)
+        while True:
+            event = self.vm_b.event_wait('MIGRATION')
+            if event['data']['status'] == 'completed':
+                break
 
         self.check_bitmap(self.vm_b, sha256 if should_migrate else False)
 
         if should_migrate:
             self.vm_b.shutdown()
+            # 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')
+            self.vm_b.add_drive(disk_a if shared_storage else disk_b)
             self.vm_b.launch()
             self.check_bitmap(self.vm_b, sha256 if persistent else False)
 
-- 
2.11.1

Re: [Qemu-devel] [PATCH v2 2/2] iotests: fix 169

[Max Reitz]

On 2018-04-11 14:26, Vladimir Sementsov-Ogievskiy wrote:
> Improve and fix 169:
>     - use MIGRATION events instead of RESUME
>     - make a TODO: enable dirty-bitmaps capability for offline case
>     - recreate vm_b without -incoming near test end
> 
> This (likely) fixes racy faults at least of the following types:
> 
>     - timeout on waiting for RESUME event
>     - sha256 mismatch on line 136 (142 after this patch)
>     - fail to self.vm_b.launch() on line 135 (141 now after this patch)
> 
> And surely fixes cat processes, left after test finish.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  tests/qemu-iotests/169 | 48 +++++++++++++++++++++++++++---------------------
>  1 file changed, 27 insertions(+), 21 deletions(-)

Looks good, makes the test pass reliably (here) and doesn't result in
overly loud meowing due to an abundance of orphaned cats.  Thanks!

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

Re: [Qemu-devel] [PATCH v2 0/2] bitmaps persistent and migration fixes

[Max Reitz]

On 2018-04-11 14:26, Vladimir Sementsov-Ogievskiy wrote:
> v2:
> 
> 01: new, proposed by Max
> 02: fix leaving cat processes
> 
> Vladimir Sementsov-Ogievskiy (2):
>   qcow2: try load bitmaps only once
>   iotests: fix 169
> 
>  block/qcow2.h          |  1 +
>  block/qcow2.c          | 16 ++++++++--------
>  tests/qemu-iotests/169 | 48 +++++++++++++++++++++++++++---------------------
>  3 files changed, 36 insertions(+), 29 deletions(-)

Thanks, changed the comment wording in patch 1 according to Eric's
suggestion and applied to my block branch:

https://github.com/XanClic/qemu/commits/block

I left the commit wording unchanged because as you said there might be
more cases where we have bitmaps at the node although we haven't yet
loaded any from the file.  I certainly don't want to have to think too
much about those cases and that to me is the main use of patch 1.

If there is going to be an rc4 (it appears there is), I will send a pull
request with these patches for it.  If not, I probably won't.  Then I'll
just add a qemu-stable CC and get them into 2.13 -- unless you disagree
and you think that this series should indeed be in 2.12.

Max