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

["Darrick J. Wong" <djwong@kernel.org>]

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