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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2023/10/6 16:39, Darrick J. Wong wrote:
> 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.

For this particular case, sure, it can be done through mkfs flags.
(As I already mentioned, I also believe this is the correct way to go,
for this particular case)

>  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?

IIRC it's mostly lack of test coverage, but I won't be surprised if we
really found some bugs.
Lack of coverage means bugs, just sooner or later.

>
> 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?

For this particular case, I think the mkfs.btrfs way is good enough to
handle, just as my first reply said.


However for the whole btrfs/fstests combination, we have several
features which can not be easily integrated into fstests.

The biggest example is multi-device management.

For now, only some btrfs specific test cases are utilizing
SCRATCH_DEV_POOL to cover multi-device functionality (including all the
RAID and seed support).
This means way less coverage for seed and btrfs RAID, all generic group
would not utilize btrfs RAID/seed functionality at all.

For a better coverage, or for more complex setup (maybe dm-dust for XFS
log device?), I am not that convinced if the current plain mkfs options
is good enough.

Thus I'm more interested in exploring the possibility to "out-source"
those basic functionality (from mkfs to check) to outside scripts, as
we're not that far away to hit the limits of the existing framework. (At
least for btrfs)

>
> 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?

That's not a big deal, we (at least me) are very open to add this mkfs
feature.

But there are other limits, like the fsck part.

For now, btrfs follows the behavior of other fses, just check the
correctness of the metadata, and ignore the correctness of data.

But remember btrfs has data checksum by default, thus it can easily
verify the data too, and we have the extra switch ("--check-data-csum"
option) to enable that for "btrfs check".

For now we're not going to enable the "--check-data-csum" option nor we
have the ability to teach fstests how to change the behavior.

Thus I'm taking the chance to explore any way to "out-source" those
mkfs/fsck functionality, even this means other fses may not even bother
as the current framework just works good enough for them.

But IIRC, even f2fs is gaining multi-device support, I believe this is
not a btrfs specific thing, but a framework limitation.

On the other hand, I can also see the possible problems of too many
things out-sourced to external scripts, but I believe we at least need
some honest and constructive discussion on the limits of the existing
framework.

Thanks,
Qu
>
> --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 $?
>>>>>    }