linux-kernel - Re: [PATCH] nfs: set block size according to pnfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 17 Jun 2021 10:39:15 +0800
From:   Gao Xiang <hsiangkao@...ux.alibaba.com>
To:     Theodore Ts'o <tytso@....edu>
Cc:     Trond Myklebust <trondmy@...merspace.com>,
        "linux-nfs@...r.kernel.org" <linux-nfs@...r.kernel.org>,
        "joseph.qi@...ux.alibaba.com" <joseph.qi@...ux.alibaba.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "anna.schumaker@...app.com" <anna.schumaker@...app.com>
Subject: Re: [PATCH] nfs: set block size according to pnfs_blksize first

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