Re: [PATCH] xfstests: Enable _test_mkfs to force a mkfs on a xfs filesystem

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, May 29, 2023 at 09:58:39AM +0200, Carlos Maiolino wrote:
> On Sat, May 27, 2023 at 03:52:34AM +0800, Zorro Lang wrote:
> > On Thu, May 25, 2023 at 12:13:12PM -0700, Darrick J. Wong wrote:
> > > On Thu, May 25, 2023 at 12:36:05PM +0800, Zorro Lang wrote:
> > > > On Wed, May 24, 2023 at 08:16:50AM -0700, Darrick J. Wong wrote:
> > > > > On Wed, May 24, 2023 at 03:42:07PM +0200, cem@kernel.org wrote:
> > > > > > From: Carlos Maiolino <cmaiolino@redhat.com>
> > > > > >
> > > > > > Calling _test_mkfs on an already initialized xfs FS will fail as the
> > > > > > initialization is not enforced by '-f' argument, unless it's included in
> > > > > > MKFS_OPTIONS.
> > > > > > So, adding 'RECREATE_TEST_DEV=true' to the config file end up being useless
> > > > > > for xfs filesystems.
> > > > > >
> > > > > > So, adding the a specific xfs optiong in _test_mkfs using -f argument
> > > > > > makes RECREATE_TEST_DEV actually useful.
> > > > > >
> > > > > > Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> > > > >
> > > > > Makese sense to me,
> > > > > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > > >
> > > > Hi Darrick,
> > > >
> > > > I just noticed that there's not any place use "_test_options mkfs", I remembered
> > > > SCRATCH_DEV support rtdev and logdev, the TEST_DEV looks not support that, but
> > > > the "_test_options()" looks like hoping to support.
> > >
> > > Hmmm, that's a very good point -- _test_mkfs() fails to format a test
> > > filesystem with external logs or rt volumes even if the config specified
> > > that.  Perhaps change the patch to add:
> > >
> > >     xfs)
> > >         _test_options mkfs
> > 
> > Current _test_options() only works for xfs, it returns directly if FSTYP != XFS.
> > It more likes a _test_xfs_options. We don't have _test_$FSTYP_options() function,
> > so if we'd like to have a common _test_options(), I think we should deal with
> > fs specific things in _test_options() inside, then call _test_options once before
> > the "case $FSTYP in ...".
> 
> This seems fine, I'm short on time these days, I'll try to find some time to
> work on this.
> 
> But in meantime, wouldn't it required the '-f' option to be passed to mkfs.xfs
> anyway? Calling _test_options before 'case $FSTYP ...' will still require use to
> call mkfs.xfs -f, so, can't the initial patch be applied already? If Darrick's
> updated solution isn't good enough for now, the original patch will fix at
> least part of the problem (without external log/rtdev cases) right away.

Yes, let's please get at least the bare fix merged.

--D

> -- 
> Carlos
> 
> > 
> > Thanks,
> > Zorro
> > 
> > >         $MKFS_XFS_PROG -f $TEST_OPTIONS $MKFS_OPTIONS $* $TEST_DEV
> > >         ;;
> > >
> > > Does that look right?
> > >
> > > --D
> > >
> > > > Thanks,
> > > > Zorro
> > > >
> > > > >
> > > > > --D
> > > > >
> > > > > > ---
> > > > > >  common/rc | 3 +++
> > > > > >  1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/common/rc b/common/rc
> > > > > > index 37074371..dd5d7bc1 100644
> > > > > > --- a/common/rc
> > > > > > +++ b/common/rc
> > > > > > @@ -605,6 +605,9 @@ _test_mkfs()
> > > > > >      ext2|ext3|ext4)
> > > > > >  	$MKFS_PROG -t $FSTYP -- -F $MKFS_OPTIONS $* $TEST_DEV
> > > > > >  	;;
> > > > > > +    xfs)
> > > > > > +	$MKFS_PROG -t $FSTYP -- -f $MKFS_OPTIONS $* $TEST_DEV
> > > > > > +	;;
> > > > > >      *)
> > > > > >  	yes | $MKFS_PROG -t $FSTYP -- $MKFS_OPTIONS $* $TEST_DEV
> > > > > >  	;;
> > > > > > --
> > > > > > 2.30.2
> > > > > >
> > > > >
> > > >
> > >
> >