linux-kernel - Re: [PATCH 6/8] xfs: introduce xfs_fs_destroy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b85c0d63-f6a5-73c4-e574-163b0b07d80a@linux.dev>
Date:   Thu, 1 Jun 2023 16:43:32 +0800
From:   Qi Zheng <qi.zheng@...ux.dev>
To:     Dave Chinner <david@...morbit.com>
Cc:     akpm@...ux-foundation.org, tkhai@...ru, roman.gushchin@...ux.dev,
        vbabka@...e.cz, viro@...iv.linux.org.uk, brauner@...nel.org,
        djwong@...nel.org, hughd@...gle.com, paulmck@...nel.org,
        muchun.song@...ux.dev, linux-mm@...ck.org,
        linux-fsdevel@...r.kernel.org, linux-xfs@...r.kernel.org,
        linux-kernel@...r.kernel.org, Qi Zheng <zhengqi.arch@...edance.com>
Subject: Re: [PATCH 6/8] xfs: introduce xfs_fs_destroy_super()

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@...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@...ru>
>> Signed-off-by: Qi Zheng <zhengqi.arch@...edance.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.