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

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