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

[PATCH v2 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 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 |  7 ++++++-
 tests/generic/020       | 10 +++++++++-
 2 files changed, 15 insertions(+), 2 deletions(-)

[PATCH v2 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 so that it is slightly < 64K in
order to accommodate any already existing xattrs in the file.

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

diff --git a/tests/generic/020 b/tests/generic/020
index d8648e96286e..76f13220fe85 100755
--- a/tests/generic/020
+++ b/tests/generic/020
@@ -128,7 +128,7 @@ _attr_get_max()
 	pvfs2)
 		max_attrval_size=8192
 		;;
-	xfs|udf|9p|ceph)
+	xfs|udf|9p)
 		max_attrval_size=65536
 		;;
 	bcachefs)
@@ -139,6 +139,14 @@ _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.  Set this to a value
+		# that is slightly smaller than 64K so that it can accommodate
+		# already existing xattrs.
+		max_attrval_size=65000
+		;;
 	*)
 		# Assume max ~1 block of attrs
 		BLOCK_SIZE=`_get_block_size $TEST_DIR`

[PATCH v2 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 ceph reports 4M as the blocksize (the default ceph object size),
generic/486 will fail in this filesystem because it will end up using
XATTR_SIZE_MAX to set the size of the 2nd (big) xattr value.

The fix is to adjust the max size in attr_replace_test so that it takes
into account the initial xattr name and value lengths.

Signed-off-by: Luís Henriques <lhenriques@suse.de>
---
 src/attr_replace_test.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
index cca8dcf8ff60..1c8d1049a1d8 100644
--- a/src/attr_replace_test.c
+++ b/src/attr_replace_test.c
@@ -29,6 +29,11 @@ int main(int argc, char *argv[])
 	char *value;
 	struct stat sbuf;
 	size_t size = sizeof(value);
+	/*
+	 * Take into account the initial (small) xattr name and value sizes and
+	 * subtract them from the XATTR_SIZE_MAX maximum.
+	 */
+	size_t maxsize = XATTR_SIZE_MAX - strlen(name) - 1;
 
 	if (argc != 2)
 		fail("Usage: %s <file>\n", argv[0]);
@@ -46,7 +51,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");

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

[David Disseldorp]

Hi Luís,

On Thu,  9 Jun 2022 11:53: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.
> 
> This patch fixes the max_attrval_size so that it is slightly < 64K in
> order to accommodate any already existing xattrs in the file.
> 
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>  tests/generic/020 | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/generic/020 b/tests/generic/020
> index d8648e96286e..76f13220fe85 100755
> --- a/tests/generic/020
> +++ b/tests/generic/020
> @@ -128,7 +128,7 @@ _attr_get_max()
>  	pvfs2)
>  		max_attrval_size=8192
>  		;;
> -	xfs|udf|9p|ceph)
> +	xfs|udf|9p)
>  		max_attrval_size=65536
>  		;;
>  	bcachefs)
> @@ -139,6 +139,14 @@ _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.  Set this to a value
> +		# that is slightly smaller than 64K so that it can accommodate
> +		# already existing xattrs.
> +		max_attrval_size=65000
> +		;;

I take it a more exact calculation would be something like:
(64K - $max_attrval_namelen - sizeof(user.snrub="fish2\012"))?

Perhaps you could calculate this on the fly for CephFS by passing in the
filename and subtracting the `getfattr -d $filename` results... That
said, it'd probably get a bit ugly, expecially if encoding needs to be
taken into account.

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

Cheers, David

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

[Luís Henriques]

David Disseldorp <ddiss@suse.de> writes:

> Hi Luís,
>
> On Thu,  9 Jun 2022 11:53: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.
>> 
>> This patch fixes the max_attrval_size so that it is slightly < 64K in
>> order to accommodate any already existing xattrs in the file.
>> 
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> ---
>>  tests/generic/020 | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>> 
>> diff --git a/tests/generic/020 b/tests/generic/020
>> index d8648e96286e..76f13220fe85 100755
>> --- a/tests/generic/020
>> +++ b/tests/generic/020
>> @@ -128,7 +128,7 @@ _attr_get_max()
>>  	pvfs2)
>>  		max_attrval_size=8192
>>  		;;
>> -	xfs|udf|9p|ceph)
>> +	xfs|udf|9p)
>>  		max_attrval_size=65536
>>  		;;
>>  	bcachefs)
>> @@ -139,6 +139,14 @@ _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.  Set this to a value
>> +		# that is slightly smaller than 64K so that it can accommodate
>> +		# already existing xattrs.
>> +		max_attrval_size=65000
>> +		;;
>
> I take it a more exact calculation would be something like:
> (64K - $max_attrval_namelen - sizeof(user.snrub="fish2\012"))?
>
> Perhaps you could calculate this on the fly for CephFS by passing in the
> filename and subtracting the `getfattr -d $filename` results... That
> said, it'd probably get a bit ugly, expecially if encoding needs to be
> taken into account.

In fact, this is *exactly* what I had before Dave suggested to keep it
simple.  After moving the code back into common/attr, where's how the
generic code would look like:

+       ceph)
+		# CephFS does have a limit for the whole set of names+values
+		# attributes in a file.  Thus, it is necessary to get the sizes
+		# of all names and values already existent and subtract them to
+		# the (default) maximum, which is 64k.
+		local len=0
+		while read line; do
+			# skip 1st line
+			[ "$line" != "${line#'#'}" ] && continue
+			n=$(echo $line | awk -F"=0x" '{print $1}')
+			v=$(echo $line | awk -F"=0x" '{print $2}')
+			nlen=${#n}
+			vlen=${#v}
+			# total is the sum of the name len and the value len
+			# divided by 2 because we're dumping them in hex format
+			t=$(($nlen + $vlen / 2))
+			len=$(($len + $t))
+		done <<< $(_getfattr -d -e hex $file 2> /dev/null)
+		echo $((65536 - $max_attrval_namelen - $len))
+		;;

so... yeah, I'm not particularly gifted on shell, it could probably be
done in more clever/cleaner ways.  Anyway, I'm open to revisit this if
this is the preferred solution.

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

Thanks David.  (And sorry!  I completely forgot to include you on CC as I
had promised.)

Cheers,
-- 
Luís

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

[David Disseldorp]

On Thu, 09 Jun 2022 15:54:15 +0100, Luís Henriques wrote:

> David Disseldorp <ddiss@suse.de> writes:
...
> > I take it a more exact calculation would be something like:
> > (64K - $max_attrval_namelen - sizeof(user.snrub="fish2\012"))?
> >
> > Perhaps you could calculate this on the fly for CephFS by passing in the
> > filename and subtracting the `getfattr -d $filename` results... That
> > said, it'd probably get a bit ugly, expecially if encoding needs to be
> > taken into account.  
> 
> In fact, this is *exactly* what I had before Dave suggested to keep it
> simple.

Arg, sorry I missed your previous round.

> After moving the code back into common/attr, where's how the
> generic code would look like:
> 
> +       ceph)
> +		# CephFS does have a limit for the whole set of names+values
> +		# attributes in a file.  Thus, it is necessary to get the sizes
> +		# of all names and values already existent and subtract them to
> +		# the (default) maximum, which is 64k.
> +		local len=0
> +		while read line; do
> +			# skip 1st line
> +			[ "$line" != "${line#'#'}" ] && continue
> +			n=$(echo $line | awk -F"=0x" '{print $1}')
> +			v=$(echo $line | awk -F"=0x" '{print $2}')
> +			nlen=${#n}
> +			vlen=${#v}
> +			# total is the sum of the name len and the value len
> +			# divided by 2 because we're dumping them in hex format
> +			t=$(($nlen + $vlen / 2))
> +			len=$(($len + $t))
> +		done <<< $(_getfattr -d -e hex $file 2> /dev/null)
> +		echo $((65536 - $max_attrval_namelen - $len))
> +		;;
> 
> so... yeah, I'm not particularly gifted on shell, it could probably be
> done in more clever/cleaner ways.  Anyway, I'm open to revisit this if
> this is the preferred solution.

hmm, I was hoping something like...
(( 65536 - $max_attrval_namelen - $(getfattr -d $file | _filter | wc -c) ))
would be possible, but getfattr output does make it a bit too messy.

Cheers, David

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

[Xiubo Li]

On 6/9/22 10:21 PM, David Disseldorp wrote:
> Hi Luís,
>
> On Thu,  9 Jun 2022 11:53: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.
>>
>> This patch fixes the max_attrval_size so that it is slightly < 64K in
>> order to accommodate any already existing xattrs in the file.
>>
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> ---
>>   tests/generic/020 | 10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/tests/generic/020 b/tests/generic/020
>> index d8648e96286e..76f13220fe85 100755
>> --- a/tests/generic/020
>> +++ b/tests/generic/020
>> @@ -128,7 +128,7 @@ _attr_get_max()
>>   	pvfs2)
>>   		max_attrval_size=8192
>>   		;;
>> -	xfs|udf|9p|ceph)
>> +	xfs|udf|9p)
>>   		max_attrval_size=65536
>>   		;;
>>   	bcachefs)
>> @@ -139,6 +139,14 @@ _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.  Set this to a value
>> +		# that is slightly smaller than 64K so that it can accommodate
>> +		# already existing xattrs.
>> +		max_attrval_size=65000
>> +		;;
> I take it a more exact calculation would be something like:
> (64K - $max_attrval_namelen - sizeof(user.snrub="fish2\012"))?

Yeah, something like this looks better to me.

I am afraid without reaching up to the real max size we couldn't test 
the real bugs out from ceph. Such as the bug you fixed in ceph Locker.cc 
code.


> Perhaps you could calculate this on the fly for CephFS by passing in the
> filename and subtracting the `getfattr -d $filename` results... That
> said, it'd probably get a bit ugly, expecially if encoding needs to be
> taken into account.
>
> Reviewed-by: David Disseldorp <ddiss@suse.de>
>
> Cheers, David
>

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

[Xiubo Li]

On 6/9/22 6:53 PM, 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 ceph reports 4M as the blocksize (the default ceph object size),
> generic/486 will fail in this filesystem because it will end up using
> XATTR_SIZE_MAX to set the size of the 2nd (big) xattr value.
>
> The fix is to adjust the max size in attr_replace_test so that it takes
> into account the initial xattr name and value lengths.
>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>   src/attr_replace_test.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
> index cca8dcf8ff60..1c8d1049a1d8 100644
> --- a/src/attr_replace_test.c
> +++ b/src/attr_replace_test.c
> @@ -29,6 +29,11 @@ int main(int argc, char *argv[])
>   	char *value;
>   	struct stat sbuf;
>   	size_t size = sizeof(value);
> +	/*
> +	 * Take into account the initial (small) xattr name and value sizes and
> +	 * subtract them from the XATTR_SIZE_MAX maximum.
> +	 */
> +	size_t maxsize = XATTR_SIZE_MAX - strlen(name) - 1;

Why not use the statfs to get the filesystem type first ? And then just 
minus the strlen(name) for ceph only ?


>   
>   	if (argc != 2)
>   		fail("Usage: %s <file>\n", argv[0]);
> @@ -46,7 +51,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");
>

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

[Dave Chinner]

On Fri, Jun 10, 2022 at 01:35:36PM +0800, Xiubo Li wrote:
> 
> On 6/9/22 6:53 PM, 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 ceph reports 4M as the blocksize (the default ceph object size),
> > generic/486 will fail in this filesystem because it will end up using
> > XATTR_SIZE_MAX to set the size of the 2nd (big) xattr value.
> > 
> > The fix is to adjust the max size in attr_replace_test so that it takes
> > into account the initial xattr name and value lengths.
> > 
> > Signed-off-by: Luís Henriques <lhenriques@suse.de>
> > ---
> >   src/attr_replace_test.c | 7 ++++++-
> >   1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
> > index cca8dcf8ff60..1c8d1049a1d8 100644
> > --- a/src/attr_replace_test.c
> > +++ b/src/attr_replace_test.c
> > @@ -29,6 +29,11 @@ int main(int argc, char *argv[])
> >   	char *value;
> >   	struct stat sbuf;
> >   	size_t size = sizeof(value);
> > +	/*
> > +	 * Take into account the initial (small) xattr name and value sizes and
> > +	 * subtract them from the XATTR_SIZE_MAX maximum.
> > +	 */
> > +	size_t maxsize = XATTR_SIZE_MAX - strlen(name) - 1;
> 
> Why not use the statfs to get the filesystem type first ? And then just
> minus the strlen(name) for ceph only ?

No. The test mechanism has no business knowing what filesystem type
it is running on - the test itself is supposed to get the limits for
the filesystem type from the test infrastructure.

As I've already said: the right thing to do is to pass the maximum
attr size for the test to use via the command line from the fstest
itself. As per g/020, the fstests infrastructure is where we encode
weird fs limit differences and behaviours based on $FSTYP.  Hacking
around weird filesystem specific behaviours deep inside random bits
of test source code is not maintainable.

AFAIA, only ceph is having a problem with this test, so it's trivial
to encode into g/486 with:

# ceph has a weird dynamic maximum xattr size and block size that is
# much, much larger than the maximum supported attr size. Hence the
# replace test can't auto-probe a sane attr size and so we have
# to provide it with a maximum size that will work.
max_attr_size=65536
[ "$FSTYP" = "ceph" ] && max_attr_size=64000
attr_replace_test -m $max_attr_size .....
.....

Cheers,

Dave.

-- 
Dave Chinner
david@fromorbit.com

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

[Zorro Lang]

On Fri, Jun 10, 2022 at 05:25:45PM +1000, Dave Chinner wrote:
> On Fri, Jun 10, 2022 at 01:35:36PM +0800, Xiubo Li wrote:
> > 
> > On 6/9/22 6:53 PM, 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 ceph reports 4M as the blocksize (the default ceph object size),
> > > generic/486 will fail in this filesystem because it will end up using
> > > XATTR_SIZE_MAX to set the size of the 2nd (big) xattr value.
> > > 
> > > The fix is to adjust the max size in attr_replace_test so that it takes
> > > into account the initial xattr name and value lengths.
> > > 
> > > Signed-off-by: Luís Henriques <lhenriques@suse.de>
> > > ---
> > >   src/attr_replace_test.c | 7 ++++++-
> > >   1 file changed, 6 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
> > > index cca8dcf8ff60..1c8d1049a1d8 100644
> > > --- a/src/attr_replace_test.c
> > > +++ b/src/attr_replace_test.c
> > > @@ -29,6 +29,11 @@ int main(int argc, char *argv[])
> > >   	char *value;
> > >   	struct stat sbuf;
> > >   	size_t size = sizeof(value);
> > > +	/*
> > > +	 * Take into account the initial (small) xattr name and value sizes and
> > > +	 * subtract them from the XATTR_SIZE_MAX maximum.
> > > +	 */
> > > +	size_t maxsize = XATTR_SIZE_MAX - strlen(name) - 1;
> > 
> > Why not use the statfs to get the filesystem type first ? And then just
> > minus the strlen(name) for ceph only ?
> 
> No. The test mechanism has no business knowing what filesystem type
> it is running on - the test itself is supposed to get the limits for
> the filesystem type from the test infrastructure.
> 
> As I've already said: the right thing to do is to pass the maximum
> attr size for the test to use via the command line from the fstest
> itself. As per g/020, the fstests infrastructure is where we encode
> weird fs limit differences and behaviours based on $FSTYP.  Hacking
> around weird filesystem specific behaviours deep inside random bits
> of test source code is not maintainable.
> 
> AFAIA, only ceph is having a problem with this test, so it's trivial
> to encode into g/486 with:
> 
> # ceph has a weird dynamic maximum xattr size and block size that is
> # much, much larger than the maximum supported attr size. Hence the
> # replace test can't auto-probe a sane attr size and so we have
> # to provide it with a maximum size that will work.
> max_attr_size=65536
> [ "$FSTYP" = "ceph" ] && max_attr_size=64000
> attr_replace_test -m $max_attr_size .....
> .....

Agree. I'd recommend changing the attr_replace_test.c, make it have a
default max xattr size (keep using the XATTR_SIZE_MAX or define one if
it's not defined), then give it an optinal option which can specify a
customed max xattr size from outside.

Then the test case (e.g. g/486) which uses attr_replace_test can
specify a max xattr size if it needs. And it's easier to figure
out what attr size is better for a specified fs in test case.

Thanks,
Zorro

> 
> Cheers,
> 
> Dave.
> 
> -- 
> Dave Chinner
> david@fromorbit.com
> 

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

[Luis Henriques]

David Disseldorp <ddiss@suse.de> writes:

> On Thu, 09 Jun 2022 15:54:15 +0100, Luís Henriques wrote:
>
>> David Disseldorp <ddiss@suse.de> writes:
> ...
>> > I take it a more exact calculation would be something like:
>> > (64K - $max_attrval_namelen - sizeof(user.snrub="fish2\012"))?
>> >
>> > Perhaps you could calculate this on the fly for CephFS by passing in the
>> > filename and subtracting the `getfattr -d $filename` results... That
>> > said, it'd probably get a bit ugly, expecially if encoding needs to be
>> > taken into account.  
>> 
>> In fact, this is *exactly* what I had before Dave suggested to keep it
>> simple.
>
> Arg, sorry I missed your previous round.
>
>> After moving the code back into common/attr, where's how the
>> generic code would look like:
>> 
>> +       ceph)
>> +		# CephFS does have a limit for the whole set of names+values
>> +		# attributes in a file.  Thus, it is necessary to get the sizes
>> +		# of all names and values already existent and subtract them to
>> +		# the (default) maximum, which is 64k.
>> +		local len=0
>> +		while read line; do
>> +			# skip 1st line
>> +			[ "$line" != "${line#'#'}" ] && continue
>> +			n=$(echo $line | awk -F"=0x" '{print $1}')
>> +			v=$(echo $line | awk -F"=0x" '{print $2}')
>> +			nlen=${#n}
>> +			vlen=${#v}
>> +			# total is the sum of the name len and the value len
>> +			# divided by 2 because we're dumping them in hex format
>> +			t=$(($nlen + $vlen / 2))
>> +			len=$(($len + $t))
>> +		done <<< $(_getfattr -d -e hex $file 2> /dev/null)
>> +		echo $((65536 - $max_attrval_namelen - $len))
>> +		;;
>> 
>> so... yeah, I'm not particularly gifted on shell, it could probably be
>> done in more clever/cleaner ways.  Anyway, I'm open to revisit this if
>> this is the preferred solution.
>
> hmm, I was hoping something like...
> (( 65536 - $max_attrval_namelen - $(getfattr -d $file | _filter | wc -c) ))
> would be possible, but getfattr output does make it a bit too messy.

Yeah, also we must decode the attributes as hex otherwise we'll miss
non-string values.  Anyway, I'll see if I find something better.  Thanks,
David.

Cheers,
-- 
Luis

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

[Luis Henriques]

Xiubo Li <xiubli@redhat.com> writes:

> On 6/9/22 10:21 PM, David Disseldorp wrote:
>> Hi Luís,
>>
>> On Thu,  9 Jun 2022 11:53: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.
>>>
>>> This patch fixes the max_attrval_size so that it is slightly < 64K in
>>> order to accommodate any already existing xattrs in the file.
>>>
>>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>>> ---
>>>   tests/generic/020 | 10 +++++++++-
>>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tests/generic/020 b/tests/generic/020
>>> index d8648e96286e..76f13220fe85 100755
>>> --- a/tests/generic/020
>>> +++ b/tests/generic/020
>>> @@ -128,7 +128,7 @@ _attr_get_max()
>>>   	pvfs2)
>>>   		max_attrval_size=8192
>>>   		;;
>>> -	xfs|udf|9p|ceph)
>>> +	xfs|udf|9p)
>>>   		max_attrval_size=65536
>>>   		;;
>>>   	bcachefs)
>>> @@ -139,6 +139,14 @@ _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.  Set this to a value
>>> +		# that is slightly smaller than 64K so that it can accommodate
>>> +		# already existing xattrs.
>>> +		max_attrval_size=65000
>>> +		;;
>> I take it a more exact calculation would be something like:
>> (64K - $max_attrval_namelen - sizeof(user.snrub="fish2\012"))?
>
> Yeah, something like this looks better to me.

Right, it could be hard-coded.  But we'd need to take into account that
the attribute value may not be ascii.  That's why my initial attempt to
fix this was to decode everything in hex.

> I am afraid without reaching up to the real max size we couldn't test the real
> bugs out from ceph. Such as the bug you fixed in ceph Locker.cc code.

OK, I'll change this to use the exact value.  Thanks, Xiubo.

Cheers,
-- 
Luis

>
>> Perhaps you could calculate this on the fly for CephFS by passing in the
>> filename and subtracting the `getfattr -d $filename` results... That
>> said, it'd probably get a bit ugly, expecially if encoding needs to be
>> taken into account.
>>
>> Reviewed-by: David Disseldorp <ddiss@suse.de>
>>
>> Cheers, David
>>
>

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

[Luís Henriques]

Zorro Lang <zlang@redhat.com> writes:

> On Fri, Jun 10, 2022 at 05:25:45PM +1000, Dave Chinner wrote:
>> On Fri, Jun 10, 2022 at 01:35:36PM +0800, Xiubo Li wrote:
>> > 
>> > On 6/9/22 6:53 PM, 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 ceph reports 4M as the blocksize (the default ceph object size),
>> > > generic/486 will fail in this filesystem because it will end up using
>> > > XATTR_SIZE_MAX to set the size of the 2nd (big) xattr value.
>> > > 
>> > > The fix is to adjust the max size in attr_replace_test so that it takes
>> > > into account the initial xattr name and value lengths.
>> > > 
>> > > Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> > > ---
>> > >   src/attr_replace_test.c | 7 ++++++-
>> > >   1 file changed, 6 insertions(+), 1 deletion(-)
>> > > 
>> > > diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
>> > > index cca8dcf8ff60..1c8d1049a1d8 100644
>> > > --- a/src/attr_replace_test.c
>> > > +++ b/src/attr_replace_test.c
>> > > @@ -29,6 +29,11 @@ int main(int argc, char *argv[])
>> > >   	char *value;
>> > >   	struct stat sbuf;
>> > >   	size_t size = sizeof(value);
>> > > +	/*
>> > > +	 * Take into account the initial (small) xattr name and value sizes and
>> > > +	 * subtract them from the XATTR_SIZE_MAX maximum.
>> > > +	 */
>> > > +	size_t maxsize = XATTR_SIZE_MAX - strlen(name) - 1;
>> > 
>> > Why not use the statfs to get the filesystem type first ? And then just
>> > minus the strlen(name) for ceph only ?
>> 
>> No. The test mechanism has no business knowing what filesystem type
>> it is running on - the test itself is supposed to get the limits for
>> the filesystem type from the test infrastructure.
>> 
>> As I've already said: the right thing to do is to pass the maximum
>> attr size for the test to use via the command line from the fstest
>> itself. As per g/020, the fstests infrastructure is where we encode
>> weird fs limit differences and behaviours based on $FSTYP.  Hacking
>> around weird filesystem specific behaviours deep inside random bits
>> of test source code is not maintainable.
>> 
>> AFAIA, only ceph is having a problem with this test, so it's trivial
>> to encode into g/486 with:
>> 
>> # ceph has a weird dynamic maximum xattr size and block size that is
>> # much, much larger than the maximum supported attr size. Hence the
>> # replace test can't auto-probe a sane attr size and so we have
>> # to provide it with a maximum size that will work.
>> max_attr_size=65536
>> [ "$FSTYP" = "ceph" ] && max_attr_size=64000
>> attr_replace_test -m $max_attr_size .....
>> .....
>
> Agree. I'd recommend changing the attr_replace_test.c, make it have a
> default max xattr size (keep using the XATTR_SIZE_MAX or define one if
> it's not defined), then give it an optinal option which can specify a
> customed max xattr size from outside.
>
> Then the test case (e.g. g/486) which uses attr_replace_test can
> specify a max xattr size if it needs. And it's easier to figure
> out what attr size is better for a specified fs in test case.

Awesome, thanks.  I'll send out next rev with these changes.  Thank you
all.

Cheers,
-- 
Luís