Re: [PATCH v6 12/12] xfs: switch to use the new mount-api

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Ian Kent <raven@themaw.net>
Cc: Christoph Hellwig <hch@infradead.org>,
	linux-xfs <linux-xfs@vger.kernel.org>,
	Brian Foster <bfoster@redhat.com>,
	Eric Sandeen <sandeen@sandeen.net>,
	David Howells <dhowells@redhat.com>,
	Dave Chinner <dchinner@redhat.com>,
	Al Viro <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH v6 12/12] xfs: switch to use the new mount-api
Date: Wed, 16 Oct 2019 21:53:30 -0700	[thread overview]
Message-ID: <20191017045330.GI13108@magnolia> (raw)
In-Reply-To: <322766092bbf885ae17eee046c917937f9e76cfc.camel@themaw.net>

On Thu, Oct 17, 2019 at 09:13:29AM +0800, Ian Kent wrote:
> On Wed, 2019-10-16 at 11:18 -0700, Christoph Hellwig wrote:
> > On Wed, Oct 16, 2019 at 08:41:48AM +0800, Ian Kent wrote:
> > > +static const struct fs_parameter_description xfs_fs_parameters = {
> > > +	.name		= "XFS",
> > > +	.specs		= xfs_param_specs,
> > > +};
> > 
> > Well spell xfs in lower case in the file system type, so I think we
> > should
> > spell it the same here.
> 
> The problem is that this will probably be used in logging later and
> there's a lot of logging that uses the upper case variant.
> 
> OTOH if all the log messages were changed to use lower case "xfs" then
> one of the problems I see with logging (that name inconsistency) would
> go away.
> 
> So I'm not sure what I should do here.

I would just leave it 'XFS' for consistency, but I might be in the back
pocket of Big Letter. ;)

--D

> > 
> > Btw, can we keep all the mount code together where most of it already
> > is at the top of the file?  I know the existing version has some
> > remount
> > stuff at the bottom, but as that get entirely rewritten we might as
> > well
> > move it all up.
> 
> Yep, sounds good.
> 
> > 
> > > +	int			silent = fc->sb_flags & SB_SILENT;
> > 
> > The silent variable is only used once, so we might as well remove it.
> 
> And again.
> 
> > 
> > > +	struct xfs_mount	*mp = fc->s_fs_info;
> > > +
> > > +	/*
> > > +	 * mp and ctx are stored in the fs_context when it is
> > > +	 * initialized. mp is transferred to the superblock on
> > > +	 * a successful mount, but if an error occurs before the
> > > +	 * transfer we have to free it here.
> > > +	 */
> > > +	if (mp) {
> > > +		xfs_free_names(mp);
> > > +		kfree(mp);
> > > +	}
> > 
> > We always pair xfs_free_names with freeing the mount structure.
> > I think it would be nice to add an xfs_free_mount that does both
> > as a refactoring at the beginning of the series. 
> 
> Ditto.
> 
> > 
> > > +static const struct fs_context_operations xfs_context_ops = {
> > > +	.parse_param = xfs_parse_param,
> > > +	.get_tree    = xfs_get_tree,
> > > +	.reconfigure = xfs_reconfigure,
> > > +	.free        = xfs_fc_free,
> > > +};
> > 
> > Should these all get a prefix like xfs_fc_free?  Maybe xfs_fsctx
> > to be a little bit more descriptive?
> 
> Good point, since it's struct fs_context* I think an "xfs_fc_"
> prefix on the context related structures and variables would make
> the most sense.
> 
> I'll do that too.
> Ian
> 