Re: [Qemu-devel] [PATCH v5 11/11] iotests: add iotest 236 for testing bitmap merge

[John Snow]

On 12/20/18 7:12 AM, Vladimir Sementsov-Ogievskiy wrote:
> 20.12.2018 5:29, John Snow wrote:
>> New interface, new smoke test.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
> 
> [...]
> 
>> +    # A: 7 clusters
>> +    # B: 4 clusters
>> +    # C: 6 clusters
>> +    log(query_bitmaps(vm), indent=2)
>> +
>> +    log('\n--- Submitting Bad Merge ---\n')
> 
> aha, spent some time, trying to understand, what is bad with merge, until 
> understand that
> that is abort. I didn't sleep enough last night, but anyway, 'Aborting Merge 
> Transaction'
> is a bit clearer, I think.
> 

Sure, I'll rephrase it: "Submitting & Aborting Merge Transaction"

>> +    vm.qmp_log("transaction", indent=2, actions=[
>> +        { "type": "block-dirty-bitmap-add",
>> +          "data": { "node": "drive0", "name": "bitmapD",
>> +                    "disabled": True, "granularity": granularity }},
>> +        { "type": "block-dirty-bitmap-merge",
>> +          "data": { "node": "drive0", "target": "bitmapD",
>> +                    "bitmaps": ["bitmapB", "bitmapC"] }},
>> +        { "type": "abort", "data": {}}
>> +    ])
>> +    log(query_bitmaps(vm), indent=2)
>> +
>> +    log('\n--- Creating D as a merge of B & C ---\n')
>> +    # Good hygiene: create a disabled bitmap as a merge target.
>> +    vm.qmp_log("transaction", indent=2, actions=[
>> +        { "type": "block-dirty-bitmap-add",
>> +          "data": { "node": "drive0", "name": "bitmapD",
>> +                    "disabled": True, "granularity": granularity }},
>> +        { "type": "block-dirty-bitmap-merge",
>> +          "data": { "node": "drive0", "target": "bitmapD",
>> +                    "bitmaps": ["bitmapB", "bitmapC"] }}
>> +    ])
>> +
>> +    # A and D should now both have 7 clusters apiece.
>> +    # B and C remain unchanged with 4 and 6 respectively.
>> +    log(query_bitmaps(vm), indent=2)
>> +
>> +    # A and D should be equivalent.
>> +    # Some formats round the size of the disk, so don't print the checksums.
> 
> Just interested: round 64M? to what?
> 

VPC does weird stuff. If you ask for 64M you get 64M+16K. "round" is
maybe a bad adjective here, but VPC really won't give you what you ask
for. Loosening the restriction to "generic" was a good idea.

>> +    check_a = vm.qmp('x-debug-block-dirty-bitmap-sha256',
>> +                     node="drive0", name="bitmapA")['return']['sha256']
>> +    check_b = vm.qmp('x-debug-block-dirty-bitmap-sha256',
>> +                     node="drive0", name="bitmapD")['return']['sha256']
>> +    assert(check_a == check_b)
> 
> hmm, a funny suggestion: s/check_b/check_d/

Oh, yes, that would be better.

> 
>> +
>> +    log('\n--- Removing bitmaps A, B, C, and D ---\n')
> 
> what about failed transaction with remove command, for a full kit?
> 

Remove isn't transactionable!

>> +    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapA")
>> +    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapB")
>> +    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapC")
>> +    vm.qmp_log("block-dirty-bitmap-remove", node="drive0", name="bitmapD")
>> +
>> +    log('\n--- Final Query ---\n')
>> +    log(query_bitmaps(vm), indent=2)
>> +
>> +    log('\n--- Done ---\n')
>> +    vm.shutdown()
> 
> 
> with or without any of my suggestions:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <address@hidden>
> 

Thanks!

--js