Re: [PATCH 2/2] fstests: add configuration option for executing post mkfs commands

[Dave Chinner <david@fromorbit.com>]

On Thu, Sep 28, 2023 at 05:10:25PM +0930, Qu Wenruo wrote:
> On 2023/9/28 15:04, Anand Jain wrote:
> > On 28/09/2023 12:26, Qu Wenruo wrote:
> > > On 2023/9/28 13:53, Anand Jain wrote:
> > > > This patch introduces new configuration file parameters,
> > > > POST_SCRATCH_MKFS_CMD and POST_SCRATCH_POOL_MKFS_CMD.
> > > > 
> > > > Usage example:
> > > > 
> > > >          POST_SCRATCH_MKFS_CMD="btrfstune -m"
> > > >          POST_SCRATCH_POOL_MKFS_CMD="btrfstune -m"
> > > 
> > > Can't we add extra options for mkfs.btrfs to support metadata uuid at
> > > mkfs time?
> > > 
> > > We already support quota and all other features, I think it would be
> > > much easier to implement metadata_uuid inside mkfs.
> > > 
> > > If this feature is only for metadata_uuid, then I really prefer to do it
> > > inside mkfs.btrfs.
> > 
> > Thanks for the comments.
> > 
> > The use of btrfstune -m is just an example; any other command,
> > function, or script can be assigned to the variable POST_SCRATCH_xx.
> 
> The last time I tried something like this, I got strong objection from
> some guy in the XFS community.

That "some guy" has used fstests for 20 years, not to mention was
the maintainer for ~4 years and did most filesystem functionality
separation work that enabled fstests to become what it is now. 

Maybe, just maybe, that "some guy" actually has good reasons for
suggesting that the functionality is done in a certain way.  Both
XFS and ext4 already have optional post-mkfs functionality triggered
by environment variables (XFS dates back to 2003, ext4 back to 2013)
implemented in their filesystem specific mkfs functions.

If the configuration requires more than one command to be run, then
it can't be encoded easily in an environment variable.

Indeed, we shouldn't even be encoding fixed commands in environment
variables; environment variables should indicate functionality that
needs to be configured, and the fstests code itself implement the
commands that do that specific configuration. This allows multiple
configurations to be mixed and matched independently and without
needing users to encode complex commands into environment
variables....

That's the architecture we currently have: filesystem specific
configuration operations done at mkfs time should be done in the
filesystem specific mkfs routine.

> Just good luck if you can have a better chance.

Bad attitude doesn't win you friends or influence people.

> > Now, regarding updating mkfs.btrfs with the btrfstune -m feature,
> > why not? It simplifies testing. However, can we identify a use case
> > other than testing?
> 
> For consistency, I believe all (at the ones we can enable) features
> should have a mkfs equivalent at least.

That's a btrfs development problem, not a fstests issue.

> > > > With this configuration option, test cases using _scratch_mkfs(),
> > > > scratch_pool_mkfs(), or _scratch_mkfs_sized() will run the above
> > > > set value after the mkfs operation.
> > > > 
> > > > Other mkfs functions, such as _mkfs_dev(), are not connected to the
> > > > POST_SCRATCH_MKFS_CMD.
> > > > 
> > > > Signed-off-by: Anand Jain <anand.jain@oracle.com>
> > > > ---
> > > >   common/btrfs | 35 +++++++++++++++++++++++++++++++++++
> > > >   1 file changed, 35 insertions(+)
> > > > 
> > > > diff --git a/common/btrfs b/common/btrfs
> > > > index 798c899f6233..b0972e882c7c 100644
> > > > --- a/common/btrfs
> > > > +++ b/common/btrfs
> > > > @@ -690,17 +690,48 @@ _require_btrfs_scratch_logical_resolve_v2()
> > > >       _scratch_unmount
> > > >   }
> > > > 
> > > > +post_scratch_mkfs_cmd()
> > > > +{
> > > > +    if [[ -v POST_SCRATCH_MKFS_CMD ]]; then
> > > > +        echo "$POST_SCRATCH_MKFS_CMD $SCRATCH_DEV"
> > > > +        $POST_SCRATCH_MKFS_CMD $SCRATCH_DEV
> > > > +        return $?
> > > > +    fi

Ideally this should be something like:

export SCRATCH_BTRFS_UUID='<uuid spec>'

btrfs_tune_dev() {
	local dev="$1"

	if [ -n "$SCRATCH_BTRFS_UUID" ]; then
		btrfstune -m $SCRATCH_BTRFS_UUID $dev
		return $?
	fi
	return 0;
}

_scratch_pool_mkfs_btrfs()
{
	.....
	btrfs_tune_dev $SCRATCH_DEV_POOL
	.....
}	

_scratch_mkfs_btrfs()
{
	.....
	btrfs_tune_dev $SCRATCH_DEV
	.....
}	

See how different it becomes when you stop thinking about filesystem
configuration as a generic post-mkfs command and instead think of it
as just another configuration option?

-Dave.
-- 
Dave Chinner
david@fromorbit.com