[PATCH 0/2] fix crash if try to migrate during backup

[PATCH 0/2] fix crash if try to migrate during backup

[Vladimir Sementsov-Ogievskiy]

Hi all!

We faced a crash and here is a fix. Look at the commits for details.

Vladimir Sementsov-Ogievskiy (2):
  tests: add migrate-during-backup
  block: bdrv_inactivate_recurse(): check for permissions and fix crash

 block.c                                       |  8 ++
 .../qemu-iotests/tests/migrate-during-backup  | 87 +++++++++++++++++++
 .../tests/migrate-during-backup.out           |  5 ++
 3 files changed, 100 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/migrate-during-backup
 create mode 100644 tests/qemu-iotests/tests/migrate-during-backup.out

-- 
2.29.2

[PATCH 1/2] tests: add migrate-during-backup

[Vladimir Sementsov-Ogievskiy]

Add a simple test which tries to run migration during backup.
bdrv_inactivate_all() should fail. But due to bug (see next commit with
fix) it doesn't, nodes are inactivated and continued backup crashes
on assertion "assert(!(bs->open_flags & BDRV_O_INACTIVE));" in
bdrv_co_write_req_prepare().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 .../qemu-iotests/tests/migrate-during-backup  | 87 +++++++++++++++++++
 .../tests/migrate-during-backup.out           |  5 ++
 2 files changed, 92 insertions(+)
 create mode 100755 tests/qemu-iotests/tests/migrate-during-backup
 create mode 100644 tests/qemu-iotests/tests/migrate-during-backup.out

diff --git a/tests/qemu-iotests/tests/migrate-during-backup b/tests/qemu-iotests/tests/migrate-during-backup
new file mode 100755
index 0000000000..c3b7f1983d
--- /dev/null
+++ b/tests/qemu-iotests/tests/migrate-during-backup
@@ -0,0 +1,87 @@
+#!/usr/bin/env python3
+# group: migration disabled
+#
+# Copyright (c) 2021 Virtuozzo International GmbH
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import os
+import iotests
+from iotests import qemu_img_create, qemu_io
+
+
+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
+
+
+class TestMigrateDuringBackup(iotests.QMPTestCase):
+    def tearDown(self):
+        self.vm.shutdown()
+        os.remove(disk_a)
+        os.remove(disk_b)
+        os.remove(mig_file)
+
+    def setUp(self):
+        qemu_img_create('-f', iotests.imgfmt, disk_a, size)
+        qemu_img_create('-f', iotests.imgfmt, disk_b, size)
+        qemu_io('-c', f'write 0 {size}', disk_a)
+
+        self.vm = iotests.VM().add_drive(disk_a)
+        self.vm.launch()
+        result = self.vm.qmp('blockdev-add', {
+            'node-name': 'target',
+            'driver': iotests.imgfmt,
+            'file': {
+                'driver': 'file',
+                'filename': disk_b
+            }
+        })
+        self.assert_qmp(result, 'return', {})
+
+    def test_migrate(self):
+        result = self.vm.qmp('blockdev-backup', device='drive0',
+                             target='target', sync='full',
+                             speed=1, x_perf={
+                                 'max-workers': 1,
+                                 'max-chunk': 64 * 1024
+                             })
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('job-pause', id='drive0')
+        self.assert_qmp(result, 'return', {})
+
+        result = self.vm.qmp('migrate-set-capabilities',
+                             capabilities=[{'capability': 'events',
+                                            'state': True}])
+        self.assert_qmp(result, 'return', {})
+        result = self.vm.qmp('migrate', uri=mig_cmd)
+        self.assert_qmp(result, 'return', {})
+
+        self.vm.events_wait((('MIGRATION', {'data': {'status': 'completed'}}),
+                             ('MIGRATION', {'data': {'status': 'failed'}})))
+
+        result = self.vm.qmp('block-job-set-speed', device='drive0',
+                             speed=0)
+        self.assert_qmp(result, 'return', {})
+        result = self.vm.qmp('job-resume', id='drive0')
+        self.assert_qmp(result, 'return', {})
+
+
+if __name__ == '__main__':
+    iotests.main(supported_fmts=['qcow2'],
+                 supported_protocols=['file'])
diff --git a/tests/qemu-iotests/tests/migrate-during-backup.out b/tests/qemu-iotests/tests/migrate-during-backup.out
new file mode 100644
index 0000000000..ae1213e6f8
--- /dev/null
+++ b/tests/qemu-iotests/tests/migrate-during-backup.out
@@ -0,0 +1,5 @@
+.
+----------------------------------------------------------------------
+Ran 1 tests
+
+OK
-- 
2.29.2

[PATCH 2/2] block: bdrv_inactivate_recurse(): check for permissions and fix crash

[Vladimir Sementsov-Ogievskiy]

We must not inactivate child when parent has write permissions on
it.

Calling .bdrv_inactivate() doesn't help: actually only qcow2 has this
handler and it is used to flush caches, not for permission
manipulations.

So, let's simply check cumulative parent permissions before
inactivating the node.

This commit fixes a crash when we do migration during backup: prior to
the commit nothing prevents all nodes inactivation at migration finish
and following backup write to the target crashes on assertion
"assert(!(bs->open_flags & BDRV_O_INACTIVE));" in
bdrv_co_write_req_prepare().

After the commit, we rely on the fact that copy-before-write filter
keeps write permission on target node to be able to write to it. So
inactivation fails and migration fails as expected.

Corresponding test now passes, so, enable it.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block.c                                        | 8 ++++++++
 tests/qemu-iotests/tests/migrate-during-backup | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/block.c b/block.c
index b2b66263f9..ceb2b66afc 100644
--- a/block.c
+++ b/block.c
@@ -6319,6 +6319,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs)
 {
     BdrvChild *child, *parent;
     int ret;
+    uint64_t cumulative_perms, cumulative_shared_perms;
 
     if (!bs->drv) {
         return -ENOMEDIUM;
@@ -6349,6 +6350,13 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs)
         }
     }
 
+    bdrv_get_cumulative_perm(bs, &cumulative_perms,
+                             &cumulative_shared_perms);
+    if (cumulative_perms & (BLK_PERM_WRITE | BLK_PERM_WRITE_UNCHANGED)) {
+        /* Our inactive parents still need write access. Inactivation failed. */
+        return -EPERM;
+    }
+
     bs->open_flags |= BDRV_O_INACTIVE;
 
     /*
diff --git a/tests/qemu-iotests/tests/migrate-during-backup b/tests/qemu-iotests/tests/migrate-during-backup
index c3b7f1983d..d18e558fa5 100755
--- a/tests/qemu-iotests/tests/migrate-during-backup
+++ b/tests/qemu-iotests/tests/migrate-during-backup
@@ -1,5 +1,5 @@
 #!/usr/bin/env python3
-# group: migration disabled
+# group: migration
 #
 # Copyright (c) 2021 Virtuozzo International GmbH
 #
-- 
2.29.2

Re: [PATCH 2/2] block: bdrv_inactivate_recurse(): check for permissions and fix crash

[Hanna Reitz]

On 10.09.21 13:01, Vladimir Sementsov-Ogievskiy wrote:
> We must not inactivate child when parent has write permissions on
> it.
>
> Calling .bdrv_inactivate() doesn't help: actually only qcow2 has this
> handler and it is used to flush caches, not for permission
> manipulations.

I guess we could ask whether block jobs should implement 
.bdrv_inactivate() to cancel themselves, but I believe it’s indeed 
better to have the migration fail and thus force the user to manually 
cancel the job (should that be what they want).

> So, let's simply check cumulative parent permissions before
> inactivating the node.
>
> This commit fixes a crash when we do migration during backup: prior to
> the commit nothing prevents all nodes inactivation at migration finish
> and following backup write to the target crashes on assertion
> "assert(!(bs->open_flags & BDRV_O_INACTIVE));" in
> bdrv_co_write_req_prepare().
>
> After the commit, we rely on the fact that copy-before-write filter
> keeps write permission on target node to be able to write to it. So
> inactivation fails and migration fails as expected.
>
> Corresponding test now passes, so, enable it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block.c                                        | 8 ++++++++
>   tests/qemu-iotests/tests/migrate-during-backup | 2 +-
>   2 files changed, 9 insertions(+), 1 deletion(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 1/2] tests: add migrate-during-backup

[Hanna Reitz]

On 10.09.21 13:00, Vladimir Sementsov-Ogievskiy wrote:
> Add a simple test which tries to run migration during backup.
> bdrv_inactivate_all() should fail. But due to bug (see next commit with
> fix) it doesn't, nodes are inactivated and continued backup crashes
> on assertion "assert(!(bs->open_flags & BDRV_O_INACTIVE));" in
> bdrv_co_write_req_prepare().
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   .../qemu-iotests/tests/migrate-during-backup  | 87 +++++++++++++++++++
>   .../tests/migrate-during-backup.out           |  5 ++
>   2 files changed, 92 insertions(+)
>   create mode 100755 tests/qemu-iotests/tests/migrate-during-backup
>   create mode 100644 tests/qemu-iotests/tests/migrate-during-backup.out
>
> diff --git a/tests/qemu-iotests/tests/migrate-during-backup b/tests/qemu-iotests/tests/migrate-during-backup
> new file mode 100755
> index 0000000000..c3b7f1983d
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/migrate-during-backup
> @@ -0,0 +1,87 @@
> +#!/usr/bin/env python3
> +# group: migration disabled
> +#
> +# Copyright (c) 2021 Virtuozzo International GmbH
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import os
> +import iotests
> +from iotests import qemu_img_create, qemu_io
> +
> +
> +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
> +
> +
> +class TestMigrateDuringBackup(iotests.QMPTestCase):
> +    def tearDown(self):
> +        self.vm.shutdown()
> +        os.remove(disk_a)
> +        os.remove(disk_b)
> +        os.remove(mig_file)
> +
> +    def setUp(self):
> +        qemu_img_create('-f', iotests.imgfmt, disk_a, size)
> +        qemu_img_create('-f', iotests.imgfmt, disk_b, size)
> +        qemu_io('-c', f'write 0 {size}', disk_a)
> +
> +        self.vm = iotests.VM().add_drive(disk_a)
> +        self.vm.launch()
> +        result = self.vm.qmp('blockdev-add', {
> +            'node-name': 'target',
> +            'driver': iotests.imgfmt,
> +            'file': {
> +                'driver': 'file',
> +                'filename': disk_b
> +            }
> +        })
> +        self.assert_qmp(result, 'return', {})
> +
> +    def test_migrate(self):
> +        result = self.vm.qmp('blockdev-backup', device='drive0',
> +                             target='target', sync='full',
> +                             speed=1, x_perf={
> +                                 'max-workers': 1,
> +                                 'max-chunk': 64 * 1024
> +                             })
> +        self.assert_qmp(result, 'return', {})
> +
> +        result = self.vm.qmp('job-pause', id='drive0')
> +        self.assert_qmp(result, 'return', {})
> +
> +        result = self.vm.qmp('migrate-set-capabilities',
> +                             capabilities=[{'capability': 'events',
> +                                            'state': True}])
> +        self.assert_qmp(result, 'return', {})
> +        result = self.vm.qmp('migrate', uri=mig_cmd)
> +        self.assert_qmp(result, 'return', {})
> +
> +        self.vm.events_wait((('MIGRATION', {'data': {'status': 'completed'}}),
> +                             ('MIGRATION', {'data': {'status': 'failed'}})))

So the migration failing is the result we expect here, right? Perhaps we 
should then have a loop that waits for MIGRATION events, and breaks on 
both status=completed and status=failed, but logs an error if the 
migration completes unexpectedly.

While I’ll give a

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

either way, I’d like to know your opinion on this still.

Hanna

> +
> +        result = self.vm.qmp('block-job-set-speed', device='drive0',
> +                             speed=0)
> +        self.assert_qmp(result, 'return', {})
> +        result = self.vm.qmp('job-resume', id='drive0')
> +        self.assert_qmp(result, 'return', {})
> +
> +
> +if __name__ == '__main__':
> +    iotests.main(supported_fmts=['qcow2'],
> +                 supported_protocols=['file'])
> diff --git a/tests/qemu-iotests/tests/migrate-during-backup.out b/tests/qemu-iotests/tests/migrate-during-backup.out
> new file mode 100644
> index 0000000000..ae1213e6f8
> --- /dev/null
> +++ b/tests/qemu-iotests/tests/migrate-during-backup.out
> @@ -0,0 +1,5 @@
> +.
> +----------------------------------------------------------------------
> +Ran 1 tests
> +
> +OK

Re: [PATCH 1/2] tests: add migrate-during-backup

[Vladimir Sementsov-Ogievskiy]

10.09.2021 17:18, Hanna Reitz wrote:
> On 10.09.21 13:00, Vladimir Sementsov-Ogievskiy wrote:
>> Add a simple test which tries to run migration during backup.
>> bdrv_inactivate_all() should fail. But due to bug (see next commit with
>> fix) it doesn't, nodes are inactivated and continued backup crashes
>> on assertion "assert(!(bs->open_flags & BDRV_O_INACTIVE));" in
>> bdrv_co_write_req_prepare().
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   .../qemu-iotests/tests/migrate-during-backup  | 87 +++++++++++++++++++
>>   .../tests/migrate-during-backup.out           |  5 ++
>>   2 files changed, 92 insertions(+)
>>   create mode 100755 tests/qemu-iotests/tests/migrate-during-backup
>>   create mode 100644 tests/qemu-iotests/tests/migrate-during-backup.out
>>
>> diff --git a/tests/qemu-iotests/tests/migrate-during-backup b/tests/qemu-iotests/tests/migrate-during-backup
>> new file mode 100755
>> index 0000000000..c3b7f1983d
>> --- /dev/null
>> +++ b/tests/qemu-iotests/tests/migrate-during-backup
>> @@ -0,0 +1,87 @@
>> +#!/usr/bin/env python3
>> +# group: migration disabled
>> +#
>> +# Copyright (c) 2021 Virtuozzo International GmbH
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +
>> +import os
>> +import iotests
>> +from iotests import qemu_img_create, qemu_io
>> +
>> +
>> +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
>> +
>> +
>> +class TestMigrateDuringBackup(iotests.QMPTestCase):
>> +    def tearDown(self):
>> +        self.vm.shutdown()
>> +        os.remove(disk_a)
>> +        os.remove(disk_b)
>> +        os.remove(mig_file)
>> +
>> +    def setUp(self):
>> +        qemu_img_create('-f', iotests.imgfmt, disk_a, size)
>> +        qemu_img_create('-f', iotests.imgfmt, disk_b, size)
>> +        qemu_io('-c', f'write 0 {size}', disk_a)
>> +
>> +        self.vm = iotests.VM().add_drive(disk_a)
>> +        self.vm.launch()
>> +        result = self.vm.qmp('blockdev-add', {
>> +            'node-name': 'target',
>> +            'driver': iotests.imgfmt,
>> +            'file': {
>> +                'driver': 'file',
>> +                'filename': disk_b
>> +            }
>> +        })
>> +        self.assert_qmp(result, 'return', {})
>> +
>> +    def test_migrate(self):
>> +        result = self.vm.qmp('blockdev-backup', device='drive0',
>> +                             target='target', sync='full',
>> +                             speed=1, x_perf={
>> +                                 'max-workers': 1,
>> +                                 'max-chunk': 64 * 1024
>> +                             })
>> +        self.assert_qmp(result, 'return', {})
>> +
>> +        result = self.vm.qmp('job-pause', id='drive0')
>> +        self.assert_qmp(result, 'return', {})
>> +
>> +        result = self.vm.qmp('migrate-set-capabilities',
>> +                             capabilities=[{'capability': 'events',
>> +                                            'state': True}])
>> +        self.assert_qmp(result, 'return', {})
>> +        result = self.vm.qmp('migrate', uri=mig_cmd)
>> +        self.assert_qmp(result, 'return', {})
>> +
>> +        self.vm.events_wait((('MIGRATION', {'data': {'status': 'completed'}}),
>> +                             ('MIGRATION', {'data': {'status': 'failed'}})))
> 
> So the migration failing is the result we expect here, right? Perhaps we should then have a loop that waits for MIGRATION events, and breaks on both status=completed and status=failed, but logs an error if the migration completes unexpectedly.
> 
> While I’ll give a
> 
> Reviewed-by: Hanna Reitz <hreitz@redhat.com>
> 
> either way, I’d like to know your opinion on this still.
> 

If migration finishes with "completed" backup will crash (current behavior).
So, if we just check that nothing crashes, we are OK with both completed and failed results.

If think more about that all...

Actually, nothing wrong if both migration and backup succeeded. It becomes possible if we don't inactivate backup target node. And actually we don't need that for migration.

On the other hand, in Qemu we are generaly OK with reading from inactivated node. But that's wrong: if I understand correctly, "inactive" means that file may be modified by external program (for example, by Qemu process on target for shared migration). In this perspective, we can't do migration during backup.

On one more hand, for non-shared migration it's absolutely possible to support migration during backup: you just need not stop source immediately after migration but wait for backup completion.


So, just to be more concrete, I can suggest to amend the following


diff --git a/tests/qemu-iotests/tests/migrate-during-backup b/tests/qemu-iotests/tests/migrate-during-backup
index c3b7f1983d..9bb7ebed3a 100755
--- a/tests/qemu-iotests/tests/migrate-during-backup
+++ b/tests/qemu-iotests/tests/migrate-during-backup
@@ -72,8 +72,13 @@ class TestMigrateDuringBackup(iotests.QMPTestCase):
          result = self.vm.qmp('migrate', uri=mig_cmd)
          self.assert_qmp(result, 'return', {})
  
-        self.vm.events_wait((('MIGRATION', {'data': {'status': 'completed'}}),
-                             ('MIGRATION', {'data': {'status': 'failed'}})))
+        e = self.vm.events_wait((('MIGRATION',
+                                  {'data': {'status': 'completed'}}),
+                                 ('MIGRATION',
+                                  {'data': {'status': 'failed'}})))
+
+        # Don't assert that e is 'failed' now: this way we'll miss
+        # possible crash when backup continues :)
  
          result = self.vm.qmp('block-job-set-speed', device='drive0',
                               speed=0)
@@ -81,6 +86,11 @@ class TestMigrateDuringBackup(iotests.QMPTestCase):
          result = self.vm.qmp('job-resume', id='drive0')
          self.assert_qmp(result, 'return', {})
  
+        # For future: if something changes so that both migration
+        # and backup pass, let's not miss that moment, as it may
+        # be a bug as well as an improvement.
+        self.assert_qmp(e, 'data/status', 'failed')
+
  
  if __name__ == '__main__':
      iotests.main(supported_fmts=['qcow2'],




-- 
Best regards,
Vladimir