[PATCH 1/2] btrfs-progs: Fix DIR_ITEM checking in lowmem

[PATCH 1/2] btrfs-progs: Fix DIR_ITEM checking in lowmem

[Nikolay Borisov]

When checking the validity of a DIR_ITEM item the index variable
is explicitly set to -1 so that the index check in find_inode_ref()
is ignored. This is necessary due to possible name collisions in DIR_ITEMS
(i.e multiple files with the same crc32c for their names, resulting in the
identical key->offset). Currently the code is broken due to index being set to
-1 at the beginning of check_dir_item and subsequently reset to the index found
in find_inode_ref. On subsequent iterations of the while loop in check_dir_items
we are going to erroneously perform index checking, since index is not -1 but 
whatever the index value of the last handled INODE_REF ITEM.
Without this patch check in lowmem mode produces the following false warnings 
positivess:

ERROR: root 5 INODE REF[1991456, 256] name 5ab4e28b~~~~~~~~QBXUT2GBJDRT5CB6ZWAJVDHK filetype 1 missing

But the items for this name are in fact correct: 

------------------SNIP------------------------
item 13 key (256 DIR_ITEM 4227063046) itemoff 2945 itemsize 140
	location key (220890 INODE_ITEM 0) type FILE
	transid 2278 data_len 0 name_len 40
	name: 5ab4e1bb~~~~~~~~65KNTAWVJ5VD4SV2R3BKFCXL
	location key (1991456 INODE_ITEM 0) type FILE
	transid 2285 data_len 0 name_len 40
	name: 5ab4e28b~~~~~~~~QBXUT2GBJDRT5CB6ZWAJVDHK

item 21 key (1991456 INODE_REF 256) itemoff 2104 itemsize 50
	index 1934146 namelen 40 name: 5ab4e28b~~~~~~~~QBXUT2GBJDRT5CB6ZWAJVDHK
-------------------SNIP-------------------------

Fix this by moving the code setting index at the beginning of the while
loop. This ensure that for each item in DIR_ITEM we have index set
correctly.

Fixes: 564901eac7a4 ("btrfs-progs: check: introduce print_dir_item_err()")
Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 check/mode-lowmem.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index 2982e92..8bd50d1 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -1217,14 +1217,6 @@ static int check_dir_item(struct btrfs_root *root, struct btrfs_key *di_key,
 	int tmp_err;
 	int need_research = 0;
 
-	/*
-	 * For DIR_ITEM set index to (u64)-1, so that find_inode_ref
-	 * ignore index check.
-	 */
-	if (di_key->type == BTRFS_DIR_INDEX_KEY)
-		index = di_key->offset;
-	else
-		index = (u64)-1;
 begin:
 	err = 0;
 	cur = 0;
@@ -1254,6 +1246,15 @@ begin:
 	memset(namebuf, 0, sizeof(namebuf) / sizeof(*namebuf));
 
 	while (cur < total) {
+		/*
+		 * For DIR_ITEM set index to (u64)-1, so that find_inode_ref
+		 * ignore index check.
+		 */
+		if (di_key->type == BTRFS_DIR_INDEX_KEY)
+			index = di_key->offset;
+		else
+			index = (u64)-1;
+
 		data_len = btrfs_dir_data_len(node, di);
 		tmp_err = 0;
 		if (data_len)
-- 
2.7.4

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

[Nikolay Borisov]

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>
---
 .../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"
+if [ $? -ne 0 ]; then
+	_fail "check --lowmem doesn't handle collissioned DIR_ITEMs correctly"
+fi
-- 
2.7.4

Re: [PATCH 1/2] btrfs-progs: Fix DIR_ITEM checking in lowmem

[Qu Wenruo]

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



On 2018年03月23日 22:48, Nikolay Borisov wrote:
> When checking the validity of a DIR_ITEM item the index variable
> is explicitly set to -1 so that the index check in find_inode_ref()
> is ignored. This is necessary due to possible name collisions in DIR_ITEMS
> (i.e multiple files with the same crc32c for their names, resulting in the
> identical key->offset). Currently the code is broken due to index being set to
> -1 at the beginning of check_dir_item and subsequently reset to the index found
> in find_inode_ref. On subsequent iterations of the while loop in check_dir_items
> we are going to erroneously perform index checking, since index is not -1 but 
> whatever the index value of the last handled INODE_REF ITEM.
> Without this patch check in lowmem mode produces the following false warnings 
> positivess:
> 
> ERROR: root 5 INODE REF[1991456, 256] name 5ab4e28b~~~~~~~~QBXUT2GBJDRT5CB6ZWAJVDHK filetype 1 missing
> 
> But the items for this name are in fact correct: 
> 
> ------------------SNIP------------------------
> item 13 key (256 DIR_ITEM 4227063046) itemoff 2945 itemsize 140
> 	location key (220890 INODE_ITEM 0) type FILE
> 	transid 2278 data_len 0 name_len 40
> 	name: 5ab4e1bb~~~~~~~~65KNTAWVJ5VD4SV2R3BKFCXL
> 	location key (1991456 INODE_ITEM 0) type FILE
> 	transid 2285 data_len 0 name_len 40
> 	name: 5ab4e28b~~~~~~~~QBXUT2GBJDRT5CB6ZWAJVDHK
> 
> item 21 key (1991456 INODE_REF 256) itemoff 2104 itemsize 50
> 	index 1934146 namelen 40 name: 5ab4e28b~~~~~~~~QBXUT2GBJDRT5CB6ZWAJVDHK
> -------------------SNIP-------------------------
> 
> Fix this by moving the code setting index at the beginning of the while
> loop. This ensure that for each item in DIR_ITEM we have index set
> correctly.
> 
> Fixes: 564901eac7a4 ("btrfs-progs: check: introduce print_dir_item_err()")
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  check/mode-lowmem.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
> index 2982e92..8bd50d1 100644
> --- a/check/mode-lowmem.c
> +++ b/check/mode-lowmem.c
> @@ -1217,14 +1217,6 @@ static int check_dir_item(struct btrfs_root *root, struct btrfs_key *di_key,
>  	int tmp_err;
>  	int need_research = 0;
>  
> -	/*
> -	 * For DIR_ITEM set index to (u64)-1, so that find_inode_ref
> -	 * ignore index check.
> -	 */
> -	if (di_key->type == BTRFS_DIR_INDEX_KEY)
> -		index = di_key->offset;
> -	else
> -		index = (u64)-1;
>  begin:
>  	err = 0;
>  	cur = 0;
> @@ -1254,6 +1246,15 @@ begin:
>  	memset(namebuf, 0, sizeof(namebuf) / sizeof(*namebuf));
>  
>  	while (cur < total) {
> +		/*
> +		 * For DIR_ITEM set index to (u64)-1, so that find_inode_ref
> +		 * ignore index check.
> +		 */
> +		if (di_key->type == BTRFS_DIR_INDEX_KEY)
> +			index = di_key->offset;
> +		else
> +			index = (u64)-1;
> +
>  		data_len = btrfs_dir_data_len(node, di);
>  		tmp_err = 0;
>  		if (data_len)
> 


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

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

[Qu Wenruo]

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



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


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

Re: [PATCH 1/2] btrfs-progs: Fix DIR_ITEM checking in lowmem

[Su Yue]

On 03/23/2018 10:48 PM, Nikolay Borisov wrote:
> When checking the validity of a DIR_ITEM item the index variable
> is explicitly set to -1 so that the index check in find_inode_ref()
> is ignored. This is necessary due to possible name collisions in DIR_ITEMS
> (i.e multiple files with the same crc32c for their names, resulting in the
> identical key->offset). Currently the code is broken due to index being set to
> -1 at the beginning of check_dir_item and subsequently reset to the index found
> in find_inode_ref. On subsequent iterations of the while loop in check_dir_items
> we are going to erroneously perform index checking, since index is not -1 but
> whatever the index value of the last handled INODE_REF ITEM.
> Without this patch check in lowmem mode produces the following false warnings
> positivess:
> 
> ERROR: root 5 INODE REF[1991456, 256] name 5ab4e28b~~~~~~~~QBXUT2GBJDRT5CB6ZWAJVDHK filetype 1 missing
> 
> But the items for this name are in fact correct:
> 
> ------------------SNIP------------------------
> item 13 key (256 DIR_ITEM 4227063046) itemoff 2945 itemsize 140
> 	location key (220890 INODE_ITEM 0) type FILE
> 	transid 2278 data_len 0 name_len 40
> 	name: 5ab4e1bb~~~~~~~~65KNTAWVJ5VD4SV2R3BKFCXL
> 	location key (1991456 INODE_ITEM 0) type FILE
> 	transid 2285 data_len 0 name_len 40
> 	name: 5ab4e28b~~~~~~~~QBXUT2GBJDRT5CB6ZWAJVDHK
> 
> item 21 key (1991456 INODE_REF 256) itemoff 2104 itemsize 50
> 	index 1934146 namelen 40 name: 5ab4e28b~~~~~~~~QBXUT2GBJDRT5CB6ZWAJVDHK
> -------------------SNIP-------------------------
> 
> Fix this by moving the code setting index at the beginning of the while
> loop. This ensure that for each item in DIR_ITEM we have index set
> correctly.
> 
> Fixes: 564901eac7a4 ("btrfs-progs: check: introduce print_dir_item_err()")
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Looks good to me.
Reviewed-by: Su Yue <suy.fnst@cn.fujitsu.com>

> ---
>   check/mode-lowmem.c | 17 +++++++++--------
>   1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
> index 2982e92..8bd50d1 100644
> --- a/check/mode-lowmem.c
> +++ b/check/mode-lowmem.c
> @@ -1217,14 +1217,6 @@ static int check_dir_item(struct btrfs_root *root, struct btrfs_key *di_key,
>   	int tmp_err;
>   	int need_research = 0;
>   
> -	/*
> -	 * For DIR_ITEM set index to (u64)-1, so that find_inode_ref
> -	 * ignore index check.
> -	 */
> -	if (di_key->type == BTRFS_DIR_INDEX_KEY)
> -		index = di_key->offset;
> -	else
> -		index = (u64)-1;
>   begin:
>   	err = 0;
>   	cur = 0;
> @@ -1254,6 +1246,15 @@ begin:
>   	memset(namebuf, 0, sizeof(namebuf) / sizeof(*namebuf));
>   
>   	while (cur < total) {
> +		/*
> +		 * For DIR_ITEM set index to (u64)-1, so that find_inode_ref
> +		 * ignore index check.
> +		 */
> +		if (di_key->type == BTRFS_DIR_INDEX_KEY)
> +			index = di_key->offset;
> +		else
> +			index = (u64)-1;
> +
>   		data_len = btrfs_dir_data_len(node, di);
>   		tmp_err = 0;
>   		if (data_len)
> 

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

[Nikolay Borisov]

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


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.

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

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

[Qu Wenruo]

[-- 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 --]

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

[David Sterba]

On Mon, Mar 26, 2018 at 04:24:47PM +0300, Nikolay Borisov wrote:
> >> +# 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_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?

This is how it's supposed to work via the commandline:

- TEST_ENABLE_OVERRIDE is a global switch for any user-defined overrides
- TEST_ARGS_CHECK specifies what should be appended to 'check'.
> 
> 
> 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.

CI config == .travis.yml, ie. configuration for the continuous
integration, that among others also runs check tests in default and
lowmem mode.

We don't want mode-specific tests but rather single testcase that can be
selectively used with original or lowmem mode. So even if you have a
regression test for lowmem, it also must pass in the original mode and
therefore there are no switches inside the test script.

[PATCH v2] btrfs-progs: Add test for collision DIR_ITEM handling

[Nikolay Borisov]

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>
---
 .../031-lowmem-collission-dir-items/test.sh        | 23 ++++++++++++++++++++++
 1 file changed, 23 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 000000000000..fa0b88af0557
--- /dev/null
+++ b/tests/fsck-tests/031-lowmem-collission-dir-items/test.sh
@@ -0,0 +1,23 @@
+#!/bin/bash
+# Ensure that running btrfs check on a fs which has name collisions of files
+# doesn't result in false positives. This test is specifically targeted at
+# lowmem mode.
+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 --readonly "$TEST_DEV"
-- 
2.7.4

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

[Qu Wenruo]

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



On 2018年03月26日 21:59, 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>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  .../031-lowmem-collission-dir-items/test.sh        | 23 ++++++++++++++++++++++
>  1 file changed, 23 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 000000000000..fa0b88af0557
> --- /dev/null
> +++ b/tests/fsck-tests/031-lowmem-collission-dir-items/test.sh
> @@ -0,0 +1,23 @@
> +#!/bin/bash
> +# Ensure that running btrfs check on a fs which has name collisions of files
> +# doesn't result in false positives. This test is specifically targeted at
> +# lowmem mode.
> +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 --readonly "$TEST_DEV"
> 


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

Re: [PATCH 1/2] btrfs-progs: Fix DIR_ITEM checking in lowmem

[Nikolay Borisov]

On 23.03.2018 16:48, Nikolay Borisov wrote:
> When checking the validity of a DIR_ITEM item the index variable
> is explicitly set to -1 so that the index check in find_inode_ref()
> is ignored. This is necessary due to possible name collisions in DIR_ITEMS
> (i.e multiple files with the same crc32c for their names, resulting in the
> identical key->offset). Currently the code is broken due to index being set to
> -1 at the beginning of check_dir_item and subsequently reset to the index found
> in find_inode_ref. On subsequent iterations of the while loop in check_dir_items
> we are going to erroneously perform index checking, since index is not -1 but 
> whatever the index value of the last handled INODE_REF ITEM.
> Without this patch check in lowmem mode produces the following false warnings 
> positivess:
> 
> ERROR: root 5 INODE REF[1991456, 256] name 5ab4e28b~~~~~~~~QBXUT2GBJDRT5CB6ZWAJVDHK filetype 1 missing
> 
> But the items for this name are in fact correct: 
> 
> ------------------SNIP------------------------
> item 13 key (256 DIR_ITEM 4227063046) itemoff 2945 itemsize 140
> 	location key (220890 INODE_ITEM 0) type FILE
> 	transid 2278 data_len 0 name_len 40
> 	name: 5ab4e1bb~~~~~~~~65KNTAWVJ5VD4SV2R3BKFCXL
> 	location key (1991456 INODE_ITEM 0) type FILE
> 	transid 2285 data_len 0 name_len 40
> 	name: 5ab4e28b~~~~~~~~QBXUT2GBJDRT5CB6ZWAJVDHK
> 
> item 21 key (1991456 INODE_REF 256) itemoff 2104 itemsize 50
> 	index 1934146 namelen 40 name: 5ab4e28b~~~~~~~~QBXUT2GBJDRT5CB6ZWAJVDHK
> -------------------SNIP-------------------------
> 
> Fix this by moving the code setting index at the beginning of the while
> loop. This ensure that for each item in DIR_ITEM we have index set
> correctly.
> 
> Fixes: 564901eac7a4 ("btrfs-progs: check: introduce print_dir_item_err()")
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---

Ping
>  check/mode-lowmem.c | 17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
> index 2982e92..8bd50d1 100644
> --- a/check/mode-lowmem.c
> +++ b/check/mode-lowmem.c
> @@ -1217,14 +1217,6 @@ static int check_dir_item(struct btrfs_root *root, struct btrfs_key *di_key,
>  	int tmp_err;
>  	int need_research = 0;
>  
> -	/*
> -	 * For DIR_ITEM set index to (u64)-1, so that find_inode_ref
> -	 * ignore index check.
> -	 */
> -	if (di_key->type == BTRFS_DIR_INDEX_KEY)
> -		index = di_key->offset;
> -	else
> -		index = (u64)-1;
>  begin:
>  	err = 0;
>  	cur = 0;
> @@ -1254,6 +1246,15 @@ begin:
>  	memset(namebuf, 0, sizeof(namebuf) / sizeof(*namebuf));
>  
>  	while (cur < total) {
> +		/*
> +		 * For DIR_ITEM set index to (u64)-1, so that find_inode_ref
> +		 * ignore index check.
> +		 */
> +		if (di_key->type == BTRFS_DIR_INDEX_KEY)
> +			index = di_key->offset;
> +		else
> +			index = (u64)-1;
> +
>  		data_len = btrfs_dir_data_len(node, di);
>  		tmp_err = 0;
>  		if (data_len)
> 

Re: [PATCH 1/2] btrfs-progs: Fix DIR_ITEM checking in lowmem

[David Sterba]

On Wed, May 02, 2018 at 08:50:38AM +0300, Nikolay Borisov wrote:
> > correctly.
> > 
> > Fixes: 564901eac7a4 ("btrfs-progs: check: introduce print_dir_item_err()")
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > ---
> 
> Ping

Applied, thanks.