[PATCHSET v2 0/3] fstests: random fixes for v2022.07.24

[PATCHSET v2 0/3] fstests: random fixes for v2022.07.24

[Darrick J. Wong]

Hi all,

Here's the usual batch of odd fixes for fstests.

v2: rework xfs/432 changes per reviewer comments, bring xfs/291 in line
    with that, and fix allocation unit size detection for the seek
    sanity test

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes
---
 src/seek_sanity_test.c |   36 +++++++++++++++++++++++++++---------
 tests/xfs/291          |    6 +-----
 tests/xfs/432          |    3 ++-
 3 files changed, 30 insertions(+), 15 deletions(-)

[PATCH 1/3] xfs/432: fix this test when external devices are in use

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

This program exercises metadump and mdrestore being run against the
scratch device.  Therefore, the test must pass external log / rt device
arguments to xfs_repair -n to check the "restored" filesystem.  Fix the
incorrect usage, and report repair failures, since this test has been
silently failing for a while now.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/432 |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/tests/xfs/432 b/tests/xfs/432
index 86012f0b..e1e610d0 100755
--- a/tests/xfs/432
+++ b/tests/xfs/432
@@ -89,7 +89,8 @@ _scratch_xfs_metadump $metadump_file -w
 xfs_mdrestore $metadump_file $metadump_img
 
 echo "Check restored metadump image"
-$XFS_REPAIR_PROG -n $metadump_img >> $seqres.full 2>&1
+SCRATCH_DEV=$metadump_img _scratch_xfs_repair -n &>> $seqres.full || \
+	echo "xfs_repair on restored fs returned $?"
 
 # success, all done
 status=0

Re: [PATCH 1/3] xfs/432: fix this test when external devices are in use

[Christoph Hellwig]

On Thu, Jul 28, 2022 at 11:17:15AM -0700, Darrick J. Wong wrote:
> +SCRATCH_DEV=$metadump_img _scratch_xfs_repair -n &>> $seqres.full || \
> +	echo "xfs_repair on restored fs returned $?"

Wouldn;t it make more sense to have a version of _scratch_xfs_repair
rather than doing a somewhat unexpected override of this global
variable?

Re: [PATCH 1/3] xfs/432: fix this test when external devices are in use

[Zorro Lang]

On Thu, Jul 28, 2022 at 01:16:28PM -0700, Christoph Hellwig wrote:
> On Thu, Jul 28, 2022 at 11:17:15AM -0700, Darrick J. Wong wrote:
> > +SCRATCH_DEV=$metadump_img _scratch_xfs_repair -n &>> $seqres.full || \
> > +	echo "xfs_repair on restored fs returned $?"
> 
> Wouldn;t it make more sense to have a version of _scratch_xfs_repair
> rather than doing a somewhat unexpected override of this global
> variable?

Any detailed ideas about how to have a new version of _scratch_xfs_repair? I'll
keep this patch unmerged this week, before Darrick reply your discussion.

Currently a few cases do some overriding [1] before calling _scratch_* helpers.
And good news is this kind of "override" affect only the environment seen by
its follow command/function. So I generally don't have objection if it works
well. But it's welcome if we have a better idea to deal with this kind of
requirement :)

Thanks,
Zorro


[1]
$ grep -rsn SCRATCH_DEV= tests/
tests/btrfs/160:36:old_SCRATCH_DEV=$SCRATCH_DEV
tests/btrfs/160:38:SCRATCH_DEV=$DMERROR_DEV
tests/btrfs/146:39:old_SCRATCH_DEV=$SCRATCH_DEV
tests/btrfs/146:41:SCRATCH_DEV=$DMERROR_DEV
tests/btrfs/146:62:SCRATCH_DEV=$old_SCRATCH_DEV
tests/xfs/507:198:LARGE_SCRATCH_DEV=yes _check_xfs_filesystem $loop_dev none none
tests/xfs/185:75:SCRATCH_DEV="$ddloop"
tests/xfs/234:56:SCRATCH_DEV=$TEST_DIR/image _scratch_mount
tests/xfs/234:57:SCRATCH_DEV=$TEST_DIR/image _scratch_unmount
tests/xfs/336:68:SCRATCH_DEV=$TEST_DIR/image _scratch_mount
tests/xfs/336:69:SCRATCH_DEV=$TEST_DIR/image _scratch_unmount
tests/xfs/157:61:       SCRATCH_DEV=$orig_ddev
tests/xfs/157:76:SCRATCH_DEV=$fake_datafile
tests/xfs/129:56:SCRATCH_DEV=$TEST_DIR/image _scratch_mount
tests/xfs/129:57:SCRATCH_DEV=$TEST_DIR/image _scratch_unmount
tests/ceph/005:27:SCRATCH_DEV="$SCRATCH_DEV/quota-dir" _scratch_mount
tests/ceph/005:29:SCRATCH_DEV="$SCRATCH_DEV_ORIG/quota-dir" _scratch_unmount
tests/ceph/005:31:SCRATCH_DEV="$SCRATCH_DEV_ORIG/quota-dir/subdir" _scratch_mount
tests/ceph/005:33:SCRATCH_DEV="$SCRATCH_DEV_ORIG/quota-dir/subdir" _scratch_unmount

> 

Re: [PATCH 1/3] xfs/432: fix this test when external devices are in use

[Darrick J. Wong]

On Sun, Jul 31, 2022 at 01:29:12PM +0800, Zorro Lang wrote:
> On Thu, Jul 28, 2022 at 01:16:28PM -0700, Christoph Hellwig wrote:
> > On Thu, Jul 28, 2022 at 11:17:15AM -0700, Darrick J. Wong wrote:
> > > +SCRATCH_DEV=$metadump_img _scratch_xfs_repair -n &>> $seqres.full || \
> > > +	echo "xfs_repair on restored fs returned $?"
> > 
> > Wouldn;t it make more sense to have a version of _scratch_xfs_repair
> > rather than doing a somewhat unexpected override of this global
> > variable?
> 
> Any detailed ideas about how to have a new version of _scratch_xfs_repair? I'll
> keep this patch unmerged this week, before Darrick reply your discussion.
> 
> Currently a few cases do some overriding [1] before calling _scratch_* helpers.
> And good news is this kind of "override" affect only the environment seen by
> its follow command/function. So I generally don't have objection if it works
> well. But it's welcome if we have a better idea to deal with this kind of
> requirement :)

I don't know of a better way to do "xfs_repair the scratch filesystem,
but with a different scratch device".

One could create a new helper wherein any parameter that happens to be a
path to a regular file or bdev would cause SCRATCH_DEV not to be
included, but now we're walking arguments instead of being agnostic
about them and merely passing them through to $XFS_REPAIR_PROG
untouched.  Worse yet, if there just happens to be (say) a file named
'-d' or 'rmapbt=1' in the current directory, the test will think it
should omit SCRATCH_DEV.  The C getopt parser in xfs_repair won't see
things this way, and these two tests become brittle over that.

This can be worked around by parsing the arguments so that any getopt
arguments will not be subjected to the "Should we override SCRATCH_DEV?"
test.  Because xfs_repair options change over time, we'd have to scan
the binary to extract the getopt string (or construct it from --help).
This is nontrivial, so the xfs_repair getopt extraction routines
themselves will need a new fstest to test the test code, lest the two
tests become brittle over that.

All that fugly parsing crap can be worked around by teaching xfs_repair
to ignore any respecified data device paths, but this is likely to cause
resistance from the xfsprogs maintainers because that sounds like an
avenue to introduce confusing behavior to end users.

*OR* we could just override the variable definition for the one line
since (for once) bash makes this easy and the syntax communicates very
loudly "HI USE THIS ALT SCRATCH_DEV PLZ".  And test authors already do
this.

--D

> Thanks,
> Zorro
> 
> 
> [1]
> $ grep -rsn SCRATCH_DEV= tests/
> tests/btrfs/160:36:old_SCRATCH_DEV=$SCRATCH_DEV
> tests/btrfs/160:38:SCRATCH_DEV=$DMERROR_DEV
> tests/btrfs/146:39:old_SCRATCH_DEV=$SCRATCH_DEV
> tests/btrfs/146:41:SCRATCH_DEV=$DMERROR_DEV
> tests/btrfs/146:62:SCRATCH_DEV=$old_SCRATCH_DEV
> tests/xfs/507:198:LARGE_SCRATCH_DEV=yes _check_xfs_filesystem $loop_dev none none
> tests/xfs/185:75:SCRATCH_DEV="$ddloop"
> tests/xfs/234:56:SCRATCH_DEV=$TEST_DIR/image _scratch_mount
> tests/xfs/234:57:SCRATCH_DEV=$TEST_DIR/image _scratch_unmount
> tests/xfs/336:68:SCRATCH_DEV=$TEST_DIR/image _scratch_mount
> tests/xfs/336:69:SCRATCH_DEV=$TEST_DIR/image _scratch_unmount
> tests/xfs/157:61:       SCRATCH_DEV=$orig_ddev
> tests/xfs/157:76:SCRATCH_DEV=$fake_datafile
> tests/xfs/129:56:SCRATCH_DEV=$TEST_DIR/image _scratch_mount
> tests/xfs/129:57:SCRATCH_DEV=$TEST_DIR/image _scratch_unmount
> tests/ceph/005:27:SCRATCH_DEV="$SCRATCH_DEV/quota-dir" _scratch_mount
> tests/ceph/005:29:SCRATCH_DEV="$SCRATCH_DEV_ORIG/quota-dir" _scratch_unmount
> tests/ceph/005:31:SCRATCH_DEV="$SCRATCH_DEV_ORIG/quota-dir/subdir" _scratch_mount
> tests/ceph/005:33:SCRATCH_DEV="$SCRATCH_DEV_ORIG/quota-dir/subdir" _scratch_unmount
> 
> > 
> 

Re: [PATCH 1/3] xfs/432: fix this test when external devices are in use

[Christoph Hellwig]

On Sun, Jul 31, 2022 at 09:58:39AM -0700, Darrick J. Wong wrote:
> *OR* we could just override the variable definition for the one line
> since (for once) bash makes this easy and the syntax communicates very
> loudly "HI USE THIS ALT SCRATCH_DEV PLZ".  And test authors already do
> this.

Ok, let's stick to that and I'll take my suggestion back.

Re: [PATCH 1/3] xfs/432: fix this test when external devices are in use

[Darrick J. Wong]

On Mon, Aug 01, 2022 at 10:14:11AM -0700, Christoph Hellwig wrote:
> On Sun, Jul 31, 2022 at 09:58:39AM -0700, Darrick J. Wong wrote:
> > *OR* we could just override the variable definition for the one line
> > since (for once) bash makes this easy and the syntax communicates very
> > loudly "HI USE THIS ALT SCRATCH_DEV PLZ".  And test authors already do
> > this.
> 
> Ok, let's stick to that and I'll take my suggestion back.

Ok, thank you! :)

--D

[PATCH 2/3] xfs/291: convert open-coded _scratch_xfs_repair usage

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Convert this test to use _scratch_xfs_repair, since the only variance
from the standard usage is that it's called against a sparse file into
which the scratch filesystem has been metadumped and mdrestored.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/291 |    6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)


diff --git a/tests/xfs/291 b/tests/xfs/291
index 6d5e247e..a2425e47 100755
--- a/tests/xfs/291
+++ b/tests/xfs/291
@@ -93,11 +93,7 @@ _scratch_xfs_check >> $seqres.full 2>&1 || _fail "xfs_check failed"
 # Can xfs_metadump cope with this monster?
 _scratch_xfs_metadump $tmp.metadump || _fail "xfs_metadump failed"
 xfs_mdrestore $tmp.metadump $tmp.img || _fail "xfs_mdrestore failed"
-[ "$USE_EXTERNAL" = yes ] && [ -n "$SCRATCH_RTDEV" ] && \
-	rt_repair_opts="-r $SCRATCH_RTDEV"
-[ "$USE_EXTERNAL" = yes ] && [ -n "$SCRATCH_LOGDEV" ] && \
-	log_repair_opts="-l $SCRATCH_LOGDEV"
-$XFS_REPAIR_PROG $rt_repair_opts $log_repair_opts -f $tmp.img >> $seqres.full 2>&1 || \
+SCRATCH_DEV=$tmp.img _scratch_xfs_repair -f &>> $seqres.full || \
 	_fail "xfs_repair of metadump failed"
 
 # Yes it can; success, all done

[PATCH 3/3] seek_sanity_test: use XFS ioctls to determine file allocation unit size

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

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
Tested-by: liuyd.fnst@fujitsu.com
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 src/seek_sanity_test.c |   36 +++++++++++++++++++++++++++---------
 1 file changed, 27 insertions(+), 9 deletions(-)


diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c
index 1030d0c5..78f835e8 100644
--- a/src/seek_sanity_test.c
+++ b/src/seek_sanity_test.c
@@ -40,6 +40,28 @@ static void get_file_system(int fd)
 	}
 }
 
+/* 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, XFS_IOC_FSGETXATTR, &fsx);
+	if (ret)
+		return -1;
+
+	alloc_size = fsgeom.blocksize;
+	if (fsx.fsx_xflags & XFS_XFLAG_REALTIME)
+		alloc_size *= fsgeom.rtextsize;
+
+	return 0;
+}
+
 static int get_io_sizes(int fd)
 {
 	off_t pos = 0, offset = 1;
@@ -47,6 +69,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 +80,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) {

Re: [PATCH 3/3] seek_sanity_test: use XFS ioctls to determine file allocation unit size

[Zorro Lang]

On Thu, Jul 28, 2022 at 11:17:26AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> 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
> Tested-by: liuyd.fnst@fujitsu.com
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---

This patch looks good to me, and I can reproduce the regression on nfs, then
test passed after merge this patch. So I'd like to merge this patch at first,
to fix that regression for nfs. Others 2 patches are still under discussion,
I'll wait.

Reviewed-by: Zorro Lang <zlang@redhat.com>

>  src/seek_sanity_test.c |   36 +++++++++++++++++++++++++++---------
>  1 file changed, 27 insertions(+), 9 deletions(-)
> 
> 
> diff --git a/src/seek_sanity_test.c b/src/seek_sanity_test.c
> index 1030d0c5..78f835e8 100644
> --- a/src/seek_sanity_test.c
> +++ b/src/seek_sanity_test.c
> @@ -40,6 +40,28 @@ static void get_file_system(int fd)
>  	}
>  }
>  
> +/* 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, XFS_IOC_FSGETXATTR, &fsx);
> +	if (ret)
> +		return -1;
> +
> +	alloc_size = fsgeom.blocksize;
> +	if (fsx.fsx_xflags & XFS_XFLAG_REALTIME)
> +		alloc_size *= fsgeom.rtextsize;
> +
> +	return 0;
> +}
> +
>  static int get_io_sizes(int fd)
>  {
>  	off_t pos = 0, offset = 1;
> @@ -47,6 +69,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 +80,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) {
> 

[PATCHSET 0/3] fstests: random fixes for v2022.07.31

[Darrick J. Wong]

Hi all,

Here's the usual batch of odd fixes for fstests.  Fix a golden output
error in the new xfs/533 test, and recycle the same "scratch fs with
not the usual scratch device" fixes.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=random-fixes

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=random-fixes

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes
---
 tests/xfs/291     |    6 +-----
 tests/xfs/432     |    3 ++-
 tests/xfs/533.out |    2 +-
 3 files changed, 4 insertions(+), 7 deletions(-)

[PATCH 1/3] xfs/432: fix this test when external devices are in use

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

This program exercises metadump and mdrestore being run against the
scratch device.  Therefore, the test must pass external log / rt device
arguments to xfs_repair -n to check the "restored" filesystem.  Fix the
incorrect usage, and report repair failures, since this test has been
silently failing for a while now.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/432 |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/tests/xfs/432 b/tests/xfs/432
index 86012f0b..e1e610d0 100755
--- a/tests/xfs/432
+++ b/tests/xfs/432
@@ -89,7 +89,8 @@ _scratch_xfs_metadump $metadump_file -w
 xfs_mdrestore $metadump_file $metadump_img
 
 echo "Check restored metadump image"
-$XFS_REPAIR_PROG -n $metadump_img >> $seqres.full 2>&1
+SCRATCH_DEV=$metadump_img _scratch_xfs_repair -n &>> $seqres.full || \
+	echo "xfs_repair on restored fs returned $?"
 
 # success, all done
 status=0

Re: [PATCH 1/3] xfs/432: fix this test when external devices are in use

[Zorro Lang]

On Tue, Aug 02, 2022 at 09:21:26PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> This program exercises metadump and mdrestore being run against the
> scratch device.  Therefore, the test must pass external log / rt device
> arguments to xfs_repair -n to check the "restored" filesystem.  Fix the
> incorrect usage, and report repair failures, since this test has been
> silently failing for a while now.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  tests/xfs/432 |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/tests/xfs/432 b/tests/xfs/432
> index 86012f0b..e1e610d0 100755
> --- a/tests/xfs/432
> +++ b/tests/xfs/432
> @@ -89,7 +89,8 @@ _scratch_xfs_metadump $metadump_file -w
>  xfs_mdrestore $metadump_file $metadump_img
>  
>  echo "Check restored metadump image"
> -$XFS_REPAIR_PROG -n $metadump_img >> $seqres.full 2>&1
> +SCRATCH_DEV=$metadump_img _scratch_xfs_repair -n &>> $seqres.full || \
> +	echo "xfs_repair on restored fs returned $?"

It's good to me,

Reviewed-by: Zorro Lang <zlang@redhat.com>

If anyone has more review points, please feel free to tell us :)

>  
>  # success, all done
>  status=0
>