2012-08-14 14:06:41

by Cyrill Gorcunov

[permalink] [raw]
Subject: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

This patch brings ability to plug in auxiliary fdinfo providers.
For example in further patches eventfd, evenpoll and fsnotify
will print out information associated with files.

This feature is CONFIG_CHECKPOINT_RESTORE guarded to eliminate
overhead for those who don't need it at all (this
unfortunately makes patch bigger than I wanted).

The basic usage rule is the following

- fdinfo provider should register own "show" method
via proc_register_fdinfo_driver call, where "show"
methods are rather well known seq-file operations

- once the kernel opens /proc/$pid/fdinfo/$fd file
it calls for ->probe() method in registered fdinfo
drivers, and if probe success, then seq-file "show"
operations will be called to provide out additional
infomation

Initially we considered to inject some "show" metod to
file_operations but since there really a number of
file_operations declared inside kernel (and in real the
further patches cover onle eventfd/epoll/inotify) the
waste of memory space will be inacceptable I think.

Pavel, I've left seq_next memthod as it was simply because
we can't leave seq_next() after calling extra->driver->ops->start
without increasing "pos", thus we need to call for "show" manually
once.

Signed-off-by: Cyrill Gorcunov <[email protected]>
CC: Al Viro <[email protected]>
CC: Alexey Dobriyan <[email protected]>
CC: Andrew Morton <[email protected]>
CC: Pavel Emelyanov <[email protected]>
CC: James Bottomley <[email protected]>
---
fs/proc/fd.c | 198 ++++++++++++++++++++++++++++++++++++++++++++++--
include/linux/proc_fs.h | 26 ++++++
2 files changed, 216 insertions(+), 8 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
@@ -8,18 +8,14 @@
#include <linux/security.h>
#include <linux/file.h>
#include <linux/seq_file.h>
+#include <linux/spinlock.h>

#include <linux/proc_fs.h>

#include "internal.h"
#include "fd.h"

-struct proc_fdinfo {
- loff_t f_pos;
- 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 +45,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);
@@ -58,6 +58,186 @@ static int fdinfo_open_helper(struct ino
return ret;
}

+#ifdef CONFIG_CHECKPOINT_RESTORE
+
+static LIST_HEAD(fdinfo_drivers);
+static DECLARE_RWSEM(fdinfo_drivers_sem);
+
+int proc_register_fdinfo_driver(struct proc_fdinfo_driver *s)
+{
+ struct proc_fdinfo_driver *i;
+ int ret = 0;
+
+ if (!s->ops || !s->probe)
+ return -EINVAL;
+
+ down_write(&fdinfo_drivers_sem);
+ list_for_each_entry(i, &fdinfo_drivers, list) {
+ if (i == s) {
+ WARN_ONCE("Trying reassign fdinfo driver `%s'\n",
+ i->name);
+ ret = -EINVAL;
+ break;
+ }
+ }
+ if (!ret)
+ list_add(&s->list, &fdinfo_drivers);
+ up_write(&fdinfo_drivers_sem);
+
+ return ret;
+}
+
+void proc_unregister_fdinfo_driver(struct proc_fdinfo_driver *s)
+{
+ struct proc_fdinfo_driver *i, *tmp;
+
+ down_write(&fdinfo_drivers_sem);
+ list_for_each_entry_safe(i, tmp, &fdinfo_drivers, list) {
+ if (i == s) {
+ list_del(&i->list);
+ break;
+ }
+ }
+ up_write(&fdinfo_drivers_sem);
+}
+
+static int prep_fdinfo_driver(struct proc_fdinfo_extra *extra)
+{
+ struct proc_fdinfo_driver *s;
+
+ down_read(&fdinfo_drivers_sem);
+ list_for_each_entry(s, &fdinfo_drivers, list) {
+ if (s->probe(extra->f_file)) {
+ extra->driver = s;
+ break;
+ }
+ }
+ up_read(&fdinfo_drivers_sem);
+
+ return 0;
+}
+
+static void *seq_start(struct seq_file *m, loff_t *pos)
+{
+ struct proc_fdinfo_extra *extra = m->private;
+
+ down_read(&fdinfo_drivers_sem);
+ extra->pos = *pos;
+
+ return *pos == 0 ? extra :
+ (extra->driver ? extra->driver->ops->start(m, pos) : NULL);
+}
+
+static void seq_stop(struct seq_file *m, void *v)
+{
+ struct proc_fdinfo_extra *extra = m->private;
+
+ if (extra->driver && extra->pos > 0)
+ extra->driver->ops->stop(m, v);
+ up_read(&fdinfo_drivers_sem);
+}
+
+static void *seq_next(struct seq_file *m, void *p, loff_t *pos)
+{
+ struct proc_fdinfo_extra *extra = m->private;
+ void *v = NULL;
+
+ if (extra->driver) {
+ int ret = 0;
+
+ if (*pos == 0) {
+ v = extra->driver->ops->start(m, pos);
+ if (v) {
+ ret = extra->driver->ops->show(m, v);
+ p = v;
+ } else
+ ret = -1;
+ }
+
+ if (!ret)
+ v = extra->driver->ops->next(m, p, pos);
+ } else
+ ++*pos;
+
+ extra->pos = *pos;
+ return v;
+}
+
+static int seq_show(struct seq_file *m, void *v)
+{
+ struct proc_fdinfo_extra *extra = m->private;
+
+ if (extra->driver && extra->pos > 0)
+ return extra->driver->ops->show(m, v);
+
+ seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
+ (long long)extra->f_file->f_pos,
+ extra->f_flags);
+ return 0;
+}
+
+static const struct seq_operations fdinfo_seq_ops = {
+ .start = seq_start,
+ .next = seq_next,
+ .stop = seq_stop,
+ .show = seq_show,
+};
+
+static int seq_fdinfo_open(struct inode *inode, struct file *file)
+{
+ struct proc_fdinfo_extra *extra;
+ struct seq_file *m;
+ int ret;
+
+ extra = kzalloc(sizeof(*extra), GFP_KERNEL);
+ if (!extra)
+ return -ENOMEM;
+
+ ret = seq_open(file, &fdinfo_seq_ops);
+ if (!ret) {
+ ret = -ENOENT;
+ m = file->private_data;
+ m->private = extra;
+
+ ret = fdinfo_open_helper(inode, &extra->f_flags,
+ &extra->f_file, NULL);
+ if (!ret)
+ ret = prep_fdinfo_driver(extra);
+ }
+
+ if (ret) {
+ if (extra->f_file)
+ put_filp(extra->f_file);
+ kfree(extra);
+ }
+ return ret;
+}
+
+static int seq_fdinfo_release(struct inode *inode, struct file *file)
+{
+ struct seq_file *m = file->private_data;
+ struct proc_fdinfo_extra *extra = m->private;
+
+ put_filp(extra->f_file);
+ kfree(m->private);
+
+ return seq_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,
+};
+
+#else /* CONFIG_CHECKPOINT_RESTORE */
+
+struct proc_fdinfo {
+ loff_t f_pos;
+ int f_flags;
+};
+
static int seq_show(struct seq_file *m, void *v)
{
struct proc_fdinfo *fdinfo = m->private;
@@ -76,7 +256,7 @@ static int seq_fdinfo_open(struct inode
if (!fdinfo)
return -ENOMEM;

- ret = fdinfo_open_helper(inode, &fdinfo->f_flags, NULL);
+ ret = fdinfo_open_helper(inode, &fdinfo->f_flags, NULL, NULL);
if (!ret) {
ret = single_open(file, seq_show, fdinfo);
if (!ret)
@@ -104,6 +284,8 @@ static const struct file_operations proc
.release = seq_fdinfo_release,
};

+#endif /* CONFIG_CHECKPOINT_RESTORE */
+
static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags)
{
struct files_struct *files;
@@ -173,7 +355,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/proc_fs.h
===================================================================
--- linux-2.6.git.orig/include/linux/proc_fs.h
+++ linux-2.6.git/include/linux/proc_fs.h
@@ -100,6 +100,24 @@ struct vmcore {
loff_t offset;
};

+struct seq_operations;
+
+/* fdinfo auxiliary information drivers */
+struct proc_fdinfo_driver {
+ struct list_head list;
+ const char *name;
+ const struct seq_operations *ops;
+ int (*probe)(struct file *file);
+};
+
+/* auxiliary data allocated per fdinfo reader */
+struct proc_fdinfo_extra {
+ struct proc_fdinfo_driver *driver;
+ loff_t pos;
+ struct file *f_file;
+ unsigned int f_flags;
+};
+
#ifdef CONFIG_PROC_FS

extern void proc_root_init(void);
@@ -175,6 +193,11 @@ extern struct proc_dir_entry *proc_net_m

extern struct file *proc_ns_fget(int fd);

+#ifdef CONFIG_CHECKPOINT_RESTORE
+extern int proc_register_fdinfo_driver(struct proc_fdinfo_driver *s);
+extern void proc_unregister_fdinfo_driver(struct proc_fdinfo_driver *s);
+#endif /* CONFIG_CHECKPOINT_RESTORE */
+
#else

#define proc_net_fops_create(net, name, mode, fops) ({ (void)(mode), NULL; })
@@ -229,6 +252,9 @@ static inline struct file *proc_ns_fget(
return ERR_PTR(-EINVAL);
}

+static inline int proc_register_fdinfo_driver(struct proc_fdinfo_driver *s) { return -EINVAL; }
+static inline void proc_unregister_fdinfo_driver(struct proc_fdinfo_driver *s) { }
+
#endif /* CONFIG_PROC_FS */

#if !defined(CONFIG_PROC_KCORE)


2012-08-14 14:22:21

by Pavel Emelyanov

[permalink] [raw]
Subject: Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

On 08/14/2012 06:03 PM, Cyrill Gorcunov wrote:
> This patch brings ability to plug in auxiliary fdinfo providers.
> For example in further patches eventfd, evenpoll and fsnotify
> will print out information associated with files.
>
> This feature is CONFIG_CHECKPOINT_RESTORE guarded to eliminate
> overhead for those who don't need it at all (this
> unfortunately makes patch bigger than I wanted).
>
> The basic usage rule is the following
>
> - fdinfo provider should register own "show" method
> via proc_register_fdinfo_driver call, where "show"
> methods are rather well known seq-file operations
>
> - once the kernel opens /proc/$pid/fdinfo/$fd file
> it calls for ->probe() method in registered fdinfo
> drivers, and if probe success, then seq-file "show"
> operations will be called to provide out additional
> infomation
>
> Initially we considered to inject some "show" metod to
> file_operations but since there really a number of
> file_operations declared inside kernel (and in real the
> further patches cover onle eventfd/epoll/inotify) the
> waste of memory space will be inacceptable I think.
>
> Pavel, I've left seq_next memthod as it was simply because
> we can't leave seq_next() after calling extra->driver->ops->start
> without increasing "pos", thus we need to call for "show" manually
> once.
>
> Signed-off-by: Cyrill Gorcunov <[email protected]>
> CC: Al Viro <[email protected]>
> CC: Alexey Dobriyan <[email protected]>
> CC: Andrew Morton <[email protected]>
> CC: Pavel Emelyanov <[email protected]>
> CC: James Bottomley <[email protected]>

Acked-by: Pavel Emelyanov <[email protected]>

2012-08-14 18:31:45

by Al Viro

[permalink] [raw]
Subject: Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

On Tue, Aug 14, 2012 at 06:03:45PM +0400, Cyrill Gorcunov wrote:
> This patch brings ability to plug in auxiliary fdinfo providers.
> For example in further patches eventfd, evenpoll and fsnotify
> will print out information associated with files.
>
> This feature is CONFIG_CHECKPOINT_RESTORE guarded to eliminate
> overhead for those who don't need it at all (this
> unfortunately makes patch bigger than I wanted).
>
> The basic usage rule is the following
>
> - fdinfo provider should register own "show" method
> via proc_register_fdinfo_driver call, where "show"
> methods are rather well known seq-file operations
>
> - once the kernel opens /proc/$pid/fdinfo/$fd file
> it calls for ->probe() method in registered fdinfo
> drivers, and if probe success, then seq-file "show"
> operations will be called to provide out additional
> infomation
>
> Initially we considered to inject some "show" metod to
> file_operations but since there really a number of
> file_operations declared inside kernel (and in real the
> further patches cover onle eventfd/epoll/inotify) the
> waste of memory space will be inacceptable I think.

NAK. This is too ugly to live. Put it into file_operations,
with NULL meaning default output, or don't do it at all.

And no, "it's only if you enable CONFIG_SOME_SHIT" gambit won't
fly - we have all seen it played too many times. All it takes
is one politically-inclined induhvidual adding a dependency to
some "vertically integrated" turd (*cough* systemd *spit* udev
*cough*) and we are stuck with the damn thing. CGROUP shite
is already there, DEVTMPFS is well on its way, etc.

2012-08-14 18:36:05

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

On Tue, Aug 14, 2012 at 07:31:42PM +0100, Al Viro wrote:
> > Initially we considered to inject some "show" metod to
> > file_operations but since there really a number of
> > file_operations declared inside kernel (and in real the
> > further patches cover onle eventfd/epoll/inotify) the
> > waste of memory space will be inacceptable I think.
>
> NAK. This is too ugly to live. Put it into file_operations,
> with NULL meaning default output, or don't do it at all.
>
> And no, "it's only if you enable CONFIG_SOME_SHIT" gambit won't
> fly - we have all seen it played too many times. All it takes
> is one politically-inclined induhvidual adding a dependency to
> some "vertically integrated" turd (*cough* systemd *spit* udev
> *cough*) and we are stuck with the damn thing. CGROUP shite
> is already there, DEVTMPFS is well on its way, etc.

OK, I'll put it into file_operations then (actually Pavel was
proposing the same but I've been scared by amount of file_operations
declared). Thanks!

Cyrill

2012-08-14 19:56:57

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

On Tue, Aug 14, 2012 at 10:35:58PM +0400, Cyrill Gorcunov wrote:
> On Tue, Aug 14, 2012 at 07:31:42PM +0100, Al Viro wrote:
> > > Initially we considered to inject some "show" metod to
> > > file_operations but since there really a number of
> > > file_operations declared inside kernel (and in real the
> > > further patches cover onle eventfd/epoll/inotify) the
> > > waste of memory space will be inacceptable I think.
> >
> > NAK. This is too ugly to live. Put it into file_operations,
> > with NULL meaning default output, or don't do it at all.
> >
> > And no, "it's only if you enable CONFIG_SOME_SHIT" gambit won't
> > fly - we have all seen it played too many times. All it takes
> > is one politically-inclined induhvidual adding a dependency to
> > some "vertically integrated" turd (*cough* systemd *spit* udev
> > *cough*) and we are stuck with the damn thing. CGROUP shite
> > is already there, DEVTMPFS is well on its way, etc.
>
> OK, I'll put it into file_operations then (actually Pavel was
> proposing the same but I've been scared by amount of file_operations
> declared). Thanks!

Al, does the patch below looks better? If so I'll fix up the rest.
---
fs/proc/fd.c | 109 ++++++++++++++++++++++++++++++++++++++----------
include/linux/fs.h | 3 +
include/linux/proc_fs.h | 8 +++
3 files changed, 99 insertions(+), 21 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
@@ -8,18 +8,14 @@
#include <linux/security.h>
#include <linux/file.h>
#include <linux/seq_file.h>
+#include <linux/spinlock.h>

#include <linux/proc_fs.h>

#include "internal.h"
#include "fd.h"

-struct proc_fdinfo {
- loff_t f_pos;
- 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 +45,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);
@@ -58,43 +58,110 @@ static int fdinfo_open_helper(struct ino
return ret;
}

+static void *seq_start(struct seq_file *m, loff_t *pos)
+{
+ struct proc_fdinfo_extra *extra = m->private;
+
+ extra->pos = *pos;
+
+ return *pos == 0 ? extra :
+ (extra->fdinfo_ops ?
+ extra->fdinfo_ops->start(m, pos) : NULL);
+}
+
+static void seq_stop(struct seq_file *m, void *v)
+{
+ struct proc_fdinfo_extra *extra = m->private;
+
+ if (extra->fdinfo_ops && extra->pos > 0)
+ extra->fdinfo_ops->stop(m, v);
+}
+
+static void *seq_next(struct seq_file *m, void *p, loff_t *pos)
+{
+ struct proc_fdinfo_extra *extra = m->private;
+ void *v = NULL;
+
+ if (extra->fdinfo_ops) {
+ int ret = 0;
+
+ if (*pos == 0) {
+ v = extra->fdinfo_ops->start(m, pos);
+ if (v) {
+ ret = extra->fdinfo_ops->show(m, v);
+ p = v;
+ } else
+ ret = -1;
+ }
+
+ if (!ret)
+ v = extra->fdinfo_ops->next(m, p, pos);
+ } else
+ ++*pos;
+
+ extra->pos = *pos;
+ return v;
+}
+
static int seq_show(struct seq_file *m, void *v)
{
- struct proc_fdinfo *fdinfo = m->private;
+ struct proc_fdinfo_extra *extra = m->private;
+
+ if (extra->fdinfo_ops && extra->pos > 0)
+ return extra->fdinfo_ops->show(m, v);
+
seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
- (long long)fdinfo->f_pos,
- fdinfo->f_flags);
+ (long long)extra->f_file->f_pos,
+ extra->f_flags);
return 0;
}

+static const struct seq_operations fdinfo_seq_ops = {
+ .start = seq_start,
+ .next = seq_next,
+ .stop = seq_stop,
+ .show = seq_show,
+};
+
static int seq_fdinfo_open(struct inode *inode, struct file *file)
{
- struct proc_fdinfo *fdinfo = NULL;
- int ret = -ENOENT;
+ struct proc_fdinfo_extra *extra;
+ struct seq_file *m;
+ int ret;

- fdinfo = kzalloc(sizeof(*fdinfo), GFP_KERNEL);
- if (!fdinfo)
+ extra = kzalloc(sizeof(*extra), GFP_KERNEL);
+ if (!extra)
return -ENOMEM;

- ret = fdinfo_open_helper(inode, &fdinfo->f_flags, NULL);
+ ret = seq_open(file, &fdinfo_seq_ops);
if (!ret) {
- ret = single_open(file, seq_show, fdinfo);
+ ret = -ENOENT;
+ m = file->private_data;
+ m->private = extra;
+
+ ret = fdinfo_open_helper(inode, &extra->f_flags,
+ &extra->f_file, NULL);
if (!ret)
- fdinfo = NULL;
+ extra->fdinfo_ops = extra->f_file->f_op->fdinfo_ops;
}

- kfree(fdinfo);
+ if (ret) {
+ if (extra->f_file)
+ put_filp(extra->f_file);
+ kfree(extra);
+ }
return ret;
}

static int seq_fdinfo_release(struct inode *inode, struct file *file)
{
struct seq_file *m = file->private_data;
- struct proc_fdinfo *fdinfo = m->private;
+ struct proc_fdinfo_extra *extra = m->private;

- kfree(fdinfo);
+ put_filp(extra->f_file);
+ kfree(m->private);

- return single_release(inode, file);
+ return seq_release(inode, file);
}

static const struct file_operations proc_fdinfo_file_operations = {
@@ -173,7 +240,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,8 +1775,11 @@ struct block_device_operations;
#define HAVE_COMPAT_IOCTL 1
#define HAVE_UNLOCKED_IOCTL 1

+struct seq_operations;
+
struct file_operations {
struct module *owner;
+ struct seq_operations *fdinfo_ops;
loff_t (*llseek) (struct file *, loff_t, int);
ssize_t (*read) (struct file *, char __user *, size_t, loff_t *);
ssize_t (*write) (struct file *, const char __user *, size_t, loff_t *);
Index: linux-2.6.git/include/linux/proc_fs.h
===================================================================
--- linux-2.6.git.orig/include/linux/proc_fs.h
+++ linux-2.6.git/include/linux/proc_fs.h
@@ -100,6 +100,14 @@ struct vmcore {
loff_t offset;
};

+/* auxiliary data allocated per fdinfo reader */
+struct proc_fdinfo_extra {
+ loff_t pos;
+ struct file *f_file;
+ struct seq_operations *fdinfo_ops;
+ unsigned int f_flags;
+};
+
#ifdef CONFIG_PROC_FS

extern void proc_root_init(void);

2012-08-14 21:27:25

by Al Viro

[permalink] [raw]
Subject: Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

On Tue, Aug 14, 2012 at 11:56:49PM +0400, Cyrill Gorcunov wrote:
> Al, does the patch below looks better? If so I'll fix up the rest.

> struct file_operations {
> struct module *owner;
> + struct seq_operations *fdinfo_ops;

IDGI. Why on the earth do you need the whole iterator? All it takes
is show_fdinfo(struct seq_file *m, struct file *f); have ->show() in
your iterator (one going through the files) call that sucker for the
file we are trying to show.

I think you are severely overdesigning that thing...

2012-08-14 21:56:22

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

On Tue, Aug 14, 2012 at 10:27:21PM +0100, Al Viro wrote:
> On Tue, Aug 14, 2012 at 11:56:49PM +0400, Cyrill Gorcunov wrote:
> > Al, does the patch below looks better? If so I'll fix up the rest.
>
> > struct file_operations {
> > struct module *owner;
> > + struct seq_operations *fdinfo_ops;
>
> IDGI. Why on the earth do you need the whole iterator? All it takes
> is show_fdinfo(struct seq_file *m, struct file *f); have ->show() in
> your iterator (one going through the files) call that sucker for the
> file we are trying to show.
>
> I think you are severely overdesigning that thing...

Hmm, in very first versions I've been using one ->show method, but
then I thought that this is not very correlate with seq-files idea
where for each record show/next sequence is called. I'll update (this
for sure will make code simplier, and I'll have to check for seq-file
overflow after seq_printf call to not continue printing data for too
long if buffer already out of space).

Cyrill

2012-08-14 22:21:53

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

On Wed, Aug 15, 2012 at 01:56:16AM +0400, Cyrill Gorcunov wrote:
> > > struct file_operations {
> > > struct module *owner;
> > > + struct seq_operations *fdinfo_ops;
> >
> > IDGI. Why on the earth do you need the whole iterator? All it takes
> > is show_fdinfo(struct seq_file *m, struct file *f); have ->show() in
> > your iterator (one going through the files) call that sucker for the
> > file we are trying to show.
> >
> > I think you are severely overdesigning that thing...
>
> Hmm, in very first versions I've been using one ->show method, but
> then I thought that this is not very correlate with seq-files idea
> where for each record show/next sequence is called. I'll update (this
> for sure will make code simplier, and I'll have to check for seq-file
> overflow after seq_printf call to not continue printing data for too
> long if buffer already out of space).

Al, I'll cook the whole series tomorrow and resend it for review,
also I guess the new show_fdinfo() member in file-operations should
be guarded with CONFIG_PROC_FS, right?

2012-08-15 00:07:08

by Al Viro

[permalink] [raw]
Subject: Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

On Wed, Aug 15, 2012 at 02:21:47AM +0400, Cyrill Gorcunov wrote:
> > Hmm, in very first versions I've been using one ->show method, but
> > then I thought that this is not very correlate with seq-files idea
> > where for each record show/next sequence is called. I'll update (this
> > for sure will make code simplier, and I'll have to check for seq-file
> > overflow after seq_printf call to not continue printing data for too
> > long if buffer already out of space).
>
> Al, I'll cook the whole series tomorrow and resend it for review,
> also I guess the new show_fdinfo() member in file-operations should
> be guarded with CONFIG_PROC_FS, right?

I seriously doubt that it's worth bothering. If somebody cares, they
can add making it conditional later.

2012-08-15 07:40:48

by Cyrill Gorcunov

[permalink] [raw]
Subject: Re: [patch 3/8] procfs: Add ability to plug in auxiliary fdinfo providers

On Wed, Aug 15, 2012 at 01:07:03AM +0100, Al Viro wrote:
> On Wed, Aug 15, 2012 at 02:21:47AM +0400, Cyrill Gorcunov wrote:
> > > Hmm, in very first versions I've been using one ->show method, but
> > > then I thought that this is not very correlate with seq-files idea
> > > where for each record show/next sequence is called. I'll update (this
> > > for sure will make code simplier, and I'll have to check for seq-file
> > > overflow after seq_printf call to not continue printing data for too
> > > long if buffer already out of space).
> >
> > Al, I'll cook the whole series tomorrow and resend it for review,
> > also I guess the new show_fdinfo() member in file-operations should
> > be guarded with CONFIG_PROC_FS, right?
>
> I seriously doubt that it's worth bothering. If somebody cares, they
> can add making it conditional later.

That's what I've beed testing, does it looks good for you?
---
From: Cyrill Gorcunov <[email protected]>
Subject: procfs: Add ability to plug in auxiliary fdinfo providers

This patch brings ability to print out auxiliary data associated
with file in procfs interface /proc/pid/fdinfo/fd.

Inparticular 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 into
struct file_operations (as Al proposed).

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 {