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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6879 bytes --]

On 02.10.20 10:23, Fabian Grünbichler wrote:
> 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.

I see.  The active bitmap the backup job adds isn’t cleared when
progress is made (it’s only used to capture dirtying writes while the
job runs, so the initial bitmap stays constant), in contrast to the one
that mirror adds.  Backup doesn’t clear any bitmap, because it doesn’t
need to – a single iteration is sufficient to catch everything that was
dirty at the beginning of the job.

So, yes, we should hide dirty_bitmap, as it can be in an arbitrary state
while the mirror job is running, and it doesn’t give us useful information.

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

Looks like there is never any other bitmap bit bitmap0 and the mirror’s
dirty_bitmap, so this sounds like a good idea.

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]