On Fri, 2019-08-30 at 07:56 -0400, Brian Foster wrote:
> On Fri, Aug 30, 2019 at 07:10:44PM +0800, Ian Kent wrote:
> > On Wed, 2019-08-28 at 09:28 -0400, Brian Foster wrote:
> > > On Fri, Aug 23, 2019 at 09:00:16AM +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 | 84
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 84 insertions(+)
> > > >
> > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > > index 76374d602257..aae0098fecab 100644
> > > > --- a/fs/xfs/xfs_super.c
> > > > +++ b/fs/xfs/xfs_super.c
> > > > @@ -1522,6 +1522,89 @@ xfs_fs_remount(
> > > > return 0;
> > > > }
> > > >
> > > > +/*
> > > > + * There have been problems in the past with options passed
> > > > from
> > > > mount(8).
> > > > + *
> > > > + * The problem being that options passed by mount(8) in the
> > > > case
> > > > where only
> > > > + * the the mount point path is given would consist of the
> > > > existing
> > > > fstab
> > > > + * options with the options from mtab for the current mount
> > > > merged
> > > > in and
> > > > + * the options given on the command line last. But the result
> > > > couldn't be
> > > > + * relied upon to accurately reflect the current mount options
> > > > so
> > > > that
> > > > + * rejecting options that can't be changed on reconfigure
> > > > could
> > > > erronously
> > > > + * cause mount failure.
> > > > + *
> > > > + * The mount-api uses a legacy mount options handler in the
> > > > VFS to
> > > > handle
> > > > + * mount(8) so these options will continue to be passed. Even
> > > > if
> > > > mount(8)
> > > > + * is updated to use fsopen()/fsconfig()/fsmount() it's likely
> > > > to
> > > > continue
> > > > + * to set the existing options so options problems with
> > > > reconfigure could
> > > > + * continue.
> > > > + *
> > > > + * For the longest time mtab locking was a problem and this
> > > > could
> > > > have been
> > > > + * one possible cause. It's also possible there could have
> > > > been
> > > > options
> > > > + * order problems.
> > > > + *
> > > > + * That has changed now as mtab is a link to the proc file
> > > > system
> > > > mount
> > > > + * table so mtab options should be always accurate.
> > > > + *
> > > > + * Consulting the util-linux maintainer (Karel Zak) he is
> > > > confident that,
> > > > + * in this case, the options passed by mount(8) will be those
> > > > of
> > > > the current
> > > > + * mount and the options order should be a correct merge of
> > > > fstab
> > > > and mtab
> > > > + * options, and new options given on the command line.
> > > > + *
> > > > + * So, in theory, 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 to prevent users from believing
> > > > they
> > > > might have
> > > > + * changed mount options using remount which can't be changed.
> > > > + *
> > > > + * But for now continue to return success for every
> > > > reconfigure
> > > > request, and
> > > > + * silently ignore all options that can't actually be changed.
> > > > + */
> > >
> > > This seems like all good information for a commit log description
> > > or
> > > perhaps to land in a common header where some of these fs context
> > > bits
> > > are declared, but overly broad for a function header comment for
> > > an
> > > XFS
> > > callback. I'd more expect some information around the fundamental
> > > difference between 'mp' and 'new_mp,' where those come from, what
> > > they
> > > mean, etc. From the code, it seems like new_mp is transient and
> > > reflects
> > > changes that we need to incorporate in the original mp..?
> >
> > The reason I looked into this is becuase of a fairly lengthy
> > comment in the original source and that lead to this rather
> > verbose tail about what I found.
> >
> > It's not so much a mount context problem becuase it's due
> > to the way user space constructs the mount options from
> > various sources where options are present.
> >
> > So I don't think it's something that the mount context
> > code can resolve.
> >
> > Which leaves a bunch of (I think) useful information
> > without a palce to live so it can be referred to ...
> > suggestions?
> >
>
> I suppose there's always the commit log description if there isn't a
> suitable place in the code.
Ok, I'll try and cut it down to a more reasonable size while
keeping the gist of what it describes and move the detail
to the changelog entry.
>
> 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
> > > > @@ -2049,6 +2132,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 = {
> > > >
