Re: [PATCH] nfs: set block size according to pnfs_blksize first

[Gao Xiang]

On Wed, Jun 16, 2021 at 06:55:31PM -0400, Theodore Ts'o wrote:
> On Thu, Jun 17, 2021 at 01:51:04AM +0800, Gao Xiang wrote:
> > 
> > Considering the original XFS regression report [1], I think
> > underlayfs blksize may still be needed. And binary search to get the
> > maximum attr value may be another new case for this as well. Or
> > alternatively just add by block size to do a trip test.
> > 
> > Although I have no idea if we can just skip the case when testing with
> > NFS. If getting underlayfs blksize is unfeasible, I think we might
> > skip such case for now since nfs blksize is not useful for generic/486.
> > 
> > [1] https://bugzilla.kernel.org/show_bug.cgi?id=199119
> 
> I've looked at the original XFS regression size, and I don't see why
> using the underlaying blocksize matters at all.  This is especially
> true if you look at the comment in the test, and the commit which
> fixed the bug.  All that is needed for the xfs regression test is to
> start with a small xattr, and replace it with a large xattr.  The
> blocksize is really irrelevant.

What I said was the original testcase strictly addressing the original
regression report, which converts from shortform to single-block
leaf format, see:

https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git/tree/src/attr_replace_test.c#n40

>	/*
>	 * The value should be 3/4 the size of a fs block to ensure that we
>	 * get to extents format.
>	 */
>	ret = fstat(fd, &sbuf);
>	if (ret < 0) die();
>	size = sbuf.st_blksize * 3 / 4;

and

https://lore.kernel.org/fstests/20180425054826.GB1661@magnolia/

> > And I found another problem in the test, it fails on 1k/2k block size
> > extN filesystems, because 2k xattr doesn't fit in single block.. e.g.
> > 
> >     -Attribute "world" has a 2048 byte value for SCRATCH_MNT/hello
> >     +No space left on device
> >     +error=22 at line 46
> >     +Attribute "world" has a 1 byte value for
> > 
> > We probably need to check the block size of SCRATCH_DEV and _notrun if
> > it's smaller than 4k.
> >
> > Or require ea_inode feature when block size < 4k? Note that this test
> > does fail on ext4 with ea_inode feature enabled (so add ext4 list to
> > cc). e.g.

> I was about to say "Eh, this is a regression test for XFS so that's
> probably fine, but then...

Of course, the testcase itself may have room to improve, I'll look at
it when I have extra time.

Thanks,
Gao Xiang

> 
> 						- Ted