| Message ID | 157009838772.13858.3951542955676751036.stgit@fedora-28 (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: mount API patch series | expand |
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..? 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? 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 = { >
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. > > 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 = { > >
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. 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 = { > > > >
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 = { > > > >
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. + */ +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 = {
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(+)