Re: [PATCH 3/3] iotests: Backup with different source/target size

[Vladimir Sementsov-Ogievskiy]

30.04.2020 14:41, Kevin Wolf wrote:
> Am 29.04.2020 um 14:28 hat Vladimir Sementsov-Ogievskiy geschrieben:
> > 29.04.2020 14:15, Kevin Wolf wrote:
> > > This tests that the backup jobs catches situations where the target node
> > > has a different size than the source node. It must also forbid resize
> > > operations when the job is already running.
> > >
> > > Signed-off-by: Kevin Wolf <address@hidden>
> > > ---
> > >    tests/qemu-iotests/055     | 60 ++++++++++++++++++++++++++++++++++++--
> > >    tests/qemu-iotests/055.out |  4 +--
>
> One general remark and question that came up while I was running 055 a
> lot and really got annonyed by the long time it takes:
>
> TestDriveCompression is quite unconventional in that 055 is raw/qcow2
> only per se, but some of the test cases always test qcow2 and vmdk.

Yes, that's bad. Oh, seems I have a patch for it not still sent. Will do soon.

> The
> slow one is vmdk.
>
> I found out that zero writes in vmdk are completely broken (I'll send
> patches), but even after fixing this, it's still slow. I think this is
> the combination of VMDK not having writeback metadata caching and
> backup serving lots of tiny 64k zero writes.
>
> Has anyone ever looked into making backup use more reasonable request
> sizes for the background copy like mirror does?

I am :)

The whole series improving backup is
 "[RFC 00/24] backup performance: block_status + async"
First part is already merged, current chunk close to be finished:
 "[PATCH v4 0/5] block-copy: use aio-task-pool"

> > >    2 files changed, 60 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/tests/qemu-iotests/055 b/tests/qemu-iotests/055
> > > index 82b9f5f47d..243d66a62e 100755
> > > --- a/tests/qemu-iotests/055
> > > +++ b/tests/qemu-iotests/055
> > > @@ -48,8 +48,10 @@ class TestSingleDrive(iotests.QMPTestCase):
> > >        def setUp(self):
> > >            qemu_img('create', '-f', iotests.imgfmt, blockdev_target_img, 
> > > str(image_len))
> > > -        self.vm = iotests.VM().add_drive('blkdebug::' + test_img)
> > > -        self.vm.add_drive(blockdev_target_img, interface="none")
> > > +        self.vm = iotests.VM()
> > > +        self.vm.add_drive('blkdebug::' + test_img, 'node-name=source')
> > > +        self.vm.add_drive(blockdev_target_img, 'node-name=target',
> > > +                          interface="none")
> > >            if iotests.qemu_default_machine == 'pc':
> > >                self.vm.add_drive(None, 'media=cdrom', 'ide')
> > >            self.vm.launch()
> > > @@ -112,6 +114,60 @@ class TestSingleDrive(iotests.QMPTestCase):
> > >        def test_pause_blockdev_backup(self):
> > >            self.do_test_pause('blockdev-backup', 'drive1', blockdev_target_img)
> > > +    def test_source_resize_blockdev_backup(self):
> > > +        self.assert_no_active_block_jobs()
> >
> > this will never fire, as vm is created a moment before, I'd drop it.
>
> This pattern exists all over the place in 055, but you're right, it's
> kind of pointless.
>
> > > +
> > > +        def pre_finalize():
> > > +            result = self.vm.qmp('block_resize', device='drive0', size=65536)
> > > +            self.assert_qmp(result, 'error/class', 'GenericError')
> > > +
> > > +            result = self.vm.qmp('block_resize', node_name='source', 
> > > size=65536)
> > > +            self.assert_qmp(result, 'error/class', 'GenericError')
> > > +
> > > +        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
> > > +                             target='drive1', sync='full', auto_finalize=False,
> > > +                             auto_dismiss=False)
> > > +        self.assert_qmp(result, 'return', {})
> > > +
> > > +        self.vm.run_job('job0', auto_finalize=False, pre_finalize=pre_finalize,
> > > +                        use_log=False)
> > > +
> > > +    def test_target_resize_blockdev_backup(self):
> > > +        self.assert_no_active_block_jobs()
> > > +
> > > +        def pre_finalize():
> > > +            result = self.vm.qmp('block_resize', device='drive1', size=65536)
> > > +            self.assert_qmp(result, 'error/class', 'GenericError')
> > > +
> > > +            result = self.vm.qmp('block_resize', node_name='target', 
> > > size=65536)
> > > +            self.assert_qmp(result, 'error/class', 'GenericError')
> > > +
> > > +        result = self.vm.qmp('blockdev-backup', job_id='job0', device='drive0',
> > > +                             target='drive1', sync='full', auto_finalize=False,
> > > +                             auto_dismiss=False)
> > > +        self.assert_qmp(result, 'return', {})
> > > +
> > > +        self.vm.run_job('job0', auto_finalize=False, pre_finalize=pre_finalize,
> > > +                        use_log=False)
> >
> > these two functions are almost identical.. worth refactoring to be use common 
> > helper?
>
> Ok, I'll see what I can do.
>
> Kevin


--
Best regards,
Vladimir