[PATCH RFC] fstests: add configuration option for executing post mkfs commands

[PATCH RFC] fstests: add configuration option for executing post mkfs commands

[Anand Jain]

This patch introduces a new configuration file parameter, POST_MKFS_CMD,
which, when set, will run immediately it after the mkfs command for the
FSTYP btrfs.

	For example:
	POST_MKFS_CMD="btrfstune -m"

Currently, only btrfstune's '-m' option is tested. However, there may be
more btrfstune options, so having this parameter as a config makes sense.
Additionally, there can be other commands besides btrfstune.

The mkfs helper functions in common/rc sends the SCRATCH_DEV as an argument
to the set POST_MKFS_CMD, which may not be compatible with other commands.
However, as of now, since those usecases are still unknown, we can modify
it later. For now, I'm marking this as RFC, I'm open to feedback if any.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 common/rc | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 5c4429ed0425..3e3c84259f38 100644
--- a/common/rc
+++ b/common/rc
@@ -667,6 +667,9 @@ _mkfs_dev()
 	exit 1
     fi
     rm -f $tmp.mkfserr $tmp.mkfsstd
+    if [[ -v POST_MKFS_CMD ]]; then
+	$POST_MKFS_CMD $(echo $* | $AWK_PROG '{print $1}')
+    fi
 }
 
 # remove all files in $SCRATCH_MNT, useful when testing on NFS/AFS/CIFS
@@ -757,6 +760,9 @@ _scratch_mkfs()
 	esac
 
 	_scratch_do_mkfs "$mkfs_cmd" "$mkfs_filter" $*
+	if [[ -v POST_MKFS_CMD ]]; then
+		$POST_MKFS_CMD $SCRATCH_DEV
+	fi
 	return $?
 }
 
@@ -878,7 +884,11 @@ _scratch_pool_mkfs()
 {
     case $FSTYP in
     btrfs)
-        $MKFS_BTRFS_PROG $MKFS_OPTIONS $* $SCRATCH_DEV_POOL > /dev/null
+        $MKFS_BTRFS_PROG $MKFS_OPTIONS $* $SCRATCH_DEV_POOL
+	if [[ -v POST_MKFS_CMD ]]; then
+		$POST_MKFS_CMD $(echo $SCRATCH_DEV_POOL |\
+						$AWK_PROG '{print $1}')
+	fi
         ;;
     *)
         echo "_scratch_pool_mkfs is not implemented for $FSTYP" 1>&2
-- 
2.39.3

[PATCH v1] fstests: add configuration option for executing post mkfs commands

[Anand Jain]

This patch introduces a new configuration file parameter, POST_MKFS_CMD,
which, when set, will run immediately it after the mkfs command for the
FSTYP btrfs.

	For example:
	POST_MKFS_CMD="btrfstune -m"

Currently, only btrfstune's '-m' option is tested. However, there may be
more btrfstune options, so having this parameter as a config makes sense.
Additionally, there can be other commands besides btrfstune.

The mkfs helper functions in common/rc passes the SCRATCH_DEV as an argument
to the set POST_MKFS_CMD, which may not be compatible with other configurable
commands in the future. However, as of now, since those usecases are still
unknown, we can modify it later.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
RFC -> v1:
 No code changes, only commit log updated.
 This feature has been a great help in spotting new bugs, and it's set to
 be really handy for testing the upcoming feature. So, I'd like to bump it
 up to v1. I'm open to feedback if any. Thanks
 
 common/rc | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/common/rc b/common/rc
index 5c4429ed0425..3e3c84259f38 100644
--- a/common/rc
+++ b/common/rc
@@ -667,6 +667,9 @@ _mkfs_dev()
 	exit 1
     fi
     rm -f $tmp.mkfserr $tmp.mkfsstd
+    if [[ -v POST_MKFS_CMD ]]; then
+	$POST_MKFS_CMD $(echo $* | $AWK_PROG '{print $1}')
+    fi
 }
 
 # remove all files in $SCRATCH_MNT, useful when testing on NFS/AFS/CIFS
@@ -757,6 +760,9 @@ _scratch_mkfs()
 	esac
 
 	_scratch_do_mkfs "$mkfs_cmd" "$mkfs_filter" $*
+	if [[ -v POST_MKFS_CMD ]]; then
+		$POST_MKFS_CMD $SCRATCH_DEV
+	fi
 	return $?
 }
 
@@ -878,7 +884,11 @@ _scratch_pool_mkfs()
 {
     case $FSTYP in
     btrfs)
-        $MKFS_BTRFS_PROG $MKFS_OPTIONS $* $SCRATCH_DEV_POOL > /dev/null
+        $MKFS_BTRFS_PROG $MKFS_OPTIONS $* $SCRATCH_DEV_POOL
+	if [[ -v POST_MKFS_CMD ]]; then
+		$POST_MKFS_CMD $(echo $SCRATCH_DEV_POOL |\
+						$AWK_PROG '{print $1}')
+	fi
         ;;
     *)
         echo "_scratch_pool_mkfs is not implemented for $FSTYP" 1>&2
-- 
2.39.3

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

[Dave Chinner]

On Thu, Sep 14, 2023 at 11:07:43PM +0800, Anand Jain wrote:
> This patch introduces a new configuration file parameter, POST_MKFS_CMD,
> which, when set, will run immediately it after the mkfs command for the
> FSTYP btrfs.
> 
> 	For example:
> 	POST_MKFS_CMD="btrfstune -m"
> 
> Currently, only btrfstune's '-m' option is tested. However, there may be
> more btrfstune options, so having this parameter as a config makes sense.
> Additionally, there can be other commands besides btrfstune.
> 
> The mkfs helper functions in common/rc passes the SCRATCH_DEV as an argument
> to the set POST_MKFS_CMD, which may not be compatible with other configurable
> commands in the future. However, as of now, since those usecases are still
> unknown, we can modify it later.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

In general, we've put filesystem specific post-mkfs commands inside
the filesystem specific mkfs function.

See _scratch_mkfs_xfs() for example. If we want to test TB scale
scratch filesystems without requiring ENOSPC tests to fill TBs of
disk space, we set LARGE_SCRATCH_DEV. This causes the mkfs function
to do the post-mkfs creation of a hidden file that consumes all but
50GB of space via fallocate (by calling _setup_large_xfs_fs()).
Hence filesystem filling tests don't spend forever filling the
filesystem, and no code outside of XFS specific functions need to
care that this hidden file exists....

Given that the use case here is to issue filesystem specific
commands rather than generic setup commands needed for all
filesystems, I think it would be better to encapsulate it inside the
btrfs specific mkfs implementation....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Anand Jain]

> In general, we've put filesystem specific post-mkfs commands inside
> the filesystem specific mkfs function.
> 
 >
> See _scratch_mkfs_xfs() for example. If we want to test TB scale
> scratch filesystems without requiring ENOSPC tests to fill TBs of
> disk space, we set LARGE_SCRATCH_DEV. This causes the mkfs function
> to do the post-mkfs creation of a hidden file that consumes all but
> 50GB of space via fallocate (by calling _setup_large_xfs_fs()).
> Hence filesystem filling tests don't spend forever filling the
> filesystem, and no code outside of XFS specific functions need to
> care that this hidden file exists....
> 
> Given that the use case here is to issue filesystem specific
> commands rather than generic setup commands needed for all
> filesystems, I think it would be better to encapsulate it inside the
> btrfs specific mkfs implementation....
> 


IMO, making it configurable and generic would also benefit other
filesystems. For instance, the XFS filesystem could set it to
'POST_MKFS_CMD="xfs_admin -p"' or something similar ?

The design choice here is to create an open and configurable command
variable. This is because we have several commands and options that
we need to test, and it wouldn't be practical to hardcode them.

Any comments would be appreciated. I thought I would check again; I
don't mind hardcoding it to a command specific to btrfs only if
you still think it wouldn't be useful otherwise."

Thanks, Anand

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

[Dave Chinner]

On Sun, Sep 17, 2023 at 07:58:11PM +0800, Anand Jain wrote:
> 
> > In general, we've put filesystem specific post-mkfs commands inside
> > the filesystem specific mkfs function.
> > 
> >
> > See _scratch_mkfs_xfs() for example. If we want to test TB scale
> > scratch filesystems without requiring ENOSPC tests to fill TBs of
> > disk space, we set LARGE_SCRATCH_DEV. This causes the mkfs function
> > to do the post-mkfs creation of a hidden file that consumes all but
> > 50GB of space via fallocate (by calling _setup_large_xfs_fs()).
> > Hence filesystem filling tests don't spend forever filling the
> > filesystem, and no code outside of XFS specific functions need to
> > care that this hidden file exists....
> > 
> > Given that the use case here is to issue filesystem specific
> > commands rather than generic setup commands needed for all
> > filesystems, I think it would be better to encapsulate it inside the
> > btrfs specific mkfs implementation....
> > 
> 
> 
> IMO, making it configurable and generic would also benefit other
> filesystems. For instance, the XFS filesystem could set it to
> 'POST_MKFS_CMD="xfs_admin -p"' or something similar ?

That's basically no different to setting up the same filesystem
config as using mkfs to do it. And a lot of the things that
xfs_admin can change are always set on v5 format filesytsem and
can't actually be modified. e.g. "-p" is such an option that is only
ever added to old v4 filesystems, and even then it's been the mkfs
default since 2013.

As it is, it can't easily be used for things like LARGE_SCRATCH_DEV,
because that requires multiple operations to create and internal
fstests knowledge that large devices are being used.

> The design choice here is to create an open and configurable command
> variable. This is because we have several commands and options that
> we need to test, and it wouldn't be practical to hardcode them.

I'm not suggesting that you hard code them. I'm just saying that for
filesystem specific post-mkfs changes prior to mounting the
filesytsem fo rthe first time, the code should be located in the
filesytsem specific mkfs functions. You *must* be doing filesystem
specific things here because the filesystem hasn't been mounted, and
that greatly limits the generic things one can do with such a
command....

That is, you can still use environment variables to specify the
-optional- post mkfs changes you want to test, but doing it from the
internal _scratch_mkfs_$FSTYP() function allows the implementation
to be specifically customised to whatever sort of complex operations
you need to perform for that filesystem type without needing to care
how that may impact other filesystems....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Anand Jain]

On 18/09/2023 09:18, Dave Chinner wrote:
> On Sun, Sep 17, 2023 at 07:58:11PM +0800, Anand Jain wrote:
>>
>>> In general, we've put filesystem specific post-mkfs commands inside
>>> the filesystem specific mkfs function.
>>>
>>>
>>> See _scratch_mkfs_xfs() for example. If we want to test TB scale
>>> scratch filesystems without requiring ENOSPC tests to fill TBs of
>>> disk space, we set LARGE_SCRATCH_DEV. This causes the mkfs function
>>> to do the post-mkfs creation of a hidden file that consumes all but
>>> 50GB of space via fallocate (by calling _setup_large_xfs_fs()).
>>> Hence filesystem filling tests don't spend forever filling the
>>> filesystem, and no code outside of XFS specific functions need to
>>> care that this hidden file exists....
>>>
>>> Given that the use case here is to issue filesystem specific
>>> commands rather than generic setup commands needed for all
>>> filesystems, I think it would be better to encapsulate it inside the
>>> btrfs specific mkfs implementation....
>>>
>>
>>
>> IMO, making it configurable and generic would also benefit other
>> filesystems. For instance, the XFS filesystem could set it to
>> 'POST_MKFS_CMD="xfs_admin -p"' or something similar ?
> 
> That's basically no different to setting up the same filesystem
> config as using mkfs to do it. And a lot of the things that
> xfs_admin can change are always set on v5 format filesytsem and
> can't actually be modified. e.g. "-p" is such an option that is only
> ever added to old v4 filesystems, and even then it's been the mkfs
> default since 2013.
> 
> As it is, it can't easily be used for things like LARGE_SCRATCH_DEV,
> because that requires multiple operations to create and internal
> fstests knowledge that large devices are being used.
> 
>> The design choice here is to create an open and configurable command
>> variable. This is because we have several commands and options that
>> we need to test, and it wouldn't be practical to hardcode them.
> 
> I'm not suggesting that you hard code them. I'm just saying that for
> filesystem specific post-mkfs changes prior to mounting the
> filesytsem fo rthe first time, the code should be located in the
> filesytsem specific mkfs functions. You *must* be doing filesystem
> specific things here because the filesystem hasn't been mounted, and
> that greatly limits the generic things one can do with such a
> command....
>  > That is, you can still use environment variables to specify the
> -optional- post mkfs changes you want to test, but doing it from the
> internal _scratch_mkfs_$FSTYP() function allows the implementation
> to be specifically customised to whatever sort of complex operations
> you need to perform for that filesystem type without needing to care
> how that may impact other filesystems....
> 


These changes have been implemented in the v2 that was sent out. Please 
review and appreciate any comments you may have.

https://lore.kernel.org/linux-btrfs/dfc4cece-d809-4b5b-93f7-7251ba3a492a@gmx.com/T/#u

Thanks, Anand

> Cheers,
> 
> Dave.