[v4] proc: report open files as size in stat() for /proc/pid/fd

  Many monitoring tools include open file count as a metric. Currently
the only way to get this number is to enumerate the files in /proc/pid/fd.

The problem with the current approach is that it does many things people
generally don't care about when they need one number for a metric.
In our tests for cadvisor, which reports open file counts per cgroup,
we observed that reading the number of open files is slow. Out of 35.23%
of CPU time spent in `proc_readfd_common`, we see 29.43% spent in
`proc_fill_cache`, which is responsible for filling dentry info.
Some of this extra time is spinlock contention, but it's a contention
for the lock we don't want to take to begin with.

We considered putting the number of open files in /proc/pid/status.
Unfortunately, counting the number of fds involves iterating the open_files
bitmap, which has a linear complexity in proportion with the number
of open files (bitmap slots really, but it's close). We don't want
to make /proc/pid/status any slower, so instead we put this info
in /proc/pid/fd as a size member of the stat syscall result.
Previously the reported number was zero, so there's very little
risk of breaking anything, while still providing a somewhat logical
way to count the open files with a fallback if it's zero.

RFC for this patch included iterating open fds under RCU. Thanks
to Frank Hofmann for the suggestion to use the bitmap instead.

Previously:

```
$ sudo stat /proc/1/fd | head -n2
  File: /proc/1/fd
  Size: 0         	Blocks: 0          IO Block: 1024   directory
```

With this patch:

```
$ sudo stat /proc/1/fd | head -n2
  File: /proc/1/fd
  Size: 65        	Blocks: 0          IO Block: 1024   directory
```

Correctness check:

```
$ sudo ls /proc/1/fd | wc -l
65
```

I added the docs for /proc/<pid>/fd while I'm at it.

Signed-off-by: Ivan Babrou <ivan@cloudflare.com>

---
v4: Return errno from proc_fd_getattr() instead of setting negative size.
    Added an explicit include for linux/bitmap.h.
v3: Made use of bitmap_weight() to count the bits.
v2: Added missing rcu_read_lock() / rcu_read_unlock(),
    task_lock() / task_unlock() and put_task_struct().
---
 Documentation/filesystems/proc.rst | 17 +++++++++++
 fs/proc/fd.c                       | 45 ++++++++++++++++++++++++++++++
 2 files changed, 62 insertions(+)
  

On Mon, Oct 24, 2022 at 10:31:40AM -0700, Ivan Babrou wrote:
> Many monitoring tools include open file count as a metric. Currently
> the only way to get this number is to enumerate the files in /proc/pid/fd.
> 
> The problem with the current approach is that it does many things people
> generally don't care about when they need one number for a metric.
> In our tests for cadvisor, which reports open file counts per cgroup,
> we observed that reading the number of open files is slow. Out of 35.23%
> of CPU time spent in `proc_readfd_common`, we see 29.43% spent in
> `proc_fill_cache`, which is responsible for filling dentry info.
> Some of this extra time is spinlock contention, but it's a contention
> for the lock we don't want to take to begin with.
> 
> We considered putting the number of open files in /proc/pid/status.
> Unfortunately, counting the number of fds involves iterating the open_files
> bitmap, which has a linear complexity in proportion with the number
> of open files (bitmap slots really, but it's close). We don't want
> to make /proc/pid/status any slower, so instead we put this info
> in /proc/pid/fd as a size member of the stat syscall result.
> Previously the reported number was zero, so there's very little
> risk of breaking anything, while still providing a somewhat logical
> way to count the open files with a fallback if it's zero.
> 
> RFC for this patch included iterating open fds under RCU. Thanks
> to Frank Hofmann for the suggestion to use the bitmap instead.
> 
> Previously:
> 
> ```
> $ sudo stat /proc/1/fd | head -n2
>   File: /proc/1/fd
>   Size: 0         	Blocks: 0          IO Block: 1024   directory
> ```
> 
> With this patch:
> 
> ```
> $ sudo stat /proc/1/fd | head -n2
>   File: /proc/1/fd
>   Size: 65        	Blocks: 0          IO Block: 1024   directory
> ```
> 
> Correctness check:
> 
> ```
> $ sudo ls /proc/1/fd | wc -l
> 65
> ```
> 
> I added the docs for /proc/<pid>/fd while I'm at it.
> 
> Signed-off-by: Ivan Babrou <ivan@cloudflare.com>
> 
> ---
> v4: Return errno from proc_fd_getattr() instead of setting negative size.
>     Added an explicit include for linux/bitmap.h.
> v3: Made use of bitmap_weight() to count the bits.
> v2: Added missing rcu_read_lock() / rcu_read_unlock(),
>     task_lock() / task_unlock() and put_task_struct().
> ---
>  Documentation/filesystems/proc.rst | 17 +++++++++++
>  fs/proc/fd.c                       | 45 ++++++++++++++++++++++++++++++
>  2 files changed, 62 insertions(+)
> 
> diff --git a/Documentation/filesystems/proc.rst b/Documentation/filesystems/proc.rst
> index 898c99eae8e4..ec6cfdf1796a 100644
> --- a/Documentation/filesystems/proc.rst
> +++ b/Documentation/filesystems/proc.rst
> @@ -47,6 +47,7 @@ fixes/update part 1.1  Stefani Seibold <stefani@seibold.net>    June 9 2009
>    3.10  /proc/<pid>/timerslack_ns - Task timerslack value
>    3.11	/proc/<pid>/patch_state - Livepatch patch operation state
>    3.12	/proc/<pid>/arch_status - Task architecture specific information
> +  3.13  /proc/<pid>/fd - List of symlinks to open files
>  
>    4	Configuring procfs
>    4.1	Mount options
> @@ -2149,6 +2150,22 @@ AVX512_elapsed_ms
>    the task is unlikely an AVX512 user, but depends on the workload and the
>    scheduling scenario, it also could be a false negative mentioned above.
>  
> +3.13 /proc/<pid>/fd - List of symlinks to open files
> +-------------------------------------------------------
> +This directory contains symbolic links which represent open files
> +the process is maintaining.  Example output::
> +
> +  lr-x------ 1 root root 64 Sep 20 17:53 0 -> /dev/null
> +  l-wx------ 1 root root 64 Sep 20 17:53 1 -> /dev/null
> +  lrwx------ 1 root root 64 Sep 20 17:53 10 -> 'socket:[12539]'
> +  lrwx------ 1 root root 64 Sep 20 17:53 11 -> 'socket:[12540]'
> +  lrwx------ 1 root root 64 Sep 20 17:53 12 -> 'socket:[12542]'
> +
> +The number of open files for the process is stored in 'size' member
> +of stat() output for /proc/<pid>/fd for fast access.
> +-------------------------------------------------------
> +
> +
>  Chapter 4: Configuring procfs
>  =============================
>  
> diff --git a/fs/proc/fd.c b/fs/proc/fd.c
> index 913bef0d2a36..fc46d6fe080c 100644
> --- a/fs/proc/fd.c
> +++ b/fs/proc/fd.c
> @@ -7,6 +7,7 @@
>  #include <linux/namei.h>
>  #include <linux/pid.h>
>  #include <linux/ptrace.h>
> +#include <linux/bitmap.h>
>  #include <linux/security.h>
>  #include <linux/file.h>
>  #include <linux/seq_file.h>
> @@ -279,6 +280,30 @@ static int proc_readfd_common(struct file *file, struct dir_context *ctx,
>  	return 0;
>  }
>  
> +static int proc_readfd_count(struct inode *inode, loff_t *count)
> +{
> +	struct task_struct *p = get_proc_task(inode);
> +	struct fdtable *fdt;
> +
> +	if (!p)
> +		return -ENOENT;
> +
> +	task_lock(p);
> +	if (p->files) {
> +		rcu_read_lock();
> +
> +		fdt = files_fdtable(p->files);
> +		*count = bitmap_weight(fdt->open_fds, fdt->max_fds);
> +
> +		rcu_read_unlock();
> +	}
> +	task_unlock(p);
> +
> +	put_task_struct(p);
> +
> +	return 0;
> +}
> +
>  static int proc_readfd(struct file *file, struct dir_context *ctx)
>  {
>  	return proc_readfd_common(file, ctx, proc_fd_instantiate);
> @@ -319,9 +344,29 @@ int proc_fd_permission(struct user_namespace *mnt_userns,
>  	return rv;
>  }
>  
> +static int proc_fd_getattr(struct user_namespace *mnt_userns,
> +			const struct path *path, struct kstat *stat,
> +			u32 request_mask, unsigned int query_flags)
> +{
> +	struct inode *inode = d_inode(path->dentry);
> +	int rv = 0;
> +
> +	generic_fillattr(&init_user_ns, inode, stat);
> +

Sorry I missed this on v3, but shouldn't this pass through the
mnt_userns parameter?

> +	/* If it's a directory, put the number of open fds there */
> +	if (S_ISDIR(inode->i_mode)) {
> +		rv = proc_readfd_count(inode, &stat->size);
> +		if (rv < 0)
> +			return rv;
> +	}

Also I suppose this could just do:

	if (S_ISDIR(inode->i_mode))
		rv = proc_readfd_count(inode, &stat->size);

	return rv;

But that's a nit. Otherwise seems reasonable to me.

Brian

> +
> +	return rv;
> +}
> +
>  const struct inode_operations proc_fd_inode_operations = {
>  	.lookup		= proc_lookupfd,
>  	.permission	= proc_fd_permission,
> +	.getattr	= proc_fd_getattr,
>  	.setattr	= proc_setattr,
>  };
>  
> -- 
> 2.37.3
>
  

On Fri, Nov 18, 2022 at 11:10 AM Brian Foster <bfoster@redhat.com> wrote:
> > +static int proc_fd_getattr(struct user_namespace *mnt_userns,
> > +                     const struct path *path, struct kstat *stat,
> > +                     u32 request_mask, unsigned int query_flags)
> > +{
> > +     struct inode *inode = d_inode(path->dentry);
> > +     int rv = 0;
> > +
> > +     generic_fillattr(&init_user_ns, inode, stat);
> > +
>
> Sorry I missed this on v3, but shouldn't this pass through the
> mnt_userns parameter?

The mnt_userns parameter was added in 549c729 (fs: make helpers idmap
mount aware), and it's not passed anywhere in fs/proc.

Looking at other uses of generic_fillattr, all of them use "init_user_ns":

$ rg generic_fillattr fs/proc
fs/proc/proc_net.c
301: generic_fillattr(&init_user_ns, inode, stat);

fs/proc/base.c
1970: generic_fillattr(&init_user_ns, inode, stat);
3856: generic_fillattr(&init_user_ns, inode, stat);

fs/proc/root.c
315: generic_fillattr(&init_user_ns, d_inode(path->dentry), stat);

fs/proc/generic.c
150: generic_fillattr(&init_user_ns, inode, stat);

fs/proc/proc_sysctl.c
841: generic_fillattr(&init_user_ns, inode, stat);
  

On Fri, Nov 18, 2022 at 11:18:36AM -0800, Ivan Babrou wrote:
> On Fri, Nov 18, 2022 at 11:10 AM Brian Foster <bfoster@redhat.com> wrote:
> > > +static int proc_fd_getattr(struct user_namespace *mnt_userns,
> > > +                     const struct path *path, struct kstat *stat,
> > > +                     u32 request_mask, unsigned int query_flags)
> > > +{
> > > +     struct inode *inode = d_inode(path->dentry);
> > > +     int rv = 0;
> > > +
> > > +     generic_fillattr(&init_user_ns, inode, stat);
> > > +
> >
> > Sorry I missed this on v3, but shouldn't this pass through the
> > mnt_userns parameter?
> 
> The mnt_userns parameter was added in 549c729 (fs: make helpers idmap
> mount aware), and it's not passed anywhere in fs/proc.
> 
> Looking at other uses of generic_fillattr, all of them use "init_user_ns":
> 

Interesting. It looks like this would have used mnt_userns from
vfs_getattr_nosec() before proc_fd_getattr() is wired up, right? I'm not
familiar enough with that change to say whether /proc should use one
value or the other, or perhaps it just doesn't matter.?

Christian?

Brian

> $ rg generic_fillattr fs/proc
> fs/proc/proc_net.c
> 301: generic_fillattr(&init_user_ns, inode, stat);
> 
> fs/proc/base.c
> 1970: generic_fillattr(&init_user_ns, inode, stat);
> 3856: generic_fillattr(&init_user_ns, inode, stat);
> 
> fs/proc/root.c
> 315: generic_fillattr(&init_user_ns, d_inode(path->dentry), stat);
> 
> fs/proc/generic.c
> 150: generic_fillattr(&init_user_ns, inode, stat);
> 
> fs/proc/proc_sysctl.c
> 841: generic_fillattr(&init_user_ns, inode, stat);
>
  

On Fri, Nov 18, 2022 at 02:33:27PM -0500, Brian Foster wrote:
> On Fri, Nov 18, 2022 at 11:18:36AM -0800, Ivan Babrou wrote:
> > On Fri, Nov 18, 2022 at 11:10 AM Brian Foster <bfoster@redhat.com> wrote:
> > > > +static int proc_fd_getattr(struct user_namespace *mnt_userns,
> > > > +                     const struct path *path, struct kstat *stat,
> > > > +                     u32 request_mask, unsigned int query_flags)
> > > > +{
> > > > +     struct inode *inode = d_inode(path->dentry);
> > > > +     int rv = 0;
> > > > +
> > > > +     generic_fillattr(&init_user_ns, inode, stat);
> > > > +
> > >
> > > Sorry I missed this on v3, but shouldn't this pass through the
> > > mnt_userns parameter?
> > 
> > The mnt_userns parameter was added in 549c729 (fs: make helpers idmap
> > mount aware), and it's not passed anywhere in fs/proc.
> > 
> > Looking at other uses of generic_fillattr, all of them use "init_user_ns":
> > 
> 
> Interesting. It looks like this would have used mnt_userns from
> vfs_getattr_nosec() before proc_fd_getattr() is wired up, right? I'm not
> familiar enough with that change to say whether /proc should use one
> value or the other, or perhaps it just doesn't matter.?
> 
> Christian?

Hey Brian,

This should pass init_user_ns. So it is correct the way it is done now.
The init_user_ns is used to indicate that no idmappings are used and
since procfs doesn't support the creation of idmapped mounts and doesn't
need to, passing it here makes the most sense. Technically passing down
mnt_userns would work too but that would make it look like procfs could
support idmapped mounts which isn't the case and so we don't do it this
way.

Starting soon this will be a lot clearer too since we're about to
introduce struct mnt_idmap and replace passing around userns here.
That'll make things also safer as the helpers that currently could be
passed a mnt_userns - which could be any userns - will now only be able
to take mnt_idmap which is a different type.

Long story short, the way your patch does it is correct.

Thanks!
Christian
  

On Sat, Nov 19, 2022 at 01:01:11PM +0100, Christian Brauner wrote:
> On Fri, Nov 18, 2022 at 02:33:27PM -0500, Brian Foster wrote:
> > On Fri, Nov 18, 2022 at 11:18:36AM -0800, Ivan Babrou wrote:
> > > On Fri, Nov 18, 2022 at 11:10 AM Brian Foster <bfoster@redhat.com> wrote:
> > > > > +static int proc_fd_getattr(struct user_namespace *mnt_userns,
> > > > > +                     const struct path *path, struct kstat *stat,
> > > > > +                     u32 request_mask, unsigned int query_flags)
> > > > > +{
> > > > > +     struct inode *inode = d_inode(path->dentry);
> > > > > +     int rv = 0;
> > > > > +
> > > > > +     generic_fillattr(&init_user_ns, inode, stat);
> > > > > +
> > > >
> > > > Sorry I missed this on v3, but shouldn't this pass through the
> > > > mnt_userns parameter?
> > > 
> > > The mnt_userns parameter was added in 549c729 (fs: make helpers idmap
> > > mount aware), and it's not passed anywhere in fs/proc.
> > > 
> > > Looking at other uses of generic_fillattr, all of them use "init_user_ns":
> > > 
> > 
> > Interesting. It looks like this would have used mnt_userns from
> > vfs_getattr_nosec() before proc_fd_getattr() is wired up, right? I'm not
> > familiar enough with that change to say whether /proc should use one
> > value or the other, or perhaps it just doesn't matter.?
> > 
> > Christian?
> 
> Hey Brian,
> 
> This should pass init_user_ns. So it is correct the way it is done now.
> The init_user_ns is used to indicate that no idmappings are used and
> since procfs doesn't support the creation of idmapped mounts and doesn't
> need to, passing it here makes the most sense. Technically passing down
> mnt_userns would work too but that would make it look like procfs could
> support idmapped mounts which isn't the case and so we don't do it this
> way.
> 

Got it, thanks for the context.

Ivan,

Sorry for the noise. FWIW, for this version of the patch:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> Starting soon this will be a lot clearer too since we're about to
> introduce struct mnt_idmap and replace passing around userns here.
> That'll make things also safer as the helpers that currently could be
> passed a mnt_userns - which could be any userns - will now only be able
> to take mnt_idmap which is a different type.
> 
> Long story short, the way your patch does it is correct.
> 
> Thanks!
> Christian
>