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@xxxxxxxxxx>
> > > > ---
> > > > 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 = {
> > > >
