From: Brian Foster <bfoster@redhat.com>
To: Ivan Babrou <ivan@cloudflare.com>
Cc: linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
kernel-team@cloudflare.com, Alexey Dobriyan <adobriyan@gmail.com>,
Al Viro <viro@zeniv.linux.org.uk>, Theodore Ts'o <tytso@mit.edu>,
David Laight <David.Laight@aculab.com>,
Jonathan Corbet <corbet@lwn.net>,
Andrew Morton <akpm@linux-foundation.org>,
David Hildenbrand <david@redhat.com>,
Johannes Weiner <hannes@cmpxchg.org>,
Christoph Anton Mitterer <mail@christoph.anton.mitterer.name>,
Mike Rapoport <rppt@kernel.org>,
Paul Gortmaker <paul.gortmaker@windriver.com>,
Kalesh Singh <kaleshsingh@google.com>
Subject: Re: [PATCH v3] proc: report open files as size in stat() for /proc/pid/fd
Date: Tue, 18 Oct 2022 14:16:10 -0400 [thread overview]
Message-ID: <Y07taqdJ/J3EyJoB@bfoster> (raw)
In-Reply-To: <20221018045844.37697-1-ivan@cloudflare.com>
On Mon, Oct 17, 2022 at 09:58:44PM -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>
>
> ---
> 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 | 41 ++++++++++++++++++++++++++++++
> 2 files changed, 58 insertions(+)
>
...
> diff --git a/fs/proc/fd.c b/fs/proc/fd.c
> index 913bef0d2a36..439a62c59381 100644
> --- a/fs/proc/fd.c
> +++ b/fs/proc/fd.c
> @@ -279,6 +279,31 @@ static int proc_readfd_common(struct file *file, struct dir_context *ctx,
> return 0;
> }
>
> +static int proc_readfd_count(struct inode *inode)
> +{
> + struct task_struct *p = get_proc_task(inode);
> + struct fdtable *fdt;
> + unsigned int open_fds = 0;
> +
> + if (!p)
> + return -ENOENT;
Maybe this shouldn't happen, but do you mean to assign the error code to
stat->size in the caller? Otherwise this seems reasonable to me.
Brian
> +
> + task_lock(p);
> + if (p->files) {
> + rcu_read_lock();
> +
> + fdt = files_fdtable(p->files);
> + open_fds = bitmap_weight(fdt->open_fds, fdt->max_fds);
> +
> + rcu_read_unlock();
> + }
> + task_unlock(p);
> +
> + put_task_struct(p);
> +
> + return open_fds;
> +}
> +
> static int proc_readfd(struct file *file, struct dir_context *ctx)
> {
> return proc_readfd_common(file, ctx, proc_fd_instantiate);
> @@ -319,9 +344,25 @@ 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);
> +
> + generic_fillattr(&init_user_ns, inode, stat);
> +
> + /* If it's a directory, put the number of open fds there */
> + if (S_ISDIR(inode->i_mode))
> + stat->size = proc_readfd_count(inode);
> +
> + return 0;
> +}
> +
> 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
>
next prev parent reply other threads:[~2022-10-18 18:16 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-18 4:58 [PATCH v3] proc: report open files as size in stat() for /proc/pid/fd Ivan Babrou
2022-10-18 18:16 ` Brian Foster [this message]
2022-10-18 18:51 ` Ivan Babrou
2022-10-19 11:28 ` Brian Foster
2022-10-21 0:52 ` Andrew Morton
2022-10-18 23:13 ` Andrew Morton
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=Y07taqdJ/J3EyJoB@bfoster \
--to=bfoster@redhat.com \
--cc=David.Laight@aculab.com \
--cc=adobriyan@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=corbet@lwn.net \
--cc=david@redhat.com \
--cc=hannes@cmpxchg.org \
--cc=ivan@cloudflare.com \
--cc=kaleshsingh@google.com \
--cc=kernel-team@cloudflare.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mail@christoph.anton.mitterer.name \
--cc=paul.gortmaker@windriver.com \
--cc=rppt@kernel.org \
--cc=tytso@mit.edu \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).