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

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

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.
> 
> Just good luck if you can have a better chance.

As another guy in the XFS community, I also don't understand why this
can't be accomplished with a _scratch_mkfs_btrfs helper that runs the
real mkfs tool and then tunes the resulting fs.  Is it significant for
bug finding to be able to run an entire separate fstests config with
this config?  Versus writing a targeted exerciser for the -m case?

Is there some reason why the exact command needs to be injected via
environment variables?  Or, why can't mkfs.btrfs do whatever "btrfstune
-m" does?

I suppose the problem there is that mkfs.btrfs won't itself create a
filesystem with the metadata_uuid field that doesn't match the other
uuid?

--D

> > 
> > 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.
> 
> (And can get around the the test limitations for sure)
> 
> Thanks,
> Qu
> > 
> > Thanks, Anand
> > 
> > > 
> > > Thanks,
> > > Qu
> > > > 
> > > > 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
> > > > +
> > > > +    return 0
> > > > +}
> > > > +
> > > > +post_scratch_pool_mkfs_cmd()
> > > > +{
> > > > +    if [[ -v POST_SCRATCH_POOL_MKFS_CMD ]]; then
> > > > +        echo "$POST_SCRATCH_POOL_MKFS_CMD $SCRATCH_DEV_POOL"
> > > > +        $POST_SCRATCH_POOL_MKFS_CMD $SCRATCH_DEV_POOL
> > > > +        return $?
> > > > +    fi
> > > > +
> > > > +    return 0
> > > > +}
> > > > +
> > > >   _scratch_mkfs_retry_btrfs()
> > > >   {
> > > >       # _scratch_do_mkfs() may retry mkfs without $MKFS_OPTIONS
> > > >       _scratch_do_mkfs "$MKFS_BTRFS_PROG" "cat" $*
> > > > 
> > > > +    if [[ $? == 0 ]]; then
> > > > +        post_scratch_mkfs_cmd
> > > > +    fi
> > > > +
> > > >       return $?
> > > >   }
> > > > 
> > > >   _scratch_mkfs_btrfs()
> > > >   {
> > > >       $MKFS_BTRFS_PROG $MKFS_OPTIONS $mixed_opt -b $fssize $SCRATCH_DEV
> > > > +
> > > > +    if [[ $? == 0 ]]; then
> > > > +        post_scratch_mkfs_cmd
> > > > +    fi
> > > > +
> > > >       return $?
> > > >   }
> > > > 
> > > > @@ -708,5 +739,9 @@ _scratch_pool_mkfs_btrfs()
> > > >   {
> > > >       $MKFS_BTRFS_PROG $MKFS_OPTIONS $* $SCRATCH_DEV_POOL
> > > > 
> > > > +    if [[ $? == 0 ]]; then
> > > > +        post_scratch_pool_mkfs_cmd
> > > > +    fi
> > > > +
> > > >       return $?
> > > >   }