Re: [PATCH 2/3] sysctl: add support for drop_caches for individual filesystem

[Thomas Weißschuh]

On 2024-10-10 19:25:42+0800, Ye Bin wrote:
> From: Ye Bin <yebin10@xxxxxxxxxx>
> 
> In order to better analyze the issue of file system uninstallation caused
> by kernel module opening files, it is necessary to perform dentry recycling
> on a single file system. But now, apart from global dentry recycling, it is
> not supported to do dentry recycling on a single file system separately.
> This feature has usage scenarios in problem localization scenarios.At the
> same time, it also provides users with a slightly fine-grained
> pagecache/entry recycling mechanism.
> This patch supports the recycling of pagecache/entry for individual file
> systems.
> 
> Signed-off-by: Ye Bin <yebin10@xxxxxxxxxx>
> ---
>  fs/drop_caches.c   | 43 +++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mm.h |  2 ++
>  kernel/sysctl.c    |  9 +++++++++
>  3 files changed, 54 insertions(+)
> 
> diff --git a/fs/drop_caches.c b/fs/drop_caches.c
> index d45ef541d848..99d412cf3e52 100644
> --- a/fs/drop_caches.c
> +++ b/fs/drop_caches.c
> @@ -77,3 +77,46 @@ int drop_caches_sysctl_handler(const struct ctl_table *table, int write,
>  	}
>  	return 0;
>  }
> +
> +int drop_fs_caches_sysctl_handler(const struct ctl_table *table, int write,
> +				  void *buffer, size_t *length, loff_t *ppos)
> +{
> +	unsigned int major, minor;
> +	unsigned int ctl;
> +	struct super_block *sb;
> +	static int stfu;
> +
> +	if (!write)
> +		return 0;
> +
> +	if (sscanf(buffer, "%u:%u:%u", &major, &minor, &ctl) != 3)
> +		return -EINVAL;
> +
> +	if (ctl < *((int *)table->extra1) || ctl > *((int *)table->extra2))
> +		return -EINVAL;
> +
> +	sb = user_get_super(MKDEV(major, minor), false);
> +	if (!sb)
> +		return -EINVAL;
> +
> +	if (ctl & 1) {

BIT(0)

> +		lru_add_drain_all();
> +		drop_pagecache_sb(sb, NULL);
> +		count_vm_event(DROP_PAGECACHE);
> +	}
> +
> +	if (ctl & 2) {
> +		shrink_dcache_sb(sb);
> +		shrink_icache_sb(sb);
> +		count_vm_event(DROP_SLAB);
> +	}
> +
> +	drop_super(sb);
> +
> +	if (!stfu)
> +		pr_info("%s (%d): drop_fs_caches: %u:%u:%d\n", current->comm,
> +			task_pid_nr(current), major, minor, ctl);
> +	stfu |= ctl & 4;

This looks very weird. I guess it's already in the original
drop_caches_sysctl_handler().

> +
> +	return 0;
> +}
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 344541f8cba0..43079478296f 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -3788,6 +3788,8 @@ extern bool process_shares_mm(struct task_struct *p, struct mm_struct *mm);
>  extern int sysctl_drop_caches;
>  int drop_caches_sysctl_handler(const struct ctl_table *, int, void *, size_t *,
>  		loff_t *);
> +int drop_fs_caches_sysctl_handler(const struct ctl_table *table, int write,
> +				  void *buffer, size_t *length, loff_t *ppos);
>  #endif
>  
>  void drop_slab(void);
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 79e6cb1d5c48..d434cbe10e47 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2101,6 +2101,15 @@ static struct ctl_table vm_table[] = {

Sooner or later this table should move out of kernel/sysctl.c and into a
subsystem-specific file.
This also means the handler doesn't need to be exported.

>  		.extra1		= SYSCTL_ONE,
>  		.extra2		= SYSCTL_FOUR,
>  	},
> +	{
> +		.procname	= "drop_fs_caches",
> +		.data		= NULL,

NULL is already the default.

> +		.maxlen		= 256,

The maxlen field refers to the data field.
As there is no data, there should be no maxlen.

> +		.mode		= 0200,
> +		.proc_handler	= drop_fs_caches_sysctl_handler,
> +		.extra1         = SYSCTL_ONE,
> +		.extra2         = SYSCTL_FOUR,

These extras are meant as parameters for generic handlers.
Inlining the limits into your hander makes it much clearer.

> +	},
>  	{
>  		.procname	= "page_lock_unfairness",
>  		.data		= &sysctl_page_lock_unfairness,
> -- 
> 2.31.1
>