[PATCH v3 0/2] Two xattrs-related fixes for ceph

[PATCH v3 0/2] Two xattrs-related fixes for ceph

[Luís Henriques]

Hi!

A bug fix in ceph has made some changes in the way xattr limits are
enforced on the client side.  This requires some fixes on tests
generic/020 and generic/486.

* Changes since v2:

  - patch 0001:
    Add logic to compute the *real* maximum.  Kudos to David Disseldorp
    for providing the right incantation to do the maths.  Also split
    _attr_get_max() in two, so that they can be invoked from different
    places in the test script.

  - patch 002:
    attr_replace_test now has an extra param as suggested by Dave Chinner,
    and added fs-specific logic to the script.  No need to have an exact
    maximum in this test, a big-enough value suffices.

* Changes since v1:

  - patch 0001:
    Set the max size for xattrs values to a 65000, so that it is close to
    the maximum, but still able to accommodate any pre-existing xattr

  - patch 0002:
    Same thing as patch 0001, but in a more precise way: actually take
    into account the exact sizes for name+value of a pre-existing xattr.

Luís Henriques (2):
  generic/020: adjust max_attrval_size for ceph
  generic/486: adjust the max xattr size

 src/attr_replace_test.c | 30 ++++++++++++++++++++++++++----
 tests/generic/020       | 33 +++++++++++++++++++++++++--------
 tests/generic/486       | 11 ++++++++++-
 3 files changed, 61 insertions(+), 13 deletions(-)

[PATCH v3 1/2] generic/020: adjust max_attrval_size for ceph

[Luís Henriques]

CephFS doesn't have a maximum xattr size.  Instead, it imposes a maximum
size for the full set of xattrs names+values, which by default is 64K.

This patch fixes the max_attrval_size for ceph so that it is takes into
account any already existing attrs in the file.

Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
 tests/generic/020 | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/tests/generic/020 b/tests/generic/020
index d8648e96286e..b91bca34dcd4 100755
--- a/tests/generic/020
+++ b/tests/generic/020
@@ -51,13 +51,9 @@ _attr_list()
     fi
 }
 
-# set 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.
+# set fs-specific max_attrs
 _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)
@@ -112,6 +108,16 @@ _attr_get_max()
 		# overhead
 		let max_attrs=$BLOCK_SIZE/40
 	esac
+}
+
+# set fs-specific 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; parameter @filename is required for
+# filesystems that need to take into account already existing attrs.
+_attr_get_maxval_size()
+{
+	local max_attrval_namelen="$1"
+	local filename="$2"
 
 	# Set max attr value size in bytes based on fs type
 	case "$FSTYP" in
@@ -128,7 +134,7 @@ _attr_get_max()
 	pvfs2)
 		max_attrval_size=8192
 		;;
-	xfs|udf|9p|ceph)
+	xfs|udf|9p)
 		max_attrval_size=65536
 		;;
 	bcachefs)
@@ -139,6 +145,15 @@ _attr_get_max()
 		# the underlying filesystem, so just use the lowest value above.
 		max_attrval_size=1024
 		;;
+	ceph)
+		# CephFS does not have a maximum value for attributes.  Instead,
+		# it imposes a maximum size for the full set of xattrs
+		# names+values, which by default is 64K.  Compute the maximum
+		# taking into account the already existing attributes
+		max_attrval_size=$(getfattr --dump -e hex $filename 2>/dev/null | \
+			awk -F "=0x" '/^user/ {len += length($1) + length($2) / 2} END {print len}')
+		max_attrval_size=$((65536 - $max_attrval_size - $max_attrval_namelen))
+		;;
 	*)
 		# Assume max ~1 block of attrs
 		BLOCK_SIZE=`_get_block_size $TEST_DIR`
@@ -181,8 +196,7 @@ echo "*** remove attribute"
 _attr -r fish $testfile
 _attr_list $testfile
 
-max_attrval_name="long_attr"	# add 5 for "user." prefix
-_attr_get_max "$(( 5 + ${#max_attrval_name} ))"
+_attr_get_max
 
 echo "*** add lots of attributes"
 v=0
@@ -226,6 +240,9 @@ done
 _attr_list $testfile
 
 echo "*** really long value"
+max_attrval_name="long_attr"	# add 5 for "user." prefix
+_attr_get_maxval_size "$(( 5 + ${#max_attrval_name} ))" "$testfile"
+
 dd if=/dev/zero bs=1 count=$max_attrval_size 2>/dev/null \
     | _attr -s "$max_attrval_name" $testfile >/dev/null
 

Re: [PATCH v3 1/2] generic/020: adjust max_attrval_size for ceph

[David Disseldorp]

On Mon, 13 Jun 2022 12:31:41 +0100, Luís Henriques wrote:

> CephFS doesn't have a maximum xattr size.  Instead, it imposes a maximum
> size for the full set of xattrs names+values, which by default is 64K.
> 
> This patch fixes the max_attrval_size for ceph so that it is takes into
> account any already existing attrs in the file.
> 
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>  tests/generic/020 | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/tests/generic/020 b/tests/generic/020
> index d8648e96286e..b91bca34dcd4 100755
> --- a/tests/generic/020
> +++ b/tests/generic/020
> @@ -51,13 +51,9 @@ _attr_list()
>      fi
>  }
>  
> -# set 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.
> +# set fs-specific max_attrs
>  _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)
> @@ -112,6 +108,16 @@ _attr_get_max()
>  		# overhead
>  		let max_attrs=$BLOCK_SIZE/40
>  	esac
> +}
> +
> +# set fs-specific 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; parameter @filename is required for
> +# filesystems that need to take into account already existing attrs.
> +_attr_get_maxval_size()
> +{
> +	local max_attrval_namelen="$1"
> +	local filename="$2"
>  
>  	# Set max attr value size in bytes based on fs type
>  	case "$FSTYP" in
> @@ -128,7 +134,7 @@ _attr_get_max()
>  	pvfs2)
>  		max_attrval_size=8192
>  		;;
> -	xfs|udf|9p|ceph)
> +	xfs|udf|9p)
>  		max_attrval_size=65536
>  		;;
>  	bcachefs)
> @@ -139,6 +145,15 @@ _attr_get_max()
>  		# the underlying filesystem, so just use the lowest value above.
>  		max_attrval_size=1024
>  		;;
> +	ceph)
> +		# CephFS does not have a maximum value for attributes.  Instead,
> +		# it imposes a maximum size for the full set of xattrs
> +		# names+values, which by default is 64K.  Compute the maximum
> +		# taking into account the already existing attributes
> +		max_attrval_size=$(getfattr --dump -e hex $filename 2>/dev/null | \
> +			awk -F "=0x" '/^user/ {len += length($1) + length($2) / 2} END {print len}')
> +		max_attrval_size=$((65536 - $max_attrval_size - $max_attrval_namelen))
> +		;;
>  	*)
>  		# Assume max ~1 block of attrs
>  		BLOCK_SIZE=`_get_block_size $TEST_DIR`
> @@ -181,8 +196,7 @@ echo "*** remove attribute"
>  _attr -r fish $testfile
>  _attr_list $testfile
>  
> -max_attrval_name="long_attr"	# add 5 for "user." prefix
> -_attr_get_max "$(( 5 + ${#max_attrval_name} ))"
> +_attr_get_max
>  
>  echo "*** add lots of attributes"
>  v=0
> @@ -226,6 +240,9 @@ done
>  _attr_list $testfile
>  
>  echo "*** really long value"
> +max_attrval_name="long_attr"	# add 5 for "user." prefix
> +_attr_get_maxval_size "$(( 5 + ${#max_attrval_name} ))" "$testfile"
> +
>  dd if=/dev/zero bs=1 count=$max_attrval_size 2>/dev/null \
>      | _attr -s "$max_attrval_name" $testfile >/dev/null
>  

Looks good.
Reviewed-by: David Disseldorp <ddiss@suse.de>

[PATCH v3 2/2] generic/486: adjust the max xattr size

[Luís Henriques]

CephFS doesn't have a maximum xattr size.  Instead, it imposes a maximum
size for the full set of xattrs names+values, which by default is 64K.  And
since it reports 4M as the blocksize (the default ceph object size),
generic/486 will fail in ceph because the XATTR_SIZE_MAX value can't be used
in attr_replace_test.

The fix is to add a new argument to the test so that the max size can be
passed in instead of trying to auto-probe a value for it.

Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
 src/attr_replace_test.c | 30 ++++++++++++++++++++++++++----
 tests/generic/486       | 11 ++++++++++-
 2 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
index cca8dcf8ff60..1218e7264c8f 100644
--- a/src/attr_replace_test.c
+++ b/src/attr_replace_test.c
@@ -20,19 +20,41 @@ exit(1); } while (0)
 fprintf(stderr, __VA_ARGS__); exit (1); \
 } while (0)
 
+void usage(char *progname)
+{
+	fail("usage: %s [-m max_attr_size] <file>\n", progname);
+}
+
 int main(int argc, char *argv[])
 {
 	int ret;
 	int fd;
+	int c;
 	char *path;
 	char *name = "user.world";
 	char *value;
 	struct stat sbuf;
 	size_t size = sizeof(value);
+	size_t maxsize = XATTR_SIZE_MAX;
+
+	while ((c = getopt(argc, argv, "m:")) != -1) {
+		char *endp;
+
+		switch (c) {
+		case 'm':
+			maxsize = strtoul(optarg, &endp, 0);
+			if (*endp || (maxsize > XATTR_SIZE_MAX))
+				fail("Invalid 'max_attr_size' value\n");
+			break;
+		default:
+			usage(argv[0]);
+		}
+	}
 
-	if (argc != 2)
-		fail("Usage: %s <file>\n", argv[0]);
-	path = argv[1];
+	if (optind == argc - 1)
+		path = argv[optind];
+	else
+		usage(argv[0]);
 
 	fd = open(path, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
 	if (fd < 0) die();
@@ -46,7 +68,7 @@ int main(int argc, char *argv[])
 	size = sbuf.st_blksize * 3 / 4;
 	if (!size)
 		fail("Invalid st_blksize(%ld)\n", sbuf.st_blksize);
-	size = MIN(size, XATTR_SIZE_MAX);
+	size = MIN(size, maxsize);
 	value = malloc(size);
 	if (!value)
 		fail("Failed to allocate memory\n");
diff --git a/tests/generic/486 b/tests/generic/486
index 7de198f93a71..7dbfcb9835d9 100755
--- a/tests/generic/486
+++ b/tests/generic/486
@@ -41,7 +41,16 @@ filter_attr_output() {
 		sed -e 's/has a [0-9]* byte value/has a NNNN byte value/g'
 }
 
-$here/src/attr_replace_test $SCRATCH_MNT/hello
+max_attr_size=65536
+
+# attr_replace_test can't easily auto-probe the attr size for ceph because:
+# - ceph imposes a maximum value for the total xattr names+values, and
+# - ceph reports the 'object size' in the block size, which is, by default, much
+#   larger than XATTR_SIZE_MAX (4M > 64k)
+# Hence, we need to provide it with a maximum size.
+[ "$FSTYP" = "ceph" ] && max_attr_size=65000
+
+$here/src/attr_replace_test -m $max_attr_size $SCRATCH_MNT/hello
 $ATTR_PROG -l $SCRATCH_MNT/hello >>$seqres.full 2>&1
 $ATTR_PROG -l $SCRATCH_MNT/hello | filter_attr_output
 

Re: [PATCH v3 2/2] generic/486: adjust the max xattr size

[David Disseldorp]

On Mon, 13 Jun 2022 12:31:42 +0100, Luís Henriques wrote:

> CephFS doesn't have a maximum xattr size.  Instead, it imposes a maximum
> size for the full set of xattrs names+values, which by default is 64K.  And
> since it reports 4M as the blocksize (the default ceph object size),
> generic/486 will fail in ceph because the XATTR_SIZE_MAX value can't be used
> in attr_replace_test.
> 
> The fix is to add a new argument to the test so that the max size can be
> passed in instead of trying to auto-probe a value for it.
> 
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>  src/attr_replace_test.c | 30 ++++++++++++++++++++++++++----
>  tests/generic/486       | 11 ++++++++++-
>  2 files changed, 36 insertions(+), 5 deletions(-)
> 
> diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
> index cca8dcf8ff60..1218e7264c8f 100644
> --- a/src/attr_replace_test.c
> +++ b/src/attr_replace_test.c
> @@ -20,19 +20,41 @@ exit(1); } while (0)
>  fprintf(stderr, __VA_ARGS__); exit (1); \
>  } while (0)
>  
> +void usage(char *progname)
> +{
> +	fail("usage: %s [-m max_attr_size] <file>\n", progname);
> +}
> +
>  int main(int argc, char *argv[])
>  {
>  	int ret;
>  	int fd;
> +	int c;
>  	char *path;
>  	char *name = "user.world";
>  	char *value;
>  	struct stat sbuf;
>  	size_t size = sizeof(value);
> +	size_t maxsize = XATTR_SIZE_MAX;
> +
> +	while ((c = getopt(argc, argv, "m:")) != -1) {
> +		char *endp;
> +
> +		switch (c) {
> +		case 'm':
> +			maxsize = strtoul(optarg, &endp, 0);
> +			if (*endp || (maxsize > XATTR_SIZE_MAX))
> +				fail("Invalid 'max_attr_size' value\n");
> +			break;
> +		default:
> +			usage(argv[0]);
> +		}
> +	}
>  
> -	if (argc != 2)
> -		fail("Usage: %s <file>\n", argv[0]);
> -	path = argv[1];
> +	if (optind == argc - 1)
> +		path = argv[optind];
> +	else
> +		usage(argv[0]);
>  
>  	fd = open(path, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR);
>  	if (fd < 0) die();
> @@ -46,7 +68,7 @@ int main(int argc, char *argv[])
>  	size = sbuf.st_blksize * 3 / 4;
>  	if (!size)
>  		fail("Invalid st_blksize(%ld)\n", sbuf.st_blksize);
> -	size = MIN(size, XATTR_SIZE_MAX);
> +	size = MIN(size, maxsize);
>  	value = malloc(size);
>  	if (!value)
>  		fail("Failed to allocate memory\n");
> diff --git a/tests/generic/486 b/tests/generic/486
> index 7de198f93a71..7dbfcb9835d9 100755
> --- a/tests/generic/486
> +++ b/tests/generic/486
> @@ -41,7 +41,16 @@ filter_attr_output() {
>  		sed -e 's/has a [0-9]* byte value/has a NNNN byte value/g'
>  }
>  
> -$here/src/attr_replace_test $SCRATCH_MNT/hello
> +max_attr_size=65536
> +
> +# attr_replace_test can't easily auto-probe the attr size for ceph because:
> +# - ceph imposes a maximum value for the total xattr names+values, and
> +# - ceph reports the 'object size' in the block size, which is, by default, much
> +#   larger than XATTR_SIZE_MAX (4M > 64k)
> +# Hence, we need to provide it with a maximum size.
> +[ "$FSTYP" = "ceph" ] && max_attr_size=65000
> +
> +$here/src/attr_replace_test -m $max_attr_size $SCRATCH_MNT/hello
>  $ATTR_PROG -l $SCRATCH_MNT/hello >>$seqres.full 2>&1
>  $ATTR_PROG -l $SCRATCH_MNT/hello | filter_attr_output

Looks okay as an alternative to going through a 73aa648c
("generic/020: move MAX_ATTRS and MAX_ATTRVAL_SIZE logic") revert.

Reviewed-by: David Disseldorp <ddiss@suse.de>

Re: [PATCH v3 0/2] Two xattrs-related fixes for ceph

[Xiubo Li]

On 6/13/22 7:31 PM, Luís Henriques wrote:
> Hi!
>
> A bug fix in ceph has made some changes in the way xattr limits are
> enforced on the client side.  This requires some fixes on tests
> generic/020 and generic/486.
>
> * Changes since v2:
>
>    - patch 0001:
>      Add logic to compute the *real* maximum.  Kudos to David Disseldorp
>      for providing the right incantation to do the maths.  Also split
>      _attr_get_max() in two, so that they can be invoked from different
>      places in the test script.
>
>    - patch 002:
>      attr_replace_test now has an extra param as suggested by Dave Chinner,
>      and added fs-specific logic to the script.  No need to have an exact
>      maximum in this test, a big-enough value suffices.
>
> * Changes since v1:
>
>    - patch 0001:
>      Set the max size for xattrs values to a 65000, so that it is close to
>      the maximum, but still able to accommodate any pre-existing xattr
>
>    - patch 0002:
>      Same thing as patch 0001, but in a more precise way: actually take
>      into account the exact sizes for name+value of a pre-existing xattr.
>
> Luís Henriques (2):
>    generic/020: adjust max_attrval_size for ceph
>    generic/486: adjust the max xattr size
>
>   src/attr_replace_test.c | 30 ++++++++++++++++++++++++++----
>   tests/generic/020       | 33 +++++++++++++++++++++++++--------
>   tests/generic/486       | 11 ++++++++++-
>   3 files changed, 61 insertions(+), 13 deletions(-)
>
This series looks good to me.

Reviewed-by: Xiubo Li <xiubli@redhat.com>