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

[Vladimir Sementsov-Ogievskiy]

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.

> +    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?

> +    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/

> +
> +    log('\n--- Removing bitmaps A, B, C, and D ---\n')

what about failed transaction with remove command, for a full kit?

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




-- 
Best regards,
Vladimir