diff mbox series

fs: Make file-nr output the total allocated file handles

Message ID 20250410112117.2851-1-lirongqing@baidu.com (mailing list archive)
State New
Headers show
Series fs: Make file-nr output the total allocated file handles | expand

Commit Message

Li,Rongqing April 10, 2025, 11:21 a.m. UTC
From: Li RongQing <lirongqing@baidu.com>

Make file-nr output the total allocated file handles, not per-cpu
cache number, it's more precise, and not in hot path

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 fs/file_table.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Christian Brauner April 11, 2025, 2:16 p.m. UTC | #1
On Thu, Apr 10, 2025 at 07:21:17PM +0800, lirongqing wrote:
> From: Li RongQing <lirongqing@baidu.com>
> 
> Make file-nr output the total allocated file handles, not per-cpu
> cache number, it's more precise, and not in hot path
> 
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---

That means grabbing a lock suddenly. Is there an actual use-case
behind this?

>  fs/file_table.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/file_table.c b/fs/file_table.c
> index c04ed94..138114d 100644
> --- a/fs/file_table.c
> +++ b/fs/file_table.c
> @@ -102,7 +102,7 @@ EXPORT_SYMBOL_GPL(get_max_files);
>  static int proc_nr_files(const struct ctl_table *table, int write, void *buffer,
>  			 size_t *lenp, loff_t *ppos)
>  {
> -	files_stat.nr_files = get_nr_files();
> +	files_stat.nr_files = percpu_counter_sum_positive(&nr_files);
>  	return proc_doulongvec_minmax(table, write, buffer, lenp, ppos);
>  }
>  
> -- 
> 2.9.4
>
Mateusz Guzik April 11, 2025, 9:15 p.m. UTC | #2
On Fri, Apr 11, 2025 at 04:16:08PM +0200, Christian Brauner wrote:
> On Thu, Apr 10, 2025 at 07:21:17PM +0800, lirongqing wrote:
> > From: Li RongQing <lirongqing@baidu.com>
> > 
> > Make file-nr output the total allocated file handles, not per-cpu
> > cache number, it's more precise, and not in hot path
> > 
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> 
> That means grabbing a lock suddenly. Is there an actual use-case
> behind this?
> 

The centralized value can be really grossly inaccurate as CPU count increases.

There is some talks about fixing that, see:
https://lore.kernel.org/linux-mm/20250410175149.1206995-1-mathieu.desnoyers@efficios.com/

In the meantime, given that this is only accessed when reading the /proc
file, this should be fine?

Note it still wont delay bumps/decs as long as they fit the batch (which
is the common case).
Li,Rongqing April 12, 2025, 12:59 p.m. UTC | #3
> On Thu, Apr 10, 2025 at 07:21:17PM +0800, lirongqing wrote:
> > From: Li RongQing <lirongqing@baidu.com>
> >
> > Make file-nr output the total allocated file handles, not per-cpu
> > cache number, it's more precise, and not in hot path
> >
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> 
> That means grabbing a lock suddenly. Is there an actual use-case behind this?

want to monitor it to find the leaking of the allocated file handler by some applications, but find it is inaccurate

So I think this is not a hot path,  percpu_counter_sum_positive may be able to be used

Thank
-Li
Christian Brauner April 14, 2025, 10:07 a.m. UTC | #4
On Thu, 10 Apr 2025 19:21:17 +0800, lirongqing wrote:
> Make file-nr output the total allocated file handles, not per-cpu
> cache number, it's more precise, and not in hot path
> 
> 

Applied to the vfs-6.16.misc branch of the vfs/vfs.git tree.
Patches in the vfs-6.16.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs-6.16.misc

[1/1] fs: Make file-nr output the total allocated file handles
      https://git.kernel.org/vfs/vfs/c/66319519f89d
Li,Rongqing April 14, 2025, 11:05 a.m. UTC | #5
> On Thu, 10 Apr 2025 19:21:17 +0800, lirongqing wrote:
> > Make file-nr output the total allocated file handles, not per-cpu
> > cache number, it's more precise, and not in hot path
> >
> >
> 


Hi Christian Brauner:

Could we change the changelog as below, it maybe more reasonable

    fs: Use percpu_counter_sum_positive in proc_nr_files

    The centralized value of percpu counter can be really grossly inaccurate
    as CPU count increases. and proc_nr_files is only accessed when reading
    proc file, not a hot path, so use percpu_counter_sum_positive in it, to make
    /proc/sys/fs/file-nr more accurate

thanks


-Li

> Applied to the vfs-6.16.misc branch of the vfs/vfs.git tree.
> Patches in the vfs-6.16.misc branch should appear in linux-next soon.
> 
> Please report any outstanding bugs that were missed during review in a new
> review to the original patch series allowing us to drop it.
> 
> It's encouraged to provide Acked-bys and Reviewed-bys even though the patch
> has now been applied. If possible patch trailers will be updated.
> 
> Note that commit hashes shown below are subject to change due to rebase,
> trailer updates or similar. If in doubt, please check the listed branch.
> 
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
> branch: vfs-6.16.misc
> 
> [1/1] fs: Make file-nr output the total allocated file handles
>       https://git.kernel.org/vfs/vfs/c/66319519f89d
diff mbox series

Patch

diff --git a/fs/file_table.c b/fs/file_table.c
index c04ed94..138114d 100644
--- a/fs/file_table.c
+++ b/fs/file_table.c
@@ -102,7 +102,7 @@  EXPORT_SYMBOL_GPL(get_max_files);
 static int proc_nr_files(const struct ctl_table *table, int write, void *buffer,
 			 size_t *lenp, loff_t *ppos)
 {
-	files_stat.nr_files = get_nr_files();
+	files_stat.nr_files = percpu_counter_sum_positive(&nr_files);
 	return proc_doulongvec_minmax(table, write, buffer, lenp, ppos);
 }