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

[PATCH 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.

Cheers,
--
Luís

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

 src/attr_replace_test.c | 5 ++++-
 tests/generic/020       | 9 +++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

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

[Luís Henriques]

CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum
size for the full set of an inode's xattrs names+values, which by default
is 64K but it can be changed by a cluster admin.

Adjust max_attrval_size so that the test can be executed in this
filesystem.

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

diff --git a/tests/generic/020 b/tests/generic/020
index d8648e96286e..cadfce5f45e3 100755
--- a/tests/generic/020
+++ b/tests/generic/020
@@ -128,15 +128,16 @@ _attr_get_max()
 	pvfs2)
 		max_attrval_size=8192
 		;;
-	xfs|udf|9p|ceph)
+	xfs|udf|9p)
 		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.
+	nfs|ceph)
+		# NFS and CephFS don'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
 		;;
 	*)

[PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size

[Luís Henriques]

CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum
size for the full set of an inode's xattrs names+values, which by default
is 64K but it can be changed by a cluster admin.

Test generic/486 started to fail after fixing a ceph bug where this limit
wasn't being imposed.  Adjust dynamically the size of the xattr being set
if the error returned is -ENOSPC.

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

diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
index cca8dcf8ff60..de18e643f469 100644
--- a/src/attr_replace_test.c
+++ b/src/attr_replace_test.c
@@ -62,7 +62,10 @@ int main(int argc, char *argv[])
 
 	/* Then, replace it with bigger one, forcing short form to leaf conversion. */
 	memset(value, '1', size);
-	ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
+	do {
+		ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
+		size -= 256;
+	} while ((ret < 0) && (errno == ENOSPC) && (size > 0));
 	if (ret < 0) die();
 	close(fd);
 

Re: [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size

[Darrick J. Wong]

On Tue, Jun 07, 2022 at 04:15:13PM +0100, Luís Henriques wrote:
> CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum
> size for the full set of an inode's xattrs names+values, which by default
> is 64K but it can be changed by a cluster admin.
> 
> Test generic/486 started to fail after fixing a ceph bug where this limit
> wasn't being imposed.  Adjust dynamically the size of the xattr being set
> if the error returned is -ENOSPC.
> 
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>  src/attr_replace_test.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
> index cca8dcf8ff60..de18e643f469 100644
> --- a/src/attr_replace_test.c
> +++ b/src/attr_replace_test.c
> @@ -62,7 +62,10 @@ int main(int argc, char *argv[])
>  
>  	/* Then, replace it with bigger one, forcing short form to leaf conversion. */
>  	memset(value, '1', size);
> -	ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
> +	do {
> +		ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
> +		size -= 256;
> +	} while ((ret < 0) && (errno == ENOSPC) && (size > 0));

Isn't @size a size_t?  Which means that it can't be less than zero?  I
wouldn't count on st_blksize (or XATTR_SIZE_MAX) always being a multiple
of 256.

--D

>  	if (ret < 0) die();
>  	close(fd);
>  

Re: [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size

[Luís Henriques]

"Darrick J. Wong" <djwong@kernel.org> writes:

> On Tue, Jun 07, 2022 at 04:15:13PM +0100, Luís Henriques wrote:
>> CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum
>> size for the full set of an inode's xattrs names+values, which by default
>> is 64K but it can be changed by a cluster admin.
>> 
>> Test generic/486 started to fail after fixing a ceph bug where this limit
>> wasn't being imposed.  Adjust dynamically the size of the xattr being set
>> if the error returned is -ENOSPC.
>> 
>> Signed-off-by: Luís Henriques <lhenriques@suse.de>
>> ---
>>  src/attr_replace_test.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>> 
>> diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
>> index cca8dcf8ff60..de18e643f469 100644
>> --- a/src/attr_replace_test.c
>> +++ b/src/attr_replace_test.c
>> @@ -62,7 +62,10 @@ int main(int argc, char *argv[])
>>  
>>  	/* Then, replace it with bigger one, forcing short form to leaf conversion. */
>>  	memset(value, '1', size);
>> -	ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
>> +	do {
>> +		ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
>> +		size -= 256;
>> +	} while ((ret < 0) && (errno == ENOSPC) && (size > 0));
>
> Isn't @size a size_t?  Which means that it can't be less than zero?  I
> wouldn't count on st_blksize (or XATTR_SIZE_MAX) always being a multiple
> of 256.

*sigh*

You're right, of course.  Do you think it would be acceptable to do this
instead:

	} while ((ret < 0) && (errno == ENOSPC) && (size > 256));

It's still a magic number, but it should do the trick.  Although it's
still a bit ugly, I know.  My initial idea was to add an arg to this
program that would be then used as the value for 'size'; this way I could
add a ceph-specific value.  But not sure that's less ugly...

Cheers,
-- 
Luís

>
> --D
>
>>  	if (ret < 0) die();
>>  	close(fd);
>>  

[PATCH v2] src/attr_replace_test: dynamically adjust the max xattr size

[Luís Henriques]

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

Test generic/486 started to fail after fixing a ceph bug where this limit
wasn't being imposed.  Adjust dynamically the size of the xattr being set
if the error returned is -ENOSPC.

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

diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
index cca8dcf8ff60..d1b92703ba2a 100644
--- a/src/attr_replace_test.c
+++ b/src/attr_replace_test.c
@@ -62,7 +62,10 @@ int main(int argc, char *argv[])
 
 	/* Then, replace it with bigger one, forcing short form to leaf conversion. */
 	memset(value, '1', size);
-	ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
+	do {
+		ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
+		size -= 256;
+	} while ((ret < 0) && (errno == ENOSPC) && (size > 256));
 	if (ret < 0) die();
 	close(fd);
 

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

[Dave Chinner]

On Tue, Jun 07, 2022 at 04:15:12PM +0100, Luís Henriques wrote:
> CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum
> size for the full set of an inode's xattrs names+values, which by default
> is 64K but it can be changed by a cluster admin.

So given the max attr name length is fixed by the kernel at 255
bytes (XATTR_NAME_MAX), that means the max value length is somewhere
around 65000 bytes, not 1024 bytes?

Really, we want to stress and exercise max supported sizes - if the
admin reduces the max size on their test filesystems, that's not
something we should be trying to work around in the test suite by
preventing the test code from ever exercising attr values > 1024
bytes.....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size

[Dave Chinner]

On Tue, Jun 07, 2022 at 04:15:13PM +0100, Luís Henriques wrote:
> CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum
> size for the full set of an inode's xattrs names+values, which by default
> is 64K but it can be changed by a cluster admin.
> 
> Test generic/486 started to fail after fixing a ceph bug where this limit
> wasn't being imposed.  Adjust dynamically the size of the xattr being set
> if the error returned is -ENOSPC.

Ah, this shouldn't be getting anywhere near the 64kB limit unless
ceph is telling userspace it's block size is > 64kB:

size = sbuf.st_blksize * 3 / 4;
.....
size = MIN(size, XATTR_SIZE_MAX);

Regardless, the correct thing to do here is pass the max supported
xattr size from the command line (because fstests knows what that it
for each filesystem type) rather than hard coding
XATTR_SIZE_MAX in the test.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v2] src/attr_replace_test: dynamically adjust the max xattr size

[Xiubo Li]

On 6/8/22 12:51 AM, Luís Henriques wrote:
> CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum size
> for the full set of xattrs names+values, which by default is 64K but may be
> changed.
>
> Test generic/486 started to fail after fixing a ceph bug where this limit
> wasn't being imposed.  Adjust dynamically the size of the xattr being set
> if the error returned is -ENOSPC.
>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>   src/attr_replace_test.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
> index cca8dcf8ff60..d1b92703ba2a 100644
> --- a/src/attr_replace_test.c
> +++ b/src/attr_replace_test.c
> @@ -62,7 +62,10 @@ int main(int argc, char *argv[])
>   
>   	/* Then, replace it with bigger one, forcing short form to leaf conversion. */
>   	memset(value, '1', size);
> -	ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
> +	do {
> +		ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
> +		size -= 256;

I am not sure whether will this break other filesystems tests.

Maybe we should get the filesystem type first from 'st_mode', and then 
in ceph case we should minus strlen(name) before replacing the it. And 
then if it fails with '-ENOSPC' do the following ?

Or maybe we could get maximum length of xattr from ioctl(fd) in ceph case ?


> +	} while ((ret < 0) && (errno == ENOSPC) && (size > 256));
>   	if (ret < 0) die();
>   	close(fd);
>   
>

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

[Xiubo Li]

On 6/7/22 11:15 PM, Luís Henriques wrote:
> CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum
> size for the full set of an inode's xattrs names+values, which by default
> is 64K but it can be changed by a cluster admin.
>
> Adjust max_attrval_size so that the test can be executed in this
> filesystem.
>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>   tests/generic/020 | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/tests/generic/020 b/tests/generic/020
> index d8648e96286e..cadfce5f45e3 100755
> --- a/tests/generic/020
> +++ b/tests/generic/020
> @@ -128,15 +128,16 @@ _attr_get_max()
>   	pvfs2)
>   		max_attrval_size=8192
>   		;;
> -	xfs|udf|9p|ceph)
> +	xfs|udf|9p)
>   		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.
> +	nfs|ceph)
> +		# NFS and CephFS don'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
>   		;;
>   	*)

Why not fixing this by making sure that the total length of 'name' + 
'value' == 64K instead for ceph case ?

IMO we shouldn't worry about the case that the max could be changeable, 
we just need to test the framework works well with the default is 
enough. And then print a warning if the test fails to let users to know 
that the max size must be as default, which is 64K, or if users didn't 
change it then it should be a real bug in ceph.

Re: [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size

[Xiubo Li]

On 6/7/22 11:15 PM, Luís Henriques wrote:
> CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum
> size for the full set of an inode's xattrs names+values, which by default
> is 64K but it can be changed by a cluster admin.
>
> Test generic/486 started to fail after fixing a ceph bug where this limit
> wasn't being imposed.  Adjust dynamically the size of the xattr being set
> if the error returned is -ENOSPC.
>
> Signed-off-by: Luís Henriques <lhenriques@suse.de>
> ---
>   src/attr_replace_test.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/attr_replace_test.c b/src/attr_replace_test.c
> index cca8dcf8ff60..de18e643f469 100644
> --- a/src/attr_replace_test.c
> +++ b/src/attr_replace_test.c
> @@ -62,7 +62,10 @@ int main(int argc, char *argv[])
>   
>   	/* Then, replace it with bigger one, forcing short form to leaf conversion. */
>   	memset(value, '1', size);
> -	ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
> +	do {
> +		ret = fsetxattr(fd, name, value, size, XATTR_REPLACE);
> +		size -= 256;
> +	} while ((ret < 0) && (errno == ENOSPC) && (size > 0));
>   	if (ret < 0) die();
>   	close(fd);
>   

And also here, we shouldn't worry about users will change the default 
value, and we can just assume that users should test it with default value.

I am afraid this won't test real bug of the related code then.

Or as I mentioned in another thread, add one ioctl cmd to get the value ?

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

[Luís Henriques]

On Wed, Jun 08, 2022 at 10:16:42AM +1000, Dave Chinner wrote:
> On Tue, Jun 07, 2022 at 04:15:12PM +0100, Luís Henriques wrote:
> > CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum
> > size for the full set of an inode's xattrs names+values, which by default
> > is 64K but it can be changed by a cluster admin.
> 
> So given the max attr name length is fixed by the kernel at 255
> bytes (XATTR_NAME_MAX), that means the max value length is somewhere
> around 65000 bytes, not 1024 bytes?

Right, but if the name is smaller (and in this test specifically we're not
using that XATTR_NAME_MAX), then that max value is > 65000.  Or if the
file already has some attributes set (which is the case in this test),
then this maximum will need to be adjusted accordingly.  (See below.)

> Really, we want to stress and exercise max supported sizes - if the
> admin reduces the max size on their test filesystems, that's not
> something we should be trying to work around in the test suite by
> preventing the test code from ever exercising attr values > 1024
> bytes.....

Agreed.  Xiubo also noted that and I also think this test shouldn't care
about other values.  I should drop (or at least rephrase) the reference to
different values in the commit text.

On Wed, Jun 08, 2022 at 04:41:25PM +0800, Xiubo Li wrote:
...
> Why not fixing this by making sure that the total length of 'name' + 'value'
> == 64K instead for ceph case ?

The reason why I didn't do that is because the $testfile *already* has
another attribute set when we set this max value:

user.snrub="fish2\012"

which means that the maximum for this case will be:

 65536 - $max_attrval_namelen - strlen("user.snrub") - strlen("fish2\012")

I'll split the _attr_get_max() function in 2:

 * _attr_get_max() sets max_attrs which is needed in several places in
   generic/020
 * _attr_get_max_size() sets max_attrval_size, and gets called immediately
   before that value is needed so that it can take into account the
   current state.

Does this sound reasonable?

Cheers,
--
Luís

Re: [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size

[Luís Henriques]

On Wed, Jun 08, 2022 at 10:23:15AM +1000, Dave Chinner wrote:
> On Tue, Jun 07, 2022 at 04:15:13PM +0100, Luís Henriques wrote:
> > CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum
> > size for the full set of an inode's xattrs names+values, which by default
> > is 64K but it can be changed by a cluster admin.
> > 
> > Test generic/486 started to fail after fixing a ceph bug where this limit
> > wasn't being imposed.  Adjust dynamically the size of the xattr being set
> > if the error returned is -ENOSPC.
> 
> Ah, this shouldn't be getting anywhere near the 64kB limit unless
> ceph is telling userspace it's block size is > 64kB:
> 
> size = sbuf.st_blksize * 3 / 4;
> .....
> size = MIN(size, XATTR_SIZE_MAX);

Yep, that's exactly what is happening.  The cephfs kernel client reports
here the value that is being used for ceph "object size", which defaults
to 4M.  Hence, we'll set size to XATTR_SIZE_MAX.

> Regardless, the correct thing to do here is pass the max supported
> xattr size from the command line (because fstests knows what that it
> for each filesystem type) rather than hard coding
> XATTR_SIZE_MAX in the test.

OK, makes sense.  But then, for the ceph case, it becomes messy because we
also need to know the attribute name to compute the maximum size.  I guess
we'll need an extra argument for that too.

Cheers,
--
Luís

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

[Dave Chinner]

On Wed, Jun 08, 2022 at 10:46:40AM +0100, Luís Henriques wrote:
> On Wed, Jun 08, 2022 at 10:16:42AM +1000, Dave Chinner wrote:
> > On Tue, Jun 07, 2022 at 04:15:12PM +0100, Luís Henriques wrote:
> > > CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum
> > > size for the full set of an inode's xattrs names+values, which by default
> > > is 64K but it can be changed by a cluster admin.
> > 
> > So given the max attr name length is fixed by the kernel at 255
> > bytes (XATTR_NAME_MAX), that means the max value length is somewhere
> > around 65000 bytes, not 1024 bytes?
> 
> Right, but if the name is smaller (and in this test specifically we're not
> using that XATTR_NAME_MAX), then that max value is > 65000.  Or if the
> file already has some attributes set (which is the case in this test),
> then this maximum will need to be adjusted accordingly.  (See below.)
> 
> > Really, we want to stress and exercise max supported sizes - if the
> > admin reduces the max size on their test filesystems, that's not
> > something we should be trying to work around in the test suite by
> > preventing the test code from ever exercising attr values > 1024
> > bytes.....
> 
> Agreed.  Xiubo also noted that and I also think this test shouldn't care
> about other values.  I should drop (or at least rephrase) the reference to
> different values in the commit text.
> 
> On Wed, Jun 08, 2022 at 04:41:25PM +0800, Xiubo Li wrote:
> ...
> > Why not fixing this by making sure that the total length of 'name' + 'value'
> > == 64K instead for ceph case ?
> 
> The reason why I didn't do that is because the $testfile *already* has
> another attribute set when we set this max value:
> 
> user.snrub="fish2\012"
> 
> which means that the maximum for this case will be:
> 
>  65536 - $max_attrval_namelen - strlen("user.snrub") - strlen("fish2\012")
> 
> I'll split the _attr_get_max() function in 2:
> 
>  * _attr_get_max() sets max_attrs which is needed in several places in
>    generic/020
>  * _attr_get_max_size() sets max_attrval_size, and gets called immediately
>    before that value is needed so that it can take into account the
>    current state.
> 
> Does this sound reasonable?

It seems like unnecessary additional complexity - keep it simple.
Just set the max size for ceph to ~65000 and add a comment that says
max name+val length for all ceph attrs is 64k and we need enough
space of that space for two attr names...


Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size

[Dave Chinner]

On Wed, Jun 08, 2022 at 10:57:22AM +0100, Luís Henriques wrote:
> On Wed, Jun 08, 2022 at 10:23:15AM +1000, Dave Chinner wrote:
> > On Tue, Jun 07, 2022 at 04:15:13PM +0100, Luís Henriques wrote:
> > > CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum
> > > size for the full set of an inode's xattrs names+values, which by default
> > > is 64K but it can be changed by a cluster admin.
> > > 
> > > Test generic/486 started to fail after fixing a ceph bug where this limit
> > > wasn't being imposed.  Adjust dynamically the size of the xattr being set
> > > if the error returned is -ENOSPC.
> > 
> > Ah, this shouldn't be getting anywhere near the 64kB limit unless
> > ceph is telling userspace it's block size is > 64kB:
> > 
> > size = sbuf.st_blksize * 3 / 4;
> > .....
> > size = MIN(size, XATTR_SIZE_MAX);
> 
> Yep, that's exactly what is happening.  The cephfs kernel client reports
> here the value that is being used for ceph "object size", which defaults
> to 4M.  Hence, we'll set size to XATTR_SIZE_MAX.

Yikes. This is known to break random applications that size buffers
based on a multiple of sbuf.st_blksize and assume that it is going
to be roughly 4kB. e.g. size a buffer at 1024 * sbuf.st_blksize,
expecting to get a ~4MB buffer, and instead it tries to allocate
a 4GB buffer....

> > Regardless, the correct thing to do here is pass the max supported
> > xattr size from the command line (because fstests knows what that it
> > for each filesystem type) rather than hard coding
> > XATTR_SIZE_MAX in the test.
> 
> OK, makes sense.  But then, for the ceph case, it becomes messy because we
> also need to know the attribute name to compute the maximum size.  I guess
> we'll need an extra argument for that too.

Just pass in a size for ceph that has enough spare space for the
attribute names in it, like for g/020. Don't make it more
complex than it needs to be.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Luís Henriques]

On Thu, Jun 09, 2022 at 07:53:41AM +1000, Dave Chinner wrote:
> On Wed, Jun 08, 2022 at 10:46:40AM +0100, Luís Henriques wrote:
> > On Wed, Jun 08, 2022 at 10:16:42AM +1000, Dave Chinner wrote:
> > > On Tue, Jun 07, 2022 at 04:15:12PM +0100, Luís Henriques wrote:
> > > > CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum
> > > > size for the full set of an inode's xattrs names+values, which by default
> > > > is 64K but it can be changed by a cluster admin.
> > > 
> > > So given the max attr name length is fixed by the kernel at 255
> > > bytes (XATTR_NAME_MAX), that means the max value length is somewhere
> > > around 65000 bytes, not 1024 bytes?
> > 
> > Right, but if the name is smaller (and in this test specifically we're not
> > using that XATTR_NAME_MAX), then that max value is > 65000.  Or if the
> > file already has some attributes set (which is the case in this test),
> > then this maximum will need to be adjusted accordingly.  (See below.)
> > 
> > > Really, we want to stress and exercise max supported sizes - if the
> > > admin reduces the max size on their test filesystems, that's not
> > > something we should be trying to work around in the test suite by
> > > preventing the test code from ever exercising attr values > 1024
> > > bytes.....
> > 
> > Agreed.  Xiubo also noted that and I also think this test shouldn't care
> > about other values.  I should drop (or at least rephrase) the reference to
> > different values in the commit text.
> > 
> > On Wed, Jun 08, 2022 at 04:41:25PM +0800, Xiubo Li wrote:
> > ...
> > > Why not fixing this by making sure that the total length of 'name' + 'value'
> > > == 64K instead for ceph case ?
> > 
> > The reason why I didn't do that is because the $testfile *already* has
> > another attribute set when we set this max value:
> > 
> > user.snrub="fish2\012"
> > 
> > which means that the maximum for this case will be:
> > 
> >  65536 - $max_attrval_namelen - strlen("user.snrub") - strlen("fish2\012")
> > 
> > I'll split the _attr_get_max() function in 2:
> > 
> >  * _attr_get_max() sets max_attrs which is needed in several places in
> >    generic/020
> >  * _attr_get_max_size() sets max_attrval_size, and gets called immediately
> >    before that value is needed so that it can take into account the
> >    current state.
> > 
> > Does this sound reasonable?
> 
> It seems like unnecessary additional complexity - keep it simple.
> Just set the max size for ceph to ~65000 and add a comment that says
> max name+val length for all ceph attrs is 64k and we need enough
> space of that space for two attr names...

OK, that sounds reasonable.  I'll send out v2 shortly.  Thanks.

Cheers,
--
Luís

Re: [PATCH 2/2] src/attr_replace_test: dynamically adjust the max xattr size

[Luís Henriques]

On Thu, Jun 09, 2022 at 07:59:50AM +1000, Dave Chinner wrote:
> On Wed, Jun 08, 2022 at 10:57:22AM +0100, Luís Henriques wrote:
> > On Wed, Jun 08, 2022 at 10:23:15AM +1000, Dave Chinner wrote:
> > > On Tue, Jun 07, 2022 at 04:15:13PM +0100, Luís Henriques wrote:
> > > > CephFS doesn't had a maximum xattr size.  Instead, it imposes a maximum
> > > > size for the full set of an inode's xattrs names+values, which by default
> > > > is 64K but it can be changed by a cluster admin.
> > > > 
> > > > Test generic/486 started to fail after fixing a ceph bug where this limit
> > > > wasn't being imposed.  Adjust dynamically the size of the xattr being set
> > > > if the error returned is -ENOSPC.
> > > 
> > > Ah, this shouldn't be getting anywhere near the 64kB limit unless
> > > ceph is telling userspace it's block size is > 64kB:
> > > 
> > > size = sbuf.st_blksize * 3 / 4;
> > > .....
> > > size = MIN(size, XATTR_SIZE_MAX);
> > 
> > Yep, that's exactly what is happening.  The cephfs kernel client reports
> > here the value that is being used for ceph "object size", which defaults
> > to 4M.  Hence, we'll set size to XATTR_SIZE_MAX.
> 
> Yikes. This is known to break random applications that size buffers
> based on a multiple of sbuf.st_blksize and assume that it is going
> to be roughly 4kB. e.g. size a buffer at 1024 * sbuf.st_blksize,
> expecting to get a ~4MB buffer, and instead it tries to allocate
> a 4GB buffer....
> 
> > > Regardless, the correct thing to do here is pass the max supported
> > > xattr size from the command line (because fstests knows what that it
> > > for each filesystem type) rather than hard coding
> > > XATTR_SIZE_MAX in the test.
> > 
> > OK, makes sense.  But then, for the ceph case, it becomes messy because we
> > also need to know the attribute name to compute the maximum size.  I guess
> > we'll need an extra argument for that too.
> 
> Just pass in a size for ceph that has enough spare space for the
> attribute names in it, like for g/020. Don't make it more
> complex than it needs to be.

Well, in that case it's just easier for attr_replace_test.c to simply set
the ceiling to (XATTR_SIZE_MAX - strlen(name+value)).  This is because the
fstests don't really know anymore the max xattr size for each filesystem
type; that knowledge is local to generic/020.  The other option is to move
that code (back) to common/attr.

Cheers,
--
Luís