Re: [PATCH RFC] fstests: use MOUNT_OPTIONS to populate TEST_FS_MOUNT_OPTS if possible

[Zorro Lang <zlang@kernel.org>]

On Tue, Jun 02, 2026 at 09:44:22AM +0930, Qu Wenruo wrote:
> If the test config only specifies "MOUNT_OPTIONS", but not
> "TEST_FS_MOUNT_OPTS", the mount for TEST_DIR can have inconsistent flag
> after _test_cycle_mount().
> 
> Here is an very simple test case to show the problem:
> 
>  . ./common/preamble
>  _begin_fstest auto
>  _require_test
> 
>  mount | grep "$TEST_DIR"
>  _test_cycle_mount

AFAIK, TEST_FS_MOUNT_OPTS is designed to be the exact counterpart to
MOUNT_OPTIONS. The former applies to TEST_DEV, while the latter affects
SCRATCH_DEV and any other manually created devices (such as loop or dm
devices). Therefore, when testing custom mount options, better to specify
both parameters.

>  mount | grep "$TEST_DIR"
>  _exit 0
> 
> And with "MOUNT_OPTIONS" set to "-o nodatasum" (one of the btrfs mount
> option that affects new inodes), but "TEST_FS_MOUNT_OPTS" not set, then
> running the test, the output will be:
> 
>  QA output created by 348
>  /dev/mapper/test-test on /mnt/test type btrfs (rw,relatime,nodatasum,discard=async,space_cache=v2,subvolid=5,subvol=/)
>  /dev/mapper/test-test on /mnt/test type btrfs (rw,relatime,discard=async,space_cache=v2,subvolid=5,subvol=/)
> 
> This changed mount option will cause several test cases to fail on btrfs
> with MOUNT_OPTIONS="-o nodatasum" set:
> 
> - generic/143
> - generic/154
> - generic/155
> - generic/463
> 
> All above test case fails because the reflink source is from the initial
> test mount, which has "nodatasum" mount option.
> But later after _test_cycle_mount(), no more "nodatasum" flags, causing
> reflink source and destination have different "nodatasum" flags.
> And btrfs will reject such reflink, causing golden output mismatch.
> 
> One way to avoid such problem is to set TEST_FS_MOUNT_OPTS to the same
> as MOUNT_OPTIONS, but I'm not sure if we really want different options
> for test and scratch mounts.
> 
> So here as a compromise, set TEST_FS_MOUNT_OPTS to MOUNT_OPTIONS if
> the former is not set but the latter is already set by the user.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
> Reason for RFC:
> 
> - Still didn't get why the initial test mount respects MOUNT_OPTIONS
>   If test mount only respects TEST_FS_MOUNT_OPTS, the first mount should
>   not get the MOUNT_OPTIONS.
> 
> - Not sure if TEST_FS_MOUNT_OPTS is needed
>   If not provided, the default config for TEST_FS_MOUNT_OPTS and
>   MOUNT_OPTIONS are the same.
>
>   The introduction of TEST_FS_MOUNT_OPTS is from commit 0e9141e49d4a
>   ("common: add cifs support"), with extra handling for
>   _test_mount_opts().

No, actually TEST_FS_MOUNT_OPTS is far more than that. For example, you can
check commit 3839d29973, it has TEST_FS_MOUNT_OPTS in _test_mount. But at
that time xfstests was still under its original form, there was not common/
directory.

Then 8c4905a4 create common/ directory and common/rc, then TEST_FS_MOUNT_OPTS
started to appear in common/rc:_test_mount().

Then commit 0e9141e4 brought in _test_mount_opts for CIFS, but it didn't change
the logic of TEST_FS_MOUNT_OPTS for TEST_DEV mount. And we can learn about why
CIFS need a seperated TEST_FS_MOUNT_OPTS from its wiki link:

  https://wiki.samba.org/images/9/99/Xfstests.local.config.txt

Therefore, TEST_FS_MOUNT_OPTS has been dedicated entirely to TEST_DEV by design
since the very inception of xfstests, and this principle seems to have remained
unchanged. The real issue now is that we run into a conflict between
TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS when the helpers try to detect features,
particularly those that depend on mount options.

As for whether TEST_FS_MOUNT_OPTS is still necessary, I feel we need more
discussion on this. Some upper filesystems might still need it, and the original
purpose of keeping it separate for XFS testing probably deserves a deeper look.
(CC'ing Dave Chinner, who might know more that that history :)

About your current Btrfs tests, I would highly recommend specifying both
TEST_FS_MOUNT_OPTS and MOUNT_OPTIONS at the same time. (I always do that
actually).

Regarding the feature detection mismatch, a quick fix could be making TEST_FS_MOUNT_OPTS
default to MOUNT_OPTIONS when it's not set. That said, it is still worth double-checking
the initial purpose of TEST_FS_MOUNT_OPTS, just in case I miss something.

Thanks,
Zorro

> 
>   But nowadays the special handling is moved to _common_mount_opts(),
>   shared by both scratch and test mounts.
>   Not sure if there is any fs that requires split options for test and
>   scratch.
> 
> - Possible reduced coverage
>   Ignoring MOUNT_OPTIONS and requiring extra TEST_FS_MOUNT_OPTS can cause
>   reduced coverage, as one may expect MOUNT_OPTIONS to affect both test
>   and scratch devices.
> ---
>  common/config | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/common/config b/common/config
> index d5299d5b..12e94088 100644
> --- a/common/config
> +++ b/common/config
> @@ -419,8 +419,14 @@ _mount_opts()
>  	export MOUNT_OPTIONS=$(_common_mount_opts)
>  }
>  
> +# Must be called before _mount_opts().
>  _test_mount_opts()
>  {
> +	# Use $MOUNT_OPTIONS if provided.
> +	if [ ! -z "$MOUNT_OPTIONS" ]; then
> +		export TEST_FS_MOUNT_OPTS="$MOUNT_OPTIONS"
> +		return
> +	fi
>  	export TEST_FS_MOUNT_OPTS=$(_common_mount_opts)
>  }
>  
> @@ -823,8 +829,8 @@ get_next_config() {
>  
>  	parse_config_section $1
>  	if [ ! -z "$OLD_FSTYP" ] && [ $OLD_FSTYP != $FSTYP ]; then
> -		[ -z "$MOUNT_OPTIONS" ] && _mount_opts
>  		[ -z "$TEST_FS_MOUNT_OPTS" ] && _test_mount_opts
> +		[ -z "$MOUNT_OPTIONS" ] && _mount_opts
>  		[ -z "$MKFS_OPTIONS" ] && _mkfs_opts
>  		[ -z "$FSCK_OPTIONS" ] && _fsck_opts
>  
> @@ -915,8 +921,8 @@ if [ -z "$CONFIG_INCLUDED" ]; then
>  	fi
>  	FSTYP=${FSTYP:=xfs}
>  	export FSTYP
> -	[ -z "$MOUNT_OPTIONS" ] && _mount_opts
>  	[ -z "$TEST_FS_MOUNT_OPTS" ] && _test_mount_opts
> +	[ -z "$MOUNT_OPTIONS" ] && _mount_opts
>  	[ -z "$MKFS_OPTIONS" ] && _mkfs_opts
>  	[ -z "$FSCK_OPTIONS" ] && _fsck_opts
>  else
> -- 
> 2.51.2
> 
>