_supports_xfs_scrub cleanups

_supports_xfs_scrub cleanups

[Christoph Hellwig]

Hi all,

this series adds a missing scrub support fix to xfs/262 and cleans
up a few bits around this.

[PATCH 1/3] xfs: check that the mountpoint is actually mounted in _supports_xfs_scrub

[Christoph Hellwig]

Add a sanity check that the passed in mount point is actually mounted
to guard against actually calling _supports_xfs_scrub before
$SCRATCH_MNT is mounted.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 common/xfs | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/common/xfs b/common/xfs
index f53b33fc5..9db998837 100644
--- a/common/xfs
+++ b/common/xfs
@@ -649,6 +649,8 @@ _supports_xfs_scrub()
 	test "$FSTYP" = "xfs" || return 1
 	test -x "$XFS_SCRUB_PROG" || return 1
 
+	mountpoint $mountpoint >/dev/null || echo "$mountpoint is not mounted"
+
 	# Probe for kernel support...
 	$XFS_IO_PROG -c 'help scrub' 2>&1 | grep -q 'types are:.*probe' || return 1
 	$XFS_IO_PROG -c "scrub probe" "$mountpoint" 2>&1 | grep -q "Inappropriate ioctl" && return 1
-- 
2.39.2

[PATCH 2/3] xfs: add a _scratch_require_xfs_scrub helper

[Christoph Hellwig]

Add a helper to call _supports_xfs_scrub with $SCRATCH_MNT and
$SCRATCH_DEV.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 common/xfs    | 7 +++++++
 tests/xfs/556 | 2 +-
 tests/xfs/716 | 2 +-
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/common/xfs b/common/xfs
index 9db998837..bfe979dbb 100644
--- a/common/xfs
+++ b/common/xfs
@@ -661,6 +661,13 @@ _supports_xfs_scrub()
 	return 0
 }
 
+# Does the scratch file system support scrub?
+_scratch_require_xfs_scrub()
+{
+	_supports_xfs_scrub $SCRATCH_MNT $SCRATCH_DEV || \
+		_notrun "Scrub not supported"
+}
+
 # Save a snapshot of a corrupt xfs filesystem for later debugging.
 _xfs_metadump() {
 	local metadump="$1"
diff --git a/tests/xfs/556 b/tests/xfs/556
index 061d8d572..6be993273 100755
--- a/tests/xfs/556
+++ b/tests/xfs/556
@@ -40,7 +40,7 @@ _scratch_mkfs >> $seqres.full
 _dmerror_init
 _dmerror_mount >> $seqres.full 2>&1
 
-_supports_xfs_scrub $SCRATCH_MNT $SCRATCH_DEV || _notrun "Scrub not supported"
+_scratch_require_xfs_scrub
 
 # Write a file with 4 file blocks worth of data
 victim=$SCRATCH_MNT/a
diff --git a/tests/xfs/716 b/tests/xfs/716
index 930a0ecbb..4cfb27f18 100755
--- a/tests/xfs/716
+++ b/tests/xfs/716
@@ -31,7 +31,7 @@ _require_test_program "punch-alternating"
 _scratch_mkfs > $tmp.mkfs
 _scratch_mount
 
-_supports_xfs_scrub $SCRATCH_MNT $SCRATCH_DEV || _notrun "Scrub not supported"
+_scratch_require_xfs_scrub
 
 # Force data device extents so that we can create a file with the exact bmbt
 # that we need regardless of rt configuration.
-- 
2.39.2

[PATCH 3/3] xfs/262: call _scratch_require_xfs_scrub

[Christoph Hellwig]

Call _scratch_require_xfs_scrub so that the test is _notrun on kernels
without online scrub support.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 tests/xfs/262 | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/xfs/262 b/tests/xfs/262
index b28a6c88b..0d1fd779d 100755
--- a/tests/xfs/262
+++ b/tests/xfs/262
@@ -29,6 +29,9 @@ _require_xfs_io_error_injection "force_repair"
 echo "Format and populate"
 _scratch_mkfs > "$seqres.full" 2>&1
 _scratch_mount
+
+_scratch_require_xfs_scrub
+
 cp $XFS_SCRUB_PROG $SCRATCH_MNT/xfs_scrub
 $LDD_PROG $XFS_SCRUB_PROG | sed -e '/\//!d;/linux-gate/d;/=>/ {s/.*=>[[:blank:]]*\([^[:blank:]]*\).*/\1/};s/[[:blank:]]*\([^[:blank:]]*\) (.*)/\1/' | while read lib; do
 	cp $lib $SCRATCH_MNT/
-- 
2.39.2

Re: [PATCH 1/3] xfs: check that the mountpoint is actually mounted in _supports_xfs_scrub

[Darrick J. Wong]

On Thu, Jan 11, 2024 at 03:24:05PM +0100, Christoph Hellwig wrote:
> Add a sanity check that the passed in mount point is actually mounted
> to guard against actually calling _supports_xfs_scrub before
> $SCRATCH_MNT is mounted.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  common/xfs | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/common/xfs b/common/xfs
> index f53b33fc5..9db998837 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -649,6 +649,8 @@ _supports_xfs_scrub()
>  	test "$FSTYP" = "xfs" || return 1
>  	test -x "$XFS_SCRUB_PROG" || return 1
>  
> +	mountpoint $mountpoint >/dev/null || echo "$mountpoint is not mounted"

The helper needs to return nonzero on failure, e.g.

	if ! mountpoint -q $mountpoint; then
		echo "$mountpoint is not mounted"
		return 1
	fi

> +
>  	# Probe for kernel support...
>  	$XFS_IO_PROG -c 'help scrub' 2>&1 | grep -q 'types are:.*probe' || return 1

Like it already does down here.

--D

>  	$XFS_IO_PROG -c "scrub probe" "$mountpoint" 2>&1 | grep -q "Inappropriate ioctl" && return 1
> -- 
> 2.39.2
> 

Re: [PATCH 2/3] xfs: add a _scratch_require_xfs_scrub helper

[Darrick J. Wong]

On Thu, Jan 11, 2024 at 03:24:06PM +0100, Christoph Hellwig wrote:
> Add a helper to call _supports_xfs_scrub with $SCRATCH_MNT and
> $SCRATCH_DEV.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Thanks for the cleanup,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  common/xfs    | 7 +++++++
>  tests/xfs/556 | 2 +-
>  tests/xfs/716 | 2 +-
>  3 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/common/xfs b/common/xfs
> index 9db998837..bfe979dbb 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -661,6 +661,13 @@ _supports_xfs_scrub()
>  	return 0
>  }
>  
> +# Does the scratch file system support scrub?
> +_scratch_require_xfs_scrub()
> +{
> +	_supports_xfs_scrub $SCRATCH_MNT $SCRATCH_DEV || \
> +		_notrun "Scrub not supported"
> +}
> +
>  # Save a snapshot of a corrupt xfs filesystem for later debugging.
>  _xfs_metadump() {
>  	local metadump="$1"
> diff --git a/tests/xfs/556 b/tests/xfs/556
> index 061d8d572..6be993273 100755
> --- a/tests/xfs/556
> +++ b/tests/xfs/556
> @@ -40,7 +40,7 @@ _scratch_mkfs >> $seqres.full
>  _dmerror_init
>  _dmerror_mount >> $seqres.full 2>&1
>  
> -_supports_xfs_scrub $SCRATCH_MNT $SCRATCH_DEV || _notrun "Scrub not supported"
> +_scratch_require_xfs_scrub
>  
>  # Write a file with 4 file blocks worth of data
>  victim=$SCRATCH_MNT/a
> diff --git a/tests/xfs/716 b/tests/xfs/716
> index 930a0ecbb..4cfb27f18 100755
> --- a/tests/xfs/716
> +++ b/tests/xfs/716
> @@ -31,7 +31,7 @@ _require_test_program "punch-alternating"
>  _scratch_mkfs > $tmp.mkfs
>  _scratch_mount
>  
> -_supports_xfs_scrub $SCRATCH_MNT $SCRATCH_DEV || _notrun "Scrub not supported"
> +_scratch_require_xfs_scrub
>  
>  # Force data device extents so that we can create a file with the exact bmbt
>  # that we need regardless of rt configuration.
> -- 
> 2.39.2
> 

Re: [PATCH 3/3] xfs/262: call _scratch_require_xfs_scrub

[Darrick J. Wong]

On Thu, Jan 11, 2024 at 03:24:07PM +0100, Christoph Hellwig wrote:
> Call _scratch_require_xfs_scrub so that the test is _notrun on kernels
> without online scrub support.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good!
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  tests/xfs/262 | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/tests/xfs/262 b/tests/xfs/262
> index b28a6c88b..0d1fd779d 100755
> --- a/tests/xfs/262
> +++ b/tests/xfs/262
> @@ -29,6 +29,9 @@ _require_xfs_io_error_injection "force_repair"
>  echo "Format and populate"
>  _scratch_mkfs > "$seqres.full" 2>&1
>  _scratch_mount
> +
> +_scratch_require_xfs_scrub
> +
>  cp $XFS_SCRUB_PROG $SCRATCH_MNT/xfs_scrub
>  $LDD_PROG $XFS_SCRUB_PROG | sed -e '/\//!d;/linux-gate/d;/=>/ {s/.*=>[[:blank:]]*\([^[:blank:]]*\).*/\1/};s/[[:blank:]]*\([^[:blank:]]*\) (.*)/\1/' | while read lib; do
>  	cp $lib $SCRATCH_MNT/
> -- 
> 2.39.2
> 

Re: [PATCH 1/3] xfs: check that the mountpoint is actually mounted in _supports_xfs_scrub

[Christoph Hellwig]

On Thu, Jan 11, 2024 at 09:20:22AM -0800, Darrick J. Wong wrote:
> > +	mountpoint $mountpoint >/dev/null || echo "$mountpoint is not mounted"
> 
> The helper needs to return nonzero on failure, e.g.
> 
> 	if ! mountpoint -q $mountpoint; then
> 		echo "$mountpoint is not mounted"
> 		return 1
> 	fi

No, it doesn't..  I actually did exactly that first, but that causes the
test to be _notrun instead of reporting the error and thus telling the
author that they usage of this helper is wrong.

Re: [PATCH 1/3] xfs: check that the mountpoint is actually mounted in _supports_xfs_scrub

[Zorro Lang]

On Thu, Jan 11, 2024 at 06:25:56PM +0100, Christoph Hellwig wrote:
> On Thu, Jan 11, 2024 at 09:20:22AM -0800, Darrick J. Wong wrote:
> > > +	mountpoint $mountpoint >/dev/null || echo "$mountpoint is not mounted"
> > 
> > The helper needs to return nonzero on failure, e.g.
> > 
> > 	if ! mountpoint -q $mountpoint; then
> > 		echo "$mountpoint is not mounted"
> > 		return 1
> > 	fi
> 
> No, it doesn't..  I actually did exactly that first, but that causes the
> test to be _notrun instead of reporting the error and thus telling the
> author that they usage of this helper is wrong.

So below "usage" message won't be gotten either, if a _notrun be called
after this helper return 1 .

        if [ -z "$device" ] || [ -z "$mountpoint" ]; then
                echo "Usage: _supports_xfs_scrub mountpoint device"
                return 1
        fi

If there's not _notrun after that, the message will be gotten I think.
So I think the "return 1" makes sense.

What do both of you think ?

Thanks,
Zorro

> 

Re: [PATCH 1/3] xfs: check that the mountpoint is actually mounted in _supports_xfs_scrub

[Darrick J. Wong]

On Fri, Jan 12, 2024 at 05:17:02AM +0800, Zorro Lang wrote:
> On Thu, Jan 11, 2024 at 06:25:56PM +0100, Christoph Hellwig wrote:
> > On Thu, Jan 11, 2024 at 09:20:22AM -0800, Darrick J. Wong wrote:
> > > > +	mountpoint $mountpoint >/dev/null || echo "$mountpoint is not mounted"
> > > 
> > > The helper needs to return nonzero on failure, e.g.
> > > 
> > > 	if ! mountpoint -q $mountpoint; then
> > > 		echo "$mountpoint is not mounted"
> > > 		return 1
> > > 	fi
> > 
> > No, it doesn't..  I actually did exactly that first, but that causes the
> > test to be _notrun instead of reporting the error and thus telling the
> > author that they usage of this helper is wrong.
> 
> So below "usage" message won't be gotten either, if a _notrun be called
> after this helper return 1 .
> 
>         if [ -z "$device" ] || [ -z "$mountpoint" ]; then
>                 echo "Usage: _supports_xfs_scrub mountpoint device"
>                 return 1
>         fi
> 
> If there's not _notrun after that, the message will be gotten I think.
> So I think the "return 1" makes sense.
> 
> What do both of you think ?

_fail "\$mountpoint must be mounted to use _require_scratch_xfs_scrub" ?

--D

> Thanks,
> Zorro
> 
> > 
> 
> 

Re: [PATCH 1/3] xfs: check that the mountpoint is actually mounted in _supports_xfs_scrub

[Christoph Hellwig]

On Fri, Jan 12, 2024 at 05:17:02AM +0800, Zorro Lang wrote:
> > No, it doesn't..  I actually did exactly that first, but that causes the
> > test to be _notrun instead of reporting the error and thus telling the
> > author that they usage of this helper is wrong.
> 
> So below "usage" message won't be gotten either, if a _notrun be called
> after this helper return 1 .

True.  But for the case reproducing my original error where I just
misplaced it it does get shown.

> If there's not _notrun after that, the message will be gotten I think.
> So I think the "return 1" makes sense.

As this point we might as well skip this patch, as it won't be useful.

Re: [PATCH 1/3] xfs: check that the mountpoint is actually mounted in _supports_xfs_scrub

[Christoph Hellwig]

On Thu, Jan 11, 2024 at 06:17:49PM -0800, Darrick J. Wong wrote:
> > If there's not _notrun after that, the message will be gotten I think.
> > So I think the "return 1" makes sense.
> > 
> > What do both of you think ?
> 
> _fail "\$mountpoint must be mounted to use _require_scratch_xfs_scrub" ?

Yes, that's better.  I didn't even remember _fail exists..