Re: [PATCH v4 13/17] xfs: mount api - add xfs_reconfigure()

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 13/17] xfs: mount api - add xfs_reconfigure()
Date: Tue, 08 Oct 2019 08:32:40 +0800	[thread overview]
Message-ID: <79f4f4319397aed5201ae31c6d381d76fbd15b93.camel@themaw.net> (raw)
In-Reply-To: <20191007115133.GA22140@bfoster>

On Mon, 2019-10-07 at 07:51 -0400, Brian Foster wrote:
> On Sat, Oct 05, 2019 at 07:16:17AM +0800, Ian Kent wrote:
> > On Fri, 2019-10-04 at 11:53 -0400, Brian Foster wrote:
> > > On Thu, Oct 03, 2019 at 06:26:27PM +0800, Ian Kent wrote:
> > > > Add the fs_context_operations method .reconfigure that performs
> > > > remount validation as previously done by the super_operations
> > > > .remount_fs method.
> > > > 
> > > > An attempt has also been made to update the comment about
> > > > options
> > > > handling problems with mount(8) to reflect the current
> > > > situation.
> > > > 
> > > > Signed-off-by: Ian Kent <raven@themaw.net>
> > > > ---
> > > >  fs/xfs/xfs_super.c |   68
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 68 insertions(+)
> > > > 
> > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > > index ddcf030cca7c..06f650fb3a8c 100644
> > > > --- a/fs/xfs/xfs_super.c
> > > > +++ b/fs/xfs/xfs_super.c
> > > > @@ -1544,6 +1544,73 @@ xfs_fs_remount(
> > > >  	return 0;
> > > >  }
> > > >  
> > > > +/*
> > > > + * There can be problems with options passed from mount(8)
> > > > when
> > > > + * only the mount point path is given. The options are a merge
> > > > + * of options from the fstab, mtab of the current mount and
> > > > options
> > > > + * given on the command line.
> > > > + *
> > > > + * But this can't be relied upon to accurately reflect the
> > > > current
> > > > + * mount options. Consequently rejecting options that can't be
> > > > + * changed on reconfigure could erronously cause a mount
> > > > failure.
> > > > + *
> > > > + * Nowadays it should be possible to compare incoming options
> > > > + * and return an error for options that differ from the
> > > > current
> > > > + * mount and can't be changed on reconfigure.
> > > > + *
> > > > + * But this still might not always be the case so for now
> > > > continue
> > > > + * to return success for every reconfigure request, and
> > > > silently
> > > > + * ignore all options that can't actually be changed.
> > > > + *
> > > > + * See the commit log entry of this change for a more detailed
> > > > + * desription of the problem.
> > > > + */
> > > 
> > > But the commit log entry doesn't include any new info..?
> > 
> > I thought I did, honest, ;)
> > 
> > > How about this.. we already have a similar comment in
> > > xfs_fs_remount()
> > > that I happen to find a bit more clear. It also obviously has
> > > precedent.
> > > How about we copy that one to the top of this function verbatim,
> > > and
> > > whatever extra you want to get across can be added to the commit
> > > log
> > > description. Hm?
> > 
> > Trying to understand that comment and whether it was still needed
> > is what lead to this.
> > 
> > It is still relevant and IIRC the only extra info. needed is that
> > the mount-api implementation can't help with the problem because
> > it's what's given to the kernel via. mount(8) and that must
> > continue
> > to be supported.
> > 
> > I'll re-read the original message, maybe retaining it is sufficient
> > to imply the above.
> > 
> 
> I think it's sufficient. There's no need to comment on the previous
> implementation in the code because that code is being removed. If
> necessary, please do that in the commit log.

I re-read the original comment and I think I may have miss-interpreted
it a little because I was reading it in the context of "why can't the
mount-api handle this problem".

But it actually reads like it's saying xfs needs to needs to improve
its options validation for this.

Given that the mount-api has no control over what comes from user
space in the way of options I don't see how it can make any difference
to this problem.

So, bottom line is I'm thinking of discarding that comment and using
the original.

> 
> Brian
> 
> > > Brian
> > > 
> > > > +STATIC int
> > > > +xfs_reconfigure(
> > > > +	struct fs_context *fc)
> > > > +{
> > > > +	struct xfs_fs_context	*ctx = fc->fs_private;
> > > > +	struct xfs_mount	*mp = XFS_M(fc->root->d_sb);
> > > > +	struct xfs_mount        *new_mp = fc->s_fs_info;
> > > > +	xfs_sb_t		*sbp = &mp->m_sb;
> > > > +	int			flags = fc->sb_flags;
> > > > +	int			error;
> > > > +
> > > > +	error = xfs_validate_params(new_mp, ctx, false);
> > > > +	if (error)
> > > > +		return error;
> > > > +
> > > > +	/* inode32 -> inode64 */
> > > > +	if ((mp->m_flags & XFS_MOUNT_SMALL_INUMS) &&
> > > > +	    !(new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {
> > > > +		mp->m_flags &= ~XFS_MOUNT_SMALL_INUMS;
> > > > +		mp->m_maxagi = xfs_set_inode_alloc(mp, sbp-
> > > > > sb_agcount);
> > > > +	}
> > > > +
> > > > +	/* inode64 -> inode32 */
> > > > +	if (!(mp->m_flags & XFS_MOUNT_SMALL_INUMS) &&
> > > > +	    (new_mp->m_flags & XFS_MOUNT_SMALL_INUMS)) {
> > > > +		mp->m_flags |= XFS_MOUNT_SMALL_INUMS;
> > > > +		mp->m_maxagi = xfs_set_inode_alloc(mp, sbp-
> > > > > sb_agcount);
> > > > +	}
> > > > +
> > > > +	/* ro -> rw */
> > > > +	if ((mp->m_flags & XFS_MOUNT_RDONLY) && !(flags &
> > > > SB_RDONLY)) {
> > > > +		error = xfs_remount_rw(mp);
> > > > +		if (error)
> > > > +			return error;
> > > > +	}
> > > > +
> > > > +	/* rw -> ro */
> > > > +	if (!(mp->m_flags & XFS_MOUNT_RDONLY) && (flags &
> > > > SB_RDONLY)) {
> > > > +		error = xfs_remount_ro(mp);
> > > > +		if (error)
> > > > +			return error;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > > +
> > > >  /*
> > > >   * Second stage of a freeze. The data is already frozen so we
> > > > only
> > > >   * need to take care of the metadata. Once that's done sync
> > > > the
> > > > superblock
> > > > @@ -2069,6 +2136,7 @@ static const struct super_operations
> > > > xfs_super_operations = {
> > > >  static const struct fs_context_operations xfs_context_ops = {
> > > >  	.parse_param = xfs_parse_param,
> > > >  	.get_tree    = xfs_get_tree,
> > > > +	.reconfigure = xfs_reconfigure,
> > > >  };
> > > >  
> > > >  static struct file_system_type xfs_fs_type = {
> > > > 