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.
>
> 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 = {
> >
