Re: [PATCH for-5.1 v2 2/2] iotests: add test for unaligned granularity bitmap backup

[Stefan Reiter <s.reiter@proxmox.com>]

On 8/10/20 5:11 PM, Max Reitz wrote:
> (Note: When submitting a patch series with multiple patches, our
> guidelines require a cover letter:
> https://wiki.qemu.org/Contribute/SubmitAPatch#Include_a_meaningful_cover_letter
> 
> But not too important now.)
> 

Sorry, remembered for next time. Thanks for applying the patches!

> On 10.08.20 11:55, Stefan Reiter wrote:
>> Start a VM with a 4097 byte image attached, add a 4096 byte granularity
>> dirty bitmap, mark it dirty, and then do a backup.
>>
>> This used to run into an assert and fail, check that it works as
>> expected and also check the created image to ensure that misaligned
>> backups in general work correctly.
>>
>> Signed-off-by: Stefan Reiter <s.reiter@proxmox.com>
>> ---
>>
>> I saw Andrey's big series covering iotest 303 so I went for 304.
> 
> Works for me.
> 
>> Never submitted
>> one before so I hope that's okay, if not feel free to renumber it.
> 
> Yep, if there’s a clash I tend to just renumber it when applying it.
> 
>>   tests/qemu-iotests/304     | 68 ++++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/304.out |  2 ++
>>   tests/qemu-iotests/group   |  1 +
>>   3 files changed, 71 insertions(+)
>>   create mode 100755 tests/qemu-iotests/304
>>   create mode 100644 tests/qemu-iotests/304.out
>>
>> diff --git a/tests/qemu-iotests/304 b/tests/qemu-iotests/304
>> new file mode 100755
>> index 0000000000..9a3b0224fa
>> --- /dev/null
>> +++ b/tests/qemu-iotests/304
>> @@ -0,0 +1,68 @@
>> +#!/usr/bin/env python3
>> +#
>> +# Tests dirty-bitmap backup with unaligned bitmap granularity
>> +#
>> +# Copyright (c) 2020 Proxmox Server Solutions
>> +#
>> +# 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/>.
>> +#
>> +# owner=s.reiter@proxmox.com
>> +
>> +import iotests
>> +from iotests import qemu_img_create, qemu_img_log, file_path
>> +
>> +iotests.script_initialize(supported_fmts=['qcow2'],
>> +                          supported_protocols=['file'])
>> +
>> +test_img = file_path('test.qcow2')
>> +target_img = file_path('target.qcow2')
>> +
>> +# unaligned by one byte
>> +image_len = 4097
>> +bitmap_granularity = 4096
>> +
>> +qemu_img_create('-f', iotests.imgfmt, test_img, str(image_len))
>> +
>> +# create VM and add dirty bitmap
>> +vm = iotests.VM().add_drive(test_img)
>> +vm.launch()
>> +
>> +vm.qmp('block-dirty-bitmap-add', **{
>> +    'node': 'drive0',
>> +    'name': 'bitmap0',
>> +    'granularity': bitmap_granularity
>> +})
>> +
>> +# mark entire bitmap as dirty
>> +vm.hmp_qemu_io('drive0', 'write -P0x16 0 4096');
>> +vm.hmp_qemu_io('drive0', 'write -P0x17 4097 1');
> 
> s/4097/4096/?
> 
> (4097 works, too, because of something somewhere aligning up the 4097 to
> 512 byte sectors, I suppose, but I don’t think it’s the address you want
> here)
> 

You're right, it seems counting is hard. I'll take you up on the offer 
from your other mail, you can fix this please :)

>> +
>> +# do backup and wait for completion
>> +vm.qmp('drive-backup', **{
>> +    'device': 'drive0',
>> +    'sync': 'full',
>> +    'target': target_img,
>> +    'bitmap': 'bitmap0',
>> +    'bitmap-mode': 'on-success'
> 
> The bitmap is unnecessary, isn’t it?  I.e., if I drop the
> block-dirty-bitmap-add call and the bitmap* parameters here, I still get
> an assertion failure without patch 1.
> 
> Not that it really matters, it’s just that this makes it look like less
> of an issue than it actually is.  (Which is why I’d drop the bitmap
> stuff in case there’s no actual reason for it.)
> 

Oh my, I just realized that I misunderstood the root cause then. I mean, 
the fix is fine, I see it now, but you're right, no dirty bitmap needed 
- you can remove that as well if you want.

>> +})
>> +
>> +event = vm.event_wait(name='BLOCK_JOB_COMPLETED',
>> +                      match={'data': {'device': 'drive0'}},
>> +                      timeout=5.0)
> 
> (By the way, “vm.run_job('drive0', auto_dismiss=True)” would have worked
> as well.  But since the backup job just needs waiting for a single
> event, I suppose it doesn’t matter.  Just a hint in case you start
> writing more iotests in the future.)
> 
> Max
>