[PATCH v3 00/10] Sort out fsnotify_nameremove() mess

[PATCH v3 00/10] Sort out fsnotify_nameremove() mess

[Amir Goldstein]

Jan,

For v3 I went with a straight forward approach.
Filesystems that have fsnotify_{create,mkdir} hooks also get
explicit fsnotify_{unlink,rmdir} hooks.

Hopefully, this approach is orthogonal to whatever changes Al is
planning for recursive tree remove code, because in many of the
cases, the hooks are added at the entry point for the recursive
tree remove.

After looking closer at all the filesystems that were converted to
simple_remove in v2, I decided to exempt another 3 filesystems from
the fsnotify delete hooks: hypfs,qibfs and aafs.
hypfs is pure cleanup (*). qibfs and aafs can remove dentry on user
configuration change, but they do not generate create events, so it
is less likely that users depend on the delete events.

That leaves configfs the only filesystem that gets the new delete hooks
even though it does not have create hooks.

The following d_delete() call sites have been audited and will no longer
generate fsnotify event after this series:

arch/s390/hypfs/inode.c:hypfs_remove() - cleanup (*)
.../usb/gadget/function/f_fs.c:ffs_epfiles_destroy() - no create events
.../infiniband/hw/qib/qib_fs.c:remove_device_files() - no create events
fs/ceph/dir.c:ceph_unlink() - from vfs_unlink()
fs/ceph/inode.c:ceph_fill_trace() - invalidate (**)
fs/ceph/inode.c:ceph_readdir_prepopulate() - invalidate (**)
fs/configfs/dir.c:detach_groups() - hooks added, from vfs or cleanup (*)
fs/configfs/dir.c:configfs_attach_item() - cleanup (*)
fs/configfs/dir.c:configfs_attach_group() - cleanup (*)
fs/efivarfs/file.c:efivarfs_file_write() - invalidate (**)
fs/fuse/dir.c:fuse_reverse_inval_entry() - invalidate (**)
fs/nfs/dir.c:nfs_dentry_handle_enoent() - invalidate (**)
fs/nsfs.c:__ns_get_path() - cleanup (*)
fs/ocfs2/dlmglue.c:ocfs2_dentry_convert_worker() - invalidate (**)
fs/reiserfs/xattr.c:xattr_{unlink,rmdir}() - hidden xattr inode
security/apparmor/apparmorfs.c:aafs_remove() - no create events

(*) There are 2 "cleanup" use cases:
  - Create;init;delete if init failed
  - Batch delete of files within dir before removing dir
  Both those cases are not interesting for users that wish to observe
  configuration changes on pseudo filesystems.  Often, there is already
  an fsnotify event generated on the directory removal which is what
  users should find interesting, for example:
  configfs_unregister_{group,subsystem}().

(**) The different "invalidate" use cases differ, but they all share
  one thing in common - user is not guarantied to get an event with
  current kernel.  For example, when a file is deleted remotely on
  nfs server, nfs client is not guarantied to get an fsnotify delete
  event.  On current kernel, nfs client could generate an fsnotify
  delete event if the local entry happens to be in cache and client
  finds out that entry is deleted on server during another user
  operation.  Incidentally, this group of use cases is where most of
  the call sites are with "unstable" d_name, which is the reason for
  this patch series to begin with.

Thanks,
Amir.

Changes since v2:
- Drop simple_rename() conversions (add explicit hooks instead)
- Drop hooks from hypfs/qibfs/aafs
- Split out debugfs re-factoring patch

Changes since v1:
- Split up per filesystem patches
- New hook names fsnotify_{unlink,rmdir}()
- Simplify fsnotify code in separate final patch

Amir Goldstein (10):
  fsnotify: add empty fsnotify_{unlink,rmdir}() hooks
  btrfs: call fsnotify_rmdir() hook
  rpc_pipefs: call fsnotify_{unlink,rmdir}() hooks
  tracefs: call fsnotify_{unlink,rmdir}() hooks
  devpts: call fsnotify_unlink() hook
  debugfs: simplify __debugfs_remove_file()
  debugfs: call fsnotify_{unlink,rmdir}() hooks
  configfs: call fsnotify_rmdir() hook
  fsnotify: move fsnotify_nameremove() hook out of d_delete()
  fsnotify: get rid of fsnotify_nameremove()

 fs/afs/dir_silly.c               |  5 ----
 fs/btrfs/ioctl.c                 |  4 +++-
 fs/configfs/dir.c                |  3 +++
 fs/dcache.c                      |  2 --
 fs/debugfs/inode.c               | 21 ++++++++--------
 fs/devpts/inode.c                |  1 +
 fs/namei.c                       |  2 ++
 fs/nfs/unlink.c                  |  6 -----
 fs/notify/fsnotify.c             | 41 --------------------------------
 fs/tracefs/inode.c               |  3 +++
 include/linux/fsnotify.h         | 26 ++++++++++++++++++++
 include/linux/fsnotify_backend.h |  4 ----
 net/sunrpc/rpc_pipe.c            |  4 ++++
 13 files changed, 52 insertions(+), 70 deletions(-)

-- 
2.17.1

[PATCH v3 01/10] fsnotify: add empty fsnotify_{unlink,rmdir}() hooks

[Amir Goldstein]

We would like to move fsnotify_nameremove() calls from d_delete()
into a higher layer where the hook makes more sense and so we can
consider every d_delete() call site individually.

Start by creating empty hook fsnotify_{unlink,rmdir}() and place
them in the proper VFS call sites.  After all d_delete() call sites
will be converted to use the new hook, the new hook will generate the
delete events and fsnotify_nameremove() hook will be removed.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/namei.c               |  2 ++
 include/linux/fsnotify.h | 26 ++++++++++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/fs/namei.c b/fs/namei.c
index 20831c2fbb34..209c51a5226c 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -3883,6 +3883,7 @@ int vfs_rmdir(struct inode *dir, struct dentry *dentry)
 	dentry->d_inode->i_flags |= S_DEAD;
 	dont_mount(dentry);
 	detach_mounts(dentry);
+	fsnotify_rmdir(dir, dentry);
 
 out:
 	inode_unlock(dentry->d_inode);
@@ -3999,6 +4000,7 @@ int vfs_unlink(struct inode *dir, struct dentry *dentry, struct inode **delegate
 			if (!error) {
 				dont_mount(dentry);
 				detach_mounts(dentry);
+				fsnotify_unlink(dir, dentry);
 			}
 		}
 	}
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 94972e8eb6d1..7f23eddefcd0 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -188,6 +188,19 @@ static inline void fsnotify_link(struct inode *dir, struct inode *inode, struct
 	fsnotify(dir, FS_CREATE, inode, FSNOTIFY_EVENT_INODE, &new_dentry->d_name, 0);
 }
 
+/*
+ * fsnotify_unlink - 'name' was unlinked
+ *
+ * Caller must make sure that dentry->d_name is stable.
+ */
+static inline void fsnotify_unlink(struct inode *dir, struct dentry *dentry)
+{
+	/* Expected to be called before d_delete() */
+	WARN_ON_ONCE(d_is_negative(dentry));
+
+	/* TODO: call fsnotify_dirent() */
+}
+
 /*
  * fsnotify_mkdir - directory 'name' was created
  */
@@ -198,6 +211,19 @@ static inline void fsnotify_mkdir(struct inode *inode, struct dentry *dentry)
 	fsnotify_dirent(inode, dentry, FS_CREATE | FS_ISDIR);
 }
 
+/*
+ * fsnotify_rmdir - directory 'name' was removed
+ *
+ * Caller must make sure that dentry->d_name is stable.
+ */
+static inline void fsnotify_rmdir(struct inode *dir, struct dentry *dentry)
+{
+	/* Expected to be called before d_delete() */
+	WARN_ON_ONCE(d_is_negative(dentry));
+
+	/* TODO: call fsnotify_dirent() */
+}
+
 /*
  * fsnotify_access - file was read
  */
-- 
2.17.1

[PATCH v3 02/10] btrfs: call fsnotify_rmdir() hook

[Amir Goldstein]

This will allow generating fsnotify delete events after the
fsnotify_nameremove() hook is removed from d_delete().

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Acked-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/ioctl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 6dafa857bbb9..2cfd1bfb3871 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2930,8 +2930,10 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file,
 	inode_lock(inode);
 	err = btrfs_delete_subvolume(dir, dentry);
 	inode_unlock(inode);
-	if (!err)
+	if (!err) {
+		fsnotify_rmdir(dir, dentry);
 		d_delete(dentry);
+	}
 
 out_dput:
 	dput(dentry);
-- 
2.17.1

[PATCH v3 03/10] rpc_pipefs: call fsnotify_{unlink,rmdir}() hooks

[Amir Goldstein]

This will allow generating fsnotify delete events after the
fsnotify_nameremove() hook is removed from d_delete().

Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
Cc: Anna Schumaker <anna.schumaker@netapp.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 net/sunrpc/rpc_pipe.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 979d23646e33..917c85f15a0b 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -597,6 +597,8 @@ static int __rpc_rmdir(struct inode *dir, struct dentry *dentry)
 
 	dget(dentry);
 	ret = simple_rmdir(dir, dentry);
+	if (!ret)
+		fsnotify_rmdir(dir, dentry);
 	d_delete(dentry);
 	dput(dentry);
 	return ret;
@@ -608,6 +610,8 @@ static int __rpc_unlink(struct inode *dir, struct dentry *dentry)
 
 	dget(dentry);
 	ret = simple_unlink(dir, dentry);
+	if (!ret)
+		fsnotify_unlink(dir, dentry);
 	d_delete(dentry);
 	dput(dentry);
 	return ret;
-- 
2.17.1

[PATCH v3 04/10] tracefs: call fsnotify_{unlink,rmdir}() hooks

[Amir Goldstein]

This will allow generating fsnotify delete events after the
fsnotify_nameremove() hook is removed from d_delete().

Cc: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/tracefs/inode.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index 7098c49f3693..497a8682b5b9 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -509,9 +509,12 @@ static int __tracefs_remove(struct dentry *dentry, struct dentry *parent)
 			switch (dentry->d_inode->i_mode & S_IFMT) {
 			case S_IFDIR:
 				ret = simple_rmdir(parent->d_inode, dentry);
+				if (!ret)
+					fsnotify_rmdir(parent->d_inode, dentry);
 				break;
 			default:
 				simple_unlink(parent->d_inode, dentry);
+				fsnotify_unlink(parent->d_inode, dentry);
 				break;
 			}
 			if (!ret)
-- 
2.17.1

[PATCH v3 05/10] devpts: call fsnotify_unlink() hook

[Amir Goldstein]

This will allow generating fsnotify delete events after the
fsnotify_nameremove() hook is removed from d_delete().

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/devpts/inode.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/devpts/inode.c b/fs/devpts/inode.c
index 553a3f3300ae..1d7844809f01 100644
--- a/fs/devpts/inode.c
+++ b/fs/devpts/inode.c
@@ -624,6 +624,7 @@ void devpts_pty_kill(struct dentry *dentry)
 
 	dentry->d_fsdata = NULL;
 	drop_nlink(dentry->d_inode);
+	fsnotify_unlink(d_inode(dentry->d_parent), dentry);
 	d_delete(dentry);
 	dput(dentry);	/* d_alloc_name() in devpts_pty_new() */
 }
-- 
2.17.1

[PATCH v3 06/10] debugfs: simplify __debugfs_remove_file()

[Amir Goldstein]

Move simple_unlink()+d_delete() from __debugfs_remove_file() into
caller __debugfs_remove() and rename helper for post remove file to
__debugfs_file_removed().

This will simplify adding fsnotify_unlink() hook.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/debugfs/inode.c | 20 ++++++++------------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index acef14ad53db..d89874da9791 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -617,13 +617,10 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
 }
 EXPORT_SYMBOL_GPL(debugfs_create_symlink);
 
-static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent)
+static void __debugfs_file_removed(struct dentry *dentry)
 {
 	struct debugfs_fsdata *fsd;
 
-	simple_unlink(d_inode(parent), dentry);
-	d_delete(dentry);
-
 	/*
 	 * Paired with the closing smp_mb() implied by a successful
 	 * cmpxchg() in debugfs_file_get(): either
@@ -644,16 +641,15 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent)
 
 	if (simple_positive(dentry)) {
 		dget(dentry);
-		if (!d_is_reg(dentry)) {
-			if (d_is_dir(dentry))
-				ret = simple_rmdir(d_inode(parent), dentry);
-			else
-				simple_unlink(d_inode(parent), dentry);
-			if (!ret)
-				d_delete(dentry);
+		if (d_is_dir(dentry)) {
+			ret = simple_rmdir(d_inode(parent), dentry);
 		} else {
-			__debugfs_remove_file(dentry, parent);
+			simple_unlink(d_inode(parent), dentry);
 		}
+		if (!ret)
+			d_delete(dentry);
+		if (d_is_reg(dentry))
+			__debugfs_file_removed(dentry);
 		dput(dentry);
 	}
 	return ret;
-- 
2.17.1

[PATCH v3 07/10] debugfs: call fsnotify_{unlink,rmdir}() hooks

[Amir Goldstein]

This will allow generating fsnotify delete events after the
fsnotify_nameremove() hook is removed from d_delete().

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/debugfs/inode.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index d89874da9791..1e444fe1f778 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -643,8 +643,11 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent)
 		dget(dentry);
 		if (d_is_dir(dentry)) {
 			ret = simple_rmdir(d_inode(parent), dentry);
+			if (!ret)
+				fsnotify_rmdir(d_inode(parent), dentry);
 		} else {
 			simple_unlink(d_inode(parent), dentry);
+			fsnotify_unlink(d_inode(parent), dentry);
 		}
 		if (!ret)
 			d_delete(dentry);
-- 
2.17.1

[PATCH v3 08/10] configfs: call fsnotify_rmdir() hook

[Amir Goldstein]

This will allow generating fsnotify delete events on unregister
of group/subsystem after the fsnotify_nameremove() hook is removed
from d_delete().

The rest of the d_delete() calls from this filesystem are either
called recursively from within debugfs_unregister_{group,subsystem},
called from a vfs function that already has delete hooks or are
called from shutdown/cleanup code.

Cc: Joel Becker <jlbec@evilplan.org>
Cc: Christoph Hellwig <hch@lst.de>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/configfs/dir.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 5e7932d668ab..ba17881a8d84 100644
--- a/fs/configfs/dir.c
+++ b/fs/configfs/dir.c
@@ -27,6 +27,7 @@
 #undef DEBUG
 
 #include <linux/fs.h>
+#include <linux/fsnotify.h>
 #include <linux/mount.h>
 #include <linux/module.h>
 #include <linux/slab.h>
@@ -1804,6 +1805,7 @@ void configfs_unregister_group(struct config_group *group)
 	configfs_detach_group(&group->cg_item);
 	d_inode(dentry)->i_flags |= S_DEAD;
 	dont_mount(dentry);
+	fsnotify_rmdir(d_inode(parent), dentry);
 	d_delete(dentry);
 	inode_unlock(d_inode(parent));
 
@@ -1932,6 +1934,7 @@ void configfs_unregister_subsystem(struct configfs_subsystem *subsys)
 	configfs_detach_group(&group->cg_item);
 	d_inode(dentry)->i_flags |= S_DEAD;
 	dont_mount(dentry);
+	fsnotify_rmdir(d_inode(root), dentry);
 	inode_unlock(d_inode(dentry));
 
 	d_delete(dentry);
-- 
2.17.1

[PATCH v3 09/10] fsnotify: move fsnotify_nameremove() hook out of d_delete()

[Amir Goldstein]

d_delete() was piggy backed for the fsnotify_nameremove() hook when
in fact not all callers of d_delete() care about fsnotify events.

For all callers of d_delete() that may be interested in fsnotify events,
we made sure to call one of fsnotify_{unlink,rmdir}() hooks before
calling d_delete().

Now we can move the fsnotify_nameremove() call from d_delete() to the
fsnotify_{unlink,rmdir}() hooks.

Two explicit calls to fsnotify_nameremove() from nfs/afs sillyrename
are also removed. This will cause a change of behavior - nfs/afs will
NOT generate an fsnotify delete event when renaming over a positive
dentry.  This change is desirable, because it is consistent with the
behavior of all other filesystems.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/afs/dir_silly.c       | 5 -----
 fs/dcache.c              | 2 --
 fs/nfs/unlink.c          | 6 ------
 include/linux/fsnotify.h | 2 ++
 4 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/fs/afs/dir_silly.c b/fs/afs/dir_silly.c
index 28f4aa015229..312687223170 100644
--- a/fs/afs/dir_silly.c
+++ b/fs/afs/dir_silly.c
@@ -64,11 +64,6 @@ static int afs_do_silly_rename(struct afs_vnode *dvnode, struct afs_vnode *vnode
 		if (test_bit(AFS_VNODE_DIR_VALID, &dvnode->flags))
 			afs_edit_dir_add(dvnode, &new->d_name,
 					 &vnode->fid, afs_edit_dir_for_silly_1);
-
-		/* vfs_unlink and the like do not issue this when a file is
-		 * sillyrenamed, so do it here.
-		 */
-		fsnotify_nameremove(old, 0);
 	}
 
 	kfree(scb);
diff --git a/fs/dcache.c b/fs/dcache.c
index 8136bda27a1f..ce131339410c 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2371,7 +2371,6 @@ EXPORT_SYMBOL(d_hash_and_lookup);
 void d_delete(struct dentry * dentry)
 {
 	struct inode *inode = dentry->d_inode;
-	int isdir = d_is_dir(dentry);
 
 	spin_lock(&inode->i_lock);
 	spin_lock(&dentry->d_lock);
@@ -2386,7 +2385,6 @@ void d_delete(struct dentry * dentry)
 		spin_unlock(&dentry->d_lock);
 		spin_unlock(&inode->i_lock);
 	}
-	fsnotify_nameremove(dentry, isdir);
 }
 EXPORT_SYMBOL(d_delete);
 
diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
index 52d533967485..0effeee28352 100644
--- a/fs/nfs/unlink.c
+++ b/fs/nfs/unlink.c
@@ -396,12 +396,6 @@ nfs_complete_sillyrename(struct rpc_task *task, struct nfs_renamedata *data)
 		nfs_cancel_async_unlink(dentry);
 		return;
 	}
-
-	/*
-	 * vfs_unlink and the like do not issue this when a file is
-	 * sillyrenamed, so do it here.
-	 */
-	fsnotify_nameremove(dentry, 0);
 }
 
 #define SILLYNAME_PREFIX ".nfs"
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 7f23eddefcd0..0145073c2b42 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -199,6 +199,7 @@ static inline void fsnotify_unlink(struct inode *dir, struct dentry *dentry)
 	WARN_ON_ONCE(d_is_negative(dentry));
 
 	/* TODO: call fsnotify_dirent() */
+	fsnotify_nameremove(dentry, 0);
 }
 
 /*
@@ -222,6 +223,7 @@ static inline void fsnotify_rmdir(struct inode *dir, struct dentry *dentry)
 	WARN_ON_ONCE(d_is_negative(dentry));
 
 	/* TODO: call fsnotify_dirent() */
+	fsnotify_nameremove(dentry, 1);
 }
 
 /*
-- 
2.17.1

[PATCH v3 10/10] fsnotify: get rid of fsnotify_nameremove()

[Amir Goldstein]

For all callers of fsnotify_{unlink,rmdir}(), we made sure that d_parent
and d_name are stable.  Therefore, fsnotify_{unlink,rmdir}() do not need
the safety measures in fsnotify_nameremove() to stabilize parent and name.
We can now simplify those hooks and get rid of fsnotify_nameremove().

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fsnotify.c             | 41 --------------------------------
 include/linux/fsnotify.h         |  6 ++---
 include/linux/fsnotify_backend.h |  4 ----
 3 files changed, 2 insertions(+), 49 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 8c7cbac7183c..5433e37fb0c5 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -107,47 +107,6 @@ void fsnotify_sb_delete(struct super_block *sb)
 	fsnotify_clear_marks_by_sb(sb);
 }
 
-/*
- * fsnotify_nameremove - a filename was removed from a directory
- *
- * This is mostly called under parent vfs inode lock so name and
- * dentry->d_parent should be stable. However there are some corner cases where
- * inode lock is not held. So to be on the safe side and be reselient to future
- * callers and out of tree users of d_delete(), we do not assume that d_parent
- * and d_name are stable and we use dget_parent() and
- * take_dentry_name_snapshot() to grab stable references.
- */
-void fsnotify_nameremove(struct dentry *dentry, int isdir)
-{
-	struct dentry *parent;
-	struct name_snapshot name;
-	__u32 mask = FS_DELETE;
-
-	/* d_delete() of pseudo inode? (e.g. __ns_get_path() playing tricks) */
-	if (IS_ROOT(dentry))
-		return;
-
-	if (isdir)
-		mask |= FS_ISDIR;
-
-	parent = dget_parent(dentry);
-	/* Avoid unneeded take_dentry_name_snapshot() */
-	if (!(d_inode(parent)->i_fsnotify_mask & FS_DELETE) &&
-	    !(dentry->d_sb->s_fsnotify_mask & FS_DELETE))
-		goto out_dput;
-
-	take_dentry_name_snapshot(&name, dentry);
-
-	fsnotify(d_inode(parent), mask, d_inode(dentry), FSNOTIFY_EVENT_INODE,
-		 &name.name, 0);
-
-	release_dentry_name_snapshot(&name);
-
-out_dput:
-	dput(parent);
-}
-EXPORT_SYMBOL(fsnotify_nameremove);
-
 /*
  * Given an inode, first check if we care what happens to our children.  Inotify
  * and dnotify both tell their parents about events.  If we care about any event
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 0145073c2b42..a2d5d175d3c1 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -198,8 +198,7 @@ static inline void fsnotify_unlink(struct inode *dir, struct dentry *dentry)
 	/* Expected to be called before d_delete() */
 	WARN_ON_ONCE(d_is_negative(dentry));
 
-	/* TODO: call fsnotify_dirent() */
-	fsnotify_nameremove(dentry, 0);
+	fsnotify_dirent(dir, dentry, FS_DELETE);
 }
 
 /*
@@ -222,8 +221,7 @@ static inline void fsnotify_rmdir(struct inode *dir, struct dentry *dentry)
 	/* Expected to be called before d_delete() */
 	WARN_ON_ONCE(d_is_negative(dentry));
 
-	/* TODO: call fsnotify_dirent() */
-	fsnotify_nameremove(dentry, 1);
+	fsnotify_dirent(dir, dentry, FS_DELETE | FS_ISDIR);
 }
 
 /*
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index a9f9dcc1e515..c28f6ed1f59b 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -355,7 +355,6 @@ extern int __fsnotify_parent(const struct path *path, struct dentry *dentry, __u
 extern void __fsnotify_inode_delete(struct inode *inode);
 extern void __fsnotify_vfsmount_delete(struct vfsmount *mnt);
 extern void fsnotify_sb_delete(struct super_block *sb);
-extern void fsnotify_nameremove(struct dentry *dentry, int isdir);
 extern u32 fsnotify_get_cookie(void);
 
 static inline int fsnotify_inode_watches_children(struct inode *inode)
@@ -525,9 +524,6 @@ static inline void __fsnotify_vfsmount_delete(struct vfsmount *mnt)
 static inline void fsnotify_sb_delete(struct super_block *sb)
 {}
 
-static inline void fsnotify_nameremove(struct dentry *dentry, int isdir)
-{}
-
 static inline void fsnotify_update_flags(struct dentry *dentry)
 {}
 
-- 
2.17.1

Re: [PATCH v3 00/10] Sort out fsnotify_nameremove() mess

[Greg Kroah-Hartman]

On Sun, May 26, 2019 at 05:34:01PM +0300, Amir Goldstein wrote:
> Jan,
> 
> For v3 I went with a straight forward approach.
> Filesystems that have fsnotify_{create,mkdir} hooks also get
> explicit fsnotify_{unlink,rmdir} hooks.
> 
> Hopefully, this approach is orthogonal to whatever changes Al is
> planning for recursive tree remove code, because in many of the
> cases, the hooks are added at the entry point for the recursive
> tree remove.
> 
> After looking closer at all the filesystems that were converted to
> simple_remove in v2, I decided to exempt another 3 filesystems from
> the fsnotify delete hooks: hypfs,qibfs and aafs.
> hypfs is pure cleanup (*). qibfs and aafs can remove dentry on user
> configuration change, but they do not generate create events, so it
> is less likely that users depend on the delete events.
> 
> That leaves configfs the only filesystem that gets the new delete hooks
> even though it does not have create hooks.

why doesn't configfs have create hooks?  That's what userspace does in
configfs, shouldn't it be notified about it?  Keeping it "unequal" seems
odd to me.


thanks,

greg k-h

Re: [PATCH v3 00/10] Sort out fsnotify_nameremove() mess

[Amir Goldstein]

On Mon, May 27, 2019 at 11:25 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Sun, May 26, 2019 at 05:34:01PM +0300, Amir Goldstein wrote:
> > Jan,
> >
> > For v3 I went with a straight forward approach.
> > Filesystems that have fsnotify_{create,mkdir} hooks also get
> > explicit fsnotify_{unlink,rmdir} hooks.
> >
> > Hopefully, this approach is orthogonal to whatever changes Al is
> > planning for recursive tree remove code, because in many of the
> > cases, the hooks are added at the entry point for the recursive
> > tree remove.
> >
> > After looking closer at all the filesystems that were converted to
> > simple_remove in v2, I decided to exempt another 3 filesystems from
> > the fsnotify delete hooks: hypfs,qibfs and aafs.
> > hypfs is pure cleanup (*). qibfs and aafs can remove dentry on user
> > configuration change, but they do not generate create events, so it
> > is less likely that users depend on the delete events.
> >
> > That leaves configfs the only filesystem that gets the new delete hooks
> > even though it does not have create hooks.
>
> why doesn't configfs have create hooks? That's what userspace does in
> configfs, shouldn't it be notified about it?  Keeping it "unequal" seems
> odd to me.
>

So it's not exactly that configfs has no create hooks at all.
For "normal" filesystems mkdir (for example) is only possible
by mkdir(2) syscall and there is create hook in vfs_mkdir().

The configfs (as well as debugfs/tracefs/etc), there are other code paths
that create directories, namely: configfs_register_grup/subsystem().
Those code paths have explicit fsnotify_mkdir() hook in debugfs/tracefs/etc,
but not in configfs. Why? because nobody put the hooks and no user
complained.

Should we add fsnotify_mkdir() hooks in configfs - probably yes.
I can do it as followup, but this is not the purpose of this patch set.
The purpose of this patch set (achieved in the last patch) is to simplify
the implementation of the fsnotify delete hook.
Today it is overly complicated by the fact that the hooks was placed
in d_delete() and d_delete() is called from some code paths that have
no business with fsnotify notifications at all.

Once this patch set is done sprinkling fsnotify_rmdir/unlink() hooks
in "proper" places, it removes the current fsnotify hook from d_delete().
d_delete() is called from configfs_unregister_group/subsystem(), so if
we do not add fsnotify delete hooks to configfs we will regress the existing
"unequal" behavior.
Maybe there are no users depending on fsnotify delete notifications from
configfs - I do not know. If we believe that is the case, we can drop the
configfs patch.
Maybe there *are* user depending on fsnotify delete notifications from aafs
(apparmorfs), which I decided to exclude from v3 patch set.
If we believe that is the case, or if we find out later that in case -
no problem
it is simple to add the missing fsnotify hooks in apparmorfs.

Thought? About inclusion of configfs? About exclusion of apparmorfs?
Again this is only exclusion/inclusion of hooks from code path that does
NOT come from vfs syscalls (e.g. aa_remove_profiles()).

Thanks,
Amir.

Re: [PATCH v3 03/10] rpc_pipefs: call fsnotify_{unlink,rmdir}() hooks

[Jan Kara]

On Sun 26-05-19 17:34:04, Amir Goldstein wrote:
> This will allow generating fsnotify delete events after the
> fsnotify_nameremove() hook is removed from d_delete().
> 
> Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> Cc: Anna Schumaker <anna.schumaker@netapp.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  net/sunrpc/rpc_pipe.c | 4 ++++
>  1 file changed, 4 insertions(+)

Hum, I don't think all __rpc_depopulate() calls are guarded by i_rwsem
(e.g. those in rpc_gssd_dummy_populate()). Why aren't we using
rpc_depopulate() in those cases? Trond, Anna?

								Honza

> 
> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> index 979d23646e33..917c85f15a0b 100644
> --- a/net/sunrpc/rpc_pipe.c
> +++ b/net/sunrpc/rpc_pipe.c
> @@ -597,6 +597,8 @@ static int __rpc_rmdir(struct inode *dir, struct dentry *dentry)
>  
>  	dget(dentry);
>  	ret = simple_rmdir(dir, dentry);
> +	if (!ret)
> +		fsnotify_rmdir(dir, dentry);
>  	d_delete(dentry);
>  	dput(dentry);
>  	return ret;
> @@ -608,6 +610,8 @@ static int __rpc_unlink(struct inode *dir, struct dentry *dentry)
>  
>  	dget(dentry);
>  	ret = simple_unlink(dir, dentry);
> +	if (!ret)
> +		fsnotify_unlink(dir, dentry);
>  	d_delete(dentry);
>  	dput(dentry);
>  	return ret;
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 00/10] Sort out fsnotify_nameremove() mess

[Greg Kroah-Hartman]

On Mon, May 27, 2019 at 12:49:23PM +0300, Amir Goldstein wrote:
> On Mon, May 27, 2019 at 11:25 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Sun, May 26, 2019 at 05:34:01PM +0300, Amir Goldstein wrote:
> > > Jan,
> > >
> > > For v3 I went with a straight forward approach.
> > > Filesystems that have fsnotify_{create,mkdir} hooks also get
> > > explicit fsnotify_{unlink,rmdir} hooks.
> > >
> > > Hopefully, this approach is orthogonal to whatever changes Al is
> > > planning for recursive tree remove code, because in many of the
> > > cases, the hooks are added at the entry point for the recursive
> > > tree remove.
> > >
> > > After looking closer at all the filesystems that were converted to
> > > simple_remove in v2, I decided to exempt another 3 filesystems from
> > > the fsnotify delete hooks: hypfs,qibfs and aafs.
> > > hypfs is pure cleanup (*). qibfs and aafs can remove dentry on user
> > > configuration change, but they do not generate create events, so it
> > > is less likely that users depend on the delete events.
> > >
> > > That leaves configfs the only filesystem that gets the new delete hooks
> > > even though it does not have create hooks.
> >
> > why doesn't configfs have create hooks? That's what userspace does in
> > configfs, shouldn't it be notified about it?  Keeping it "unequal" seems
> > odd to me.
> >
> 
> So it's not exactly that configfs has no create hooks at all.
> For "normal" filesystems mkdir (for example) is only possible
> by mkdir(2) syscall and there is create hook in vfs_mkdir().

Ah, ok, missed that.

thanks,

greg k-h

Re: [PATCH v3 00/10] Sort out fsnotify_nameremove() mess

[Jan Kara]

Hi Amir!

On Sun 26-05-19 17:34:01, Amir Goldstein wrote:
> For v3 I went with a straight forward approach.
> Filesystems that have fsnotify_{create,mkdir} hooks also get
> explicit fsnotify_{unlink,rmdir} hooks.
> 
> Hopefully, this approach is orthogonal to whatever changes Al is
> planning for recursive tree remove code, because in many of the
> cases, the hooks are added at the entry point for the recursive
> tree remove.
> 
> After looking closer at all the filesystems that were converted to
> simple_remove in v2, I decided to exempt another 3 filesystems from
> the fsnotify delete hooks: hypfs,qibfs and aafs.
> hypfs is pure cleanup (*). qibfs and aafs can remove dentry on user
> configuration change, but they do not generate create events, so it
> is less likely that users depend on the delete events.
> 
> That leaves configfs the only filesystem that gets the new delete hooks
> even though it does not have create hooks.

OK, I went through the patches and they look OK to me. So once we get acks
from respective maintainers I think we can merge them.

								Honza

> 
> The following d_delete() call sites have been audited and will no longer
> generate fsnotify event after this series:
> 
> arch/s390/hypfs/inode.c:hypfs_remove() - cleanup (*)
> .../usb/gadget/function/f_fs.c:ffs_epfiles_destroy() - no create events
> .../infiniband/hw/qib/qib_fs.c:remove_device_files() - no create events
> fs/ceph/dir.c:ceph_unlink() - from vfs_unlink()
> fs/ceph/inode.c:ceph_fill_trace() - invalidate (**)
> fs/ceph/inode.c:ceph_readdir_prepopulate() - invalidate (**)
> fs/configfs/dir.c:detach_groups() - hooks added, from vfs or cleanup (*)
> fs/configfs/dir.c:configfs_attach_item() - cleanup (*)
> fs/configfs/dir.c:configfs_attach_group() - cleanup (*)
> fs/efivarfs/file.c:efivarfs_file_write() - invalidate (**)
> fs/fuse/dir.c:fuse_reverse_inval_entry() - invalidate (**)
> fs/nfs/dir.c:nfs_dentry_handle_enoent() - invalidate (**)
> fs/nsfs.c:__ns_get_path() - cleanup (*)
> fs/ocfs2/dlmglue.c:ocfs2_dentry_convert_worker() - invalidate (**)
> fs/reiserfs/xattr.c:xattr_{unlink,rmdir}() - hidden xattr inode
> security/apparmor/apparmorfs.c:aafs_remove() - no create events
> 
> (*) There are 2 "cleanup" use cases:
>   - Create;init;delete if init failed
>   - Batch delete of files within dir before removing dir
>   Both those cases are not interesting for users that wish to observe
>   configuration changes on pseudo filesystems.  Often, there is already
>   an fsnotify event generated on the directory removal which is what
>   users should find interesting, for example:
>   configfs_unregister_{group,subsystem}().
> 
> (**) The different "invalidate" use cases differ, but they all share
>   one thing in common - user is not guarantied to get an event with
>   current kernel.  For example, when a file is deleted remotely on
>   nfs server, nfs client is not guarantied to get an fsnotify delete
>   event.  On current kernel, nfs client could generate an fsnotify
>   delete event if the local entry happens to be in cache and client
>   finds out that entry is deleted on server during another user
>   operation.  Incidentally, this group of use cases is where most of
>   the call sites are with "unstable" d_name, which is the reason for
>   this patch series to begin with.
> 
> Thanks,
> Amir.
> 
> Changes since v2:
> - Drop simple_rename() conversions (add explicit hooks instead)
> - Drop hooks from hypfs/qibfs/aafs
> - Split out debugfs re-factoring patch
> 
> Changes since v1:
> - Split up per filesystem patches
> - New hook names fsnotify_{unlink,rmdir}()
> - Simplify fsnotify code in separate final patch
> 
> Amir Goldstein (10):
>   fsnotify: add empty fsnotify_{unlink,rmdir}() hooks
>   btrfs: call fsnotify_rmdir() hook
>   rpc_pipefs: call fsnotify_{unlink,rmdir}() hooks
>   tracefs: call fsnotify_{unlink,rmdir}() hooks
>   devpts: call fsnotify_unlink() hook
>   debugfs: simplify __debugfs_remove_file()
>   debugfs: call fsnotify_{unlink,rmdir}() hooks
>   configfs: call fsnotify_rmdir() hook
>   fsnotify: move fsnotify_nameremove() hook out of d_delete()
>   fsnotify: get rid of fsnotify_nameremove()
> 
>  fs/afs/dir_silly.c               |  5 ----
>  fs/btrfs/ioctl.c                 |  4 +++-
>  fs/configfs/dir.c                |  3 +++
>  fs/dcache.c                      |  2 --
>  fs/debugfs/inode.c               | 21 ++++++++--------
>  fs/devpts/inode.c                |  1 +
>  fs/namei.c                       |  2 ++
>  fs/nfs/unlink.c                  |  6 -----
>  fs/notify/fsnotify.c             | 41 --------------------------------
>  fs/tracefs/inode.c               |  3 +++
>  include/linux/fsnotify.h         | 26 ++++++++++++++++++++
>  include/linux/fsnotify_backend.h |  4 ----
>  net/sunrpc/rpc_pipe.c            |  4 ++++
>  13 files changed, 52 insertions(+), 70 deletions(-)
> 
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 03/10] rpc_pipefs: call fsnotify_{unlink,rmdir}() hooks

[Amir Goldstein]

On Mon, May 27, 2019 at 1:54 PM Jan Kara <jack@suse.cz> wrote:
>
> On Sun 26-05-19 17:34:04, Amir Goldstein wrote:
> > This will allow generating fsnotify delete events after the
> > fsnotify_nameremove() hook is removed from d_delete().
> >
> > Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> > Cc: Anna Schumaker <anna.schumaker@netapp.com>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  net/sunrpc/rpc_pipe.c | 4 ++++
> >  1 file changed, 4 insertions(+)
>
> Hum, I don't think all __rpc_depopulate() calls are guarded by i_rwsem
> (e.g. those in rpc_gssd_dummy_populate()). Why aren't we using
> rpc_depopulate() in those cases? Trond, Anna?
>

Do we care? For fsnotify hook, we should only care that
d_parent/d_name are stable.
They are stable because rpc_pipefs has no rename.

Thanks,
Amir.

Re: [PATCH v3 03/10] rpc_pipefs: call fsnotify_{unlink,rmdir}() hooks

[Jan Kara]

On Mon 27-05-19 16:26:03, Amir Goldstein wrote:
> On Mon, May 27, 2019 at 1:54 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Sun 26-05-19 17:34:04, Amir Goldstein wrote:
> > > This will allow generating fsnotify delete events after the
> > > fsnotify_nameremove() hook is removed from d_delete().
> > >
> > > Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > Cc: Anna Schumaker <anna.schumaker@netapp.com>
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > ---
> > >  net/sunrpc/rpc_pipe.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> >
> > Hum, I don't think all __rpc_depopulate() calls are guarded by i_rwsem
> > (e.g. those in rpc_gssd_dummy_populate()). Why aren't we using
> > rpc_depopulate() in those cases? Trond, Anna?
> >
> 
> Do we care? For fsnotify hook, we should only care that
> d_parent/d_name are stable.
> They are stable because rpc_pipefs has no rename.

Yeah, good point. Probably we don't. I was just wondering...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v3 03/10] rpc_pipefs: call fsnotify_{unlink,rmdir}() hooks

[Amir Goldstein]

Hi Trond,Anna,Bruce,

Seems that rpc_pipefs is co-maintained by client and server trees, so
not sure who is the best to ask for an ack.
We need to add those explicit fsnotify hooks to match the existing
fsnotify_create/mkdir hooks, because
the hook embedded inside d_delete() is going away [1].

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/20190526143411.11244-1-amir73il@gmail.com/

On Sun, May 26, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> This will allow generating fsnotify delete events after the
> fsnotify_nameremove() hook is removed from d_delete().
>
> Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> Cc: Anna Schumaker <anna.schumaker@netapp.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  net/sunrpc/rpc_pipe.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> index 979d23646e33..917c85f15a0b 100644
> --- a/net/sunrpc/rpc_pipe.c
> +++ b/net/sunrpc/rpc_pipe.c
> @@ -597,6 +597,8 @@ static int __rpc_rmdir(struct inode *dir, struct dentry *dentry)
>
>         dget(dentry);
>         ret = simple_rmdir(dir, dentry);
> +       if (!ret)
> +               fsnotify_rmdir(dir, dentry);
>         d_delete(dentry);
>         dput(dentry);
>         return ret;
> @@ -608,6 +610,8 @@ static int __rpc_unlink(struct inode *dir, struct dentry *dentry)
>
>         dget(dentry);
>         ret = simple_unlink(dir, dentry);
> +       if (!ret)
> +               fsnotify_unlink(dir, dentry);
>         d_delete(dentry);
>         dput(dentry);
>         return ret;
> --
> 2.17.1
>

Re: [PATCH v3 06/10] debugfs: simplify __debugfs_remove_file()

[Amir Goldstein]

On Sun, May 26, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Move simple_unlink()+d_delete() from __debugfs_remove_file() into
> caller __debugfs_remove() and rename helper for post remove file to
> __debugfs_file_removed().
>
> This will simplify adding fsnotify_unlink() hook.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Hi Greg,

Will be you be able to provide an ACK on this debugfs patch and the next one.

Thanks,
Amir.

> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/debugfs/inode.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index acef14ad53db..d89874da9791 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -617,13 +617,10 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
>  }
>  EXPORT_SYMBOL_GPL(debugfs_create_symlink);
>
> -static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent)
> +static void __debugfs_file_removed(struct dentry *dentry)
>  {
>         struct debugfs_fsdata *fsd;
>
> -       simple_unlink(d_inode(parent), dentry);
> -       d_delete(dentry);
> -
>         /*
>          * Paired with the closing smp_mb() implied by a successful
>          * cmpxchg() in debugfs_file_get(): either
> @@ -644,16 +641,15 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent)
>
>         if (simple_positive(dentry)) {
>                 dget(dentry);
> -               if (!d_is_reg(dentry)) {
> -                       if (d_is_dir(dentry))
> -                               ret = simple_rmdir(d_inode(parent), dentry);
> -                       else
> -                               simple_unlink(d_inode(parent), dentry);
> -                       if (!ret)
> -                               d_delete(dentry);
> +               if (d_is_dir(dentry)) {
> +                       ret = simple_rmdir(d_inode(parent), dentry);
>                 } else {
> -                       __debugfs_remove_file(dentry, parent);
> +                       simple_unlink(d_inode(parent), dentry);
>                 }
> +               if (!ret)
> +                       d_delete(dentry);
> +               if (d_is_reg(dentry))
> +                       __debugfs_file_removed(dentry);
>                 dput(dentry);
>         }
>         return ret;
> --
> 2.17.1
>

Re: [PATCH v3 08/10] configfs: call fsnotify_rmdir() hook

[Amir Goldstein]

On Sun, May 26, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> This will allow generating fsnotify delete events on unregister
> of group/subsystem after the fsnotify_nameremove() hook is removed
> from d_delete().
>
> The rest of the d_delete() calls from this filesystem are either
> called recursively from within debugfs_unregister_{group,subsystem},
> called from a vfs function that already has delete hooks or are
> called from shutdown/cleanup code.
>
> Cc: Joel Becker <jlbec@evilplan.org>
> Cc: Christoph Hellwig <hch@lst.de>

Hi Christoph and Joel,

Per Christoph's request, I cc'ed you guys on the entire patch series
for context,
so my discussion with Greg [1] about the special status of configfs in
this patch set
should already be somewhere in your mailboxes...

Could I ask you to provide an ACK on this patch and on the chosen
policy. To recap:
Before patch set:
1. User gets create/delete events when files/dirs created/removed via vfs_*()
2. User does *not* get create events when files/dirs created via
debugfs_register_*()
3. User *does* get delete events when files/dirs removed via
debugfs_unregister_*()

After patch set:
1. No change
2. No change
3. User will get delete events only on the root group/subsystem dir
when tree is removed via debugfs_unregister_*()

For symmetry, we could also add create events for  root group/subsystem dir
when tree is created via debugfs_unregister_*(), but that would be a
followup patch.
For users though, it may be that delete events are more important than
create events
(i.e. for user cleanup tasks).

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/CAOQ4uxjyg5AVPrcR4bPm4zMY9BKmgV8g7TAuH--cfKNJv8pRYQ@mail.gmail.com/

> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/configfs/dir.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 5e7932d668ab..ba17881a8d84 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -27,6 +27,7 @@
>  #undef DEBUG
>
>  #include <linux/fs.h>
> +#include <linux/fsnotify.h>
>  #include <linux/mount.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> @@ -1804,6 +1805,7 @@ void configfs_unregister_group(struct config_group *group)
>         configfs_detach_group(&group->cg_item);
>         d_inode(dentry)->i_flags |= S_DEAD;
>         dont_mount(dentry);
> +       fsnotify_rmdir(d_inode(parent), dentry);
>         d_delete(dentry);
>         inode_unlock(d_inode(parent));
>
> @@ -1932,6 +1934,7 @@ void configfs_unregister_subsystem(struct configfs_subsystem *subsys)
>         configfs_detach_group(&group->cg_item);
>         d_inode(dentry)->i_flags |= S_DEAD;
>         dont_mount(dentry);
> +       fsnotify_rmdir(d_inode(root), dentry);
>         inode_unlock(d_inode(dentry));
>
>         d_delete(dentry);
> --
> 2.17.1
>

Re: [PATCH v3 03/10] rpc_pipefs: call fsnotify_{unlink,rmdir}() hooks

[Trond Myklebust]

On Thu, 2019-05-30 at 08:43 +0300, Amir Goldstein wrote:
> Hi Trond,Anna,Bruce,
> 
> Seems that rpc_pipefs is co-maintained by client and server trees, so
> not sure who is the best to ask for an ack.
> We need to add those explicit fsnotify hooks to match the existing
> fsnotify_create/mkdir hooks, because
> the hook embedded inside d_delete() is going away [1].
> 
> Thanks,
> Amir.
> 
> [1] 
> https://lore.kernel.org/linux-fsdevel/20190526143411.11244-1-amir73il@gmail.com/
> 
> On Sun, May 26, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com>
> wrote:
> > This will allow generating fsnotify delete events after the
> > fsnotify_nameremove() hook is removed from d_delete().
> > 
> > Cc: Trond Myklebust <trond.myklebust@hammerspace.com>
> > Cc: Anna Schumaker <anna.schumaker@netapp.com>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  net/sunrpc/rpc_pipe.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
> > index 979d23646e33..917c85f15a0b 100644
> > --- a/net/sunrpc/rpc_pipe.c
> > +++ b/net/sunrpc/rpc_pipe.c
> > @@ -597,6 +597,8 @@ static int __rpc_rmdir(struct inode *dir,
> > struct dentry *dentry)
> > 
> >         dget(dentry);
> >         ret = simple_rmdir(dir, dentry);
> > +       if (!ret)
> > +               fsnotify_rmdir(dir, dentry);
> >         d_delete(dentry);
> >         dput(dentry);
> >         return ret;
> > @@ -608,6 +610,8 @@ static int __rpc_unlink(struct inode *dir,
> > struct dentry *dentry)
> > 
> >         dget(dentry);
> >         ret = simple_unlink(dir, dentry);
> > +       if (!ret)
> > +               fsnotify_unlink(dir, dentry);
> >         d_delete(dentry);
> >         dput(dentry);
> >         return ret;
> > --
> > 2.17.1
> > 

This looks just fine to me.

Acked-by: Trond Myklebust <trond.myklebust@hammerspace.com>


-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH v3 06/10] debugfs: simplify __debugfs_remove_file()

[Greg KH]

On Thu, May 30, 2019 at 08:49:52AM +0300, Amir Goldstein wrote:
> On Sun, May 26, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > Move simple_unlink()+d_delete() from __debugfs_remove_file() into
> > caller __debugfs_remove() and rename helper for post remove file to
> > __debugfs_file_removed().
> >
> > This will simplify adding fsnotify_unlink() hook.
> >
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> 
> Hi Greg,
> 
> Will be you be able to provide an ACK on this debugfs patch and the next one.

It's only been 4 days, and I'm traveling this week, give me a chance...

Re: [PATCH v3 06/10] debugfs: simplify __debugfs_remove_file()

[Greg Kroah-Hartman]

On Sun, May 26, 2019 at 05:34:07PM +0300, Amir Goldstein wrote:
> Move simple_unlink()+d_delete() from __debugfs_remove_file() into
> caller __debugfs_remove() and rename helper for post remove file to
> __debugfs_file_removed().
> 
> This will simplify adding fsnotify_unlink() hook.
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/debugfs/inode.c | 20 ++++++++------------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index acef14ad53db..d89874da9791 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -617,13 +617,10 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
>  }
>  EXPORT_SYMBOL_GPL(debugfs_create_symlink);
>  
> -static void __debugfs_remove_file(struct dentry *dentry, struct dentry *parent)
> +static void __debugfs_file_removed(struct dentry *dentry)
>  {
>  	struct debugfs_fsdata *fsd;
>  
> -	simple_unlink(d_inode(parent), dentry);
> -	d_delete(dentry);
> -
>  	/*
>  	 * Paired with the closing smp_mb() implied by a successful
>  	 * cmpxchg() in debugfs_file_get(): either
> @@ -644,16 +641,15 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent)
>  
>  	if (simple_positive(dentry)) {
>  		dget(dentry);
> -		if (!d_is_reg(dentry)) {
> -			if (d_is_dir(dentry))
> -				ret = simple_rmdir(d_inode(parent), dentry);
> -			else
> -				simple_unlink(d_inode(parent), dentry);
> -			if (!ret)
> -				d_delete(dentry);
> +		if (d_is_dir(dentry)) {
> +			ret = simple_rmdir(d_inode(parent), dentry);
>  		} else {
> -			__debugfs_remove_file(dentry, parent);
> +			simple_unlink(d_inode(parent), dentry);
>  		}
> +		if (!ret)
> +			d_delete(dentry);
> +		if (d_is_reg(dentry))
> +			__debugfs_file_removed(dentry);
>  		dput(dentry);
>  	}
>  	return ret;

Ugh, I had to stare at this for a long time.  I _think_ it all looks
equalivant now, but if this breaks, I know who to blame :)

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v3 07/10] debugfs: call fsnotify_{unlink,rmdir}() hooks

[Greg Kroah-Hartman]

On Sun, May 26, 2019 at 05:34:08PM +0300, Amir Goldstein wrote:
> This will allow generating fsnotify delete events after the
> fsnotify_nameremove() hook is removed from d_delete().
> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/debugfs/inode.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
> index d89874da9791..1e444fe1f778 100644
> --- a/fs/debugfs/inode.c
> +++ b/fs/debugfs/inode.c
> @@ -643,8 +643,11 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent)
>  		dget(dentry);
>  		if (d_is_dir(dentry)) {
>  			ret = simple_rmdir(d_inode(parent), dentry);
> +			if (!ret)
> +				fsnotify_rmdir(d_inode(parent), dentry);
>  		} else {
>  			simple_unlink(d_inode(parent), dentry);
> +			fsnotify_unlink(d_inode(parent), dentry);
>  		}
>  		if (!ret)
>  			d_delete(dentry);
> -- 
> 2.17.1

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH v3 04/10] tracefs: call fsnotify_{unlink,rmdir}() hooks

[Amir Goldstein]

On Sun, May 26, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> This will allow generating fsnotify delete events after the
> fsnotify_nameremove() hook is removed from d_delete().
>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Hi Steven,

Would you be able to provide an ACK on this patch?
We need to add those explicit fsnotify hooks to match the existing
fsnotify_create/mkdir hooks in tracefs, because
the hook embedded inside d_delete() is going away [1].

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/20190526143411.11244-1-amir73il@gmail.com/


> ---
>  fs/tracefs/inode.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
> index 7098c49f3693..497a8682b5b9 100644
> --- a/fs/tracefs/inode.c
> +++ b/fs/tracefs/inode.c
> @@ -509,9 +509,12 @@ static int __tracefs_remove(struct dentry *dentry, struct dentry *parent)
>                         switch (dentry->d_inode->i_mode & S_IFMT) {
>                         case S_IFDIR:
>                                 ret = simple_rmdir(parent->d_inode, dentry);
> +                               if (!ret)
> +                                       fsnotify_rmdir(parent->d_inode, dentry);
>                                 break;
>                         default:
>                                 simple_unlink(parent->d_inode, dentry);
> +                               fsnotify_unlink(parent->d_inode, dentry);
>                                 break;
>                         }
>                         if (!ret)
> --
> 2.17.1
>

Re: [PATCH v3 08/10] configfs: call fsnotify_rmdir() hook

[Amir Goldstein]

On Thu, May 30, 2019 at 9:06 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Sun, May 26, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > This will allow generating fsnotify delete events on unregister
> > of group/subsystem after the fsnotify_nameremove() hook is removed
> > from d_delete().
> >
> > The rest of the d_delete() calls from this filesystem are either
> > called recursively from within debugfs_unregister_{group,subsystem},
> > called from a vfs function that already has delete hooks or are
> > called from shutdown/cleanup code.
> >
> > Cc: Joel Becker <jlbec@evilplan.org>
> > Cc: Christoph Hellwig <hch@lst.de>
>
> Hi Christoph and Joel,
>
> Per Christoph's request, I cc'ed you guys on the entire patch series
> for context,
> so my discussion with Greg [1] about the special status of configfs in
> this patch set
> should already be somewhere in your mailboxes...
>
> Could I ask you to provide an ACK on this patch and on the chosen

PING.
Any comment from configfs maintainers?

Jan,

We can also drop this patch from the series given that configs has no
explicit fsnotify create hooks. If users come shouting, we can add
the missing create and delete hooks.

Thanks,
Amir.

> policy. To recap:
> Before patch set:
> 1. User gets create/delete events when files/dirs created/removed via vfs_*()
> 2. User does *not* get create events when files/dirs created via
> debugfs_register_*()
> 3. User *does* get delete events when files/dirs removed via
> debugfs_unregister_*()
>
> After patch set:
> 1. No change
> 2. No change
> 3. User will get delete events only on the root group/subsystem dir
> when tree is removed via debugfs_unregister_*()
>
> For symmetry, we could also add create events for  root group/subsystem dir
> when tree is created via debugfs_unregister_*(), but that would be a
> followup patch.
> For users though, it may be that delete events are more important than
> create events
> (i.e. for user cleanup tasks).
>
> Thanks,
> Amir.
>
> [1] https://lore.kernel.org/linux-fsdevel/CAOQ4uxjyg5AVPrcR4bPm4zMY9BKmgV8g7TAuH--cfKNJv8pRYQ@mail.gmail.com/
>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/configfs/dir.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> > index 5e7932d668ab..ba17881a8d84 100644
> > --- a/fs/configfs/dir.c
> > +++ b/fs/configfs/dir.c
> > @@ -27,6 +27,7 @@
> >  #undef DEBUG
> >
> >  #include <linux/fs.h>
> > +#include <linux/fsnotify.h>
> >  #include <linux/mount.h>
> >  #include <linux/module.h>
> >  #include <linux/slab.h>
> > @@ -1804,6 +1805,7 @@ void configfs_unregister_group(struct config_group *group)
> >         configfs_detach_group(&group->cg_item);
> >         d_inode(dentry)->i_flags |= S_DEAD;
> >         dont_mount(dentry);
> > +       fsnotify_rmdir(d_inode(parent), dentry);
> >         d_delete(dentry);
> >         inode_unlock(d_inode(parent));
> >
> > @@ -1932,6 +1934,7 @@ void configfs_unregister_subsystem(struct configfs_subsystem *subsys)
> >         configfs_detach_group(&group->cg_item);
> >         d_inode(dentry)->i_flags |= S_DEAD;
> >         dont_mount(dentry);
> > +       fsnotify_rmdir(d_inode(root), dentry);
> >         inode_unlock(d_inode(dentry));
> >
> >         d_delete(dentry);
> > --
> > 2.17.1
> >

Re: [PATCH v3 04/10] tracefs: call fsnotify_{unlink,rmdir}() hooks

[Steven Rostedt]

On Thu, 13 Jun 2019 19:53:25 +0300
Amir Goldstein <amir73il@gmail.com> wrote:

> On Sun, May 26, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
> >
> > This will allow generating fsnotify delete events after the
> > fsnotify_nameremove() hook is removed from d_delete().
> >
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>  
> 
> Hi Steven,
> 
> Would you be able to provide an ACK on this patch?
> We need to add those explicit fsnotify hooks to match the existing
> fsnotify_create/mkdir hooks in tracefs, because
> the hook embedded inside d_delete() is going away [1].
> 
> Thanks,
> Amir.
> 
> [1] https://lore.kernel.org/linux-fsdevel/20190526143411.11244-1-amir73il@gmail.com/
> 

Sorry, this got lost in my INBOX. I see it was already merged, but I
would have acked it ;-)

-- Steve

Re: [PATCH v3 04/10] tracefs: call fsnotify_{unlink,rmdir}() hooks

[Jan Kara]

On Fri 30-08-19 15:48:04, Steven Rostedt wrote:
> On Thu, 13 Jun 2019 19:53:25 +0300
> Amir Goldstein <amir73il@gmail.com> wrote:
> 
> > On Sun, May 26, 2019 at 5:34 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > This will allow generating fsnotify delete events after the
> > > fsnotify_nameremove() hook is removed from d_delete().
> > >
> > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>  
> > 
> > Hi Steven,
> > 
> > Would you be able to provide an ACK on this patch?
> > We need to add those explicit fsnotify hooks to match the existing
> > fsnotify_create/mkdir hooks in tracefs, because
> > the hook embedded inside d_delete() is going away [1].
> > 
> > Thanks,
> > Amir.
> > 
> > [1] https://lore.kernel.org/linux-fsdevel/20190526143411.11244-1-amir73il@gmail.com/
> > 
> 
> Sorry, this got lost in my INBOX. I see it was already merged, but I
> would have acked it ;-)

Thanks for the dealayed ack. I've used my best judgement and decided that
you probably would not object when pushing the patch :).
								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR