extended fdinfo via procfs series, v7
Subsystems (6)
cover extended fdinfo via procfs series, v7
Hi guys,
here is updated version of the fdinfo via procfs series, the changes from previous one are the following
- fhandle is carried inside inotify mark but this feature is CONFIG dependent to not bloat the kernel for users who don't need it
- a small fix in exportfs code to prevent nil dereference
the comments would be appreciated.
PATCH 1/9 procfs: Move /proc/pid/fd[info] handling code to fd.[ch]
This patch prepares the ground for further extension of /proc/pid/fd[info] handling code by moving fdinfo handling code into fs/proc/fd.c.
I think such move makes both fs/proc/base.c and fs/proc/fd.c easier to read.
--- fs/proc/Makefile | 2 fs/proc/base.c | 388 ----------------------------------------------------- fs/proc/fd.c | 351 +++++++++++++++++++++++++++++++++++++++++++++++ fs/proc/fd.h | 14 + fs/proc/internal.h | 48 ++++++ 5 files changed, 416 insertions(+), 387 deletions(-)
fs/proc/Makefile +1 -1
@@ -8,7 +8,7 @@ proc-y := nommu.o task_nommu.o proc-$(CONFIG_MMU) := mmu.o task_mmu.o proc-y += inode.o root.o base.o generic.o array.o \ - proc_tty.o + proc_tty.o fd.o proc-y += cmdline.o proc-y += consoles.o proc-y += cpuinfo.o
fs/proc/base.c +2 -386
430 lines not on this page · raw message
fs/proc/fd.c new file +351 -0
351 lines not on this page · raw message
fs/proc/fd.h new file +14 -0
@@ -0,0 +1,14 @@ +#ifndef __PROCFS_FD_H__ +#define __PROCFS_FD_H__ + +#include <linux/fs.h> + +extern const struct file_operations proc_fd_operations; +extern const struct inode_operations proc_fd_inode_operations; + +extern const struct file_operations proc_fdinfo_operations; +extern const struct inode_operations proc_fdinfo_inode_operations; + +extern int proc_fd_permission (struct inode *inode,int mask); + +#endif /* __PROCFS_FD_H__ */
fs/proc/internal.h +48 -0
66 lines not on this page · raw message
Al Viro
maintainerRe: [patch 1/9] procfs: Move /proc/pid/fd[info] handling code to fd.[ch]
Quoted 6 lines · Cyrill Gorcunov
This patch prepares the ground for further extension of /proc/pid/fd[info] handling code by moving fdinfo handling code into
fs/proc/fd.c.I think such move makes both
fs/proc/base.candfs/proc/fd.ceasier to read.
BTW, looking at the other stuff in fs/proc/base.c, why the hell is struct file * grabbed in proc_map_files_readdir()? All we do with it is passing it to proc_fill_cache(), which passes it to proc_map_files_instantiate(), which looks at two sodding bits in file->f_mode. Then we go and fput() all those struct file references we'd been putting into the array... What for?
Re: [patch 1/9] procfs: Move /proc/pid/fd[info] handling code to fd.[ch]
Quoted 8 lines · Al Viro
Quoted 6 lines · Cyrill Gorcunov
This patch prepares the ground for further extension of /proc/pid/fd[info] handling code by moving fdinfo handling code into
fs/proc/fd.c.I think such move makes both
fs/proc/base.candfs/proc/fd.ceasier to read.
Quoted 6 lines
BTW, looking at the other stuff in
fs/proc/base.c, why the hell is struct file * grabbed inproc_map_files_readdir()? All we do with it is passing it toproc_fill_cache(), which passes it toproc_map_files_instantiate(), which looks at two sodding bits infile->f_mode. Then we go andfput()all those struct file references we'd been putting into the array... What for?
Well, this could be simplified indeed, if I understand you correctly you propose just save f_mode in flexible array and use it instead of struct file, right? (which will require to rewrite code a bit)
Cyrill
Al Viro
maintainerRe: [patch 1/9] procfs: Move /proc/pid/fd[info] handling code to fd.[ch]
Quoted 16 lines · Cyrill Gorcunov
Quoted 8 lines · Al Viro
Quoted 6 lines · Cyrill Gorcunov
This patch prepares the ground for further extension of /proc/pid/fd[info] handling code by moving fdinfo handling code into
fs/proc/fd.c.I think such move makes both
fs/proc/base.candfs/proc/fd.ceasier to read.Quoted 6 lines
BTW, looking at the other stuff in
fs/proc/base.c, why the hell is struct file * grabbed inproc_map_files_readdir()? All we do with it is passing it toproc_fill_cache(), which passes it toproc_map_files_instantiate(), which looks at two sodding bits infile->f_mode. Then we go andfput()all those struct file references we'd been putting into the array... What for?
Quoted 3 lines
Well, this could be simplified indeed, if I understand you correctly you propose just save
f_modein flexible array and use it instead of struct file, right? (which will require to rewrite code a bit)
Yes. FWIW, proc_fill_cache() is really atrocious ;-/ Not to mention anything else, if we ever get a negative dentry there, we have a dentry leak. I don't think it's possible in practice, but... Furthermore,
if (!child || IS_ERR(child) || !child->d_inode)
goto end_instantiate;
inode = child->d_inode;
if (inode) {
ino = inode->i_ino;
type = inode->i_mode >> 12;
}
dput(child);looks really weird - how can we possibly get !inode when we'd just checked that child->inode is non-NULL? Moreover, that find_inode_number() a bit below is also as weird as it gets - in effect, we repeat d_lookup() we'd just done earlier. How *can* it get us anything?
Re: [patch 1/9] procfs: Move /proc/pid/fd[info] handling code to fd.[ch]
Quoted 5 lines · Al Viro
Quoted 3 lines
Well, this could be simplified indeed, if I understand you correctly you propose just save
f_modein flexible array and use it instead of struct file, right? (which will require to rewrite code a bit)Yes. FWIW,
proc_fill_cache()is really atrocious ;-/ Not to mention
OK, thanks. I'm putting this cleanup task in my big todo list. Hope I'll manage on the next week with it.
Quoted 2 lines
anything else, if we ever get a negative dentry there, we have a dentry leak. I don't think it's possible in practice, but... Furthermore,
could you please elaborate, you mean this string?
struct dentry *child, *dir = filp->f_path.dentry;
Quoted 2 lines
if (!child || IS_ERR(child) || !child->d_inode)
goto end_instantiate;this could be IS_ERR_OR_NULL i guess
Quoted 10 lines
inode = child->d_inode; if (inode) { ino = inode->i_ino; type = inode->i_mode >> 12; } dput(child);looks really weird - how can we possibly get !inode when we'd just checked that
child->inodeis non-NULL? Moreover, thatfind_inode_number()a bit below is also as weird as it gets - in effect, we repeatd_lookup()we'd just done earlier. How *can* it get us anything?
to be fair -- I don't know ;) I mean I didn't invent this function but it definitely could be cleaned up. That was partly a reason why I've moved fd related code to fd.c|h (base.c is really big in content already and it's always a problem at least for me to follow big "c" files).
I can try to clean this code up, but not in this patch series, just to not mess the series even more.
Cyrill
Al Viro
maintainerRe: [patch 1/9] procfs: Move /proc/pid/fd[info] handling code to fd.[ch]
Quoted 7 lines · Cyrill Gorcunov
Quoted 5 lines · Al Viro
Quoted 3 lines
Well, this could be simplified indeed, if I understand you correctly you propose just save
f_modein flexible array and use it instead of struct file, right? (which will require to rewrite code a bit)Yes. FWIW,
proc_fill_cache()is really atrocious ;-/ Not to mention
Quoted 7 lines
OK, thanks. I'm putting this cleanup task in my big todo list. Hope I'll manage on the next week with it.
Quoted 2 lines
anything else, if we ever get a negative dentry there, we have a dentry leak. I don't think it's possible in practice, but... Furthermore,
could you please elaborate, you mean this string?
I mean that if we get to that if (... || !child->d_inode) and end up evaluating the last part at all, we have acquired a reference to that struct dentry. And if that last part ends up being true (i.e. if it's a negative dentry), we'll return from function without having dropped the reference we'd acquired.
Re: [patch 1/9] procfs: Move /proc/pid/fd[info] handling code to fd.[ch]
Quoted 14 lines · Al Viro
Quoted 13 lines · Cyrill Gorcunov
Quoted 5 lines · Al Viro
Quoted 3 lines
Well, this could be simplified indeed, if I understand you correctly you propose just save
f_modein flexible array and use it instead of struct file, right? (which will require to rewrite code a bit)Yes. FWIW,
proc_fill_cache()is really atrocious ;-/ Not to mentionOK, thanks. I'm putting this cleanup task in my big todo list. Hope I'll manage on the next week with it.
Quoted 2 lines
anything else, if we ever get a negative dentry there, we have a dentry leak. I don't think it's possible in practice, but... Furthermore,
Quoted 7 lines
Quoted 1 line
could you please elaborate, you mean this string?
I mean that if we get to that if (... || !child->d_inode) and end up evaluating the last part at all, we have acquired a reference to that struct dentry. And if that last part ends up being true (i.e. if it's a negative dentry), we'll return from function without having dropped the reference we'd acquired.
Would the patch below improve the code? Look, I've not dropped find_inode_number call since it's a bit unclear for me what would happen if !child case hit
child = d_lookup(dir, &qname);
if (!child) {
struct dentry *new = d_alloc(dir, &qname);
if (new) {
child = instantiate(dir->d_inode, new, task, ptr);
if (child)
dput(new);
else
child = new;
}
}can we be sure that i_ino won't be zero here?
--- fs/proc/base.c | 27 +++++++++++++-------------- 1 file changed, 13 insertions(+), 14 deletions(-)
fs/proc/base.c +13 -14
@@ -1650,7 +1650,6 @@ int proc_fill_cache(struct file *filp, v instantiate_t instantiate, struct task_struct *task, const void *ptr) { struct dentry *child, *dir = filp->f_path.dentry; -struct inode *inode;struct qstr qname;ino_t ino =0 ;unsigned type =DT_UNKNOWN ;
@@ -1661,8 +1660,7 @@ int proc_fill_cache(struct file *filp, v child =d_lookup (dir, &qname);if (!child) { -struct dentry *new; - new =d_alloc (dir, &qname); +struct dentry *new =d_alloc (dir, &qname);if (new) { child =instantiate (dir->d_inode, new, task, ptr);if (child)
@@ -1671,19 +1669,20 @@ int proc_fill_cache(struct file *filp, v child = new; } } -if (!child ||IS_ERR (child) || !child->d_inode) - goto end_instantiate; - inode = child->d_inode; -if (inode) { - ino = inode->i_ino; - type = inode->i_mode >>12 ; - } +if (IS_ERR_OR_NULL (child)) + goto err; +if (!child->d_inode) + goto err_put; + ino = child->d_inode->i_ino; + type = child->d_inode->i_mode >>12 ; +err_put:dput (child); -end_instantiate: -if (!ino) +err: +if (!ino) { ino =find_inode_number (dir, &qname); -if (!ino) - ino =1 ; +if (!ino) + ino =1 ; + }return filldir (dirent, name, len, filp->f_pos, ino, type); }
Al Viro
maintainerRe: [patch 1/9] procfs: Move /proc/pid/fd[info] handling code to fd.[ch]
Quoted 16 lines · Cyrill Gorcunov
Would the patch below improve the code? Look, I've not dropped
find_inode_numbercall since it's a bit unclear for me what would happen if !child case hitchild = d_lookup(dir, &qname); if (!child) { struct dentry *new = d_alloc(dir, &qname); if (new) { child = instantiate(dir->d_inode, new, task, ptr); if (child) dput(new); else child = new; } }
Quoted 1 line
can we be sure that
i_inowon't be zero here?
First of all, ->i_ino is not going to be zero for any procfs inode. As for !child case, that's possible only if d_lookup() returns NULL *and* d_alloc() fails. In that case find_inode_number() will call d_hash_and_lookup(), which will call d_lookup(), get NULL from it and return NULL to find_inode_number(). Which will return 0 to the caller.
AFAICS, if find_inode_number() is called there at all, it will return 0. IOW, this if (!ino) ino = find_inode_number(...); is a no-op.
Al Viro
maintainerRe: [patch 1/9] procfs: Move /proc/pid/fd[info] handling code to fd.[ch]
In reply to Al ViroQuoted 8 lines · Al Viro
Quoted 6 lines · Cyrill Gorcunov
This patch prepares the ground for further extension of /proc/pid/fd[info] handling code by moving fdinfo handling code into
fs/proc/fd.c.I think such move makes both
fs/proc/base.candfs/proc/fd.ceasier to read.
Quoted 6 lines
BTW, looking at the other stuff in
fs/proc/base.c, why the hell is struct file * grabbed inproc_map_files_readdir()? All we do with it is passing it toproc_fill_cache(), which passes it toproc_map_files_instantiate(), which looks at two sodding bits infile->f_mode. Then we go andfput()all those struct file references we'd been putting into the array... What for?
Another thing:
f_flags = fd_file->f_flags & ~O_CLOEXEC;
in there is really wrong; we shouldn't leak that bit into ->f_flags at all (which is properly fixed in fs/open.c). Close-on-exec is a property of descriptor, not of file...
tid_fd_revalidate(): these dances with get_files_struct()/put_files_struct() are pointless. We really only need "is it opened" + f_mode if it is. Extracting that information is cheap enough to have it done right under task_lock() (see what get_files_struct() is doing); no need to mess with files_struct refcount, etc.
proc_fd_link() and seq_fd_open(): ditto.
As the matter of fact, the only place in there where get_files_struct() is warranted is proc_readfd_common().
intantiate callback: sodding atrocity; as the absolute minimum it should lose its "dir" argument. dentry->d_parent->d_inode is stable (we are holding ->i_mutex on parent in all callers) and it's equal to dir in all cases.
PATCH 2/9 procfs: Convert /proc/pid/fdinfo/ handling routines to seq-file v2
In reply to Cyrill GorcunovThis 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
--- fs/proc/fd.c | 145 ++++++++++++++++++++++++++++++++++++++++------------------- 1 file changed, 99 insertions(+), 46 deletions(-)
fs/proc/fd.c +99 -46
@@ -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 tistatic 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);
Al Viro
maintainerRe: [patch 2/9] procfs: Convert /proc/pid/fdinfo/ handling routines to seq-file v2
Quoted 7 lines · Cyrill Gorcunov
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_linkno longer call forproc_fd_info, simply becauseproc_fd_infois converted toseq_fdinfo_open(which is seq-fileopen()prototype).
Actually, now that I've looked at it a bit more... You've just introduced an ABI change here. Look:
echo foo > /tmp/a
exec 8</tmp/a # fd 8 reads from /tmp/a
read i <&8 # read line from it
exec 9</proc/self/fdinfo/8 # fd 9 is /proc/self/fdinfo/8
exec 8</tmp/a # close fd 8 and reopen it to the same /tmp/a
cat <&9 # now read from fd 9
With the mainline it will print
pos: 0 flags: 0100000
With that commit you will get
pos: 4 flags: 0100000
since the file you've opened refers to what used to be at fd 8 at the moment of open(2), not read(2). It may or may not be harmless, but it definitely is a userland ABI change. And that way it's actually an extra PITA for yourself - think what /proc/self/fdinfo/9 should contain now! That's right, you've got hidden state there and would need to print it to be able to reconstruct the state on restart. Only it doesn't end just there - what if you've taken that one step further and got the struct file stashed in there at open(2) time also of the same kind?
IMO doing that at open() time is just a headache for no good reason - resolving descriptor to struct file * at read() time as we do now is much saner. Better do that in your ->show(), since you are using a single-shot iterator anyway...
Re: [patch 2/9] procfs: Convert /proc/pid/fdinfo/ handling routines to seq-file v2
Quoted 11 lines · Al Viro
Quoted 7 lines · Cyrill Gorcunov
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_linkno longer call forproc_fd_info, simply becauseproc_fd_infois converted toseq_fdinfo_open(which is seq-fileopen()prototype).Actually, now that I've looked at it a bit more... You've just introduced an ABI change here. Look:
Crap, ineed. Thanks, Al! I'll fix it up, sorry.
...
Quoted 4 lines
IMO doing that at
open()time is just a headache for no good reason - resolving descriptor to struct file * atread()time as we do now is much saner. Better do that in your ->show(), since you are using a single-shot iterator anyway...
Oh, thanks for the hint, Al! I'll rework and send an updated version.
Cyrill
Re: [patch 2/9] procfs: Convert /proc/pid/fdinfo/ handling routines to seq-file v2
In reply to Al ViroQuoted 5 lines · Al Viro
IMO doing that at
open()time is just a headache for no good reason - resolving descriptor to struct file * atread()time as we do now is much saner. Better do that in your ->show(), since you are using a single-shot iterator anyway...
Al, the updated version is below. I suppose I can grab proc inode then and lookup for file position and flags at show method. (I remember what you've said about O_CLOEXEC bit, but I'll address this in another patch).
---
From: Cyrill Gorcunov <[email protected]>
Subject: procfs: Convert /proc/pid/fdinfo/ handling routines to seq-file v3
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
v3 (by Al Viro):
- Don't grab struct file at read(2) time since it breaks ABI, instead all manipulations should be done at ->show() call.
--- fs/proc/fd.c | 142 +++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 95 insertions(+), 47 deletions(-)
fs/proc/fd.c +95 -47
@@ -6,61 +6,100 @@#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 inode *inode; +}; -static int proc_fd_info (struct inode *inode,struct path *path,char *info) +static int seq_show (struct seq_file *m,void *v) { -struct task_struct *task =get_proc_task (inode); + struct proc_fdinfo *fdinfo = container_of(m, struct proc_fdinfo, m);struct files_struct *files =NULL ; -int fd =proc_fd (inode); -struct file *file; +int f_flags =0 , ret = -ENOENT ; +struct file *file =NULL ; +struct task_struct *task; + + task =get_proc_task (fdinfo->inode); +if (!task) +return -ENOENT ; + + files =get_files_struct (task); +put_task_struct (task); -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 (fdinfo->inode); +spin_lock (&files->file_lock); file =fcheck_files (files, fd);if (file) { -unsigned int f_flags; -struct fdtable *fdt; +struct fdtable *fdt =files_fdtable (files); - fdt =files_fdtable (files); f_flags = 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 (file); + ret =0 ; }spin_unlock (&files->file_lock);put_files_struct (files); } -return -ENOENT ; + +if (!ret) { +seq_printf (m,"pos:\t%lli\nflags:\t0%o\n" , + (long long )file->f_pos, f_flags); +fput (file); + } + +return ret; +} + +static int seq_fdinfo_open (struct inode *inode,struct file *file) +{ +struct proc_fdinfo *fdinfo; +int ret =0 ; + + fdinfo =kmalloc (sizeof (*fdinfo),GFP_KERNEL ); +if (!fdinfo) +return -ENOMEM ; + +ihold (inode); + fdinfo->inode = inode; + + file->private_data = &fdinfo->m; + ret =single_open (file, seq_show,NULL ); +if (ret) { +iput (inode); +kfree (fdinfo); + } + +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); +iput (fdinfo->inode); +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 +169,32 @@ static const struct dentry_operations tistatic 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 +309,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);
Al Viro
maintainerRe: [patch 2/9] procfs: Convert /proc/pid/fdinfo/ handling routines to seq-file v2
Quoted 11 lines · Cyrill Gorcunov
Quoted 5 lines · Al Viro
IMO doing that at
open()time is just a headache for no good reason - resolving descriptor to struct file * atread()time as we do now is much saner. Better do that in your ->show(), since you are using a single-shot iterator anyway...Al, the updated version is below. I suppose I can grab proc inode then and lookup for file position and flags at show method. (I remember what you've said about
O_CLOEXECbit, but I'll address this in another patch).
Applied, with a couple of changes:
* there's no need for those games with ihold/iput - opened file pins
its inode down just fine, TYVM.
* struct fd_info is pointless in that form - the last argument
of single_open() will end up in seq_file ->private, so let's just pass the inode there and use m->private in ->show().
I'll push that into vfs.git#master (along with the previous patch) in a few hours.
O_CLOEXEC is taken care of in my tree. FWIW, I'm consolidating descriptor handling in general into fs/file.c (and I'm seriously tempted to rename that sucker to something like fs/descriptors.c or fs/fdtable.c); some of that stuff is already in #master. I'm probably going to move some of the code from your fs/proc/fd.c there as well...
Re: [patch 2/9] procfs: Convert /proc/pid/fdinfo/ handling routines to seq-file v2
Quoted 4 lines · Al Viro
Applied, with a couple of changes:
* there's no need for those games with ihold/iput - opened file pins its inode down just fine, TYVM.
Ah, yeah, thanks!
Quoted 3 lines
* struct
fd_infois pointless in that form - the last argument ofsingle_open()will end up inseq_file->private, so let's just pass the inode there and usem->privatein ->show().
Yes. Thanks!
Quoted 7 lines
I'll push that into vfs.git#master (along with the previous patch) in a few hours.
O_CLOEXECis taken care of in my tree. FWIW, I'm consolidating descriptor handling in general intofs/file.c(and I'm seriously tempted to rename that sucker to something likefs/descriptors.corfs/fdtable.c); some of that stuff is already in #master. I'm probably going to move some of the code from yourfs/proc/fd.cthere as well...
OK, so I'll fetch your tree and rebase the rest of series on top then, to continue fdinfo handling. Sounds good?
PATCH 3/9 procfs: Add ability to plug in auxiliary fdinfo providers
In reply to Cyrill GorcunovThis 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 and Pavel are proposing).
--- fs/proc/fd.c | 5 +++++ include/linux/fs.h | 3 +++ 2 files changed, 8 insertions(+)
fs/proc/fd.c +5 -0
@@ -23,8 +23,13 @@ struct proc_fdinfo {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); + +if (fdinfo->fd_file->f_op->show_fdinfo) +return fdinfo->fd_file->f_op->show_fdinfo (m, fdinfo->fd_file); +return 0 ; }
include/linux/fs.h +3 -0
@@ -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 {
PATCH 4/9 fs, exportfs: Fix nil dereference if no s_export_op present
In reply to Cyrill GorcunovIf there is no s_export_op present in a target superblock we might have nil dereference. Fix it with eplicit test if s_export_op is provided.
--- fs/exportfs/expfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/exportfs/expfs.c +1 -1
@@ -357,7 +357,7 @@ int exportfs_encode_fh(struct dentry *de*/ parent = p->d_inode; } -if (nop->encode_fh) +if (nop && nop->encode_fh) error = nop->encode_fh (inode, fid->raw, max_len, parent);else error =export_encode_fh (inode, fid, max_len, parent);
Re: [patch 4/9] fs, exportfs: Fix nil dereference if no s_export_op present
Quoted 2 lines · Cyrill Gorcunov
If there is no
s_export_oppresent in a target superblock we might have nil dereference.
Is that NULL dereference possible with current code, or is it a check you're adding to account for a new caller that you're about to add?
I believe it's the latter, but this would be a good thing to make clear in the changelog.
--b.
Quoted 32 lines
Fix it with eplicit test
ifs_export_opis provided.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]>CC "Aneesh Kumar K.V" <[email protected]>CC Alexey Dobriyan <[email protected]>CC Matthew Helsley <[email protected]>CC "J. Bruce Fields" <[email protected]>CC "Aneesh Kumar K.V" <[email protected]>--- fs/exportfs/expfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Index:
linux-2.6.git/fs/exportfs/expfs.c
===================================================================fs/exportfs/expfs.c +1
-1@@ -357,7 +357,7 @@ int exportfs_encode_fh(struct dentry *de*/ parent = p->d_inode; } -if (nop->encode_fh) +if (nop && nop->encode_fh) error = nop->encode_fh (inode, fid->raw, max_len, parent);else error =export_encode_fh (inode, fid, max_len, parent);
Re: [patch 4/9] fs, exportfs: Fix nil dereference if no s_export_op present
Quoted 9 lines · J. Bruce Fields
Quoted 2 lines · Cyrill Gorcunov
If there is no
s_export_oppresent in a target superblock we might have nil dereference.Is that NULL dereference possible with current code, or is it a check you're adding to account for a new caller that you're about to add?
I believe it's the latter, but this would be a good thing to make clear in the changelog.
With the current code it seems to be impossible (well, i can't be sure about nfs caller) because do_sys_name_to_handle does check for s_export_op to exist. Updated changelog below. After all I think not checking s_export_op was a mistake in general -- this routine is exported to other modules but has no a single line of comment about possibility of nil dereference.
---
From: Cyrill Gorcunov <[email protected]>
Subject: fs, exportfs: Escape nil dereference if no s_export_op present
This routine will be used to generate a file handle in fdinfo output for inotify subsystem, where if no s_export_op present the general export_encode_fh should be used. Thus add a test if s_export_op present inside exportfs_encode_fh itself.
--- fs/exportfs/expfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/exportfs/expfs.c +1 -1
@@ -357,7 +357,7 @@ int exportfs_encode_fh(struct dentry *de*/ parent = p->d_inode; } -if (nop->encode_fh) +if (nop && nop->encode_fh) error = nop->encode_fh (inode, fid->raw, max_len, parent);else error =export_encode_fh (inode, fid, max_len, parent);
Re: [patch 4/9] fs, exportfs: Fix nil dereference if no s_export_op present
Quoted 11 lines · Cyrill Gorcunov
Quoted 9 lines · J. Bruce Fields
Quoted 2 lines · Cyrill Gorcunov
If there is no
s_export_oppresent in a target superblock we might have nil dereference.Is that NULL dereference possible with current code, or is it a check you're adding to account for a new caller that you're about to add?
I believe it's the latter, but this would be a good thing to make clear in the changelog.
Quoted 6 lines
With the current code it seems to be impossible (well, i can't be sure about nfs caller) because
do_sys_name_to_handledoes check fors_export_opto exist. Updated changelog below. After all I think not checkings_export_opwas a mistake in general -- this routine is exported to other modules but has no a single line of comment about possibility of nil dereference.
Fine, just make sure that's explained in the changelog.
For distributors looking for patches to backport, "fix nil dereference" may set off alarm bells unnecessarily.
--b.
Quoted 41 lines
---
From: Cyrill Gorcunov <[email protected]>
Subject: fs, exportfs: Escape nil dereference if nos_export_oppresentThis routine will be used to generate a file handle in fdinfo output for inotify subsystem, where if no
s_export_oppresent the generalexport_encode_fhshould be used. Thus add a test ifs_export_oppresent insideexportfs_encode_fhitself.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]>CC "Aneesh Kumar K.V" <[email protected]>CC Alexey Dobriyan <[email protected]>CC Matthew Helsley <[email protected]>CC "J. Bruce Fields" <[email protected]>CC "Aneesh Kumar K.V" <[email protected]>--- fs/exportfs/expfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)Index:
linux-2.6.git/fs/exportfs/expfs.c
===================================================================fs/exportfs/expfs.c +1
-2@@ -357,7 +357,7 @@ int exportfs_encode_fh(struct dentry *de*/ parent = p->d_inode; } -if (nop->encode_fh) +if (nop && nop->encode_fh) error = nop->encode_fh (inode, fid->raw, max_len, parent);else error =export_encode_fh (inode, fid, max_len, parent); --To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH 5/9 fs, notify: Add file handle entry into inotify_inode_mark
In reply to Cyrill GorcunovThis file handle will be used in /proc/pid/fdinfo/fd output, which in turn will allow to restore a watch target after checkpoint.
--- fs/notify/inotify/inotify.h | 8 +++++++ fs/notify/inotify/inotify_user.c | 42 ++++++++++++++++++++++++++++++++++----- 2 files changed, 45 insertions(+), 5 deletions(-)
fs/notify/inotify/inotify.h +8 -0
@@ -1,6 +1,7 @@#include <linux/fsnotify_backend.h> #include <linux/inotify.h> #include <linux/slab.h> /* struct kmem_cache */ +#include <linux/exportfs.h> extern struct kmem_cache *event_priv_cachep;
@@ -9,9 +10,16 @@ struct inotify_event_private_data {int wd; }; +#if defined(CONFIG_PROC_FS ) && defined(CONFIG_EXPORTFS ) && defined(CONFIG_CHECKPOINT_RESTORE ) +# define INOTIFY_USE_FHANDLE +#endif +struct inotify_inode_mark {struct fsnotify_mark fsn_mark;int wd; +#ifdef INOTIFY_USE_FHANDLE +__u8 fhandle[sizeof (struct file_handle ) +MAX_HANDLE_SZ ]; +#endif }; extern void inotify_ignored_and_remove_idr(struct fsnotify_mark *fsn_mark,
fs/notify/inotify/inotify_user.c +37 -5
82 lines not on this page · raw message
PATCH 6/9 fs, notify: Add procfs fdinfo helper v4
In reply to Cyrill GorcunovThis allow us to print out fsnotify details such as watchee inode, device, mask and optionally a file handle.
For inotify objects if kernel compiled with exportfs support the output will be
| pos: 0 | flags: 02000000 | inotify wd: 3 ino: 9e7e sdev: 800013 mask: 800afce ignored_mask: 0 fhandle-bytes: 8 fhandle-type: 1 f_handle: 7e9e0000640d1b6d | inotify wd: 2 ino: a111 sdev: 800013 mask: 800afce ignored_mask: 0 fhandle-bytes: 8 fhandle-type: 1 f_handle: 11a1000020542153 | inotify wd: 1 ino: 6b149 sdev: 800013 mask: 800afce ignored_mask: 0 fhandle-bytes: 8 fhandle-type: 1 f_handle: 49b1060023552153
If kernel compiled without exportfs support, the file handle won't be provided but inode and device only.
| pos: 0 | flags: 02000000 | inotify wd: 3 ino: 9e7e sdev: 800013 mask: 800afce ignored_mask: 0 | inotify wd: 2 ino: a111 sdev: 800013 mask: 800afce ignored_mask: 0 | inotify wd: 1 ino: 6b149 sdev: 800013 mask: 800afce ignored_mask: 0
For fanotify the output is like
| pos: 0 | flags: 02 | fanotify ino: 68f71 sdev: 800013 mask: 1 ignored_mask: 40000000 | fanotify mnt_id: 13 mask: 1 ignored_mask: 40000000
To minimize impact on general fsnotify code the new functionality is gathered in fs/notify/fdinfo.c file.
--- fs/notify/Makefile | 2 fs/notify/fanotify/fanotify_user.c | 4 + fs/notify/fdinfo.c | 124 +++++++++++++++++++++++++++++++++++++ fs/notify/fdinfo.h | 22 ++++++ fs/notify/inotify/inotify_user.c | 4 + 5 files changed, 155 insertions(+), 1 deletion(-)
fs/notify/Makefile +1 -1
@@ -1,5 +1,5 @@ obj-$(CONFIG_FSNOTIFY) += fsnotify.o notification.o group.o inode_mark.o \ - mark.o vfsmount_mark.o + mark.o vfsmount_mark.o fdinfo.o obj-y += dnotify/ obj-y += inotify/
fs/notify/fanotify/fanotify_user.c +4 -0
@@ -17,6 +17,7 @@#include <asm/ioctls.h> #include "../../mount.h" +#include "../fdinfo.h" #define FANOTIFY_DEFAULT_MAX_EVENTS 16384#define FANOTIFY_DEFAULT_MAX_MARKS 8192
@@ -446,6 +447,9 @@ static long fanotify_ioctl(struct file * }static const struct file_operations fanotify_fops = { +#ifdef CONFIG_PROC_FS + .show_fdinfo = fanotify_show_fdinfo, +#endif .poll = fanotify_poll, .read = fanotify_read, .write = fanotify_write,
fs/notify/fdinfo.c new file +124 -0
124 lines not on this page · raw message
fs/notify/fdinfo.h new file +22 -0
@@ -0,0 +1,22 @@ +#ifndef __FSNOTIFY_FDINFO_H__ +#define __FSNOTIFY_FDINFO_H__ + +#include <linux/errno.h> +#include <linux/proc_fs.h> + +struct seq_file ; +struct file ; + +#ifdef CONFIG_PROC_FS + +#ifdef CONFIG_INOTIFY_USER +extern int inotify_show_fdinfo (struct seq_file *m,struct file *f); +#endif + +#ifdef CONFIG_FANOTIFY +extern int fanotify_show_fdinfo (struct seq_file *m,struct file *f); +#endif + +#endif /* CONFIG_PROC_FS */ + +#endif /* __FSNOTIFY_FDINFO_H__ */
fs/notify/inotify/inotify_user.c +4 -0
@@ -40,6 +40,7 @@#include <linux/wait.h> #include "inotify.h" +#include "../fdinfo.h" #include <asm/ioctls.h>
@@ -335,6 +336,9 @@ static long inotify_ioctl(struct file *f }static const struct file_operations inotify_fops = { +#ifdef CONFIG_PROC_FS + .show_fdinfo = inotify_show_fdinfo, +#endif .poll = inotify_poll, .read = inotify_read, .fasync = inotify_fasync,
PATCH 7/9 fs, eventfd: Add procfs fdinfo helper
In reply to Cyrill GorcunovThis allow us to print out raw counter value. The /proc/pid/fdinfo/fd output is
| pos: 0 | flags: 04002 | eventfd-count: 5a
--- fs/eventfd.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
fs/eventfd.c +20 -0
@@ -19,6 +19,8 @@#include <linux/export.h> #include <linux/kref.h> #include <linux/eventfd.h> +#include <linux/proc_fs.h> +#include <linux/seq_file.h> struct eventfd_ctx {struct kref kref;
@@ -284,7 +286,25 @@ static ssize_t eventfd_write(struct filereturn res; } +#ifdef CONFIG_PROC_FS +static int eventfd_show_fdinfo (struct seq_file *m,struct file *f) +{ +struct eventfd_ctx *ctx = f->private_data; +int ret; + +spin_lock_irq (&ctx->wqh.lock); + ret =seq_printf (m,"eventfd-count: %16llx\n" , + (unsigned long long )ctx->count); +spin_unlock_irq (&ctx->wqh.lock); + +return ret; +} +#endif +static const struct file_operations eventfd_fops = { +#ifdef CONFIG_PROC_FS + .show_fdinfo = eventfd_show_fdinfo, +#endif .release = eventfd_release, .poll = eventfd_poll, .read = eventfd_read,
PATCH 8/9 fs, epoll: Add procfs fdinfo helper v2
In reply to Cyrill GorcunovThis allow us to print out eventpoll target file descriptor, events and data, the /proc/pid/fdinfo/fd consists of
| pos: 0 | flags: 02 | tfd: 5 events: 1d data: ffffffffffffffff
This feature is CONFIG_CHECKPOINT_RESTORE only.
--- fs/eventpoll.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)
fs/eventpoll.c +28 -0
@@ -38,6 +38,8 @@#include <asm/io.h> #include <asm/mman.h> #include <linux/atomic.h> +#include <linux/proc_fs.h> +#include <linux/seq_file.h> /* * LOCKING:
@@ -783,8 +785,34 @@ static unsigned int ep_eventpoll_poll(streturn pollflags !=-1 ? pollflags :0 ; } +#ifdef CONFIG_PROC_FS +static int ep_show_fdinfo (struct seq_file *m,struct file *f) +{ +struct eventpoll *ep = f->private_data; +struct rb_node *rbp; +int ret; + +mutex_lock (&ep->mtx); +for (rbp =rb_first (&ep->rbr); rbp; rbp =rb_next (rbp)) { + struct epitem *epi = rb_entry(rbp, struct epitem, rbn); + + ret =seq_printf (m,"tfd: %8d events: %8x data: %16llx\n" , + epi->ffd.fd, epi->event.events, + (long long )epi->event.data); +if (ret) +break ; + } +mutex_unlock (&ep->mtx); + +return ret; +} +#endif +/* File callbacks that implement the eventpoll file behaviour */ static const struct file_operations eventpoll_fops = { +#ifdef CONFIG_PROC_FS + .show_fdinfo = ep_show_fdinfo, +#endif .release = ep_eventpoll_release, .poll = ep_eventpoll_poll, .llseek = noop_llseek,
PATCH 9/9 fdinfo: Show sigmask for signalfd fd v2
In reply to Cyrill Gorcunov--- fs/proc/array.c | 2 +- fs/signalfd.c | 26 ++++++++++++++++++++++++++ include/linux/proc_fs.h | 3 +++ 3 files changed, 30 insertions(+), 1 deletion(-)
fs/proc/array.c +1 -1
@@ -220,7 +220,7 @@ static inline void task_state(struct seqseq_putc (m,'\n' ); } -static void render_sigset_t (struct seq_file *m,const char *header, +void render_sigset_t (struct seq_file *m,const char *header,sigset_t *set) {int i;
fs/signalfd.c +26 -0
@@ -29,6 +29,7 @@#include <linux/anon_inodes.h> #include <linux/signalfd.h> #include <linux/syscalls.h> +#include <linux/proc_fs.h> void signalfd_cleanup (struct sighand_struct *sighand) {
@@ -46,6 +47,7 @@ void signalfd_cleanup(struct sighand_str }struct signalfd_ctx { +seqcount_t cnt;sigset_t sigmask; };
@@ -227,7 +229,28 @@ static ssize_t signalfd_read(struct filereturn total ? total: ret; } +#ifdef CONFIG_PROC_FS +static int signalfd_show_fdinfo (struct seq_file *m,struct file *f) +{ +struct signalfd_ctx *ctx = f->private_data; +sigset_t sigmask; +unsigned seq; + +do { + seq =read_seqcount_begin (&ctx->cnt); + sigmask = ctx->sigmask; + }while (read_seqcount_retry (&ctx->cnt, seq)); + +signotset (&sigmask); +render_sigset_t (m,"sigmask:\t" , &sigmask); +return 0 ; +} +#endif +static const struct file_operations signalfd_fops = { +#ifdef CONFIG_PROC_FS + .show_fdinfo = signalfd_show_fdinfo, +#endif .release = signalfd_release, .poll = signalfd_poll, .read = signalfd_read,
@@ -259,6 +282,7 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigreturn -ENOMEM ; ctx->sigmask = sigmask; +seqcount_init (&ctx->cnt);/* * When we call this, the initialization must be complete, since
@@ -279,7 +303,9 @@ SYSCALL_DEFINE4(signalfd4, int, ufd, sigreturn -EINVAL ; }spin_lock_irq (¤t->sighand->siglock); +write_seqcount_begin (&ctx->cnt); ctx->sigmask = sigmask; +write_seqcount_end (&ctx->cnt);spin_unlock_irq (¤t->sighand->siglock);wake_up (¤t->sighand->signalfd_wqh);
include/linux/proc_fs.h +3 -0
@@ -290,4 +290,7 @@ static inline struct net *PDE_NET(struct return pde->parent->data; } +#include <asm/signal.h> + +void render_sigset_t (struct seq_file *m,const char *header,sigset_t *set);#endif /* _LINUX_PROC_FS_H */
Quoted 8 lines · Cyrill Gorcunov
Hi guys,
here is updated version of the fdinfo via procfs series, the changes from previous one are the following
- fhandle is carried inside inotify mark but this feature is CONFIG dependent to not bloat the kernel for users who don't need it
As Al points out, this doesn't help much: if this feature is something a distro will want to provide, then in practice all their users are eventually going to end up with it turned on.
Could you quantify the cost somehow?
I wonder if you could get away with something less than MAX_HANDLE_SIZE? 128 bytes is the maximum allowable by NFSv4. In practice I don't think any of our filesystems need more than 40 or so right now.
--b.
Quoted 4 lines
- a small fix in exportfs code to prevent nil dereference
the comments would be appreciated.
Quoted 6 lines · J. Bruce Fields
Quoted 5 lines · Cyrill Gorcunov
Hi guys,
here is updated version of the fdinfo via procfs series, the changes from previous one are the following
Quoted 8 lines
Quoted 3 lines
- fhandle is carried inside inotify mark but this feature is CONFIG dependent to not bloat the kernel for users who don't need it
As Al points out, this doesn't help much: if this feature is something a distro will want to provide, then in practice all their users are eventually going to end up with it turned on.
Yes, I remember what Al has said, the problem is that this data attached to inotify mark is not just a couple of bytes but rather about 136 bytes per mark, and encoding this fhandle will take some cycles on mark creation as well. Thus when in a sake of c/r we simply have no other way and are to pay some trade off cost for c/r functionality, i don't think the regular users (and note that CONFIG_CHECKPOINT_RESTORE is off by default) should pay same cost for nothing. That's why I made it config dependant. Again if you still think that making it config-option is a bad idea I'll rip this symbols off, it's not a problem.
Quoted 2 lines
Could you quantify the cost somehow?
About 136 bytes per inotify mark.
Quoted 3 lines
I wonder if you could get away with something less than
MAX_HANDLE_SIZE? 128 bytes is the maximum allowable by NFSv4. In practice I don't think any of our filesystems need more than 40 or so right now.
Look, Bruce, I would like to follow the limits we have #define'd in kernel, because it makes code easier to support. I can #define some limit for inotify fhandle but what should I print in fdinfo if say there is no space left in buffer?
Cyrill
Quoted 16 lines · Cyrill Gorcunov
Quoted 6 lines · J. Bruce Fields
Quoted 5 lines · Cyrill Gorcunov
Hi guys,
here is updated version of the fdinfo via procfs series, the changes from previous one are the following
Quoted 8 lines
Quoted 3 lines
- fhandle is carried inside inotify mark but this feature is CONFIG dependent to not bloat the kernel for users who don't need it
As Al points out, this doesn't help much: if this feature is something a distro will want to provide, then in practice all their users are eventually going to end up with it turned on.
Quoted 9 lines
Yes, I remember what Al has said, the problem is that this data attached to inotify mark is not just a couple of bytes but rather about 136 bytes per mark, and encoding this fhandle will take some cycles on mark creation as well. Thus when in a sake of c/r we simply have no other way and are to pay some trade off cost for c/r functionality, i don't think the regular users (and note that
CONFIG_CHECKPOINT_RESTOREis off by default) should pay same cost for nothing. That's why I made it config dependant. Again if you still think that making it config-option is a bad idea I'll rip this symbols off, it's not a problem.
I don't have any opinion on whether there should be a configuration option. Just want to make sure the cost when it's turned on is still taken seriously.
--b.
Quoted 16 lines
Quoted 3 lines
Could you quantify the cost somehow?
About 136 bytes per inotify mark.
Quoted 3 lines
I wonder if you could get away with something less than
MAX_HANDLE_SIZE? 128 bytes is the maximum allowable by NFSv4. In practice I don't think any of our filesystems need more than 40 or so right now.Look, Bruce, I would like to follow the limits we have #define'd in kernel, because it makes code easier to support. I can #define some limit for inotify fhandle but what should I print in fdinfo if say there is no space left in buffer?
Cyrill
Quoted 4 lines · J. Bruce Fields
I don't have any opinion on whether there should be a configuration option. Just want to make sure the cost when it's turned on is still taken seriously.
I see. So, does the rest of series looks fine for you (except this config moment)?
Cyrill
Quoted 8 lines · Cyrill Gorcunov
Quoted 4 lines · J. Bruce Fields
I don't have any opinion on whether there should be a configuration option. Just want to make sure the cost when it's turned on is still taken seriously.
I see. So, does the rest of series looks fine for you (except this config moment)?
You're using the common filehandle code instead of encoding your own from scratch, which was my only serious objection.
I haven't reviewed the rest of it carefully.
--b.
Quoted 7 lines · J. Bruce Fields
Quoted 6 lines · Cyrill Gorcunov
Quoted 4 lines · J. Bruce Fields
I don't have any opinion on whether there should be a configuration option. Just want to make sure the cost when it's turned on is still taken seriously.
Quoted 6 lines
Quoted 2 lines
I see. So, does the rest of series looks fine for you (except this config moment)?
You're using the common filehandle code instead of encoding your own from scratch, which was my only serious objection.
Wait, Bruce, now I'm confused, if I understand all correctly using common filehandle code from exportfs is preferred, or not? What you mean by "encoding your own from scratch"? Could you please clarify?
Quoted 1 line
I haven't reviewed the rest of it carefully.
ok
Cyrill
Quoted 11 lines · Cyrill Gorcunov
Quoted 10 lines · J. Bruce Fields
Quoted 8 lines · Cyrill Gorcunov
Quoted 4 lines · J. Bruce Fields
I don't have any opinion on whether there should be a configuration option. Just want to make sure the cost when it's turned on is still taken seriously.
I see. So, does the rest of series looks fine for you (except this config moment)?
Quoted 6 lines
Quoted 3 lines
You're using the common filehandle code instead of encoding your own from scratch, which was my only serious objection.
Wait, Bruce, now I'm confused, if I understand all correctly using common filehandle code from exportfs is preferred,
Right, that's what I meant, sorry if I was unclear.
--b.
Quoted 9 lines
or
not? What you mean by "encoding your own from scratch"? Could you please clarify?Quoted 1 line
I haven't reviewed the rest of it carefully.
ok
Cyrill
Quoted 8 lines · J. Bruce Fields
Quoted 6 lines
Quoted 3 lines
You're using the common filehandle code instead of encoding your own from scratch, which was my only serious objection.
Wait, Bruce, now I'm confused, if I understand all correctly using common filehandle code from exportfs is preferred,
Right, that's what I meant, sorry if I was unclear.
I might be simply translating it wrong ;) Thanks Bruce!
Cyrill
Quoted 16 lines · Cyrill Gorcunov
Quoted 6 lines · J. Bruce Fields
Quoted 5 lines · Cyrill Gorcunov
Hi guys,
here is updated version of the fdinfo via procfs series, the changes from previous one are the following
Quoted 8 lines
Quoted 3 lines
- fhandle is carried inside inotify mark but this feature is CONFIG dependent to not bloat the kernel for users who don't need it
As Al points out, this doesn't help much: if this feature is something a distro will want to provide, then in practice all their users are eventually going to end up with it turned on.
Quoted 9 lines
Yes, I remember what Al has said, the problem is that this data attached to inotify mark is not just a couple of bytes but rather about 136 bytes per mark, and encoding this fhandle will take some cycles on mark creation as well. Thus when in a sake of c/r we simply have no other way and are to pay some trade off cost for c/r functionality, i don't think the regular users (and note that
CONFIG_CHECKPOINT_RESTOREis off by default) should pay same cost for nothing. That's why I made it config dependant. Again if you still think that making it config-option is a bad idea I'll rip this symbols off, it's not a problem.
Btw, Bruce, I forgot to mention that Linus and Andrew asked us to wrap all code related to c/r we bring in with CONFIG_CHECKPOINT_RESTORE symbol. So while wrapping show_fdinfo member in file_operations with CONFIG might be an overhead (as Al and Pavel pointed me) the fhandle in mark is reverse, a good candidate for CONFIG wrapping I think.
Cyrill