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

[Dave Chinner]

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.

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx