[cc linux-nfs for review]
On Fri, Jul 30, 2021 at 08:42:52PM +0800, Hao Xu wrote:
> The block size of localfs for nfs may be much smaller than nfs itself.
> So we'd better set MAX_ATTRS and MAX_ATTRVAL_SIZE to 4096 to avoid
> 'no space' error when we test adding a bunch of xattrs to nfs.
>
> Signed-off-by: Hao Xu <[email protected]>
Since the xattr support is relatively new (merged a year ago for
NFSv4.2), I'd like nfs folks to take a look as well.
> ---
>
> It's better to set BLOCK_SIZE to `_get_block_size $variable`
> here $variable is the localfs for nfs, since I'm not familiar with
> xfstests, anyone tell what's the name of it.
fstests doesn't know the exported filesystem under NFS, so I don't think
we could the block size of it.
>
> common/attr | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/common/attr b/common/attr
> index 42ceab92335a..a833f00e0884 100644
> --- a/common/attr
> +++ b/common/attr
> @@ -253,9 +253,13 @@ _getfattr()
>
> # set maximum total attr space based on fs type
> case "$FSTYP" in
> -xfs|udf|pvfs2|9p|ceph|nfs)
> +xfs|udf|pvfs2|9p|ceph)
> MAX_ATTRS=1000
> ;;
> +nfs)
> + BLOCK_SIZE=4096
> + let MAX_ATTRS=$BLOCK_SIZE/40
> + ;;
> *)
> # Assume max ~1 block of attrs
> BLOCK_SIZE=`_get_block_size $TEST_DIR`
> @@ -273,12 +277,15 @@ xfs|udf|btrfs)
> pvfs2)
> MAX_ATTRVAL_SIZE=8192
> ;;
> -9p|ceph|nfs)
> +9p|ceph)
> MAX_ATTRVAL_SIZE=65536
> ;;
> bcachefs)
> MAX_ATTRVAL_SIZE=1024
> ;;
> +nfs)
> + MAX_ATTRVAL_SIZE=3840
> + ;;
Where does this value come from?
Thanks,
Eryu
> *)
> # Assume max ~1 block of attrs
> BLOCK_SIZE=`_get_block_size $TEST_DIR`
> --
> 2.24.4
> On Jul 30, 2021, at 10:01, Eryu Guan <[email protected]> wrote:
>
> [cc linux-nfs for review]
>
> On Fri, Jul 30, 2021 at 08:42:52PM +0800, Hao Xu wrote:
>> The block size of localfs for nfs may be much smaller than nfs itself.
>> So we'd better set MAX_ATTRS and MAX_ATTRVAL_SIZE to 4096 to avoid
>> 'no space' error when we test adding a bunch of xattrs to nfs.
>>
>> Signed-off-by: Hao Xu <[email protected]>
>
> Since the xattr support is relatively new (merged a year ago for
> NFSv4.2), I'd like nfs folks to take a look as well.
>
>> ---
>>
>> It's better to set BLOCK_SIZE to `_get_block_size $variable`
>> here $variable is the localfs for nfs, since I'm not familiar with
>> xfstests, anyone tell what's the name of it.
>
> fstests doesn't know the exported filesystem under NFS, so I don't think
> we could the block size of it.
>
>>
>> common/attr | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/attr b/common/attr
>> index 42ceab92335a..a833f00e0884 100644
>> --- a/common/attr
>> +++ b/common/attr
>> @@ -253,9 +253,13 @@ _getfattr()
>>
>> # set maximum total attr space based on fs type
>> case "$FSTYP" in
>> -xfs|udf|pvfs2|9p|ceph|nfs)
>> +xfs|udf|pvfs2|9p|ceph)
>> MAX_ATTRS=1000
>> ;;
>> +nfs)
>> + BLOCK_SIZE=4096
>> + let MAX_ATTRS=$BLOCK_SIZE/40
>> + ;;
>> *)
>> # Assume max ~1 block of attrs
>> BLOCK_SIZE=`_get_block_size $TEST_DIR`
>> @@ -273,12 +277,15 @@ xfs|udf|btrfs)
>> pvfs2)
>> MAX_ATTRVAL_SIZE=8192
>> ;;
>> -9p|ceph|nfs)
>> +9p|ceph)
>> MAX_ATTRVAL_SIZE=65536
>> ;;
>> bcachefs)
>> MAX_ATTRVAL_SIZE=1024
>> ;;
>> +nfs)
>> + MAX_ATTRVAL_SIZE=3840
>> + ;;
>
> Where does this value come from?
>
> Thanks,
> Eryu
>
>> *)
>> # Assume max ~1 block of attrs
>> BLOCK_SIZE=`_get_block_size $TEST_DIR`
>> --
>> 2.24.4
The above hackery proves beyond a shadow of a doubt that this test is utterly broken. Filesystem block sizes have nothing at all to do with xattrs.
Please move this test into the filesystem-specific categories or else remove it altogether. It definitely does not belong in ‘generic’.
_________________________________
Trond Myklebust
Linux NFS client maintainer, Hammerspace
[email protected]
On Sat, Jul 31, 2021 at 10:07:13PM +0000, Trond Myklebust wrote:
>
>
> > On Jul 30, 2021, at 10:01, Eryu Guan <[email protected]> wrote:
> >
> > [cc linux-nfs for review]
> >
> > On Fri, Jul 30, 2021 at 08:42:52PM +0800, Hao Xu wrote:
> >> The block size of localfs for nfs may be much smaller than nfs itself.
> >> So we'd better set MAX_ATTRS and MAX_ATTRVAL_SIZE to 4096 to avoid
> >> 'no space' error when we test adding a bunch of xattrs to nfs.
> >>
> >> Signed-off-by: Hao Xu <[email protected]>
> >
> > Since the xattr support is relatively new (merged a year ago for
> > NFSv4.2), I'd like nfs folks to take a look as well.
> >
> >> ---
> >>
> >> It's better to set BLOCK_SIZE to `_get_block_size $variable`
> >> here $variable is the localfs for nfs, since I'm not familiar with
> >> xfstests, anyone tell what's the name of it.
> >
> > fstests doesn't know the exported filesystem under NFS, so I don't think
> > we could the block size of it.
> >
> >>
> >> common/attr | 11 +++++++++--
> >> 1 file changed, 9 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/common/attr b/common/attr
> >> index 42ceab92335a..a833f00e0884 100644
> >> --- a/common/attr
> >> +++ b/common/attr
> >> @@ -253,9 +253,13 @@ _getfattr()
> >>
> >> # set maximum total attr space based on fs type
> >> case "$FSTYP" in
> >> -xfs|udf|pvfs2|9p|ceph|nfs)
> >> +xfs|udf|pvfs2|9p|ceph)
> >> MAX_ATTRS=1000
> >> ;;
> >> +nfs)
> >> + BLOCK_SIZE=4096
> >> + let MAX_ATTRS=$BLOCK_SIZE/40
> >> + ;;
> >> *)
> >> # Assume max ~1 block of attrs
> >> BLOCK_SIZE=`_get_block_size $TEST_DIR`
> >> @@ -273,12 +277,15 @@ xfs|udf|btrfs)
> >> pvfs2)
> >> MAX_ATTRVAL_SIZE=8192
> >> ;;
> >> -9p|ceph|nfs)
> >> +9p|ceph)
> >> MAX_ATTRVAL_SIZE=65536
> >> ;;
> >> bcachefs)
> >> MAX_ATTRVAL_SIZE=1024
> >> ;;
> >> +nfs)
> >> + MAX_ATTRVAL_SIZE=3840
> >> + ;;
> >
> > Where does this value come from?
> >
> > Thanks,
> > Eryu
> >
> >> *)
> >> # Assume max ~1 block of attrs
> >> BLOCK_SIZE=`_get_block_size $TEST_DIR`
> >> --
> >> 2.24.4
>
> The above hackery proves beyond a shadow of a doubt that this test is utterly broken. Filesystem block sizes have nothing at all to do with xattrs.
>
> Please move this test into the filesystem-specific categories or else remove it altogether. It definitely does not belong in ‘generic’.
I ran in to this basic problem when trying to add support for NFS xattr tests in fstests.
fstests wants to see if the xattr implementation of filesystems acts as expected when you hit the xattr size limits. But there is no interface to query those limits. So fstests resorts to special knowledge about the filesystem implementation to deduce these limits.
That, however, falls apart for NFS, which has no builtin limits (aside from the max RPC xfer size). The limit for NFS is just whatever the filesystem on the server side has, so there is no one-size-fits-all limit you can set here. What works against a server exporting XFS will not work against a server exporting ext4, etc. And then you might have a server running on a system that implements xattrs as a 'resource fork', so the size could be equal to the maximum filesystem size. You just don't know.
If you're on Linux, you could try to deduce the limit by just trying to set an xattr with increasing size until you hit the limit. That's sort of doable because Linux has an upper limit (64k) enforced by the fs-independent code, so you don't have to go beyond that. But, you're trying to see if things behave correctly in the first place when hitting the limit, so that's kind of a chicken-and-egg problem.
It's messy, there is no clean solution here.
- Frank
在 2021/8/2 下午11:55, Frank van der Linden 写道:
> On Sat, Jul 31, 2021 at 10:07:13PM +0000, Trond Myklebust wrote:
>>
>>
>>> On Jul 30, 2021, at 10:01, Eryu Guan <[email protected]> wrote:
>>>
>>> [cc linux-nfs for review]
>>>
>>> On Fri, Jul 30, 2021 at 08:42:52PM +0800, Hao Xu wrote:
>>>> The block size of localfs for nfs may be much smaller than nfs itself.
>>>> So we'd better set MAX_ATTRS and MAX_ATTRVAL_SIZE to 4096 to avoid
>>>> 'no space' error when we test adding a bunch of xattrs to nfs.
>>>>
>>>> Signed-off-by: Hao Xu <[email protected]>
>>>
>>> Since the xattr support is relatively new (merged a year ago for
>>> NFSv4.2), I'd like nfs folks to take a look as well.
>>>
>>>> ---
>>>>
>>>> It's better to set BLOCK_SIZE to `_get_block_size $variable`
>>>> here $variable is the localfs for nfs, since I'm not familiar with
>>>> xfstests, anyone tell what's the name of it.
>>>
>>> fstests doesn't know the exported filesystem under NFS, so I don't think
>>> we could the block size of it.
>>>
>>>>
>>>> common/attr | 11 +++++++++--
>>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/common/attr b/common/attr
>>>> index 42ceab92335a..a833f00e0884 100644
>>>> --- a/common/attr
>>>> +++ b/common/attr
>>>> @@ -253,9 +253,13 @@ _getfattr()
>>>>
>>>> # set maximum total attr space based on fs type
>>>> case "$FSTYP" in
>>>> -xfs|udf|pvfs2|9p|ceph|nfs)
>>>> +xfs|udf|pvfs2|9p|ceph)
>>>> MAX_ATTRS=1000
>>>> ;;
>>>> +nfs)
>>>> + BLOCK_SIZE=4096
>>>> + let MAX_ATTRS=$BLOCK_SIZE/40
>>>> + ;;
>>>> *)
>>>> # Assume max ~1 block of attrs
>>>> BLOCK_SIZE=`_get_block_size $TEST_DIR`
>>>> @@ -273,12 +277,15 @@ xfs|udf|btrfs)
>>>> pvfs2)
>>>> MAX_ATTRVAL_SIZE=8192
>>>> ;;
>>>> -9p|ceph|nfs)
>>>> +9p|ceph)
>>>> MAX_ATTRVAL_SIZE=65536
>>>> ;;
>>>> bcachefs)
>>>> MAX_ATTRVAL_SIZE=1024
>>>> ;;
>>>> +nfs)
>>>> + MAX_ATTRVAL_SIZE=3840
>>>> + ;;
>>>
>>> Where does this value come from?
>>>
>>> Thanks,
>>> Eryu
>>>
>>>> *)
>>>> # Assume max ~1 block of attrs
>>>> BLOCK_SIZE=`_get_block_size $TEST_DIR`
>>>> --
>>>> 2.24.4
>>
>> The above hackery proves beyond a shadow of a doubt that this test is utterly broken. Filesystem block sizes have nothing at all to do with xattrs.
>>
>> Please move this test into the filesystem-specific categories or else remove it altogether. It definitely does not belong in ‘generic’.
>
> I ran in to this basic problem when trying to add support for NFS xattr tests in fstests.
>
> fstests wants to see if the xattr implementation of filesystems acts as expected when you hit the xattr size limits. But there is no interface to query those limits. So fstests resorts to special knowledge about the filesystem implementation to deduce these limits.
>
> That, however, falls apart for NFS, which has no builtin limits (aside from the max RPC xfer size). The limit for NFS is just whatever the filesystem on the server side has, so there is no one-size-fits-all limit you can set here. What works against a server exporting XFS will not work against a server exporting ext4, etc. And then you might have a server running on a system that implements xattrs as a 'resource fork', so the size could be equal to the maximum filesystem size. You just don't know.
So I now believe that doing this test for NFS is kind of pointless. If a
user want to test xattrs for NFS, the right way is to test the fs on the
server side directly.
>
> If you're on Linux, you could try to deduce the limit by just trying to set an xattr with increasing size until you hit the limit. That's sort of doable because Linux has an upper limit (64k) enforced by the fs-independent code, so you don't have to go beyond that. But, you're trying to see if things behave correctly in the first place when hitting the limit, so that's kind of a chicken-and-egg problem.
>
> It's messy, there is no clean solution here.
>
> - Frank
>
在 2021/7/30 下午10:01, Eryu Guan 写道:
> [cc linux-nfs for review]
>
> On Fri, Jul 30, 2021 at 08:42:52PM +0800, Hao Xu wrote:
>> The block size of localfs for nfs may be much smaller than nfs itself.
>> So we'd better set MAX_ATTRS and MAX_ATTRVAL_SIZE to 4096 to avoid
>> 'no space' error when we test adding a bunch of xattrs to nfs.
>>
>> Signed-off-by: Hao Xu <[email protected]>
>
> Since the xattr support is relatively new (merged a year ago for
> NFSv4.2), I'd like nfs folks to take a look as well.
>
>> ---
>>
>> It's better to set BLOCK_SIZE to `_get_block_size $variable`
>> here $variable is the localfs for nfs, since I'm not familiar with
>> xfstests, anyone tell what's the name of it.
>
> fstests doesn't know the exported filesystem under NFS, so I don't think
> we could the block size of it.
>
>>
>> common/attr | 11 +++++++++--
>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/common/attr b/common/attr
>> index 42ceab92335a..a833f00e0884 100644
>> --- a/common/attr
>> +++ b/common/attr
>> @@ -253,9 +253,13 @@ _getfattr()
>>
>> # set maximum total attr space based on fs type
>> case "$FSTYP" in
>> -xfs|udf|pvfs2|9p|ceph|nfs)
>> +xfs|udf|pvfs2|9p|ceph)
>> MAX_ATTRS=1000
>> ;;
>> +nfs)
>> + BLOCK_SIZE=4096
>> + let MAX_ATTRS=$BLOCK_SIZE/40
>> + ;;
>> *)
>> # Assume max ~1 block of attrs
>> BLOCK_SIZE=`_get_block_size $TEST_DIR`
>> @@ -273,12 +277,15 @@ xfs|udf|btrfs)
>> pvfs2)
>> MAX_ATTRVAL_SIZE=8192
>> ;;
>> -9p|ceph|nfs)
>> +9p|ceph)
>> MAX_ATTRVAL_SIZE=65536
>> ;;
>> bcachefs)
>> MAX_ATTRVAL_SIZE=1024
>> ;;
>> +nfs)
>> + MAX_ATTRVAL_SIZE=3840
>> + ;;
>
> Where does this value come from?
>
3840 = 4096 - 256, which is like the case of *)
> Thanks,
> Eryu
>
>> *)
>> # Assume max ~1 block of attrs
>> BLOCK_SIZE=`_get_block_size $TEST_DIR`
>> --
>> 2.24.4