[patch 0/7] Providing additional information in fdinfo sufficient for c/r

[patch 0/7] Providing additional information in fdinfo sufficient for c/r

[Cyrill Gorcunov]

Hi guys, sorry for sending this series again but I hope this
time to gather the final opinions on the idea in general. I've
rebased the series on top of 3.7-rc5. Take a look please.
Any comments are highly appreciated.

	Cyrill

[patch 1/7] procfs: Add ability to plug in auxiliary fdinfo providers

[Cyrill Gorcunov]

[-- Attachment #1: seq-fdinfo-seq-ops-helpers-12 --]
[-- Type: text/plain, Size: 2156 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 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>
CC: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Matthew Helsley <matt.helsley@gmail.com>
CC: "J. Bruce Fields" <bfields@fieldses.org>
CC: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
---
 fs/proc/fd.c       |    2 ++
 include/linux/fs.h |    3 +++
 2 files changed, 5 insertions(+)

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
@@ -50,6 +50,8 @@ static int seq_show(struct seq_file *m,
 	if (!ret) {
                 seq_printf(m, "pos:\t%lli\nflags:\t0%o\n",
 			   (long long)file->f_pos, f_flags);
+		if (file->f_op->show_fdinfo)
+			ret = file->f_op->show_fdinfo(m, file);
 		fput(file);
 	}
 
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
@@ -1517,6 +1517,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);
@@ -1545,6 +1547,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 {

Re: [patch 1/7] procfs: Add ability to plug in auxiliary fdinfo providers

[Andrew Morton]

On Mon, 12 Nov 2012 14:14:41 +0400
Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> 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 and Pavel are proposing).
>
> ...
>
> --- linux-2.6.git.orig/include/linux/fs.h
> +++ linux-2.6.git/include/linux/fs.h
> @@ -1517,6 +1517,8 @@ struct block_device_operations;
>  #define HAVE_COMPAT_IOCTL 1
>  #define HAVE_UNLOCKED_IOCTL 1
>  
> +struct seq_file;
> +

We already have a forward declaration of seq_file in fs.h.  At line
1583(!).

This is why forward declarations should always be placed at the start
of the file, please.

--- a/include/linux/fs.h~procfs-add-ability-to-plug-in-auxiliary-fdinfo-providers-fix
+++ a/include/linux/fs.h
@@ -44,6 +44,7 @@ struct vm_area_struct;
 struct vfsmount;
 struct cred;
 struct swap_info_struct;
+struct seq_file;
 
 extern void __init inode_init(void);
 extern void __init inode_init_early(void);
@@ -1517,8 +1518,6 @@ 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);
@@ -1583,8 +1582,6 @@ struct inode_operations {
 			   umode_t create_mode, int *opened);
 } ____cacheline_aligned;
 
-struct seq_file;
-
 ssize_t rw_copy_check_uvector(int type, const struct iovec __user * uvector,
 			      unsigned long nr_segs, unsigned long fast_segs,
 			      struct iovec *fast_pointer,
_

Re: [patch 1/7] procfs: Add ability to plug in auxiliary fdinfo providers

[Cyrill Gorcunov]

On Mon, Nov 12, 2012 at 04:40:22PM -0800, Andrew Morton wrote:
> >
> > --- linux-2.6.git.orig/include/linux/fs.h
> > +++ linux-2.6.git/include/linux/fs.h
> > @@ -1517,6 +1517,8 @@ struct block_device_operations;
> >  #define HAVE_COMPAT_IOCTL 1
> >  #define HAVE_UNLOCKED_IOCTL 1
> >  
> > +struct seq_file;
> > +
> 
> We already have a forward declaration of seq_file in fs.h.  At line
> 1583(!).
> 
> This is why forward declarations should always be placed at the start
> of the file, please.

Yes, thanks a lot, Andrew!

	Cyrill

[patch 2/7] fs, exportfs: Escape nil dereference if no s_export_op present

[Cyrill Gorcunov]

[-- Attachment #1: fs-exportfs-add-null-check --]
[-- Type: text/plain, Size: 1304 bytes --]

This routine will be used to generate a file handle in fdinfo
output for inotify subsystem, where if no s_export_op present
the general export_encode_fh should be used. Thus add
a test if s_export_op present inside exportfs_encode_fh itself.

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: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Matthew Helsley <matt.helsley@gmail.com>
CC: "J. Bruce Fields" <bfields@fieldses.org>
CC: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
---
 fs/exportfs/expfs.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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
@@ -357,7 +357,7 @@ int exportfs_encode_fh(struct dentry *de
 		 */
 		parent = p->d_inode;
 	}
-	if (nop->encode_fh)
+	if (nop && nop->encode_fh)
 		error = nop->encode_fh(inode, fid->raw, max_len, parent);
 	else
 		error = export_encode_fh(inode, fid, max_len, parent);

[patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Cyrill Gorcunov]

[-- Attachment #1: fsnotify-add-fhandler --]
[-- Type: text/plain, Size: 4759 bytes --]

This file handle will be used in /proc/pid/fdinfo/fd
output, which in turn will allow to restore a watch
target after checkpoint (thus it's provided for
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: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Matthew Helsley <matt.helsley@gmail.com>
CC: "J. Bruce Fields" <bfields@fieldses.org>
CC: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
---
 fs/notify/inotify/inotify.h      |    8 +++++++
 fs/notify/inotify/inotify_user.c |   42 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 45 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,33 @@ 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;
+	fhandle->handle_bytes = size * sizeof(u32);
+
+	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 +648,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 +675,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 +705,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 +817,7 @@ SYSCALL_DEFINE3(inotify_add_watch, int,
 	group = f.file->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:
 	fdput(f);

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Andrew Morton]

On Mon, 12 Nov 2012 14:14:43 +0400
Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> This file handle will be used in /proc/pid/fdinfo/fd
> output, which in turn will allow to restore a watch
> target after checkpoint (thus it's provided for
> CONFIG_CHECKPOINT_RESTORE only).

This changelog isn't very good.

What appears to be happening is that you're borrowing exportfs's
ability to encode file handles and this is being reused to transport
inotify fd's to userspace for c/r?  Or something else - I didn't try
very hard.  Please explain better?

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

Does this mean that c/r will fail to work properly if
CONFIG_EXPORTFS=n?  If so, that should be expressed in Kconfig
dependencies?

>  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
>  };

Whoa.  This adds 128+8 bytes to the inotify_inode_mark.  That's a lot of
bloat, and there can be a lot of inotify_inode_mark's in the system?

Also, what about alignment?  That embedded `struct file_handle' will
have a 4-byte alignment and if there's anything in there which is an
8-byte quantity then some architectures (ia64?) might get upset about
the kernel-mode unaligned access.  It appears that this won't happen in
the present code, but are we future-proof?

Why did you use a __u8, anyway?  Could have done something like

	struct {
		struct file_handle fhandle;
		u8 pad[MAX_HANDLE_SZ];
	};

and got some additional type safety and less typecasting?

>  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,33 @@ 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))

Sigh.  ENOSPC means "your disk is full".  If this error ever gets back
to userspace, our poor user will go and provision more disks and then
wonder why that didn't fix anything.

> +		return -EOVERFLOW;

That doesn't seem very appropriate either, unsure.

> +	fhandle->handle_type = ret;
> +	fhandle->handle_bytes = size * sizeof(u32);
> +
> +	return 0;
> +}
>
> ...
>

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Cyrill Gorcunov]

On Mon, Nov 12, 2012 at 04:55:40PM -0800, Andrew Morton wrote:
> On Mon, 12 Nov 2012 14:14:43 +0400
> Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> 
> > This file handle will be used in /proc/pid/fdinfo/fd
> > output, which in turn will allow to restore a watch
> > target after checkpoint (thus it's provided for
> > CONFIG_CHECKPOINT_RESTORE only).
> 
> This changelog isn't very good.
> 
> What appears to be happening is that you're borrowing exportfs's
> ability to encode file handles and this is being reused to transport
> inotify fd's to userspace for c/r?  Or something else - I didn't try
> very hard.  Please explain better?

Yes, we use fhandle mechanism to encode a watch target. This allows us
to remember fhandle on dump and open() it at restore time (ie when we do
restore a target we use open_by_handle_at syscall with fhandle).

I'll update the changelog and send it.

> > --- 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
> 
> Does this mean that c/r will fail to work properly if
> CONFIG_EXPORTFS=n?  If so, that should be expressed in Kconfig
> dependencies?

Well, strictly speaking -- yes. We need exportfs to be compiled in.
But note the c/r will fail iif the task we're dumping does use
inotify. if there is no inotify usage -- then nothing will fail
even if exportfs is not compiled in. Also in our tool itself we
provide the "check" command which does verify if all features
we need are provided by the kernel. I'll think about adding
this config entry to deps. Thanks!

> >  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
> >  };
> 
> Whoa.  This adds 128+8 bytes to the inotify_inode_mark.  That's a lot of
> bloat, and there can be a lot of inotify_inode_mark's in the system?

Yes, that's why it's not turned on by default, only if is c/r turned on.
iirc I've been said that usually only about 40 bytes is used (in the tests
I met only 8 bytes). Letme re-check all things.

> Also, what about alignment?  That embedded `struct file_handle' will
> have a 4-byte alignment and if there's anything in there which is an
> 8-byte quantity then some architectures (ia64?) might get upset about
> the kernel-mode unaligned access.  It appears that this won't happen in
> the present code, but are we future-proof?
> 
> Why did you use a __u8, anyway?  Could have done something like
> 
> 	struct {
> 		struct file_handle fhandle;
> 		u8 pad[MAX_HANDLE_SZ];
> 	};
> 
> and got some additional type safety and less typecasting?

Good point! Agreed on all. I'll update, thanks!

> > +#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))
> 
> Sigh.  ENOSPC means "your disk is full".  If this error ever gets back
> to userspace, our poor user will go and provision more disks and then
> wonder why that didn't fix anything.

Hmm, this errno is returned from exportfs_encode_fh. Letme think which one
is better here. I'll update. Thanks!

> 
> > +		return -EOVERFLOW;
> 
> That doesn't seem very appropriate either, unsure.

	Cyrill

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Andrew Morton]

On Tue, 13 Nov 2012 11:20:57 +0400 Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> > >  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
> > >  };
> > 
> > Whoa.  This adds 128+8 bytes to the inotify_inode_mark.  That's a lot of
> > bloat, and there can be a lot of inotify_inode_mark's in the system?
> 
> Yes, that's why it's not turned on by default, only if is c/r turned on.
> iirc I've been said that usually only about 40 bytes is used (in the tests
> I met only 8 bytes). Letme re-check all things.

The question is, how many `struct inotify_inode_mark's are instantiated
system-wide?  Could be millions?

Dumb question: do we really need inotify_inode_mark.fhandle at all? 
What prevents us from assembling this info on demand when ->show_fdinfo() is
called?

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Cyrill Gorcunov]

On Mon, Nov 12, 2012 at 11:40:01PM -0800, Andrew Morton wrote:
> On Tue, 13 Nov 2012 11:20:57 +0400 Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> > > 
> > > Whoa.  This adds 128+8 bytes to the inotify_inode_mark.  That's a lot of
> > > bloat, and there can be a lot of inotify_inode_mark's in the system?
> > 
> > Yes, that's why it's not turned on by default, only if is c/r turned on.
> > iirc I've been said that usually only about 40 bytes is used (in the tests
> > I met only 8 bytes). Letme re-check all things.
> 
> The question is, how many `struct inotify_inode_mark's are instantiated
> system-wide?  Could be millions?

Well, hard to tell, to be fair. On my testing machine only apache has been
using inotify system as far as I remember, but for sure nothing except memory
limit the number of inotify. But I think if one running machine with millions
of inotify it's rather powerful machine with enough memory.

> Dumb question: do we really need inotify_inode_mark.fhandle at all? 
> What prevents us from assembling this info on demand when ->show_fdinfo() is
> called?

exportfs requires the dentry to be passed as an argument while inotify works
with inodes instead and at moment of show-fdinfo the target dentry might be
already deleted but inode yet present as far as I remember.

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[David Rientjes]

On Tue, 13 Nov 2012, Cyrill Gorcunov wrote:

> > The question is, how many `struct inotify_inode_mark's are instantiated
> > system-wide?  Could be millions?
> 
> Well, hard to tell, to be fair. On my testing machine only apache has been
> using inotify system as far as I remember, but for sure nothing except memory
> limit the number of inotify. But I think if one running machine with millions
> of inotify it's rather powerful machine with enough memory.
> 

Seems easy to determine if you boot with slub_nomerge on the command line 
and then read /sys/kernel/slab/inotify_inode_mark/objects.  On my system, 
that happens to be 210, but I'm sure you could come up with a realistic 
synthetic workload to make it much higher.

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Cyrill Gorcunov]

On Tue, Nov 13, 2012 at 12:19:25AM -0800, David Rientjes wrote:
> On Tue, 13 Nov 2012, Cyrill Gorcunov wrote:
> 
> > > The question is, how many `struct inotify_inode_mark's are instantiated
> > > system-wide?  Could be millions?
> > 
> > Well, hard to tell, to be fair. On my testing machine only apache has been
> > using inotify system as far as I remember, but for sure nothing except memory
> > limit the number of inotify. But I think if one running machine with millions
> > of inotify it's rather powerful machine with enough memory.
> > 
> 
> Seems easy to determine if you boot with slub_nomerge on the command line 
> and then read /sys/kernel/slab/inotify_inode_mark/objects.  On my system, 
> that happens to be 210, but I'm sure you could come up with a realistic 
> synthetic workload to make it much higher.

Which would give about 26K of additional memory if c/r get used here. Not a
big number i guess?

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Cyrill Gorcunov]

On Tue, Nov 13, 2012 at 12:37:23PM +0000, Tvrtko Ursulin wrote:
>> Which would give about 26K of additional memory if c/r get used here.
>>  Not a big number i guess?
> 
> I am pretty sure there are desktop file indexing packages which use
> inotify or fanotify which will put a mark on every single directory within
> users home.
> 
> You probably need to test this with default installs of popular desktop
> environments and realistic home directories.

I'm about to shrink the handle down to 40/64 bytes as being proposed in one
of early review cycles (i'll do that with patch on top), which should minimize
the amount of memory needed (look, it's pretty clear that if the system
uses millions of inotify watchers each inotify mark will need the fhandle here
in c/r sake, i simply see no way at moment how to escape this completely,
but if the c/r is turned off, which is by default, no additional memory
needed).

p.s.: could you please don't use the html formatted messages

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Tvrtko Ursulin]

On Tuesday 13 November 2012 18:40:36 Cyrill Gorcunov wrote:
> On Tue, Nov 13, 2012 at 12:37:23PM +0000, Tvrtko Ursulin wrote:
> >> Which would give about 26K of additional memory if c/r get used here.
> >> 
> >>  Not a big number i guess?
> > 
> > I am pretty sure there are desktop file indexing packages which use
> > inotify or fanotify which will put a mark on every single directory within
> > users home.
> > 
> > You probably need to test this with default installs of popular desktop
> > environments and realistic home directories.
> 
> I'm about to shrink the handle down to 40/64 bytes as being proposed in one
> of early review cycles (i'll do that with patch on top), which should
> minimize the amount of memory needed (look, it's pretty clear that if the
> system uses millions of inotify watchers each inotify mark will need the
> fhandle here in c/r sake, i simply see no way at moment how to escape this

Well I spotted uncertainty in this thread about how many of these structures 
will typically be instantiated at runtime which is what I tried to add to this 
discussion. I have 60k directories in my home for example...

If you don't want to use information I provided that is your choice. Because 
you are still probably doubling this structure. Give or take - I haven't 
actually bothered counting.

Perhaps there could be a different way, where you could use additional space 
only when it is actually used at runtime. But as I said, I am not following 
closely.

> completely, but if the c/r is turned off, which is by default, no
> additional memory needed).

By turned on and off you are not talking about runtime but about kernel 
compile time, right? Do you envisage distributions turning this on, like they 
do for most things?
 
> p.s.: could you please don't use the html formatted messages

I thought I sent a multi-part message which had a plain text part as well. 
Granted this is not something I usually do, but on this occasion haven't 
completed configuring my email client after a configuration loss.

Regards,

Tvrtko

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Cyrill Gorcunov]

On Tue, Nov 13, 2012 at 03:02:22PM +0000, Tvrtko Ursulin wrote:
> On Tuesday 13 November 2012 18:40:36 Cyrill Gorcunov wrote:
> > On Tue, Nov 13, 2012 at 12:37:23PM +0000, Tvrtko Ursulin wrote:
> > >> Which would give about 26K of additional memory if c/r get used here.
> > >> 
> > >>  Not a big number i guess?
> > > 
> > > I am pretty sure there are desktop file indexing packages which use
> > > inotify or fanotify which will put a mark on every single directory within
> > > users home.
> > > 
> > > You probably need to test this with default installs of popular desktop
> > > environments and realistic home directories.
> > 
> > I'm about to shrink the handle down to 40/64 bytes as being proposed in one
> > of early review cycles (i'll do that with patch on top), which should
> > minimize the amount of memory needed (look, it's pretty clear that if the
> > system uses millions of inotify watchers each inotify mark will need the
> > fhandle here in c/r sake, i simply see no way at moment how to escape this
> 
> Well I spotted uncertainty in this thread about how many of these structures 
> will typically be instantiated at runtime which is what I tried to add to this 
> discussion. I have 60k directories in my home for example...

And I appreciate it, really, thanks for info!

> If you don't want to use information I provided that is your choice. Because 
> you are still probably doubling this structure. Give or take - I haven't 
> actually bothered counting.
> 
> Perhaps there could be a different way, where you could use additional space 
> only when it is actually used at runtime. But as I said, I am not following 
> closely.

Unfortunatelly at moment I see no way how to make it in on-demand fashion.

> 
> > completely, but if the c/r is turned off, which is by default, no
> > additional memory needed).
> 
> By turned on and off you are not talking about runtime but about kernel 
> compile time, right? Do you envisage distributions turning this on, like they 
> do for most things?

yeah, compile time only. i don't expect it to be turned on by default
sometime soon but sure distro people have own opinions.

	Cyrill

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Tvrtko Ursulin]

On Tuesday 13 November 2012 19:28:46 Cyrill Gorcunov wrote:
> On Tue, Nov 13, 2012 at 03:02:22PM +0000, Tvrtko Ursulin wrote:
> > Perhaps there could be a different way, where you could use additional
> > space only when it is actually used at runtime. But as I said, I am not
> > following closely.
> 
> Unfortunatelly at moment I see no way how to make it in on-demand fashion.

You could not use a pointer and then allocate your buffers on the check point 
operation, freeing on restore?

Regards,

Tvrtko

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Cyrill Gorcunov]

On Wed, Nov 14, 2012 at 09:20:51AM +0000, Tvrtko Ursulin wrote:
> On Tuesday 13 November 2012 19:28:46 Cyrill Gorcunov wrote:
> > On Tue, Nov 13, 2012 at 03:02:22PM +0000, Tvrtko Ursulin wrote:
> > > Perhaps there could be a different way, where you could use additional
> > > space only when it is actually used at runtime. But as I said, I am not
> > > following closely.
> > 
> > Unfortunatelly at moment I see no way how to make it in on-demand fashion.
> 
> You could not use a pointer and then allocate your buffers on the check point 
> operation, freeing on restore?

The problem is not allocating the memory itself but rather the time when the
information needed (ie the dentry) is available. The only moment when we can
use dentry of the target file/directory is at inotify_new_watch, that's why
i need to compose fhandle that early. At any later point we simply have no
dentry to use.

	Cyrill

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Tvrtko Ursulin]

On Wednesday 14 November 2012 13:38:49 Cyrill Gorcunov wrote:
> On Wed, Nov 14, 2012 at 09:20:51AM +0000, Tvrtko Ursulin wrote:
> > On Tuesday 13 November 2012 19:28:46 Cyrill Gorcunov wrote:
> > > On Tue, Nov 13, 2012 at 03:02:22PM +0000, Tvrtko Ursulin wrote:
> > > > Perhaps there could be a different way, where you could use additional
> > > > space only when it is actually used at runtime. But as I said, I am
> > > > not
> > > > following closely.
> > > 
> > > Unfortunatelly at moment I see no way how to make it in on-demand
> > > fashion.
> > 
> > You could not use a pointer and then allocate your buffers on the check
> > point operation, freeing on restore?
> 
> The problem is not allocating the memory itself but rather the time when the
> information needed (ie the dentry) is available. The only moment when we
> can use dentry of the target file/directory is at inotify_new_watch, that's
> why i need to compose fhandle that early. At any later point we simply have
> no dentry to use.

But you do not fundamentally need the dentry to restore a watch, right? 
Couldn't you restore, creating a new restore path if needed, using the inode 
which is pinned anyway while the watch exists?

Regards,

Tvrtko

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Cyrill Gorcunov]

On Wed, Nov 14, 2012 at 09:50:55AM +0000, Tvrtko Ursulin wrote:
> > > You could not use a pointer and then allocate your buffers on the check
> > > point operation, freeing on restore?
> > 
> > The problem is not allocating the memory itself but rather the time when the
> > information needed (ie the dentry) is available. The only moment when we
> > can use dentry of the target file/directory is at inotify_new_watch, that's
> > why i need to compose fhandle that early. At any later point we simply have
> > no dentry to use.
> 
> But you do not fundamentally need the dentry to restore a watch, right? 

dentry only needed to encode the file handle.

> Couldn't you restore, creating a new restore path if needed, using the inode 
> which is pinned anyway while the watch exists?

plain inode is not enough as far as i can tell, iow i don't see the way
to restore path from inode solely. or there something i miss?

	Cyrill

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Tvrtko Ursulin]

On Wednesday 14 November 2012 13:58:12 Cyrill Gorcunov wrote:
> On Wed, Nov 14, 2012 at 09:50:55AM +0000, Tvrtko Ursulin wrote:
> > > > You could not use a pointer and then allocate your buffers on the
> > > > check
> > > > point operation, freeing on restore?
> > > 
> > > The problem is not allocating the memory itself but rather the time when
> > > the information needed (ie the dentry) is available. The only moment
> > > when we can use dentry of the target file/directory is at
> > > inotify_new_watch, that's why i need to compose fhandle that early. At
> > > any later point we simply have no dentry to use.
> > 
> > But you do not fundamentally need the dentry to restore a watch, right?
> 
> dentry only needed to encode the file handle.
> 
> > Couldn't you restore, creating a new restore path if needed, using the
> > inode which is pinned anyway while the watch exists?
> 
> plain inode is not enough as far as i can tell, iow i don't see the way
> to restore path from inode solely. or there something i miss?

I don't know, as I said I was not following this at all until now. Just 
throwing in ideas.

I thought, since inotify does not use the path or dentry outside the system 
call at all, perhaps you need a different entry point allowing you to restore 
the watch using the inode or something. Assuming life time of objects and 
stuff in C&R world would allow you that. Since you don't need the full path, 
just something 64 bytes long, I assumed that could be the case.
 
Regards,

Tvrtko

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Pavel Emelyanov]

On 11/14/2012 02:08 PM, Tvrtko Ursulin wrote:
> On Wednesday 14 November 2012 13:58:12 Cyrill Gorcunov wrote:
>> On Wed, Nov 14, 2012 at 09:50:55AM +0000, Tvrtko Ursulin wrote:
>>>>> You could not use a pointer and then allocate your buffers on the
>>>>> check
>>>>> point operation, freeing on restore?
>>>>
>>>> The problem is not allocating the memory itself but rather the time when
>>>> the information needed (ie the dentry) is available. The only moment
>>>> when we can use dentry of the target file/directory is at
>>>> inotify_new_watch, that's why i need to compose fhandle that early. At
>>>> any later point we simply have no dentry to use.
>>>
>>> But you do not fundamentally need the dentry to restore a watch, right?
>>
>> dentry only needed to encode the file handle.
>>
>>> Couldn't you restore, creating a new restore path if needed, using the
>>> inode which is pinned anyway while the watch exists?
>>
>> plain inode is not enough as far as i can tell, iow i don't see the way
>> to restore path from inode solely. or there something i miss?
> 
> I don't know, as I said I was not following this at all until now. Just 
> throwing in ideas.
> 
> I thought, since inotify does not use the path or dentry outside the system 
> call at all, perhaps you need a different entry point allowing you to restore 
> the watch using the inode or something. Assuming life time of objects and 
> stuff in C&R world would allow you that. Since you don't need the full path, 
> just something 64 bytes long, I assumed that could be the case.

Well, the kernel already has all the API we need but one -- it shows us _nothing_
about the inode being watched. And we'd appreciate any information about it. Even
the ino:dev pair would work. We propose to show the handle because we believe, that
such API is better that ino:dev. You can get the handle, call the open_by_handle_at
right at once and get much much more information about the inode with any other
API (e.g. calling fstat() will give you the ino:dev pair). Having just ino:dev
pair at hands is not that flexible.

> Regards,
> 
> Tvrtko
> 
> .
> 

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Tvrtko Ursulin]

On Wednesday 14 November 2012 14:13:41 Pavel Emelyanov wrote:
> On 11/14/2012 02:08 PM, Tvrtko Ursulin wrote:
> > On Wednesday 14 November 2012 13:58:12 Cyrill Gorcunov wrote:
> >> On Wed, Nov 14, 2012 at 09:50:55AM +0000, Tvrtko Ursulin wrote:
> >>>>> You could not use a pointer and then allocate your buffers on the
> >>>>> check
> >>>>> point operation, freeing on restore?
> >>>> 
> >>>> The problem is not allocating the memory itself but rather the time
> >>>> when
> >>>> the information needed (ie the dentry) is available. The only moment
> >>>> when we can use dentry of the target file/directory is at
> >>>> inotify_new_watch, that's why i need to compose fhandle that early. At
> >>>> any later point we simply have no dentry to use.
> >>> 
> >>> But you do not fundamentally need the dentry to restore a watch, right?
> >> 
> >> dentry only needed to encode the file handle.
> >> 
> >>> Couldn't you restore, creating a new restore path if needed, using the
> >>> inode which is pinned anyway while the watch exists?
> >> 
> >> plain inode is not enough as far as i can tell, iow i don't see the way
> >> to restore path from inode solely. or there something i miss?
> > 
> > I don't know, as I said I was not following this at all until now. Just
> > throwing in ideas.
> > 
> > I thought, since inotify does not use the path or dentry outside the
> > system
> > call at all, perhaps you need a different entry point allowing you to
> > restore the watch using the inode or something. Assuming life time of
> > objects and stuff in C&R world would allow you that. Since you don't need
> > the full path, just something 64 bytes long, I assumed that could be the
> > case.
> 
> Well, the kernel already has all the API we need but one -- it shows us
> _nothing_ about the inode being watched. And we'd appreciate any
> information about it. Even the ino:dev pair would work. We propose to show
> the handle because we believe, that such API is better that ino:dev. You
> can get the handle, call the open_by_handle_at right at once and get much
> much more information about the inode with any other API (e.g. calling
> fstat() will give you the ino:dev pair). Having just ino:dev pair at hands
> is not that flexible.

How much space does a typical file system need to encode a handle? Am I right 
that for must it is just a few bytes? (I just glanced at the code so I might 
be wrong.) In which case, could the handle buffer be allocated dynamically 
depending on the underlying filesystem? Perhaps adding a facility to query a 
filesystem about its maximum handle buffer needs? Do you think the saving 
would justify this extra work?

Regards,

Tvrtko

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Pavel Emelyanov]

On 11/14/2012 02:38 PM, Tvrtko Ursulin wrote:
> On Wednesday 14 November 2012 14:13:41 Pavel Emelyanov wrote:
>> On 11/14/2012 02:08 PM, Tvrtko Ursulin wrote:
>>> On Wednesday 14 November 2012 13:58:12 Cyrill Gorcunov wrote:
>>>> On Wed, Nov 14, 2012 at 09:50:55AM +0000, Tvrtko Ursulin wrote:
>>>>>>> You could not use a pointer and then allocate your buffers on the
>>>>>>> check
>>>>>>> point operation, freeing on restore?
>>>>>>
>>>>>> The problem is not allocating the memory itself but rather the time
>>>>>> when
>>>>>> the information needed (ie the dentry) is available. The only moment
>>>>>> when we can use dentry of the target file/directory is at
>>>>>> inotify_new_watch, that's why i need to compose fhandle that early. At
>>>>>> any later point we simply have no dentry to use.
>>>>>
>>>>> But you do not fundamentally need the dentry to restore a watch, right?
>>>>
>>>> dentry only needed to encode the file handle.
>>>>
>>>>> Couldn't you restore, creating a new restore path if needed, using the
>>>>> inode which is pinned anyway while the watch exists?
>>>>
>>>> plain inode is not enough as far as i can tell, iow i don't see the way
>>>> to restore path from inode solely. or there something i miss?
>>>
>>> I don't know, as I said I was not following this at all until now. Just
>>> throwing in ideas.
>>>
>>> I thought, since inotify does not use the path or dentry outside the
>>> system
>>> call at all, perhaps you need a different entry point allowing you to
>>> restore the watch using the inode or something. Assuming life time of
>>> objects and stuff in C&R world would allow you that. Since you don't need
>>> the full path, just something 64 bytes long, I assumed that could be the
>>> case.
>>
>> Well, the kernel already has all the API we need but one -- it shows us
>> _nothing_ about the inode being watched. And we'd appreciate any
>> information about it. Even the ino:dev pair would work. We propose to show
>> the handle because we believe, that such API is better that ino:dev. You
>> can get the handle, call the open_by_handle_at right at once and get much
>> much more information about the inode with any other API (e.g. calling
>> fstat() will give you the ino:dev pair). Having just ino:dev pair at hands
>> is not that flexible.
> 
> How much space does a typical file system need to encode a handle? Am I right 
> that for must it is just a few bytes? (I just glanced at the code so I might 
> be wrong.) In which case, could the handle buffer be allocated dynamically 
> depending on the underlying filesystem? Perhaps adding a facility to query a 
> filesystem about its maximum handle buffer needs? Do you think the saving 
> would justify this extra work?

Well, the MAX_HANDLE_SZ is taken from NFSv4 and is 128 bytes which is quite
big for inotify extension indeed. The good news is that this amount of bytes
seem to be required for the most descriptive fhandle -- with info about parent,
etc. We don't need such, we can live with shorter handle, people said that
40 bytes was enough for that.

However, your idea about determining the handle size dynamically seems promising.
As far as I can see from the code we can call for encode_fh with size equals zero
and filesystem would report back the amount of bytes it requires for a handle.

We can try going this route, what do you think?

> Regards,
> 
> Tvrtko

Thanks,
Pavel

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Tvrtko Ursulin]

On Wednesday 14 November 2012 14:46:42 Pavel Emelyanov wrote:
> On 11/14/2012 02:38 PM, Tvrtko Ursulin wrote:
> > On Wednesday 14 November 2012 14:13:41 Pavel Emelyanov wrote:
> >> On 11/14/2012 02:08 PM, Tvrtko Ursulin wrote:
> >>> On Wednesday 14 November 2012 13:58:12 Cyrill Gorcunov wrote:
> >>>> On Wed, Nov 14, 2012 at 09:50:55AM +0000, Tvrtko Ursulin wrote:
> >>>>>>> You could not use a pointer and then allocate your buffers on the
> >>>>>>> check
> >>>>>>> point operation, freeing on restore?
> >>>>>> 
> >>>>>> The problem is not allocating the memory itself but rather the time
> >>>>>> when
> >>>>>> the information needed (ie the dentry) is available. The only moment
> >>>>>> when we can use dentry of the target file/directory is at
> >>>>>> inotify_new_watch, that's why i need to compose fhandle that early.
> >>>>>> At
> >>>>>> any later point we simply have no dentry to use.
> >>>>> 
> >>>>> But you do not fundamentally need the dentry to restore a watch,
> >>>>> right?
> >>>> 
> >>>> dentry only needed to encode the file handle.
> >>>> 
> >>>>> Couldn't you restore, creating a new restore path if needed, using the
> >>>>> inode which is pinned anyway while the watch exists?
> >>>> 
> >>>> plain inode is not enough as far as i can tell, iow i don't see the way
> >>>> to restore path from inode solely. or there something i miss?
> >>> 
> >>> I don't know, as I said I was not following this at all until now. Just
> >>> throwing in ideas.
> >>> 
> >>> I thought, since inotify does not use the path or dentry outside the
> >>> system
> >>> call at all, perhaps you need a different entry point allowing you to
> >>> restore the watch using the inode or something. Assuming life time of
> >>> objects and stuff in C&R world would allow you that. Since you don't
> >>> need
> >>> the full path, just something 64 bytes long, I assumed that could be the
> >>> case.
> >> 
> >> Well, the kernel already has all the API we need but one -- it shows us
> >> _nothing_ about the inode being watched. And we'd appreciate any
> >> information about it. Even the ino:dev pair would work. We propose to
> >> show
> >> the handle because we believe, that such API is better that ino:dev. You
> >> can get the handle, call the open_by_handle_at right at once and get much
> >> much more information about the inode with any other API (e.g. calling
> >> fstat() will give you the ino:dev pair). Having just ino:dev pair at
> >> hands
> >> is not that flexible.
> > 
> > How much space does a typical file system need to encode a handle? Am I
> > right that for must it is just a few bytes? (I just glanced at the code
> > so I might be wrong.) In which case, could the handle buffer be allocated
> > dynamically depending on the underlying filesystem? Perhaps adding a
> > facility to query a filesystem about its maximum handle buffer needs? Do
> > you think the saving would justify this extra work?
> 
> Well, the MAX_HANDLE_SZ is taken from NFSv4 and is 128 bytes which is quite
> big for inotify extension indeed. The good news is that this amount of bytes
> seem to be required for the most descriptive fhandle -- with info about
> parent, etc. We don't need such, we can live with shorter handle, people
> said that 40 bytes was enough for that.
> 
> However, your idea about determining the handle size dynamically seems
> promising. As far as I can see from the code we can call for encode_fh with
> size equals zero and filesystem would report back the amount of bytes it
> requires for a handle.
> 
> We can try going this route, what do you think?

Sounds much better since that would only add one pointer to the watch 
structure in the normal case.

Also at checkpoint time it will use only a few bytes (compared to 64) for the 
encode buffer for most filesystems. This part is probably not that important 
but still a win.

Regards,

Tvrtko

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Pavel Emelyanov]

>>> How much space does a typical file system need to encode a handle? Am I
>>> right that for must it is just a few bytes? (I just glanced at the code
>>> so I might be wrong.) In which case, could the handle buffer be allocated
>>> dynamically depending on the underlying filesystem? Perhaps adding a
>>> facility to query a filesystem about its maximum handle buffer needs? Do
>>> you think the saving would justify this extra work?
>>
>> Well, the MAX_HANDLE_SZ is taken from NFSv4 and is 128 bytes which is quite
>> big for inotify extension indeed. The good news is that this amount of bytes
>> seem to be required for the most descriptive fhandle -- with info about
>> parent, etc. We don't need such, we can live with shorter handle, people
>> said that 40 bytes was enough for that.
>>
>> However, your idea about determining the handle size dynamically seems
>> promising. As far as I can see from the code we can call for encode_fh with
>> size equals zero and filesystem would report back the amount of bytes it
>> requires for a handle.
>>
>> We can try going this route, what do you think?
> 
> Sounds much better since that would only add one pointer to the watch 
> structure in the normal case.
> 
> Also at checkpoint time it will use only a few bytes (compared to 64) for the 
> encode buffer for most filesystems. This part is probably not that important 
> but still a win.

No, the thing is -- we need to know the handle _before_ we start checkpoint.
More exactly -- at the time the inotify_add_watch is called. So the memory save
would be not that big.

> Regards,
> 
> Tvrtko

Thanks,
Pavel

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Cyrill Gorcunov]

On Wed, Nov 14, 2012 at 02:56:00PM +0400, Pavel Emelyanov wrote:
> >>> How much space does a typical file system need to encode a handle? Am I
> >>> right that for must it is just a few bytes? (I just glanced at the code
> >>> so I might be wrong.) In which case, could the handle buffer be allocated
> >>> dynamically depending on the underlying filesystem? Perhaps adding a
> >>> facility to query a filesystem about its maximum handle buffer needs? Do
> >>> you think the saving would justify this extra work?
> >>
> >> Well, the MAX_HANDLE_SZ is taken from NFSv4 and is 128 bytes which is quite
> >> big for inotify extension indeed. The good news is that this amount of bytes
> >> seem to be required for the most descriptive fhandle -- with info about
> >> parent, etc. We don't need such, we can live with shorter handle, people
> >> said that 40 bytes was enough for that.
> >>
> >> However, your idea about determining the handle size dynamically seems
> >> promising. As far as I can see from the code we can call for encode_fh with
> >> size equals zero and filesystem would report back the amount of bytes it
> >> requires for a handle.
> >>
> >> We can try going this route, what do you think?
> > 
> > Sounds much better since that would only add one pointer to the watch 
> > structure in the normal case.
> > 
> > Also at checkpoint time it will use only a few bytes (compared to 64) for the 
> > encode buffer for most filesystems. This part is probably not that important 
> > but still a win.
> 
> No, the thing is -- we need to know the handle _before_ we start checkpoint.
> More exactly -- at the time the inotify_add_watch is called. So the memory save
> would be not that big.

for the worst case (NFSv4) it'll be 128 bytes + 8|4 byte pointer, but for
more common cases such as extXfs it'll be shrinked down to 8 byte handle +
8|4 byte pointer, which is a pretty good i think.

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Tvrtko Ursulin]

On Wednesday 14 November 2012 14:56:00 Pavel Emelyanov wrote:
> >>> How much space does a typical file system need to encode a handle? Am I
> >>> right that for must it is just a few bytes? (I just glanced at the code
> >>> so I might be wrong.) In which case, could the handle buffer be
> >>> allocated
> >>> dynamically depending on the underlying filesystem? Perhaps adding a
> >>> facility to query a filesystem about its maximum handle buffer needs? Do
> >>> you think the saving would justify this extra work?
> >> 
> >> Well, the MAX_HANDLE_SZ is taken from NFSv4 and is 128 bytes which is
> >> quite
> >> big for inotify extension indeed. The good news is that this amount of
> >> bytes seem to be required for the most descriptive fhandle -- with info
> >> about parent, etc. We don't need such, we can live with shorter handle,
> >> people said that 40 bytes was enough for that.
> >> 
> >> However, your idea about determining the handle size dynamically seems
> >> promising. As far as I can see from the code we can call for encode_fh
> >> with
> >> size equals zero and filesystem would report back the amount of bytes it
> >> requires for a handle.
> >> 
> >> We can try going this route, what do you think?
> > 
> > Sounds much better since that would only add one pointer to the watch
> > structure in the normal case.
> > 
> > Also at checkpoint time it will use only a few bytes (compared to 64) for
> > the encode buffer for most filesystems. This part is probably not that
> > important but still a win.
> 
> No, the thing is -- we need to know the handle _before_ we start checkpoint.
> More exactly -- at the time the inotify_add_watch is called. So the memory
> save would be not that big.

Ah yes, I forgot about that. But the saving is quite solid as Cyrill already 
wrote.

It is still a bit unfortunate you have to have handles allocated all the time 
just because C&R is compiled in. There is no way you could ask the filesystem 
to create you one on demand. What would you need? Just the superblock and 
inode, or more?

Regards,

Tvrtko

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[J. Bruce Fields]

On Wed, Nov 14, 2012 at 02:46:42PM +0400, Pavel Emelyanov wrote:
> Well, the MAX_HANDLE_SZ is taken from NFSv4 and is 128 bytes which is quite
> big for inotify extension indeed. The good news is that this amount of bytes
> seem to be required for the most descriptive fhandle

That 128-byte constant is just the protocol-defined maximum.

In practice my memory is that no existing filesystems require NFSv4 for
exports, so they all fit in the NFSv3 64-byte limit.  (But I seem to
recall the NFSv2 32-byte limit being too small in some cases.)

> -- with info about parent, etc. We don't need such, we can live with
> shorter handle, people said that 40 bytes was enough for that.
> 
> However, your idea about determining the handle size dynamically seems
> promising.  As far as I can see from the code we can call for
> encode_fh with size equals zero and filesystem would report back the
> amount of bytes it requires for a handle.
> 
> We can try going this route, what do you think?

I still don't understand why you need a dentry to get the filehandle.
The current api may ask for one, but it shouldn't really be necessary
(assuming you don't want parent directory information encoded in the
filehandle, which I hope you don't).

--b.

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Cyrill Gorcunov]

On Wed, Nov 14, 2012 at 07:45:49AM -0500, J. Bruce Fields wrote:
> > 
> > We can try going this route, what do you think?
> 
> I still don't understand why you need a dentry to get the filehandle.
> The current api may ask for one, but it shouldn't really be necessary
> (assuming you don't want parent directory information encoded in the
> filehandle, which I hope you don't).

As far as I know we don't need parent encoded. So Bruce, you think
to modify exportfs instead to work with inode directly?

	Cyrill

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[J. Bruce Fields]

On Wed, Nov 14, 2012 at 05:03:47PM +0400, Cyrill Gorcunov wrote:
> On Wed, Nov 14, 2012 at 07:45:49AM -0500, J. Bruce Fields wrote:
> > > 
> > > We can try going this route, what do you think?
> > 
> > I still don't understand why you need a dentry to get the filehandle.
> > The current api may ask for one, but it shouldn't really be necessary
> > (assuming you don't want parent directory information encoded in the
> > filehandle, which I hope you don't).
> 
> As far as I know we don't need parent encoded. So Bruce, you think
> to modify exportfs instead to work with inode directly?

Looks like the filesystem encode_fh method just takes inodes (with the
parent inode allowed to be NULL), so all you'd need would be a version
of exportfs_encode_fh that took an inode.

(Worst case, if that didn't work, you could fake up a dentry with
something like d_obtain_alias, but better not to if it's not necessary.)

--b.

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Cyrill Gorcunov]

On Wed, Nov 14, 2012 at 08:26:22AM -0500, J. Bruce Fields wrote:
> On Wed, Nov 14, 2012 at 05:03:47PM +0400, Cyrill Gorcunov wrote:
> > On Wed, Nov 14, 2012 at 07:45:49AM -0500, J. Bruce Fields wrote:
> > > > 
> > > > We can try going this route, what do you think?
> > > 
> > > I still don't understand why you need a dentry to get the filehandle.
> > > The current api may ask for one, but it shouldn't really be necessary
> > > (assuming you don't want parent directory information encoded in the
> > > filehandle, which I hope you don't).
> > 
> > As far as I know we don't need parent encoded. So Bruce, you think
> > to modify exportfs instead to work with inode directly?
> 
> Looks like the filesystem encode_fh method just takes inodes (with the
> parent inode allowed to be NULL), so all you'd need would be a version
> of exportfs_encode_fh that took an inode.
> 
> (Worst case, if that didn't work, you could fake up a dentry with
> something like d_obtain_alias, but better not to if it's not necessary.)

Yeah, I'll update the series, test it and show ther results. Thanks!

	Cyrill

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Andrew Morton]

On Tue, 13 Nov 2012 12:00:32 +0400
Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> > Dumb question: do we really need inotify_inode_mark.fhandle at all? 
> > What prevents us from assembling this info on demand when ->show_fdinfo() is
> > called?
> 
> exportfs requires the dentry to be passed as an argument while inotify works
> with inodes instead and at moment of show-fdinfo the target dentry might be
> already deleted but inode yet present as far as I remember.

How can the c/r restore code reestablish the inode data if the dentry
isn't there any more?

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Cyrill Gorcunov]

On Tue, Nov 13, 2012 at 02:38:08PM -0800, Andrew Morton wrote:
> On Tue, 13 Nov 2012 12:00:32 +0400
> Cyrill Gorcunov <gorcunov@openvz.org> wrote:
> 
> > > Dumb question: do we really need inotify_inode_mark.fhandle at all? 
> > > What prevents us from assembling this info on demand when ->show_fdinfo() is
> > > called?
> > 
> > exportfs requires the dentry to be passed as an argument while inotify works
> > with inodes instead and at moment of show-fdinfo the target dentry might be
> > already deleted but inode yet present as far as I remember.
> 
> How can the c/r restore code reestablish the inode data if the dentry
> isn't there any more?

By "deleted" I meant deleted from dcache, thus when we call for
open_by_handle_at with fhandle, the kernel reconstruct the path
and we simply read the /proc/self/fd/ link, and then pass this
path to inotify_add_watch.

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Pavel Emelyanov]

On 11/14/2012 10:46 AM, Cyrill Gorcunov wrote:
> On Tue, Nov 13, 2012 at 02:38:08PM -0800, Andrew Morton wrote:
>> On Tue, 13 Nov 2012 12:00:32 +0400
>> Cyrill Gorcunov <gorcunov@openvz.org> wrote:
>>
>>>> Dumb question: do we really need inotify_inode_mark.fhandle at all? 
>>>> What prevents us from assembling this info on demand when ->show_fdinfo() is
>>>> called?
>>>
>>> exportfs requires the dentry to be passed as an argument while inotify works
>>> with inodes instead and at moment of show-fdinfo the target dentry might be
>>> already deleted but inode yet present as far as I remember.
>>
>> How can the c/r restore code reestablish the inode data if the dentry
>> isn't there any more?
> 
> By "deleted" I meant deleted from dcache, thus when we call for
> open_by_handle_at with fhandle, the kernel reconstruct the path
> and we simply read the /proc/self/fd/ link, and then pass this
> path to inotify_add_watch.

No we don't do readlink as the path we'd see would be empty. Instead after
we called the open_by_handle_at, we pass the "/proc/self/fd/<fd>" _path_ itself
to inotify_add_watch. The path resolution code follows the link properly and
adds the target inode into the watch list.

> .
> 

Re: [patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Cyrill Gorcunov]

On Wed, Nov 14, 2012 at 02:10:50PM +0400, Pavel Emelyanov wrote:
> >>
> >> How can the c/r restore code reestablish the inode data if the dentry
> >> isn't there any more?
> > 
> > By "deleted" I meant deleted from dcache, thus when we call for
> > open_by_handle_at with fhandle, the kernel reconstruct the path
> > and we simply read the /proc/self/fd/ link, and then pass this
> > path to inotify_add_watch.
> 
> No we don't do readlink as the path we'd see would be empty. Instead after
> we called the open_by_handle_at, we pass the "/proc/self/fd/<fd>" _path_ itself
> to inotify_add_watch. The path resolution code follows the link properly and
> adds the target inode into the watch list.

Yeah, sorry for confusion.

[patch 4/7] fs, notify: Add procfs fdinfo helper v4

[Cyrill Gorcunov]

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

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

For inotify objects if kernel compiled with exportfs support
the output will be

 | pos:	0
 | flags:	02000000
 | inotify wd:        3 ino:             9e7e sdev:   800013 mask:  800afce ignored_mask:        0 fhandle-bytes:        8 fhandle-type:        1 f_handle: 7e9e0000640d1b6d
 | inotify wd:        2 ino:             a111 sdev:   800013 mask:  800afce ignored_mask:        0 fhandle-bytes:        8 fhandle-type:        1 f_handle: 11a1000020542153
 | inotify wd:        1 ino:            6b149 sdev:   800013 mask:  800afce ignored_mask:        0 fhandle-bytes:        8 fhandle-type:        1 f_handle: 49b1060023552153

If kernel compiled without exportfs support, the file handle
won't be provided but inode and device only.

 | pos:	0
 | flags:	02000000
 | inotify wd:        3 ino:             9e7e sdev:   800013 mask:  800afce ignored_mask:        0
 | inotify wd:        2 ino:             a111 sdev:   800013 mask:  800afce ignored_mask:        0
 | inotify wd:        1 ino:            6b149 sdev:   800013 mask:  800afce ignored_mask:        0

For fanotify the output is like

 | pos:	0
 | flags:	02
 | fanotify ino:            68f71 sdev:   800013 mask:        1 ignored_mask: 40000000
 | fanotify mnt_id:       13 mask:        1 ignored_mask: 40000000

To minimize impact on general fsnotify code the new functionality
is gathered in fs/notify/fdinfo.c file.

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: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Matthew Helsley <matt.helsley@gmail.com>
CC: "J. Bruce Fields" <bfields@fieldses.org>
CC: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
---
 fs/notify/Makefile                 |    2 
 fs/notify/fanotify/fanotify_user.c |    4 +
 fs/notify/fdinfo.c                 |  124 +++++++++++++++++++++++++++++++++++++
 fs/notify/fdinfo.h                 |   22 ++++++
 fs/notify/inotify/inotify_user.c   |    4 +
 5 files changed, 155 insertions(+), 1 deletion(-)

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
@@ -427,6 +428,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,124 @@
+#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/proc_fs.h>
+
+#include "inotify/inotify.h"
+#include "../fs/mount.h"
+
+#if defined(CONFIG_PROC_FS)
+
+#if defined(CONFIG_INOTIFY_USER) || defined(CONFIG_FANOTIFY)
+
+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) {
+		ret = seq_printf(m, "inotify wd: %8d ino: %16lx sdev: %8x "
+				 "mask: %8x ignored_mask: %8x ",
+				 inode_mark->wd, inode->i_ino,
+				 inode->i_sb->s_dev,
+				 mark->mask, mark->ignored_mask);
+#ifdef INOTIFY_USE_FHANDLE
+		if (!ret) {
+			int i;
+			struct file_handle *fhandle = (struct file_handle *)inode_mark->fhandle;
+			ret = seq_printf(m, "fhandle-bytes: %8x "
+					 "fhandle-type: %8x f_handle: ",
+					fhandle->handle_bytes,
+					fhandle->handle_type);
+
+			for (i = 0; i < fhandle->handle_bytes; i++) {
+				ret |= seq_printf(m, "%02x",
+						  (int)(unsigned char)fhandle->f_handle[i]);
+			}
+		}
+#endif
+		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) {
+		inode = igrab(mark->i.inode);
+		if (!inode)
+			goto out;
+		ret = seq_printf(m, "fanotify ino: %16lx sdev: %8x "
+				 "mask: %8x ignored_mask: %8x\n",
+				 inode->i_ino, inode->i_sb->s_dev,
+				 mark->mask, mark->ignored_mask);
+		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,

Re: [patch 4/7] fs, notify: Add procfs fdinfo helper v4

[Andrew Morton]

On Mon, 12 Nov 2012 14:14:44 +0400
Cyrill Gorcunov <gorcunov@openvz.org> wrote:

> This allow us to print out fsnotify details such as
> watchee inode, device, mask and optionally a file handle.
> 
> For inotify objects if kernel compiled with exportfs support
> the output will be
> 
>  | pos:	0
>  | flags:	02000000
>  | inotify wd:        3 ino:             9e7e sdev:   800013 mask:  800afce ignored_mask:        0 fhandle-bytes:        8 fhandle-type:        1 f_handle: 7e9e0000640d1b6d
>  | inotify wd:        2 ino:             a111 sdev:   800013 mask:  800afce ignored_mask:        0 fhandle-bytes:        8 fhandle-type:        1 f_handle: 11a1000020542153
>  | inotify wd:        1 ino:            6b149 sdev:   800013 mask:  800afce ignored_mask:        0 fhandle-bytes:        8 fhandle-type:        1 f_handle: 49b1060023552153
> 
> If kernel compiled without exportfs support, the file handle
> won't be provided but inode and device only.
> 
>  | pos:	0
>  | flags:	02000000
>  | inotify wd:        3 ino:             9e7e sdev:   800013 mask:  800afce ignored_mask:        0
>  | inotify wd:        2 ino:             a111 sdev:   800013 mask:  800afce ignored_mask:        0
>  | inotify wd:        1 ino:            6b149 sdev:   800013 mask:  800afce ignored_mask:        0
> 
> For fanotify the output is like
> 
>  | pos:	0
>  | flags:	02
>  | fanotify ino:            68f71 sdev:   800013 mask:        1 ignored_mask: 40000000
>  | fanotify mnt_id:       13 mask:        1 ignored_mask: 40000000
> 
> To minimize impact on general fsnotify code the new functionality
> is gathered in fs/notify/fdinfo.c file.
> 
>
> ...
>
> --- /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

#else	/* CONFIG_PROC_FS */
#define inotify_show_fdinfo NULL
#define fanotify_show_fdinfo NULL
#endif	/* CONFIG_PROC_FS */

> +
>
> ...
>
> @@ -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,
> @@ -427,6 +428,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,

Then remove these ifdefs.

That's if you can be bothered.  It's a bit of a party trick which
doesn't make things much clearer IMO.

Re: [patch 4/7] fs, notify: Add procfs fdinfo helper v4

[Cyrill Gorcunov]

On Mon, Nov 12, 2012 at 05:00:17PM -0800, Andrew Morton wrote:
> >  
> >  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,
> > @@ -427,6 +428,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,
> 
> Then remove these ifdefs.
> 
> That's if you can be bothered.  It's a bit of a party trick which
> doesn't make things much clearer IMO.

Sure, i'll update.

[patch 5/7] fs, eventfd: Add procfs fdinfo helper

[Cyrill Gorcunov]

[-- Attachment #1: seq-fdinfo-eventfd-7 --]
[-- Type: text/plain, Size: 1727 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>
CC: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Matthew Helsley <matt.helsley@gmail.com>
CC: "J. Bruce Fields" <bfields@fieldses.org>
CC: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.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 6/7] fs, epoll: Add procfs fdinfo helper v2

[Cyrill Gorcunov]

[-- Attachment #1: seq-fdinfo-eventpoll-8 --]
[-- Type: text/plain, Size: 2202 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 enabled: 1

[avagin@: fix for unitialized ret variable]

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: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Matthew Helsley <matt.helsley@gmail.com>
CC: "J. Bruce Fields" <bfields@fieldses.org>
CC: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
CC: Andrey Vagin <avagin@openvz.org>
---
 fs/eventpoll.c |   29 +++++++++++++++++++++++++++++
 1 file changed, 29 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,35 @@ 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 = 0;
+
+	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 enabled: %d\n",
+				 epi->ffd.fd, epi->event.events,
+				 (long long)epi->event.data,
+				 ep_is_linked(&epi->rdllink));
+		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 7/7] fdinfo: Show sigmask for signalfd fd v2

[Cyrill Gorcunov]

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

Signed-off-by: Pavel Emelyanov <xemul@parallels.com>
Signed-off-by: Cyrill Gorcunov <gorcunov@openvz.org>
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: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Matthew Helsley <matt.helsley@gmail.com>
CC: "J. Bruce Fields" <bfields@fieldses.org>
CC: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
---
 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
@@ -278,7 +302,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 7/7] fdinfo: Show sigmask for signalfd fd v2

[Andrew Morton]

On Mon, 12 Nov 2012 14:14:47 +0400
Cyrill Gorcunov <gorcunov@openvz.org> wrote:

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

hm.  Are you sure asm/signal.h is the way to get sigset_t?  "grep -l
sigset_t arch/*/include/asm/signal.h" is unpromising and
kernel/signal.c doesn't do it this way?

[patch 0/7] auxiliary fdinfo, new round

[Cyrill Gorcunov]

Hi Al, could you please share your opinion on this series? Is there something
I should fix or redesign?

Cyrill

[patch 3/7] fs, notify: Add file handle entry into inotify_inode_mark

[Cyrill Gorcunov]

[-- Attachment #1: fsnotify-add-fhandler --]
[-- Type: text/plain, Size: 4722 bytes --]

This file handle will be used in /proc/pid/fdinfo/fd
output, which in turn will allow to restore a watch
target after checkpoint.

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: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
CC: Alexey Dobriyan <adobriyan@gmail.com>
CC: Matthew Helsley <matt.helsley@gmail.com>
CC: "J. Bruce Fields" <bfields@fieldses.org>
CC: "Aneesh Kumar K.V" <aneesh.kumar@linux.vnet.ibm.com>
---
 fs/notify/inotify/inotify.h      |    8 +++++++
 fs/notify/inotify/inotify_user.c |   42 ++++++++++++++++++++++++++++++++++-----
 2 files changed, 45 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,33 @@ 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;
+	fhandle->handle_bytes = size * sizeof(u32);
+
+	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 +648,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 +675,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 +705,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 +817,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);