[patch 0/8] procfs, fdinfo updated

[patch 0/8] procfs, fdinfo updated

[Cyrill Gorcunov]

Hi guys,

here is an updated series. As being discussed with Al
the fdinfo helper provided via file_operations. Also
I've dropped CONFIG_CHECKPOINT_RESTORE wrap from inside
of particular subsystems, thus this new feature will be
available by default. I've tested the whole series but
additional review would be appreciated.

Please tell me wht you think.

	Cyrill

[patch 1/8] procfs: Move /proc/pid/fd[info] handling code to fd.[ch]

[Cyrill Gorcunov]

[-- Attachment #1: seq-fdinfo-base-proc-5 --]
[-- Type: text/plain, Size: 23066 bytes --]

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.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Acked-by: Pavel Emelyanov <xemul@parallels.com>
CC: Al Viro <viro@ZenIV.linux.org.uk>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: James Bottomley <jbottomley@parallels.com>
---
 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(-)

Index: linux-2.6.git/fs/proc/Makefile
===================================================================
--- linux-2.6.git.orig/fs/proc/Makefile
+++ linux-2.6.git/fs/proc/Makefile
@@ -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
Index: linux-2.6.git/fs/proc/base.c
===================================================================
--- linux-2.6.git.orig/fs/proc/base.c
+++ linux-2.6.git/fs/proc/base.c
@@ -90,6 +90,7 @@
 #endif
 #include <trace/events/oom.h>
 #include "internal.h"
+#include "fd.h"
 
 /* NOTE:
  *	Implementing inode permission operations in /proc is almost
@@ -136,8 +137,6 @@ struct pid_entry {
 		NULL, &proc_single_file_operations,	\
 		{ .proc_show = show } )
 
-static int proc_fd_permission(struct inode *inode, int mask);
-
 /*
  * Count the number of hardlinks for the pid_entry table, excluding the .
  * and .. links.
@@ -1492,7 +1491,7 @@ out:
 	return error;
 }
 
-static const struct inode_operations proc_pid_link_inode_operations = {
+const struct inode_operations proc_pid_link_inode_operations = {
 	.readlink	= proc_pid_readlink,
 	.follow_link	= proc_pid_follow_link,
 	.setattr	= proc_setattr,
@@ -1501,21 +1500,6 @@ static const struct inode_operations pro
 
 /* building an inode */
 
-static int task_dumpable(struct task_struct *task)
-{
-	int dumpable = 0;
-	struct mm_struct *mm;
-
-	task_lock(task);
-	mm = task->mm;
-	if (mm)
-		dumpable = get_dumpable(mm);
-	task_unlock(task);
-	if(dumpable == 1)
-		return 1;
-	return 0;
-}
-
 struct inode *proc_pid_make_inode(struct super_block * sb, struct task_struct *task)
 {
 	struct inode * inode;
@@ -1641,15 +1625,6 @@ int pid_revalidate(struct dentry *dentry
 	return 0;
 }
 
-static int pid_delete_dentry(const struct dentry * dentry)
-{
-	/* Is the task we represent dead?
-	 * If so, then don't put the dentry on the lru list,
-	 * kill it immediately.
-	 */
-	return !proc_pid(dentry->d_inode)->tasks[PIDTYPE_PID].first;
-}
-
 const struct dentry_operations pid_dentry_operations =
 {
 	.d_revalidate	= pid_revalidate,
@@ -1712,289 +1687,6 @@ end_instantiate:
 	return filldir(dirent, name, len, filp->f_pos, ino, type);
 }
 
-static unsigned name_to_int(struct dentry *dentry)
-{
-	const char *name = dentry->d_name.name;
-	int len = dentry->d_name.len;
-	unsigned n = 0;
-
-	if (len > 1 && *name == '0')
-		goto out;
-	while (len-- > 0) {
-		unsigned c = *name++ - '0';
-		if (c > 9)
-			goto out;
-		if (n >= (~0U-9)/10)
-			goto out;
-		n *= 10;
-		n += c;
-	}
-	return n;
-out:
-	return ~0U;
-}
-
-#define PROC_FDINFO_MAX 64
-
-static int proc_fd_info(struct inode *inode, struct path *path, char *info)
-{
-	struct task_struct *task = get_proc_task(inode);
-	struct files_struct *files = NULL;
-	struct file *file;
-	int fd = proc_fd(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.
-		 */
-		spin_lock(&files->file_lock);
-		file = fcheck_files(files, fd);
-		if (file) {
-			unsigned int f_flags;
-			struct fdtable *fdt;
-
-			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;
-		}
-		spin_unlock(&files->file_lock);
-		put_files_struct(files);
-	}
-	return -ENOENT;
-}
-
-static int proc_fd_link(struct dentry *dentry, struct path *path)
-{
-	return proc_fd_info(dentry->d_inode, path, NULL);
-}
-
-static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags)
-{
-	struct inode *inode;
-	struct task_struct *task;
-	int fd;
-	struct files_struct *files;
-	const struct cred *cred;
-
-	if (flags & LOOKUP_RCU)
-		return -ECHILD;
-
-	inode = dentry->d_inode;
-	task = get_proc_task(inode);
-	fd = proc_fd(inode);
-
-	if (task) {
-		files = get_files_struct(task);
-		if (files) {
-			struct file *file;
-			rcu_read_lock();
-			file = fcheck_files(files, fd);
-			if (file) {
-				unsigned f_mode = file->f_mode;
-
-				rcu_read_unlock();
-				put_files_struct(files);
-
-				if (task_dumpable(task)) {
-					rcu_read_lock();
-					cred = __task_cred(task);
-					inode->i_uid = cred->euid;
-					inode->i_gid = cred->egid;
-					rcu_read_unlock();
-				} else {
-					inode->i_uid = GLOBAL_ROOT_UID;
-					inode->i_gid = GLOBAL_ROOT_GID;
-				}
-
-				if (S_ISLNK(inode->i_mode)) {
-					unsigned i_mode = S_IFLNK;
-					if (f_mode & FMODE_READ)
-						i_mode |= S_IRUSR | S_IXUSR;
-					if (f_mode & FMODE_WRITE)
-						i_mode |= S_IWUSR | S_IXUSR;
-					inode->i_mode = i_mode;
-				}
-
-				security_task_to_inode(task, inode);
-				put_task_struct(task);
-				return 1;
-			}
-			rcu_read_unlock();
-			put_files_struct(files);
-		}
-		put_task_struct(task);
-	}
-	d_drop(dentry);
-	return 0;
-}
-
-static const struct dentry_operations tid_fd_dentry_operations =
-{
-	.d_revalidate	= tid_fd_revalidate,
-	.d_delete	= pid_delete_dentry,
-};
-
-static struct dentry *proc_fd_instantiate(struct inode *dir,
-	struct dentry *dentry, struct task_struct *task, const void *ptr)
-{
-	unsigned fd = (unsigned long)ptr;
- 	struct inode *inode;
- 	struct proc_inode *ei;
-	struct dentry *error = ERR_PTR(-ENOENT);
-
-	inode = proc_pid_make_inode(dir->i_sb, task);
-	if (!inode)
-		goto out;
-	ei = PROC_I(inode);
-	ei->fd = fd;
-
-	inode->i_mode = S_IFLNK;
-	inode->i_op = &proc_pid_link_inode_operations;
-	inode->i_size = 64;
-	ei->op.proc_get_link = proc_fd_link;
-	d_set_d_op(dentry, &tid_fd_dentry_operations);
-	d_add(dentry, inode);
-	/* Close the race of the process dying before we return the dentry */
-	if (tid_fd_revalidate(dentry, 0))
-		error = NULL;
-
- out:
-	return error;
-}
-
-static struct dentry *proc_lookupfd_common(struct inode *dir,
-					   struct dentry *dentry,
-					   instantiate_t instantiate)
-{
-	struct task_struct *task = get_proc_task(dir);
-	unsigned fd = name_to_int(dentry);
-	struct dentry *result = ERR_PTR(-ENOENT);
-
-	if (!task)
-		goto out_no_task;
-	if (fd == ~0U)
-		goto out;
-
-	result = instantiate(dir, dentry, task, (void *)(unsigned long)fd);
-out:
-	put_task_struct(task);
-out_no_task:
-	return result;
-}
-
-static int proc_readfd_common(struct file * filp, void * dirent,
-			      filldir_t filldir, instantiate_t instantiate)
-{
-	struct dentry *dentry = filp->f_path.dentry;
-	struct inode *inode = dentry->d_inode;
-	struct task_struct *p = get_proc_task(inode);
-	unsigned int fd, ino;
-	int retval;
-	struct files_struct * files;
-
-	retval = -ENOENT;
-	if (!p)
-		goto out_no_task;
-	retval = 0;
-
-	fd = filp->f_pos;
-	switch (fd) {
-		case 0:
-			if (filldir(dirent, ".", 1, 0, inode->i_ino, DT_DIR) < 0)
-				goto out;
-			filp->f_pos++;
-		case 1:
-			ino = parent_ino(dentry);
-			if (filldir(dirent, "..", 2, 1, ino, DT_DIR) < 0)
-				goto out;
-			filp->f_pos++;
-		default:
-			files = get_files_struct(p);
-			if (!files)
-				goto out;
-			rcu_read_lock();
-			for (fd = filp->f_pos-2;
-			     fd < files_fdtable(files)->max_fds;
-			     fd++, filp->f_pos++) {
-				char name[PROC_NUMBUF];
-				int len;
-				int rv;
-
-				if (!fcheck_files(files, fd))
-					continue;
-				rcu_read_unlock();
-
-				len = snprintf(name, sizeof(name), "%d", fd);
-				rv = proc_fill_cache(filp, dirent, filldir,
-						     name, len, instantiate, p,
-						     (void *)(unsigned long)fd);
-				if (rv < 0)
-					goto out_fd_loop;
-				rcu_read_lock();
-			}
-			rcu_read_unlock();
-out_fd_loop:
-			put_files_struct(files);
-	}
-out:
-	put_task_struct(p);
-out_no_task:
-	return retval;
-}
-
-static struct dentry *proc_lookupfd(struct inode *dir, struct dentry *dentry,
-				    unsigned int flags)
-{
-	return proc_lookupfd_common(dir, dentry, proc_fd_instantiate);
-}
-
-static int proc_readfd(struct file *filp, void *dirent, filldir_t filldir)
-{
-	return proc_readfd_common(filp, dirent, filldir, proc_fd_instantiate);
-}
-
-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 const struct file_operations proc_fd_operations = {
-	.read		= generic_read_dir,
-	.readdir	= proc_readfd,
-	.llseek		= default_llseek,
-};
-
 #ifdef CONFIG_CHECKPOINT_RESTORE
 
 /*
@@ -2337,82 +2029,6 @@ static const struct file_operations proc
 
 #endif /* CONFIG_CHECKPOINT_RESTORE */
 
-/*
- * /proc/pid/fd needs a special permission handler so that a process can still
- * access /proc/self/fd after it has executed a setuid().
- */
-static int proc_fd_permission(struct inode *inode, int mask)
-{
-	int rv = generic_permission(inode, mask);
-	if (rv == 0)
-		return 0;
-	if (task_pid(current) == proc_pid(inode))
-		rv = 0;
-	return rv;
-}
-
-/*
- * proc directories can do almost nothing..
- */
-static const struct inode_operations proc_fd_inode_operations = {
-	.lookup		= proc_lookupfd,
-	.permission	= proc_fd_permission,
-	.setattr	= proc_setattr,
-};
-
-static struct dentry *proc_fdinfo_instantiate(struct inode *dir,
-	struct dentry *dentry, struct task_struct *task, const void *ptr)
-{
-	unsigned fd = (unsigned long)ptr;
- 	struct inode *inode;
- 	struct proc_inode *ei;
-	struct dentry *error = ERR_PTR(-ENOENT);
-
-	inode = proc_pid_make_inode(dir->i_sb, task);
-	if (!inode)
-		goto out;
-	ei = PROC_I(inode);
-	ei->fd = fd;
-	inode->i_mode = S_IFREG | S_IRUSR;
-	inode->i_fop = &proc_fdinfo_file_operations;
-	d_set_d_op(dentry, &tid_fd_dentry_operations);
-	d_add(dentry, inode);
-	/* Close the race of the process dying before we return the dentry */
-	if (tid_fd_revalidate(dentry, 0))
-		error = NULL;
-
- out:
-	return error;
-}
-
-static struct dentry *proc_lookupfdinfo(struct inode *dir,
-					struct dentry *dentry,
-					unsigned int flags)
-{
-	return proc_lookupfd_common(dir, dentry, proc_fdinfo_instantiate);
-}
-
-static int proc_readfdinfo(struct file *filp, void *dirent, filldir_t filldir)
-{
-	return proc_readfd_common(filp, dirent, filldir,
-				  proc_fdinfo_instantiate);
-}
-
-static const struct file_operations proc_fdinfo_operations = {
-	.read		= generic_read_dir,
-	.readdir	= proc_readfdinfo,
-	.llseek		= default_llseek,
-};
-
-/*
- * proc directories can do almost nothing..
- */
-static const struct inode_operations proc_fdinfo_inode_operations = {
-	.lookup		= proc_lookupfdinfo,
-	.setattr	= proc_setattr,
-};
-
-
 static struct dentry *proc_pident_instantiate(struct inode *dir,
 	struct dentry *dentry, struct task_struct *task, const void *ptr)
 {
Index: linux-2.6.git/fs/proc/fd.c
===================================================================
--- /dev/null
+++ linux-2.6.git/fs/proc/fd.c
@@ -0,0 +1,351 @@
+#include <linux/sched.h>
+#include <linux/errno.h>
+#include <linux/dcache.h>
+#include <linux/path.h>
+#include <linux/fdtable.h>
+#include <linux/namei.h>
+#include <linux/pid.h>
+#include <linux/security.h>
+
+#include <linux/proc_fs.h>
+
+#include "internal.h"
+#include "fd.h"
+
+#define PROC_FDINFO_MAX 64
+
+static int proc_fd_info(struct inode *inode, struct path *path, char *info)
+{
+	struct task_struct *task = get_proc_task(inode);
+	struct files_struct *files = NULL;
+	int fd = proc_fd(inode);
+	struct file *file;
+
+	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.
+		 */
+		spin_lock(&files->file_lock);
+		file = fcheck_files(files, fd);
+		if (file) {
+			unsigned int f_flags;
+			struct fdtable *fdt;
+
+			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;
+		}
+		spin_unlock(&files->file_lock);
+		put_files_struct(files);
+	}
+	return -ENOENT;
+}
+
+static int tid_fd_revalidate(struct dentry *dentry, unsigned int flags)
+{
+	struct files_struct *files;
+	struct task_struct *task;
+	const struct cred *cred;
+	struct inode *inode;
+	int fd;
+
+	if (flags & LOOKUP_RCU)
+		return -ECHILD;
+
+	inode = dentry->d_inode;
+	task = get_proc_task(inode);
+	fd = proc_fd(inode);
+
+	if (task) {
+		files = get_files_struct(task);
+		if (files) {
+			struct file *file;
+
+			rcu_read_lock();
+			file = fcheck_files(files, fd);
+			if (file) {
+				unsigned f_mode = file->f_mode;
+
+				rcu_read_unlock();
+				put_files_struct(files);
+
+				if (task_dumpable(task)) {
+					rcu_read_lock();
+					cred = __task_cred(task);
+					inode->i_uid = cred->euid;
+					inode->i_gid = cred->egid;
+					rcu_read_unlock();
+				} else {
+					inode->i_uid = GLOBAL_ROOT_UID;
+					inode->i_gid = GLOBAL_ROOT_GID;
+				}
+
+				if (S_ISLNK(inode->i_mode)) {
+					unsigned i_mode = S_IFLNK;
+					if (f_mode & FMODE_READ)
+						i_mode |= S_IRUSR | S_IXUSR;
+					if (f_mode & FMODE_WRITE)
+						i_mode |= S_IWUSR | S_IXUSR;
+					inode->i_mode = i_mode;
+				}
+
+				security_task_to_inode(task, inode);
+				put_task_struct(task);
+				return 1;
+			}
+			rcu_read_unlock();
+			put_files_struct(files);
+		}
+		put_task_struct(task);
+	}
+
+	d_drop(dentry);
+	return 0;
+}
+
+static const struct dentry_operations tid_fd_dentry_operations = {
+	.d_revalidate	= tid_fd_revalidate,
+	.d_delete	= pid_delete_dentry,
+};
+
+static int proc_fd_link(struct dentry *dentry, struct path *path)
+{
+	return proc_fd_info(dentry->d_inode, path, NULL);
+}
+
+static struct dentry *
+proc_fd_instantiate(struct inode *dir, struct dentry *dentry,
+		    struct task_struct *task, const void *ptr)
+{
+	struct dentry *error = ERR_PTR(-ENOENT);
+	unsigned fd = (unsigned long)ptr;
+	struct proc_inode *ei;
+	struct inode *inode;
+
+	inode = proc_pid_make_inode(dir->i_sb, task);
+	if (!inode)
+		goto out;
+
+	ei = PROC_I(inode);
+	ei->fd = fd;
+
+	inode->i_mode = S_IFLNK;
+	inode->i_op = &proc_pid_link_inode_operations;
+	inode->i_size = 64;
+
+	ei->op.proc_get_link = proc_fd_link;
+
+	d_set_d_op(dentry, &tid_fd_dentry_operations);
+	d_add(dentry, inode);
+
+	/* Close the race of the process dying before we return the dentry */
+	if (tid_fd_revalidate(dentry, 0))
+		error = NULL;
+ out:
+	return error;
+}
+
+static struct dentry *proc_lookupfd_common(struct inode *dir,
+					   struct dentry *dentry,
+					   instantiate_t instantiate)
+{
+	struct task_struct *task = get_proc_task(dir);
+	struct dentry *result = ERR_PTR(-ENOENT);
+	unsigned fd = name_to_int(dentry);
+
+	if (!task)
+		goto out_no_task;
+	if (fd == ~0U)
+		goto out;
+
+	result = instantiate(dir, dentry, task, (void *)(unsigned long)fd);
+out:
+	put_task_struct(task);
+out_no_task:
+	return result;
+}
+
+static int proc_readfd_common(struct file * filp, void * dirent,
+			      filldir_t filldir, instantiate_t instantiate)
+{
+	struct dentry *dentry = filp->f_path.dentry;
+	struct inode *inode = dentry->d_inode;
+	struct task_struct *p = get_proc_task(inode);
+	struct files_struct *files;
+	unsigned int fd, ino;
+	int retval;
+
+	retval = -ENOENT;
+	if (!p)
+		goto out_no_task;
+	retval = 0;
+
+	fd = filp->f_pos;
+	switch (fd) {
+		case 0:
+			if (filldir(dirent, ".", 1, 0, inode->i_ino, DT_DIR) < 0)
+				goto out;
+			filp->f_pos++;
+		case 1:
+			ino = parent_ino(dentry);
+			if (filldir(dirent, "..", 2, 1, ino, DT_DIR) < 0)
+				goto out;
+			filp->f_pos++;
+		default:
+			files = get_files_struct(p);
+			if (!files)
+				goto out;
+			rcu_read_lock();
+			for (fd = filp->f_pos - 2;
+			     fd < files_fdtable(files)->max_fds;
+			     fd++, filp->f_pos++) {
+				char name[PROC_NUMBUF];
+				int len;
+				int rv;
+
+				if (!fcheck_files(files, fd))
+					continue;
+				rcu_read_unlock();
+
+				len = snprintf(name, sizeof(name), "%d", fd);
+				rv = proc_fill_cache(filp, dirent, filldir,
+						     name, len, instantiate, p,
+						     (void *)(unsigned long)fd);
+				if (rv < 0)
+					goto out_fd_loop;
+				rcu_read_lock();
+			}
+			rcu_read_unlock();
+out_fd_loop:
+			put_files_struct(files);
+	}
+out:
+	put_task_struct(p);
+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);
+}
+
+const struct file_operations proc_fd_operations = {
+	.read		= generic_read_dir,
+	.readdir	= proc_readfd,
+	.llseek		= default_llseek,
+};
+
+static struct dentry *proc_lookupfd(struct inode *dir, struct dentry *dentry,
+				    unsigned int flags)
+{
+	return proc_lookupfd_common(dir, dentry, proc_fd_instantiate);
+}
+
+/*
+ * /proc/pid/fd needs a special permission handler so that a process can still
+ * access /proc/self/fd after it has executed a setuid().
+ */
+int proc_fd_permission(struct inode *inode, int mask)
+{
+	int rv = generic_permission(inode, mask);
+	if (rv == 0)
+		return 0;
+	if (task_pid(current) == proc_pid(inode))
+		rv = 0;
+	return rv;
+}
+
+const struct inode_operations proc_fd_inode_operations = {
+	.lookup		= proc_lookupfd,
+	.permission	= proc_fd_permission,
+	.setattr	= proc_setattr,
+};
+
+static struct dentry *
+proc_fdinfo_instantiate(struct inode *dir, struct dentry *dentry,
+			struct task_struct *task, const void *ptr)
+{
+	struct dentry *error = ERR_PTR(-ENOENT);
+	unsigned fd = (unsigned long)ptr;
+	struct proc_inode *ei;
+	struct inode *inode;
+
+	inode = proc_pid_make_inode(dir->i_sb, task);
+	if (!inode)
+		goto out;
+
+	ei = PROC_I(inode);
+	ei->fd = fd;
+
+	inode->i_mode = S_IFREG | S_IRUSR;
+	inode->i_fop = &proc_fdinfo_file_operations;
+
+	d_set_d_op(dentry, &tid_fd_dentry_operations);
+	d_add(dentry, inode);
+
+	/* Close the race of the process dying before we return the dentry */
+	if (tid_fd_revalidate(dentry, 0))
+		error = NULL;
+ out:
+	return error;
+}
+
+static struct dentry *
+proc_lookupfdinfo(struct inode *dir, struct dentry *dentry, unsigned int flags)
+{
+	return proc_lookupfd_common(dir, dentry, proc_fdinfo_instantiate);
+}
+
+static int proc_readfdinfo(struct file *filp, void *dirent, filldir_t filldir)
+{
+	return proc_readfd_common(filp, dirent, filldir,
+				  proc_fdinfo_instantiate);
+}
+
+const struct inode_operations proc_fdinfo_inode_operations = {
+	.lookup		= proc_lookupfdinfo,
+	.setattr	= proc_setattr,
+};
+
+const struct file_operations proc_fdinfo_operations = {
+	.read		= generic_read_dir,
+	.readdir	= proc_readfdinfo,
+	.llseek		= default_llseek,
+};
Index: linux-2.6.git/fs/proc/fd.h
===================================================================
--- /dev/null
+++ linux-2.6.git/fs/proc/fd.h
@@ -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__ */
Index: linux-2.6.git/fs/proc/internal.h
===================================================================
--- linux-2.6.git.orig/fs/proc/internal.h
+++ linux-2.6.git/fs/proc/internal.h
@@ -9,6 +9,7 @@
  * 2 of the License, or (at your option) any later version.
  */
 
+#include <linux/sched.h>
 #include <linux/proc_fs.h>
 struct  ctl_table_header;
 
@@ -65,6 +66,7 @@ extern const struct file_operations proc
 extern const struct file_operations proc_pagemap_operations;
 extern const struct file_operations proc_net_operations;
 extern const struct inode_operations proc_net_inode_operations;
+extern const struct inode_operations proc_pid_link_inode_operations;
 
 struct proc_maps_private {
 	struct pid *pid;
@@ -91,6 +93,52 @@ static inline int proc_fd(struct inode *
 	return PROC_I(inode)->fd;
 }
 
+static inline int task_dumpable(struct task_struct *task)
+{
+	int dumpable = 0;
+	struct mm_struct *mm;
+
+	task_lock(task);
+	mm = task->mm;
+	if (mm)
+		dumpable = get_dumpable(mm);
+	task_unlock(task);
+	if(dumpable == 1)
+		return 1;
+	return 0;
+}
+
+static inline int pid_delete_dentry(const struct dentry * dentry)
+{
+	/* Is the task we represent dead?
+	 * If so, then don't put the dentry on the lru list,
+	 * kill it immediately.
+	 */
+	return !proc_pid(dentry->d_inode)->tasks[PIDTYPE_PID].first;
+}
+
+static inline unsigned name_to_int(struct dentry *dentry)
+{
+	const char *name = dentry->d_name.name;
+	int len = dentry->d_name.len;
+	unsigned n = 0;
+
+	if (len > 1 && *name == '0')
+		goto out;
+	while (len-- > 0) {
+		unsigned c = *name++ - '0';
+		if (c > 9)
+			goto out;
+		if (n >= (~0U-9)/10)
+			goto out;
+		n *= 10;
+		n += c;
+	}
+	return n;
+out:
+	return ~0U;
+}
+
 struct dentry *proc_lookup_de(struct proc_dir_entry *de, struct inode *ino,
 		struct dentry *dentry);
 int proc_readdir_de(struct proc_dir_entry *de, struct file *filp, void *dirent,

[patch 2/8] procfs: Convert /proc/pid/fdinfo/ handling routines to seq-file

[Cyrill Gorcunov]

[-- Attachment #1: seq-fdinfo-seq-ops-6 --]
[-- Type: text/plain, Size: 5152 bytes --]

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).

Also, to eliminate code duplication (and Pavel's concerns) the fdinfo_open_helper
function introduced which is used in both seq_fdinfo_open and proc_fd_link.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Acked-by: Pavel Emelyanov <xemul@parallels.com>
CC: Al Viro <viro@ZenIV.linux.org.uk>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: James Bottomley <jbottomley@parallels.com>
---
 fs/proc/fd.c |  123 +++++++++++++++++++++++++++++++++++------------------------
 1 file changed, 75 insertions(+), 48 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,104 @@
 #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 {
+	loff_t	f_pos;
+	int	f_flags;
+};
 
-static int proc_fd_info(struct inode *inode, struct path *path, char *info)
+static int fdinfo_open_helper(struct inode *inode, int *f_flags, struct path *path)
 {
-	struct task_struct *task = get_proc_task(inode);
 	struct files_struct *files = NULL;
-	int fd = proc_fd(inode);
-	struct file *file;
+	struct task_struct *task;
+	int 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.
-		 */
-		spin_lock(&files->file_lock);
-		file = fcheck_files(files, fd);
-		if (file) {
-			unsigned int f_flags;
-			struct fdtable *fdt;
-
-			fdt = files_fdtable(files);
-			f_flags = file->f_flags & ~O_CLOEXEC;
-			if (close_on_exec(fd, fdt))
-				f_flags |= O_CLOEXEC;
+		int fd = proc_fd(inode);
+		struct file *fd_file;
 
+		spin_lock(&files->file_lock);
+		fd_file = fcheck_files(files, fd);
+		if (fd_file) {
+			if (f_flags) {
+				struct fdtable *fdt = files_fdtable(files);
+
+				*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);
+				*path = fd_file->f_path;
+				path_get(&fd_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;
+			ret = 0;
 		}
 		spin_unlock(&files->file_lock);
 		put_files_struct(files);
 	}
-	return -ENOENT;
+
+	return ret;
 }
 
+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;
+}
+
+static int seq_fdinfo_open(struct inode *inode, struct file *file)
+{
+	struct proc_fdinfo *fdinfo = NULL;
+	int ret = -ENOENT;
+
+	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;
+	}
+
+	kfree(fdinfo);
+	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;
+
+	kfree(fdinfo);
+
+	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 +173,7 @@ 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);
+	return fdinfo_open_helper(dentry->d_inode, NULL, path);
 }
 
 static struct dentry *
@@ -245,22 +288,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);

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

[Cyrill Gorcunov]

[-- Attachment #1: seq-fdinfo-seq-ops-helpers-10 --]
[-- Type: text/plain, Size: 4175 bytes --]

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 <gorcunov@openvz.org>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Al Viro <viro@ZenIV.linux.org.uk>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: James Bottomley <jbottomley@parallels.com>
---
 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 {

[patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

[-- Attachment #1: seq-fs-exportfs-ino-3 --]
[-- Type: text/plain, Size: 1885 bytes --]

To provide fsnotify object inodes being watched without
binding to alphabetical path we need to encode them with
exportfs help. This patch adds a helper which operates
with plain inodes directly.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
Acked-by: Pavel Emelyanov <xemul@parallels.com>
CC: Al Viro <viro@ZenIV.linux.org.uk>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: James Bottomley <jbottomley@parallels.com>
---
 fs/exportfs/expfs.c      |   19 +++++++++++++++++++
 include/linux/exportfs.h |    2 ++
 2 files changed, 21 insertions(+)

Index: linux-2.6.git/fs/exportfs/expfs.c
===================================================================
--- linux-2.6.git.orig/fs/exportfs/expfs.c
+++ linux-2.6.git/fs/exportfs/expfs.c
@@ -302,6 +302,25 @@ out:
 	return error;
 }
 
+int export_encode_inode_fh(struct inode *inode, struct fid *fid, int *max_len)
+{
+	int len = *max_len;
+	int type = FILEID_INO32_GEN;
+
+	if (len < 2) {
+		*max_len = 2;
+		return 255;
+	}
+
+	len = 2;
+	fid->i32.ino = inode->i_ino;
+	fid->i32.gen = inode->i_generation;
+	*max_len = len;
+
+	return type;
+}
+EXPORT_SYMBOL_GPL(export_encode_inode_fh);
+
 /**
  * export_encode_fh - default export_operations->encode_fh function
  * @inode:   the object to encode
Index: linux-2.6.git/include/linux/exportfs.h
===================================================================
--- linux-2.6.git.orig/include/linux/exportfs.h
+++ linux-2.6.git/include/linux/exportfs.h
@@ -177,6 +177,8 @@ struct export_operations {
 	int (*commit_metadata)(struct inode *inode);
 };
 
+extern int export_encode_inode_fh(struct inode *inode, struct fid *fid, int *max_len);
+
 extern int exportfs_encode_fh(struct dentry *dentry, struct fid *fid,
 	int *max_len, int connectable);
 extern struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,

[patch 5/8] fs, notify: Add procfs fdinfo helper v3

[Cyrill Gorcunov]

[-- Attachment #1: seq-fdinfo-fsnotify-5 --]
[-- Type: text/plain, Size: 8462 bytes --]

This allow us to print out fsnotify details such as
watchee inode, device, mask and file handle.

For example for inotify objects the output is

 | 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

For fanotify it is like

 | pos:	0
 | flags:	02
 | fanotify ino:            68f71 sdev:   800013 mask:        1 ignored_mask: 40000000 fhandle-bytes:        8 fhandle-type:        1 f_handle: 718f0600b9f42053
 | 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.

v2:
 - append missing colons to terms
v3:
 - continue from pervious position in list on ->next

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Al Viro <viro@ZenIV.linux.org.uk>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: James Bottomley <jbottomley@parallels.com>
---
 fs/notify/Makefile                 |    2 
 fs/notify/fanotify/fanotify_user.c |    4 
 fs/notify/fdinfo.c                 |  167 +++++++++++++++++++++++++++++++++++++
 fs/notify/fdinfo.h                 |   22 ++++
 fs/notify/inotify/inotify_user.c   |    4 
 5 files changed, 198 insertions(+), 1 deletion(-)

Index: linux-2.6.git/fs/notify/Makefile
===================================================================
--- linux-2.6.git.orig/fs/notify/Makefile
+++ linux-2.6.git/fs/notify/Makefile
@@ -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/
Index: linux-2.6.git/fs/notify/fanotify/fanotify_user.c
===================================================================
--- linux-2.6.git.orig/fs/notify/fanotify/fanotify_user.c
+++ linux-2.6.git/fs/notify/fanotify/fanotify_user.c
@@ -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,
Index: linux-2.6.git/fs/notify/fdinfo.c
===================================================================
--- /dev/null
+++ linux-2.6.git/fs/notify/fdinfo.c
@@ -0,0 +1,167 @@
+#include <linux/file.h>
+#include <linux/fs.h>
+#include <linux/fsnotify_backend.h>
+#include <linux/idr.h>
+#include <linux/init.h>
+#include <linux/inotify.h>
+#include <linux/kernel.h>
+#include <linux/namei.h>
+#include <linux/sched.h>
+#include <linux/types.h>
+#include <linux/seq_file.h>
+#include <linux/exportfs.h>
+#include <linux/proc_fs.h>
+
+#include "inotify/inotify.h"
+#include "../fs/mount.h"
+
+struct inode_file_handle {
+	struct file_handle		h;
+	struct fid			fid;
+} __packed;
+
+#if defined(CONFIG_PROC_FS)
+
+#if defined(CONFIG_INOTIFY_USER) || defined(CONFIG_FANOTIFY)
+
+#ifdef CONFIG_EXPORTFS
+static int inotify_encode_target(struct inode *inode, struct inode_file_handle *fhandle)
+{
+	int ret, size;
+
+	size = sizeof(fhandle->fid) >> 2;
+	ret = export_encode_inode_fh(inode, &fhandle->fid, &size);
+	BUG_ON(ret != FILEID_INO32_GEN);
+
+	fhandle->h.handle_type = FILEID_INO32_GEN;
+	fhandle->h.handle_bytes = size * sizeof(u32);
+
+	return 0;
+}
+#else
+static int inotify_encode_target(struct inode *inode, struct inode_file_handle *fhandle)
+{
+	fhandle->h.handle_type = FILEID_ROOT;
+	fhandle->h.handle_bytes = 0;
+	return 0;
+}
+#endif /* CONFIG_EXPORTFS */
+
+static int show_fdinfo(struct seq_file *m, struct file *f,
+		       int (*show)(struct seq_file *m, struct fsnotify_mark *mark))
+{
+	struct fsnotify_group *group = f->private_data;
+	struct fsnotify_mark *mark;
+	int ret = 0;
+
+	spin_lock(&group->mark_lock);
+	list_for_each_entry(mark, &group->marks_list, g_list) {
+		ret = show(m, mark);
+		if (ret)
+			break;
+	}
+	spin_unlock(&group->mark_lock);
+	return ret;
+}
+
+#ifdef CONFIG_INOTIFY_USER
+
+static int inotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
+{
+	struct inotify_inode_mark *inode_mark;
+	struct inode *inode;
+	int ret = 0;
+
+	if (!(mark->flags & (FSNOTIFY_MARK_FLAG_ALIVE | FSNOTIFY_MARK_FLAG_INODE)))
+		return 0;
+
+	inode_mark = container_of(mark, struct inotify_inode_mark, fsn_mark);
+	inode = igrab(mark->i.inode);
+	if (inode) {
+		struct inode_file_handle fhandle;
+		int i;
+
+		inotify_encode_target(inode, &fhandle);
+
+		ret = seq_printf(m, "inotify wd: %8d ino: %16lx sdev: %8x "
+				 "mask: %8x ignored_mask: %8x "
+				 "fhandle-bytes: %8x fhandle-type: %8x f_handle: ",
+				 inode_mark->wd, inode->i_ino,
+				 inode->i_sb->s_dev,
+				 mark->mask,
+				 mark->ignored_mask,
+				 fhandle.h.handle_bytes,
+				 fhandle.h.handle_type);
+		for (i = 0; i < fhandle.h.handle_bytes; i++) {
+			ret |= seq_printf(m, "%02x",
+					  (int)(unsigned char)fhandle.h.f_handle[i]);
+		}
+		ret |= seq_putc(m, '\n');
+		iput(inode);
+	}
+
+	return ret;
+}
+
+int inotify_show_fdinfo(struct seq_file *m, struct file *f)
+{
+	return show_fdinfo(m, f, inotify_fdinfo);
+}
+
+#endif /* CONFIG_INOTIFY_USER */
+
+#ifdef CONFIG_FANOTIFY
+
+static int fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
+{
+	struct inode *inode;
+	int ret = 0;
+
+	if (!(mark->flags & FSNOTIFY_MARK_FLAG_ALIVE))
+		return 0;
+
+	if (mark->flags & FSNOTIFY_MARK_FLAG_INODE) {
+		struct inode_file_handle fhandle;
+		int i;
+
+		inode = igrab(mark->i.inode);
+		if (!inode)
+			goto out;
+		inotify_encode_target(inode, &fhandle);
+
+		ret = seq_printf(m, "fanotify ino: %16lx sdev: %8x "
+				 "mask: %8x ignored_mask: %8x "
+				 "fhandle-bytes: %8x fhandle-type: %8x f_handle: ",
+				 inode->i_ino,
+				 inode->i_sb->s_dev,
+				 mark->mask,
+				 mark->ignored_mask,
+				 fhandle.h.handle_bytes,
+				 fhandle.h.handle_type);
+
+		for (i = 0; i < fhandle.h.handle_bytes; i++) {
+			ret |= seq_printf(m, "%02x",
+					  (int)(unsigned char)fhandle.h.f_handle[i]);
+		}
+		ret |= seq_putc(m, '\n');
+		iput(inode);
+	} else if (mark->flags & FSNOTIFY_MARK_FLAG_VFSMOUNT) {
+		struct mount *mnt = real_mount(mark->m.mnt);
+
+		ret = seq_printf(m, "fanotify mnt_id: %8x mask: %8x ignored_mask: %8x\n",
+				 mnt->mnt_id, mark->mask, mark->ignored_mask);
+	}
+out:
+	return ret;
+}
+
+int fanotify_show_fdinfo(struct seq_file *m, struct file *f)
+{
+	return show_fdinfo(m, f, fanotify_fdinfo);
+}
+
+#endif /* CONFIG_FANOTIFY */
+
+#endif /* CONFIG_INOTIFY_USER || CONFIG_FANOTIFY */
+
+#endif /* CONFIG_PROC_FS */
Index: linux-2.6.git/fs/notify/fdinfo.h
===================================================================
--- /dev/null
+++ linux-2.6.git/fs/notify/fdinfo.h
@@ -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__ */
Index: linux-2.6.git/fs/notify/inotify/inotify_user.c
===================================================================
--- linux-2.6.git.orig/fs/notify/inotify/inotify_user.c
+++ linux-2.6.git/fs/notify/inotify/inotify_user.c
@@ -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 6/8] fs, eventfd: Add procfs fdinfo helper

[Cyrill Gorcunov]

[-- Attachment #1: seq-fdinfo-eventfd-7 --]
[-- Type: text/plain, Size: 1481 bytes --]

This allow us to print out raw counter value.
The /proc/pid/fdinfo/fd output is

 | pos:	0
 | flags:	04002
 | eventfd-count:               5a

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Al Viro <viro@ZenIV.linux.org.uk>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: James Bottomley <jbottomley@parallels.com>
---
 fs/eventfd.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

Index: linux-2.6.git/fs/eventfd.c
===================================================================
--- linux-2.6.git.orig/fs/eventfd.c
+++ linux-2.6.git/fs/eventfd.c
@@ -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 file
 	return 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 7/8] fs, epoll: Add procfs fdinfo helper v2

[Cyrill Gorcunov]

[-- Attachment #1: seq-fdinfo-eventpoll-7 --]
[-- Type: text/plain, Size: 1905 bytes --]

This 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.

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Al Viro <viro@ZenIV.linux.org.uk>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: James Bottomley <jbottomley@parallels.com>
CC: Matthew Helsley <matt.helsley@gmail.com>
---
 fs/eventpoll.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

Index: linux-2.6.git/fs/eventpoll.c
===================================================================
--- linux-2.6.git.orig/fs/eventpoll.c
+++ linux-2.6.git/fs/eventpoll.c
@@ -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(st
 	return 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 8/8] fdinfo: Show sigmask for signalfd fd v2

[Cyrill Gorcunov]

[-- Attachment #1: seq-fdinfo-signalfd-3 --]
[-- Type: text/plain, Size: 2893 bytes --]

Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 fs/proc/array.c         |    2 +-
 fs/signalfd.c           |   26 ++++++++++++++++++++++++++
 include/linux/proc_fs.h |    3 +++
 3 files changed, 30 insertions(+), 1 deletion(-)

Index: linux-2.6.git/fs/proc/array.c
===================================================================
--- linux-2.6.git.orig/fs/proc/array.c
+++ linux-2.6.git/fs/proc/array.c
@@ -220,7 +220,7 @@ static inline void task_state(struct seq
 	seq_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;
Index: linux-2.6.git/fs/signalfd.c
===================================================================
--- linux-2.6.git.orig/fs/signalfd.c
+++ linux-2.6.git/fs/signalfd.c
@@ -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 file
 	return 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, sig
 			return -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, sig
 			return -EINVAL;
 		}
 		spin_lock_irq(&current->sighand->siglock);
+		write_seqcount_begin(&ctx->cnt);
 		ctx->sigmask = sigmask;
+		write_seqcount_end(&ctx->cnt);
 		spin_unlock_irq(&current->sighand->siglock);
 
 		wake_up(&current->sighand->signalfd_wqh);
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
@@ -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 */

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[J. Bruce Fields]

On Wed, Aug 15, 2012 at 01:21:20PM +0400, Cyrill Gorcunov wrote:
> To provide fsnotify object inodes being watched without
> binding to alphabetical path we need to encode them with
> exportfs help. This patch adds a helper which operates
> with plain inodes directly.

I don't get it--this seems like a really roundabout way to get inode and
generation number, if that's all you want.

On the other hand, if you want a real filehandle then wouldn't you want
to e.g. call the filesystem's ->encode_fh() if necessary, as
exportfs_encode_fh() does?

--b.

> 
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Acked-by: Pavel Emelyanov <xemul@parallels.com>
> CC: Al Viro <viro@ZenIV.linux.org.uk>
> CC: Alexey Dobriyan <adobriyan@gmail.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: James Bottomley <jbottomley@parallels.com>
> ---
>  fs/exportfs/expfs.c      |   19 +++++++++++++++++++
>  include/linux/exportfs.h |    2 ++
>  2 files changed, 21 insertions(+)
> 
> Index: linux-2.6.git/fs/exportfs/expfs.c
> ===================================================================
> --- linux-2.6.git.orig/fs/exportfs/expfs.c
> +++ linux-2.6.git/fs/exportfs/expfs.c
> @@ -302,6 +302,25 @@ out:
>  	return error;
>  }
>  
> +int export_encode_inode_fh(struct inode *inode, struct fid *fid, int *max_len)
> +{
> +	int len = *max_len;
> +	int type = FILEID_INO32_GEN;
> +
> +	if (len < 2) {
> +		*max_len = 2;
> +		return 255;
> +	}
> +
> +	len = 2;
> +	fid->i32.ino = inode->i_ino;
> +	fid->i32.gen = inode->i_generation;
> +	*max_len = len;
> +
> +	return type;
> +}
> +EXPORT_SYMBOL_GPL(export_encode_inode_fh);
> +
>  /**
>   * export_encode_fh - default export_operations->encode_fh function
>   * @inode:   the object to encode
> Index: linux-2.6.git/include/linux/exportfs.h
> ===================================================================
> --- linux-2.6.git.orig/include/linux/exportfs.h
> +++ linux-2.6.git/include/linux/exportfs.h
> @@ -177,6 +177,8 @@ struct export_operations {
>  	int (*commit_metadata)(struct inode *inode);
>  };
>  
> +extern int export_encode_inode_fh(struct inode *inode, struct fid *fid, int *max_len);
> +
>  extern int exportfs_encode_fh(struct dentry *dentry, struct fid *fid,
>  	int *max_len, int connectable);
>  extern struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Wed, Aug 15, 2012 at 04:45:46PM -0400, J. Bruce Fields wrote:
> On Wed, Aug 15, 2012 at 01:21:20PM +0400, Cyrill Gorcunov wrote:
> > To provide fsnotify object inodes being watched without
> > binding to alphabetical path we need to encode them with
> > exportfs help. This patch adds a helper which operates
> > with plain inodes directly.
> 
> I don't get it--this seems like a really roundabout way to get inode and
> generation number, if that's all you want.

We can re-open the targets via filehandle on restore, this was the idea.
All this series aimed to achieve the way to restore objects after checkpoit,
thus we need to provide additional information which would be enough.

> On the other hand, if you want a real filehandle then wouldn't you want
> to e.g. call the filesystem's ->encode_fh() if necessary, as
> exportfs_encode_fh() does?

Well, one of the problem I hit when I've been trying to use encode_fh
is that every new implementation of encode_fh will require some size
(even unknown) in buffer where encoded data pushed. Correct me please
if I'm wrong. But with export_encode_inode_fh there is a small buffer
with pretty known size needed on stack needed for printing data in
fdinfo.

	Cyrill

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

[Al Viro]

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.

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

[Al Viro]

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...

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

[Cyrill Gorcunov]

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

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

[Cyrill Gorcunov]

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

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[J. Bruce Fields]

On Thu, Aug 16, 2012 at 01:02:37AM +0400, Cyrill Gorcunov wrote:
> On Wed, Aug 15, 2012 at 04:45:46PM -0400, J. Bruce Fields wrote:
> > On Wed, Aug 15, 2012 at 01:21:20PM +0400, Cyrill Gorcunov wrote:
> > > To provide fsnotify object inodes being watched without
> > > binding to alphabetical path we need to encode them with
> > > exportfs help. This patch adds a helper which operates
> > > with plain inodes directly.
> > 
> > I don't get it--this seems like a really roundabout way to get inode and
> > generation number, if that's all you want.
> 
> We can re-open the targets via filehandle on restore, this was the idea.
> All this series aimed to achieve the way to restore objects after checkpoit,
> thus we need to provide additional information which would be enough.

For this to work it'll need to be something you can pass to
open_by_handle_at, won't it?

> > On the other hand, if you want a real filehandle then wouldn't you want
> > to e.g. call the filesystem's ->encode_fh() if necessary, as
> > exportfs_encode_fh() does?
> 
> Well, one of the problem I hit when I've been trying to use encode_fh
> is that every new implementation of encode_fh will require some size
> (even unknown) in buffer where encoded data pushed. Correct me please
> if I'm wrong. But with export_encode_inode_fh there is a small buffer
> with pretty known size needed on stack needed for printing data in
> fdinfo.

You can just give encode_fh a too-small data and let it fail if it's not
big enough.

(In practice I think everyone supports NFSv3 filehandles which have a
maximum size of 64 bytes.)

--b.

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Wed, Aug 15, 2012 at 06:06:23PM -0400, J. Bruce Fields wrote:
> On Thu, Aug 16, 2012 at 01:02:37AM +0400, Cyrill Gorcunov wrote:
> > On Wed, Aug 15, 2012 at 04:45:46PM -0400, J. Bruce Fields wrote:
> > > On Wed, Aug 15, 2012 at 01:21:20PM +0400, Cyrill Gorcunov wrote:
> > > > To provide fsnotify object inodes being watched without
> > > > binding to alphabetical path we need to encode them with
> > > > exportfs help. This patch adds a helper which operates
> > > > with plain inodes directly.
> > > 
> > > I don't get it--this seems like a really roundabout way to get inode and
> > > generation number, if that's all you want.
> > 
> > We can re-open the targets via filehandle on restore, this was the idea.
> > All this series aimed to achieve the way to restore objects after checkpoit,
> > thus we need to provide additional information which would be enough.
> 
> For this to work it'll need to be something you can pass to
> open_by_handle_at, won't it?

Yes, and for inotify we print out this handled via fdinfo, later on restore
we use it together with open_by_handle_at.

> 
> > > On the other hand, if you want a real filehandle then wouldn't you want
> > > to e.g. call the filesystem's ->encode_fh() if necessary, as
> > > exportfs_encode_fh() does?
> > 
> > Well, one of the problem I hit when I've been trying to use encode_fh
> > is that every new implementation of encode_fh will require some size
> > (even unknown) in buffer where encoded data pushed. Correct me please
> > if I'm wrong. But with export_encode_inode_fh there is a small buffer
> > with pretty known size needed on stack needed for printing data in
> > fdinfo.
> 
> You can just give encode_fh a too-small data and let it fail if it's not
> big enough.
> 
> (In practice I think everyone supports NFSv3 filehandles which have a
> maximum size of 64 bytes.)

I'll think about it, thanks!

	Cyrill

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

[Cyrill Gorcunov]

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 <gorcunov@openvz.org>
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 <gorcunov@openvz.org>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Al Viro <viro@ZenIV.linux.org.uk>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: James Bottomley <jbottomley@parallels.com>
---
 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);

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Thu, Aug 16, 2012 at 10:24:48AM +0400, Cyrill Gorcunov wrote:
> > > > On the other hand, if you want a real filehandle then wouldn't you want
> > > > to e.g. call the filesystem's ->encode_fh() if necessary, as
> > > > exportfs_encode_fh() does?
> > > 
> > > Well, one of the problem I hit when I've been trying to use encode_fh
> > > is that every new implementation of encode_fh will require some size
> > > (even unknown) in buffer where encoded data pushed. Correct me please
> > > if I'm wrong. But with export_encode_inode_fh there is a small buffer
> > > with pretty known size needed on stack needed for printing data in
> > > fdinfo.
> > 
> > You can just give encode_fh a too-small data and let it fail if it's not
> > big enough.
> > 
> > (In practice I think everyone supports NFSv3 filehandles which have a
> > maximum size of 64 bytes.)
> 
> I'll think about it, thanks!

Hi Bruce, thinking a bit more I guess using general encode_fh is not that
convenient since it operates with dentries while our fdinfo output deals
with inodes. Thus I should either provide some new encode_fh variant
which would deal with inodes directly without "parents". Which doesn't
look for me anyhow better than the new export_encode_inode_fh helper.

After all, if the use of encode_fh become a mandatory rule we can easily
extend fsnotify fdinfo output to support new scheme without breaking
user space, because output looks like

 | fhandle-type:        1 f_handle: 49b1060023552153

(ie if something is changed than these fields will be simply updated).

Or maybe I miss something?

	Cyrill

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[J. Bruce Fields]

On Thu, Aug 16, 2012 at 04:38:14PM +0400, Cyrill Gorcunov wrote:
> On Thu, Aug 16, 2012 at 10:24:48AM +0400, Cyrill Gorcunov wrote:
> > > > > On the other hand, if you want a real filehandle then wouldn't you want
> > > > > to e.g. call the filesystem's ->encode_fh() if necessary, as
> > > > > exportfs_encode_fh() does?
> > > > 
> > > > Well, one of the problem I hit when I've been trying to use encode_fh
> > > > is that every new implementation of encode_fh will require some size
> > > > (even unknown) in buffer where encoded data pushed. Correct me please
> > > > if I'm wrong. But with export_encode_inode_fh there is a small buffer
> > > > with pretty known size needed on stack needed for printing data in
> > > > fdinfo.
> > > 
> > > You can just give encode_fh a too-small data and let it fail if it's not
> > > big enough.
> > > 
> > > (In practice I think everyone supports NFSv3 filehandles which have a
> > > maximum size of 64 bytes.)
> > 
> > I'll think about it, thanks!
> 
> Hi Bruce, thinking a bit more I guess using general encode_fh is not that
> convenient since it operates with dentries while our fdinfo output deals
> with inodes. Thus I should either provide some new encode_fh variant
> which would deal with inodes directly without "parents".

I can't see why that wouldn't work.

> Which doesn't
> look for me anyhow better than the new export_encode_inode_fh helper.

That isn't going to work for filesystems that define their own
encode_fh:

	$ git grep '\.encode_fh'
	fs/btrfs/export.c:      .encode_fh      = btrfs_encode_fh,
	fs/ceph/export.c:       .encode_fh = ceph_encode_fh,
	fs/fat/inode.c: .encode_fh      = fat_encode_fh,
	fs/fuse/inode.c:        .encode_fh      = fuse_encode_fh,
	fs/gfs2/export.c:       .encode_fh = gfs2_encode_fh,
	fs/isofs/export.c:      .encode_fh      = isofs_export_encode_fh,
	fs/nilfs2/namei.c:      .encode_fh = nilfs_encode_fh,
	fs/ocfs2/export.c:      .encode_fh      = ocfs2_encode_fh,
	fs/reiserfs/super.c:    .encode_fh = reiserfs_encode_fh,
	fs/udf/namei.c: .encode_fh      = udf_encode_fh,
	fs/xfs/xfs_export.c:    .encode_fh              = xfs_fs_encode_fh,
	mm/shmem.c:     .encode_fh      = shmem_encode_fh,

--b.

> After all, if the use of encode_fh become a mandatory rule we can easily
> extend fsnotify fdinfo output to support new scheme without breaking
> user space, because output looks like
> 
>  | fhandle-type:        1 f_handle: 49b1060023552153
> 
> (ie if something is changed than these fields will be simply updated).
> 
> Or maybe I miss something?
> 
> 	Cyrill

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Thu, Aug 16, 2012 at 08:47:03AM -0400, J. Bruce Fields wrote:
> > Hi Bruce, thinking a bit more I guess using general encode_fh is not that
> > convenient since it operates with dentries while our fdinfo output deals
> > with inodes. Thus I should either provide some new encode_fh variant
> > which would deal with inodes directly without "parents".
> 
> I can't see why that wouldn't work.
> 
> > Which doesn't
> > look for me anyhow better than the new export_encode_inode_fh helper.
> 
> That isn't going to work for filesystems that define their own
> encode_fh:
> 
> 	$ git grep '\.encode_fh'
> 	fs/btrfs/export.c:      .encode_fh      = btrfs_encode_fh,
> 	fs/ceph/export.c:       .encode_fh = ceph_encode_fh,
	...

Agreed. I'll try to cook something.

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Al Viro]

On Thu, Aug 16, 2012 at 04:38:14PM +0400, Cyrill Gorcunov wrote:

> Hi Bruce, thinking a bit more I guess using general encode_fh is not that
> convenient since it operates with dentries while our fdinfo output deals
> with inodes. Thus I should either provide some new encode_fh variant
> which would deal with inodes directly without "parents". Which doesn't
> look for me anyhow better than the new export_encode_inode_fh helper.

Huh?  You do have dentries, for crying out loud...

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Pavel Emelyanov]

On 08/16/2012 05:43 PM, Al Viro wrote:
> On Thu, Aug 16, 2012 at 04:38:14PM +0400, Cyrill Gorcunov wrote:
> 
>> Hi Bruce, thinking a bit more I guess using general encode_fh is not that
>> convenient since it operates with dentries while our fdinfo output deals
>> with inodes. Thus I should either provide some new encode_fh variant
>> which would deal with inodes directly without "parents". Which doesn't
>> look for me anyhow better than the new export_encode_inode_fh helper.
> 
> Huh?  You do have dentries, for crying out loud...

Sometimes we don't -- the inotify thing gets an inode only.
Unlike other notifies that have dentries at hands...

> .
> 

Thanks,
Pavel

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Al Viro]

On Thu, Aug 16, 2012 at 02:43:39PM +0100, Al Viro wrote:
> On Thu, Aug 16, 2012 at 04:38:14PM +0400, Cyrill Gorcunov wrote:
> 
> > Hi Bruce, thinking a bit more I guess using general encode_fh is not that
> > convenient since it operates with dentries while our fdinfo output deals
> > with inodes. Thus I should either provide some new encode_fh variant
> > which would deal with inodes directly without "parents". Which doesn't
> > look for me anyhow better than the new export_encode_inode_fh helper.
> 
> Huh?  You do have dentries, for crying out loud...

Oww...  You are using it for idiotify and the rest of that pile of
garbage?  What a mess...

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Al Viro]

On Thu, Aug 16, 2012 at 05:47:06PM +0400, Pavel Emelyanov wrote:
> On 08/16/2012 05:43 PM, Al Viro wrote:
> > On Thu, Aug 16, 2012 at 04:38:14PM +0400, Cyrill Gorcunov wrote:
> > 
> >> Hi Bruce, thinking a bit more I guess using general encode_fh is not that
> >> convenient since it operates with dentries while our fdinfo output deals
> >> with inodes. Thus I should either provide some new encode_fh variant
> >> which would deal with inodes directly without "parents". Which doesn't
> >> look for me anyhow better than the new export_encode_inode_fh helper.
> > 
> > Huh?  You do have dentries, for crying out loud...
> 
> Sometimes we don't -- the inotify thing gets an inode only.
> Unlike other notifies that have dentries at hands...

What's wrong with saying "we don't support idiotify"?

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Pavel Emelyanov]

On 08/16/2012 05:50 PM, Al Viro wrote:
> On Thu, Aug 16, 2012 at 05:47:06PM +0400, Pavel Emelyanov wrote:
>> On 08/16/2012 05:43 PM, Al Viro wrote:
>>> On Thu, Aug 16, 2012 at 04:38:14PM +0400, Cyrill Gorcunov wrote:
>>>
>>>> Hi Bruce, thinking a bit more I guess using general encode_fh is not that
>>>> convenient since it operates with dentries while our fdinfo output deals
>>>> with inodes. Thus I should either provide some new encode_fh variant
>>>> which would deal with inodes directly without "parents". Which doesn't
>>>> look for me anyhow better than the new export_encode_inode_fh helper.
>>>
>>> Huh?  You do have dentries, for crying out loud...
>>
>> Sometimes we don't -- the inotify thing gets an inode only.
>> Unlike other notifies that have dentries at hands...
> 
> What's wrong with saying "we don't support idiotify"?

Lot's of exiting software uses them and we cannot say something like "we cannot
migrate a container with Apache running inside" :(

> .

Thanks,
Pavel

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Thu, Aug 16, 2012 at 02:50:19PM +0100, Al Viro wrote:
> On Thu, Aug 16, 2012 at 05:47:06PM +0400, Pavel Emelyanov wrote:
> > On 08/16/2012 05:43 PM, Al Viro wrote:
> > > On Thu, Aug 16, 2012 at 04:38:14PM +0400, Cyrill Gorcunov wrote:
> > > 
> > >> Hi Bruce, thinking a bit more I guess using general encode_fh is not that
> > >> convenient since it operates with dentries while our fdinfo output deals
> > >> with inodes. Thus I should either provide some new encode_fh variant
> > >> which would deal with inodes directly without "parents". Which doesn't
> > >> look for me anyhow better than the new export_encode_inode_fh helper.
> > > 
> > > Huh?  You do have dentries, for crying out loud...
> > 
> > Sometimes we don't -- the inotify thing gets an inode only.
> > Unlike other notifies that have dentries at hands...
> 
> What's wrong with saying "we don't support idiotify"?

Al, we need some way to restore inotifies after checkpoint.
At the very early versions of these patches I simply added
dentry to the inotify mark thus once inotify created we always
have a dentry to refer on in encode_fh, but I'm not sure if
this will be good design.

	Cyrill

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[James Bottomley]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1782 bytes --]

On Thu, 2012-08-16 at 17:54 +0400, Cyrill Gorcunov wrote:
> On Thu, Aug 16, 2012 at 02:50:19PM +0100, Al Viro wrote:
> > On Thu, Aug 16, 2012 at 05:47:06PM +0400, Pavel Emelyanov wrote:
> > > On 08/16/2012 05:43 PM, Al Viro wrote:
> > > > On Thu, Aug 16, 2012 at 04:38:14PM +0400, Cyrill Gorcunov wrote:
> > > > 
> > > >> Hi Bruce, thinking a bit more I guess using general encode_fh is not that
> > > >> convenient since it operates with dentries while our fdinfo output deals
> > > >> with inodes. Thus I should either provide some new encode_fh variant
> > > >> which would deal with inodes directly without "parents". Which doesn't
> > > >> look for me anyhow better than the new export_encode_inode_fh helper.
> > > > 
> > > > Huh?  You do have dentries, for crying out loud...
> > > 
> > > Sometimes we don't -- the inotify thing gets an inode only.
> > > Unlike other notifies that have dentries at hands...
> > 
> > What's wrong with saying "we don't support idiotify"?
> 
> Al, we need some way to restore inotifies after checkpoint.
> At the very early versions of these patches I simply added
> dentry to the inotify mark thus once inotify created we always
> have a dentry to refer on in encode_fh, but I'm not sure if
> this will be good design.

Actually, I was about to suggest this.  This can be done internally
within fs/notify without actually modifying the syscall interface, can't
it, since they take a path which is used to obtain the inode?  It looks
like the whole of the inotify interface could be internally recast to
use dentries instead of inodes.  Unless I've missed something obvious?

James

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Pavel Emelyanov]

On 08/16/2012 06:03 PM, James Bottomley wrote:
> On Thu, 2012-08-16 at 17:54 +0400, Cyrill Gorcunov wrote:
>> On Thu, Aug 16, 2012 at 02:50:19PM +0100, Al Viro wrote:
>>> On Thu, Aug 16, 2012 at 05:47:06PM +0400, Pavel Emelyanov wrote:
>>>> On 08/16/2012 05:43 PM, Al Viro wrote:
>>>>> On Thu, Aug 16, 2012 at 04:38:14PM +0400, Cyrill Gorcunov wrote:
>>>>>
>>>>>> Hi Bruce, thinking a bit more I guess using general encode_fh is not that
>>>>>> convenient since it operates with dentries while our fdinfo output deals
>>>>>> with inodes. Thus I should either provide some new encode_fh variant
>>>>>> which would deal with inodes directly without "parents". Which doesn't
>>>>>> look for me anyhow better than the new export_encode_inode_fh helper.
>>>>>
>>>>> Huh?  You do have dentries, for crying out loud...
>>>>
>>>> Sometimes we don't -- the inotify thing gets an inode only.
>>>> Unlike other notifies that have dentries at hands...
>>>
>>> What's wrong with saying "we don't support idiotify"?
>>
>> Al, we need some way to restore inotifies after checkpoint.
>> At the very early versions of these patches I simply added
>> dentry to the inotify mark thus once inotify created we always
>> have a dentry to refer on in encode_fh, but I'm not sure if
>> this will be good design.
> 
> Actually, I was about to suggest this.  This can be done internally
> within fs/notify without actually modifying the syscall interface, can't
> it, since they take a path which is used to obtain the inode?  It looks
> like the whole of the inotify interface could be internally recast to
> use dentries instead of inodes.  Unless I've missed something obvious?

This will change the observable by userspace behavior. Various apps inotify a
file, then rename/unlink/link it or do tricks with mounts container the file.
And it works one way if the dentry+mount reference is 0 (now) and some other
way if it's not (after the proposed change).

The dentries-related behavior is especially bad on NFS with its silly-renames
decisions based on dentry reference counters. The mount-related one is bad in
general.

I'm saying this, because we were facing such problems at approx. once-a-week
rate when we did this in OpenVZ :(

> James
> 

Thanks,
Pavel

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Thu, Aug 16, 2012 at 02:03:00PM +0000, James Bottomley wrote:
> > > What's wrong with saying "we don't support idiotify"?
> > 
> > Al, we need some way to restore inotifies after checkpoint.
> > At the very early versions of these patches I simply added
> > dentry to the inotify mark thus once inotify created we always
> > have a dentry to refer on in encode_fh, but I'm not sure if
> > this will be good design.
> 
> Actually, I was about to suggest this.  This can be done internally
> within fs/notify without actually modifying the syscall interface, can't
> it, since they take a path which is used to obtain the inode?  It looks
> like the whole of the inotify interface could be internally recast to
> use dentries instead of inodes.  Unless I've missed something obvious?

Well, after looking into do_sys_name_to_handle->exportfs_encode_fh
sequence more precisely it seems it will be easier to extend
exportfs_encode_fh to support inodes directly instead of playing
with notify code (again, if i'm not missing something too).
i'm cooking a patch to show (once it's tested i'll send it out).

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Al Viro]

On Thu, Aug 16, 2012 at 06:15:53PM +0400, Cyrill Gorcunov wrote:
> On Thu, Aug 16, 2012 at 02:03:00PM +0000, James Bottomley wrote:
> > > > What's wrong with saying "we don't support idiotify"?
> > > 
> > > Al, we need some way to restore inotifies after checkpoint.
> > > At the very early versions of these patches I simply added
> > > dentry to the inotify mark thus once inotify created we always
> > > have a dentry to refer on in encode_fh, but I'm not sure if
> > > this will be good design.
> > 
> > Actually, I was about to suggest this.  This can be done internally
> > within fs/notify without actually modifying the syscall interface, can't
> > it, since they take a path which is used to obtain the inode?  It looks
> > like the whole of the inotify interface could be internally recast to
> > use dentries instead of inodes.  Unless I've missed something obvious?
> 
> Well, after looking into do_sys_name_to_handle->exportfs_encode_fh
> sequence more precisely it seems it will be easier to extend
> exportfs_encode_fh to support inodes directly instead of playing
> with notify code (again, if i'm not missing something too).
> i'm cooking a patch to show (once it's tested i'll send it out).

Good luck doing that with e.g. VFAT...  And then there's such thing
as filesystems that don't have ->encode_fh() for a lot of very good
reasons; just try to do that on sysfs, for example.  Or on ramfs,
for that matter...  And while saying "you can't export that over
NFS" seems to work fine, idiotify-lovers will screech if you try
to ban their perversion of choice on those filesystems.

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Thu, Aug 16, 2012 at 03:41:52PM +0100, Al Viro wrote:
> On Thu, Aug 16, 2012 at 06:15:53PM +0400, Cyrill Gorcunov wrote:
> > On Thu, Aug 16, 2012 at 02:03:00PM +0000, James Bottomley wrote:
> > > > > What's wrong with saying "we don't support idiotify"?
> > > > 
> > > > Al, we need some way to restore inotifies after checkpoint.
> > > > At the very early versions of these patches I simply added
> > > > dentry to the inotify mark thus once inotify created we always
> > > > have a dentry to refer on in encode_fh, but I'm not sure if
> > > > this will be good design.
> > > 
> > > Actually, I was about to suggest this.  This can be done internally
> > > within fs/notify without actually modifying the syscall interface, can't
> > > it, since they take a path which is used to obtain the inode?  It looks
> > > like the whole of the inotify interface could be internally recast to
> > > use dentries instead of inodes.  Unless I've missed something obvious?
> > 
> > Well, after looking into do_sys_name_to_handle->exportfs_encode_fh
> > sequence more precisely it seems it will be easier to extend
> > exportfs_encode_fh to support inodes directly instead of playing
> > with notify code (again, if i'm not missing something too).
> > i'm cooking a patch to show (once it's tested i'll send it out).
> 
> Good luck doing that with e.g. VFAT...  And then there's such thing
> as filesystems that don't have ->encode_fh() for a lot of very good

Wait, Al, it seems I messed up. If some fs has no encode_fh() implemented
the default encoding with FILEID_INO32_GEN_PARENT will be used for that.

> reasons; just try to do that on sysfs, for example.  Or on ramfs,
> for that matter...  And while saying "you can't export that over
> NFS" seems to work fine, idiotify-lovers will screech if you try
> to ban their perversion of choice on those filesystems.

	Cyrill

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[J. Bruce Fields]

On Thu, Aug 16, 2012 at 06:48:35PM +0400, Cyrill Gorcunov wrote:
> On Thu, Aug 16, 2012 at 03:41:52PM +0100, Al Viro wrote:
> > On Thu, Aug 16, 2012 at 06:15:53PM +0400, Cyrill Gorcunov wrote:
> > > On Thu, Aug 16, 2012 at 02:03:00PM +0000, James Bottomley wrote:
> > > > > > What's wrong with saying "we don't support idiotify"?
> > > > > 
> > > > > Al, we need some way to restore inotifies after checkpoint.
> > > > > At the very early versions of these patches I simply added
> > > > > dentry to the inotify mark thus once inotify created we always
> > > > > have a dentry to refer on in encode_fh, but I'm not sure if
> > > > > this will be good design.
> > > > 
> > > > Actually, I was about to suggest this.  This can be done internally
> > > > within fs/notify without actually modifying the syscall interface, can't
> > > > it, since they take a path which is used to obtain the inode?  It looks
> > > > like the whole of the inotify interface could be internally recast to
> > > > use dentries instead of inodes.  Unless I've missed something obvious?
> > > 
> > > Well, after looking into do_sys_name_to_handle->exportfs_encode_fh
> > > sequence more precisely it seems it will be easier to extend
> > > exportfs_encode_fh to support inodes directly instead of playing
> > > with notify code (again, if i'm not missing something too).
> > > i'm cooking a patch to show (once it's tested i'll send it out).
> > 
> > Good luck doing that with e.g. VFAT...  And then there's such thing
> > as filesystems that don't have ->encode_fh() for a lot of very good
> 
> Wait, Al, it seems I messed up. If some fs has no encode_fh() implemented
> the default encoding with FILEID_INO32_GEN_PARENT will be used for that.

Yeah, but then try decoding it; the first two lines of exportfs_decode_fh:

	if (!nop || !nop->fh_to_dentry)
		return ERR_PTR(-ESTALE);

Fundamentally, if you're asking for something that you can use to look up an
inode on a filesystem (and that works even after the inode's diseappeared from
the inode cache), then you're asking for a filehandle.  Filesystems that
currently don't support filehandles probably lack that support for some good
reason.

--b.

> 
> > reasons; just try to do that on sysfs, for example.  Or on ramfs,
> > for that matter...  And while saying "you can't export that over
> > NFS" seems to work fine, idiotify-lovers will screech if you try
> > to ban their perversion of choice on those filesystems.
> 
> 	Cyrill

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Al Viro]

On Thu, Aug 16, 2012 at 06:48:35PM +0400, Cyrill Gorcunov wrote:

> > Good luck doing that with e.g. VFAT...  And then there's such thing
> > as filesystems that don't have ->encode_fh() for a lot of very good
> 
> Wait, Al, it seems I messed up. If some fs has no encode_fh() implemented
> the default encoding with FILEID_INO32_GEN_PARENT will be used for that.

... which doesn't work for a lot of filesystems.  Not if you want to be
able to decode the result afterwards and get something useful out of
that.  Trying to implement ->fh_to_dentry(), especially with fhandle
generated by inode alone is going to be really interesting for a bunch
of stuff...

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Thu, Aug 16, 2012 at 08:47:03AM -0400, J. Bruce Fields wrote:
> On Thu, Aug 16, 2012 at 04:38:14PM +0400, Cyrill Gorcunov wrote:
> > On Thu, Aug 16, 2012 at 10:24:48AM +0400, Cyrill Gorcunov wrote:
> > > > > > On the other hand, if you want a real filehandle then wouldn't you want
> > > > > > to e.g. call the filesystem's ->encode_fh() if necessary, as
> > > > > > exportfs_encode_fh() does?
> > > > > 
> > > > > Well, one of the problem I hit when I've been trying to use encode_fh
> > > > > is that every new implementation of encode_fh will require some size
> > > > > (even unknown) in buffer where encoded data pushed. Correct me please
> > > > > if I'm wrong. But with export_encode_inode_fh there is a small buffer
> > > > > with pretty known size needed on stack needed for printing data in
> > > > > fdinfo.
> > > > 
> > > > You can just give encode_fh a too-small data and let it fail if it's not
> > > > big enough.
> > > > 
> > > > (In practice I think everyone supports NFSv3 filehandles which have a
> > > > maximum size of 64 bytes.)
> > > 
> > > I'll think about it, thanks!
> > 
> > Hi Bruce, thinking a bit more I guess using general encode_fh is not that
> > convenient since it operates with dentries while our fdinfo output deals
> > with inodes. Thus I should either provide some new encode_fh variant
> > which would deal with inodes directly without "parents".
> 
> I can't see why that wouldn't work.

Guys, would the patch below be more-less acceptible?
In inotify I think we could pass "parent" as NULL and use general
encode engine then (ie it will look like someone called for
name_to_handle_at on inotify target).

---
fs, exportfs: Add exportfs_encode_inode_fh helper

This helpers allows to use per-sb encode_fh
functionality where inodes come in as arguments
(say the caller has no dentries to provide).

Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Al Viro <viro@ZenIV.linux.org.uk>
CC: "J. Bruce Fields" <bfields@fieldses.org>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: James Bottomley <jbottomley@parallels.com>
---
 fs/exportfs/expfs.c      |   19 ++++++++++++++-----
 include/linux/exportfs.h |    2 ++
 2 files changed, 16 insertions(+), 5 deletions(-)

Index: linux-2.6.git/fs/exportfs/expfs.c
===================================================================
--- linux-2.6.git.orig/fs/exportfs/expfs.c
+++ linux-2.6.git/fs/exportfs/expfs.c
@@ -341,10 +341,21 @@ static int export_encode_fh(struct inode
 	return type;
 }
 
+int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
+			     int *max_len, struct inode *parent)
+{
+	const struct export_operations *nop = inode->i_sb->s_export_op;
+
+	if (nop)
+		return nop->encode_fh(inode, fid->raw, max_len, parent);
+
+	return export_encode_fh(inode, fid, max_len, parent);
+}
+EXPORT_SYMBOL_GPL(exportfs_encode_inode_fh);
+
 int exportfs_encode_fh(struct dentry *dentry, struct fid *fid, int *max_len,
 		int connectable)
 {
-	const struct export_operations *nop = dentry->d_sb->s_export_op;
 	int error;
 	struct dentry *p = NULL;
 	struct inode *inode = dentry->d_inode, *parent = NULL;
@@ -357,10 +368,8 @@ int exportfs_encode_fh(struct dentry *de
 		 */
 		parent = p->d_inode;
 	}
-	if (nop->encode_fh)
-		error = nop->encode_fh(inode, fid->raw, max_len, parent);
-	else
-		error = export_encode_fh(inode, fid, max_len, parent);
+
+	error = exportfs_encode_inode_fh(inode, fid, max_len, parent);
 	dput(p);
 
 	return error;
Index: linux-2.6.git/include/linux/exportfs.h
===================================================================
--- linux-2.6.git.orig/include/linux/exportfs.h
+++ linux-2.6.git/include/linux/exportfs.h
@@ -177,6 +177,8 @@ struct export_operations {
 	int (*commit_metadata)(struct inode *inode);
 };
 
+extern int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
+				    int *max_len, struct inode *parent);
 extern int exportfs_encode_fh(struct dentry *dentry, struct fid *fid,
 	int *max_len, int connectable);
 extern struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Al Viro]

On Thu, Aug 16, 2012 at 06:57:00PM +0400, Cyrill Gorcunov wrote:

> Guys, would the patch below be more-less acceptible?
> In inotify I think we could pass "parent" as NULL and use general
> encode engine then (ie it will look like someone called for
> name_to_handle_at on inotify target).

Wait.  What the hell are you going to do with those afterwards?
Again, there's a shitload of filesystems that cannot be exported
over NFS, exactly because there's no way to implement sanely
working fhandles.  And idiotify is allowed for all of them.

You *can't* decode anything fhandle-like on e.g. sysfs.  Or procfs.
Or configfs.  Or any network filesystem (I'd argue that we should simply
ban idiotify on those, but good luck doing that).  You *can* do that
for FAT derivatives, but only if you have parent directory when creating
that sucker.

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Pavel Emelyanov]

On 08/16/2012 06:55 PM, Al Viro wrote:
> On Thu, Aug 16, 2012 at 06:48:35PM +0400, Cyrill Gorcunov wrote:
> 
>>> Good luck doing that with e.g. VFAT...  And then there's such thing
>>> as filesystems that don't have ->encode_fh() for a lot of very good
>>
>> Wait, Al, it seems I messed up. If some fs has no encode_fh() implemented
>> the default encoding with FILEID_INO32_GEN_PARENT will be used for that.
> 
> ... which doesn't work for a lot of filesystems.  Not if you want to be
> able to decode the result afterwards and get something useful out of
> that.  Trying to implement ->fh_to_dentry(), especially with fhandle
> generated by inode alone is going to be really interesting for a bunch
> of stuff...
> .
> 

Hm... Then I suppose the best we can do is -- show in a fdinfo file the inode
number, device where it is and a filehandle _iff_ provided by a filesystem.
For fanotify/dnotify -- only a path.

Thanks,
Pavel

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Thu, Aug 16, 2012 at 03:55:27PM +0100, Al Viro wrote:
> On Thu, Aug 16, 2012 at 06:48:35PM +0400, Cyrill Gorcunov wrote:
> 
> > > Good luck doing that with e.g. VFAT...  And then there's such thing
> > > as filesystems that don't have ->encode_fh() for a lot of very good
> > 
> > Wait, Al, it seems I messed up. If some fs has no encode_fh() implemented
> > the default encoding with FILEID_INO32_GEN_PARENT will be used for that.
> 
> ... which doesn't work for a lot of filesystems.  Not if you want to be
> able to decode the result afterwards and get something useful out of
> that.  Trying to implement ->fh_to_dentry(), especially with fhandle
> generated by inode alone is going to be really interesting for a bunch
> of stuff...

hmm, yup (and Bruce just pointed me to exportfs_decode_fh). So, if fs doesn't
provide encode_fh (and decode as well) but the FILEID_INO32_GEN_PARENT used
instead, we will not be able to restore such inotify later. Then I suppose
such application will be unrestorable at least for a while. Would not this
be acceptable trade off?

	Cyrill

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Thu, Aug 16, 2012 at 04:05:42PM +0100, Al Viro wrote:
> On Thu, Aug 16, 2012 at 06:57:00PM +0400, Cyrill Gorcunov wrote:
> 
> > Guys, would the patch below be more-less acceptible?
> > In inotify I think we could pass "parent" as NULL and use general
> > encode engine then (ie it will look like someone called for
> > name_to_handle_at on inotify target).
> 
> Wait.  What the hell are you going to do with those afterwards?
> Again, there's a shitload of filesystems that cannot be exported
> over NFS, exactly because there's no way to implement sanely
> working fhandles.  And idiotify is allowed for all of them.

Yes, just recognized that, Al. There will be no way to restore them
from fhandle provided via fdinfo. /me scratching the head...

> You *can't* decode anything fhandle-like on e.g. sysfs.  Or procfs.
> Or configfs.  Or any network filesystem (I'd argue that we should simply
> ban idiotify on those, but good luck doing that).  You *can* do that
> for FAT derivatives, but only if you have parent directory when creating
> that sucker.

	Cyrill

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Thu, Aug 16, 2012 at 07:06:18PM +0400, Pavel Emelyanov wrote:
> On 08/16/2012 06:55 PM, Al Viro wrote:
> > On Thu, Aug 16, 2012 at 06:48:35PM +0400, Cyrill Gorcunov wrote:
> > 
> >>> Good luck doing that with e.g. VFAT...  And then there's such thing
> >>> as filesystems that don't have ->encode_fh() for a lot of very good
> >>
> >> Wait, Al, it seems I messed up. If some fs has no encode_fh() implemented
> >> the default encoding with FILEID_INO32_GEN_PARENT will be used for that.
> > 
> > ... which doesn't work for a lot of filesystems.  Not if you want to be
> > able to decode the result afterwards and get something useful out of
> > that.  Trying to implement ->fh_to_dentry(), especially with fhandle
> > generated by inode alone is going to be really interesting for a bunch
> > of stuff...
> > .
> > 
> 
> Hm... Then I suppose the best we can do is -- show in a fdinfo file the inode
> number, device where it is and a filehandle _iff_ provided by a filesystem.
> For fanotify/dnotify -- only a path.

For notifications on mount points it's not a problem (we already do that

+		ret = seq_printf(m, "fanotify mnt_id: %8x mask: %8x ignored_mask: %8x\n",
+				 mnt->mnt_id, mark->mask, mark->ignored_mask);

printing inode number and device it's easy as well. Fetching the path is not
obvious for me since inotify carries inodes only. To generate path I would
have to obtain dentry from inode I suppose, that's what you mean?

	Cyrill

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Eric W. Biederman]

Al Viro <viro@ZenIV.linux.org.uk> writes:

> On Thu, Aug 16, 2012 at 06:15:53PM +0400, Cyrill Gorcunov wrote:
>> On Thu, Aug 16, 2012 at 02:03:00PM +0000, James Bottomley wrote:
>> > > > What's wrong with saying "we don't support idiotify"?
>> > > 
>> > > Al, we need some way to restore inotifies after checkpoint.
>> > > At the very early versions of these patches I simply added
>> > > dentry to the inotify mark thus once inotify created we always
>> > > have a dentry to refer on in encode_fh, but I'm not sure if
>> > > this will be good design.
>> > 
>> > Actually, I was about to suggest this.  This can be done internally
>> > within fs/notify without actually modifying the syscall interface, can't
>> > it, since they take a path which is used to obtain the inode?  It looks
>> > like the whole of the inotify interface could be internally recast to
>> > use dentries instead of inodes.  Unless I've missed something obvious?
>> 
>> Well, after looking into do_sys_name_to_handle->exportfs_encode_fh
>> sequence more precisely it seems it will be easier to extend
>> exportfs_encode_fh to support inodes directly instead of playing
>> with notify code (again, if i'm not missing something too).
>> i'm cooking a patch to show (once it's tested i'll send it out).
>
> Good luck doing that with e.g. VFAT...  And then there's such thing
> as filesystems that don't have ->encode_fh() for a lot of very good
> reasons; just try to do that on sysfs, for example.  Or on ramfs,
> for that matter...  And while saying "you can't export that over
> NFS" seems to work fine, idiotify-lovers will screech if you try
> to ban their perversion of choice on those filesystems.

For whatever it is worth inotify does not currently work on sysfs or
procfs or any other filesystem that looks like a network filesystem and
whose modifications don't proceed through the vfs like a normal
filesystem.

Eric

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Aneesh Kumar K.V]

Cyrill Gorcunov <gorcunov@openvz.org> writes:

> To provide fsnotify object inodes being watched without
> binding to alphabetical path we need to encode them with
> exportfs help. This patch adds a helper which operates
> with plain inodes directly.

doesn't name_to_handle_at()  work for you ? It also allows to get a file
handle using file descriptor.

>
> Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
> Acked-by: Pavel Emelyanov <xemul@parallels.com>
> CC: Al Viro <viro@ZenIV.linux.org.uk>
> CC: Alexey Dobriyan <adobriyan@gmail.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: James Bottomley <jbottomley@parallels.com>
> ---
>  fs/exportfs/expfs.c      |   19 +++++++++++++++++++
>  include/linux/exportfs.h |    2 ++
>  2 files changed, 21 insertions(+)
>
> Index: linux-2.6.git/fs/exportfs/expfs.c
> ===================================================================
> --- linux-2.6.git.orig/fs/exportfs/expfs.c
> +++ linux-2.6.git/fs/exportfs/expfs.c
> @@ -302,6 +302,25 @@ out:
>  	return error;
>  }
>
> +int export_encode_inode_fh(struct inode *inode, struct fid *fid, int *max_len)
> +{
> +	int len = *max_len;
> +	int type = FILEID_INO32_GEN;
> +
> +	if (len < 2) {
> +		*max_len = 2;
> +		return 255;
> +	}
> +
> +	len = 2;
> +	fid->i32.ino = inode->i_ino;
> +	fid->i32.gen = inode->i_generation;
> +	*max_len = len;
> +
> +	return type;
> +}
> +EXPORT_SYMBOL_GPL(export_encode_inode_fh);
> +


If you are looking at getting file handle, that may not be
sufficient. Some file system put more info in file handle (btrfs)

>  /**
>   * export_encode_fh - default export_operations->encode_fh function
>   * @inode:   the object to encode
> Index: linux-2.6.git/include/linux/exportfs.h
> ===================================================================
> --- linux-2.6.git.orig/include/linux/exportfs.h
> +++ linux-2.6.git/include/linux/exportfs.h
> @@ -177,6 +177,8 @@ struct export_operations {
>  	int (*commit_metadata)(struct inode *inode);
>  };
>
> +extern int export_encode_inode_fh(struct inode *inode, struct fid *fid, int *max_len);
> +
>  extern int exportfs_encode_fh(struct dentry *dentry, struct fid *fid,
>  	int *max_len, int connectable);
>  extern struct dentry *exportfs_decode_fh(struct vfsmount *mnt, struct fid *fid,
>

-aneesh

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Mon, Aug 20, 2012 at 07:49:23PM +0530, Aneesh Kumar K.V wrote:
> Cyrill Gorcunov <gorcunov@openvz.org> writes:
> 
> > To provide fsnotify object inodes being watched without
> > binding to alphabetical path we need to encode them with
> > exportfs help. This patch adds a helper which operates
> > with plain inodes directly.
> 
> doesn't name_to_handle_at()  work for you ? It also allows to get a file
> handle using file descriptor.

Hi, sorry for dealy. Well, the last idea is to get rid of this helper,
I've sent out an updated version where ino+dev is only printed.

	Cyrill

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[J. Bruce Fields]

On Mon, Aug 20, 2012 at 08:33:38PM +0400, Cyrill Gorcunov wrote:
> On Mon, Aug 20, 2012 at 07:49:23PM +0530, Aneesh Kumar K.V wrote:
> > Cyrill Gorcunov <gorcunov@openvz.org> writes:
> > 
> > > To provide fsnotify object inodes being watched without
> > > binding to alphabetical path we need to encode them with
> > > exportfs help. This patch adds a helper which operates
> > > with plain inodes directly.
> > 
> > doesn't name_to_handle_at()  work for you ? It also allows to get a file
> > handle using file descriptor.
> 
> Hi, sorry for dealy. Well, the last idea is to get rid of this helper,
> I've sent out an updated version where ino+dev is only printed.

I don't understand how ino and dev are useful to you, though, if you're
still hoping to be able to look up inodes using this information later
on.

--b.

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Mon, Aug 20, 2012 at 02:32:25PM -0400, J. Bruce Fields wrote:
> On Mon, Aug 20, 2012 at 08:33:38PM +0400, Cyrill Gorcunov wrote:
> > On Mon, Aug 20, 2012 at 07:49:23PM +0530, Aneesh Kumar K.V wrote:
> > > Cyrill Gorcunov <gorcunov@openvz.org> writes:
> > > 
> > > > To provide fsnotify object inodes being watched without
> > > > binding to alphabetical path we need to encode them with
> > > > exportfs help. This patch adds a helper which operates
> > > > with plain inodes directly.
> > > 
> > > doesn't name_to_handle_at()  work for you ? It also allows to get a file
> > > handle using file descriptor.
> > 
> > Hi, sorry for dealy. Well, the last idea is to get rid of this helper,
> > I've sent out an updated version where ino+dev is only printed.
> 
> I don't understand how ino and dev are useful to you, though, if you're
> still hoping to be able to look up inodes using this information later
> on.

Hi Bruce, I believe having ino+dev is better than nothing. Otherwise we
simply have no clue which targets are bound to inotify mark. Sometime
(!) we can try to generate fhandle in userspace from this ino+dev bundle
and then open the target file.

	Cyrill

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[J. Bruce Fields]

On Mon, Aug 20, 2012 at 11:06:06PM +0400, Cyrill Gorcunov wrote:
> On Mon, Aug 20, 2012 at 02:32:25PM -0400, J. Bruce Fields wrote:
> > On Mon, Aug 20, 2012 at 08:33:38PM +0400, Cyrill Gorcunov wrote:
> > > On Mon, Aug 20, 2012 at 07:49:23PM +0530, Aneesh Kumar K.V wrote:
> > > > Cyrill Gorcunov <gorcunov@openvz.org> writes:
> > > > 
> > > > > To provide fsnotify object inodes being watched without
> > > > > binding to alphabetical path we need to encode them with
> > > > > exportfs help. This patch adds a helper which operates
> > > > > with plain inodes directly.
> > > > 
> > > > doesn't name_to_handle_at()  work for you ? It also allows to get a file
> > > > handle using file descriptor.
> > > 
> > > Hi, sorry for dealy. Well, the last idea is to get rid of this helper,
> > > I've sent out an updated version where ino+dev is only printed.
> > 
> > I don't understand how ino and dev are useful to you, though, if you're
> > still hoping to be able to look up inodes using this information later
> > on.
> 
> Hi Bruce, I believe having ino+dev is better than nothing. Otherwise we
> simply have no clue which targets are bound to inotify mark. Sometime
> (!) we can try to generate fhandle in userspace from this ino+dev bundle
> and then open the target file.

That's insufficient to generate a filehandle in general.

(Also: there's the usual inode-number aliasing problem: the inode number
could get reused by another file.  Unless you know the file is being
held open the whole time.)

--b.

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Mon, Aug 20, 2012 at 03:32:04PM -0400, J. Bruce Fields wrote:
> > > > Hi, sorry for dealy. Well, the last idea is to get rid of this helper,
> > > > I've sent out an updated version where ino+dev is only printed.
> > > 
> > > I don't understand how ino and dev are useful to you, though, if you're
> > > still hoping to be able to look up inodes using this information later
> > > on.
> > 
> > Hi Bruce, I believe having ino+dev is better than nothing. Otherwise we
> > simply have no clue which targets are bound to inotify mark. Sometime
> > (!) we can try to generate fhandle in userspace from this ino+dev bundle
> > and then open the target file.
> 
> That's insufficient to generate a filehandle in general.

That's why I said `sometime', and this is better than nothing.

> (Also: there's the usual inode-number aliasing problem: the inode number
> could get reused by another file.  Unless you know the file is being
> held open the whole time.)

For c/r session inode should exist and not get reused.

	Cyrill

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Pavel Emelyanov]

On 08/20/2012 11:32 PM, J. Bruce Fields wrote:
> On Mon, Aug 20, 2012 at 11:06:06PM +0400, Cyrill Gorcunov wrote:
>> On Mon, Aug 20, 2012 at 02:32:25PM -0400, J. Bruce Fields wrote:
>>> On Mon, Aug 20, 2012 at 08:33:38PM +0400, Cyrill Gorcunov wrote:
>>>> On Mon, Aug 20, 2012 at 07:49:23PM +0530, Aneesh Kumar K.V wrote:
>>>>> Cyrill Gorcunov <gorcunov@openvz.org> writes:
>>>>>
>>>>>> To provide fsnotify object inodes being watched without
>>>>>> binding to alphabetical path we need to encode them with
>>>>>> exportfs help. This patch adds a helper which operates
>>>>>> with plain inodes directly.
>>>>>
>>>>> doesn't name_to_handle_at()  work for you ? It also allows to get a file
>>>>> handle using file descriptor.
>>>>
>>>> Hi, sorry for dealy. Well, the last idea is to get rid of this helper,
>>>> I've sent out an updated version where ino+dev is only printed.
>>>
>>> I don't understand how ino and dev are useful to you, though, if you're
>>> still hoping to be able to look up inodes using this information later
>>> on.
>>
>> Hi Bruce, I believe having ino+dev is better than nothing. Otherwise we
>> simply have no clue which targets are bound to inotify mark. Sometime
>> (!) we can try to generate fhandle in userspace from this ino+dev bundle
>> and then open the target file.
> 
> That's insufficient to generate a filehandle in general.

Yes, sure, but for live migration having inode and device is enough and that's why.
We can use two ways of having a filesystem on the target machine in the same
state (from paths points of view) as it was on destination one:

1. copy file tree in a rsync manner
2. copy a virtual disk image file

In the 1st case we can map inode number to path easily, since we iterate over a filesystem
anyway. I agree, that rsync is not perfect for migration but still.

In the 2nd case we can generate filehandle out of an inode number only since we _do_ know
that inode will not get reused.


However, if you have some better ideas on what information about inode should be exported
to the userspace please share.

> (Also: there's the usual inode-number aliasing problem: the inode number
> could get reused by another file.  Unless you know the file is being
> held open the whole time.)
> 
> --b.
> .
> 

Thanks,
Pavel

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Aneesh Kumar K.V]

Pavel Emelyanov <xemul@parallels.com> writes:

> On 08/20/2012 11:32 PM, J. Bruce Fields wrote:
>> On Mon, Aug 20, 2012 at 11:06:06PM +0400, Cyrill Gorcunov wrote:
>>> On Mon, Aug 20, 2012 at 02:32:25PM -0400, J. Bruce Fields wrote:
>>>> On Mon, Aug 20, 2012 at 08:33:38PM +0400, Cyrill Gorcunov wrote:
>>>>> On Mon, Aug 20, 2012 at 07:49:23PM +0530, Aneesh Kumar K.V wrote:
>>>>>> Cyrill Gorcunov <gorcunov@openvz.org> writes:
>>>>>>
>>>>>>> To provide fsnotify object inodes being watched without
>>>>>>> binding to alphabetical path we need to encode them with
>>>>>>> exportfs help. This patch adds a helper which operates
>>>>>>> with plain inodes directly.
>>>>>>
>>>>>> doesn't name_to_handle_at()  work for you ? It also allows to get a file
>>>>>> handle using file descriptor.
>>>>>
>>>>> Hi, sorry for dealy. Well, the last idea is to get rid of this helper,
>>>>> I've sent out an updated version where ino+dev is only printed.
>>>>
>>>> I don't understand how ino and dev are useful to you, though, if you're
>>>> still hoping to be able to look up inodes using this information later
>>>> on.
>>>
>>> Hi Bruce, I believe having ino+dev is better than nothing. Otherwise we
>>> simply have no clue which targets are bound to inotify mark. Sometime
>>> (!) we can try to generate fhandle in userspace from this ino+dev bundle
>>> and then open the target file.
>> 
>> That's insufficient to generate a filehandle in general.
>
> Yes, sure, but for live migration having inode and device is enough and that's why.
> We can use two ways of having a filesystem on the target machine in the same
> state (from paths points of view) as it was on destination one:
>
> 1. copy file tree in a rsync manner
> 2. copy a virtual disk image file
>
> In the 1st case we can map inode number to path easily, since we iterate over a filesystem
> anyway. I agree, that rsync is not perfect for migration but still.
>
> In the 2nd case we can generate filehandle out of an inode number only since we _do_ know
> that inode will not get reused.

If you are going to to use open_by_handle, then that handle is not
sufficient right ? Or do you have open_by_inode ? as part of c/r ?

>
>
> However, if you have some better ideas on what information about inode should be exported
> to the userspace please share.
>

Why not use name_to_handle(fd,...) and open_by_handle(handle,..) ?


>> (Also: there's the usual inode-number aliasing problem: the inode number
>> could get reused by another file.  Unless you know the file is being
>> held open the whole time.)
>> 

-aneesh

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Pavel Emelyanov]

On 08/21/2012 02:42 PM, Aneesh Kumar K.V wrote:
> Pavel Emelyanov <xemul@parallels.com> writes:
> 
>> On 08/20/2012 11:32 PM, J. Bruce Fields wrote:
>>> On Mon, Aug 20, 2012 at 11:06:06PM +0400, Cyrill Gorcunov wrote:
>>>> On Mon, Aug 20, 2012 at 02:32:25PM -0400, J. Bruce Fields wrote:
>>>>> On Mon, Aug 20, 2012 at 08:33:38PM +0400, Cyrill Gorcunov wrote:
>>>>>> On Mon, Aug 20, 2012 at 07:49:23PM +0530, Aneesh Kumar K.V wrote:
>>>>>>> Cyrill Gorcunov <gorcunov@openvz.org> writes:
>>>>>>>
>>>>>>>> To provide fsnotify object inodes being watched without
>>>>>>>> binding to alphabetical path we need to encode them with
>>>>>>>> exportfs help. This patch adds a helper which operates
>>>>>>>> with plain inodes directly.
>>>>>>>
>>>>>>> doesn't name_to_handle_at()  work for you ? It also allows to get a file
>>>>>>> handle using file descriptor.
>>>>>>
>>>>>> Hi, sorry for dealy. Well, the last idea is to get rid of this helper,
>>>>>> I've sent out an updated version where ino+dev is only printed.
>>>>>
>>>>> I don't understand how ino and dev are useful to you, though, if you're
>>>>> still hoping to be able to look up inodes using this information later
>>>>> on.
>>>>
>>>> Hi Bruce, I believe having ino+dev is better than nothing. Otherwise we
>>>> simply have no clue which targets are bound to inotify mark. Sometime
>>>> (!) we can try to generate fhandle in userspace from this ino+dev bundle
>>>> and then open the target file.
>>>
>>> That's insufficient to generate a filehandle in general.
>>
>> Yes, sure, but for live migration having inode and device is enough and that's why.
>> We can use two ways of having a filesystem on the target machine in the same
>> state (from paths points of view) as it was on destination one:
>>
>> 1. copy file tree in a rsync manner
>> 2. copy a virtual disk image file
>>
>> In the 1st case we can map inode number to path easily, since we iterate over a filesystem
>> anyway. I agree, that rsync is not perfect for migration but still.
>>
>> In the 2nd case we can generate filehandle out of an inode number only since we _do_ know
>> that inode will not get reused.
> 
> If you are going to to use open_by_handle, then that handle is not
> sufficient right ? Or do you have open_by_inode ? as part of c/r ?

Why? For e.g. ext4 you can construct a handle in userspace and open by it.

>>
>> However, if you have some better ideas on what information about inode should be exported
>> to the userspace please share.
>>
> 
> Why not use name_to_handle(fd,...) and open_by_handle(handle,..) ?

Because we don't have an fd at hands by the time we need to know the handle.

> 
>>> (Also: there's the usual inode-number aliasing problem: the inode number
>>> could get reused by another file.  Unless you know the file is being
>>> held open the whole time.)
>>>
> 
> -aneesh
> 
> .
> 

Thanks,
Pavel

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Tue, Aug 21, 2012 at 02:49:47PM +0400, Pavel Emelyanov wrote:
> >>
> >> However, if you have some better ideas on what information about inode should be exported
> >> to the userspace please share.
> >>
> > 
> > Why not use name_to_handle(fd,...) and open_by_handle(handle,..) ?
> 
> Because we don't have an fd at hands by the time we need to know the handle.

Yeah, this might be not clear from patchset itself but inotify marks carry
inodes inside kernel thus it's inodes what we can use when we fetch information
about targets and put it into fdinfo output.

	Cyrill

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Pavel Emelyanov]

On 08/21/2012 02:54 PM, Cyrill Gorcunov wrote:
> On Tue, Aug 21, 2012 at 02:49:47PM +0400, Pavel Emelyanov wrote:
>>>>
>>>> However, if you have some better ideas on what information about inode should be exported
>>>> to the userspace please share.
>>>>
>>>
>>> Why not use name_to_handle(fd,...) and open_by_handle(handle,..) ?
>>
>> Because we don't have an fd at hands by the time we need to know the handle.
> 
> Yeah, this might be not clear from patchset itself but inotify marks carry
> inodes inside kernel thus it's inodes what we can use when we fetch information
> about targets and put it into fdinfo output.

Al, Bruce, Aneesh,

What if we calculate the handle at the time we do have struct path at hands (i.e.
when we create the inotify) and store it on the inotify structure purely to be 
shown later in proc. Would that be acceptable?

> 	Cyrill
> .

Thanks,
Pavel

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[J. Bruce Fields]

On Tue, Aug 21, 2012 at 02:49:47PM +0400, Pavel Emelyanov wrote:
> On 08/21/2012 02:42 PM, Aneesh Kumar K.V wrote:
> > Pavel Emelyanov <xemul@parallels.com> writes:
> > 
> >> On 08/20/2012 11:32 PM, J. Bruce Fields wrote:
> >>> On Mon, Aug 20, 2012 at 11:06:06PM +0400, Cyrill Gorcunov wrote:
> >>>> On Mon, Aug 20, 2012 at 02:32:25PM -0400, J. Bruce Fields wrote:
> >>>>> On Mon, Aug 20, 2012 at 08:33:38PM +0400, Cyrill Gorcunov wrote:
> >>>>>> On Mon, Aug 20, 2012 at 07:49:23PM +0530, Aneesh Kumar K.V wrote:
> >>>>>>> Cyrill Gorcunov <gorcunov@openvz.org> writes:
> >>>>>>>
> >>>>>>>> To provide fsnotify object inodes being watched without
> >>>>>>>> binding to alphabetical path we need to encode them with
> >>>>>>>> exportfs help. This patch adds a helper which operates
> >>>>>>>> with plain inodes directly.
> >>>>>>>
> >>>>>>> doesn't name_to_handle_at()  work for you ? It also allows to get a file
> >>>>>>> handle using file descriptor.
> >>>>>>
> >>>>>> Hi, sorry for dealy. Well, the last idea is to get rid of this helper,
> >>>>>> I've sent out an updated version where ino+dev is only printed.
> >>>>>
> >>>>> I don't understand how ino and dev are useful to you, though, if you're
> >>>>> still hoping to be able to look up inodes using this information later
> >>>>> on.
> >>>>
> >>>> Hi Bruce, I believe having ino+dev is better than nothing. Otherwise we
> >>>> simply have no clue which targets are bound to inotify mark. Sometime
> >>>> (!) we can try to generate fhandle in userspace from this ino+dev bundle
> >>>> and then open the target file.
> >>>
> >>> That's insufficient to generate a filehandle in general.
> >>
> >> Yes, sure, but for live migration having inode and device is enough and that's why.
> >> We can use two ways of having a filesystem on the target machine in the same
> >> state (from paths points of view) as it was on destination one:
> >>
> >> 1. copy file tree in a rsync manner
> >> 2. copy a virtual disk image file
> >>
> >> In the 1st case we can map inode number to path easily, since we iterate over a filesystem

OK.  Then you don't care about unlinked files?

If the filesystem's frozen by the time you get here, I suppose you could
also just use paths?

> >> anyway. I agree, that rsync is not perfect for migration but still.
> >>
> >> In the 2nd case we can generate filehandle out of an inode number only since we _do_ know
> >> that inode will not get reused.
> > 
> > If you are going to to use open_by_handle, then that handle is not
> > sufficient right ? Or do you have open_by_inode ? as part of c/r ?
> 
> Why? For e.g. ext4 you can construct a handle in userspace and open by it.

If it's a real filehandle you want, in general you don't want to
construct it in userspace--depending on the filesystem it may require
filesystem-specific knowledge.

--b.

> 
> >>
> >> However, if you have some better ideas on what information about inode should be exported
> >> to the userspace please share.
> >>
> > 
> > Why not use name_to_handle(fd,...) and open_by_handle(handle,..) ?
> 
> Because we don't have an fd at hands by the time we need to know the handle.
> 
> > 
> >>> (Also: there's the usual inode-number aliasing problem: the inode number
> >>> could get reused by another file.  Unless you know the file is being
> >>> held open the whole time.)
> >>>
> > 
> > -aneesh
> > 
> > .
> > 
> 
> Thanks,
> Pavel

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[J. Bruce Fields]

On Tue, Aug 21, 2012 at 03:09:05PM +0400, Pavel Emelyanov wrote:
> On 08/21/2012 02:54 PM, Cyrill Gorcunov wrote:
> > On Tue, Aug 21, 2012 at 02:49:47PM +0400, Pavel Emelyanov wrote:
> >>>>
> >>>> However, if you have some better ideas on what information about inode should be exported
> >>>> to the userspace please share.
> >>>>
> >>>
> >>> Why not use name_to_handle(fd,...) and open_by_handle(handle,..) ?
> >>
> >> Because we don't have an fd at hands by the time we need to know the handle.
> > 
> > Yeah, this might be not clear from patchset itself but inotify marks carry
> > inodes inside kernel thus it's inodes what we can use when we fetch information
> > about targets and put it into fdinfo output.
> 
> Al, Bruce, Aneesh,
> 
> What if we calculate the handle at the time we do have struct path at hands (i.e.
> when we create the inotify) and store it on the inotify structure purely to be 
> shown later in proc. Would that be acceptable?

Was it the lack of a dentry that was really the problem?  I thought it
was just the fact that not all filesystems support filehandles.

--b.

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Pavel Emelyanov]

On 08/21/2012 04:11 PM, J. Bruce Fields wrote:
> On Tue, Aug 21, 2012 at 03:09:05PM +0400, Pavel Emelyanov wrote:
>> On 08/21/2012 02:54 PM, Cyrill Gorcunov wrote:
>>> On Tue, Aug 21, 2012 at 02:49:47PM +0400, Pavel Emelyanov wrote:
>>>>>>
>>>>>> However, if you have some better ideas on what information about inode should be exported
>>>>>> to the userspace please share.
>>>>>>
>>>>>
>>>>> Why not use name_to_handle(fd,...) and open_by_handle(handle,..) ?
>>>>
>>>> Because we don't have an fd at hands by the time we need to know the handle.
>>>
>>> Yeah, this might be not clear from patchset itself but inotify marks carry
>>> inodes inside kernel thus it's inodes what we can use when we fetch information
>>> about targets and put it into fdinfo output.
>>
>> Al, Bruce, Aneesh,
>>
>> What if we calculate the handle at the time we do have struct path at hands (i.e.
>> when we create the inotify) and store it on the inotify structure purely to be 
>> shown later in proc. Would that be acceptable?
> 
> Was it the lack of a dentry that was really the problem?  I thought it
> was just the fact that not all filesystems support filehandles.

Initial problem -- we don't know what is being watched by an inotify fd.

Having a dentry somewhere was the 1st attempt to solve this -- keep a path
in inotify and show it when required. It doesn't work since holding a ref on
path changes the behavior of watched inode (we cannot rename/unlink/remount
it the same way as we could before patching the kernel).

> --b.
> .
> 

Thanks,
Pavel

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Pavel Emelyanov]

On 08/21/2012 04:09 PM, J. Bruce Fields wrote:
> On Tue, Aug 21, 2012 at 02:49:47PM +0400, Pavel Emelyanov wrote:
>> On 08/21/2012 02:42 PM, Aneesh Kumar K.V wrote:
>>> Pavel Emelyanov <xemul@parallels.com> writes:
>>>
>>>> On 08/20/2012 11:32 PM, J. Bruce Fields wrote:
>>>>> On Mon, Aug 20, 2012 at 11:06:06PM +0400, Cyrill Gorcunov wrote:
>>>>>> On Mon, Aug 20, 2012 at 02:32:25PM -0400, J. Bruce Fields wrote:
>>>>>>> On Mon, Aug 20, 2012 at 08:33:38PM +0400, Cyrill Gorcunov wrote:
>>>>>>>> On Mon, Aug 20, 2012 at 07:49:23PM +0530, Aneesh Kumar K.V wrote:
>>>>>>>>> Cyrill Gorcunov <gorcunov@openvz.org> writes:
>>>>>>>>>
>>>>>>>>>> To provide fsnotify object inodes being watched without
>>>>>>>>>> binding to alphabetical path we need to encode them with
>>>>>>>>>> exportfs help. This patch adds a helper which operates
>>>>>>>>>> with plain inodes directly.
>>>>>>>>>
>>>>>>>>> doesn't name_to_handle_at()  work for you ? It also allows to get a file
>>>>>>>>> handle using file descriptor.
>>>>>>>>
>>>>>>>> Hi, sorry for dealy. Well, the last idea is to get rid of this helper,
>>>>>>>> I've sent out an updated version where ino+dev is only printed.
>>>>>>>
>>>>>>> I don't understand how ino and dev are useful to you, though, if you're
>>>>>>> still hoping to be able to look up inodes using this information later
>>>>>>> on.
>>>>>>
>>>>>> Hi Bruce, I believe having ino+dev is better than nothing. Otherwise we
>>>>>> simply have no clue which targets are bound to inotify mark. Sometime
>>>>>> (!) we can try to generate fhandle in userspace from this ino+dev bundle
>>>>>> and then open the target file.
>>>>>
>>>>> That's insufficient to generate a filehandle in general.
>>>>
>>>> Yes, sure, but for live migration having inode and device is enough and that's why.
>>>> We can use two ways of having a filesystem on the target machine in the same
>>>> state (from paths points of view) as it was on destination one:
>>>>
>>>> 1. copy file tree in a rsync manner
>>>> 2. copy a virtual disk image file
>>>>
>>>> In the 1st case we can map inode number to path easily, since we iterate over a filesystem
> 
> OK.  Then you don't care about unlinked files?

Yes. If it's unlinked file, we can emulate this on restore with opening any path,
then unlinking it. The inode number will change, yes, but in many cases this is
acceptable trade-off.

> If the filesystem's frozen by the time you get here, I suppose you could
> also just use paths?

Sure, but where can we get the path from? This is what we're trying to resolve.

>>>> anyway. I agree, that rsync is not perfect for migration but still.
>>>>
>>>> In the 2nd case we can generate filehandle out of an inode number only since we _do_ know
>>>> that inode will not get reused.
>>>
>>> If you are going to to use open_by_handle, then that handle is not
>>> sufficient right ? Or do you have open_by_inode ? as part of c/r ?
>>
>> Why? For e.g. ext4 you can construct a handle in userspace and open by it.
> 
> If it's a real filehandle you want, in general you don't want to
> construct it in userspace--depending on the filesystem it may require
> filesystem-specific knowledge.

I see.

> --b.

Thanks,
Pavel

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[J. Bruce Fields]

On Tue, Aug 21, 2012 at 04:22:31PM +0400, Pavel Emelyanov wrote:
> On 08/21/2012 04:11 PM, J. Bruce Fields wrote:
> > On Tue, Aug 21, 2012 at 03:09:05PM +0400, Pavel Emelyanov wrote:
> >> On 08/21/2012 02:54 PM, Cyrill Gorcunov wrote:
> >>> On Tue, Aug 21, 2012 at 02:49:47PM +0400, Pavel Emelyanov wrote:
> >>>>>>
> >>>>>> However, if you have some better ideas on what information about inode should be exported
> >>>>>> to the userspace please share.
> >>>>>>
> >>>>>
> >>>>> Why not use name_to_handle(fd,...) and open_by_handle(handle,..) ?
> >>>>
> >>>> Because we don't have an fd at hands by the time we need to know the handle.
> >>>
> >>> Yeah, this might be not clear from patchset itself but inotify marks carry
> >>> inodes inside kernel thus it's inodes what we can use when we fetch information
> >>> about targets and put it into fdinfo output.
> >>
> >> Al, Bruce, Aneesh,
> >>
> >> What if we calculate the handle at the time we do have struct path at hands (i.e.
> >> when we create the inotify) and store it on the inotify structure purely to be 
> >> shown later in proc. Would that be acceptable?
> > 
> > Was it the lack of a dentry that was really the problem?  I thought it
> > was just the fact that not all filesystems support filehandles.
> 
> Initial problem -- we don't know what is being watched by an inotify fd.
> 
> Having a dentry somewhere was the 1st attempt to solve this -- keep a path
> in inotify and show it when required. It doesn't work since holding a ref on
> path changes the behavior of watched inode (we cannot rename/unlink/remount
> it the same way as we could before patching the kernel).

OK.  So if you don't mind the fact that there are filesystems with
inotify support but not filehandle support, then I think generating a
filehandle early as you describe would work.  I guess it's a little more
memory per watched inode.

--b.

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Pavel Emelyanov]

>>>> Al, Bruce, Aneesh,
>>>>
>>>> What if we calculate the handle at the time we do have struct path at hands (i.e.
>>>> when we create the inotify) and store it on the inotify structure purely to be 
>>>> shown later in proc. Would that be acceptable?
>>>
>>> Was it the lack of a dentry that was really the problem?  I thought it
>>> was just the fact that not all filesystems support filehandles.
>>
>> Initial problem -- we don't know what is being watched by an inotify fd.
>>
>> Having a dentry somewhere was the 1st attempt to solve this -- keep a path
>> in inotify and show it when required. It doesn't work since holding a ref on
>> path changes the behavior of watched inode (we cannot rename/unlink/remount
>> it the same way as we could before patching the kernel).
> 
> OK.  So if you don't mind the fact that there are filesystems with
> inotify support but not filehandle support, then I think generating a
> filehandle early as you describe would work.  I guess it's a little more
> memory per watched inode.

Great! Thanks, Bruce, we'll rework the patch accordingly :)

> --b.
> .
> 

Thanks,
Pavel

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Boaz Harrosh]

On 08/21/2012 03:29 PM, J. Bruce Fields wrote:
<>

> OK.  So if you don't mind the fact that there are filesystems with
> inotify support but not filehandle support, then I think generating a
> filehandle early as you describe would work.  I guess it's a little more
> memory per watched inode.
> 


For the minority of FSs that do not have a filehandle support it should
be easy to generate a generic one, that should work 95% of the time.

Are we guaranteed that after the checkpoint restore the version of
the Kernel is the same as the one that did the checkpoint? If yes
then I don't see any problem.

> --b.


Cheers
Boaz

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Pavel Emelyanov]

On 08/21/2012 04:51 PM, Boaz Harrosh wrote:
> On 08/21/2012 03:29 PM, J. Bruce Fields wrote:
> <>
> 
>> OK.  So if you don't mind the fact that there are filesystems with
>> inotify support but not filehandle support, then I think generating a
>> filehandle early as you describe would work.  I guess it's a little more
>> memory per watched inode.
>>
> 
> 
> For the minority of FSs that do not have a filehandle support it should
> be easy to generate a generic one, that should work 95% of the time.

Yup, this is how exportfd_encode_fh currently works.

> Are we guaranteed that after the checkpoint restore the version of
> the Kernel is the same as the one that did the checkpoint? If yes
> then I don't see any problem.

Strictly speaking -- no we don't. Migration should to work across kernel
versions (from older to newer). Why kernel version matters in this case?

>> --b.
> 
> 
> Cheers
> Boaz
> 

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Boaz Harrosh]

On 08/21/2012 03:59 PM, Pavel Emelyanov wrote:

> On 08/21/2012 04:51 PM, Boaz Harrosh wrote:
>> On 08/21/2012 03:29 PM, J. Bruce Fields wrote:
>> <>
> 
> Strictly speaking -- no we don't. Migration should to work across kernel
> versions (from older to newer). Why kernel version matters in this case?


I was thinking of an FS that used to not implement export_fh but starts
to in the new version. Or some other such change in fhandles. But it's
such a far fetch I agree.

Boaz

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Al Viro]

On Tue, Aug 21, 2012 at 03:51:36PM +0300, Boaz Harrosh wrote:
> For the minority of FSs that do not have a filehandle support it should
> be easy to generate a generic one, that should work 95% of the time.

Great.  Your task, then, is to show how to do that for sysfs.  Or for nfs.
Those should be representative enough for you to get the appropriate
reality check.

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Tue, Aug 21, 2012 at 08:29:08AM -0400, J. Bruce Fields wrote:
> > Initial problem -- we don't know what is being watched by an inotify fd.
> > 
> > Having a dentry somewhere was the 1st attempt to solve this -- keep a path
> > in inotify and show it when required. It doesn't work since holding a ref on
> > path changes the behavior of watched inode (we cannot rename/unlink/remount
> > it the same way as we could before patching the kernel).
> 
> OK.  So if you don't mind the fact that there are filesystems with
> inotify support but not filehandle support, then I think generating a
> filehandle early as you describe would work.  I guess it's a little more
> memory per watched inode.

So, I thought about something like below, any comments?
---
 fs/notify/inotify/inotify.h      |    8 +++++++
 fs/notify/inotify/inotify_user.c |   41 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 44 insertions(+), 5 deletions(-)

Index: linux-2.6.git/fs/notify/inotify/inotify.h
===================================================================
--- linux-2.6.git.orig/fs/notify/inotify/inotify.h
+++ linux-2.6.git/fs/notify/inotify/inotify.h
@@ -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,
Index: linux-2.6.git/fs/notify/inotify/inotify_user.c
===================================================================
--- linux-2.6.git.orig/fs/notify/inotify/inotify_user.c
+++ linux-2.6.git/fs/notify/inotify/inotify_user.c
@@ -566,6 +566,32 @@ static void inotify_free_mark(struct fsn
 	kmem_cache_free(inotify_inode_mark_cachep, i_mark);
 }
 
+#ifdef INOTIFY_USE_FHANDLE
+static int inotify_encode_wd_fhandle(struct inotify_inode_mark *mark, struct dentry *dentry)
+{
+	struct file_handle *fhandle = (struct file_handle *)mark->fhandle;
+	int size, ret;
+
+	BUILD_BUG_ON(sizeof(mark->fhandle) <= sizeof(struct file_handle));
+
+	fhandle->handle_bytes = sizeof(mark->fhandle) - sizeof(struct file_handle);
+	size = fhandle->handle_bytes >> 2;
+
+	ret = exportfs_encode_fh(dentry, (struct fid *)fhandle->f_handle, &size,  0);
+	if ((ret == 255) || (ret == -ENOSPC))
+		return -EOVERFLOW;
+
+	fhandle->handle_type = ret;
+
+	return 0;
+}
+# else
+static int inotify_encode_wd_fhandle(struct inotify_inode_mark *mark, struct dentry *dentry)
+{
+	return 0;
+}
+#endif
+
 static int inotify_update_existing_watch(struct fsnotify_group *group,
 					 struct inode *inode,
 					 u32 arg)
@@ -621,10 +647,11 @@ static int inotify_update_existing_watch
 }
 
 static int inotify_new_watch(struct fsnotify_group *group,
-			     struct inode *inode,
+			     struct dentry *dentry,
 			     u32 arg)
 {
 	struct inotify_inode_mark *tmp_i_mark;
+	struct inode *inode = dentry->d_inode;
 	__u32 mask;
 	int ret;
 	struct idr *idr = &group->inotify_data.idr;
@@ -647,6 +674,10 @@ static int inotify_new_watch(struct fsno
 	if (atomic_read(&group->inotify_data.user->inotify_watches) >= inotify_max_user_watches)
 		goto out_err;
 
+	ret = inotify_encode_wd_fhandle(tmp_i_mark, dentry);
+	if (ret)
+		goto out_err;
+
 	ret = inotify_add_to_idr(idr, idr_lock, &group->inotify_data.last_wd,
 				 tmp_i_mark);
 	if (ret)
@@ -673,16 +704,16 @@ out_err:
 	return ret;
 }
 
-static int inotify_update_watch(struct fsnotify_group *group, struct inode *inode, u32 arg)
+static int inotify_update_watch(struct fsnotify_group *group, struct dentry *dentry, u32 arg)
 {
 	int ret = 0;
 
 retry:
 	/* try to update and existing watch with the new arg */
-	ret = inotify_update_existing_watch(group, inode, arg);
+	ret = inotify_update_existing_watch(group, dentry->d_inode, arg);
 	/* no mark present, try to add a new one */
 	if (ret == -ENOENT)
-		ret = inotify_new_watch(group, inode, arg);
+		ret = inotify_new_watch(group, dentry, arg);
 	/*
 	 * inotify_new_watch could race with another thread which did an
 	 * inotify_new_watch between the update_existing and the add watch
@@ -785,7 +816,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int,
 	group = filp->private_data;
 
 	/* create/update an inode mark */
-	ret = inotify_update_watch(group, inode, mask);
+	ret = inotify_update_watch(group, path.dentry, mask);
 	path_put(&path);
 fput_and_out:
 	fput_light(filp, fput_needed);

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Aneesh Kumar K.V]

Pavel Emelyanov <xemul@parallels.com> writes:

> On 08/21/2012 02:42 PM, Aneesh Kumar K.V wrote:
>> Pavel Emelyanov <xemul@parallels.com> writes:
>> 
>>> On 08/20/2012 11:32 PM, J. Bruce Fields wrote:
>>>> On Mon, Aug 20, 2012 at 11:06:06PM +0400, Cyrill Gorcunov wrote:
>>>>> On Mon, Aug 20, 2012 at 02:32:25PM -0400, J. Bruce Fields wrote:
>>>>>> On Mon, Aug 20, 2012 at 08:33:38PM +0400, Cyrill Gorcunov wrote:
>>>>>>> On Mon, Aug 20, 2012 at 07:49:23PM +0530, Aneesh Kumar K.V wrote:
>>>>>>>> Cyrill Gorcunov <gorcunov@openvz.org> writes:
>>>>>>>>
>>>>>>>>> To provide fsnotify object inodes being watched without
>>>>>>>>> binding to alphabetical path we need to encode them with
>>>>>>>>> exportfs help. This patch adds a helper which operates
>>>>>>>>> with plain inodes directly.
>>>>>>>>
>>>>>>>> doesn't name_to_handle_at()  work for you ? It also allows to get a file
>>>>>>>> handle using file descriptor.
>>>>>>>
>>>>>>> Hi, sorry for dealy. Well, the last idea is to get rid of this helper,
>>>>>>> I've sent out an updated version where ino+dev is only printed.
>>>>>>
>>>>>> I don't understand how ino and dev are useful to you, though, if you're
>>>>>> still hoping to be able to look up inodes using this information later
>>>>>> on.
>>>>>
>>>>> Hi Bruce, I believe having ino+dev is better than nothing. Otherwise we
>>>>> simply have no clue which targets are bound to inotify mark. Sometime
>>>>> (!) we can try to generate fhandle in userspace from this ino+dev bundle
>>>>> and then open the target file.
>>>>
>>>> That's insufficient to generate a filehandle in general.
>>>
>>> Yes, sure, but for live migration having inode and device is enough and that's why.
>>> We can use two ways of having a filesystem on the target machine in the same
>>> state (from paths points of view) as it was on destination one:
>>>
>>> 1. copy file tree in a rsync manner
>>> 2. copy a virtual disk image file
>>>
>>> In the 1st case we can map inode number to path easily, since we iterate over a filesystem
>>> anyway. I agree, that rsync is not perfect for migration but still.
>>>
>>> In the 2nd case we can generate filehandle out of an inode number only since we _do_ know
>>> that inode will not get reused.
>> 
>> If you are going to to use open_by_handle, then that handle is not
>> sufficient right ? Or do you have open_by_inode ? as part of c/r ?
>
> Why? For e.g. ext4 you can construct a handle in userspace and open by
> it.

open_by_handle use exportfs_decode_fh which use file system specific
fh_to_dentry

foe ext4 we require a generation number

		inode = get_inode(sb, fid->i32.ino, fid->i32.gen);

For brtfs

	objectid = fid->objectid;
	root_objectid = fid->root_objectid;
	generation = fid->gen;

	return btrfs_get_dentry(sb, objectid, root_objectid, generation, 1);

-aneesh

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Wed, Aug 22, 2012 at 11:19:07AM +0530, Aneesh Kumar K.V wrote:
> Pavel Emelyanov <xemul@parallels.com> writes:
> 
> > Why? For e.g. ext4 you can construct a handle in userspace and open by
> > it.
> 
> open_by_handle use exportfs_decode_fh which use file system specific
> fh_to_dentry
> 
> foe ext4 we require a generation number
> 
> 		inode = get_inode(sb, fid->i32.ino, fid->i32.gen);
> 

Hi Aneesh, yes we need i_generation but for ext3/4 it could be
fetched with ioctl as far as i see.

> For brtfs
> 
> 	objectid = fid->objectid;
> 	root_objectid = fid->root_objectid;
> 	generation = fid->gen;
> 
> 	return btrfs_get_dentry(sb, objectid, root_objectid, generation, 1);

For btrfs it become more complex. But still the last version I'm about
to send for review today (once everything get tested) will provide
fhandle carried with inotify mark _and_ inode number and device. This
information should be enough for us. After all having inode and device
should allow us to figure out the fs used on inotify target.

	Cyrill

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Marco Stornelli]

Il 23/08/2012 10:06, Cyrill Gorcunov ha scritto:
> On Wed, Aug 22, 2012 at 11:19:07AM +0530, Aneesh Kumar K.V wrote:
>> Pavel Emelyanov <xemul@parallels.com> writes:
>>
>>> Why? For e.g. ext4 you can construct a handle in userspace and open by
>>> it.
>>
>> open_by_handle use exportfs_decode_fh which use file system specific
>> fh_to_dentry
>>
>> foe ext4 we require a generation number
>>
>> 		inode = get_inode(sb, fid->i32.ino, fid->i32.gen);
>>
>
> Hi Aneesh, yes we need i_generation but for ext3/4 it could be
> fetched with ioctl as far as i see.
>
>> For brtfs
>>
>> 	objectid = fid->objectid;
>> 	root_objectid = fid->root_objectid;
>> 	generation = fid->gen;
>>
>> 	return btrfs_get_dentry(sb, objectid, root_objectid, generation, 1);
>
> For btrfs it become more complex. But still the last version I'm about
> to send for review today (once everything get tested) will provide
> fhandle carried with inotify mark _and_ inode number and device. This
> information should be enough for us. After all having inode and device
> should allow us to figure out the fs used on inotify target.
>
> 	Cyrill
> --

Sorry if it's a really stupid question but I didn't follow deeply the 
thread. How can you provide a device if the fs is mounted on "none" (ex 
tmpfs)? In this case you can't associate device <=> fs, because you 
haven't got a /dev/something.

Marco

Re: [patch 4/8] fs, exportfs: Add export_encode_inode_fh helper

[Cyrill Gorcunov]

On Thu, Aug 23, 2012 at 10:54:44AM +0200, Marco Stornelli wrote:
> >>For brtfs
> >>
> >>	objectid = fid->objectid;
> >>	root_objectid = fid->root_objectid;
> >>	generation = fid->gen;
> >>
> >>	return btrfs_get_dentry(sb, objectid, root_objectid, generation, 1);
> >
> >For btrfs it become more complex. But still the last version I'm about
> >to send for review today (once everything get tested) will provide
> >fhandle carried with inotify mark _and_ inode number and device. This
> >information should be enough for us. After all having inode and device
> >should allow us to figure out the fs used on inotify target.
> 
> Sorry if it's a really stupid question but I didn't follow deeply
> the thread. How can you provide a device if the fs is mounted on
> "none" (ex tmpfs)? In this case you can't associate device <=> fs,
> because you haven't got a /dev/something.

That's ::s_dev from superblock, together with parsing /proc/$pid/mountinfo
and /proc/mounts you can figure out anything you need.

	Cyrill