[PATCH 0/2] qcow2: Release read-only bitmaps when inactivated

[PATCH 0/2] qcow2: Release read-only bitmaps when inactivated

[Max Reitz]

Hi,

When beginning migration, the qcow2 driver syncs all persistent bitmaps
to disk and then releases them.  If the user decides to continue on the
source after migration, those bitmaps are re-loaded from the qcow2
image.

However, we only do this for bitmaps that were actively synced, i.e. R/W
bitmaps.  RO bitmaps (those on backing images) are not written and thus
not released.  However, we still try to re-load them when continuing,
and that will then fail.

To fix this problem, I think we should just consider RO bitmaps to be in
sync from the beginning, so we can release them just like bitmaps that
we have actively written back to the image.  This is done by patch 1.

However, there’s a catch: Peter Krempa noted that it isn’t in libvirt’s
interest for the bitmaps to be released before migration at all, because
this makes them disappear from query-named-block-node’s dirty-bitmaps
list, but libvirt needs the bitmaps to be there:

https://bugzilla.redhat.com/show_bug.cgi?id=1858739#c3

If it’s really not feasible to keep the bitmaps around, then I suppose
what might work for libvirt is to query
image/format-specific/data/bitmaps in addition to dirty-bitmaps (every
bitmap that we released before migration must be a persistent bitmap).

What are your thoughts on this?


Max Reitz (2):
  qcow2: Release read-only bitmaps when inactivated
  iotests/169: Test source cont with backing bmap

 block/qcow2-bitmap.c       | 23 +++++++++++---
 tests/qemu-iotests/169     | 64 +++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/169.out |  4 +--
 3 files changed, 84 insertions(+), 7 deletions(-)

-- 
2.26.2

[PATCH 1/2] qcow2: Release read-only bitmaps when inactivated

[Max Reitz]

During migration, we release all bitmaps after storing them on disk, as
long as they are (1) stored on disk, (2) not read-only, and (3)
consistent.

(2) seems arbitrary, though.  The reason we do not release them is
because we do not write them, as there is no need to; and then we just
forget about all bitmaps that we have not written to the file.  However,
read-only persistent bitmaps are still in the file and in sync with
their in-memory representation, so we may as well release them just like
any R/W bitmap that we have updated.

It leads to actual problems, too: After migration, letting the source
continue may result in an error if there were any bitmaps on read-only
nodes (such as backing images), because those have not been released by
bdrv_inactive_all(), but bdrv_invalidate_cache_all() attempts to reload
them (which fails, because they are still present in memory).

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 block/qcow2-bitmap.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
index 1f38806ca6..8c34b2aef7 100644
--- a/block/qcow2-bitmap.c
+++ b/block/qcow2-bitmap.c
@@ -1562,11 +1562,22 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
         Qcow2Bitmap *bm;
 
         if (!bdrv_dirty_bitmap_get_persistence(bitmap) ||
-            bdrv_dirty_bitmap_readonly(bitmap) ||
             bdrv_dirty_bitmap_inconsistent(bitmap)) {
             continue;
         }
 
+        if (bdrv_dirty_bitmap_readonly(bitmap)) {
+            /*
+             * Store the bitmap in the associated Qcow2Bitmap so it
+             * can be released later
+             */
+            bm = find_bitmap_by_name(bm_list, name);
+            if (bm) {
+                bm->dirty_bitmap = bitmap;
+            }
+            continue;
+        }
+
         need_write = true;
 
         if (check_constraints_on_bitmap(bs, name, granularity, errp) < 0) {
@@ -1618,7 +1629,9 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
 
     /* allocate clusters and store bitmaps */
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-        if (bm->dirty_bitmap == NULL) {
+        BdrvDirtyBitmap *bitmap = bm->dirty_bitmap;
+
+        if (bitmap == NULL || bdrv_dirty_bitmap_readonly(bitmap)) {
             continue;
         }
 
@@ -1641,6 +1654,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
         g_free(tb);
     }
 
+success:
     if (release_stored) {
         QSIMPLEQ_FOREACH(bm, bm_list, entry) {
             if (bm->dirty_bitmap == NULL) {
@@ -1651,13 +1665,14 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
         }
     }
 
-success:
     bitmap_list_free(bm_list);
     return;
 
 fail:
     QSIMPLEQ_FOREACH(bm, bm_list, entry) {
-        if (bm->dirty_bitmap == NULL || bm->table.offset == 0) {
+        if (bm->dirty_bitmap == NULL || bm->table.offset == 0 ||
+            bdrv_dirty_bitmap_readonly(bm->dirty_bitmap))
+        {
             continue;
         }
 
-- 
2.26.2

Re: [PATCH 1/2] qcow2: Release read-only bitmaps when inactivated

[Peter Krempa]

On Thu, Jul 30, 2020 at 14:02:33 +0200, Max Reitz wrote:
> During migration, we release all bitmaps after storing them on disk, as
> long as they are (1) stored on disk, (2) not read-only, and (3)
> consistent.
> 
> (2) seems arbitrary, though.  The reason we do not release them is
> because we do not write them, as there is no need to; and then we just
> forget about all bitmaps that we have not written to the file.  However,
> read-only persistent bitmaps are still in the file and in sync with
> their in-memory representation, so we may as well release them just like
> any R/W bitmap that we have updated.
> 
> It leads to actual problems, too: After migration, letting the source
> continue may result in an error if there were any bitmaps on read-only
> nodes (such as backing images), because those have not been released by
> bdrv_inactive_all(), but bdrv_invalidate_cache_all() attempts to reload
> them (which fails, because they are still present in memory).
> 

I've tested it with same commands as I've used before and now the 'cont'
succeeds and also the bitmaps after the cont call are loaded and active
at least according to 'query-named-block-nodes'

Tested-by: Peter Krempa <pkrempa@redhat.com>

Re: [PATCH 1/2] qcow2: Release read-only bitmaps when inactivated

[Eric Blake]

On 7/30/20 7:02 AM, Max Reitz wrote:
> During migration, we release all bitmaps after storing them on disk, as
> long as they are (1) stored on disk, (2) not read-only, and (3)
> consistent.
> 
> (2) seems arbitrary, though.  The reason we do not release them is
> because we do not write them, as there is no need to; and then we just
> forget about all bitmaps that we have not written to the file.  However,
> read-only persistent bitmaps are still in the file and in sync with
> their in-memory representation, so we may as well release them just like
> any R/W bitmap that we have updated.
> 
> It leads to actual problems, too: After migration, letting the source
> continue may result in an error if there were any bitmaps on read-only
> nodes (such as backing images), because those have not been released by
> bdrv_inactive_all(), but bdrv_invalidate_cache_all() attempts to reload
> them (which fails, because they are still present in memory).

I think our alternatives are ensuring no bitmaps are in memory so that 
reloading the RO bitmap from the file succeeds (which then hits the 
earlier question about whether releasing ALL bitmaps affects libvirt's 
ability to query which bitmaps were on the source, but makes reloading 
on the destination easy), or teaching the reload to special-case a RO 
bitmap from the disk that is already in memory (either to make the 
reload a graceful no-op instead of an error that it was already loaded, 
or to go one step further and validate whether the contents in memory 
match the contents reloaded from disk).  If I understand your patch, you 
went with the first of these alternatives.  And since Peter was able to 
test that it fixed the libvirt scenario, I'm okay with the approach you 
took, although I would love a second opinion from Virtuozzo folks.

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   block/qcow2-bitmap.c | 23 +++++++++++++++++++----
>   1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/block/qcow2-bitmap.c b/block/qcow2-bitmap.c
> index 1f38806ca6..8c34b2aef7 100644
> --- a/block/qcow2-bitmap.c

> @@ -1641,6 +1654,7 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>           g_free(tb);
>       }
>   
> +success:
>       if (release_stored) {
>           QSIMPLEQ_FOREACH(bm, bm_list, entry) {
>               if (bm->dirty_bitmap == NULL) {
> @@ -1651,13 +1665,14 @@ void qcow2_store_persistent_dirty_bitmaps(BlockDriverState *bs,
>           }
>       }
>   
> -success:

Moving the label was an interesting change; I had to look at the file in 
context to see the real effect: basically, you now reach the line:

bdrv_release_dirty_bitmap(bm->dirty_bitmap);

for the set of persistent RO bitmaps that were previously ignored.

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 1/2] qcow2: Release read-only bitmaps when inactivated

[Vladimir Sementsov-Ogievskiy]

30.07.2020 15:02, Max Reitz wrote:
> During migration, we release all bitmaps after storing them on disk, as
> long as they are (1) stored on disk, (2) not read-only, and (3)
> consistent.
> 
> (2) seems arbitrary, though.  The reason we do not release them is
> because we do not write them, as there is no need to; and then we just
> forget about all bitmaps that we have not written to the file.  However,
> read-only persistent bitmaps are still in the file and in sync with
> their in-memory representation, so we may as well release them just like
> any R/W bitmap that we have updated.

Agree, better to release all image-owned bitmaps on inactivation of the image.

> 
> It leads to actual problems, too: After migration, letting the source
> continue may result in an error if there were any bitmaps on read-only
> nodes (such as backing images), because those have not been released by
> bdrv_inactive_all(), but bdrv_invalidate_cache_all() attempts to reload
> them (which fails, because they are still present in memory).
> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>

The patch seems OK to me, thanks!

-- 
Best regards,
Vladimir

[PATCH 2/2] iotests/169: Test source cont with backing bmap

[Max Reitz]

Test migrating from a VM with a persistent bitmap in the backing chain,
and then continuing that VM after the migration

Signed-off-by: Max Reitz <mreitz@redhat.com>
---
 tests/qemu-iotests/169     | 64 +++++++++++++++++++++++++++++++++++++-
 tests/qemu-iotests/169.out |  4 +--
 2 files changed, 65 insertions(+), 3 deletions(-)

diff --git a/tests/qemu-iotests/169 b/tests/qemu-iotests/169
index 2c5a132aa3..40afb15299 100755
--- a/tests/qemu-iotests/169
+++ b/tests/qemu-iotests/169
@@ -24,11 +24,12 @@ import time
 import itertools
 import operator
 import re
-from iotests import qemu_img
+from iotests import qemu_img, qemu_img_create, Timeout
 
 
 disk_a = os.path.join(iotests.test_dir, 'disk_a')
 disk_b = os.path.join(iotests.test_dir, 'disk_b')
+base_a = os.path.join(iotests.test_dir, 'base_a')
 size = '1M'
 mig_file = os.path.join(iotests.test_dir, 'mig_file')
 mig_cmd = 'exec: cat > ' + mig_file
@@ -234,6 +235,67 @@ for cmb in list(itertools.product((True, False), repeat=2)):
     inject_test_case(TestDirtyBitmapMigration, name,
                      'do_test_migration_resume_source', *list(cmb))
 
+
+class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
+    def setUp(self):
+        qemu_img_create('-f', iotests.imgfmt, base_a, size)
+        qemu_img_create('-f', iotests.imgfmt, '-F', iotests.imgfmt,
+                        '-b', base_a, disk_a, size)
+
+        for f in (disk_a, base_a):
+            qemu_img('bitmap', '--add', f, 'bmap0')
+
+        blockdev = {
+            'node-name': 'node0',
+            'driver': iotests.imgfmt,
+            'file': {
+                'driver': 'file',
+                'filename': disk_a
+            },
+            'backing': {
+                'node-name': 'node0-base',
+                'driver': iotests.imgfmt,
+                'file': {
+                    'driver': 'file',
+                    'filename': base_a
+                }
+            }
+        }
+
+        self.vm = iotests.VM()
+        self.vm.launch()
+
+        result = self.vm.qmp('blockdev-add', **blockdev)
+        self.assert_qmp(result, 'return', {})
+
+        # Check that the bitmaps are there
+        for node in self.vm.qmp('query-named-block-nodes', flat=True)['return']:
+            if 'node0' in node['node-name']:
+                self.assert_qmp(node, 'dirty-bitmaps[0]/name', 'bmap0')
+
+        caps = [{'capability': 'events', 'state': True}]
+        result = self.vm.qmp('migrate-set-capabilities', capabilities=caps)
+        self.assert_qmp(result, 'return', {})
+
+    def tearDown(self):
+        self.vm.shutdown()
+        for f in (disk_a, base_a):
+            os.remove(f)
+
+    def test_cont_on_source(self):
+        """
+        Continue the source after migration.
+        """
+        result = self.vm.qmp('migrate', uri=f'exec: cat > /dev/null')
+        self.assert_qmp(result, 'return', {})
+
+        with Timeout(10, 'Migration timeout'):
+            self.vm.wait_migration('postmigrate')
+
+        result = self.vm.qmp('cont')
+        self.assert_qmp(result, 'return', {})
+
+
 if __name__ == '__main__':
     iotests.main(supported_fmts=['qcow2'],
                  supported_protocols=['file'])
diff --git a/tests/qemu-iotests/169.out b/tests/qemu-iotests/169.out
index 5c26d15c0d..cafb8161f7 100644
--- a/tests/qemu-iotests/169.out
+++ b/tests/qemu-iotests/169.out
@@ -1,5 +1,5 @@
-....................................
+.....................................
 ----------------------------------------------------------------------
-Ran 36 tests
+Ran 37 tests
 
 OK
-- 
2.26.2

Re: [PATCH 2/2] iotests/169: Test source cont with backing bmap

[Eric Blake]

On 7/30/20 7:02 AM, Max Reitz wrote:
> Test migrating from a VM with a persistent bitmap in the backing chain,
> and then continuing that VM after the migration

Indeed, the more our iotest is like what libvirt actually does, the 
better we protect ourselves from future bugs in this area.

> 
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
>   tests/qemu-iotests/169     | 64 +++++++++++++++++++++++++++++++++++++-
>   tests/qemu-iotests/169.out |  4 +--
>   2 files changed, 65 insertions(+), 3 deletions(-)
> 

> +class TestDirtyBitmapBackingMigration(iotests.QMPTestCase):
> +    def setUp(self):
> +        qemu_img_create('-f', iotests.imgfmt, base_a, size)
> +        qemu_img_create('-f', iotests.imgfmt, '-F', iotests.imgfmt,
> +                        '-b', base_a, disk_a, size)
> +
> +        for f in (disk_a, base_a):
> +            qemu_img('bitmap', '--add', f, 'bmap0')

I'm so glad to see 'qemu-img bitmap' getting some use :)

> +
> +        blockdev = {
> +            'node-name': 'node0',
> +            'driver': iotests.imgfmt,
> +            'file': {
> +                'driver': 'file',
> +                'filename': disk_a
> +            },
> +            'backing': {
> +                'node-name': 'node0-base',
> +                'driver': iotests.imgfmt,
> +                'file': {
> +                    'driver': 'file',
> +                    'filename': base_a
> +                }
> +            }
> +        }
> +
> +        self.vm = iotests.VM()
> +        self.vm.launch()
> +
> +        result = self.vm.qmp('blockdev-add', **blockdev)
> +        self.assert_qmp(result, 'return', {})
> +
> +        # Check that the bitmaps are there
> +        for node in self.vm.qmp('query-named-block-nodes', flat=True)['return']:
> +            if 'node0' in node['node-name']:

This caught me on the first read, before I realized it was a clever way 
to test both 'node0' and 'node0-base'.

> +                self.assert_qmp(node, 'dirty-bitmaps[0]/name', 'bmap0')
> +
> +        caps = [{'capability': 'events', 'state': True}]
> +        result = self.vm.qmp('migrate-set-capabilities', capabilities=caps)
> +        self.assert_qmp(result, 'return', {})
> +
> +    def tearDown(self):
> +        self.vm.shutdown()
> +        for f in (disk_a, base_a):
> +            os.remove(f)
> +
> +    def test_cont_on_source(self):
> +        """
> +        Continue the source after migration.
> +        """
> +        result = self.vm.qmp('migrate', uri=f'exec: cat > /dev/null')
> +        self.assert_qmp(result, 'return', {})
> +
> +        with Timeout(10, 'Migration timeout'):
> +            self.vm.wait_migration('postmigrate')
> +
> +        result = self.vm.qmp('cont')
> +        self.assert_qmp(result, 'return', {})
> +

Reviewed-by: Eric Blake <eblake@redhat.com>

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

Re: [PATCH 0/2] qcow2: Release read-only bitmaps when inactivated

[Eric Blake]

On 7/30/20 7:02 AM, Max Reitz wrote:
> Hi,
> 
> When beginning migration, the qcow2 driver syncs all persistent bitmaps
> to disk and then releases them.  If the user decides to continue on the
> source after migration, those bitmaps are re-loaded from the qcow2
> image.
> 
> However, we only do this for bitmaps that were actively synced, i.e. R/W
> bitmaps.  RO bitmaps (those on backing images) are not written and thus
> not released.  However, we still try to re-load them when continuing,
> and that will then fail.
> 
> To fix this problem, I think we should just consider RO bitmaps to be in
> sync from the beginning, so we can release them just like bitmaps that
> we have actively written back to the image.  This is done by patch 1.
> 
> However, there’s a catch: Peter Krempa noted that it isn’t in libvirt’s
> interest for the bitmaps to be released before migration at all, because
> this makes them disappear from query-named-block-node’s dirty-bitmaps
> list, but libvirt needs the bitmaps to be there:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1858739#c3

And that is enough to make me think this series is -rc3 material. 
Although I'm not yet sure whether the solution is this series as 
written, or to patch libvirt to look elsewhere for bitmap information, 
or to patch qemu on incoming migration to not complain when reloading a 
RO bitmap, or something else.

> 
> If it’s really not feasible to keep the bitmaps around, then I suppose
> what might work for libvirt is to query
> image/format-specific/data/bitmaps in addition to dirty-bitmaps (every
> bitmap that we released before migration must be a persistent bitmap).
> 
> What are your thoughts on this?

I'd really like to hear from Virtuozzo on the topic before committing to 
this series, but I will at least review it in the meantime.

> 
> 
> Max Reitz (2):
>    qcow2: Release read-only bitmaps when inactivated
>    iotests/169: Test source cont with backing bmap
> 
>   block/qcow2-bitmap.c       | 23 +++++++++++---
>   tests/qemu-iotests/169     | 64 +++++++++++++++++++++++++++++++++++++-
>   tests/qemu-iotests/169.out |  4 +--
>   3 files changed, 84 insertions(+), 7 deletions(-)
> 

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

Re: [PATCH 0/2] qcow2: Release read-only bitmaps when inactivated

[Vladimir Sementsov-Ogievskiy]

30.07.2020 15:02, Max Reitz wrote:
> Hi,
> 
> When beginning migration, the qcow2 driver syncs all persistent bitmaps
> to disk and then releases them.  If the user decides to continue on the
> source after migration, those bitmaps are re-loaded from the qcow2
> image.
> 
> However, we only do this for bitmaps that were actively synced, i.e. R/W
> bitmaps.  RO bitmaps (those on backing images) are not written and thus
> not released.  However, we still try to re-load them when continuing,
> and that will then fail.
> 
> To fix this problem, I think we should just consider RO bitmaps to be in
> sync from the beginning, so we can release them just like bitmaps that
> we have actively written back to the image.  This is done by patch 1.
> 
> However, there’s a catch: Peter Krempa noted that it isn’t in libvirt’s
> interest for the bitmaps to be released before migration at all, because
> this makes them disappear from query-named-block-node’s dirty-bitmaps
> list, but libvirt needs the bitmaps to be there:
> 
> https://bugzilla.redhat.com/show_bug.cgi?id=1858739#c3
> 
> If it’s really not feasible to keep the bitmaps around, then I suppose
> what might work for libvirt is to query
> image/format-specific/data/bitmaps in addition to dirty-bitmaps (every
> bitmap that we released before migration must be a persistent bitmap).
> 
> What are your thoughts on this?

Sorry, I was on vocation for a week, so missed this. I see, it was merged, still, I'll look through it now.

Hmm.

1. Why we sync bitmaps on inactivation: it's obvious, it's the last point when it is possible to store them

2. Why we release bitmaps on inactivation: after inactivation, the bitmaps are not owned by Qemu. The image is not locked, someone other can change persistent bitmaps (target Qemu for example)..

> 
> 
> Max Reitz (2):
>    qcow2: Release read-only bitmaps when inactivated
>    iotests/169: Test source cont with backing bmap
> 
>   block/qcow2-bitmap.c       | 23 +++++++++++---
>   tests/qemu-iotests/169     | 64 +++++++++++++++++++++++++++++++++++++-
>   tests/qemu-iotests/169.out |  4 +--
>   3 files changed, 84 insertions(+), 7 deletions(-)
> 


-- 
Best regards,
Vladimir