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

[Anand Jain <anand.jain@oracle.com>]

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.