| Message ID | 20240313152822.626493-6-vsementsov@yandex-team.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | backup: discard-source parameter | expand |
Am 13.03.2024 um 16:28 hat Vladimir Sementsov-Ogievskiy geschrieben: > Add test for a new backup option: discard-source. > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > Reviewed-by: Fiona Ebner <f.ebner@proxmox.com> > Tested-by: Fiona Ebner <f.ebner@proxmox.com> This test fails for me, and it already does so after this commit that introduced it. I haven't checked what get_actual_size(), but I'm running on XFS, so its preallocation could be causing this. We generally avoid checking the number of allocated blocks in image files for this reason. Kevin backup-discard-source fail [19:45:49] [19:45:50] 0.8s failed, exit status 1 --- /home/kwolf/source/qemu/tests/qemu-iotests/tests/backup-discard-source.out +++ /home/kwolf/source/qemu/build-clang/scratch/qcow2-file-backup-discard-source/backup-discard-source.out.bad @@ -1,5 +1,14 @@ -.. +F. +====================================================================== +FAIL: test_discard_cbw (__main__.TestBackup.test_discard_cbw) +1. do backup(discard_source=True), which should inform +---------------------------------------------------------------------- +Traceback (most recent call last): + File "/home/kwolf/source/qemu/tests/qemu-iotests/tests/backup-discard-source", line 147, in test_discard_cbw + self.assertLess(get_actual_size(self.vm, 'temp'), 512 * 1024) +AssertionError: 1249280 not less than 524288 + ---------------------------------------------------------------------- Ran 2 tests -OK +FAILED (failures=1) Failures: backup-discard-source Failed 1 of 1 iotests
On 11.06.24 20:49, Kevin Wolf wrote: > Am 13.03.2024 um 16:28 hat Vladimir Sementsov-Ogievskiy geschrieben: >> Add test for a new backup option: discard-source. >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com> >> Tested-by: Fiona Ebner <f.ebner@proxmox.com> > > This test fails for me, and it already does so after this commit that > introduced it. I haven't checked what get_actual_size(), but I'm running > on XFS, so its preallocation could be causing this. We generally avoid > checking the number of allocated blocks in image files for this reason. > Hmm right, I see that relying on allocated size is bad thing. Better is to check block status, to see how many qcow2 clusters are allocated. Do we have some qmp command to get such information? The simplest way I see is to add dirty to temp block-node, and then check its dirty count through query-named-block-nodes
Am 12.06.2024 um 21:21 hat Vladimir Sementsov-Ogievskiy geschrieben: > On 11.06.24 20:49, Kevin Wolf wrote: > > Am 13.03.2024 um 16:28 hat Vladimir Sementsov-Ogievskiy geschrieben: > > > Add test for a new backup option: discard-source. > > > > > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> > > > Reviewed-by: Fiona Ebner <f.ebner@proxmox.com> > > > Tested-by: Fiona Ebner <f.ebner@proxmox.com> > > > > This test fails for me, and it already does so after this commit that > > introduced it. I haven't checked what get_actual_size(), but I'm running > > on XFS, so its preallocation could be causing this. We generally avoid > > checking the number of allocated blocks in image files for this reason. > > > > Hmm right, I see that relying on allocated size is bad thing. Better > is to check block status, to see how many qcow2 clusters are > allocated. > > Do we have some qmp command to get such information? The simplest way > I see is to add dirty to temp block-node, and then check its dirty > count through query-named-block-nodes Hm, does it have to be QMP in a running QEMU process? I'm not sure what the best way would be there. Otherwise, my approach would be 'qemu-img check' or even 'qemu-img map', depending on what you want to verify with it. Kevin
On 13.06.24 11:02, Kevin Wolf wrote: > Am 12.06.2024 um 21:21 hat Vladimir Sementsov-Ogievskiy geschrieben: >> On 11.06.24 20:49, Kevin Wolf wrote: >>> Am 13.03.2024 um 16:28 hat Vladimir Sementsov-Ogievskiy geschrieben: >>>> Add test for a new backup option: discard-source. >>>> >>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> >>>> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com> >>>> Tested-by: Fiona Ebner <f.ebner@proxmox.com> >>> >>> This test fails for me, and it already does so after this commit that >>> introduced it. I haven't checked what get_actual_size(), but I'm running >>> on XFS, so its preallocation could be causing this. We generally avoid >>> checking the number of allocated blocks in image files for this reason. >>> >> >> Hmm right, I see that relying on allocated size is bad thing. Better >> is to check block status, to see how many qcow2 clusters are >> allocated. >> >> Do we have some qmp command to get such information? The simplest way >> I see is to add dirty to temp block-node, and then check its dirty >> count through query-named-block-nodes > > Hm, does it have to be QMP in a running QEMU process? hmm, yes, seems in test_discard_written() we do want to examine running process. I'll try to go with bitmap > I'm not sure what > the best way would be there. > > Otherwise, my approach would be 'qemu-img check' or even 'qemu-img map', > depending on what you want to verify with it. > > Kevin >
diff --git a/tests/qemu-iotests/tests/backup-discard-source b/tests/qemu-iotests/tests/backup-discard-source new file mode 100755 index 0000000000..2391b12acd --- /dev/null +++ b/tests/qemu-iotests/tests/backup-discard-source @@ -0,0 +1,152 @@ +#!/usr/bin/env python3 +# +# Test backup discard-source parameter +# +# Copyright (c) Virtuozzo International GmbH. +# Copyright (c) Yandex +# +# 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_img_map, qemu_io + + +temp_img = os.path.join(iotests.test_dir, 'temp') +source_img = os.path.join(iotests.test_dir, 'source') +target_img = os.path.join(iotests.test_dir, 'target') +size = '1M' + + +def get_actual_size(vm, node_name): + nodes = vm.cmd('query-named-block-nodes', flat=True) + node = next(n for n in nodes if n['node-name'] == node_name) + return node['image']['actual-size'] + + +class TestBackup(iotests.QMPTestCase): + def setUp(self): + qemu_img_create('-f', iotests.imgfmt, source_img, size) + qemu_img_create('-f', iotests.imgfmt, temp_img, size) + qemu_img_create('-f', iotests.imgfmt, target_img, size) + qemu_io('-c', 'write 0 1M', source_img) + + self.vm = iotests.VM() + self.vm.launch() + + self.vm.cmd('blockdev-add', { + 'node-name': 'cbw', + 'driver': 'copy-before-write', + 'file': { + 'driver': iotests.imgfmt, + 'file': { + 'driver': 'file', + 'filename': source_img, + } + }, + 'target': { + 'driver': iotests.imgfmt, + 'discard': 'unmap', + 'node-name': 'temp', + 'file': { + 'driver': 'file', + 'filename': temp_img + } + } + }) + + self.vm.cmd('blockdev-add', { + 'node-name': 'access', + 'discard': 'unmap', + 'driver': 'snapshot-access', + 'file': 'cbw' + }) + + self.vm.cmd('blockdev-add', { + 'driver': iotests.imgfmt, + 'node-name': 'target', + 'file': { + 'driver': 'file', + 'filename': target_img + } + }) + + self.assertLess(get_actual_size(self.vm, 'temp'), 512 * 1024) + + def tearDown(self): + # That should fail, because region is discarded + self.vm.hmp_qemu_io('access', 'read 0 1M') + + self.vm.shutdown() + + self.assertTrue('read failed: Permission denied' in self.vm.get_log()) + + # Final check that temp image is empty + mapping = qemu_img_map(temp_img) + self.assertEqual(len(mapping), 1) + self.assertEqual(mapping[0]['start'], 0) + self.assertEqual(mapping[0]['length'], 1024 * 1024) + self.assertEqual(mapping[0]['data'], False) + + os.remove(temp_img) + os.remove(source_img) + os.remove(target_img) + + def do_backup(self): + self.vm.cmd('blockdev-backup', device='access', + sync='full', target='target', + job_id='backup0', + discard_source=True) + + self.vm.event_wait(name='BLOCK_JOB_COMPLETED') + + def test_discard_written(self): + """ + 1. Guest writes + 2. copy-before-write operation, data is stored to temp + 3. start backup(discard_source=True), check that data is + removed from temp + """ + # Trigger copy-before-write operation + result = self.vm.hmp_qemu_io('cbw', 'write 0 1M') + self.assert_qmp(result, 'return', '') + + # Check that data is written to temporary image + self.assertGreater(get_actual_size(self.vm, 'temp'), 1024 * 1024) + + self.do_backup() + + def test_discard_cbw(self): + """ + 1. do backup(discard_source=True), which should inform + copy-before-write that data is not needed anymore + 2. Guest writes + 3. Check that copy-before-write operation is not done + """ + self.do_backup() + + # Try trigger copy-before-write operation + result = self.vm.hmp_qemu_io('cbw', 'write 0 1M') + self.assert_qmp(result, 'return', '') + + # Check that data is not written to temporary image, as region + # is discarded from copy-before-write process + self.assertLess(get_actual_size(self.vm, 'temp'), 512 * 1024) + + +if __name__ == '__main__': + iotests.main(supported_fmts=['qcow2'], + supported_protocols=['file']) diff --git a/tests/qemu-iotests/tests/backup-discard-source.out b/tests/qemu-iotests/tests/backup-discard-source.out new file mode 100644 index 0000000000..fbc63e62f8 --- /dev/null +++ b/tests/qemu-iotests/tests/backup-discard-source.out @@ -0,0 +1,5 @@ +.. +---------------------------------------------------------------------- +Ran 2 tests + +OK