Re: [PATCH v4 10/17] xfs: mount-api - add xfs_get_tree()

From: Ian Kent <raven@themaw.net>
To: Brian Foster <bfoster@redhat.com>
Cc: linux-xfs <linux-xfs@vger.kernel.org>,
	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 v4 10/17] xfs: mount-api - add xfs_get_tree()
Date: Sat, 05 Oct 2019 06:56:30 +0800	[thread overview]
Message-ID: <962b6ae811edec0dc50e3e98c0a7aa44251ea67f.camel@themaw.net> (raw)
In-Reply-To: <20191004155242.GD7208@bfoster>

On Fri, 2019-10-04 at 11:52 -0400, Brian Foster wrote:
> On Thu, Oct 03, 2019 at 06:26:12PM +0800, Ian Kent wrote:
> > Add the fs_context_operations method .get_tree that validates
> > mount options and fills the super block as previously done
> > by the file_system_type .mount method.
> > 
> > Signed-off-by: Ian Kent <raven@themaw.net>
> > ---
> >  fs/xfs/xfs_super.c |   50
> > ++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 50 insertions(+)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index cc2da9093e34..b984120667da 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1925,6 +1925,51 @@ xfs_fs_fill_super(
> >  	return error;
> >  }
> >  
> > +STATIC int
> > +xfs_fill_super(
> > +	struct super_block	*sb,
> > +	struct fs_context	*fc)
> > +{
> > +	struct xfs_fs_context	*ctx = fc->fs_private;
> > +	struct xfs_mount	*mp = sb->s_fs_info;
> > +	int			silent = fc->sb_flags & SB_SILENT;
> > +	int			error = -ENOMEM;
> > +
> > +	mp->m_super = sb;
> > +
> > +	/*
> > +	 * set up the mount name first so all the errors will refer to
> > the
> > +	 * correct device.
> > +	 */
> > +	mp->m_fsname = kstrndup(sb->s_id, MAXNAMELEN, GFP_KERNEL);
> > +	if (!mp->m_fsname)
> > +		goto out_free_fsname;
> > +	mp->m_fsname_len = strlen(mp->m_fsname) + 1;
> > +
> > +	error = xfs_validate_params(mp, ctx, false);
> > +	if (error)
> > +		goto out_free_fsname;
> > +
> > +	error = __xfs_fs_fill_super(mp, silent);
> > +	if (error)
> > +		goto out_free_fsname;
> > +
> > +	return 0;
> > +
> > + out_free_fsname:
> > +	sb->s_fs_info = NULL;
> > +	xfs_free_fsname(mp);
> > +	kfree(mp);
> > +	return error;
> 
> Ok, I think I have a better understanding of how this is supposed to
> work with the background context. mp starts off in fc->s_fs_info,
> ends
> up transferred to sb->s_fs_info and passed into here. We allocate
> ->m_fsname and carry on from here with ownership of mp.
> 
> The only thing I'll note is that the out_free_fsname label is
> misnamed
> and probably could be out_free or out_free_mp or something. With that
> nit addressed:

Or out_error perhaps, IIRC I remember thinking I needed to do that but
it slipped my mind?

> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> > +}
> > +
> > +STATIC int
> > +xfs_get_tree(
> > +	struct fs_context	*fc)
> > +{
> > +	return get_tree_bdev(fc, xfs_fill_super);
> > +}
> > +
> >  STATIC void
> >  xfs_fs_put_super(
> >  	struct super_block	*sb)
> > @@ -1995,6 +2040,11 @@ static const struct super_operations
> > xfs_super_operations = {
> >  	.free_cached_objects	= xfs_fs_free_cached_objects,
> >  };
> >  
> > +static const struct fs_context_operations xfs_context_ops = {
> > +	.parse_param = xfs_parse_param,
> > +	.get_tree    = xfs_get_tree,
> > +};
> > +
> >  static struct file_system_type xfs_fs_type = {
> >  	.owner			= THIS_MODULE,
> >  	.name			= "xfs",
> > 