This patch brings ability to print out auxiliary data associated
with file in procfs interface /proc/pid/fdinfo/fd.
In particular further patches make eventfd, evenpoll, signalfd
and fsnotify to print additional information complete enough
to restore these objects after checkpoint.
To simplify the code we add show_fdinfo callback inside
struct file_operations (as Al proposed and Pavel are proposing).
Signed-off-by: Cyrill Gorcunov <[email protected]>
CC: Pavel Emelyanov <[email protected]>
CC: Al Viro <[email protected]>
CC: Alexey Dobriyan <[email protected]>
CC: Andrew Morton <[email protected]>
CC: James Bottomley <[email protected]>
---
fs/proc/fd.c | 51 ++++++++++++++++++++++++++++++++++++---------------
include/linux/fs.h | 3 +++
2 files changed, 39 insertions(+), 15 deletions(-)
Index: linux-2.6.git/fs/proc/fd.c
===================================================================
--- linux-2.6.git.orig/fs/proc/fd.c
+++ linux-2.6.git/fs/proc/fd.c
@@ -15,11 +15,11 @@
#include "fd.h"
struct proc_fdinfo {
- loff_t f_pos;
- int f_flags;
+ struct file *f_file;
+ int f_flags;
};
-static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct path *path)
+static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct file **f_file, struct path *path)
{
struct files_struct *files = NULL;
struct task_struct *task;
@@ -49,6 +49,10 @@ static int fdinfo_open_helper(struct ino
*path = fd_file->f_path;
path_get(&fd_file->f_path);
}
+ if (f_file) {
+ *f_file = fd_file;
+ get_file(fd_file);
+ }
ret = 0;
}
spin_unlock(&files->file_lock);
@@ -61,28 +65,44 @@ static int fdinfo_open_helper(struct ino
static int seq_show(struct seq_file *m, void *v)
{
struct proc_fdinfo *fdinfo = m->private;
- seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
- (long long)fdinfo->f_pos,
- fdinfo->f_flags);
- return 0;
+ int ret;
+
+ ret = seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
+ (long long)fdinfo->f_file->f_pos,
+ fdinfo->f_flags);
+
+ if (!ret && fdinfo->f_file->f_op->show_fdinfo)
+ ret = fdinfo->f_file->f_op->show_fdinfo(m, fdinfo->f_file);
+
+ return ret;
}
static int seq_fdinfo_open(struct inode *inode, struct file *file)
{
- struct proc_fdinfo *fdinfo = NULL;
- int ret = -ENOENT;
+ struct proc_fdinfo *fdinfo;
+ struct seq_file *m;
+ int ret;
fdinfo = kzalloc(sizeof(*fdinfo), GFP_KERNEL);
if (!fdinfo)
return -ENOMEM;
- ret = fdinfo_open_helper(inode, &fdinfo->f_flags, NULL);
- if (!ret) {
- ret = single_open(file, seq_show, fdinfo);
- if (!ret)
- fdinfo = NULL;
+ ret = fdinfo_open_helper(inode, &fdinfo->f_flags, &fdinfo->f_file, NULL);
+ if (ret)
+ goto err_free;
+
+ ret = single_open(file, seq_show, fdinfo);
+ if (ret) {
+ put_filp(fdinfo->f_file);
+ goto err_free;
}
+ m = file->private_data;
+ m->private = fdinfo;
+
+ return ret;
+
+err_free:
kfree(fdinfo);
return ret;
}
@@ -92,6 +112,7 @@ static int seq_fdinfo_release(struct ino
struct seq_file *m = file->private_data;
struct proc_fdinfo *fdinfo = m->private;
+ put_filp(fdinfo->f_file);
kfree(fdinfo);
return single_release(inode, file);
@@ -173,7 +194,7 @@ static const struct dentry_operations ti
static int proc_fd_link(struct dentry *dentry, struct path *path)
{
- return fdinfo_open_helper(dentry->d_inode, NULL, path);
+ return fdinfo_open_helper(dentry->d_inode, NULL, NULL, path);
}
static struct dentry *
Index: linux-2.6.git/include/linux/fs.h
===================================================================
--- linux-2.6.git.orig/include/linux/fs.h
+++ linux-2.6.git/include/linux/fs.h
@@ -1775,6 +1775,8 @@ struct block_device_operations;
#define HAVE_COMPAT_IOCTL 1
#define HAVE_UNLOCKED_IOCTL 1
+struct seq_file;
+
struct file_operations {
struct module *owner;
loff_t (*llseek) (struct file *, loff_t, int);
@@ -1803,6 +1805,7 @@ struct file_operations {
int (*setlease)(struct file *, long, struct file_lock **);
long (*fallocate)(struct file *file, int mode, loff_t offset,
loff_t len);
+ int (*show_fdinfo)(struct seq_file *m, struct file *f);
};
struct inode_operations {
On Wed, Aug 15, 2012 at 01:21:19PM +0400, Cyrill Gorcunov wrote:
> -static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct path *path)
> +static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct file **f_file, struct path *path)
Bloody bad taste, that... This kind of optional arguments is almost always
a bad sign - tends to happen when you have two barely related functions
crammed into one. And yes, proc_fd_info() suffers the same braindamage.
Trying to avoid code duplication is generally a good thing, but it's not
always worth doing - less obfuscated code wins.
> static int seq_show(struct seq_file *m, void *v)
> {
> struct proc_fdinfo *fdinfo = m->private;
> - seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
> - (long long)fdinfo->f_pos,
> - fdinfo->f_flags);
> - return 0;
> + int ret;
> +
> + ret = seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
> + (long long)fdinfo->f_file->f_pos,
> + fdinfo->f_flags);
Realistically, that one is not going to overflow; you are almost certainly
wasting more cycles on that check of !ret just below than you'll save on
not going into ->show_fdinfo() in case of full buffer.
> + if (!ret && fdinfo->f_file->f_op->show_fdinfo)
> + ret = fdinfo->f_file->f_op->show_fdinfo(m, fdinfo->f_file);
> +
> + return ret;
> }
> + ret = single_open(file, seq_show, fdinfo);
> + if (ret) {
> + put_filp(fdinfo->f_file);
Excuse me? We should *never* do put_filp() on anything that has already
been opened. Think what happens if you race with close(); close() would
rip the reference from descriptor table and do fput(), leaving you with
the last reference to that struct file. You really don't want to just
go and free it. IOW, that one should be fput().
> + put_filp(fdinfo->f_file);
Ditto.
On Wed, Aug 15, 2012 at 01:21:19PM +0400, Cyrill Gorcunov wrote:
> struct proc_fdinfo {
> - loff_t f_pos;
> - int f_flags;
> + struct file *f_file;
> + int f_flags;
> };
> + struct proc_fdinfo *fdinfo;
> + struct seq_file *m;
> + int ret;
>
> fdinfo = kzalloc(sizeof(*fdinfo), GFP_KERNEL);
> if (!fdinfo)
> return -ENOMEM;
> + ret = single_open(file, seq_show, fdinfo);
> + if (ret) {
> + put_filp(fdinfo->f_file);
> + goto err_free;
> }
>
> + m = file->private_data;
> + m->private = fdinfo;
This, BTW, is too convoluted for its own good. What you need is
something like
struct whatever {
struct seq_file *m;
struct file *f;
int flags;
};
with single allocation of that sucker in your ->open(). Set
file->private_data to address of seq_file field in your object *before*
calling seq_open() and don't bother with m->private at all - just use
container_of(m, struct whatever, m) in your ->show() to get to that
structure...
On Wed, Aug 15, 2012 at 10:16:28PM +0100, Al Viro wrote:
> On Wed, Aug 15, 2012 at 01:21:19PM +0400, Cyrill Gorcunov wrote:
> > -static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct path *path)
> > +static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct file **f_file, struct path *path)
>
> Bloody bad taste, that... This kind of optional arguments is almost always
> a bad sign - tends to happen when you have two barely related functions
> crammed into one. And yes, proc_fd_info() suffers the same braindamage.
> Trying to avoid code duplication is generally a good thing, but it's not
> always worth doing - less obfuscated code wins.
Sure I'll update. Thanks.
> > static int seq_show(struct seq_file *m, void *v)
> > {
> > struct proc_fdinfo *fdinfo = m->private;
> > - seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
> > - (long long)fdinfo->f_pos,
> > - fdinfo->f_flags);
> > - return 0;
> > + int ret;
> > +
> > + ret = seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
> > + (long long)fdinfo->f_file->f_pos,
> > + fdinfo->f_flags);
>
> Realistically, that one is not going to overflow; you are almost certainly
> wasting more cycles on that check of !ret just below than you'll save on
> not going into ->show_fdinfo() in case of full buffer.
Yes, this is redundant, thanks. Will fix.
>
> > + if (!ret && fdinfo->f_file->f_op->show_fdinfo)
> > + ret = fdinfo->f_file->f_op->show_fdinfo(m, fdinfo->f_file);
> > +
> > + return ret;
> > }
>
> > + ret = single_open(file, seq_show, fdinfo);
> > + if (ret) {
> > + put_filp(fdinfo->f_file);
>
> Excuse me? We should *never* do put_filp() on anything that has already
> been opened. Think what happens if you race with close(); close() would
> rip the reference from descriptor table and do fput(), leaving you with
> the last reference to that struct file. You really don't want to just
> go and free it. IOW, that one should be fput().
>
> > + put_filp(fdinfo->f_file);
> Ditto.
It seems I indeed missed this scenario, thanks Al, will update!
Cyrill
On Wed, Aug 15, 2012 at 10:29:27PM +0100, Al Viro wrote:
>
> This, BTW, is too convoluted for its own good. What you need is
> something like
> struct whatever {
> struct seq_file *m;
> struct file *f;
> int flags;
> };
> with single allocation of that sucker in your ->open(). Set
> file->private_data to address of seq_file field in your object *before*
> calling seq_open() and don't bother with m->private at all - just use
> container_of(m, struct whatever, m) in your ->show() to get to that
> structure...
I will try and post results, thanks!
Cyrill
On Wed, Aug 15, 2012 at 10:29:27PM +0100, Al Viro wrote:
> This, BTW, is too convoluted for its own good. What you need is
> something like
> struct whatever {
> struct seq_file *m;
> struct file *f;
> int flags;
> };
> with single allocation of that sucker in your ->open(). Set
> file->private_data to address of seq_file field in your object *before*
> calling seq_open() and don't bother with m->private at all - just use
> container_of(m, struct whatever, m) in your ->show() to get to that
> structure...
Al, does the version below looks better? (Since it's harder to review diff,
here is the code after the patch applied).
---
struct proc_fdinfo {
struct seq_file m;
struct file *fd_file;
int f_flags;
};
static int seq_show(struct seq_file *m, void *v)
{
struct proc_fdinfo *fdinfo = container_of(m, struct proc_fdinfo, m);
seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
(long long)fdinfo->fd_file->f_pos, fdinfo->f_flags);
return 0;
}
static int seq_fdinfo_open(struct inode *inode, struct file *file)
{
struct proc_fdinfo *fdinfo = NULL;
struct files_struct *files = NULL;
struct task_struct *task;
struct file *fd_file;
int f_flags, ret;
ret = -ENOENT;
task = get_proc_task(inode);
if (task) {
files = get_files_struct(task);
put_task_struct(task);
}
if (files) {
int fd = proc_fd(inode);
spin_lock(&files->file_lock);
fd_file = fcheck_files(files, fd);
if (fd_file) {
struct fdtable *fdt = files_fdtable(files);
f_flags = fd_file->f_flags & ~O_CLOEXEC;
if (close_on_exec(fd, fdt))
f_flags |= O_CLOEXEC;
get_file(fd_file);
ret = 0;
}
spin_unlock(&files->file_lock);
put_files_struct(files);
}
if (ret)
return ret;
ret = -ENOMEM;
fdinfo = kmalloc(sizeof(*fdinfo), GFP_KERNEL);
if (!fdinfo)
goto err_put;
fdinfo->fd_file = fd_file;
fdinfo->f_flags = f_flags;
file->private_data = &fdinfo->m;
ret = single_open(file, seq_show, NULL);
if (ret)
goto err_free;
return 0;
err_free:
kfree(fdinfo);
err_put:
fput(fd_file);
return ret;
}
static int seq_fdinfo_release(struct inode *inode, struct file *file)
{
struct proc_fdinfo *fdinfo =
container_of((struct seq_file *)file->private_data,
struct proc_fdinfo, m);
fput(fdinfo->fd_file);
return single_release(inode, file);
}
static const struct file_operations proc_fdinfo_file_operations = {
.open = seq_fdinfo_open,
.read = seq_read,
.llseek = seq_lseek,
.release = seq_fdinfo_release,
};
---
From: Cyrill Gorcunov <[email protected]>
Subject: procfs: Convert /proc/pid/fdinfo/ handling routines to seq-file v2
This patch converts /proc/pid/fdinfo/ handling routines to seq-file which
is needed to extend seq operations and plug in auxiliary fdinfo provides
from subsystems like eventfd/eventpoll/fsnotify.
Note the proc_fd_link no longer call for proc_fd_info, simply because
proc_fd_info is converted to seq_fdinfo_open (which is seq-file open()
prototype).
v2 (by Al Viro):
- Don't use helper function with optional arguments, thus proc_fd_info get deprecated
- Use proc_fdinfo structure with seq_file embedded, thus we can use container_of helper
- Use fput to free reference to the file we've grabbed
Signed-off-by: Cyrill Gorcunov <[email protected]>
CC: Pavel Emelyanov <[email protected]>
CC: Al Viro <[email protected]>
CC: Alexey Dobriyan <[email protected]>
CC: Andrew Morton <[email protected]>
CC: James Bottomley <[email protected]>
---
fs/proc/fd.c | 145 ++++++++++++++++++++++++++++++++++++++++-------------------
1 file changed, 99 insertions(+), 46 deletions(-)
Index: linux-2.6.git/fs/proc/fd.c
===================================================================
--- linux-2.6.git.orig/fs/proc/fd.c
+++ linux-2.6.git/fs/proc/fd.c
@@ -6,61 +6,105 @@
#include <linux/namei.h>
#include <linux/pid.h>
#include <linux/security.h>
+#include <linux/file.h>
+#include <linux/seq_file.h>
#include <linux/proc_fs.h>
#include "internal.h"
#include "fd.h"
-#define PROC_FDINFO_MAX 64
+struct proc_fdinfo {
+ struct seq_file m;
+ struct file *fd_file;
+ int f_flags;
+};
+
+static int seq_show(struct seq_file *m, void *v)
+{
+ struct proc_fdinfo *fdinfo = container_of(m, struct proc_fdinfo, m);
+ seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
+ (long long)fdinfo->fd_file->f_pos, fdinfo->f_flags);
+ return 0;
+}
-static int proc_fd_info(struct inode *inode, struct path *path, char *info)
+static int seq_fdinfo_open(struct inode *inode, struct file *file)
{
- struct task_struct *task = get_proc_task(inode);
+ struct proc_fdinfo *fdinfo = NULL;
struct files_struct *files = NULL;
- int fd = proc_fd(inode);
- struct file *file;
+ struct task_struct *task;
+ struct file *fd_file;
+ int f_flags, ret;
+ ret = -ENOENT;
+
+ task = get_proc_task(inode);
if (task) {
files = get_files_struct(task);
put_task_struct(task);
}
+
if (files) {
- /*
- * We are not taking a ref to the file structure, so we must
- * hold ->file_lock.
- */
+ int fd = proc_fd(inode);
+
spin_lock(&files->file_lock);
- file = fcheck_files(files, fd);
- if (file) {
- unsigned int f_flags;
- struct fdtable *fdt;
+ fd_file = fcheck_files(files, fd);
+ if (fd_file) {
+ struct fdtable *fdt = files_fdtable(files);
- fdt = files_fdtable(files);
- f_flags = file->f_flags & ~O_CLOEXEC;
+ f_flags = fd_file->f_flags & ~O_CLOEXEC;
if (close_on_exec(fd, fdt))
f_flags |= O_CLOEXEC;
- if (path) {
- *path = file->f_path;
- path_get(&file->f_path);
- }
- if (info)
- snprintf(info, PROC_FDINFO_MAX,
- "pos:\t%lli\n"
- "flags:\t0%o\n",
- (long long) file->f_pos,
- f_flags);
- spin_unlock(&files->file_lock);
- put_files_struct(files);
- return 0;
+ get_file(fd_file);
+ ret = 0;
}
spin_unlock(&files->file_lock);
+
put_files_struct(files);
}
- return -ENOENT;
+
+ if (ret)
+ return ret;
+
+ ret = -ENOMEM;
+ fdinfo = kmalloc(sizeof(*fdinfo), GFP_KERNEL);
+ if (!fdinfo)
+ goto err_put;
+
+ fdinfo->fd_file = fd_file;
+ fdinfo->f_flags = f_flags;
+ file->private_data = &fdinfo->m;
+
+ ret = single_open(file, seq_show, NULL);
+ if (ret)
+ goto err_free;
+
+ return 0;
+
+err_free:
+ kfree(fdinfo);
+err_put:
+ fput(fd_file);
+ return ret;
}
+static int seq_fdinfo_release(struct inode *inode, struct file *file)
+{
+ struct proc_fdinfo *fdinfo =
+ container_of((struct seq_file *)file->private_data,
+ struct proc_fdinfo, m);
+ fput(fdinfo->fd_file);
+ return single_release(inode, file);
+}
+
+static const struct file_operations proc_fdinfo_file_operations = {
+ .open = seq_fdinfo_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = seq_fdinfo_release,
+};
+
static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags)
{
struct files_struct *files;
@@ -130,7 +174,32 @@ static const struct dentry_operations ti
static int proc_fd_link(struct dentry *dentry, struct path *path)
{
- return proc_fd_info(dentry->d_inode, path, NULL);
+ struct files_struct *files = NULL;
+ struct task_struct *task;
+ int ret = -ENOENT;
+
+ task = get_proc_task(dentry->d_inode);
+ if (task) {
+ files = get_files_struct(task);
+ put_task_struct(task);
+ }
+
+ if (files) {
+ int fd = proc_fd(dentry->d_inode);
+ struct file *fd_file;
+
+ spin_lock(&files->file_lock);
+ fd_file = fcheck_files(files, fd);
+ if (fd_file) {
+ *path = fd_file->f_path;
+ path_get(&fd_file->f_path);
+ ret = 0;
+ }
+ spin_unlock(&files->file_lock);
+ put_files_struct(files);
+ }
+
+ return ret;
}
static struct dentry *
@@ -245,22 +314,6 @@ out_no_task:
return retval;
}
-static ssize_t proc_fdinfo_read(struct file *file, char __user *buf,
- size_t len, loff_t *ppos)
-{
- char tmp[PROC_FDINFO_MAX];
- int err = proc_fd_info(file->f_path.dentry->d_inode, NULL, tmp);
- if (!err)
- err = simple_read_from_buffer(buf, len, ppos, tmp, strlen(tmp));
- return err;
-}
-
-static const struct file_operations proc_fdinfo_file_operations = {
- .open = nonseekable_open,
- .read = proc_fdinfo_read,
- .llseek = no_llseek,
-};
-
static int proc_readfd(struct file *filp, void *dirent, filldir_t filldir)
{
return proc_readfd_common(filp, dirent, filldir, proc_fd_instantiate);