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

Vladimir Sementsov-Ogievskiy posted 21 patches 4 years, 8 months ago
Maintainers: John Snow <jsnow@redhat.com>, Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>, Markus Armbruster <armbru@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Eric Blake <eblake@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, Kevin Wolf <kwolf@redhat.com>, "Daniel P. Berrangé" <berrange@redhat.com>, Max Reitz <mreitz@redhat.com>
There is a newer version of this series
[PATCH 21/21] itotests/222: add test-case for copy-before-write filter
Posted by Vladimir Sementsov-Ogievskiy 4 years, 8 months ago
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.

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(-)

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')
 
@@ -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
+        }))
+
+        log(vm.qmp("qom-set", path="/machine/peripheral-anon/device[0]",
+                   property="drive", value="fl-cbw"))
+    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"))
+        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()
@@ -157,3 +176,14 @@ with iotests.FilePath('base.img') as base_img_path, \
 
     log('')
     log('Done')
+
+def test(use_cbw):
+    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:
+        do_test(use_cbw, base_img_path, fleece_img_path, nbd_sock_path, vm)
+
+test(False)
+test(True)
diff --git a/tests/qemu-iotests/222.out b/tests/qemu-iotests/222.out
index 16643dde30..bdc0f7912f 100644
--- a/tests/qemu-iotests/222.out
+++ b/tests/qemu-iotests/222.out
@@ -1,3 +1,5 @@
+=== Test backup(sync=none) based fleecing ===
+
 --- Setting up images ---
 
 Done
@@ -65,3 +67,73 @@ read -P0xdc 32M 32k
 read -P0xcd 0x3ff0000 64k
 
 Done
+=== Test filter based fleecing ===
+
+--- Setting up images ---
+
+Done
+
+--- Launching VM ---
+
+Done
+
+--- Setting up Fleecing Graph ---
+
+{"return": {}}
+{"return": {}}
+{"return": {}}
+
+--- Setting up NBD Export ---
+
+{"return": {}}
+{"return": {}}
+
+--- Sanity Check ---
+
+read -P0x5d 0 64k
+read -P0xd5 1M 64k
+read -P0xdc 32M 64k
+read -P0xcd 0x3ff0000 64k
+read -P0 0x00f8000 32k
+read -P0 0x2010000 32k
+read -P0 0x3fe0000 64k
+
+--- Testing COW ---
+
+write -P0xab 0 64k
+{"return": ""}
+write -P0xad 0x00f8000 64k
+{"return": ""}
+write -P0x1d 0x2008000 64k
+{"return": ""}
+write -P0xea 0x3fe0000 64k
+{"return": ""}
+
+--- Verifying Data ---
+
+read -P0x5d 0 64k
+read -P0xd5 1M 64k
+read -P0xdc 32M 64k
+read -P0xcd 0x3ff0000 64k
+read -P0 0x00f8000 32k
+read -P0 0x2010000 32k
+read -P0 0x3fe0000 64k
+
+--- Cleanup ---
+
+{"return": {}}
+{"return": {}}
+{"return": {}}
+{"return": {}}
+
+--- Confirming writes ---
+
+read -P0xab 0 64k
+read -P0xad 0x00f8000 64k
+read -P0x1d 0x2008000 64k
+read -P0xea 0x3fe0000 64k
+read -P0xd5 0x108000 32k
+read -P0xdc 32M 32k
+read -P0xcd 0x3ff0000 64k
+
+Done
-- 
2.29.2


Re: [PATCH 21/21] itotests/222: add test-case for copy-before-write filter
Posted by Max Reitz 4 years, 8 months ago
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.

> +        }))
> +
> +        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']

> +    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()


Re: [PATCH 21/21] itotests/222: add test-case for copy-before-write filter
Posted by Vladimir Sementsov-Ogievskiy 4 years, 8 months ago
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