RFC: fsnotify - Add support for ignoring self initiated events

RFC: fsnotify - Add support for ignoring self initiated events

[Jim Lieb]

Our use case is an NFS+pNFS+9P user mode server.  We need to keep our
caches (dentry+inode) current with the underlying kernel.  To do this we
need inotify to feed filesystem events to our upcall infrastructure.  We
place a watch on each directory we have cached and any events that morph
that directory would cause invalidates and/or updates to those entries.

The current fsnotify subsystem does most of what we want but implicit in
the dnotify/inotify/fanotify interfaces is the assumption that the watcher
is an "innocent bystander" whose sole/main function is to draw/remove icons
on a window when someone else adds/removes things from a directory.  Part
of our use case is that the ganesha.nfsd server is co-resident with a CIFS
server which is also exporting the same filesystem(s) and service management
tools that modify the filesystem structure (snapshots and volume adds...).
The current inotify interface will send up all events but if both servers
are equally busy we get two bad results:

1. each server gets twice the traffic it really needs (theirs and ours).

2. there is no simple way to tell their events from ours in each event.

This patch set adds a new watch/mark flag (FS_IGNORE_ME) to fsnotify.
Setting this flag causes the watching process's pid to be stored in the
mark for the inode.  The flag is tested at event time and if set and if
the pid of the event generating process matches the stored pid, the event
is ignored, saving the overhead of allocating an event, pushing it up to
user space only to be rejected. Being in fsnotify makes it available to
any notification scheme built on fsnotify.

The IN_IGNORE_ME flag bit is added to inotify.  When set, none of the other
event flags will generate an event if the calling process generated the
event.  Given the current way that inotify_add_watch() validates the flags
argument, discovering whether the kernel supports the flag requires an
extra test (set the watch and generate an event...).

The FAN_IGNORE_ME flag bit does the same for fanotify.  fanotify in current
kernels will return an EINVAL error if this bit is set, making discovery
easier.  One performance side effect is that this flag eliminates the need and
overhead for a test of my_pid == e.pid in the event processing loop.

We chose inotify rather than the current fanotify because we need the extra
events that fanotify cannot (currently) support.  dnotify was not touched
because it is both obsolete and its api makes this extension difficult.

Please review and comment.  If it is acceptable, please ACK and merge.

Thanks

Jim Lieb, NFS Ganesha project

[PATCH 1/3] fsnotify: add support for ignoring events from self

[Jim Lieb]

Base support to allow inotify and fanotify instances to ignore
self generated events.  This allows user mode file servers to monitor
changes to their exported shares by other servers or other activities
on their server.  With FS_IGNORE_ME enabled, any f/s event triggered by
my pid is not queued into my watch/notify group.

Signed-off-by: Jim Lieb <jlieb@panasas.com>
---
 fs/notify/fsnotify.c             | 35 ++++++++++++++++++++++++-----------
 fs/notify/mark.c                 | 16 ++++++++++++++++
 include/linux/fsnotify_backend.h |  5 ++++-
 3 files changed, 44 insertions(+), 12 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 4bb21d6..88770da 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -134,6 +134,7 @@ static int send_to_group(struct inode *to_tell,
 	struct fsnotify_group *group = NULL;
 	__u32 inode_test_mask = 0;
 	__u32 vfsmount_test_mask = 0;
+	struct pid *my_pid;
 
 	if (unlikely(!inode_mark && !vfsmount_mark)) {
 		BUG();
@@ -150,23 +151,35 @@ static int send_to_group(struct inode *to_tell,
 			vfsmount_mark->ignored_mask = 0;
 	}
 
+	my_pid = get_pid(task_tgid(current));
 	/* does the inode mark tell us to do something? */
 	if (inode_mark) {
-		group = inode_mark->group;
-		inode_test_mask = (mask & ~FS_EVENT_ON_CHILD);
-		inode_test_mask &= inode_mark->mask;
-		inode_test_mask &= ~inode_mark->ignored_mask;
+		if (inode_mark->mask & FS_IGNORE_ME &&
+		    my_pid == inode_mark->ignore_pid) {
+			inode_test_mask = 0;
+		} else {
+			group = inode_mark->group;
+			inode_test_mask = (mask & ~FS_EVENT_ON_CHILD);
+			inode_test_mask &= inode_mark->mask;
+			inode_test_mask &= ~inode_mark->ignored_mask;
+		}
 	}
 
 	/* does the vfsmount_mark tell us to do something? */
 	if (vfsmount_mark) {
-		vfsmount_test_mask = (mask & ~FS_EVENT_ON_CHILD);
-		group = vfsmount_mark->group;
-		vfsmount_test_mask &= vfsmount_mark->mask;
-		vfsmount_test_mask &= ~vfsmount_mark->ignored_mask;
-		if (inode_mark)
-			vfsmount_test_mask &= ~inode_mark->ignored_mask;
+		if (vfsmount_mark->mask & FS_IGNORE_ME &&
+		    my_pid == vfsmount_mark->ignore_pid) {
+			vfsmount_test_mask = 0;
+		} else {
+			vfsmount_test_mask = (mask & ~FS_EVENT_ON_CHILD);
+			group = vfsmount_mark->group;
+			vfsmount_test_mask &= vfsmount_mark->mask;
+			vfsmount_test_mask &= ~vfsmount_mark->ignored_mask;
+			if (inode_mark)
+				vfsmount_test_mask &= ~inode_mark->ignored_mask;
+		}
 	}
+	put_pid(my_pid);
 
 	pr_debug("%s: group=%p to_tell=%p mask=%x inode_mark=%p"
 		 " inode_test_mask=%x vfsmount_mark=%p vfsmount_test_mask=%x"
@@ -300,7 +313,7 @@ static __init int fsnotify_init(void)
 {
 	int ret;
 
-	BUG_ON(hweight32(ALL_FSNOTIFY_EVENTS) != 23);
+	BUG_ON(hweight32(ALL_FSNOTIFY_EVENTS) != 24);
 
 	ret = init_srcu_struct(&fsnotify_mark_srcu);
 	if (ret)
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 923fe4a..ec08c48 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -132,6 +132,11 @@ void fsnotify_destroy_mark_locked(struct fsnotify_mark *mark,
 
 	mark->flags &= ~FSNOTIFY_MARK_FLAG_ALIVE;
 
+	if (mark->ignore_pid != NULL) {
+		put_pid(mark->ignore_pid);
+		mark->ignore_pid = NULL;
+	}
+
 	if (mark->flags & FSNOTIFY_MARK_FLAG_INODE) {
 		inode = mark->i.inode;
 		fsnotify_destroy_inode_mark(mark);
@@ -198,6 +203,17 @@ void fsnotify_set_mark_mask_locked(struct fsnotify_mark *mark, __u32 mask)
 
 	mark->mask = mask;
 
+	if (mask & FS_IGNORE_ME) {
+		if (mark->ignore_pid != NULL)
+			put_pid(mark->ignore_pid);
+		mark->ignore_pid = get_pid(task_tgid(current));
+	} else {
+		if (mark->ignore_pid != NULL) {
+			put_pid(mark->ignore_pid);
+			mark->ignore_pid = NULL;
+		}
+	}
+
 	if (mark->flags & FSNOTIFY_MARK_FLAG_INODE)
 		fsnotify_set_inode_mark_mask_locked(mark, mask);
 }
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 4b2ee8d..b219081 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -44,6 +44,8 @@
 #define FS_OPEN_PERM		0x00010000	/* open event in an permission hook */
 #define FS_ACCESS_PERM		0x00020000	/* access event in a permissions hook */
 
+#define FS_IGNORE_ME		0x00800000	/* do not send me events I caused */
+
 #define FS_EXCL_UNLINK		0x04000000	/* do not send events if object is unlinked */
 #define FS_ISDIR		0x40000000	/* event occurred against dir */
 #define FS_IN_ONESHOT		0x80000000	/* only send event once */
@@ -71,7 +73,7 @@
 			     FS_MOVED_FROM | FS_MOVED_TO | FS_CREATE | \
 			     FS_DELETE | FS_DELETE_SELF | FS_MOVE_SELF | \
 			     FS_UNMOUNT | FS_Q_OVERFLOW | FS_IN_IGNORED | \
-			     FS_OPEN_PERM | FS_ACCESS_PERM | FS_EXCL_UNLINK | \
+			     FS_OPEN_PERM | FS_ACCESS_PERM | FS_IGNORE_ME | FS_EXCL_UNLINK | \
 			     FS_ISDIR | FS_IN_ONESHOT | FS_DN_RENAME | \
 			     FS_DN_MULTISHOT | FS_EVENT_ON_CHILD)
 
@@ -281,6 +283,7 @@ struct fsnotify_mark {
 	/* we hold ref for each i_list and g_list.  also one ref for each 'thing'
 	 * in kernel that found and may be using this mark. */
 	atomic_t refcnt;		/* active things looking at this mark */
+	struct pid *ignore_pid;		/* ignore events generated by this thread group */
 	struct fsnotify_group *group;	/* group this mark is for */
 	struct list_head g_list;	/* list of marks by group->i_fsnotify_marks */
 	spinlock_t lock;		/* protect group and inode */
-- 
1.8.3.1

[PATCH 2/3] fanotify: enable support for ignoring self generated events

[Jim Lieb]

Add FAN_IGNORE_ME flag definition to be identical to FS_IGNORE_ME.

Signed-off-by: Jim Lieb <jlieb@panasas.com>
---
 fs/notify/fanotify/fanotify_user.c | 5 +++--
 include/uapi/linux/fanotify.h      | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index e44cb64..963a023 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -815,9 +815,10 @@ SYSCALL_DEFINE5(fanotify_mark, int, fanotify_fd, unsigned int, flags,
 	}
 
 #ifdef CONFIG_FANOTIFY_ACCESS_PERMISSIONS
-	if (mask & ~(FAN_ALL_EVENTS | FAN_ALL_PERM_EVENTS | FAN_EVENT_ON_CHILD))
+	if (mask & ~(FAN_ALL_EVENTS | FAN_ALL_PERM_EVENTS |
+		     FAN_IGNORE_ME | FAN_EVENT_ON_CHILD))
 #else
-	if (mask & ~(FAN_ALL_EVENTS | FAN_EVENT_ON_CHILD))
+	if (mask & ~(FAN_ALL_EVENTS | FAN_IGNORE_ME | FAN_EVENT_ON_CHILD))
 #endif
 		return -EINVAL;
 
diff --git a/include/uapi/linux/fanotify.h b/include/uapi/linux/fanotify.h
index 030508d..93e9f09 100644
--- a/include/uapi/linux/fanotify.h
+++ b/include/uapi/linux/fanotify.h
@@ -17,6 +17,7 @@
 
 #define FAN_ONDIR		0x40000000	/* event occurred against dir */
 
+#define FAN_IGNORE_ME		0x00800000	/* do not send me events I caused */
 #define FAN_EVENT_ON_CHILD	0x08000000	/* interested in child events */
 
 /* helper events */
-- 
1.8.3.1

[PATCH 3/3] inotify: enable support to ignore self generated events

[Jim Lieb]

Add IN_IGNORE_ME defined identical with FS_IGNORE_ME.

Signed-off-by: Jim Lieb <jlieb@panasas.com>
---
 fs/notify/inotify/inotify_user.c | 3 ++-
 include/linux/inotify.h          | 2 +-
 include/uapi/linux/inotify.h     | 3 ++-
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 60f954a..cf5bcf0 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -829,11 +829,12 @@ static int __init inotify_user_setup(void)
 	BUILD_BUG_ON(IN_UNMOUNT != FS_UNMOUNT);
 	BUILD_BUG_ON(IN_Q_OVERFLOW != FS_Q_OVERFLOW);
 	BUILD_BUG_ON(IN_IGNORED != FS_IN_IGNORED);
+	BUILD_BUG_ON(IN_IGNORE_ME != FS_IGNORE_ME);
 	BUILD_BUG_ON(IN_EXCL_UNLINK != FS_EXCL_UNLINK);
 	BUILD_BUG_ON(IN_ISDIR != FS_ISDIR);
 	BUILD_BUG_ON(IN_ONESHOT != FS_IN_ONESHOT);
 
-	BUG_ON(hweight32(ALL_INOTIFY_BITS) != 21);
+	BUG_ON(hweight32(ALL_INOTIFY_BITS) != 22);
 
 	inotify_inode_mark_cachep = KMEM_CACHE(inotify_inode_mark, SLAB_PANIC);
 	event_priv_cachep = KMEM_CACHE(inotify_event_private_data, SLAB_PANIC);
diff --git a/include/linux/inotify.h b/include/linux/inotify.h
index 23aede0..270bddb 100644
--- a/include/linux/inotify.h
+++ b/include/linux/inotify.h
@@ -15,7 +15,7 @@ extern struct ctl_table inotify_table[]; /* for sysctl */
 			  IN_CLOSE_NOWRITE | IN_OPEN | IN_MOVED_FROM | \
 			  IN_MOVED_TO | IN_CREATE | IN_DELETE | \
 			  IN_DELETE_SELF | IN_MOVE_SELF | IN_UNMOUNT | \
-			  IN_Q_OVERFLOW | IN_IGNORED | IN_ONLYDIR | \
+			  IN_Q_OVERFLOW | IN_IGNORED | IN_IGNORE_ME | IN_ONLYDIR | \
 			  IN_DONT_FOLLOW | IN_EXCL_UNLINK | IN_MASK_ADD | \
 			  IN_ISDIR | IN_ONESHOT)
 
diff --git a/include/uapi/linux/inotify.h b/include/uapi/linux/inotify.h
index e6bf35b..d542335 100644
--- a/include/uapi/linux/inotify.h
+++ b/include/uapi/linux/inotify.h
@@ -49,6 +49,7 @@ struct inotify_event {
 #define IN_MOVE			(IN_MOVED_FROM | IN_MOVED_TO) /* moves */
 
 /* special flags */
+#define IN_IGNORE_ME		0x00800000	/* do not send me events I caused */
 #define IN_ONLYDIR		0x01000000	/* only watch the path if it is a directory */
 #define IN_DONT_FOLLOW		0x02000000	/* don't follow a sym link */
 #define IN_EXCL_UNLINK		0x04000000	/* exclude events on unlinked objects */
@@ -64,7 +65,7 @@ struct inotify_event {
 #define IN_ALL_EVENTS	(IN_ACCESS | IN_MODIFY | IN_ATTRIB | IN_CLOSE_WRITE | \
 			 IN_CLOSE_NOWRITE | IN_OPEN | IN_MOVED_FROM | \
 			 IN_MOVED_TO | IN_DELETE | IN_CREATE | IN_DELETE_SELF | \
-			 IN_MOVE_SELF)
+			 IN_MOVE_SELF | IN_IGNORE_ME)
 
 /* Flags for sys_inotify_init1.  */
 #define IN_CLOEXEC O_CLOEXEC
-- 
1.8.3.1

Re: RFC: fsnotify - Add support for ignoring self initiated events

[J. Bruce Fields]

What sort of cache consistency does this give you?  (It's not perfect,
because you always get the notification after the event has already
happened, right?)

It looks like you're using the tgid.  I guess Ganesha runs a bunch of
threads all sharing the same tgid?  Would a server using multiple
processes instead need a different interface?

--b.

On Wed, Sep 04, 2013 at 11:30:59AM -0700, Jim Lieb wrote:
> Our use case is an NFS+pNFS+9P user mode server.  We need to keep our
> caches (dentry+inode) current with the underlying kernel.  To do this we
> need inotify to feed filesystem events to our upcall infrastructure.  We
> place a watch on each directory we have cached and any events that morph
> that directory would cause invalidates and/or updates to those entries.
> 
> The current fsnotify subsystem does most of what we want but implicit in
> the dnotify/inotify/fanotify interfaces is the assumption that the watcher
> is an "innocent bystander" whose sole/main function is to draw/remove icons
> on a window when someone else adds/removes things from a directory.  Part
> of our use case is that the ganesha.nfsd server is co-resident with a CIFS
> server which is also exporting the same filesystem(s) and service management
> tools that modify the filesystem structure (snapshots and volume adds...).
> The current inotify interface will send up all events but if both servers
> are equally busy we get two bad results:
> 
> 1. each server gets twice the traffic it really needs (theirs and ours).
> 
> 2. there is no simple way to tell their events from ours in each event.
> 
> This patch set adds a new watch/mark flag (FS_IGNORE_ME) to fsnotify.
> Setting this flag causes the watching process's pid to be stored in the
> mark for the inode.  The flag is tested at event time and if set and if
> the pid of the event generating process matches the stored pid, the event
> is ignored, saving the overhead of allocating an event, pushing it up to
> user space only to be rejected. Being in fsnotify makes it available to
> any notification scheme built on fsnotify.
> 
> The IN_IGNORE_ME flag bit is added to inotify.  When set, none of the other
> event flags will generate an event if the calling process generated the
> event.  Given the current way that inotify_add_watch() validates the flags
> argument, discovering whether the kernel supports the flag requires an
> extra test (set the watch and generate an event...).
> 
> The FAN_IGNORE_ME flag bit does the same for fanotify.  fanotify in current
> kernels will return an EINVAL error if this bit is set, making discovery
> easier.  One performance side effect is that this flag eliminates the need and
> overhead for a test of my_pid == e.pid in the event processing loop.
> 
> We chose inotify rather than the current fanotify because we need the extra
> events that fanotify cannot (currently) support.  dnotify was not touched
> because it is both obsolete and its api makes this extension difficult.
> 
> Please review and comment.  If it is acceptable, please ACK and merge.
> 
> Thanks
> 
> Jim Lieb, NFS Ganesha project
> 
> --
> 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: Re: RFC: fsnotify - Add support for ignoring self initiated events

[Jim Lieb]

On Wednesday, September 11, 2013 10:27:54 J. Bruce Fields wrote:
> What sort of cache consistency does this give you?  (It's not perfect,
> because you always get the notification after the event has already
> happened, right?)

It is certainly not atomic.  However, our (nfs-ganesha) use cases can tolerate 
some level of delay.

1. We populate an avl tree with the names from a readdir.  If another process, 
e.g. samba, adds/removes a file we want to know about it.  Readdir already has 
races in that the same thing can happen in a portion of the directory already 
scanned while we are still reading the last of it.

2. We use the avl tree for lookups.  Same thing applies here.  If the 
directory is current, we can serve the lookup from cache.  In the case of a 
lookup prior to event, the next step in the protocol would be to do something 
directy to the file which will discover the removal.  We've always had the 
problem of lookups for files that don't exist yet and we cope with retries or 
???.  All we would get here is a bit more delay.

The event will trigger an upcall into the cache inode layer to mark the 
directory as "stale".  The inotify payload gives us the name as well so we can 
do things to the tree like add/remove the entry.  In the lookup case, if we 
know the directory is stale, we can go directly to the filesystem rather than 
assume the directory is consistant.

In the end, yes there is an inconsistancy window but it does close.
> 
> It looks like you're using the tgid.  I guess Ganesha runs a bunch of
> threads all sharing the same tgid?  Would a server using multiple
> processes instead need a different interface?

Yes, it is using the tgid which, of course, implies that the server is NPTL 
based (or equiv).  It does not take into consideration the multiple processes 
with shared memory case.   The completely separate process case is covered in 
the samba+nfs-gaanesha case.  One knows what the other is doing filtered by the 
ignore flag.
> 
> --b.
> 
> On Wed, Sep 04, 2013 at 11:30:59AM -0700, Jim Lieb wrote:
> > Our use case is an NFS+pNFS+9P user mode server.  We need to keep our
> > caches (dentry+inode) current with the underlying kernel.  To do this we
> > need inotify to feed filesystem events to our upcall infrastructure.  We
> > place a watch on each directory we have cached and any events that morph
> > that directory would cause invalidates and/or updates to those entries.
> > 
> > The current fsnotify subsystem does most of what we want but implicit in
> > the dnotify/inotify/fanotify interfaces is the assumption that the watcher
> > is an "innocent bystander" whose sole/main function is to draw/remove
> > icons
> > on a window when someone else adds/removes things from a directory.  Part
> > of our use case is that the ganesha.nfsd server is co-resident with a CIFS
> > server which is also exporting the same filesystem(s) and service
> > management tools that modify the filesystem structure (snapshots and
> > volume adds...). The current inotify interface will send up all events
> > but if both servers are equally busy we get two bad results:
> > 
> > 1. each server gets twice the traffic it really needs (theirs and ours).
> > 
> > 2. there is no simple way to tell their events from ours in each event.
> > 
> > This patch set adds a new watch/mark flag (FS_IGNORE_ME) to fsnotify.
> > Setting this flag causes the watching process's pid to be stored in the
> > mark for the inode.  The flag is tested at event time and if set and if
> > the pid of the event generating process matches the stored pid, the event
> > is ignored, saving the overhead of allocating an event, pushing it up to
> > user space only to be rejected. Being in fsnotify makes it available to
> > any notification scheme built on fsnotify.
> > 
> > The IN_IGNORE_ME flag bit is added to inotify.  When set, none of the
> > other
> > event flags will generate an event if the calling process generated the
> > event.  Given the current way that inotify_add_watch() validates the flags
> > argument, discovering whether the kernel supports the flag requires an
> > extra test (set the watch and generate an event...).
> > 
> > The FAN_IGNORE_ME flag bit does the same for fanotify.  fanotify in
> > current
> > kernels will return an EINVAL error if this bit is set, making discovery
> > easier.  One performance side effect is that this flag eliminates the need
> > and overhead for a test of my_pid == e.pid in the event processing loop.
> > 
> > We chose inotify rather than the current fanotify because we need the
> > extra
> > events that fanotify cannot (currently) support.  dnotify was not touched
> > because it is both obsolete and its api makes this extension difficult.
> > 
> > Please review and comment.  If it is acceptable, please ACK and merge.
> > 
> > Thanks
> > 
> > Jim Lieb, NFS Ganesha project
> > 
> > --
> > 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
-- 
Jim Lieb
Linux Systems Engineer
Panasas Inc.

"If ease of use was the only requirement, we would all be riding tricycles"
- Douglas Engelbart 1925–2013
--
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