| Message ID | 165644768327.1045534.10420155448662856970.stgit@magnolia (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fstests: random fixes | expand |
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
Hi, guys.
Recently I hit a regression during test xfstest. Reverting this commit
could fix that error.
Reproduce steps (only nfs4.2 has this error):
```
# cat local.config
export TEST_DEV=127.0.0.1:/home/nfs/share0
export TEST_DIR=/mnt/test
export SCRATCH_DEV=127.0.0.1:/home/nfs/share1
export SCRATCH_MNT=/mnt/scratch
export FSX_AVOID="-E"
export NFS_MOUNT_OPTIONS="-o rw,relatime,vers=4.2"
# ./check -nfs generic/285
FSTYP -- nfs
PLATFORM -- Linux/aarch64 hpe-apollo80-01-n00
5.14.0-131.el9.aarch64 #1 SMP PREEMPT_DYNAMIC Mon Jul 18 16:13:44 EDT 2022
MKFS_OPTIONS -- 127.0.0.1:/home/nfs/share1
MOUNT_OPTIONS -- -o rw,relatime,vers=4.2 -o
context=system_u:object_r:root_t:s0 127.0.0.1:/home/nfs/share1 /mnt/scratch
generic/285 2s ... [failed, exit status 1]- output mismatch (see
/root/xfstests/results//generic/285.out.bad)
--- tests/generic/285.out 2022-07-27 21:07:43.160268552 -0400
+++ /root/xfstests/results//generic/285.out.bad 2022-07-27
21:31:27.887090532 -0400
@@ -1 +1,3 @@
QA output created by 285
+seek sanity check failed!
+(see /root/xfstests/results//generic/285.full for details)
...
(Run 'diff -u /root/xfstests/tests/generic/285.out
/root/xfstests/results//generic/285.out.bad' to see the entire diff)
Ran: generic/285
Failures: generic/285
Failed 1 of 1 tests
```
Reverting this commit then test pass.
```
# git revert e861a30255c9780425ee5193325d30882fbe7410
# make -j && make install -j
---snip---
# ./check -nfs generic/285
FSTYP -- nfs
PLATFORM -- Linux/aarch64 hpe-apollo80-01-n00
5.14.0-131.el9.aarch64 #1 SMP PREEMPT_DYNAMIC Mon Jul 18 16:13:44 EDT 2022
MKFS_OPTIONS -- 127.0.0.1:/home/nfs/share1
MOUNT_OPTIONS -- -o rw,relatime,vers=4.2 -o
context=system_u:object_r:root_t:s0 127.0.0.1:/home/nfs/share1 /mnt/scratch
generic/285 1s ... 1s
Ran: generic/285
Passed all 1 tests
```
On 6/29/22 04:21, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
>
> The seek sanity test tries to figure out a file space allocation unit by
> calling stat and then using an iterative SEEK_DATA method to try to
> detect a smaller blocksize based on SEEK_DATA's consultation of the
> filesystem's internal block mapping. This was put in (AFAICT) because
> XFS' stat implementation returns max(filesystem blocksize, PAGESIZE) for
> most regular files.
>
> Unfortunately, for a realtime file with an extent size larger than a
> single filesystem block this doesn't work at all because block mappings
> still work at filesystem block granularity, but allocation units do not.
> To fix this, detect the specific case where st_blksize != PAGE_SIZE and
> trust the fstat results.
>
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
> src/seek_sanity_test.c | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
>
> diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c
> index 76587b7f..1030d0c5 100644
> --- a/src/seek_sanity_test.c
> +++ b/src/seek_sanity_test.c
> @@ -45,6 +45,7 @@ static int get_io_sizes(int fd)
> off_t pos = 0, offset = 1;
> struct stat buf;
> int shift, ret;
> + int pagesz = sysconf(_SC_PAGE_SIZE);
>
> ret = fstat(fd, &buf);
> if (ret) {
> @@ -53,8 +54,16 @@ static int get_io_sizes(int fd)
> return ret;
> }
>
> - /* st_blksize is typically also the allocation size */
> + /*
> + * st_blksize is typically also the allocation size. However, XFS
> + * rounds this up to the page size, so if the stat blocksize is exactly
> + * one page, use this iterative algorithm to see if SEEK_DATA will hint
> + * at a more precise answer based on the filesystem's (pre)allocation
> + * decisions.
> + */
> alloc_size = buf.st_blksize;
> + if (alloc_size != pagesz)
> + goto done;
>
> /* try to discover the actual alloc size */
> while (pos == 0 && offset < alloc_size) {
> @@ -80,6 +89,7 @@ static int get_io_sizes(int fd)
> if (!shift)
> offset += pos ? 0 : 1;
> alloc_size = offset;
> +done:
> fprintf(stdout, "Allocation size: %ld\n", alloc_size);
> return 0;
>
>
On Thu, Jul 28, 2022 at 01:37:08AM +0000, liuyd.fnst@fujitsu.com wrote: > Hi, guys. > > Recently I hit a regression during test xfstest. Reverting this commit > could fix that error. > > Reproduce steps (only nfs4.2 has this error): > ``` > # cat local.config > > export TEST_DEV=127.0.0.1:/home/nfs/share0 > > export TEST_DIR=/mnt/test > > export SCRATCH_DEV=127.0.0.1:/home/nfs/share1 ahahaaaa, NFS. I forgot that it sets st_blksize == 1048576, which will trip this up. Let me work on a fix to seek_sanity_test.c to make this work for xfs realtime without breaking NFS. I /think/ the solution is to replace the "if (...) goto done;" bit with something that queries FS_IOC_GETXATTR and XFS_IOC_FSGEOMETRY to figure out if the file being tested is a realtime file on an XFS filesystem, and set alloc_size to the rt extent size. /* Compute the file allocation unit size for an XFS file. */ static int detect_xfs_alloc_unit(int fd) { struct fsxattr fsx; struct xfs_fsop_geom fsgeom; int ret; ret = ioctl(fd, XFS_IOC_FSGEOMETRY, &fsgeom); if (ret) return -1; ret = ioctl(fd, FS_IOC_FSGETXATTR, &fsx); if (ret) return -1; alloc_size = fsgeom.blocksize; if (fsx.fsx_xflags & FS_XFLAG_REALTIME) alloc_size *= fsgeom.rtextsize; return 0; } should suffice to fix these testcase on xfs realtime without messing up nfs. --D > export SCRATCH_MNT=/mnt/scratch > > export FSX_AVOID="-E" > > export NFS_MOUNT_OPTIONS="-o rw,relatime,vers=4.2" > > > # ./check -nfs generic/285 > > FSTYP -- nfs > > PLATFORM -- Linux/aarch64 hpe-apollo80-01-n00 > 5.14.0-131.el9.aarch64 #1 SMP PREEMPT_DYNAMIC Mon Jul 18 16:13:44 EDT 2022 > > MKFS_OPTIONS -- 127.0.0.1:/home/nfs/share1 > > MOUNT_OPTIONS -- -o rw,relatime,vers=4.2 -o > context=system_u:object_r:root_t:s0 127.0.0.1:/home/nfs/share1 /mnt/scratch > > > > generic/285 2s ... [failed, exit status 1]- output mismatch (see > /root/xfstests/results//generic/285.out.bad) > > --- tests/generic/285.out 2022-07-27 21:07:43.160268552 -0400 > > +++ /root/xfstests/results//generic/285.out.bad 2022-07-27 > 21:31:27.887090532 -0400 > > @@ -1 +1,3 @@ > > QA output created by 285 > > +seek sanity check failed! > > +(see /root/xfstests/results//generic/285.full for details) > > ... > > (Run 'diff -u /root/xfstests/tests/generic/285.out > /root/xfstests/results//generic/285.out.bad' to see the entire diff) > > Ran: generic/285 > > Failures: generic/285 > > Failed 1 of 1 tests > > > ``` > > Reverting this commit then test pass. > ``` > # git revert e861a30255c9780425ee5193325d30882fbe7410 > # make -j && make install -j > ---snip--- > # ./check -nfs generic/285 > > FSTYP -- nfs > > PLATFORM -- Linux/aarch64 hpe-apollo80-01-n00 > 5.14.0-131.el9.aarch64 #1 SMP PREEMPT_DYNAMIC Mon Jul 18 16:13:44 EDT 2022 > > MKFS_OPTIONS -- 127.0.0.1:/home/nfs/share1 > > MOUNT_OPTIONS -- -o rw,relatime,vers=4.2 -o > context=system_u:object_r:root_t:s0 127.0.0.1:/home/nfs/share1 /mnt/scratch > > > > generic/285 1s ... 1s > > Ran: generic/285 > > Passed all 1 tests > > ``` > > On 6/29/22 04:21, Darrick J. Wong wrote: > > From: Darrick J. Wong <djwong@kernel.org> > > > > The seek sanity test tries to figure out a file space allocation unit by > > calling stat and then using an iterative SEEK_DATA method to try to > > detect a smaller blocksize based on SEEK_DATA's consultation of the > > filesystem's internal block mapping. This was put in (AFAICT) because > > XFS' stat implementation returns max(filesystem blocksize, PAGESIZE) for > > most regular files. > > > > Unfortunately, for a realtime file with an extent size larger than a > > single filesystem block this doesn't work at all because block mappings > > still work at filesystem block granularity, but allocation units do not. > > To fix this, detect the specific case where st_blksize != PAGE_SIZE and > > trust the fstat results. > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > src/seek_sanity_test.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > > > diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c > > index 76587b7f..1030d0c5 100644 > > --- a/src/seek_sanity_test.c > > +++ b/src/seek_sanity_test.c > > @@ -45,6 +45,7 @@ static int get_io_sizes(int fd) > > off_t pos = 0, offset = 1; > > struct stat buf; > > int shift, ret; > > + int pagesz = sysconf(_SC_PAGE_SIZE); > > > > ret = fstat(fd, &buf); > > if (ret) { > > @@ -53,8 +54,16 @@ static int get_io_sizes(int fd) > > return ret; > > } > > > > - /* st_blksize is typically also the allocation size */ > > + /* > > + * st_blksize is typically also the allocation size. However, XFS > > + * rounds this up to the page size, so if the stat blocksize is exactly > > + * one page, use this iterative algorithm to see if SEEK_DATA will hint > > + * at a more precise answer based on the filesystem's (pre)allocation > > + * decisions. > > + */ > > alloc_size = buf.st_blksize; > > + if (alloc_size != pagesz) > > + goto done; > > > > /* try to discover the actual alloc size */ > > while (pos == 0 && offset < alloc_size) { > > @@ -80,6 +89,7 @@ static int get_io_sizes(int fd) > > if (!shift) > > offset += pos ? 0 : 1; > > alloc_size = offset; > > +done: > > fprintf(stdout, "Allocation size: %ld\n", alloc_size); > > return 0; > > > >
Does this patch fix NFS for you? --D --- From: Darrick J. Wong <djwong@kernel.org> seek_sanity_test: use XFS ioctls to determine file allocation unit size liuyd.fnst@fujitsu.com reported that my recent change to the seek sanity test broke NFS. I foolishly thought that st_blksize was sufficient to find the file allocation unit size so that applications could figure out the SEEK_HOLE granularity. Replace that with an explicit callout to XFS ioctls so that xfs realtime will work again. Fixes: e861a302 ("seek_sanity_test: fix allocation unit detection on XFS realtime") Reported-by: liuyd.fnst@fujitsu.com Signed-off-by: Darrick J. Wong <djwong@kernel.org> --- src/Makefile | 4 ++++ src/seek_sanity_test.c | 40 +++++++++++++++++++++++++++++++--------- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/src/Makefile b/src/Makefile index 38628a22..b89a7a5e 100644 --- a/src/Makefile +++ b/src/Makefile @@ -82,6 +82,10 @@ ifeq ($(HAVE_LIBCAP), true) LLDLIBS += -lcap endif +ifeq ($(HAVE_FSXATTR_XFLAG_HASATTR), yes) +LCFLAGS += -DHAVE_FSXATTR_XFLAG_HASATTR +endif + ifeq ($(HAVE_SEEK_DATA), yes) ifeq ($(HAVE_FSXATTR_XFLAG_HASATTR), yes) ifeq ($(HAVE_NFTW), yes) diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c index 1030d0c5..b53f4862 100644 --- a/src/seek_sanity_test.c +++ b/src/seek_sanity_test.c @@ -40,6 +40,32 @@ static void get_file_system(int fd) } } +#ifdef HAVE_FSXATTR_XFLAG_HASATTR +/* Compute the file allocation unit size for an XFS file. */ +static int detect_xfs_alloc_unit(int fd) +{ + struct fsxattr fsx; + struct xfs_fsop_geom fsgeom; + int ret; + + ret = ioctl(fd, XFS_IOC_FSGEOMETRY, &fsgeom); + if (ret) + return -1; + + ret = ioctl(fd, FS_IOC_FSGETXATTR, &fsx); + if (ret) + return -1; + + alloc_size = fsgeom.blocksize; + if (fsx.fsx_xflags & FS_XFLAG_REALTIME) + alloc_size *= fsgeom.rtextsize; + + return 0; +} +#else +# define detect_xfs_alloc_unit(fd) (-1) +#endif + static int get_io_sizes(int fd) { off_t pos = 0, offset = 1; @@ -47,6 +73,10 @@ static int get_io_sizes(int fd) int shift, ret; int pagesz = sysconf(_SC_PAGE_SIZE); + ret = detect_xfs_alloc_unit(fd); + if (!ret) + goto done; + ret = fstat(fd, &buf); if (ret) { fprintf(stderr, " ERROR %d: Failed to find io blocksize\n", @@ -54,16 +84,8 @@ static int get_io_sizes(int fd) return ret; } - /* - * st_blksize is typically also the allocation size. However, XFS - * rounds this up to the page size, so if the stat blocksize is exactly - * one page, use this iterative algorithm to see if SEEK_DATA will hint - * at a more precise answer based on the filesystem's (pre)allocation - * decisions. - */ + /* st_blksize is typically also the allocation size */ alloc_size = buf.st_blksize; - if (alloc_size != pagesz) - goto done; /* try to discover the actual alloc size */ while (pos == 0 && offset < alloc_size) {
Hi, Darrick. On 7/28/22 10:37, Darrick J. Wong wrote: > Does this patch fix NFS for you? It works for me. Thanks. BTW. It has conflict during "git am". Looks like your branch are ahead of master. > > --D > --- > From: Darrick J. Wong <djwong@kernel.org> > > seek_sanity_test: use XFS ioctls to determine file allocation unit size > > liuyd.fnst@fujitsu.com reported that my recent change to the seek sanity > test broke NFS. I foolishly thought that st_blksize was sufficient to > find the file allocation unit size so that applications could figure out > the SEEK_HOLE granularity. Replace that with an explicit callout to XFS > ioctls so that xfs realtime will work again. > > Fixes: e861a302 ("seek_sanity_test: fix allocation unit detection on XFS realtime") > Reported-by: liuyd.fnst@fujitsu.com > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > --- > src/Makefile | 4 ++++ > src/seek_sanity_test.c | 40 +++++++++++++++++++++++++++++++--------- > 2 files changed, 35 insertions(+), 9 deletions(-) > > diff --git a/src/Makefile b/src/Makefile > index 38628a22..b89a7a5e 100644 > --- a/src/Makefile > +++ b/src/Makefile > @@ -82,6 +82,10 @@ ifeq ($(HAVE_LIBCAP), true) > LLDLIBS += -lcap > endif > > +ifeq ($(HAVE_FSXATTR_XFLAG_HASATTR), yes) > +LCFLAGS += -DHAVE_FSXATTR_XFLAG_HASATTR > +endif > + > ifeq ($(HAVE_SEEK_DATA), yes) > ifeq ($(HAVE_FSXATTR_XFLAG_HASATTR), yes) > ifeq ($(HAVE_NFTW), yes) > diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c > index 1030d0c5..b53f4862 100644 > --- a/src/seek_sanity_test.c > +++ b/src/seek_sanity_test.c > @@ -40,6 +40,32 @@ static void get_file_system(int fd) > } > } > > +#ifdef HAVE_FSXATTR_XFLAG_HASATTR > +/* Compute the file allocation unit size for an XFS file. */ > +static int detect_xfs_alloc_unit(int fd) > +{ > + struct fsxattr fsx; > + struct xfs_fsop_geom fsgeom; > + int ret; > + > + ret = ioctl(fd, XFS_IOC_FSGEOMETRY, &fsgeom); > + if (ret) > + return -1; > + > + ret = ioctl(fd, FS_IOC_FSGETXATTR, &fsx); > + if (ret) > + return -1; > + > + alloc_size = fsgeom.blocksize; > + if (fsx.fsx_xflags & FS_XFLAG_REALTIME) > + alloc_size *= fsgeom.rtextsize; > + > + return 0; > +} > +#else > +# define detect_xfs_alloc_unit(fd) (-1) > +#endif > + > static int get_io_sizes(int fd) > { > off_t pos = 0, offset = 1; > @@ -47,6 +73,10 @@ static int get_io_sizes(int fd) > int shift, ret; > int pagesz = sysconf(_SC_PAGE_SIZE); > > + ret = detect_xfs_alloc_unit(fd); > + if (!ret) > + goto done; > + > ret = fstat(fd, &buf); > if (ret) { > fprintf(stderr, " ERROR %d: Failed to find io blocksize\n", > @@ -54,16 +84,8 @@ static int get_io_sizes(int fd) > return ret; > } > > - /* > - * st_blksize is typically also the allocation size. However, XFS > - * rounds this up to the page size, so if the stat blocksize is exactly > - * one page, use this iterative algorithm to see if SEEK_DATA will hint > - * at a more precise answer based on the filesystem's (pre)allocation > - * decisions. > - */ > + /* st_blksize is typically also the allocation size */ > alloc_size = buf.st_blksize; > - if (alloc_size != pagesz) > - goto done; > > /* try to discover the actual alloc size */ > while (pos == 0 && offset < alloc_size) {
On Thu, Jul 28, 2022 at 03:38:38AM +0000, liuyd.fnst@fujitsu.com wrote: > Hi, Darrick. > > > On 7/28/22 10:37, Darrick J. Wong wrote: > > Does this patch fix NFS for you? > > It works for me. Thanks. > BTW. It has conflict during "git am". Looks like your branch are ahead > of master. Yes, I base my development branch off of for-next nowadays. Thanks for the report, I'll add a tested-by if that's ok and send it out as a bug fix. --D > > > > --D > > --- > > From: Darrick J. Wong <djwong@kernel.org> > > > > seek_sanity_test: use XFS ioctls to determine file allocation unit size > > > > liuyd.fnst@fujitsu.com reported that my recent change to the seek sanity > > test broke NFS. I foolishly thought that st_blksize was sufficient to > > find the file allocation unit size so that applications could figure out > > the SEEK_HOLE granularity. Replace that with an explicit callout to XFS > > ioctls so that xfs realtime will work again. > > > > Fixes: e861a302 ("seek_sanity_test: fix allocation unit detection on XFS realtime") > > Reported-by: liuyd.fnst@fujitsu.com > > Signed-off-by: Darrick J. Wong <djwong@kernel.org> > > --- > > src/Makefile | 4 ++++ > > src/seek_sanity_test.c | 40 +++++++++++++++++++++++++++++++--------- > > 2 files changed, 35 insertions(+), 9 deletions(-) > > > > diff --git a/src/Makefile b/src/Makefile > > index 38628a22..b89a7a5e 100644 > > --- a/src/Makefile > > +++ b/src/Makefile > > @@ -82,6 +82,10 @@ ifeq ($(HAVE_LIBCAP), true) > > LLDLIBS += -lcap > > endif > > > > +ifeq ($(HAVE_FSXATTR_XFLAG_HASATTR), yes) > > +LCFLAGS += -DHAVE_FSXATTR_XFLAG_HASATTR > > +endif > > + > > ifeq ($(HAVE_SEEK_DATA), yes) > > ifeq ($(HAVE_FSXATTR_XFLAG_HASATTR), yes) > > ifeq ($(HAVE_NFTW), yes) > > diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c > > index 1030d0c5..b53f4862 100644 > > --- a/src/seek_sanity_test.c > > +++ b/src/seek_sanity_test.c > > @@ -40,6 +40,32 @@ static void get_file_system(int fd) > > } > > } > > > > +#ifdef HAVE_FSXATTR_XFLAG_HASATTR > > +/* Compute the file allocation unit size for an XFS file. */ > > +static int detect_xfs_alloc_unit(int fd) > > +{ > > + struct fsxattr fsx; > > + struct xfs_fsop_geom fsgeom; > > + int ret; > > + > > + ret = ioctl(fd, XFS_IOC_FSGEOMETRY, &fsgeom); > > + if (ret) > > + return -1; > > + > > + ret = ioctl(fd, FS_IOC_FSGETXATTR, &fsx); > > + if (ret) > > + return -1; > > + > > + alloc_size = fsgeom.blocksize; > > + if (fsx.fsx_xflags & FS_XFLAG_REALTIME) > > + alloc_size *= fsgeom.rtextsize; > > + > > + return 0; > > +} > > +#else > > +# define detect_xfs_alloc_unit(fd) (-1) > > +#endif > > + > > static int get_io_sizes(int fd) > > { > > off_t pos = 0, offset = 1; > > @@ -47,6 +73,10 @@ static int get_io_sizes(int fd) > > int shift, ret; > > int pagesz = sysconf(_SC_PAGE_SIZE); > > > > + ret = detect_xfs_alloc_unit(fd); > > + if (!ret) > > + goto done; > > + > > ret = fstat(fd, &buf); > > if (ret) { > > fprintf(stderr, " ERROR %d: Failed to find io blocksize\n", > > @@ -54,16 +84,8 @@ static int get_io_sizes(int fd) > > return ret; > > } > > > > - /* > > - * st_blksize is typically also the allocation size. However, XFS > > - * rounds this up to the page size, so if the stat blocksize is exactly > > - * one page, use this iterative algorithm to see if SEEK_DATA will hint > > - * at a more precise answer based on the filesystem's (pre)allocation > > - * decisions. > > - */ > > + /* st_blksize is typically also the allocation size */ > > alloc_size = buf.st_blksize; > > - if (alloc_size != pagesz) > > - goto done; > > > > /* try to discover the actual alloc size */ > > while (pos == 0 && offset < alloc_size) {
diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c index 76587b7f..1030d0c5 100644 --- a/src/seek_sanity_test.c +++ b/src/seek_sanity_test.c @@ -45,6 +45,7 @@ static int get_io_sizes(int fd) off_t pos = 0, offset = 1; struct stat buf; int shift, ret; + int pagesz = sysconf(_SC_PAGE_SIZE); ret = fstat(fd, &buf); if (ret) { @@ -53,8 +54,16 @@ static int get_io_sizes(int fd) return ret; } - /* st_blksize is typically also the allocation size */ + /* + * st_blksize is typically also the allocation size. However, XFS + * rounds this up to the page size, so if the stat blocksize is exactly + * one page, use this iterative algorithm to see if SEEK_DATA will hint + * at a more precise answer based on the filesystem's (pre)allocation + * decisions. + */ alloc_size = buf.st_blksize; + if (alloc_size != pagesz) + goto done; /* try to discover the actual alloc size */ while (pos == 0 && offset < alloc_size) { @@ -80,6 +89,7 @@ static int get_io_sizes(int fd) if (!shift) offset += pos ? 0 : 1; alloc_size = offset; +done: fprintf(stdout, "Allocation size: %ld\n", alloc_size); return 0;