Skip to contentSkip to conversation

attr: adbjust acl_max of f2fs

PATCH In review Chao Yu 4 participants 6 messages Apr 28, 2017 – May 4, 2017

lore
Share & tools
lore

Chao Yu

author
#0 lore raw

PATCH 1/1 attr: adbjust acl_max of f2fs

From: Chao Yu <[email protected]>

f2fs has set inline_xattr as a default option, and introduced a new option named 'noinline_xattr' for disabling default inline_xattr option. So in _acl_get_max we need to check 'noinline_xattr' string in fs option, otherwise we may select the wrong max acl number since we always found the string 'ininline_xattr' in fs option.

Additionally, f2fs has changed disk layout of xattr block a bit, so will contain one more entry in both inline and noinline xattr inode, this patch will modify the max acl number to adjust it.

Signed-off-by Chao Yu <[email protected]>
---
 common/attr | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
common/attr +3 -3
index ac139e61..6d4f68ed 100644
@@ -43,11 +43,11 @@ _acl_get_max()
 		echo 8191
 		;;
 	f2fs)
-		_fs_options $TEST_DEV | grep "inline_xattr" >/dev/null 2>&1
+		_fs_options $TEST_DEV | grep "noinline_xattr" >/dev/null 2>&1
 		if [ $? -eq 0 ]; then
-			echo 531
+			echo 507
 		else
-			echo 506
+			echo 532
 		fi
 		;;
 	*)
Signature · 1 line
2.12.2.575.gb14f27f
#1 lore raw
Quoted 8 lines · Chao Yu

From: Chao Yu <[email protected]>

f2fs has set inline_xattr as a default option, and introduced a new option named 'noinline_xattr' for disabling default inline_xattr option. So in _acl_get_max we need to check 'noinline_xattr' string in fs option, otherwise we may select the wrong max acl number since we always found the string 'ininline_xattr' in fs option.

Quoted 5 lines

Additionally, f2fs has changed disk layout of xattr block a bit, so will contain one more entry in both inline and noinline xattr inode, this patch will modify the max acl number to adjust it.

Signed-off-by Chao Yu <[email protected]>

Thanks for the patch! It looks fine to me from fstests POV, but need review from f2fs developers too.

Thanks,
Eryu

Quoted 26 lines
---
 common/attr | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
common/attr +3 -3
index ac139e61..6d4f68ed 100644
@@ -43,11 +43,11 @@ _acl_get_max()
 		echo 8191
 		;;
 	f2fs)
-		_fs_options $TEST_DEV | grep "inline_xattr" >/dev/null 2>&1
+		_fs_options $TEST_DEV | grep "noinline_xattr" >/dev/null 2>&1
 		if [ $? -eq 0 ]; then
-			echo 531
+			echo 507
 		else
-			echo 506
+			echo 532
 		fi
 		;;
 	*)
Signature · 1 line
2.12.2.575.gb14f27f
#2 lore raw
Quoted 9 lines · Eryu Guan
Quoted 8 lines · Chao Yu

From: Chao Yu <[email protected]>

f2fs has set inline_xattr as a default option, and introduced a new option named 'noinline_xattr' for disabling default inline_xattr option. So in _acl_get_max we need to check 'noinline_xattr' string in fs option, otherwise we may select the wrong max acl number since we always found the string 'ininline_xattr' in fs option.

Quoted 8 lines
Quoted 5 lines

Additionally, f2fs has changed disk layout of xattr block a bit, so will contain one more entry in both inline and noinline xattr inode, this patch will modify the max acl number to adjust it.

Signed-off-by Chao Yu <[email protected]>

Thanks for the patch! It looks fine to me from fstests POV, but need review from f2fs developers too.

Eryu, thanks for the review. ;)

To Jaegeuk, could you help to review this patch?

Thanks,

Quoted 24 lines

Thanks,
Eryu

Quoted 20 lines
---
 common/attr | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
common/attr +3 -3
index ac139e61..6d4f68ed 100644
@@ -43,11 +43,11 @@ _acl_get_max()
 		echo 8191
 		;;
 	f2fs)
-		_fs_options $TEST_DEV | grep "inline_xattr" >/dev/null 2>&1
+		_fs_options $TEST_DEV | grep "noinline_xattr" >/dev/null 2>&1
 		if [ $? -eq 0 ]; then
-			echo 531
+			echo 507
 		else
-			echo 506
+			echo 532
Quoted 9 lines
Quoted 6 lines
common/attr +0 -0
 		fi
 		;;
 	*)
Signature · 1 line
2.12.2.575.gb14f27f

.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot

#3 lore raw
In reply to Chao Yu

Hello,

Quoted 22 lines · Chao Yu

From: Chao Yu <[email protected]>

f2fs has set inline_xattr as a default option, and introduced a new option named 'noinline_xattr' for disabling default inline_xattr option. So in _acl_get_max we need to check 'noinline_xattr' string in fs option, otherwise we may select the wrong max acl number since we always found the string 'ininline_xattr' in fs option.

Additionally, f2fs has changed disk layout of xattr block a bit, so will contain one more entry in both inline and noinline xattr inode, this patch will modify the max acl number to adjust it.

Signed-off-by Chao Yu <[email protected]>
---
 common/attr | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
common/attr +0 -0
index ac139e61..6d4f68ed 100644

common/attr _acl_get_max() · line 46

Quoted 5 lines
common/attr +1 -1
@@ -43,11 +43,11 @@ _acl_get_max()
 		echo 8191
 		;;
 	f2fs)
-		_fs_options $TEST_DEV | grep "inline_xattr" >/dev/null 2>&1
+		_fs_options $TEST_DEV | grep "noinline_xattr" >/dev/null 2>&1

This breaks old kernel support which has no "noinline_xattr". It'd be good to check "inline_xattr" again after checking "noinline_xattr". And, in terms of different number of entries, can we get the number from local.config by adding an export symbol likewise FSTYP?

Thanks,

Quoted 11 lines
common/attr +2 -2
 		if [ $? -eq 0 ]; then
-			echo 531
+			echo 507
 		else
-			echo 506
+			echo 532
 		fi
 		;;
 	*)
Signature · 1 line
2.12.2.575.gb14f27f
author
#4 lore raw

Hi Jaegeuk,

Quoted 20 lines · Jaegeuk Kim

Hello,

Quoted 17 lines · Chao Yu

From: Chao Yu <[email protected]>

f2fs has set inline_xattr as a default option, and introduced a new option named 'noinline_xattr' for disabling default inline_xattr option. So in _acl_get_max we need to check 'noinline_xattr' string in fs option, otherwise we may select the wrong max acl number since we always found the string 'ininline_xattr' in fs option.

Additionally, f2fs has changed disk layout of xattr block a bit, so will contain one more entry in both inline and noinline xattr inode, this patch will modify the max acl number to adjust it.

Signed-off-by Chao Yu <[email protected]>
---
 common/attr | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Quoted 13 lines

common/attr _acl_get_max() · line 46

Quoted 10 lines
common/attr +1 -1
index ac139e61..6d4f68ed 100644
@@ -43,11 +43,11 @@ _acl_get_max()
 		echo 8191
 		;;
 	f2fs)
-		_fs_options $TEST_DEV | grep "inline_xattr" >/dev/null 2>&1
+		_fs_options $TEST_DEV | grep "noinline_xattr" >/dev/null 2>&1

This breaks old kernel support which has no "noinline_xattr". It'd be good to check "inline_xattr" again after checking "noinline_xattr". And, in terms of

Hmm, we have changed xattr layout a bit in new kernel, so even we have knew that user set inline_xattr mount option, still we don't know whether the last kernel was been used, so we can not decide to choose 532 or 531 as acl_max value. Maybe we need to check result of 'uname -r' either.

Quoted 2 lines

different number of entries, can we get the number from local.config by adding an export symbol likewise FSTYP?

You mean adding one more configuration parameter which can be defined by user, then _acl_get_max can return correct acl_max value depend on that parameter?

Thanks,

Quoted 14 lines

Thanks,

Quoted 11 lines
common/attr +2 -2
 		if [ $? -eq 0 ]; then
-			echo 531
+			echo 507
 		else
-			echo 506
+			echo 532
 		fi
 		;;
 	*)
Signature · 1 line
2.12.2.575.gb14f27f
#5 lore raw
Quoted 43 lines · Chao Yu

Hi Jaegeuk,

Quoted 20 lines · Jaegeuk Kim

Hello,

Quoted 17 lines · Chao Yu

From: Chao Yu <[email protected]>

f2fs has set inline_xattr as a default option, and introduced a new option named 'noinline_xattr' for disabling default inline_xattr option. So in _acl_get_max we need to check 'noinline_xattr' string in fs option, otherwise we may select the wrong max acl number since we always found the string 'ininline_xattr' in fs option.

Additionally, f2fs has changed disk layout of xattr block a bit, so will contain one more entry in both inline and noinline xattr inode, this patch will modify the max acl number to adjust it.

Signed-off-by Chao Yu <[email protected]>
---
 common/attr | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Quoted 13 lines

common/attr _acl_get_max() · line 46

Quoted 10 lines
common/attr +1 -1
index ac139e61..6d4f68ed 100644
@@ -43,11 +43,11 @@ _acl_get_max()
 		echo 8191
 		;;
 	f2fs)
-		_fs_options $TEST_DEV | grep "inline_xattr" >/dev/null 2>&1
+		_fs_options $TEST_DEV | grep "noinline_xattr" >/dev/null 2>&1

This breaks old kernel support which has no "noinline_xattr". It'd be good to check "inline_xattr" again after checking "noinline_xattr". And, in terms of

Hmm, we have changed xattr layout a bit in new kernel, so even we have knew that user set inline_xattr mount option, still we don't know whether the last kernel was been used, so we can not decide to choose 532 or 531 as acl_max value. Maybe we need to check result of 'uname -r' either.

Quoted 6 lines
Quoted 2 lines

different number of entries, can we get the number from local.config by adding an export symbol likewise FSTYP?

You mean adding one more configuration parameter which can be defined by user, then _acl_get_max can return correct acl_max value depend on that parameter?

How about this?

	if noinline_xattr exists
		acl_max=506
	else if inline_xattr exists
		acl_max=531
	else
		/* for old kernel */
		acl_max=506
	fi

	if $ACL_MAX_ADJ is defined; then
		acl_max += $ACL_MAX_ADJ;
	fi

Thanks,

Quoted 17 lines

Thanks,

Quoted 14 lines

Thanks,

Quoted 11 lines
common/attr +2 -2
 		if [ $? -eq 0 ]; then
-			echo 531
+			echo 507
 		else
-			echo 506
+			echo 532
 		fi
 		;;
 	*)
Signature · 1 line
2.12.2.575.gb14f27f