Re: [PATCH 1/2] xfs: remove deprecated mount options

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Thu, Sep 24, 2020 at 07:07:46PM +0200, Pavel Reichl wrote:
> ikeep/noikeep was a workaround for old DMAPI code which is no longer
> relevant.
> 
> attr2/noattr2 - is for controlling upgrade behaviour from fixed attribute
> fork sizes in the inode (attr1) and dynamic attribute fork sizes (attr2).
> mkfs has defaulted to setting attr2 since 2007, hence just about every
> XFS filesystem out there in production right now uses attr2.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  Documentation/admin-guide/xfs.rst |  2 ++
>  fs/xfs/xfs_super.c                | 30 +++++++++++++++++-------------
>  2 files changed, 19 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
> index f461d6c33534..413f68efccc0 100644
> --- a/Documentation/admin-guide/xfs.rst
> +++ b/Documentation/admin-guide/xfs.rst
> @@ -217,6 +217,8 @@ Deprecated Mount Options
>  ===========================     ================
>    Name				Removal Schedule
>  ===========================     ================
> +  ikeep/noikeep			TBD
> +  attr2/noattr2			TBD

Er... what date did you have in mind?

>  ===========================     ================
>  
>  
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 71ac6c1cdc36..4c26b283b7d8 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1234,25 +1234,12 @@ xfs_fc_parse_param(
>  	case Opt_nouuid:
>  		mp->m_flags |= XFS_MOUNT_NOUUID;
>  		return 0;
> -	case Opt_ikeep:
> -		mp->m_flags |= XFS_MOUNT_IKEEP;
> -		return 0;
> -	case Opt_noikeep:
> -		mp->m_flags &= ~XFS_MOUNT_IKEEP;
> -		return 0;
>  	case Opt_largeio:
>  		mp->m_flags |= XFS_MOUNT_LARGEIO;
>  		return 0;
>  	case Opt_nolargeio:
>  		mp->m_flags &= ~XFS_MOUNT_LARGEIO;
>  		return 0;
> -	case Opt_attr2:
> -		mp->m_flags |= XFS_MOUNT_ATTR2;
> -		return 0;
> -	case Opt_noattr2:
> -		mp->m_flags &= ~XFS_MOUNT_ATTR2;
> -		mp->m_flags |= XFS_MOUNT_NOATTR2;
> -		return 0;
>  	case Opt_filestreams:
>  		mp->m_flags |= XFS_MOUNT_FILESTREAMS;
>  		return 0;
> @@ -1304,6 +1291,23 @@ xfs_fc_parse_param(
>  		xfs_mount_set_dax_mode(mp, result.uint_32);
>  		return 0;
>  #endif
> +	case Opt_ikeep:
> +		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		mp->m_flags |= XFS_MOUNT_IKEEP;

It's a little odd that you didn't then remove these XFS_MOUNT_ flags.
It's strange to declare a mount option deprecated but still have it
change behavior.

In this case, I guess we should keep ikeep/noikeep in the mount options
table so that scripts won't fail, but then we remove XFS_MOUNT_IKEEP and
change the codebase to always take the IKEEP behavior and delete the
code that handled the !IKEEP behavior.

> +		return 0;
> +	case Opt_noikeep:
> +		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		mp->m_flags &= ~XFS_MOUNT_IKEEP;
> +		return 0;
> +	case Opt_attr2:
> +		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		mp->m_flags |= XFS_MOUNT_ATTR2;

If the kernel /does/ encounter an attr1 filesystem, what will it do now?
IIRC the default (if there is no attr2/noattr2 mount option) is to
auto-upgrade the fs, right?  So will we stop doing that, or are we
making the upgrade mandatory now?

> +		return 0;
> +	case Opt_noattr2:
> +		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		mp->m_flags &= ~XFS_MOUNT_ATTR2;
> +		mp->m_flags |= XFS_MOUNT_NOATTR2;

Also, uh, why move these code hunks?

--D

> +		return 0;
>  	default:
>  		xfs_warn(mp, "unknown mount option [%s].", param->key);
>  		return -EINVAL;
> -- 
> 2.26.2
>