[PATCH v1 0/3] Add mount and remount related tests

[PATCH v1 0/3] Add mount and remount related tests

[Nirjhar Roy (IBM)]

This patch modifies an existing test (xfs/539), adds a new xfs specific helper
function and adds a new mount/remount test. The individual patches have the
details.

Nirjhar Roy (IBM) (3):
  xfs/539: Skip noattr2 remount option on v5 filesystems
  common/xfs: Add a new helper function to check v5 XFS
  xfs: Add a testcase to check remount with noattr2 on a v5 xfs

 common/xfs        | 13 +++++++++++++
 tests/xfs/539     | 10 ++++++++--
 tests/xfs/634     | 35 +++++++++++++++++++++++++++++++++++
 tests/xfs/634.out |  3 +++
 4 files changed, 59 insertions(+), 2 deletions(-)
 create mode 100755 tests/xfs/634
 create mode 100644 tests/xfs/634.out

--
2.34.1

[PATCH v1 1/3] xfs/539: Skip noattr2 remount option on v5 filesystems

[Nirjhar Roy (IBM)]

This test is to verify that repeated warnings are not printed
for default options (attr2, noikeep) and warnings are
printed for non default options (noattr2, ikeep). Remount
with noattr2 fails on a v5 filesystem, so skip the mount option.

Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
---
 tests/xfs/539 | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tests/xfs/539 b/tests/xfs/539
index b9bb7cc1..58eead67 100755
--- a/tests/xfs/539
+++ b/tests/xfs/539
@@ -42,7 +42,8 @@ echo "Silence is golden."
 
 # Skip old kernels that did not print the warning yet
 log_tag
-_scratch_mkfs > $seqres.full 2>&1
+is_v5=true
+_scratch_mkfs |& grep -q "crc=0" && is_v5=false >> $seqres.full 2>&1
 _scratch_mount -o attr2
 _scratch_unmount
 check_dmesg_for_since_tag "XFS: attr2 mount option is deprecated" || \
@@ -60,8 +61,13 @@ for VAR in {attr2,noikeep}; do
 		echo "Should not be able to find deprecation warning for $VAR"
 done
 for VAR in {noattr2,ikeep}; do
+	if [[ "$VAR" == "noattr2" ]] && $is_v5; then
+		echo "remount with noattr2 will fail in v5 filesystem. Skip" \
+			>> $seqres.full
+		continue
+	fi
 	log_tag
-	_scratch_remount $VAR
+    _scratch_remount $VAR >> $seqres.full 2>&1
 	check_dmesg_for_since_tag "XFS: $VAR mount option is deprecated" || \
 		echo "Could not find deprecation warning for $VAR"
 done
-- 
2.34.1

Re: [PATCH v1 1/3] xfs/539: Skip noattr2 remount option on v5 filesystems

[Dave Chinner]

On Wed, Feb 12, 2025 at 12:39:56PM +0000, Nirjhar Roy (IBM) wrote:
> This test is to verify that repeated warnings are not printed
> for default options (attr2, noikeep) and warnings are
> printed for non default options (noattr2, ikeep). Remount
> with noattr2 fails on a v5 filesystem, so skip the mount option.

Why do we care if remount succeeds or fails? That's not what the
test is exercising.

i.e. We are testing to see if the appropriate deprecation warning
for a deprecated mount option has been issued or not, and that
should happen regardless of whether the mount option is valid or not
for the given filesysetm format....

Hence I don't see any reason for changing the test to exclude
noattr2 testing on v5 filesystems...

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v1 1/3] xfs/539: Skip noattr2 remount option on v5 filesystems

[Nirjhar Roy (IBM)]

On 2/13/25 02:31, Dave Chinner wrote:
> On Wed, Feb 12, 2025 at 12:39:56PM +0000, Nirjhar Roy (IBM) wrote:
>> This test is to verify that repeated warnings are not printed
>> for default options (attr2, noikeep) and warnings are
>> printed for non default options (noattr2, ikeep). Remount
>> with noattr2 fails on a v5 filesystem, so skip the mount option.
> Why do we care if remount succeeds or fails? That's not what the
> test is exercising.
>
> i.e. We are testing to see if the appropriate deprecation warning
> for a deprecated mount option has been issued or not, and that
> should happen regardless of whether the mount option is valid or not
> for the given filesysetm format....

Okay, thank you for the clarification. Also, based on your response on 
patch 3/3, remount with noattr2(or any other invalid remount options), 
should be silently ignored, so this patch won't be necessary. However, 
we have observed failure of the test xfs/539 because remount with 
noattr2 was failing with CONFIG_XFS_SUPPORT_V4=n on v5 xfs and this 
failure looks like a kernel bug. More on this on my reply[1] to your 
comments on patch 3/3.

[1] 
https://lore.kernel.org/all/b43e4cd9-d8aa-4cc0-a5ff-35f2e0553682@gmail.com/

>
> Hence I don't see any reason for changing the test to exclude
> noattr2 testing on v5 filesystems...
>
> -Dave.

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v1 1/3] xfs/539: Skip noattr2 remount option on v5 filesystems

[Nirjhar Roy (IBM)]

On 2/13/25 02:31, Dave Chinner wrote:
> On Wed, Feb 12, 2025 at 12:39:56PM +0000, Nirjhar Roy (IBM) wrote:
>> This test is to verify that repeated warnings are not printed
>> for default options (attr2, noikeep) and warnings are
>> printed for non default options (noattr2, ikeep). Remount
>> with noattr2 fails on a v5 filesystem, so skip the mount option.
> Why do we care if remount succeeds or fails? That's not what the
> test is exercising.
>
> i.e. We are testing to see if the appropriate deprecation warning
> for a deprecated mount option has been issued or not, and that
> should happen regardless of whether the mount option is valid or not
> for the given filesysetm format....
>
> Hence I don't see any reason for changing the test to exclude
> noattr2 testing on v5 filesystems...

Yes, this makes sense. The test indeed just checks for the dmesg 
warnings, and they appear even if the remount fails. I wrote the patch 
because xfs/539 has started failing in one of our fstests CI runs 
because RHEL 10 has started disabling xfs v4 support i.e, 
CONFIG_XFS_SUPPORT_V4=n. Do you think modifying this patch in such a way 
that the test ignores the remount failures with noattr2 and continues 
the test is an appropriate idea (since the test xfs/539 only intends to 
check the dmesg warnings)? So something like:,

--- a/tests/xfs/539
+++ b/tests/xfs/539
@@ -61,7 +61,11 @@ for VAR in {attr2,noikeep}; do
  done
  for VAR in {noattr2,ikeep}; do
      log_tag
-    _scratch_remount $VAR
+    _scratch_remount $VAR >> $seqres.full 2>&1
+    if [[ "$VAR" == "noattr2" && "$?" != "0" ]]; then
+        echo "remount will fail in v5 filesystem but the warning should 
be printed" \
+            >> $seqres.full
+    fi
      check_dmesg_for_since_tag "XFS: $VAR mount option is deprecated" || \
          echo "Could not find deprecation warning for $VAR"

I also suggested something similar in one of my previous replies[1] in 
this patch series. Can you please let me know your thoughts on this?

[1] 
https://lore.kernel.org/all/90be3350-67e5-4dec-bc65-442762f5f856@gmail.com/

--NR

>
> -Dave.

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v1 1/3] xfs/539: Skip noattr2 remount option on v5 filesystems

[Darrick J. Wong]

On Wed, Feb 12, 2025 at 12:39:56PM +0000, Nirjhar Roy (IBM) wrote:
> This test is to verify that repeated warnings are not printed
> for default options (attr2, noikeep) and warnings are
> printed for non default options (noattr2, ikeep). Remount
> with noattr2 fails on a v5 filesystem, so skip the mount option.
> 
> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> ---
>  tests/xfs/539 | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/xfs/539 b/tests/xfs/539
> index b9bb7cc1..58eead67 100755
> --- a/tests/xfs/539
> +++ b/tests/xfs/539
> @@ -42,7 +42,8 @@ echo "Silence is golden."
>  
>  # Skip old kernels that did not print the warning yet
>  log_tag
> -_scratch_mkfs > $seqres.full 2>&1
> +is_v5=true
> +_scratch_mkfs |& grep -q "crc=0" && is_v5=false >> $seqres.full 2>&1

Usually we do this with something more like:

_scratch_mkfs | _filter_mkfs >>$seqres.full 2>$tmp.mkfs
. $tmp.mkfs

if [ $_fs_has_crcs -eq 1 ]; then
	# v5 stuff
else
	# v4 stuff
endif

>  _scratch_mount -o attr2
>  _scratch_unmount
>  check_dmesg_for_since_tag "XFS: attr2 mount option is deprecated" || \
> @@ -60,8 +61,13 @@ for VAR in {attr2,noikeep}; do
>  		echo "Should not be able to find deprecation warning for $VAR"
>  done
>  for VAR in {noattr2,ikeep}; do
> +	if [[ "$VAR" == "noattr2" ]] && $is_v5; then
> +		echo "remount with noattr2 will fail in v5 filesystem. Skip" \
> +			>> $seqres.full
> +		continue

/me wonders if it'd be cleaner to do:

VARS=(ikeep)
test $_fs_has_crcs -eq 0 && VARS+=(noattr2)

for VAR in "${VARS[@]}"; do
	...
done

> +	fi
>  	log_tag
> -	_scratch_remount $VAR
> +    _scratch_remount $VAR >> $seqres.full 2>&1

Nit: Indentation.

--D

>  	check_dmesg_for_since_tag "XFS: $VAR mount option is deprecated" || \
>  		echo "Could not find deprecation warning for $VAR"
>  done
> -- 
> 2.34.1
> 
> 

[PATCH v1 2/3] common/xfs: Add a new helper function to check v5 XFS

[Nirjhar Roy (IBM)]

This commit adds a new helper to function to check that we can
create a V5 XFS filesystem in the scratch device

Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
---
 common/xfs | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/common/xfs b/common/xfs
index 0417a40a..cc0a62e4 100644
--- a/common/xfs
+++ b/common/xfs
@@ -468,6 +468,19 @@ _require_scratch_xfs_crc()
 	_scratch_unmount
 }
 
+# this test requires the xfs kernel support crc feature on scratch device
+#
+_require_scratch_xfs_v5()
+{
+	_require_scratch_xfs_crc
+	_scratch_mkfs_xfs -m crc=1 > $seqres.full 2>&1 ||
+		_notrun "v5 filesystem isn't supported by the kernel"
+	_try_scratch_mount >/dev/null 2>&1
+	ret="$?"
+	_scratch_unmount
+	[[ "$ret" != "0" ]] && _notrun "couldn't mount a V5 xfs filesystem"
+}
+
 # this test requires the finobt feature to be available in mkfs.xfs
 #
 _require_xfs_mkfs_finobt()
-- 
2.34.1

Re: [PATCH v1 2/3] common/xfs: Add a new helper function to check v5 XFS

[Dave Chinner]

On Wed, Feb 12, 2025 at 12:39:57PM +0000, Nirjhar Roy (IBM) wrote:
> This commit adds a new helper to function to check that we can
> create a V5 XFS filesystem in the scratch device
> 
> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> ---
>  common/xfs | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/common/xfs b/common/xfs
> index 0417a40a..cc0a62e4 100644
> --- a/common/xfs
> +++ b/common/xfs
> @@ -468,6 +468,19 @@ _require_scratch_xfs_crc()
>  	_scratch_unmount
>  }
>  
> +# this test requires the xfs kernel support crc feature on scratch device
> +#
> +_require_scratch_xfs_v5()
> +{
> +	_require_scratch_xfs_crc
> +	_scratch_mkfs_xfs -m crc=1 > $seqres.full 2>&1 ||
> +		_notrun "v5 filesystem isn't supported by the kernel"

This is testing mkfs.xfs, not the kernel.

We already have a helper for that: _scratch_mkfs_xfs_supported()

> +	_try_scratch_mount >/dev/null 2>&1
> +	ret="$?"
> +	_scratch_unmount
> +	[[ "$ret" != "0" ]] && _notrun "couldn't mount a V5 xfs filesystem"
> +}

This doesn't actually check that the mounted filesystem is a v5
format filesystem. That's what _require_scratch_xfs_crc() does.

Hence I don't see what this adds over _require_scratch_xfs_crc(),
which does the right thing on any mkfs.xfs released in the past
decade (i.e. when we changed mkfs.xfs to create v5 filesystems by
default). 

What test environment doesn't _require_scratch_xfs_crc() work for?

-Dave.
-- 
Dave Chinner
david@fromorbit.com

[PATCH v1 3/3] xfs: Add a testcase to check remount with noattr2 on a v5 xfs

[Nirjhar Roy (IBM)]

This testcase reproduces the following bug:
Bug:
mount -o remount,noattr2 <device> <mount_point> succeeds
unexpectedly on a v5 xfs when CONFIG_XFS_SUPPORT_V4 is set.

Ideally the above mount command should always fail with a v5 xfs
filesystem irrespective of whether CONFIG_XFS_SUPPORT_V4 is set
or not.

Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
---
 tests/xfs/634     | 35 +++++++++++++++++++++++++++++++++++
 tests/xfs/634.out |  3 +++
 2 files changed, 38 insertions(+)
 create mode 100755 tests/xfs/634
 create mode 100644 tests/xfs/634.out

diff --git a/tests/xfs/634 b/tests/xfs/634
new file mode 100755
index 00000000..dc153047
--- /dev/null
+++ b/tests/xfs/634
@@ -0,0 +1,35 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2025 Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>.  All Rights Reserved.
+#
+# FS QA Test 634
+#
+# This test checks that mounting and remounting a v5 xfs filesystem with
+# noattr2 fails. Currently, this test will pass with CONFIG_XFS_SUPPORT_V4=n but
+# with CONFIG_XFS_SUPPORT_V4=y, this will fail i.e, mount -o remount,noattr2
+# command succeeds incorrectly.
+#
+. ./common/preamble
+. ./common/filter
+
+_begin_fstest auto quick mount
+_require_scratch
+# Import common functions.
+
+_fixed_by_kernel_commit xxxx \
+	""
+_require_scratch_xfs_v5
+_scratch_mkfs -m crc=1 > $seqres.full 2>&1 ||
+	_notrun "need an xfs v5 filesystem"
+_scratch_mount "-o noattr2" |& grep -iq "fail" && \
+	echo "mount failed successfully"
+_scratch_mount
+! _scratch_remount noattr2 >> $seqres.full 2>&1 && \
+	echo "remount failed successfully"
+_scratch_unmount
+
+# success, all done
+status=0
+exit
+status=0
+exit
diff --git a/tests/xfs/634.out b/tests/xfs/634.out
new file mode 100644
index 00000000..8a98c05c
--- /dev/null
+++ b/tests/xfs/634.out
@@ -0,0 +1,3 @@
+QA output created by 634
+mount failed successfully
+remount failed successfully
-- 
2.34.1

Re: [PATCH v1 3/3] xfs: Add a testcase to check remount with noattr2 on a v5 xfs

[Dave Chinner]

On Wed, Feb 12, 2025 at 12:39:58PM +0000, Nirjhar Roy (IBM) wrote:
> This testcase reproduces the following bug:
> Bug:
> mount -o remount,noattr2 <device> <mount_point> succeeds
> unexpectedly on a v5 xfs when CONFIG_XFS_SUPPORT_V4 is set.

AFAICT, this is expected behaviour. Remount intentionally ignores
options that cannot be changed.

> Ideally the above mount command should always fail with a v5 xfs
> filesystem irrespective of whether CONFIG_XFS_SUPPORT_V4 is set
> or not.

No, we cannot fail remount when invalid options are passed to the
kernel by the mount command for historical reasons. i.e. the mount
command has historically passed invalid options to the kernel on
remount, but expects the kernel to apply just the new options that
they understand and ignore the rest without error.

i.e. to keep compatibility with older userspace, we cannot fail a
remount because userspace passed an option the kernel does not
understand or cannot change.

Hence, in this case, XFS emits a deprecation warning for the noattr2
mount option on remount (because it is understood), then ignores
because it it isn't a valid option that remount can change.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v1 3/3] xfs: Add a testcase to check remount with noattr2 on a v5 xfs

[Nirjhar Roy (IBM)]

On 2/13/25 03:17, Dave Chinner wrote:
> On Wed, Feb 12, 2025 at 12:39:58PM +0000, Nirjhar Roy (IBM) wrote:
>> This testcase reproduces the following bug:
>> Bug:
>> mount -o remount,noattr2 <device> <mount_point> succeeds
>> unexpectedly on a v5 xfs when CONFIG_XFS_SUPPORT_V4 is set.
> AFAICT, this is expected behaviour. Remount intentionally ignores
> options that cannot be changed.
>
>> Ideally the above mount command should always fail with a v5 xfs
>> filesystem irrespective of whether CONFIG_XFS_SUPPORT_V4 is set
>> or not.
> No, we cannot fail remount when invalid options are passed to the
> kernel by the mount command for historical reasons. i.e. the mount
> command has historically passed invalid options to the kernel on
> remount, but expects the kernel to apply just the new options that
> they understand and ignore the rest without error.
>
> i.e. to keep compatibility with older userspace, we cannot fail a
> remount because userspace passed an option the kernel does not
> understand or cannot change.
>
> Hence, in this case, XFS emits a deprecation warning for the noattr2
> mount option on remount (because it is understood), then ignores
> because it it isn't a valid option that remount can change.

Thank you, Dave, for the background. This was really helpful. So just to 
confirm the behavior of mount - remount with noattr2 (or any other 
invalid option) should always pass irrespective of whether 
CONFIG_XFS_SUPPORT_V4 is set or not, correct?

This is the behavior that I have observed with CONFIG_XFS_SUPPORT_V4=n 
on v5 xfs:

$ mount -o "remount,noattr2" /dev/loop0 /mnt1/test
mount: /mnt1/test: mount point not mounted or bad option.
$ echo "$?"
32

With this test, I am also parallelly working on a kernel fix to make the 
behavior of remount with noattr2 same irrespective of the 
CONFIG_XFS_SUPPORT_V4's value, and I was under the impression that it 
should always fail. But, it seems like it should always pass (silently 
ignoring the invalid mount options) and the failure when 
CONFIG_XFS_SUPPORT_V4=n is a bug. Is my understanding correct?

--NR

>
> -Dave.

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v1 3/3] xfs: Add a testcase to check remount with noattr2 on a v5 xfs

[Dave Chinner]

On Thu, Feb 13, 2025 at 03:30:50PM +0530, Nirjhar Roy (IBM) wrote:
> 
> On 2/13/25 03:17, Dave Chinner wrote:
> > On Wed, Feb 12, 2025 at 12:39:58PM +0000, Nirjhar Roy (IBM) wrote:
> > > This testcase reproduces the following bug:
> > > Bug:
> > > mount -o remount,noattr2 <device> <mount_point> succeeds
> > > unexpectedly on a v5 xfs when CONFIG_XFS_SUPPORT_V4 is set.
> > AFAICT, this is expected behaviour. Remount intentionally ignores
> > options that cannot be changed.
> > 
> > > Ideally the above mount command should always fail with a v5 xfs
> > > filesystem irrespective of whether CONFIG_XFS_SUPPORT_V4 is set
> > > or not.
> > No, we cannot fail remount when invalid options are passed to the
> > kernel by the mount command for historical reasons. i.e. the mount
> > command has historically passed invalid options to the kernel on
> > remount, but expects the kernel to apply just the new options that
> > they understand and ignore the rest without error.
> > 
> > i.e. to keep compatibility with older userspace, we cannot fail a
> > remount because userspace passed an option the kernel does not
> > understand or cannot change.
> > 
> > Hence, in this case, XFS emits a deprecation warning for the noattr2
> > mount option on remount (because it is understood), then ignores
> > because it it isn't a valid option that remount can change.
> 
> Thank you, Dave, for the background. This was really helpful. So just to
> confirm the behavior of mount - remount with noattr2 (or any other invalid
> option) should always pass irrespective of whether CONFIG_XFS_SUPPORT_V4 is
> set or not, correct?

Not necessarily.

It depends on whether the filesystem considers it a known option or
not. noattr2 is a known option, so if it is invalid to use it as a
remount option, the remount should always fail.

If the option is -unknown-, then the behaviour of remount is largely
dependent on filesystem implementation -and- what mount syscall
interface is being used by userspace.

e.g. a modern mount binary using
fsconfig(2) allows the kernel to reject unknown options before the
filesystem is remounted. However, we cannot do that with the
mount(2) interface because of the historic behaviour of the mount
binary (see the comment above xfs_fs_reconfigure() about this).

Hence with a modern mount binary using the fsconfig(2) interface,
the kernel can actually reject bad/unknown mount options without
breaking anything. i.e. kernel behaviour is dependent on userspace
implementation...

> This is the behavior that I have observed with CONFIG_XFS_SUPPORT_V4=n on v5
> xfs:
> 
> $ mount -o "remount,noattr2" /dev/loop0 /mnt1/test
> mount: /mnt1/test: mount point not mounted or bad option.
> $ echo "$?"
> 32

This is not useful in itself because of all the above possibilities.
i.e. What generated that error?

Was if from the mount binary, or the kernel?  What syscall is mount
using - strace output will tell us if it is fsconfig(2) or mount(2)
and what is being passed to the kernel.  What does dmesg say - did
the kernel parse the option and then fail, or something else?

i.e. this is actually really hard to write a kernel and userspace
version agnostic regression test for.

> With this test, I am also parallelly working on a kernel fix to make the
> behavior of remount with noattr2 same irrespective of the
> CONFIG_XFS_SUPPORT_V4's value, and I was under the impression that it should
> always fail. But, it seems like it should always pass (silently ignoring the
> invalid mount options) and the failure when CONFIG_XFS_SUPPORT_V4=n is a
> bug. Is my understanding correct?

As per above, the behaviour we expose to userspace is actually
dependent on the syscall interface the mount is using.

That said, I still don't see why CONFIG_XFS_SUPPORT_V4 would change
how we parse and process noattr2.....

.... Ohhh.

The new xfs_mount being used for reconfiguring the
superblock on remount doesn't have the superblock feature
flags initialised. attr2 is defined as:

__XFS_ADD_V4_FEAT(attr2, ATTR2)

Which means if CONFIG_XFS_SUPPORT_V4=n it will always return true.

However, if CONFIG_XFS_SUPPORT_V4=y, then it checks for the ATTR2
feature flag in the xfs_mount.

Hence when we are validating the noattr2 flag in
xfs_fs_validate_params(), this check:

	/*                                                                       
         * We have not read the superblock at this point, so only the attr2      
         * mount option can set the attr2 feature by this stage.                 
         */                                                                      
        if (xfs_has_attr2(mp) && xfs_has_noattr2(mp)) {                          
                xfs_warn(mp, "attr2 and noattr2 cannot both be specified.");     
                return -EINVAL;                                                  
        }

Never triggers on remount when CONFIG_XFS_SUPPORT_V4=y because
xfs_has_attr2(mp) is always false.  OTOH, when
CONFIG_XFS_SUPPORT_V4=n, xfs_has_attr2(mp) is always true because of
the __XFS_ADD_V4_FEAT() macro implementation, and so now it rejects
the noattr2 mount option because it isn't valid on a v5 filesystem.

Ok, so CONFIG_XFS_SUPPORT_V4=n is the correct behaviour (known mount
option, invalid configuration being asked for), and it is the
CONFIG_XFS_SUPPORT_V4=y behaviour that is broken.

This likely has been broken since the mount option parsing was
first changed to use the fscontext interfaces....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v1 3/3] xfs: Add a testcase to check remount with noattr2 on a v5 xfs

[Nirjhar Roy (IBM)]

On 2/14/25 03:19, Dave Chinner wrote:
> On Thu, Feb 13, 2025 at 03:30:50PM +0530, Nirjhar Roy (IBM) wrote:
>> On 2/13/25 03:17, Dave Chinner wrote:
>>> On Wed, Feb 12, 2025 at 12:39:58PM +0000, Nirjhar Roy (IBM) wrote:
>>>> This testcase reproduces the following bug:
>>>> Bug:
>>>> mount -o remount,noattr2 <device> <mount_point> succeeds
>>>> unexpectedly on a v5 xfs when CONFIG_XFS_SUPPORT_V4 is set.
>>> AFAICT, this is expected behaviour. Remount intentionally ignores
>>> options that cannot be changed.
>>>
>>>> Ideally the above mount command should always fail with a v5 xfs
>>>> filesystem irrespective of whether CONFIG_XFS_SUPPORT_V4 is set
>>>> or not.
>>> No, we cannot fail remount when invalid options are passed to the
>>> kernel by the mount command for historical reasons. i.e. the mount
>>> command has historically passed invalid options to the kernel on
>>> remount, but expects the kernel to apply just the new options that
>>> they understand and ignore the rest without error.
>>>
>>> i.e. to keep compatibility with older userspace, we cannot fail a
>>> remount because userspace passed an option the kernel does not
>>> understand or cannot change.
>>>
>>> Hence, in this case, XFS emits a deprecation warning for the noattr2
>>> mount option on remount (because it is understood), then ignores
>>> because it it isn't a valid option that remount can change.
>> Thank you, Dave, for the background. This was really helpful. So just to
>> confirm the behavior of mount - remount with noattr2 (or any other invalid
>> option) should always pass irrespective of whether CONFIG_XFS_SUPPORT_V4 is
>> set or not, correct?
> Not necessarily.
>
> It depends on whether the filesystem considers it a known option or
> not. noattr2 is a known option, so if it is invalid to use it as a
> remount option, the remount should always fail.
>
> If the option is -unknown-, then the behaviour of remount is largely
> dependent on filesystem implementation -and- what mount syscall
> interface is being used by userspace.
>
> e.g. a modern mount binary using
> fsconfig(2) allows the kernel to reject unknown options before the
> filesystem is remounted. However, we cannot do that with the
> mount(2) interface because of the historic behaviour of the mount
> binary (see the comment above xfs_fs_reconfigure() about this).
Okay, I will look into the comments above xfs_fs_reconfigure(). Thank 
you for the pointer.
>
> Hence with a modern mount binary using the fsconfig(2) interface,
> the kernel can actually reject bad/unknown mount options without
> breaking anything. i.e. kernel behaviour is dependent on userspace
> implementation...
>
>> This is the behavior that I have observed with CONFIG_XFS_SUPPORT_V4=n on v5
>> xfs:
>>
>> $ mount -o "remount,noattr2" /dev/loop0 /mnt1/test
>> mount: /mnt1/test: mount point not mounted or bad option.
>> $ echo "$?"
>> 32
> This is not useful in itself because of all the above possibilities.
> i.e. What generated that error?
>
> Was if from the mount binary, or the kernel?  What syscall is mount
> using - strace output will tell us if it is fsconfig(2) or mount(2)
> and what is being passed to the kernel.  What does dmesg say - did
> the kernel parse the option and then fail, or something else?
>
> i.e. this is actually really hard to write a kernel and userspace
> version agnostic regression test for.
>
>> With this test, I am also parallelly working on a kernel fix to make the
>> behavior of remount with noattr2 same irrespective of the
>> CONFIG_XFS_SUPPORT_V4's value, and I was under the impression that it should
>> always fail. But, it seems like it should always pass (silently ignoring the
>> invalid mount options) and the failure when CONFIG_XFS_SUPPORT_V4=n is a
>> bug. Is my understanding correct?
> As per above, the behaviour we expose to userspace is actually
> dependent on the syscall interface the mount is using.
>
> That said, I still don't see why CONFIG_XFS_SUPPORT_V4 would change
> how we parse and process noattr2.....
>
> .... Ohhh.
>
> The new xfs_mount being used for reconfiguring the
> superblock on remount doesn't have the superblock feature
> flags initialised. attr2 is defined as:
>
> __XFS_ADD_V4_FEAT(attr2, ATTR2)
>
> Which means if CONFIG_XFS_SUPPORT_V4=n it will always return true.
>
> However, if CONFIG_XFS_SUPPORT_V4=y, then it checks for the ATTR2
> feature flag in the xfs_mount.
>
> Hence when we are validating the noattr2 flag in
> xfs_fs_validate_params(), this check:
>
> 	/*
>           * We have not read the superblock at this point, so only the attr2
>           * mount option can set the attr2 feature by this stage.
>           */
>          if (xfs_has_attr2(mp) && xfs_has_noattr2(mp)) {
>                  xfs_warn(mp, "attr2 and noattr2 cannot both be specified.");
>                  return -EINVAL;
>          }
>
> Never triggers on remount when CONFIG_XFS_SUPPORT_V4=y because
> xfs_has_attr2(mp) is always false.  OTOH, when
> CONFIG_XFS_SUPPORT_V4=n, xfs_has_attr2(mp) is always true because of
> the __XFS_ADD_V4_FEAT() macro implementation, and so now it rejects
> the noattr2 mount option because it isn't valid on a v5 filesystem.
Yes, that is correct. This is my analysis too.
>
> Ok, so CONFIG_XFS_SUPPORT_V4=n is the correct behaviour (known mount
> option, invalid configuration being asked for), and it is the
> CONFIG_XFS_SUPPORT_V4=y behaviour that is broken.

Okay, so do you find this testcase (patch 3/3 xfs: Add a testcase to 
check remount with noattr2 on a v5 xfs) useful, and shall I work on the 
corresponding kernel fix for it? I can make the change in "[patch1/3] 
xfs/539: Skip noattr2 remount option on v5 filesystems" to ignore the 
mount failures (since that test is checking for dmesg warnings), what do 
you think? Do you have any other suggestions?

--NR

>
> This likely has been broken since the mount option parsing was
> first changed to use the fscontext interfaces....
>
> -Dave.

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore

Re: [PATCH v1 3/3] xfs: Add a testcase to check remount with noattr2 on a v5 xfs

[Dave Chinner]

On Mon, Feb 17, 2025 at 10:18:48AM +0530, Nirjhar Roy (IBM) wrote:
> On 2/14/25 03:19, Dave Chinner wrote:
> > On Thu, Feb 13, 2025 at 03:30:50PM +0530, Nirjhar Roy (IBM) wrote:
> > > On 2/13/25 03:17, Dave Chinner wrote:
> > > > On Wed, Feb 12, 2025 at 12:39:58PM +0000, Nirjhar Roy (IBM) wrote:
> > Ok, so CONFIG_XFS_SUPPORT_V4=n is the correct behaviour (known mount
> > option, invalid configuration being asked for), and it is the
> > CONFIG_XFS_SUPPORT_V4=y behaviour that is broken.
> 
> Okay, so do you find this testcase (patch 3/3 xfs: Add a testcase to check
> remount with noattr2 on a v5 xfs) useful,

Not at this point in time, because xfs/189 is supposed to exercise
attr2/noattr2 mount/remount behaviour and take into account all the
weirdness of the historic mount behaviour.

Obviously, it is not detecting that this noattr2 remount behaviour
was broken, so that test needs fixing/additions.  Indeed, it's
probably important to understand why xfs/189 isn't detecting this
failure before going any further, right?

IMO, it is better to fix existing tests that exercise the behaviour
in question than it is to add a new test that covers just what the
old test missed.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH v1 3/3] xfs: Add a testcase to check remount with noattr2 on a v5 xfs

[Nirjhar Roy (IBM)]

On 2/18/25 03:59, Dave Chinner wrote:
> On Mon, Feb 17, 2025 at 10:18:48AM +0530, Nirjhar Roy (IBM) wrote:
>> On 2/14/25 03:19, Dave Chinner wrote:
>>> On Thu, Feb 13, 2025 at 03:30:50PM +0530, Nirjhar Roy (IBM) wrote:
>>>> On 2/13/25 03:17, Dave Chinner wrote:
>>>>> On Wed, Feb 12, 2025 at 12:39:58PM +0000, Nirjhar Roy (IBM) wrote:
>>> Ok, so CONFIG_XFS_SUPPORT_V4=n is the correct behaviour (known mount
>>> option, invalid configuration being asked for), and it is the
>>> CONFIG_XFS_SUPPORT_V4=y behaviour that is broken.
>> Okay, so do you find this testcase (patch 3/3 xfs: Add a testcase to check
>> remount with noattr2 on a v5 xfs) useful,
> Not at this point in time, because xfs/189 is supposed to exercise
> attr2/noattr2 mount/remount behaviour and take into account all the
> weirdness of the historic mount behaviour.
>
> Obviously, it is not detecting that this noattr2 remount behaviour
> was broken, so that test needs fixing/additions.  Indeed, it's
> probably important to understand why xfs/189 isn't detecting this
> failure before going any further, right?
Yes. Let me look into what xfs/189 does and why it isn't detecting the 
noattr2 remount broken behavior. Thank you for the pointer.

About "Patch 1/3: xfs/539: Skip noattr2 remount option on v5 file 
systems" --> I wrote the patch because xfs/539 has started failing in 
one of fstests CI runs because RHEL 10 has started disabling xfs v4 
support i.e, CONFIG_XFS_SUPPORT_V4=n. Do you think modifying patch 
1/3(xfs/539) in such a way that the test ignores the remount failures 
with noattr2 and continues the test is an appropriate idea (since the 
test xfs/539 only intends to check the dmesg warnings)?

--NR

>
> IMO, it is better to fix existing tests that exercise the behaviour
> in question than it is to add a new test that covers just what the
> old test missed.
>
> -Dave.

-- 
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore