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

[Vladimir Sementsov-Ogievskiy]

01.08.2019 20:43, Vladimir Sementsov-Ogievskiy wrote:
> 01.08.2019 20:35, Max Reitz wrote:
>> 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 <address@hidden>
>>>>>> ---
>>>>>>     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.
>>
> 
> Ah, understand. Hmm, then it's strange that I see that error..
> 
> Clearly, in job_finalize_single(), job_clean() is called first, which should 
> call backup_clean to unref target,
> then job_event_completed() is called...
> 
> 

But BlockJob.nodes are removed at very-end from block_job_free, so it seems to 
be another reference to target,
which is still alive when event is sent.

> 
>>
>>>>> 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 <address@hidden>
>>>>> Tested-by: Vladimir Sementsov-Ogievskiy <address@hidden>
>>>>
>>>> In any case, thanks!
>>>>
>>>> Max
>>>>
>>>
>>>
>>
>>
> 
> 


-- 
Best regards,
Vladimir