Re: [PATCH 2/2] btrfs-progs: Add test for collision DIR_ITEM handling

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

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



On 2018年03月26日 21:24, Nikolay Borisov wrote:
> 
> 
> On 25.03.2018 14:44, Qu Wenruo wrote:
>>
>>
>> On 2018年03月23日 22:48, Nikolay Borisov wrote:
>>> Verify that if we have an otherwise clean filesystem, containging collided 
>>> DIR_ITEM, btrfs check lowmem's mode can correctly handle those and not produce
>>> any false positives. 
>>>
>>> This if fixed by commit titled:
>>>
>>>  "btrfs-progs: Fix DIR_ITEM checking in lowmem" 
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>
>> Looks pretty good.
>>
>> However a nitpick inlined below.
>>
>>> ---
>>>  .../031-lowmem-collission-dir-items/test.sh        | 27 ++++++++++++++++++++++
>>>  1 file changed, 27 insertions(+)
>>>  create mode 100755 tests/fsck-tests/031-lowmem-collission-dir-items/test.sh
>>>
>>> diff --git a/tests/fsck-tests/031-lowmem-collission-dir-items/test.sh b/tests/fsck-tests/031-lowmem-collission-dir-items/test.sh
>>> new file mode 100755
>>> index 0000000..8a01889
>>> --- /dev/null
>>> +++ b/tests/fsck-tests/031-lowmem-collission-dir-items/test.sh
>>> @@ -0,0 +1,27 @@
>>> +#!/bin/bash
>>> +# Ensure that running btrfs check in lowmem mode on a fs
>>> +# which contains DIR_ITEM with collissions handles it
>>> +# properly
>>> +source "$TEST_TOP/common"
>>> +
>>> +check_prereq btrfs
>>> +check_prereq mkfs.btrfs
>>> +
>>> +setup_root_helper
>>> +prepare_test_dev
>>> +
>>> +run_check $SUDO_HELPER "$TOP/mkfs.btrfs" -f "$TEST_DEV"
>>> +run_check_mount_test_dev
>>> +
>>> +# Create 2 files whose names collide
>>> +
>>> +run_check $SUDO_HELPER touch "$TEST_MNT/5ab4e206~~~~~~~~XVT1U3ZF647YS2PD4AKAG826"
>>> +run_check $SUDO_HELPER touch "$TEST_MNT/5ab4e26a~~~~~~~~AP1C3VQBE79IJOTVOEZIR9YU"
>>> +
>>> +run_check_umount_test_dev
>>> +
>>> +# The fs is clean so lowmem shouldn't produce any warnings
>>> +run_check "$TOP/btrfs" check --mode=lowmem --readonly "$TEST_DEV"
>>
>> I understand this is a pinpoint test case for lowmem mode, but for
>> lowmem mode test, we set TEST_ENABLE_OVERRIDE and the whole test case
>> will use lowmem mode.
> 
> So how exactly is this argument supposed to be used. I tried:

TEST_ARGS_CHECK="--mode=lowmem" is forgot.

> 
> TEST_ENABLE_OVERRIDE=true TEST=031\* ./fsck-tests.sh  - no --lowmem is
> appended
> 
> I modify the TEST_ENABLE_OVERRIDE=false to TEST_ENABLE_OVERRIDE=true in
> common.local and then just run TEST=031\* ./fsck-tests.sh and it works.
> 
> TEST_ENABLE_OVERRIDE=true TEST_ARGS_CHECK=--mode=lowmem TEST=031\*
> ./fsck-tests.sh - this also works?

Yep

> 
> 
> I'm not entirely sure what the ci config is but at this point I'm
> beginning to worry that if I omit the explicit --mode option this test
> might never be run in lowmem mode.
> 
> The whole TEST_ENABLE_OVERRIDE situation seems a bit finicky.

I'm not familiar with CI things so it may be a good time point to check
and enhance it if possible?

Thanks,
Qu

> 
>>
>> So it doesn't seem necessary to explicitly call lowmem check here.
>> Just normal run_check "$TOP/btrfs" check would be enough.
>> (And this will also test original mode)
>>
>>> +if [ $? -ne 0 ]; then
>>> +	_fail "check --lowmem doesn't handle collissioned DIR_ITEMs correctly"
>>> +fi
>>>
>> IIRC run_check() will check the return value and exit if failure.
>> So this seems not necessary.
>>
>> Thanks,
>> Qu
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


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