| Message ID | 9c6d36835c04f18a59005a8994ba128970bac20a.1690446808.git.anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] fstests: add configuration option for executing post mkfs commands | expand |
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.
> 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
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.
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.
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
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(-)