Re: [Qemu-devel] [PATCH for-4.1 2/2] iotests: Test backup job with two guest writes

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6547 bytes --]

On 01.08.19 19:25, Vladimir Sementsov-Ogievskiy wrote:
> 01.08.2019 20:06, Max Reitz wrote:
>> On 01.08.19 18:03, Vladimir Sementsov-Ogievskiy wrote:
>>> 01.08.2019 18:12, Max Reitz wrote:
>>>> Perform two guest writes to not yet backed up areas of an image, where
>>>> the former touches an inner area of the latter.
>>>>
>>>> Before HEAD^, copy offloading broke this in two ways:
>>>> (1) The output differs from the reference output (what the source was
>>>>       before the guest writes).
>>>> (2) But you will not see that in the failing output, because the job
>>>>       offset is reported as being greater than the job length.  This is
>>>>       because one cluster is copied twice, and thus accounted for twice,
>>>>       but of course the job length does not increase.
>>>>
>>>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>>>> ---
>>>>    tests/qemu-iotests/056     | 34 ++++++++++++++++++++++++++++++++++
>>>>    tests/qemu-iotests/056.out |  4 ++--
>>>>    2 files changed, 36 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/056 b/tests/qemu-iotests/056
>>>> index f40fc11a09..d7198507f5 100755
>>>> --- a/tests/qemu-iotests/056
>>>> +++ b/tests/qemu-iotests/056
>>>> @@ -133,6 +133,7 @@ class BackupTest(iotests.QMPTestCase):
>>>>            self.vm = iotests.VM()
>>>>            self.test_img = img_create('test')
>>>>            self.dest_img = img_create('dest')
>>>> +        self.ref_img = img_create('ref')
>>>>            self.vm.add_drive(self.test_img)
>>>>            self.vm.launch()
>>>>    
>>>> @@ -140,6 +141,7 @@ class BackupTest(iotests.QMPTestCase):
>>>>            self.vm.shutdown()
>>>>            try_remove(self.test_img)
>>>>            try_remove(self.dest_img)
>>>> +        try_remove(self.ref_img)
>>>>    
>>>>        def hmp_io_writes(self, drive, patterns):
>>>>            for pattern in patterns:
>>>> @@ -177,6 +179,38 @@ class BackupTest(iotests.QMPTestCase):
>>>>                self.assert_qmp(event, 'data/error', qerror)
>>>>                return False
>>>>    
>>>> +    def test_overlapping_writes(self):
>>>> +        # Write something to back up
>>>> +        self.hmp_io_writes('drive0', [('42', '0M', '2M')])
>>>> +
>>>> +        # Create a reference backup
>>>> +        self.qmp_backup_and_wait(device='drive0', format=iotests.imgfmt,
>>>> +                                 sync='full', target=self.ref_img)
>>>> +
>>>> +        # Now to the test backup: We simulate the following guest
>>>> +        # writes:
>>>> +        # (1) [1M + 64k, 1M + 128k): Afterwards, everything in that
>>>> +        #     area should be in the target image, and we must not copy
>>>> +        #     it again (because the source image has changed now)
>>>> +        #     (64k is the job's cluster size)
>>>> +        # (2) [1M, 2M): The backup job must not get overeager.  It
>>>> +        #     must copy [1M, 1M + 64k) and [1M + 128k, 2M) separately,
>>>> +        #     but not the area in between.
>>>> +
>>>> +        self.qmp_backup(device='drive0', format=iotests.imgfmt, sync='full',
>>>> +                        target=self.dest_img, speed=1)
>>>> +
>>>> +        self.hmp_io_writes('drive0', [('23', '%ik' % (1024 + 64), '64k'),
>>>> +                                      ('66', '1M', '1M')])
>>>> +
>>>> +        # Let the job complete
>>>> +        res = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
>>>> +        self.assert_qmp(res, 'return', {})
>>>> +        self.qmp_backup_wait('drive0')
>>>> +
>>>> +        self.assertTrue(iotests.compare_images(self.ref_img, self.dest_img),
>>>> +                        'target image does not match reference image')
>>>> +
>>>>        def test_dismiss_false(self):
>>>>            res = self.vm.qmp('query-block-jobs')
>>>>            self.assert_qmp(res, 'return', [])
>>>> diff --git a/tests/qemu-iotests/056.out b/tests/qemu-iotests/056.out
>>>> index dae404e278..36376bed87 100644
>>>> --- a/tests/qemu-iotests/056.out
>>>> +++ b/tests/qemu-iotests/056.out
>>>> @@ -1,5 +1,5 @@
>>>> -.........
>>>> +..........
>>>>    ----------------------------------------------------------------------
>>>> -Ran 9 tests
>>>> +Ran 10 tests
>>>>    
>>>>    OK
>>>>
>>>
>>> Failed for me:
>>> -..........
>>> +qemu-img: Could not open '/work/src/qemu/master/tests/qemu-iotests/scratch/dest.qcow2': Failed to get shared "write" lock
>>> +Is another process using the image [/work/src/qemu/master/tests/qemu-iotests/scratch/dest.qcow2]?
>>> +......F...
>>> +======================================================================
>>> +FAIL: test_overlapping_writes (__main__.BackupTest)
>>> +----------------------------------------------------------------------
>>> +Traceback (most recent call last):
>>> +  File "056", line 212, in test_overlapping_writes
>>> +    'target image does not match reference image')
>>> +AssertionError: False is not true : target image does not match reference image
>>> +
>>>    ----------------------------------------------------------------------
>>>    Ran 10 tests
>>>
>>> -OK
>>> +FAILED (failures=1)
>>
>> Hm.  I hoped seeing BLOCK_JOB_COMPLETED would be enough.
> 
> The problem is higher: "Failed to get shared "write" lock". Because of this iotests.compare_images
> can't work. So, because of locking, we need to shutdown qemu before starting qemu-img.
> And it's done so in test_complete_top() (I just looked at it as it's the only other user of compare_images
> in 056)

The destination image shouldn’t be in use by qemu after the block job is
done, though.  Therefore, there shouldn’t be a lock issue.  That’s what
I meant.

Max

>>> So, with applied
>>>
>>> @@ -207,6 +207,7 @@ class BackupTest(iotests.QMPTestCase):
>>>            res = self.vm.qmp('block-job-set-speed', device='drive0', speed=0)
>>>            self.assert_qmp(res, 'return', {})
>>>            self.qmp_backup_wait('drive0')
>>> +        self.vm.shutdown()
>>>
>>>            self.assertTrue(iotests.compare_images(self.ref_img, self.dest_img),
>>>                            'target image does not match reference image')
>>
>> I’d personally prefer auto_dismiss=False and then block-job-dismiss.
>> Although I can’t give a reason why.
>>
>>> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>> Tested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>
>> In any case, thanks!
>>
>> Max
>>
> 
> 



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