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