Re: [PATCH qemu 4/4] iotests: add test for bitmap mirror

[Fabian Grünbichler]

On October 1, 2020 7:31 pm, Max Reitz wrote:
> On 22.09.20 11:14, Fabian Grünbichler wrote:
>> heavily based on/practically forked off iotest 257 for bitmap backups,
>> but:
>> 
>> - no writes to filter node 'mirror-top' between completion and
>> finalization, as those seem to deadlock?
>> - no inclusion of not-yet-available full/top sync modes in combination
>> with bitmaps
>> - extra set of reference/test mirrors to verify that writes in parallel
>> with active mirror work
>> 
>> intentionally keeping copyright and ownership of original test case to
>> honor provenance.
>> 
>> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
>> ---
>>  tests/qemu-iotests/306     |  546 +++++++
>>  tests/qemu-iotests/306.out | 2846 ++++++++++++++++++++++++++++++++++++
>>  tests/qemu-iotests/group   |    1 +
>>  3 files changed, 3393 insertions(+)
>>  create mode 100755 tests/qemu-iotests/306
>>  create mode 100644 tests/qemu-iotests/306.out
>> 
>> diff --git a/tests/qemu-iotests/306 b/tests/qemu-iotests/306
>> new file mode 100755
>> index 0000000000..1bb8bd4138
>> --- /dev/null
>> +++ b/tests/qemu-iotests/306
> 
> [...]
> 
>> +def test_bitmap_sync(bsync_mode, msync_mode='bitmap', failure=None):
>> +    """
>> +    Test bitmap mirror routines.
>> +
>> +    :param bsync_mode: Is the Bitmap Sync mode, and can be any of:
>> +        - on-success: This is the "incremental" style mode. Bitmaps are
>> +                      synchronized to what was copied out only on success.
>> +                      (Partial images must be discarded.)
>> +        - never:      This is the "differential" style mode.
>> +                      Bitmaps are never synchronized.
>> +        - always:     This is a "best effort" style mode.
>> +                      Bitmaps are always synchronized, regardless of 
>> failure.
>> +                      (Partial images must be kept.)
>> +
>> +    :param msync_mode: The mirror sync mode to use for the first mirror.
>> +                       Can be any one of:
>> +        - bitmap: mirrors based on bitmap manifest.
>> +        - full:   Full mirrors.
>> +        - top:    Full mirrors of the top layer only.
>> +
>> +    :param failure: Is the (optional) failure mode, and can be any of:
>> +        - None:         No failure. Test the normative path. Default.
>> +        - simulated:    Cancel the job right before it completes.
>> +                        This also tests writes "during" the job.
>> +        - intermediate: This tests a job that fails mid-process and produces
>> +                        an incomplete mirror. Testing limitations prevent
>> +                        testing competing writes.
>> +    """
>> +    with iotests.FilePath('img', 'bsync1', 'bsync2', 'bsync3',
>> +                            'fmirror0', 'fmirror1', 'fmirror2', 'fmirror3') 
>> as \
> 
> The indentation is off now.
> 
>> +                            (img_path, bsync1, bsync2, bsync3,
>> +                             fmirror0, fmirror1, fmirror2, fmirror3), \
>> +         iotests.VM() as vm:
> Hm.  On tmpfs, this test fails for me:
> 
> ($ TEST_DIR=/tmp/iotest ./check -qcow2 306)
> 
> @@ -170,7 +170,7 @@
>      "drive0": [
>        {
>          "busy": false,
> -        "count": 262144,
> +        "count": 458752,
>          "granularity": 65536,
>          "persistent": false,
>          "recording": true,
> @@ -464,7 +464,7 @@
>      "drive0": [
>        {
>          "busy": false,
> -        "count": 262144,
> +        "count": 458752,
>          "granularity": 65536,
>          "persistent": false,
>          "recording": true,
> @@ -760,7 +760,7 @@
>      "drive0": [
>        {
>          "busy": false,
> -        "count": 262144,
> +        "count": 393216,
>          "granularity": 65536,
>          "persistent": false,
>          "recording": true,
> @@ -1056,7 +1056,7 @@
>      "drive0": [
>        {
>          "busy": false,
> -        "count": 262144,
> +        "count": 458752,
>          "granularity": 65536,
>          "persistent": false,
>          "recording": true,
> @@ -1350,7 +1350,7 @@
>      "drive0": [
>        {
>          "busy": false,
> -        "count": 262144,
> +        "count": 458752,
>          "granularity": 65536,
>          "persistent": false,
>          "recording": true,
> @@ -2236,7 +2236,7 @@
>      "drive0": [
>        {
>          "busy": false,
> -        "count": 262144,
> +        "count": 458752,
>          "granularity": 65536,
>          "persistent": false,
>          "recording": true,
> 
> Can you see the same?

yes, but also only on tmpfs. ext4, xfs, ZFS all work fine.. the numbers 
for tmpfs vary between runs, each wrong count is sometimes 393216 (256k 
expected + 128k extra), sometimes 458752 (+192k). it's always the dirty 
bitmap used by the mirror job which is 'wrong', not the passed-in sync 
bitmap which verifies correctly. the final mirror results also seem 
correct.

for the first diff hunk (bitmap + never + simulated), we did

- reference mirror #0
- add sync bitmap 'bitmap0'
- do writes to dirty 6 sectors (393216)
- reference mirror #1
- test mirror #1
- bitmap0 still has count 393216
- reference mirror #2
- test mirror #2
-- while that is not yet completed, do 4 more writes
-- bitmap0 now has count 393216 + 262144 655360
-- dirty bitmap 'should have' count 262144, but has 458752 or 393216

this is not what actually interests us at this point: how far the mirror 
has progressed does not matter, we just want to see that the writes we 
did while it was ongoing have been reflected in the sync bitmap. so 
unless there is some hunch that this difference in mirroring 'speed' 
between the file systems is something that we need to take a look at, 
I'd say we just dump bitmap0 after the writes have been performed, 
instead of all bitmaps (in line 230f).