| Message ID | 20220408191943.27655-1-ddiss@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | common/attr: reduce MAX_ATTRVAL_SIZE for NFS | expand |
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.
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
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.
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)
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(-)