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
> 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.
> + /* 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;
}
> + 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.
