[PATCH v2 0/3]: generic/020: fix MAX_ATTRVAL_SIZE values

[PATCH v2 0/3]: generic/020: fix MAX_ATTRVAL_SIZE values

[David Disseldorp]

MAX_ATTRVAL_SIZE provides a per-filesystem maximum xattr value length
limit. There are a few problems with the current values:
- XFS, UDF and Btrfs are incorrectly hardcoded to use 64 bytes
  + This is a regression from the larger value used prior to fff4359d
  + Btrfs's should be calculated using nodesize and xattr name length
- NFS currently uses a 64K limit
  + This may be above the server's underlying failsystem limit, so use
    a conservative estimate instead

Cheers, David

 common/attr           | 175 ++++++++++++++++++++++++------------------
 tests/generic/020     |  10 ++-
 tests/generic/020.out |   2 +-
 3 files changed, 108 insertions(+), 79 deletions(-)

[PATCH v2 1/3] common/attr: add and use _attr_get_max()

[David Disseldorp]

No functional change. The MAX_ATTRS and MAX_ATTRVAL_SIZE exports are
only used by generic/020. In preparation for taking into account the
attr name length when calculating MAX_ATTRVAL_SIZE, split the current
logic out into a _attr_get_max() helper function.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 common/attr       | 163 +++++++++++++++++++++++++---------------------
 tests/generic/020 |  10 ++-
 2 files changed, 95 insertions(+), 78 deletions(-)

diff --git a/common/attr b/common/attr
index dae8a1bb..a80b10a1 100644
--- a/common/attr
+++ b/common/attr
@@ -264,80 +264,93 @@ _getfattr()
 	return ${PIPESTATUS[0]}
 }
 
-# set maximum total attr space based on fs type
-case "$FSTYP" in
-xfs|udf|pvfs2|9p|ceph|nfs)
-	MAX_ATTRS=1000
-	;;
-ext2|ext3|ext4)
-	# For 4k blocksizes, most of the attributes have an attr_name of
-	# "attribute_NN" which is 12, and "value_NN" which is 8.
-	# But for larger block sizes, we start having extended attributes of the
-	# form "attribute_NNN" or "attribute_NNNN", and "value_NNN" and
-	# "value_NNNN", which causes the round(len(..), 4) to jump up by 4
-	# bytes.  So round_up(len(attr_name, 4)) becomes 16 instead of 12, and
-	# round_up(len(value, 4)) becomes 12 instead of 8.
-	#
-	# For 64K blocksize the calculation becomes
-	# 	max_attrs = (block_size - 32) / (16 + 12 + 16)
-	# or
-	# 	max_attrs = (block_size - 32) / 44
-	#
-	# For 4K blocksize:-
-	# 	max_attrs = (block_size - 32) / (16 + 8 + 12)
-	# or
-	# 	max_attrs = (block_size - 32) / 36
-	#
-	# Note (for 4K bs) above are exact calculations for attrs of type
-	# attribute_NN with values of type value_NN.
-	# With above calculations, for 4k blocksize max_attrs becomes 112.
-	# This means we can have few attrs of type attribute_NNN with values of
-	# type value_NNN. To avoid/handle this we need to add extra 4 bytes of
-	# headroom.
-	#
-	# So for 4K, the calculations becomes:-
-	# 	max_attrs = (block_size - 32) / (16 + 8 + 12 + 4)
-	# or
-	# 	max_attrs = (block_size - 32) / 40
-	#
-	# Assume max ~1 block of attrs
-	BLOCK_SIZE=`_get_block_size $TEST_DIR`
-	if [ $BLOCK_SIZE -le 4096 ]; then
-		let MAX_ATTRS=$((($BLOCK_SIZE - 32) / (16 + 8 + 12 + 4)))
-	else
-		let MAX_ATTRS=$((($BLOCK_SIZE - 32) / (16 + 12 + 16 )))
-	fi
-	;;
-*)
-	# Assume max ~1 block of attrs
-	BLOCK_SIZE=`_get_block_size $TEST_DIR`
-	# user.attribute_XXX="value.XXX" is about 32 bytes; leave some overhead
-	let MAX_ATTRS=$BLOCK_SIZE/40
-esac
-
-export MAX_ATTRS
-
-# Set max attr value size based on fs type
-case "$FSTYP" in
-xfs|udf|btrfs)
-	MAX_ATTRVAL_SIZE=64
-	;;
-pvfs2)
-	MAX_ATTRVAL_SIZE=8192
-	;;
-9p|ceph|nfs)
-	MAX_ATTRVAL_SIZE=65536
-	;;
-bcachefs)
-	MAX_ATTRVAL_SIZE=1024
-	;;
-*)
-	# Assume max ~1 block of attrs
-	BLOCK_SIZE=`_get_block_size $TEST_DIR`
-	# leave a little overhead
-	let MAX_ATTRVAL_SIZE=$BLOCK_SIZE-256
-esac
-
-export MAX_ATTRVAL_SIZE
+# export fs-specific MAX_ATTRS and MAX_ATTRVAL_SIZE values. The parameter
+# @max_attrval_namelen is required for filesystems which take into account attr
+# name lengths (including namespace prefix) when determining limits.
+_attr_get_max()
+{
+	local max_attrval_namelen="$1"
+
+	# set maximum total attr space based on fs type
+	case "$FSTYP" in
+	xfs|udf|pvfs2|9p|ceph|nfs)
+		MAX_ATTRS=1000
+		;;
+	ext2|ext3|ext4)
+		# For 4k blocksizes, most of the attributes have an attr_name of
+		# "attribute_NN" which is 12, and "value_NN" which is 8.
+		# But for larger block sizes, we start having extended
+		# attributes of the
+		# form "attribute_NNN" or "attribute_NNNN", and "value_NNN" and
+		# "value_NNNN", which causes the round(len(..), 4) to jump up by
+		# 4 bytes.  So round_up(len(attr_name, 4)) becomes 16 instead of
+		# 12, and round_up(len(value, 4)) becomes 12 instead of 8.
+		#
+		# For 64K blocksize the calculation becomes
+		# 	max_attrs = (block_size - 32) / (16 + 12 + 16)
+		# or
+		# 	max_attrs = (block_size - 32) / 44
+		#
+		# For 4K blocksize:-
+		# 	max_attrs = (block_size - 32) / (16 + 8 + 12)
+		# or
+		# 	max_attrs = (block_size - 32) / 36
+		#
+		# Note (for 4K bs) above are exact calculations for attrs of
+		# type attribute_NN with values of type value_NN.
+		# With above calculations, for 4k blocksize max_attrs becomes
+		# 112.
+		# This means we can have few attrs of type attribute_NNN with
+		# values of
+		# type value_NNN. To avoid/handle this we need to add extra 4
+		# bytes of headroom.
+		#
+		# So for 4K, the calculations becomes:-
+		# 	max_attrs = (block_size - 32) / (16 + 8 + 12 + 4)
+		# or
+		# 	max_attrs = (block_size - 32) / 40
+		#
+		# Assume max ~1 block of attrs
+		BLOCK_SIZE=`_get_block_size $TEST_DIR`
+		if [ $BLOCK_SIZE -le 4096 ]; then
+			let MAX_ATTRS=$((($BLOCK_SIZE - 32) / (16 + 8 + 12 + 4)))
+		else
+			let MAX_ATTRS=$((($BLOCK_SIZE - 32) / (16 + 12 + 16 )))
+		fi
+		;;
+	*)
+		# Assume max ~1 block of attrs
+		BLOCK_SIZE=`_get_block_size $TEST_DIR`
+		# user.attribute_XXX="value.XXX" is about 32 bytes; leave some
+		# overhead
+		let MAX_ATTRS=$BLOCK_SIZE/40
+	esac
+
+	export MAX_ATTRS
+
+	# Set max attr value size based on fs type
+	case "$FSTYP" in
+	xfs|udf|btrfs)
+		MAX_ATTRVAL_SIZE=64
+		;;
+	pvfs2)
+		MAX_ATTRVAL_SIZE=8192
+		;;
+	9p|ceph|nfs)
+		MAX_ATTRVAL_SIZE=65536
+		;;
+	bcachefs)
+		MAX_ATTRVAL_SIZE=1024
+		;;
+	*)
+		# Assume max ~1 block of attrs
+		BLOCK_SIZE=`_get_block_size $TEST_DIR`
+		# leave a little overhead
+		let MAX_ATTRVAL_SIZE=$BLOCK_SIZE-256
+	esac
+
+	export MAX_ATTRVAL_SIZE
+}
+
 # make sure this script returns success
 /bin/true
diff --git a/tests/generic/020 b/tests/generic/020
index 29ef853c..34861401 100755
--- a/tests/generic/020
+++ b/tests/generic/020
@@ -88,6 +88,9 @@ _attr_list $testfile
 echo "*** add lots of attributes"
 v=0
 
+max_attrval_name="long_attr"	# add 5 for "user." prefix
+_attr_get_max "$(( 5 + ${#max_attrval_name} ))"
+
 while [ $v -lt $MAX_ATTRS ]
 do
     echo -n "value_$v" | attr -s "attribute_$v" $testfile >>$seqres.full
@@ -128,11 +131,12 @@ _attr_list $testfile
 
 echo "*** really long value"
 dd if=/dev/zero bs=1 count=$MAX_ATTRVAL_SIZE 2>/dev/null \
-    | _attr -s "long_attr" $testfile >/dev/null
+    | _attr -s "$max_attrval_name" $testfile >/dev/null
 
 OCTAL_SIZE=`echo "obase=8; $MAX_ATTRVAL_SIZE" | bc`
-_attr -q -g "long_attr" $testfile | od -t x1 | sed -e "s/^0*$OCTAL_SIZE$/ATTRSIZE/"    
-_attr -r "long_attr" $testfile >/dev/null
+_attr -q -g "$max_attrval_name" $testfile | od -t x1 \
+    | sed -e "s/^0*$OCTAL_SIZE$/ATTRSIZE/"
+_attr -r "$max_attrval_name" $testfile >/dev/null
 
 echo "*** set/get/remove really long names (expect failure)"
 short="XXXXXXXXXX"
-- 
2.34.1

Re: [PATCH v2 1/3] common/attr: add and use _attr_get_max()

[Dave Chinner]

On Tue, Apr 12, 2022 at 12:49:59PM +0200, David Disseldorp wrote:
> No functional change. The MAX_ATTRS and MAX_ATTRVAL_SIZE exports are
> only used by generic/020. In preparation for taking into account the
> attr name length when calculating MAX_ATTRVAL_SIZE, split the current
> logic out into a _attr_get_max() helper function.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  common/attr       | 163 +++++++++++++++++++++++++---------------------
>  tests/generic/020 |  10 ++-
>  2 files changed, 95 insertions(+), 78 deletions(-)

Hmmm. when you said you were going to make these helper functions,
I thought you meant you were going to move them to generic/020
as helper functions as that is the only place that uses them.

I don't see any point in making them single use helper functions
specific to a single test and then leaving them in common/attr....

Then you can get rid of the exported variable and use a test local
variable for MAX_ATTRS and MAX_ATTRVAL_SIZE....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH v2 2/3] common/attr: fix MAX_ATTRVAL_SIZE for XFS, UDF, Btrfs and NFS

[David Disseldorp]

As found by Dave Chinner, fff4359d ("020: make this xattr test generic")
unintentionally changed the long attribute value length from 100K to 64
*bytes* for XFS, UDF and Btrfs.
Update XFS and UDF to use 64K MAX_ATTRVAL_SIZE value. For Btrfs, use the
nodesize, xattr length and tree entry overhead sizes to calculate the
maximum.
NFS doesn't provide a way to find out the MAX_ATTRVAL_SIZE for the
underlying filesystem on the server, so just use a rough 1K limit.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 common/attr | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/common/attr b/common/attr
index a80b10a1..1e1c8f6e 100644
--- a/common/attr
+++ b/common/attr
@@ -328,20 +328,32 @@ _attr_get_max()
 
 	export MAX_ATTRS
 
-	# Set max attr value size based on fs type
+	# Set max attr value size in bytes based on fs type
 	case "$FSTYP" in
-	xfs|udf|btrfs)
-		MAX_ATTRVAL_SIZE=64
+	btrfs)
+		_require_btrfs_command inspect-internal dump-super
+		local ns=$($BTRFS_UTIL_PROG inspect-internal dump-super \
+				$TEST_DEV | sed -n 's/nodesize\s*\(.*\)/\1/p')
+		[ -n "$ns" ] || _fail "failed to obtain nodesize"
+		# max == nodesize - sizeof(struct btrfs_header)
+		#		- sizeof(struct btrfs_item)
+		#		- sizeof(struct btrfs_dir_item) - name_len
+		MAX_ATTRVAL_SIZE=$(( $ns - 156 - $max_attrval_namelen ))
 		;;
 	pvfs2)
 		MAX_ATTRVAL_SIZE=8192
 		;;
-	9p|ceph|nfs)
+	xfs|udf|9p|ceph)
 		MAX_ATTRVAL_SIZE=65536
 		;;
 	bcachefs)
 		MAX_ATTRVAL_SIZE=1024
 		;;
+	nfs)
+		# NFS doesn't provide a way to find out the MAX_ATTRVAL_SIZE for
+		# the underlying filesystem, so just use the lowest value above.
+		MAX_ATTRVAL_SIZE=1024
+		;;
 	*)
 		# Assume max ~1 block of attrs
 		BLOCK_SIZE=`_get_block_size $TEST_DIR`
-- 
2.34.1

[PATCH v2 3/3] generic/020: fix unaligned MAX_ATTRVAL_SIZE output filter

[David Disseldorp]

The current attr -g "$max_attrval_name" output filter is broken if
MAX_ATTRVAL_SIZE isn't 16-byte aligned, due to od's duplicate
suppression behaviour.
Fix it by having od only dump one byte per line.

Signed-off-by: David Disseldorp <ddiss@suse.de>
---
 tests/generic/020     | 2 +-
 tests/generic/020.out | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/generic/020 b/tests/generic/020
index 34861401..d1913953 100755
--- a/tests/generic/020
+++ b/tests/generic/020
@@ -134,7 +134,7 @@ dd if=/dev/zero bs=1 count=$MAX_ATTRVAL_SIZE 2>/dev/null \
     | _attr -s "$max_attrval_name" $testfile >/dev/null
 
 OCTAL_SIZE=`echo "obase=8; $MAX_ATTRVAL_SIZE" | bc`
-_attr -q -g "$max_attrval_name" $testfile | od -t x1 \
+_attr -q -g "$max_attrval_name" $testfile | od -w1 -t x1 \
     | sed -e "s/^0*$OCTAL_SIZE$/ATTRSIZE/"
 _attr -r "$max_attrval_name" $testfile >/dev/null
 
diff --git a/tests/generic/020.out b/tests/generic/020.out
index 7e3e65bd..0dc5e09f 100644
--- a/tests/generic/020.out
+++ b/tests/generic/020.out
@@ -47,7 +47,7 @@ user.snrub="fish2\012"
 user.snrub="fish2\012"
 
 *** really long value
-0000000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
+0000000 00
 *
 ATTRSIZE
 *** set/get/remove really long names (expect failure)
-- 
2.34.1

Re: [PATCH v2 3/3] generic/020: fix unaligned MAX_ATTRVAL_SIZE output filter

[Dave Chinner]

On Tue, Apr 12, 2022 at 12:50:01PM +0200, David Disseldorp wrote:
> The current attr -g "$max_attrval_name" output filter is broken if
> MAX_ATTRVAL_SIZE isn't 16-byte aligned, due to od's duplicate
> suppression behaviour.
> Fix it by having od only dump one byte per line.
> 
> Signed-off-by: David Disseldorp <ddiss@suse.de>
> ---
>  tests/generic/020     | 2 +-
>  tests/generic/020.out | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)

looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com