[PATCH 1/2] common/xfs: fix _scratch_mkfs_xfs_supports_concurrency

[PATCH 1/2] common/xfs: fix _scratch_mkfs_xfs_supports_concurrency

[Darrick J. Wong]

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

generic/347 regresses when external logs are in the configuration:

 --- a/generic/347.out		2025-07-15 14:45:15.044714644 -0700
 +++ b/generic/347.out.bad	2026-05-18 23:17:01.687750781 -0700
 @@ -1,2 +1,36 @@
  QA output created by 347
 -=== completed
 +Cannot specify both -l logdev and -l concurrency
 +Usage: mkfs.xfs

Since this actually tests running mkfs.xfs with $SCRATCH_OPTIONS, the
helper ought to have detected that you can't give both.  Unfortunately,
there's a weird switch that turns -l into -d even though they're not
quite the same option.  Delete that, and fix the indentation (tabs, not
spaces) in the helper.

Cc: <fstests@vger.kernel.org> # v2026.05.17
Fixes: 7f162f5bcf50fc ("common/xfs: helper function to check if -l/-d/-r concurrecy flags.")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 common/xfs |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/common/xfs b/common/xfs
index 4b3a7f7d153282..88299acdd086be 100644
--- a/common/xfs
+++ b/common/xfs
@@ -2431,8 +2431,7 @@ _require_xfs_healer()
 # -l/-d concurrency came in same xfsprogs release v6.7
  _scratch_mkfs_xfs_supports_concurrency()
 {
-    local arg="${1:-d}"
+	local arg="${1:-d}"
 
-    test "$arg" = "-l" && arg="-d"
-    _scratch_mkfs_xfs_supported "$arg" concurrency=0
+	_scratch_mkfs_xfs_supported "$arg" concurrency=0
 }

[PATCH 2/2] xfs/21[67]: fix mkfs log concurrency detection

[Darrick J. Wong]

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

Both of these tests use _scratch_mkfs_xfs_supports_concurrency to detect
if they should be adding -lconcurrency=0 to the mkfs options that are
used to format a loop device.

However, this doesn't work in practice because of three factors: 1) the
scratch device could have an external log set up; 2) the loop device
does /not/ have an external log device; and 3) -lconcurrency isn't
compatible with -llogdev.

Fix this by creating a more general mkfs option detection helper and use
that once we have the loop device set up.

Cc: <fstests@vger.kernel.org> # v2026.05.17
Fixes: ffc8bad17e5b2f ("xfs/21{6,7} Use default -l concurrency=0 on mkfs.xfs that supports it")
Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
---
 common/xfs    |   15 +++++++++++++++
 tests/xfs/216 |    9 ++++-----
 tests/xfs/217 |    9 ++++-----
 3 files changed, 23 insertions(+), 10 deletions(-)

diff --git a/common/xfs b/common/xfs
index 88299acdd086be..232d40fd5e01b1 100644
--- a/common/xfs
+++ b/common/xfs
@@ -64,6 +64,21 @@ _scratch_mkfs_xfs_opts()
 	echo "$MKFS_XFS_PROG $SCRATCH_OPTIONS $mkfs_opts"
 }
 
+_mkfs_xfs_supported()
+{
+	local mkfs_opts=$*
+
+	$MKFS_XFS_PROG -f -N $MKFS_OPTIONS $mkfs_opts
+	local mkfs_status=$?
+
+	# a mkfs failure may be caused by conflicts between $MKFS_OPTIONS and
+	# $mkfs_opts, try again without $MKFS_OPTIONS
+	if [ $mkfs_status -ne 0 -a -n "$mkfs_opts" ]; then
+		$MKFS_XFS_PROG -f -N $mkfs_opts
+		mkfs_status=$?
+	fi
+	return $mkfs_status
+}
 
 _scratch_mkfs_xfs_supported()
 {
diff --git a/tests/xfs/216 b/tests/xfs/216
index e3be9f3fe39ee0..bd98ee4dbfe960 100755
--- a/tests/xfs/216
+++ b/tests/xfs/216
@@ -22,11 +22,6 @@ _cleanup()
 
 _require_scratch
 _scratch_mkfs_xfs >/dev/null 2>&1
-if _scratch_mkfs_xfs_supports_concurrency -l >> $seqres.full 2>&1; then
-	loop_mkfs_opts="-l concurrency=0"
-else
-	loop_mkfs_opts=""
-fi
 _scratch_mount
 
 _require_loop
@@ -76,6 +71,10 @@ mkdir $LOOP_MNT
 
 loop_dev=$(_create_loop_device $LOOP_IMG)
 
+if _mkfs_xfs_supported $loop_mkfs_opts -l concurrency=0 $loop_dev &>> $seqres.full; then
+	loop_mkfs_opts="$loop_mkfs_opts -l concurrency=0"
+fi
+
 # walk over standard sizes (up to 256GB)
 _do_mkfs 1 2 4 8 16 32 64 128 256
 
diff --git a/tests/xfs/217 b/tests/xfs/217
index 5b210627b5ed18..5ba0b2198eb84f 100755
--- a/tests/xfs/217
+++ b/tests/xfs/217
@@ -21,11 +21,6 @@ _cleanup()
 
 _require_scratch
 _scratch_mkfs_xfs >/dev/null 2>&1
-if _scratch_mkfs_xfs_supports_concurrency -l >> $seqres.full 2>&1; then
-	loop_mkfs_opts="-l concurrency=0 -d concurrency=0"
-else
-	loop_mkfs_opts=""
-fi
 _scratch_mount
 # 16T mkfs requires a bit over 2G free
 _require_fs_space $SCRATCH_MNT 2202000
@@ -63,6 +58,10 @@ fi
 
 loop_dev=$(_create_loop_device $LOOP_IMG)
 
+if _mkfs_xfs_supported $loop_mkfs_opts -l concurrency=0 -d concurrency=0 $loop_dev &>> $seqres.full; then
+	loop_mkfs_opts="$loop_mkfs_opts -l concurrency=0 -d concurrency=0"
+fi
+
 #
 # walk over "new" sizes supported by recent xfsprogs.
 # Note that the last test is for 16TB-1GB as 32bit platforms only support

Re: [PATCH 1/2] common/xfs: fix _scratch_mkfs_xfs_supports_concurrency

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] xfs/21[67]: fix mkfs log concurrency detection

[Christoph Hellwig]

On Wed, May 20, 2026 at 12:01:12PM -0700, Darrick J. Wong wrote:
> +_mkfs_xfs_supported()

Can you add a top of function comment explaininging this helper?

> +{
> +	local mkfs_opts=$*
> +
> +	$MKFS_XFS_PROG -f -N $MKFS_OPTIONS $mkfs_opts
> +	local mkfs_status=$?
> +
> +	# a mkfs failure may be caused by conflicts between $MKFS_OPTIONS and
> +	# $mkfs_opts, try again without $MKFS_OPTIONS
> +	if [ $mkfs_status -ne 0 -a -n "$mkfs_opts" ]; then
> +		$MKFS_XFS_PROG -f -N $mkfs_opts
> +		mkfs_status=$?
> +	fi

Do we want this for a helper like this?  The automatic dropping of
options has been a major source of mess, and if we're using a helper
that checks to see if mkfs works, returning false and then _notrun()ing
would seem like the saner option in general.

Re: [PATCH 2/2] xfs/21[67]: fix mkfs log concurrency detection

[Darrick J. Wong]

On Sun, May 24, 2026 at 10:46:08PM -0700, Christoph Hellwig wrote:
> On Wed, May 20, 2026 at 12:01:12PM -0700, Darrick J. Wong wrote:
> > +_mkfs_xfs_supported()
> 
> Can you add a top of function comment explaininging this helper?

Done:

# Figure out if the supplied mkfs.xfs options are supported.  A device
# or file path must be specified as one of the options, though it will
# not be written.

> > +{
> > +	local mkfs_opts=$*
> > +
> > +	$MKFS_XFS_PROG -f -N $MKFS_OPTIONS $mkfs_opts
> > +	local mkfs_status=$?
> > +
> > +	# a mkfs failure may be caused by conflicts between $MKFS_OPTIONS and
> > +	# $mkfs_opts, try again without $MKFS_OPTIONS
> > +	if [ $mkfs_status -ne 0 -a -n "$mkfs_opts" ]; then
> > +		$MKFS_XFS_PROG -f -N $mkfs_opts
> > +		mkfs_status=$?
> > +	fi
> 
> Do we want this for a helper like this?  The automatic dropping of
> options has been a major source of mess, and if we're using a helper
> that checks to see if mkfs works, returning false and then _notrun()ing
> would seem like the saner option in general.

I copy-pasted it from the other helper, but now that I've written the
doc comment I agree with you that we should only determine if the
passed-in option set actually works.

Digression: I wonder if _scratch_mkfs_xfs_supported should have a
"-f" in its $XFS_MKFS_PROG invocations?

--D

Re: [PATCH 2/2] xfs/21[67]: fix mkfs log concurrency detection

[Christoph Hellwig]

On Thu, May 28, 2026 at 09:21:31PM -0700, Darrick J. Wong wrote:
> On Sun, May 24, 2026 at 10:46:08PM -0700, Christoph Hellwig wrote:
> > On Wed, May 20, 2026 at 12:01:12PM -0700, Darrick J. Wong wrote:
> > > +_mkfs_xfs_supported()
> > 
> > Can you add a top of function comment explaininging this helper?
> 
> Done:
> 
> # Figure out if the supplied mkfs.xfs options are supported.  A device
> # or file path must be specified as one of the options, though it will
> # not be written.

Sounds good.

> Digression: I wonder if _scratch_mkfs_xfs_supported should have a
> "-f" in its $XFS_MKFS_PROG invocations?

Probably, otherwise if you run this when something else was on the
device it randomly fails.

Re: [PATCH 2/2] xfs/21[67]: fix mkfs log concurrency detection

[Zorro Lang]

On Wed, May 20, 2026 at 12:01:12PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Both of these tests use _scratch_mkfs_xfs_supports_concurrency to detect
> if they should be adding -lconcurrency=0 to the mkfs options that are
> used to format a loop device.
> 
> However, this doesn't work in practice because of three factors: 1) the
> scratch device could have an external log set up; 2) the loop device
> does /not/ have an external log device; and 3) -lconcurrency isn't
> compatible with -llogdev.
> 
> Fix this by creating a more general mkfs option detection helper and use
> that once we have the loop device set up.
> 
> Cc: <fstests@vger.kernel.org> # v2026.05.17
> Fixes: ffc8bad17e5b2f ("xfs/21{6,7} Use default -l concurrency=0 on mkfs.xfs that supports it")
> Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> ---
>  common/xfs    |   15 +++++++++++++++
>  tests/xfs/216 |    9 ++++-----
>  tests/xfs/217 |    9 ++++-----
>  3 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/common/xfs b/common/xfs
> index 88299acdd086be..232d40fd5e01b1 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -64,6 +64,21 @@ _scratch_mkfs_xfs_opts()
>  	echo "$MKFS_XFS_PROG $SCRATCH_OPTIONS $mkfs_opts"
>  }
>  
> +_mkfs_xfs_supported()
> +{
> +	local mkfs_opts=$*

While it seems mkfs.xfs doesn't currently receive arguments with spaces, or
using "$@" might be safer, e.g.

  local -a mkfs_opts=("$@")
  $MKFS_XFS_PROG -f -N $MKFS_OPTIONS "${mkfs_opts[@]}"

> +
> +	$MKFS_XFS_PROG -f -N $MKFS_OPTIONS $mkfs_opts
> +	local mkfs_status=$?
> +
> +	# a mkfs failure may be caused by conflicts between $MKFS_OPTIONS and
> +	# $mkfs_opts, try again without $MKFS_OPTIONS
> +	if [ $mkfs_status -ne 0 -a -n "$mkfs_opts" ]; then
> +		$MKFS_XFS_PROG -f -N $mkfs_opts
> +		mkfs_status=$?
> +	fi
> +	return $mkfs_status
> +}

Since we now have _mkfs_xfs_supported, should we call it inside
_scratch_mkfs_xfs_supported ?

Thanks,
Zorro

>  
>  _scratch_mkfs_xfs_supported()
>  {
> diff --git a/tests/xfs/216 b/tests/xfs/216
> index e3be9f3fe39ee0..bd98ee4dbfe960 100755
> --- a/tests/xfs/216
> +++ b/tests/xfs/216
> @@ -22,11 +22,6 @@ _cleanup()
>  
>  _require_scratch
>  _scratch_mkfs_xfs >/dev/null 2>&1
> -if _scratch_mkfs_xfs_supports_concurrency -l >> $seqres.full 2>&1; then
> -	loop_mkfs_opts="-l concurrency=0"
> -else
> -	loop_mkfs_opts=""
> -fi
>  _scratch_mount
>  
>  _require_loop
> @@ -76,6 +71,10 @@ mkdir $LOOP_MNT
>  
>  loop_dev=$(_create_loop_device $LOOP_IMG)
>  
> +if _mkfs_xfs_supported $loop_mkfs_opts -l concurrency=0 $loop_dev &>> $seqres.full; then
> +	loop_mkfs_opts="$loop_mkfs_opts -l concurrency=0"
> +fi
> +
>  # walk over standard sizes (up to 256GB)
>  _do_mkfs 1 2 4 8 16 32 64 128 256
>  
> diff --git a/tests/xfs/217 b/tests/xfs/217
> index 5b210627b5ed18..5ba0b2198eb84f 100755
> --- a/tests/xfs/217
> +++ b/tests/xfs/217
> @@ -21,11 +21,6 @@ _cleanup()
>  
>  _require_scratch
>  _scratch_mkfs_xfs >/dev/null 2>&1
> -if _scratch_mkfs_xfs_supports_concurrency -l >> $seqres.full 2>&1; then
> -	loop_mkfs_opts="-l concurrency=0 -d concurrency=0"
> -else
> -	loop_mkfs_opts=""
> -fi
>  _scratch_mount
>  # 16T mkfs requires a bit over 2G free
>  _require_fs_space $SCRATCH_MNT 2202000
> @@ -63,6 +58,10 @@ fi
>  
>  loop_dev=$(_create_loop_device $LOOP_IMG)
>  
> +if _mkfs_xfs_supported $loop_mkfs_opts -l concurrency=0 -d concurrency=0 $loop_dev &>> $seqres.full; then
> +	loop_mkfs_opts="$loop_mkfs_opts -l concurrency=0 -d concurrency=0"
> +fi
> +
>  #
>  # walk over "new" sizes supported by recent xfsprogs.
>  # Note that the last test is for 16TB-1GB as 32bit platforms only support