Re: [PATCH 6/8] xfs: introduce xfs_fs_destroy_super()

[Christian Brauner <brauner@kernel.org>]

On Thu, Jun 01, 2023 at 04:43:32PM +0800, Qi Zheng wrote:
> Hi Dave,
> 
> On 2023/6/1 07:48, Dave Chinner wrote:
> > On Wed, May 31, 2023 at 09:57:40AM +0000, Qi Zheng wrote:
> > > From: Kirill Tkhai <tkhai@ya.ru>
> > > 
> > > xfs_fs_nr_cached_objects() touches sb->s_fs_info,
> > > and this patch makes it to be destructed later.
> > > 
> > > After this patch xfs_fs_nr_cached_objects() is safe
> > > for splitting unregister_shrinker(): mp->m_perag_tree
> > > is stable till destroy_super_work(), while iteration
> > > over it is already RCU-protected by internal XFS
> > > business.
> > > 
> > > Signed-off-by: Kirill Tkhai <tkhai@ya.ru>
> > > Signed-off-by: Qi Zheng <zhengqi.arch@bytedance.com>
> > > ---
> > >   fs/xfs/xfs_super.c | 25 ++++++++++++++++++++++---
> > >   1 file changed, 22 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > index 7e706255f165..694616524c76 100644
> > > --- a/fs/xfs/xfs_super.c
> > > +++ b/fs/xfs/xfs_super.c
> > > @@ -743,11 +743,18 @@ xfs_fs_drop_inode(
> > >   }
> > >   static void
> > > -xfs_mount_free(
> > > +xfs_free_names(
> > >   	struct xfs_mount	*mp)
> > >   {
> > >   	kfree(mp->m_rtname);
> > >   	kfree(mp->m_logname);
> > > +}
> > > +
> > > +static void
> > > +xfs_mount_free(
> > > +	struct xfs_mount	*mp)
> > > +{
> > > +	xfs_free_names(mp);
> > >   	kmem_free(mp);
> > >   }
> > > @@ -1136,8 +1143,19 @@ xfs_fs_put_super(
> > >   	xfs_destroy_mount_workqueues(mp);
> > >   	xfs_close_devices(mp);
> > > -	sb->s_fs_info = NULL;
> > > -	xfs_mount_free(mp);
> > > +	xfs_free_names(mp);
> > > +}
> > > +
> > > +static void
> > > +xfs_fs_destroy_super(
> > > +	struct super_block	*sb)
> > > +{
> > > +	if (sb->s_fs_info) {
> > > +		struct xfs_mount	*mp = XFS_M(sb);
> > > +
> > > +		kmem_free(mp);
> > > +		sb->s_fs_info = NULL;
> > > +	}
> > >   }
> > >   static long
> > > @@ -1165,6 +1183,7 @@ static const struct super_operations xfs_super_operations = {
> > >   	.dirty_inode		= xfs_fs_dirty_inode,
> > >   	.drop_inode		= xfs_fs_drop_inode,
> > >   	.put_super		= xfs_fs_put_super,
> > > +	.destroy_super		= xfs_fs_destroy_super,
> > >   	.sync_fs		= xfs_fs_sync_fs,
> > >   	.freeze_fs		= xfs_fs_freeze,
> > >   	.unfreeze_fs		= xfs_fs_unfreeze,
> > 
> > I don't really like this ->destroy_super() callback, especially as
> > it's completely undocumented as to why it exists. This is purely a
> > work-around for handling extended filesystem superblock shrinker
> > functionality, yet there's nothing that tells the reader this.
> > 
> > It also seems to imply that the superblock shrinker can continue to
> > run after the existing unregister_shrinker() call before ->kill_sb()
> > is called. This violates the assumption made in filesystems that the
> > superblock shrinkers have been stopped and will never run again
> > before ->kill_sb() is called. Hence ->kill_sb() implementations
> > assume there is nothing else accessing filesystem owned structures
> > and it can tear down internal structures safely.
> > 
> > Realistically, the days of XFS using this superblock shrinker
> > extension are numbered. We've got a lot of the infrastructure we
> > need in place to get rid of the background inode reclaim
> > infrastructure that requires this shrinker extension, and it's on my
> > list of things that need to be addressed in the near future.
> > 
> > In fact, now that I look at it, I think the shmem usage of this
> > superblock shrinker interface is broken - it returns SHRINK_STOP to
> > ->free_cached_objects(), but the only valid return value is the
> > number of objects freed (i.e. 0 is nothing freed). These special
> > superblock extension interfaces do not work like a normal
> > shrinker....
> > 
> > Hence I think the shmem usage should be replaced with an separate
> > internal shmem shrinker that is managed by the filesystem itself
> > (similar to how XFS has multiple internal shrinkers).
> > 
> > At this point, then the only user of this interface is (again) XFS.
> > Given this, adding new VFS methods for a single filesystem
> > for functionality that is planned to be removed is probably not the
> > best approach to solving the problem.
> 
> Thanks for such a detailed analysis. Kirill Tkhai just proposeed a
> new method[1], I cc'd you on the email.
> 
> [1].
> https://lore.kernel.org/lkml/bab60fe4-964c-43a6-ecce-4cbd4981d875@ya.ru/

As long as we agree that we're not adding a new super operation that
sounds like a better way forward.