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

["Luís Henriques" <lhenriques@suse.de>]

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