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

[Qi Zheng]

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@xxxxx>
> >
> > 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@xxxxx>
> > Signed-off-by: Qi Zheng <zhengqi.arch@xxxxxxxxxxxxx>
> > ---
> >   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@xxxxx/

Thanks,
Qi

> Cheers,
>
> Dave.