[PATCH 0/2] xfs: remove deprecated mount and sysctl options

[PATCH 0/2] xfs: remove deprecated mount and sysctl options

[Pavel Reichl]

Hi,

by Eric and Dave's suggestion I prepared a patchset which adds warnings about
using deprecated options. I tried to justify the changes in commit
messages based on the info from Eric and Dave.

If this patchsed should be merged I need to know when the options are
actually eliminated, so documentation can be properly updated.

Thanks.

Pavel Reichl (2):
  xfs: remove deprecated mount options
  xfs: remove deprecated sysctl options

 Documentation/admin-guide/xfs.rst |  5 ++++-
 fs/xfs/xfs_super.c                | 30 +++++++++++++++-----------
 fs/xfs/xfs_sysctl.c               | 36 +++++++++++++++++++++++++++++--
 3 files changed, 55 insertions(+), 16 deletions(-)

-- 
2.26.2

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

[Pavel Reichl]

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
 ===========================     ================
 
 
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;
+		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;
+		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;
+		return 0;
 	default:
 		xfs_warn(mp, "unknown mount option [%s].", param->key);
 		return -EINVAL;
-- 
2.26.2

[PATCH 2/2] xfs: remove deprecated sysctl options

[Pavel Reichl]

These optionr were for Irix compatibility, probably for clustered XFS
clients in a heterogenous cluster which contained both Irix & Linux
machines, so that behavior would be consistent. That doesn't exist anymore
and it's no longer needed.

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 Documentation/admin-guide/xfs.rst |  3 ++-
 fs/xfs/xfs_sysctl.c               | 36 +++++++++++++++++++++++++++++--
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
index 413f68efccc0..208e17810459 100644
--- a/Documentation/admin-guide/xfs.rst
+++ b/Documentation/admin-guide/xfs.rst
@@ -333,7 +333,8 @@ The following sysctls are available for the XFS filesystem:
 Deprecated Sysctls
 ==================
 
-None at present.
+fs.xfs.irix_sgid_inherit
+fs.xfs.irix_symlink_mode
 
 
 Removed Sysctls
diff --git a/fs/xfs/xfs_sysctl.c b/fs/xfs/xfs_sysctl.c
index 021ef96d0542..fac9de7ee6d0 100644
--- a/fs/xfs/xfs_sysctl.c
+++ b/fs/xfs/xfs_sysctl.c
@@ -50,13 +50,45 @@ xfs_panic_mask_proc_handler(
 }
 #endif /* CONFIG_PROC_FS */
 
+STATIC int
+xfs_deprecate_irix_sgid_inherit_proc_handler(
+	struct ctl_table	*ctl,
+	int			write,
+	void			*buffer,
+	size_t			*lenp,
+	loff_t			*ppos)
+{
+	if (write) {
+		printk_once(KERN_WARNING
+				"XFS: " "%s sysctl option is deprecated.\n",
+				ctl->procname);
+	}
+	return proc_dointvec_minmax(ctl, write, buffer, lenp, ppos);
+}
+
+STATIC int
+xfs_deprecate_irix_symlink_mode_proc_handler(
+	struct ctl_table	*ctl,
+	int			write,
+	void			*buffer,
+	size_t			*lenp,
+	loff_t			*ppos)
+{
+	if (write) {
+		printk_once(KERN_WARNING
+				"XFS: " "%s sysctl option is deprecated.\n",
+				ctl->procname);
+	}
+	return proc_dointvec_minmax(ctl, write, buffer, lenp, ppos);
+}
+
 static struct ctl_table xfs_table[] = {
 	{
 		.procname	= "irix_sgid_inherit",
 		.data		= &xfs_params.sgid_inherit.val,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
+		.proc_handler	= xfs_deprecate_irix_sgid_inherit_proc_handler,
 		.extra1		= &xfs_params.sgid_inherit.min,
 		.extra2		= &xfs_params.sgid_inherit.max
 	},
@@ -65,7 +97,7 @@ static struct ctl_table xfs_table[] = {
 		.data		= &xfs_params.symlink_mode.val,
 		.maxlen		= sizeof(int),
 		.mode		= 0644,
-		.proc_handler	= proc_dointvec_minmax,
+		.proc_handler	= xfs_deprecate_irix_symlink_mode_proc_handler,
 		.extra1		= &xfs_params.symlink_mode.min,
 		.extra2		= &xfs_params.symlink_mode.max
 	},
-- 
2.26.2

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

[Darrick J. Wong]

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
> 

Re: [PATCH 2/2] xfs: remove deprecated sysctl options

[Darrick J. Wong]

On Thu, Sep 24, 2020 at 07:07:47PM +0200, Pavel Reichl wrote:
> These optionr were for Irix compatibility, probably for clustered XFS
> clients in a heterogenous cluster which contained both Irix & Linux
> machines, so that behavior would be consistent. That doesn't exist anymore
> and it's no longer needed.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  Documentation/admin-guide/xfs.rst |  3 ++-
>  fs/xfs/xfs_sysctl.c               | 36 +++++++++++++++++++++++++++++--
>  2 files changed, 36 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
> index 413f68efccc0..208e17810459 100644
> --- a/Documentation/admin-guide/xfs.rst
> +++ b/Documentation/admin-guide/xfs.rst
> @@ -333,7 +333,8 @@ The following sysctls are available for the XFS filesystem:
>  Deprecated Sysctls
>  ==================
>  
> -None at present.
> +fs.xfs.irix_sgid_inherit
> +fs.xfs.irix_symlink_mode

When will these be removed from sysctl?

>  Removed Sysctls
> diff --git a/fs/xfs/xfs_sysctl.c b/fs/xfs/xfs_sysctl.c
> index 021ef96d0542..fac9de7ee6d0 100644
> --- a/fs/xfs/xfs_sysctl.c
> +++ b/fs/xfs/xfs_sysctl.c
> @@ -50,13 +50,45 @@ xfs_panic_mask_proc_handler(
>  }
>  #endif /* CONFIG_PROC_FS */
>  
> +STATIC int
> +xfs_deprecate_irix_sgid_inherit_proc_handler(
> +	struct ctl_table	*ctl,
> +	int			write,
> +	void			*buffer,
> +	size_t			*lenp,
> +	loff_t			*ppos)
> +{
> +	if (write) {
> +		printk_once(KERN_WARNING
> +				"XFS: " "%s sysctl option is deprecated.\n",
> +				ctl->procname);
> +	}
> +	return proc_dointvec_minmax(ctl, write, buffer, lenp, ppos);
> +}
> +
> +STATIC int
> +xfs_deprecate_irix_symlink_mode_proc_handler(
> +	struct ctl_table	*ctl,
> +	int			write,
> +	void			*buffer,
> +	size_t			*lenp,
> +	loff_t			*ppos)
> +{
> +	if (write) {
> +		printk_once(KERN_WARNING
> +				"XFS: " "%s sysctl option is deprecated.\n",
> +				ctl->procname);
> +	}
> +	return proc_dointvec_minmax(ctl, write, buffer, lenp, ppos);
> +}
> +
>  static struct ctl_table xfs_table[] = {
>  	{
>  		.procname	= "irix_sgid_inherit",
>  		.data		= &xfs_params.sgid_inherit.val,

Same comments as the last patch regarding a sysctl that spits out
deprecation warnings but continues to actually change xfs' behavior...

--D

>  		.maxlen		= sizeof(int),
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec_minmax,
> +		.proc_handler	= xfs_deprecate_irix_sgid_inherit_proc_handler,
>  		.extra1		= &xfs_params.sgid_inherit.min,
>  		.extra2		= &xfs_params.sgid_inherit.max
>  	},
> @@ -65,7 +97,7 @@ static struct ctl_table xfs_table[] = {
>  		.data		= &xfs_params.symlink_mode.val,
>  		.maxlen		= sizeof(int),
>  		.mode		= 0644,
> -		.proc_handler	= proc_dointvec_minmax,
> +		.proc_handler	= xfs_deprecate_irix_symlink_mode_proc_handler,
>  		.extra1		= &xfs_params.symlink_mode.min,
>  		.extra2		= &xfs_params.symlink_mode.max
>  	},
> -- 
> 2.26.2
> 

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

[Eric Sandeen]

On 9/24/20 12:26 PM, Darrick J. Wong wrote:
> 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.

but ... this doesn't change behavior, right?  The flag is still set.

I think it makes sense to announce deprecation, with a date set for future
removal, but keep all other behavior the same.  That gives people who still
need it (if any exist) time to complain, right?

> 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?

The same as it did yesterday; the flag is still set for 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?

That's my fault, I had suggested moving all the deprecated options to the end.

Maybe with a comment, /* REMOVE ME 2089 */ or whatever we pick?

-Eric

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

[Darrick J. Wong]

On Thu, Sep 24, 2020 at 12:39:07PM -0500, Eric Sandeen wrote:
> On 9/24/20 12:26 PM, Darrick J. Wong wrote:
> > 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?

June 65th, 2089 it is, then. ;)

> >>  ===========================     ================
> >>  
> >>  
> >> 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.
> 
> but ... this doesn't change behavior, right?  The flag is still set.
> 
> I think it makes sense to announce deprecation, with a date set for future
> removal, but keep all other behavior the same.  That gives people who still
> need it (if any exist) time to complain, right?

Ok.  I'm fine with these knobs continuing to affect xfs behavior right
up to the deprecation date.

> > 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;

Side note: shouldn't this clause be clearing XFS_MOUNT_NOATTR2?

> > 
> > If the kernel /does/ encounter an attr1 filesystem, what will it do now?
> 
> The same as it did yesterday; the flag is still set for 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?
> 
> That's my fault, I had suggested moving all the deprecated options to the end.
> 
> Maybe with a comment, /* REMOVE ME 2089 */ or whatever we pick?

Ah.

--D

> -Eric

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

[Pavel Reichl]

 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;
> 
> Side note: shouldn't this clause be clearing XFS_MOUNT_NOATTR2?
> 

I don't know but since nobody complained so far and we are actually starting work to remove it...I think it's safe to ignore that...?

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

[Pavel Reichl]

Thanks for discussion, if I get it right, the only thing to change is to add the date when mount options will me removed (September 2025)?

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

[Eric Sandeen]

On 9/25/20 8:40 AM, Pavel Reichl wrote:
> Thanks for discussion, if I get it right, the only thing to change is to add the date when mount options will me removed (September 2025)?

Please also add a comment above the moved mount options indicating that
all options below the comment are slated for deprecation.

Not sure if Darrick had anything else.  Are we happy w/ the kernel logging?

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

[Darrick J. Wong]

On Fri, Sep 25, 2020 at 09:50:45AM -0500, Eric Sandeen wrote:
> On 9/25/20 8:40 AM, Pavel Reichl wrote:
> > Thanks for discussion, if I get it right, the only thing to change is to add the date when mount options will me removed (September 2025)?
> 
> Please also add a comment above the moved mount options indicating that
> all options below the comment are slated for deprecation.
> 
> Not sure if Darrick had anything else.  Are we happy w/ the kernel logging?

Yeah, I'm fine with it. :)

--D