| Message ID | 20230808161600.1099516-4-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/13] xfs: reformat the xfs_fs_free prototype | expand |
On Tue, Aug 08, 2023 at 09:15:50AM -0700, Christoph Hellwig wrote: > As a rule of thumb everything allocated to the fs_context and moved into > the super_block should be freed by ->kill_sb so that the teardown > handling doesn't need to be duplicated between the fill_super error > path and put_super. Implement a XFS-specific kill_sb method to do that. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- Looks good to me, Reviewed-by: Christian Brauner <brauner@kernel.org> For filesystems on the new mount api it's always nice if the rules can simply be made: * prior to sget_fc() succeeding fc->s_fs_info and freed in fs_context_operations->free() * after sget_fc() succeeding fc->s_fs_info transfered to sb->s_fs_info and freed in fs_type->kill_sb()
On Tue, Aug 08, 2023 at 09:15:50AM -0700, Christoph Hellwig wrote: > As a rule of thumb everything allocated to the fs_context and moved into > the super_block should be freed by ->kill_sb so that the teardown > handling doesn't need to be duplicated between the fill_super error > path and put_super. Implement a XFS-specific kill_sb method to do that. > > Signed-off-by: Christoph Hellwig <hch@lst.de> Looks good, Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > --- > fs/xfs/xfs_super.c | 20 +++++++++++--------- > 1 file changed, 11 insertions(+), 9 deletions(-) > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index 128f4a2924d49c..d2f3ae6ba8938b 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -1143,9 +1143,6 @@ xfs_fs_put_super( > xfs_destroy_percpu_counters(mp); > xfs_destroy_mount_workqueues(mp); > xfs_close_devices(mp); > - > - sb->s_fs_info = NULL; > - xfs_mount_free(mp); > } > > static long > @@ -1487,7 +1484,7 @@ xfs_fs_fill_super( > > error = xfs_fs_validate_params(mp); > if (error) > - goto out_free_names; > + return error; > > sb_min_blocksize(sb, BBSIZE); > sb->s_xattr = xfs_xattr_handlers; > @@ -1514,7 +1511,7 @@ xfs_fs_fill_super( > > error = xfs_open_devices(mp); > if (error) > - goto out_free_names; > + return error; > > error = xfs_init_mount_workqueues(mp); > if (error) > @@ -1734,9 +1731,6 @@ xfs_fs_fill_super( > xfs_destroy_mount_workqueues(mp); > out_close_devices: > xfs_close_devices(mp); > - out_free_names: > - sb->s_fs_info = NULL; > - xfs_mount_free(mp); > return error; > > out_unmount: > @@ -1999,12 +1993,20 @@ static int xfs_init_fs_context( > return 0; > } > > +static void > +xfs_kill_sb( > + struct super_block *sb) > +{ > + kill_block_super(sb); > + xfs_mount_free(XFS_M(sb)); > +} > + > static struct file_system_type xfs_fs_type = { > .owner = THIS_MODULE, > .name = "xfs", > .init_fs_context = xfs_init_fs_context, > .parameters = xfs_fs_parameters, > - .kill_sb = kill_block_super, > + .kill_sb = xfs_kill_sb, > .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP, > }; > MODULE_ALIAS_FS("xfs"); > -- > 2.39.2 >
On Wed, Aug 09, 2023 at 09:38:13AM +0200, Christian Brauner wrote: > For filesystems on the new mount api it's always nice if the rules > can simply be made: > > * prior to sget_fc() succeeding fc->s_fs_info and freed in fs_context_operations->free() > * after sget_fc() succeeding fc->s_fs_info transfered to sb->s_fs_info and freed in fs_type->kill_sb() Yes. I've been wanting to write this down somewhere, but I can't really think of a good place.
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 128f4a2924d49c..d2f3ae6ba8938b 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -1143,9 +1143,6 @@ xfs_fs_put_super( xfs_destroy_percpu_counters(mp); xfs_destroy_mount_workqueues(mp); xfs_close_devices(mp); - - sb->s_fs_info = NULL; - xfs_mount_free(mp); } static long @@ -1487,7 +1484,7 @@ xfs_fs_fill_super( error = xfs_fs_validate_params(mp); if (error) - goto out_free_names; + return error; sb_min_blocksize(sb, BBSIZE); sb->s_xattr = xfs_xattr_handlers; @@ -1514,7 +1511,7 @@ xfs_fs_fill_super( error = xfs_open_devices(mp); if (error) - goto out_free_names; + return error; error = xfs_init_mount_workqueues(mp); if (error) @@ -1734,9 +1731,6 @@ xfs_fs_fill_super( xfs_destroy_mount_workqueues(mp); out_close_devices: xfs_close_devices(mp); - out_free_names: - sb->s_fs_info = NULL; - xfs_mount_free(mp); return error; out_unmount: @@ -1999,12 +1993,20 @@ static int xfs_init_fs_context( return 0; } +static void +xfs_kill_sb( + struct super_block *sb) +{ + kill_block_super(sb); + xfs_mount_free(XFS_M(sb)); +} + static struct file_system_type xfs_fs_type = { .owner = THIS_MODULE, .name = "xfs", .init_fs_context = xfs_init_fs_context, .parameters = xfs_fs_parameters, - .kill_sb = kill_block_super, + .kill_sb = xfs_kill_sb, .fs_flags = FS_REQUIRES_DEV | FS_ALLOW_IDMAP, }; MODULE_ALIAS_FS("xfs");
As a rule of thumb everything allocated to the fs_context and moved into the super_block should be freed by ->kill_sb so that the teardown handling doesn't need to be duplicated between the fill_super error path and put_super. Implement a XFS-specific kill_sb method to do that. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_super.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-)