* [PATCH] common/attr: reduce MAX_ATTRVAL_SIZE for NFS @ 2022-04-08 19:19 David Disseldorp 2022-04-11 5:47 ` Dave Chinner 0 siblings, 1 reply; 4+ messages in thread From: David Disseldorp @ 2022-04-08 19:19 UTC (permalink / raw) To: fstests; +Cc: David Disseldorp When testing against an NFS export backed by a Btrfs filesystem, generic/020 may fail, e.g. --- /xfstests/tests/generic/020.out +++ /xfstests/results/generic/020.out.bad @@ -47,9 +47,13 @@ user.snrub="fish2\012" *** really long value -0000000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 -* -ATTRSIZE +attr_set: No space left on device +Could not set "long_attr" for <TESTFILE> +attr_get: No data available +Could not get "long_attr" for <TESTFILE> +0000000 +attr_remove: No data available +Could not remove "long_attr" for <TESTFILE> This is due to the MAX_ATTRVAL_SIZE=65536 setting for NFS, which exceeds the Btrfs (and XFS) limit of MAX_ATTRVAL_SIZE=64. Change NFS to use this lower bound value. Signed-off-by: David Disseldorp <ddiss@suse.de> --- common/attr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/common/attr b/common/attr index dae8a1bb..1778e8b2 100644 --- a/common/attr +++ b/common/attr @@ -319,13 +319,13 @@ export MAX_ATTRS # Set max attr value size based on fs type case "$FSTYP" in -xfs|udf|btrfs) +xfs|udf|btrfs|nfs) MAX_ATTRVAL_SIZE=64 ;; pvfs2) MAX_ATTRVAL_SIZE=8192 ;; -9p|ceph|nfs) +9p|ceph) MAX_ATTRVAL_SIZE=65536 ;; bcachefs) -- 2.34.1 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] common/attr: reduce MAX_ATTRVAL_SIZE for NFS 2022-04-08 19:19 [PATCH] common/attr: reduce MAX_ATTRVAL_SIZE for NFS David Disseldorp @ 2022-04-11 5:47 ` Dave Chinner 2022-04-11 8:57 ` David Disseldorp 0 siblings, 1 reply; 4+ messages in thread From: Dave Chinner @ 2022-04-11 5:47 UTC (permalink / raw) To: David Disseldorp; +Cc: fstests On Fri, Apr 08, 2022 at 09:19:43PM +0200, David Disseldorp wrote: > When testing against an NFS export backed by a Btrfs filesystem, > generic/020 may fail, e.g. > > --- /xfstests/tests/generic/020.out > +++ /xfstests/results/generic/020.out.bad > @@ -47,9 +47,13 @@ > user.snrub="fish2\012" > > *** really long value > -0000000 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 > -* > -ATTRSIZE > +attr_set: No space left on device > +Could not set "long_attr" for <TESTFILE> > +attr_get: No data available > +Could not get "long_attr" for <TESTFILE> > +0000000 > +attr_remove: No data available > +Could not remove "long_attr" for <TESTFILE> > > This is due to the MAX_ATTRVAL_SIZE=65536 setting for NFS, which exceeds > the Btrfs (and XFS) limit of MAX_ATTRVAL_SIZE=64. Change NFS to use this > lower bound value. I think that what XFS/UDF/BTRFS set here is bogus. There is *one* test - generic/020 - that uses MAX_ATTRS and MAX_ATTRVAL_SIZE, and it uses MAX_ATTRVAL_SIZE as a byte count. Which means for those 3 filesytems, the correct value for them is 65536, not 64.... This looks like it was broken back when the test was made generic - the dd command before this used a bs=1024, so a maz size of 64 would have been correct. Except the dd command also go changed to use bs=1, which meant 64 bytes.... So, yeah, the test got "broken" for XFS back in 2012 and so the correct fix here is to change (at least) XFS and btrfs to have a MAX_ATTRVAL_SIZE=65536.... Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] common/attr: reduce MAX_ATTRVAL_SIZE for NFS 2022-04-11 5:47 ` Dave Chinner @ 2022-04-11 8:57 ` David Disseldorp 2022-04-11 22:29 ` Dave Chinner 0 siblings, 1 reply; 4+ messages in thread From: David Disseldorp @ 2022-04-11 8:57 UTC (permalink / raw) To: Dave Chinner; +Cc: fstests Thanks for the review, Dave... On Mon, 11 Apr 2022 15:47:14 +1000, Dave Chinner wrote: > > This is due to the MAX_ATTRVAL_SIZE=65536 setting for NFS, which exceeds > > the Btrfs (and XFS) limit of MAX_ATTRVAL_SIZE=64. Change NFS to use this > > lower bound value. > > I think that what XFS/UDF/BTRFS set here is bogus. > > There is *one* test - generic/020 - that uses MAX_ATTRS and > MAX_ATTRVAL_SIZE, and it uses MAX_ATTRVAL_SIZE as a byte count. > > Which means for those 3 filesytems, the correct value for them is > 65536, not 64.... > > This looks like it was broken back when the test was made generic > - the dd command before this used a bs=1024, so a maz size of 64 > would have been correct. Except the dd command also go changed to > use bs=1, which meant 64 bytes.... > > So, yeah, the test got "broken" for XFS back in 2012 and so the > correct fix here is to change (at least) XFS and btrfs to have a > MAX_ATTRVAL_SIZE=65536.... I'll change XFS over to use a 64K bytes limit. It looks like we'll need a separate special case for Btrfs, as the size limit also takes the attr name length into account: 79 int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode, 80 const char *name, const void *value, size_t size, int flags) 81 { ... 91 if (name_len + size > BTRFS_MAX_XATTR_SIZE(root->fs_info)) 92 return -ENOSPC; BTRFS_MAX_XATTR_SIZE() is also 16K by default on my test system. Given that this is for generic/020 only, my plan is to turn the existing MAX_ATTRVAL_SIZE logic into a helper function. Cheers, David ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] common/attr: reduce MAX_ATTRVAL_SIZE for NFS 2022-04-11 8:57 ` David Disseldorp @ 2022-04-11 22:29 ` Dave Chinner 0 siblings, 0 replies; 4+ messages in thread From: Dave Chinner @ 2022-04-11 22:29 UTC (permalink / raw) To: David Disseldorp; +Cc: fstests On Mon, Apr 11, 2022 at 10:57:52AM +0200, David Disseldorp wrote: > Thanks for the review, Dave... > > On Mon, 11 Apr 2022 15:47:14 +1000, Dave Chinner wrote: > > > > This is due to the MAX_ATTRVAL_SIZE=65536 setting for NFS, which exceeds > > > the Btrfs (and XFS) limit of MAX_ATTRVAL_SIZE=64. Change NFS to use this > > > lower bound value. > > > > I think that what XFS/UDF/BTRFS set here is bogus. > > > > There is *one* test - generic/020 - that uses MAX_ATTRS and > > MAX_ATTRVAL_SIZE, and it uses MAX_ATTRVAL_SIZE as a byte count. > > > > Which means for those 3 filesytems, the correct value for them is > > 65536, not 64.... > > > > This looks like it was broken back when the test was made generic > > - the dd command before this used a bs=1024, so a maz size of 64 > > would have been correct. Except the dd command also go changed to > > use bs=1, which meant 64 bytes.... > > > > So, yeah, the test got "broken" for XFS back in 2012 and so the > > correct fix here is to change (at least) XFS and btrfs to have a > > MAX_ATTRVAL_SIZE=65536.... > > I'll change XFS over to use a 64K bytes limit. It looks like we'll need > a separate special case for Btrfs, as the size limit also takes the attr > name length into account: > > 79 int btrfs_setxattr(struct btrfs_trans_handle *trans, struct inode *inode, > 80 const char *name, const void *value, size_t size, int flags) > 81 { > ... > 91 if (name_len + size > BTRFS_MAX_XATTR_SIZE(root->fs_info)) > 92 return -ENOSPC; > > BTRFS_MAX_XATTR_SIZE() is also 16K by default on my test system. Given > that this is for generic/020 only, my plan is to turn the existing > MAX_ATTRVAL_SIZE logic into a helper function. Sounds good! Can you do the same with MAX_ATTRS, too? It's a g/020 only value, too. Cheers, Dave. -- Dave Chinner david@fromorbit.com ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-04-11 22:29 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2022-04-08 19:19 [PATCH] common/attr: reduce MAX_ATTRVAL_SIZE for NFS David Disseldorp 2022-04-11 5:47 ` Dave Chinner 2022-04-11 8:57 ` David Disseldorp 2022-04-11 22:29 ` Dave Chinner
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox