On Mon, May 12, 2025 at 03:27:14PM +0530, Nirjhar Roy (IBM) wrote:
> Bug: When we compile the kernel with CONFIG_XFS_SUPPORT_V4=y,
> remount with "-o remount,noattr2" on a v5 XFS does not
> fail explicitly.
>
> Reproduction:
> mkfs.xfs -f /dev/loop0
> mount /dev/loop0 /mnt/scratch
> mount -o remount,noattr2 /dev/loop0 /mnt/scratch
>
> However, with CONFIG_XFS_SUPPORT_V4=n, the remount
> correctly fails explicitly. This is because the way the
> following 2 functions are defined:
>
> static inline bool xfs_has_attr2 (struct xfs_mount *mp)
> {
> return !IS_ENABLED(CONFIG_XFS_SUPPORT_V4) ||
> (mp->m_features & XFS_FEAT_ATTR2);
> }
> static inline bool xfs_has_noattr2 (const struct xfs_mount *mp)
> {
> return mp->m_features & XFS_FEAT_NOATTR2;
> }
>
> xfs_has_attr2() returns true when CONFIG_XFS_SUPPORT_V4=n
> and hence, 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.
>
> Fix: Check if the existing mount has crc enabled(i.e, of
> type v5 and has attr2 enabled) and the
> remount has noattr2, if yes, return -EINVAL.
>
> I have tested xfs/{189,539} in fstests with v4
> and v5 XFS with both CONFIG_XFS_SUPPORT_V4=y/n and
> they both behave as expected.
>
> This patch also fixes remount from noattr2 -> attr2 (on a v4 xfs).
>
> Related discussion in [1]
>
> [1] https://lore.kernel.org/all/Z65o6nWxT00MaUrW@xxxxxxxxxxxxxxxxxxx/
>
> Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@xxxxxxxxx>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/xfs_super.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index b2dd0c0bf509..606a95ac816f 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -2114,6 +2114,21 @@ xfs_fs_reconfigure(
> if (error)
> return error;
>
> + /* attr2 -> noattr2 */
> + if (xfs_has_noattr2(new_mp)) {
> + if (xfs_has_crc(mp)) {
> + xfs_warn(mp,
> + "attr2 is always enabled for a V5 filesystem - can't be changed.");
> + return -EINVAL;
This looks good to me now:
Reviewed-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
I still wish hch's opinion here though before merging it. Giving his was the
first RwB, I want to make sure he still keeps his RwB with the above change.
FWIW, for a next patch, there is no need to copy ext4 list for a code change
that is totally unrelated to ext4. This just generates unnecessary extra
traffic.
> + }
> + 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;
> + }
> +
> /* inode32 -> inode64 */
> if (xfs_has_small_inums(mp) && !xfs_has_small_inums(new_mp)) {
> mp->m_features &= ~XFS_FEAT_SMALL_INUMS;
> @@ -2126,6 +2141,17 @@ xfs_fs_reconfigure(
> mp->m_maxagi = xfs_set_inode_alloc(mp, mp->m_sb.sb_agcount);
> }
>
> + /*
> + * Now that mp has been modified according to the remount options,
> + * we do a final option validation with xfs_finish_flags()
> + * just like it is done during mount. We cannot use
> + * xfs_finish_flags()on new_mp as it contains only the user
> + * given options.
> + */
> + error = xfs_finish_flags(mp);
> + if (error)
> + return error;
> +
> /* ro -> rw */
> if (xfs_is_readonly(mp) && !(flags & SB_RDONLY)) {
> error = xfs_remount_rw(mp);
> --
> 2.43.5
>
>
