| Message ID | 156652202212.2607.8621137631843273531.stgit@fedora-28 (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: mount API patch series | expand |
On Fri, Aug 23, 2019 at 09:00:22AM +0800, Ian Kent wrote: > Add the fs_context_operations method .free that performs fs > context cleanup on context release. > > Signed-off-by: Ian Kent <raven@themaw.net> > --- > fs/xfs/xfs_super.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index aae0098fecab..9976163dc537 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -2129,10 +2129,32 @@ static const struct super_operations xfs_super_operations = { > .free_cached_objects = xfs_fs_free_cached_objects, > }; > > +static void xfs_fc_free(struct fs_context *fc) > +{ > + struct xfs_fs_context *ctx = fc->fs_private; > + struct xfs_mount *mp = fc->s_fs_info; > + > + if (mp) { > + /* > + * If an error occurs before ownership the xfs_mount > + * info struct is passed to xfs by the VFS (by assigning > + * it to sb->s_fs_info and clearing the corresponding > + * fs_context field, which is done before calling fill > + * super via .get_tree()) there may be some strings to > + * cleanup. > + */ The code looks straightforward but I find the comment confusing. How can the VFS pass ownership of the xfs_mount if it's an XFS private data structure? > + kfree(mp->m_logname); > + kfree(mp->m_rtname); > + kfree(mp); > + } > + kfree(ctx); Also, should we at least reassign associated fc pointers to NULL if we have multiple places to free things like ctx or mp? Brian > +} > + > static const struct fs_context_operations xfs_context_ops = { > .parse_param = xfs_parse_param, > .get_tree = xfs_get_tree, > .reconfigure = xfs_reconfigure, > + .free = xfs_fc_free, > }; > > static struct file_system_type xfs_fs_type = { >
On Wed, 2019-08-28 at 09:28 -0400, Brian Foster wrote: > On Fri, Aug 23, 2019 at 09:00:22AM +0800, Ian Kent wrote: > > Add the fs_context_operations method .free that performs fs > > context cleanup on context release. > > > > Signed-off-by: Ian Kent <raven@themaw.net> > > --- > > fs/xfs/xfs_super.c | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > > index aae0098fecab..9976163dc537 100644 > > --- a/fs/xfs/xfs_super.c > > +++ b/fs/xfs/xfs_super.c > > @@ -2129,10 +2129,32 @@ static const struct super_operations > > xfs_super_operations = { > > .free_cached_objects = xfs_fs_free_cached_objects, > > }; > > > > +static void xfs_fc_free(struct fs_context *fc) > > +{ > > + struct xfs_fs_context *ctx = fc->fs_private; > > + struct xfs_mount *mp = fc->s_fs_info; > > + > > + if (mp) { > > + /* > > + * If an error occurs before ownership the xfs_mount > > + * info struct is passed to xfs by the VFS (by > > assigning > > + * it to sb->s_fs_info and clearing the corresponding > > + * fs_context field, which is done before calling fill > > + * super via .get_tree()) there may be some strings to > > + * cleanup. > > + */ > > The code looks straightforward but I find the comment confusing. How > can > the VFS pass ownership of the xfs_mount if it's an XFS private data > structure? *sigh*, Darrick was similarly confused about my original comment so it seems it's still not good enough. The mount context holds values for various things as the VFS allocates/constructs them and when they get assigned to an object owned by the file system they are NULLed in the moun t context structure. I'm calling that process "ownership" in the above comment and, perhaps mistenly, extending that to the xfs_mount object which might never actually make it to the "file system" in term of that ownership, such as when an error occurs in the VFS during the mount procedure before the mount context is realeased. I can try and re-word the comment again, clearly it's still not good enough ... > > > + kfree(mp->m_logname); > > + kfree(mp->m_rtname); > > + kfree(mp); > > + } > > + kfree(ctx); > > Also, should we at least reassign associated fc pointers to NULL if > we > have multiple places to free things like ctx or mp? > > Brian > > > +} > > + > > static const struct fs_context_operations xfs_context_ops = { > > .parse_param = xfs_parse_param, > > .get_tree = xfs_get_tree, > > .reconfigure = xfs_reconfigure, > > + .free = xfs_fc_free, > > }; > > > > static struct file_system_type xfs_fs_type = { > >
On Wed, 2019-08-28 at 09:28 -0400, Brian Foster wrote: > On Fri, Aug 23, 2019 at 09:00:22AM +0800, Ian Kent wrote: > > Add the fs_context_operations method .free that performs fs > > context cleanup on context release. > > > > Signed-off-by: Ian Kent <raven@themaw.net> > > --- > > fs/xfs/xfs_super.c | 22 ++++++++++++++++++++++ > > 1 file changed, 22 insertions(+) > > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > > index aae0098fecab..9976163dc537 100644 > > --- a/fs/xfs/xfs_super.c > > +++ b/fs/xfs/xfs_super.c > > @@ -2129,10 +2129,32 @@ static const struct super_operations > > xfs_super_operations = { > > .free_cached_objects = xfs_fs_free_cached_objects, > > }; > > > > +static void xfs_fc_free(struct fs_context *fc) > > +{ > > + struct xfs_fs_context *ctx = fc->fs_private; > > + struct xfs_mount *mp = fc->s_fs_info; > > + > > + if (mp) { > > + /* > > + * If an error occurs before ownership the xfs_mount > > + * info struct is passed to xfs by the VFS (by > > assigning > > + * it to sb->s_fs_info and clearing the corresponding > > + * fs_context field, which is done before calling fill > > + * super via .get_tree()) there may be some strings to > > + * cleanup. > > + */ > > The code looks straightforward but I find the comment confusing. How > can > the VFS pass ownership of the xfs_mount if it's an XFS private data > structure? > > > + kfree(mp->m_logname); > > + kfree(mp->m_rtname); > > + kfree(mp); > > + } > > + kfree(ctx); > > Also, should we at least reassign associated fc pointers to NULL if > we > have multiple places to free things like ctx or mp? Not sure about that, I think the next thing to happen here is the mount context is freed by the VFS. I'll check. > > Brian > > > +} > > + > > static const struct fs_context_operations xfs_context_ops = { > > .parse_param = xfs_parse_param, > > .get_tree = xfs_get_tree, > > .reconfigure = xfs_reconfigure, > > + .free = xfs_fc_free, > > }; > > > > static struct file_system_type xfs_fs_type = { > >
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index aae0098fecab..9976163dc537 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -2129,10 +2129,32 @@ static const struct super_operations xfs_super_operations = { .free_cached_objects = xfs_fs_free_cached_objects, }; +static void xfs_fc_free(struct fs_context *fc) +{ + struct xfs_fs_context *ctx = fc->fs_private; + struct xfs_mount *mp = fc->s_fs_info; + + if (mp) { + /* + * If an error occurs before ownership the xfs_mount + * info struct is passed to xfs by the VFS (by assigning + * it to sb->s_fs_info and clearing the corresponding + * fs_context field, which is done before calling fill + * super via .get_tree()) there may be some strings to + * cleanup. + */ + kfree(mp->m_logname); + kfree(mp->m_rtname); + kfree(mp); + } + kfree(ctx); +} + static const struct fs_context_operations xfs_context_ops = { .parse_param = xfs_parse_param, .get_tree = xfs_get_tree, .reconfigure = xfs_reconfigure, + .free = xfs_fc_free, }; static struct file_system_type xfs_fs_type = {
Add the fs_context_operations method .free that performs fs context cleanup on context release. Signed-off-by: Ian Kent <raven@themaw.net> --- fs/xfs/xfs_super.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)