Re: [PATCH 21/21] itotests/222: add test-case for copy-before-write filter

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

18.05.2021 18:24, Max Reitz wrote:
> On 17.05.21 08:44, Vladimir Sementsov-Ogievskiy wrote:
>> New fleecing method becomes available: copy-before-write filter.
>>
>> Actually we don't need backup job to setup image fleecing. Add test
>> fore new recommended way of image fleecing.
> 
> *for
> 
>>
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   tests/qemu-iotests/222     | 56 ++++++++++++++++++++++-------
>>   tests/qemu-iotests/222.out | 72 ++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 115 insertions(+), 13 deletions(-)
> 
> Looks good, just wondering about some variable usage (why is src_node a BB name?) and whether the virtio-blk qdev path couldn’t be expressed in some nicer way.
> 
>> diff --git a/tests/qemu-iotests/222 b/tests/qemu-iotests/222
>> index b48afe623e..8f8e1e4d7f 100755
>> --- a/tests/qemu-iotests/222
>> +++ b/tests/qemu-iotests/222
>> @@ -48,11 +48,13 @@ remainder = [("0xd5", "0x108000",  "32k"), # Right-end of partial-left [1]
>>                ("0xdc", "32M",       "32k"), # Left-end of partial-right [2]
>>                ("0xcd", "0x3ff0000", "64k")] # patterns[3]
>> -with iotests.FilePath('base.img') as base_img_path, \
>> -     iotests.FilePath('fleece.img') as fleece_img_path, \
>> -     iotests.FilePath('nbd.sock', base_dir=iotests.sock_dir) as nbd_sock_path, \
>> -     iotests.VM() as vm:
>> +def do_test(use_cbw, base_img_path, fleece_img_path, nbd_sock_path, vm):
>> +    if use_cbw:
>> +        log('=== Test filter based fleecing ===')
>> +    else:
>> +        log('=== Test backup(sync=none) based fleecing ===')
>> +    log('')
>>       log('--- Setting up images ---')
>>       log('')
>> @@ -69,7 +71,7 @@ with iotests.FilePath('base.img') as base_img_path, \
>>       log('--- Launching VM ---')
>>       log('')
>> -    vm.add_drive(base_img_path)
>> +    vm.add_drive(base_img_path, "node-name=node0")
>>       vm.launch()
>>       log('Done')
>>
> 
> Afterwards, src_node is set to 'drive0'.  So src_node is actually a BB name...
> 
>> @@ -91,11 +93,22 @@ with iotests.FilePath('base.img') as base_img_path, \
>>           "backing": src_node,
>>       }))
>> -    # Establish COW from source to fleecing node
>> -    log(vm.qmp("blockdev-backup",
>> -               device=src_node,
>> -               target=tgt_node,
>> -               sync="none"))
>> +    # Establish CBW from source to fleecing node
>> +    if use_cbw:
>> +        log(vm.qmp("blockdev-add", **{
>> +            "driver": "copy-before-write",
>> +            "node-name": "fl-cbw",
>> +            "file": src_node,
>> +            "target": tgt_node
> 
> Which makes this look strange, and also TIL (or maybe “today I was reminded”?) that you can use BB names as node references.
> 
> (It’s also already weird in the `"backing": src_node` line right at the beginning of this hunk...)
> 
> So I guess it works, but I’d find it would be nicer to distinguish between the node name and the BB name.

Oh yes, that's all the mess, I'll improve.

> 
>> +        }))
>> +
>> +        log(vm.qmp("qom-set", path="/machine/peripheral-anon/device[0]",
>> +                   property="drive", value="fl-cbw"))
> 
> Is that path portable across targets?  Should we maybe instead manually add a virtio-scsi device that we can give a proper name?
> 
> Or I suppose we could do
> 
> path = next(dev for dev in vm.qmp('query-block')['return']
>              if dev['device'] == 'drive0')['qdev']

OK

> 
>> +    else:
>> +        log(vm.qmp("blockdev-backup",
>> +                   device=src_node,
>> +                   target=tgt_node,
>> +                   sync="none"))
>>       log('')
>>       log('--- Setting up NBD Export ---')
>> @@ -139,9 +152,15 @@ with iotests.FilePath('base.img') as base_img_path, \
>>       log('--- Cleanup ---')
>>       log('')
>> -    log(vm.qmp('block-job-cancel', device=src_node))
>> -    log(vm.event_wait('BLOCK_JOB_CANCELLED'),
>> -        filters=[iotests.filter_qmp_event])
>> +    if use_cbw:
>> +        log(vm.qmp("qom-set", path="/machine/peripheral-anon/device[0]",
>> +                   property="drive", value="node0"))
> 
> If src_node were 'node0', we wouldn’t need to use the 'node0' literal here and could use src_node instead.  (Because it kind of seems like this test wants to use those variables instead of literals.  I mean, we could also just call the node 'src-node', but, well.)
> 
> Max
> 
>> +        log(vm.qmp("blockdev-del", node_name="fl-cbw"))
>> +    else:
>> +        log(vm.qmp('block-job-cancel', device=src_node))
>> +        log(vm.event_wait('BLOCK_JOB_CANCELLED'),
>> +            filters=[iotests.filter_qmp_event])
>> +
>>       log(vm.qmp('nbd-server-stop'))
>>       log(vm.qmp('blockdev-del', node_name=tgt_node))
>>       vm.shutdown()
> 

Great thanks for reviewing!

-- 
Best regards,
Vladimir