Re: [PATCH v6 11/15] iotests: add 298 to test new preallocate filter driver

[Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>]

25.09.2020 18:11, Vladimir Sementsov-Ogievskiy wrote:
> 25.09.2020 12:11, Max Reitz wrote:
>> On 25.09.20 10:49, Vladimir Sementsov-Ogievskiy wrote:
>>> 25.09.2020 11:26, Max Reitz wrote:
>>>> On 18.09.20 20:19, Vladimir Sementsov-Ogievskiy wrote:
>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>> ---
>>>>>    tests/qemu-iotests/298     | 186 +++++++++++++++++++++++++++++++++++++
>>>>>    tests/qemu-iotests/298.out |   5 +
>>>>>    tests/qemu-iotests/group   |   1 +
>>>>>    3 files changed, 192 insertions(+)
>>>>>    create mode 100644 tests/qemu-iotests/298
>>>>>    create mode 100644 tests/qemu-iotests/298.out
>>
>> [...]
>>
>>>>> +class TestTruncate(iotests.QMPTestCase):
>>>>
>>>> The same decorator could be placed here, although this class doesn’t
>>>> start a VM, and so is unaffected by the allowlist.  Still may be
>>>> relevant in case of block modules, I don’t know.
>>>
>>> Or just global test skip at file top
>>
>> Hm.  Like verify_quorum()?  Is there a generic function for that already?
>>
>> [...]
>>
>>>>> +        # Probably we'll want preallocate filter to keep align to
>>>>> cluster when
>>>>> +        # shrink preallocation, so, ignore small differece
>>>>> +        self.assertLess(abs(stat.st_size - refstat.st_size), 64 * 1024)
>>>>> +
>>>>> +        # Preallocate filter may leak some internal clusters (for
>>>>> example, if
>>>>> +        # guest write far over EOF, skipping some clusters - they
>>>>> will remain
>>>>> +        # fallocated, preallocate filter don't care about such
>>>>> leaks, it drops
>>>>> +        # only trailing preallocation.
>>>>
>>>> True, but that isn’t what’s happening here.  (We only write 10M at 0, so
>>>> there are no gaps.)  Why do we need this 1M margin?
>>>
>>> We write 10M, but qcow2 also writes metadata as it wants
>>
>> Ah, yes, sure.  Shouldn’t result in 1M, but why not.
>>
>>>>> +        self.assertLess(abs(stat.st_blocks - refstat.st_blocks) * 512,
>>>>> +                        1024 * 1024)
>>>>
>>>> [...]
>>>>
>>>>> diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
>>>>> index ff59cfd2d4..15d5f9619b 100644
>>>>> --- a/tests/qemu-iotests/group
>>>>> +++ b/tests/qemu-iotests/group
>>>>> @@ -307,6 +307,7 @@
>>>>>    295 rw
>>>>>    296 rw
>>>>>    297 meta
>>>>> +298 auto quick
>>>>
>>>> I wouldn’t mark it as quick, there is at least one preallocate=full of
>>>> 140M, and one of 40M, plus multiple 10M data writes and falloc
>>>> preallocations.
>>>>
>>>> Also, since you mark it as “auto”, have you run this test on all
>>>> CI-relevant hosts?  (Among other things I can’t predict) I wonder how
>>>> preallocation behaves on macOS.  Just because that one was always a bit
>>>> weird about not-really-data areas.
>>>>
>>>
>>> Ofcourse, I didn't run on all hosts. I'm a bit out of sync about this..

Sorry, I see now that it sounds rude.

>>
>> Well, someone has to do it.  The background story is that tests are
>> added to auto all the time (because “why not”), and then they fail on
>> BSD or macOS.  We have BSD docker test build targets at least, so they
>> can be easily tested.  (Well, it takes like half an hour, but you know.)
>>
>> (We don’t have macOS builds, as far as I can tell, but I personally
>> don’t even know why we run the iotests on macOS at all.  (Well, I also
>> wonder about the BSDs, but given the test build targets, I shouldn’t
>> complain, I suppose.))
>>
>> (Though macOS isn’t part of the gating CI, is it?  I seem to remember
>> macOS errors are generally only reported to me half a week after the
>> pull request is merged, which is even worse.)
>>
>> Anyway.  I just ran the test for OpenBSD
>> (EXTRA_CONFIGURE_OPTS='--target-list=x86_64-softmmu' \
>>     make vm-build-openbsd)
> 
> Oh, I didn't know that it's so simple. What another things you are running before sending a PULL?
> 
>> and got some failures:
>>
>> --- /home/qemu/qemu-test.PGo2ls/src/tests/qemu-iotests/298.out  Fri Sep
>> 25 07:10:31 2020
>> +++ /home/qemu/qemu-test.PGo2ls/build/tests/qemu-iotests/298.out.bad
>> Fri Sep 25 08:57:56 2020
>> @@ -1,5 +1,67 @@
>> -.............
>> +qemu-io: Failed to resize underlying file: Unsupported preallocation
>> mode: falloc

[..]

>> -OK
>> +FAILED (failures=6)
>>
>>> If I don't put new test in "auto", is there any chance that test would
>>> be automatically run somewhere?
>>
>> I run all tests before pull requests at least.
>>

OK, so it doesn't work on BSD at all. I don't really want to investigate, but seems it's because of absence of fallocate. So let's drop "auto" group.

Another thing: maybe, add auto-linux test group for running only in linux-build? So, new tests will be added to it because why not, and we will not bother with BSD and MacOS?

-- 
Best regards,
Vladimir