lkml.org 
[lkml]   [2023]   [Jun]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 6/8] xfs: introduce xfs_fs_destroy_super()
From
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/

Thanks,
Qi

>
> Cheers,
>
> Dave.

\
 
 \ /
  Last update: 2023-06-01 10:44    [W:0.742 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site