Re: [Qemu-devel] [PATCH v4 03/10] iotests: add test-case to 165 to test reopening qcow2 bitmaps to RW

[John Snow <jsnow@redhat.com>]

On 9/27/19 6:29 AM, Vladimir Sementsov-Ogievskiy wrote:
> 27.09.2019 10:28, Vladimir Sementsov-Ogievskiy wrote:
>> 27.09.2019 1:57, John Snow wrote:
>>>
>>>
>>> On 8/7/19 10:12 AM, Vladimir Sementsov-Ogievskiy wrote:
>>>> Reopening bitmaps to RW was broken prior to previous commit. Check that
>>>> it works now.
>>>>
>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>> ---
>>>>   tests/qemu-iotests/165     | 46 ++++++++++++++++++++++++++++++++++++--
>>>>   tests/qemu-iotests/165.out |  4 ++--
>>>>   2 files changed, 46 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/tests/qemu-iotests/165 b/tests/qemu-iotests/165
>>>> index 88f62d3c6d..dd93b5a2d0 100755
>>>> --- a/tests/qemu-iotests/165
>>>> +++ b/tests/qemu-iotests/165
>>>> @@ -43,10 +43,10 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
>>>>           os.remove(disk)
>>>>       def mkVm(self):
>>>> -        return iotests.VM().add_drive(disk)
>>>> +        return iotests.VM().add_drive(disk, opts='node-name=node0')
>>>>       def mkVmRo(self):
>>>> -        return iotests.VM().add_drive(disk, opts='readonly=on')
>>>> +        return iotests.VM().add_drive(disk, opts='readonly=on,node-name=node0')
>>>>       def getSha256(self):
>>>>           result = self.vm.qmp('x-debug-block-dirty-bitmap-sha256',
>>>> @@ -102,5 +102,47 @@ class TestPersistentDirtyBitmap(iotests.QMPTestCase):
>>>>           self.vm.shutdown()
>>>> +    def test_reopen_rw(self):
>>>> +        self.vm = self.mkVm()
>>>> +        self.vm.launch()
>>>> +        self.qmpAddBitmap()
>>>> +
>>>> +        # Calculate sha256 corresponding to regions1
>>>> +        self.writeRegions(regions1)
>>>> +        sha256 = self.getSha256()
>>>> +        result = self.vm.qmp('block-dirty-bitmap-clear', node='drive0',
>>>> +                             name='bitmap0')
>>>> +        self.assert_qmp(result, 'return', {})
>>>> +
>>>> +        self.vm.shutdown()
>>>> +
>>>> +        self.vm = self.mkVmRo()
>>>> +        self.vm.launch()
>>>> +
>>>> +        # We've loaded empty bitmap
>>>> +        assert sha256 != self.getSha256()
>>>> +
>>>> +        # Check that we are in RO mode
>>>> +        self.writeRegions(regions1)
>>>> +        assert sha256 != self.getSha256()
>>>> +
>>>
>>> the HMP monitor lets you attempt writes to a Read Only drive...? Or does
>>> it error out and we just don't check the reply?
>>
>> It must fail and we check this by comparing dirty bitmap hash.
>>
>>>
>>> I would prefer we use an actual dirty sector count here instead; we have
>>> the new API that should make it easy to do.
>>>
>>> If the debug SHA changes this might be a little fragile.
>>
>> Hmm, I agree that checking that bitmap is empty by comparing with some hash
>> is not very reliable. Will do.
>>
>>>
>>> ACK otherwise.
>>>
>>>> +        result = self.vm.qmp('x-blockdev-reopen', **{
>>>> +            'node-name': 'node0',
>>>> +            'driver': iotests.imgfmt,
>>>> +            'file': {
>>>> +                'driver': 'file',
>>>> +                'filename': disk
>>>> +            },
>>>> +            'read-only': False
>>>> +        })
>>>> +        self.assert_qmp(result, 'return', {})
>>>> +
>>>> +        # Check that bitmap is reopened to RW and we can write to it.
>>>> +        self.writeRegions(regions1)
>>>> +        assert sha256 == self.getSha256()
>>>> +
>>>> +        self.vm.shutdown()
>>>> +
>>>> +
>>>>   if __name__ == '__main__':
>>>>       iotests.main(supported_fmts=['qcow2'])
>>>> diff --git a/tests/qemu-iotests/165.out b/tests/qemu-iotests/165.out
>>>> index ae1213e6f8..fbc63e62f8 100644
>>>> --- a/tests/qemu-iotests/165.out
>>>> +++ b/tests/qemu-iotests/165.out
>>>> @@ -1,5 +1,5 @@
>>>> -.
>>>> +..
>>>>   ----------------------------------------------------------------------
>>>> -Ran 1 tests
>>>> +Ran 2 tests
>>>>   OK
>>>>
>>>
>>> This is a suggestion for an even more rigorous test:
>>>
>>> - Create bitmap
>>> - Write a region or two
>>> - Record the dirty count and the SHA; assert it is equal to known /
>>> predetermined values.
>>> - reopen RO
>>> - verify the bitmap still exists and that the hash and count are the same.
>>> - Stop the VM
>>> - Start the VM in readonly mode
>>> - verify the bitmap still exists and that the hash and count are the same.
>>> - Reopen-RW
>>> - verify the bitmap still exists and that the hash and count are the same.
>>> - Write further region(s)
>>> - Get the new dirty count and SHA, and assert it is equal to known /
>>> predetermined values.
>>>
>>
>> OK
>>
> 
> Ah, stop. We can't check reopen RO at this point, as it will be fixed in later patch
> 

Oh :(

Is it a reopen bug, a qcow2 bug, or a qcow2-bitmap bug?