diff mbox series

[1/1] xfs: Skip repetitive warnings about mount options

Message ID 20210220221549.290538-3-preichl@redhat.com (mailing list archive)
State Superseded
Headers show
Series [1/1] xfs: Skip repetitive warnings about mount options | expand

Commit Message

Pavel Reichl Feb. 20, 2021, 10:15 p.m. UTC
Skip the warnings about mount option being deprecated if we are
remounting and deprecated option state is not changing.

Bug: https://bugzilla.kernel.org/show_bug.cgi?id=211605
Fix-suggested-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

Comments

Darrick J. Wong Feb. 22, 2021, 9:28 p.m. UTC | #1
On Sat, Feb 20, 2021 at 11:15:49PM +0100, Pavel Reichl wrote:
> Skip the warnings about mount option being deprecated if we are
> remounting and deprecated option state is not changing.
> 
> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=211605
> Fix-suggested-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 813be879a5e5..6724a7018d1f 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1169,6 +1169,13 @@ xfs_fs_parse_param(
>  	struct fs_parse_result	result;
>  	int			size = 0;
>  	int			opt;
> +	uint64_t                prev_m_flags = 0; /* Mount flags of prev. mount */

Nit: spaces here^^^^^^^^^^^^^^^^ should be tabs.

> +	bool			remounting = fc->purpose & FS_CONTEXT_FOR_RECONFIGURE;
> +
> +	/* if reconfiguring then get mount flags of previous flags */
> +	if (remounting) {
> +		prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
> +	}
>  
>  	opt = fs_parse(fc, xfs_fs_parameters, param, &result);
>  	if (opt < 0)
> @@ -1294,19 +1301,27 @@ xfs_fs_parse_param(
>  #endif
>  	/* Following mount options will be removed in September 2025 */
>  	case Opt_ikeep:
> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		if (!remounting ||  !(prev_m_flags & XFS_MOUNT_IKEEP)) {
> +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		}

/me wonders if these could be refactored into a common helper, though I
can't really think of anything less clunky than:

static inline void
xfs_fs_warn_deprecated(
	struct fs_context	*fc,
	struct fs_parameter	*param)
	uint64_t		flag,
	bool			value);
{
	uint64_t		prev_m_flags;

	if (!(fc->purpose & FS_CONTEXT_FOR_RECONFIGURE))
		goto warn;
	prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
	if (!!(prev_m_flags & flag) == value)
		goto warn;
	return;
warn:
	xfs_warn(mp, "%s mount option is deprecated.", param->key);
}
...
	case Opt_ikeep:
		xfs_fs_warn_deprecated(fc, param, XFS_MOUNT_IKEEP, true);
		mp->m_flags |= XFS_MOUNT_IKEEP;
		break;
	case Opt_noikeep:
		xfs_fs_warn_deprecated(fc, param, XFS_MOUNT_IKEEP, false);
		mp->m_flags &= ~XFS_MOUNT_IKEEP;
		break;

Thoughts?

--D

>  		mp->m_flags |= XFS_MOUNT_IKEEP;
>  		return 0;
>  	case Opt_noikeep:
> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		if (!remounting || prev_m_flags & XFS_MOUNT_IKEEP) {
> +			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);
> +		if (!remounting || !(prev_m_flags & XFS_MOUNT_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);
> +		if (!remounting || !(prev_m_flags & XFS_MOUNT_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;
> -- 
> 2.29.2
>
Eric Sandeen Feb. 22, 2021, 10:19 p.m. UTC | #2
On 2/20/21 4:15 PM, Pavel Reichl wrote:
> Skip the warnings about mount option being deprecated if we are
> remounting and deprecated option state is not changing.
> 
> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=211605
> Fix-suggested-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 813be879a5e5..6724a7018d1f 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1169,6 +1169,13 @@ xfs_fs_parse_param(
>  	struct fs_parse_result	result;
>  	int			size = 0;
>  	int			opt;
> +	uint64_t                prev_m_flags = 0; /* Mount flags of prev. mount */
> +	bool			remounting = fc->purpose & FS_CONTEXT_FOR_RECONFIGURE;
> +
> +	/* if reconfiguring then get mount flags of previous flags */
> +	if (remounting) {
> +		prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;

I wonder, does mp->m_flags work just as well for this purpose? I do get lost
in how the mount api stashes things. I /think/ that the above is just a
long way of getting to mp->m_flags.

> +	}
>  
>  	opt = fs_parse(fc, xfs_fs_parameters, param, &result);
>  	if (opt < 0)
> @@ -1294,19 +1301,27 @@ xfs_fs_parse_param(
>  #endif
>  	/* Following mount options will be removed in September 2025 */
>  	case Opt_ikeep:
> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		if (!remounting ||  !(prev_m_flags & XFS_MOUNT_IKEEP)) {

while we're nitpicking whitespace, ^^ 2 spaces there

as for the prev_m_flags usage, does:

+		if (!remounting || !(mp->m_flags & XFS_MOUNT_IKEEP)) {

work just as well here or no?

> +			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);
> +		if (!remounting || prev_m_flags & XFS_MOUNT_IKEEP) {

and I dunno, I think I'd like parentheses for clarity here i.e.:

+		if (!remounting || (prev_m_flags & XFS_MOUNT_IKEEP)) {

Thanks,
-Eric


> +			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);
> +		if (!remounting || !(prev_m_flags & XFS_MOUNT_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);
> +		if (!remounting || !(prev_m_flags & XFS_MOUNT_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;
>
Dave Chinner Feb. 23, 2021, 4:32 a.m. UTC | #3
On Mon, Feb 22, 2021 at 01:28:30PM -0800, Darrick J. Wong wrote:
> On Sat, Feb 20, 2021 at 11:15:49PM +0100, Pavel Reichl wrote:
> > Skip the warnings about mount option being deprecated if we are
> > remounting and deprecated option state is not changing.
> > 
> > Bug: https://bugzilla.kernel.org/show_bug.cgi?id=211605
> > Fix-suggested-by: Eric Sandeen <sandeen@redhat.com>
> > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> > ---
> >  fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 813be879a5e5..6724a7018d1f 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1169,6 +1169,13 @@ xfs_fs_parse_param(
> >  	struct fs_parse_result	result;
> >  	int			size = 0;
> >  	int			opt;
> > +	uint64_t                prev_m_flags = 0; /* Mount flags of prev. mount */
> 
> Nit: spaces here^^^^^^^^^^^^^^^^ should be tabs.
> 
> > +	bool			remounting = fc->purpose & FS_CONTEXT_FOR_RECONFIGURE;
> > +
> > +	/* if reconfiguring then get mount flags of previous flags */
> > +	if (remounting) {
> > +		prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
> > +	}
> >  
> >  	opt = fs_parse(fc, xfs_fs_parameters, param, &result);
> >  	if (opt < 0)
> > @@ -1294,19 +1301,27 @@ xfs_fs_parse_param(
> >  #endif
> >  	/* Following mount options will be removed in September 2025 */
> >  	case Opt_ikeep:
> > -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> > +		if (!remounting ||  !(prev_m_flags & XFS_MOUNT_IKEEP)) {
> > +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
> > +		}
> 
> /me wonders if these could be refactored into a common helper, though I
> can't really think of anything less clunky than:
> 
> static inline void
> xfs_fs_warn_deprecated(
> 	struct fs_context	*fc,
> 	struct fs_parameter	*param)
> 	uint64_t		flag,
> 	bool			value);
> {
> 	uint64_t		prev_m_flags;
> 
> 	if (!(fc->purpose & FS_CONTEXT_FOR_RECONFIGURE))
> 		goto warn;
> 	prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
> 	if (!!(prev_m_flags & flag) == value)
> 		goto warn;
> 	return;
> warn:
> 	xfs_warn(mp, "%s mount option is deprecated.", param->key);
> }
> ...
> 	case Opt_ikeep:
> 		xfs_fs_warn_deprecated(fc, param, XFS_MOUNT_IKEEP, true);
> 		mp->m_flags |= XFS_MOUNT_IKEEP;
> 		break;
> 	case Opt_noikeep:
> 		xfs_fs_warn_deprecated(fc, param, XFS_MOUNT_IKEEP, false);
> 		mp->m_flags &= ~XFS_MOUNT_IKEEP;
> 		break;

	case Opt_ikeep:
		xfs_fs_warn_deprecated(fc, param,
				(mp->m_flags & XFS_MOUNT_IKEEP), true);
		mp->m_flags |= XFS_MOUNT_IKEEP;
		break;
	case Opt_noikeep:
		xfs_fs_warn_deprecated(fc, param,
				(mp->m_flags & XFS_MOUNT_IKEEP), false);
		mp->m_flags &= ~XFS_MOUNT_IKEEP;
		break;

static inline void
xfs_fs_warn_deprecated(
	struct fs_context	*fc,
	struct fs_parameter	*param)
	bool			old_value,
	bool			new_value);
{
	if ((fc->purpose & FS_CONTEXT_FOR_RECONFIGURE) &&
	    old_value == new_value)
		return;
	xfs_warn(mp, "%s mount option is deprecated.", param->key);
}

-Dave.

> 
> Thoughts?
> 
> --D
> 
> >  		mp->m_flags |= XFS_MOUNT_IKEEP;
> >  		return 0;
> >  	case Opt_noikeep:
> > -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> > +		if (!remounting || prev_m_flags & XFS_MOUNT_IKEEP) {
> > +			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);
> > +		if (!remounting || !(prev_m_flags & XFS_MOUNT_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);
> > +		if (!remounting || !(prev_m_flags & XFS_MOUNT_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;
> > -- 
> > 2.29.2
> > 
>
Pavel Reichl Feb. 23, 2021, 5:53 p.m. UTC | #4
On 2/22/21 11:19 PM, Eric Sandeen wrote:
> 
> On 2/20/21 4:15 PM, Pavel Reichl wrote:
>> Skip the warnings about mount option being deprecated if we are
>> remounting and deprecated option state is not changing.
>>
>> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=211605
>> Fix-suggested-by: Eric Sandeen <sandeen@redhat.com>
>> Signed-off-by: Pavel Reichl <preichl@redhat.com>
>> ---
>>  fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index 813be879a5e5..6724a7018d1f 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -1169,6 +1169,13 @@ xfs_fs_parse_param(
>>  	struct fs_parse_result	result;
>>  	int			size = 0;
>>  	int			opt;
>> +	uint64_t                prev_m_flags = 0; /* Mount flags of prev. mount */
>> +	bool			remounting = fc->purpose & FS_CONTEXT_FOR_RECONFIGURE;
>> +
>> +	/* if reconfiguring then get mount flags of previous flags */
>> +	if (remounting) {
>> +		prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
> 
> I wonder, does mp->m_flags work just as well for this purpose? I do get lost
> in how the mount api stashes things. I /think/ that the above is just a
> long way of getting to mp->m_flags.

Hi Eric, I'm sorry to disagree, but I think that mp->m_flags is newly allocated for this mount and it's not populated with previous mount's mount options.

static int xfs_init_fs_context(
        struct fs_context       *fc)
{
        struct xfs_mount        *mp;

So here it's allocated and zeroed

        mp = kmem_alloc(sizeof(struct xfs_mount), KM_ZERO);
        if (!mp)
                return -ENOMEM;
                
...

a few flags are set by values from super block, but not nearly all of them

        /*
         * Copy binary VFS mount flags we are interested in.
         */
        if (fc->sb_flags & SB_RDONLY)
                mp->m_flags |= XFS_MOUNT_RDONLY;
        if (fc->sb_flags & SB_DIRSYNC)
                mp->m_flags |= XFS_MOUNT_DIRSYNC;
        if (fc->sb_flags & SB_SYNCHRONOUS)
                mp->m_flags |= XFS_MOUNT_WSYNC;

here it's assigned

        fc->s_fs_info = mp;

...

> 
>> +	}
>>  
>>  	opt = fs_parse(fc, xfs_fs_parameters, param, &result);
>>  	if (opt < 0)
>> @@ -1294,19 +1301,27 @@ xfs_fs_parse_param(
>>  #endif
>>  	/* Following mount options will be removed in September 2025 */
>>  	case Opt_ikeep:
>> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
>> +		if (!remounting ||  !(prev_m_flags & XFS_MOUNT_IKEEP)) {
> 
> while we're nitpicking whitespace, ^^ 2 spaces there
> 
> as for the prev_m_flags usage, does:
> 
> +		if (!remounting || !(mp->m_flags & XFS_MOUNT_IKEEP)) {
> 
> work just as well here or no?

I don't think so - while developing the path I printk-ed  the value and it did not reflect the mount options of previous mount...IIRC we discussed it and you hinted me to use the XFS_M(fc->root->d_sb)->m_flags...which works.

So I'm a bit confused by your comment, but I get confused a lot :-)

> 
>> +			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);
>> +		if (!remounting || prev_m_flags & XFS_MOUNT_IKEEP) {
> 
> and I dunno, I think I'd like parentheses for clarity here i.e.:
> 
> +		if (!remounting || (prev_m_flags & XFS_MOUNT_IKEEP)) {
> 
> Thanks,
> -Eric
> 
> 
>> +			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);
>> +		if (!remounting || !(prev_m_flags & XFS_MOUNT_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);
>> +		if (!remounting || !(prev_m_flags & XFS_MOUNT_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;
>>
>
Eric Sandeen Feb. 23, 2021, 6:10 p.m. UTC | #5
On 2/23/21 11:53 AM, Pavel Reichl wrot
> 
> On 2/22/21 11:19 PM, Eric Sandeen wrote:
>>
>> On 2/20/21 4:15 PM, Pavel Reichl wrote:
>>> Skip the warnings about mount option being deprecated if we are
>>> remounting and deprecated option state is not changing.
>>>
>>> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=211605
>>> Fix-suggested-by: Eric Sandeen <sandeen@redhat.com>
>>> Signed-off-by: Pavel Reichl <preichl@redhat.com>
>>> ---
>>>  fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
>>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>>> index 813be879a5e5..6724a7018d1f 100644
>>> --- a/fs/xfs/xfs_super.c
>>> +++ b/fs/xfs/xfs_super.c
>>> @@ -1169,6 +1169,13 @@ xfs_fs_parse_param(
>>>  	struct fs_parse_result	result;
>>>  	int			size = 0;
>>>  	int			opt;
>>> +	uint64_t                prev_m_flags = 0; /* Mount flags of prev. mount */
>>> +	bool			remounting = fc->purpose & FS_CONTEXT_FOR_RECONFIGURE;
>>> +
>>> +	/* if reconfiguring then get mount flags of previous flags */
>>> +	if (remounting) {
>>> +		prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
>>
>> I wonder, does mp->m_flags work just as well for this purpose? I do get lost
>> in how the mount api stashes things. I /think/ that the above is just a
>> long way of getting to mp->m_flags.
> 
> Hi Eric, I'm sorry to disagree, but I think that mp->m_flags is newly allocated for this mount and it's not populated with previous mount's mount options.

No need to be sorry ;) And in any case, you're corrrect here.

> 
> static int xfs_init_fs_context(
>         struct fs_context       *fc)
> {
>         struct xfs_mount        *mp;
> 
> So here it's allocated and zeroed
> 
>         mp = kmem_alloc(sizeof(struct xfs_mount), KM_ZERO);
>         if (!mp)
>                 return -ENOMEM;
>                 
> ...

and eventually:

	fc->s_fs_info = mp;

Ok, yup, I see.  so I guess we kind of have:

*parsing_mp = fc->s_fs_info;

and 

*current_mp = XFS_M(fc->root->d_sb);

(variable names not actually in the code, just used for example)

Sorry for the noise, my mistake!

-Eric
Darrick J. Wong Feb. 23, 2021, 6:25 p.m. UTC | #6
On Tue, Feb 23, 2021 at 12:10:41PM -0600, Eric Sandeen wrote:
> On 2/23/21 11:53 AM, Pavel Reichl wrot
> > 
> > On 2/22/21 11:19 PM, Eric Sandeen wrote:
> >>
> >> On 2/20/21 4:15 PM, Pavel Reichl wrote:
> >>> Skip the warnings about mount option being deprecated if we are
> >>> remounting and deprecated option state is not changing.
> >>>
> >>> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=211605
> >>> Fix-suggested-by: Eric Sandeen <sandeen@redhat.com>
> >>> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> >>> ---
> >>>  fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
> >>>  1 file changed, 19 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> >>> index 813be879a5e5..6724a7018d1f 100644
> >>> --- a/fs/xfs/xfs_super.c
> >>> +++ b/fs/xfs/xfs_super.c
> >>> @@ -1169,6 +1169,13 @@ xfs_fs_parse_param(
> >>>  	struct fs_parse_result	result;
> >>>  	int			size = 0;
> >>>  	int			opt;
> >>> +	uint64_t                prev_m_flags = 0; /* Mount flags of prev. mount */
> >>> +	bool			remounting = fc->purpose & FS_CONTEXT_FOR_RECONFIGURE;
> >>> +
> >>> +	/* if reconfiguring then get mount flags of previous flags */
> >>> +	if (remounting) {
> >>> +		prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
> >>
> >> I wonder, does mp->m_flags work just as well for this purpose? I do get lost
> >> in how the mount api stashes things. I /think/ that the above is just a
> >> long way of getting to mp->m_flags.
> > 
> > Hi Eric, I'm sorry to disagree, but I think that mp->m_flags is
> > newly allocated for this mount and it's not populated with previous
> > mount's mount options.

Yeah, that's one of the (IMHO) ugliest warts of the new fs parsing code.

> No need to be sorry ;) And in any case, you're corrrect here.
> 
> > 
> > static int xfs_init_fs_context(
> >         struct fs_context       *fc)
> > {
> >         struct xfs_mount        *mp;
> > 
> > So here it's allocated and zeroed
> > 
> >         mp = kmem_alloc(sizeof(struct xfs_mount), KM_ZERO);
> >         if (!mp)
> >                 return -ENOMEM;
> >                 
> > ...
> 
> and eventually:
> 
> 	fc->s_fs_info = mp;
> 
> Ok, yup, I see.  so I guess we kind of have:
> 
> *parsing_mp = fc->s_fs_info;
> 
> and 
> 
> *current_mp = XFS_M(fc->root->d_sb);
> 
> (variable names not actually in the code, just used for example)

Maybe they should be. ;)

--D

> Sorry for the noise, my mistake!
> 
> -Eric
Eric Sandeen Feb. 23, 2021, 9:05 p.m. UTC | #7
On 2/23/21 12:25 PM, Darrick J. Wong wrote:

>> Ok, yup, I see.  so I guess we kind of have:
>>
>> *parsing_mp = fc->s_fs_info;
>>
>> and 
>>
>> *current_mp = XFS_M(fc->root->d_sb);
>>
>> (variable names not actually in the code, just used for example)
> 
> Maybe they should be. ;)

Yup I almost suggested at least a comment. And since we do care about the
actual/live *mp now, maybe adding self-describing variable names would be a
help.

-Eric

> --D
> 
>> Sorry for the noise, my mistake!
>>
>> -Eric
>
diff mbox series

Patch

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 813be879a5e5..6724a7018d1f 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1169,6 +1169,13 @@  xfs_fs_parse_param(
 	struct fs_parse_result	result;
 	int			size = 0;
 	int			opt;
+	uint64_t                prev_m_flags = 0; /* Mount flags of prev. mount */
+	bool			remounting = fc->purpose & FS_CONTEXT_FOR_RECONFIGURE;
+
+	/* if reconfiguring then get mount flags of previous flags */
+	if (remounting) {
+		prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
+	}
 
 	opt = fs_parse(fc, xfs_fs_parameters, param, &result);
 	if (opt < 0)
@@ -1294,19 +1301,27 @@  xfs_fs_parse_param(
 #endif
 	/* Following mount options will be removed in September 2025 */
 	case Opt_ikeep:
-		xfs_warn(mp, "%s mount option is deprecated.", param->key);
+		if (!remounting ||  !(prev_m_flags & XFS_MOUNT_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);
+		if (!remounting || prev_m_flags & XFS_MOUNT_IKEEP) {
+			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);
+		if (!remounting || !(prev_m_flags & XFS_MOUNT_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);
+		if (!remounting || !(prev_m_flags & XFS_MOUNT_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;