On 4/14/25 11:18, Christoph Hellwig wrote:
On Fri, Apr 11, 2025 at 11:44:52PM +0530, Nirjhar Roy (IBM) wrote:
mkfs.xfs -f /dev/loop0
mount /dev/loop0 /mnt/scratch
mount -o remount,noattr2 /dev/loop0 /mnt/scratch # This should fail but it doesn't
Please reflow your commit log to not exceed the standard 73 characters
Noted. I will update this in the next revision.
xfs_has_attr2() returns true when CONFIG_XFS_SUPPORT_V4=n and hence, the
the following if condition in xfs_fs_validate_params() succeeds and returns -EINVAL:
/*
* 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;
}
With CONFIG_XFS_SUPPORT_V4=y, xfs_has_attr2() always return false and hence no error
is returned.
But that also means the mount time check is wrong as well.
So during mount, xfs_fs_fill_super() calls the following functions are
called in sequence :
xfs_fs_validate_params()
<...>
xfs_readsb()
xfs_finish_flags().
If I am trying to "mount -o noattr2 /dev/loop0 /mnt1/test", then the
invalid condition(noattr2 on v5) is not caught in
xfs_fs_validate_params() because the superblock isn't read yet and
"struct xfs_mount *mp" is still not aware of whether the underlying
filesystem is v5 or v4 (i.e, whether crc=0 or crc=1). So, even if the
underlying filesystem is v5, xfs_has_attr2() will return false, because
the m_features isn't populated yet. However, once xfs_readsb() is done,
m_features is populated (mp->m_features |=
xfs_sb_version_to_features(sbp); called at the end of xfs_readsb()).
After that, when xfs_finish_flags() is called, the invalid mount option
(i.e, noattr2 with crc=1) is caught, and the mount fails correctly. So,
m_features is partially populated while xfs_fs_validate_params() is
getting executed, I am not sure if that is done intentionally. IMO, we
should have read the superblock, made sure that the m_features is fully
populated within xfs_fs_validate_params() with the existing
configurations of the underlying disk/fs and the ones supplied the by
mount program - this can avoid such false negatives. Can you please let
me know if my understanding is correct?
+ /* attr2 -> noattr2 */
+ if (xfs_has_noattr2(new_mp)) {
+ if (xfs_has_crc(mp)) {
+ xfs_warn(mp, "attr2 and noattr2 cannot both be specified.");
+ return -EINVAL;
+ }
So this check should probably go into xfs_fs_validate_params, and
also have a more useful warning like:
if (xfs_has_crc(mp) && xfs_has_noattr2(new_mp)) {
xfs_warn(mp,
"noattr2 cannot be specified for v5 file systems.");
return -EINVAL;
}
xfs_fs_validate_params() takes only one parameter. Are you suggesting to
add another optional (NULLable) parameter "new_mp" and add the above
check there? In that case, all other remount related checks in
xfs_fs_reconfigure() qualify to be moved to xfs_fs_validate_params(),
right? Is my understanding correct?
+ else {
+ mp->m_features &= ~XFS_FEAT_ATTR2;
+ mp->m_features |= XFS_FEAT_NOATTR2;
+ }
+
+ } else if (xfs_has_attr2(new_mp)) {
+ /* noattr2 -> attr2 */
+ mp->m_features &= ~XFS_FEAT_NOATTR2;
+ mp->m_features |= XFS_FEAT_ATTR2;
+ }
Some of the indentation here looks broken. Please always use one
tab per indentation level, places the closing brace before the else,
and don't use else after a return statement.
Okay, I will fix this in the next revision. Thank you for pointing this
out.
--NR
--
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore
