Hi Nirjhar.
The patch looks fine, with a caveat below.
On Wed, May 07, 2025 at 12:59:13PM +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..58a0431ab52d 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 and noattr2 cannot both be specified.");
This message doesn't seem to make sense to me. Your code checks the attr2 option
is not being changed on a V5 FS, but your error message states both mount
options are bing specified at the command line, confusing the user.
V5 format always use attr2, and can't use noattr2. So, this message is
misleading.
IMO this should be something like:
"attr2 is always enabled for for a V5 filesystem and can't be changed."
Carlos
> + return -EINVAL;
> + }
> + 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
>
>
