[RFC][PATCH 0/4] Sort out fsnotify_nameremove() mess

[RFC][PATCH 0/4] Sort out fsnotify_nameremove() mess

[Amir Goldstein]

Jan,

I started out working on your suggestion [1] of annotating
simple_unlink/rmdir_notify() callers when I realized we could
do better (IMO).

Please see this RFC. If you like the idea, I can split patches 3-4
to per filesystem patches and a final patch to make the switch from
fsnotify_nameremove() to fsnotify_remove().

I audited all the d_delete() call sites that will NOT generate
fsnotify events after these changes and noted to myself why that
makes sense.  I will include those notes in next posting if this
works out for you.

Note that configfs got a special treatment, because its helpers
that call simple_unlink/rmdir() are called from both vfs_XXX code
path and non vfs_XXX code path.

Thanks,
Amir.

[1] https://lore.kernel.org/linux-fsdevel/20190513163309.GE13297@quack2.suse.cz/

Amir Goldstein (4):
  fs: create simple_remove() helper
  fsnotify: add empty fsnotify_remove() hook
  fs: convert filesystems to use simple_remove() helper
  fsnotify: move fsnotify_nameremove() hook out of d_delete()

 arch/s390/hypfs/inode.c            |  9 ++-----
 drivers/infiniband/hw/qib/qib_fs.c |  3 +--
 fs/afs/dir_silly.c                 |  5 ----
 fs/btrfs/ioctl.c                   |  4 ++-
 fs/configfs/dir.c                  |  3 +++
 fs/dcache.c                        |  2 --
 fs/debugfs/inode.c                 | 20 +++------------
 fs/devpts/inode.c                  |  1 +
 fs/libfs.c                         | 25 ++++++++++++++++++
 fs/namei.c                         |  2 ++
 fs/nfs/unlink.c                    |  6 -----
 fs/notify/fsnotify.c               | 41 ------------------------------
 fs/tracefs/inode.c                 | 23 +++--------------
 include/linux/fs.h                 |  1 +
 include/linux/fsnotify.h           | 18 +++++++++++++
 include/linux/fsnotify_backend.h   |  4 ---
 net/sunrpc/rpc_pipe.c              | 16 ++----------
 security/apparmor/apparmorfs.c     |  6 +----
 18 files changed, 67 insertions(+), 122 deletions(-)

-- 
2.17.1

[RFC][PATCH 1/4] fs: create simple_remove() helper

[Amir Goldstein]

There is a common pattern among pseudo filesystems for removing a dentry
from code paths that are NOT coming from vfs_{unlink,rmdir}, using a
combination of simple_{unlink,rmdir} and d_delete().

Create an helper to perform this common operation.  This helper is going
to be used as a place holder for the new fsnotify_remove() hook.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/libfs.c         | 22 ++++++++++++++++++++++
 include/linux/fs.h |  1 +
 2 files changed, 23 insertions(+)

diff --git a/fs/libfs.c b/fs/libfs.c
index 4b59b1816efb..030e67c52b5f 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -353,6 +353,28 @@ int simple_rmdir(struct inode *dir, struct dentry *dentry)
 }
 EXPORT_SYMBOL(simple_rmdir);
 
+/*
+ * Unlike simple_unlink/rmdir, this helper is NOT called from vfs_unlink/rmdir.
+ * Caller must guaranty that d_parent and d_name are stable.
+ */
+int simple_remove(struct inode *dir, struct dentry *dentry)
+{
+	int ret;
+
+	dget(dentry);
+	if (d_is_dir(dentry))
+		ret = simple_rmdir(dir, dentry);
+	else
+		ret = simple_unlink(dir, dentry);
+
+	if (!ret)
+		d_delete(dentry);
+	dput(dentry);
+
+	return ret;
+}
+EXPORT_SYMBOL(simple_remove);
+
 int simple_rename(struct inode *old_dir, struct dentry *old_dentry,
 		  struct inode *new_dir, struct dentry *new_dentry,
 		  unsigned int flags)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index f7fdfe93e25d..74ea5f0b3b9d 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3245,6 +3245,7 @@ extern int simple_open(struct inode *inode, struct file *file);
 extern int simple_link(struct dentry *, struct inode *, struct dentry *);
 extern int simple_unlink(struct inode *, struct dentry *);
 extern int simple_rmdir(struct inode *, struct dentry *);
+extern int simple_remove(struct inode *, struct dentry *);
 extern int simple_rename(struct inode *, struct dentry *,
 			 struct inode *, struct dentry *, unsigned int);
 extern int noop_fsync(struct file *, loff_t, loff_t, int);
-- 
2.17.1

[RFC][PATCH 2/4] fsnotify: add empty fsnotify_remove() hook

[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 an empty hook called fsnotify_remove() and place
it in the proper VFS call sites.  After all d_delete() call sites
will be converted to use the new hook, it will replace the old
fsnotify_nameremove() hook in d_delete().

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/libfs.c               |  5 ++++-
 fs/namei.c               |  2 ++
 include/linux/fsnotify.h | 13 +++++++++++++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index 030e67c52b5f..0dd676fc9272 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -10,6 +10,7 @@
 #include <linux/cred.h>
 #include <linux/mount.h>
 #include <linux/vfs.h>
+#include <linux/fsnotify.h>
 #include <linux/quotaops.h>
 #include <linux/mutex.h>
 #include <linux/namei.h>
@@ -367,8 +368,10 @@ int simple_remove(struct inode *dir, struct dentry *dentry)
 	else
 		ret = simple_unlink(dir, dentry);
 
-	if (!ret)
+	if (!ret) {
+		fsnotify_remove(dir, dentry);
 		d_delete(dentry);
+	}
 	dput(dentry);
 
 	return ret;
diff --git a/fs/namei.c b/fs/namei.c
index 20831c2fbb34..c9eda9cc5d43 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_remove(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_remove(dir, dentry);
 			}
 		}
 	}
diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index 94972e8eb6d1..455dff82595e 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -151,6 +151,19 @@ static inline void fsnotify_vfsmount_delete(struct vfsmount *mnt)
 	__fsnotify_vfsmount_delete(mnt);
 }
 
+/*
+ * fsnotify_remove - a filename was removed from a directory
+ *
+ * Caller must make sure that dentry->d_name is stable.
+ */
+static inline void fsnotify_remove(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_inoderemove - an inode is going away
  */
-- 
2.17.1

[RFC][PATCH 3/4] fs: convert filesystems to use simple_remove() helper

[Amir Goldstein]

Convert some filesystems to use the new simple_remove() helper.
This will allow them to generate fsnotify delete events after the
fsnotify_nameremove() hook is removed from d_delete().

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 arch/s390/hypfs/inode.c            |  9 ++-------
 drivers/infiniband/hw/qib/qib_fs.c |  3 +--
 fs/debugfs/inode.c                 | 20 ++++----------------
 fs/tracefs/inode.c                 | 23 ++++-------------------
 net/sunrpc/rpc_pipe.c              | 16 ++--------------
 security/apparmor/apparmorfs.c     |  6 +-----
 6 files changed, 14 insertions(+), 63 deletions(-)

diff --git a/arch/s390/hypfs/inode.c b/arch/s390/hypfs/inode.c
index ccad1398abd4..30a0ab967461 100644
--- a/arch/s390/hypfs/inode.c
+++ b/arch/s390/hypfs/inode.c
@@ -70,13 +70,8 @@ static void hypfs_remove(struct dentry *dentry)
 
 	parent = dentry->d_parent;
 	inode_lock(d_inode(parent));
-	if (simple_positive(dentry)) {
-		if (d_is_dir(dentry))
-			simple_rmdir(d_inode(parent), dentry);
-		else
-			simple_unlink(d_inode(parent), dentry);
-	}
-	d_delete(dentry);
+	if (simple_positive(dentry))
+		simple_remove(d_inode(parent), dentry);
 	dput(dentry);
 	inode_unlock(d_inode(parent));
 }
diff --git a/drivers/infiniband/hw/qib/qib_fs.c b/drivers/infiniband/hw/qib/qib_fs.c
index ceb42d948412..795938a2488b 100644
--- a/drivers/infiniband/hw/qib/qib_fs.c
+++ b/drivers/infiniband/hw/qib/qib_fs.c
@@ -491,8 +491,7 @@ static int remove_device_files(struct super_block *sb,
 	}
 	remove_file(dir, "flash");
 	inode_unlock(d_inode(dir));
-	ret = simple_rmdir(d_inode(root), dir);
-	d_delete(dir);
+	ret = simple_remove(d_inode(root), dir);
 	dput(dir);
 
 bail:
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index acef14ad53db..bc96198df1d4 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
@@ -643,18 +640,9 @@ static int __debugfs_remove(struct dentry *dentry, struct dentry *parent)
 	int ret = 0;
 
 	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);
-		} else {
-			__debugfs_remove_file(dentry, parent);
-		}
-		dput(dentry);
+		ret = simple_remove(d_inode(parent), dentry);
+		if (d_is_reg(dentry))
+			__debugfs_file_removed(dentry);
 	}
 	return ret;
 }
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index 7098c49f3693..6ac31ea9ad5d 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -501,25 +501,10 @@ __init struct dentry *tracefs_create_instance_dir(const char *name,
 
 static int __tracefs_remove(struct dentry *dentry, struct dentry *parent)
 {
-	int ret = 0;
-
-	if (simple_positive(dentry)) {
-		if (dentry->d_inode) {
-			dget(dentry);
-			switch (dentry->d_inode->i_mode & S_IFMT) {
-			case S_IFDIR:
-				ret = simple_rmdir(parent->d_inode, dentry);
-				break;
-			default:
-				simple_unlink(parent->d_inode, dentry);
-				break;
-			}
-			if (!ret)
-				d_delete(dentry);
-			dput(dentry);
-		}
-	}
-	return ret;
+	if (simple_positive(dentry))
+		return simple_remove(d_inode(parent), dentry);
+
+	return 0;
 }
 
 /**
diff --git a/net/sunrpc/rpc_pipe.c b/net/sunrpc/rpc_pipe.c
index 979d23646e33..5b1a59776b9a 100644
--- a/net/sunrpc/rpc_pipe.c
+++ b/net/sunrpc/rpc_pipe.c
@@ -593,24 +593,12 @@ static int __rpc_mkpipe_dentry(struct inode *dir, struct dentry *dentry,
 
 static int __rpc_rmdir(struct inode *dir, struct dentry *dentry)
 {
-	int ret;
-
-	dget(dentry);
-	ret = simple_rmdir(dir, dentry);
-	d_delete(dentry);
-	dput(dentry);
-	return ret;
+	return simple_remove(dir, dentry);
 }
 
 static int __rpc_unlink(struct inode *dir, struct dentry *dentry)
 {
-	int ret;
-
-	dget(dentry);
-	ret = simple_unlink(dir, dentry);
-	d_delete(dentry);
-	dput(dentry);
-	return ret;
+	return simple_remove(dir, dentry);
 }
 
 static int __rpc_rmpipe(struct inode *dir, struct dentry *dentry)
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 9ab5613fe07c..4a10acb4a6d3 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -351,11 +351,7 @@ static void aafs_remove(struct dentry *dentry)
 	dir = d_inode(dentry->d_parent);
 	inode_lock(dir);
 	if (simple_positive(dentry)) {
-		if (d_is_dir(dentry))
-			simple_rmdir(dir, dentry);
-		else
-			simple_unlink(dir, dentry);
-		d_delete(dentry);
+		simple_remove(dir, dentry);
 		dput(dentry);
 	}
 	inode_unlock(dir);
-- 
2.17.1

[RFC][PATCH 4/4] 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 that parent dir and d_name are stable and
we call the fsnotify_remove() hook before calling d_delete().
Because of that, fsnotify_remove() does not need the safety measures
that were in fsnotify_nameremove() to stabilize parent and name.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/afs/dir_silly.c               |  5 ----
 fs/btrfs/ioctl.c                 |  4 +++-
 fs/configfs/dir.c                |  3 +++
 fs/dcache.c                      |  2 --
 fs/devpts/inode.c                |  1 +
 fs/nfs/unlink.c                  |  6 -----
 fs/notify/fsnotify.c             | 41 --------------------------------
 include/linux/fsnotify.h         |  7 +++++-
 include/linux/fsnotify_backend.h |  4 ----
 9 files changed, 13 insertions(+), 60 deletions(-)

diff --git a/fs/afs/dir_silly.c b/fs/afs/dir_silly.c
index f6f89fdab6b2..d3494825d08a 100644
--- a/fs/afs/dir_silly.c
+++ b/fs/afs/dir_silly.c
@@ -57,11 +57,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);
 	}
 
 	_leave(" = %d", ret);
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 6dafa857bbb9..cd76e705d401 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_remove(dir, dentry);
 		d_delete(dentry);
+	}
 
 out_dput:
 	dput(dentry);
diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
index 591e82ba443c..78566002234a 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>
@@ -1797,6 +1798,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_remove(d_inode(parent), dentry);
 	d_delete(dentry);
 	inode_unlock(d_inode(parent));
 
@@ -1925,6 +1927,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_remove(d_inode(root), dentry);
 	inode_unlock(d_inode(dentry));
 
 	d_delete(dentry);
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/devpts/inode.c b/fs/devpts/inode.c
index 553a3f3300ae..aea8dda154e2 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_remove(d_inode(dentry->d_parent), dentry);
 	d_delete(dentry);
 	dput(dentry);	/* d_alloc_name() in devpts_pty_new() */
 }
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/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 455dff82595e..7f68cb9825dd 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -158,10 +158,15 @@ static inline void fsnotify_vfsmount_delete(struct vfsmount *mnt)
  */
 static inline void fsnotify_remove(struct inode *dir, struct dentry *dentry)
 {
+	__u32 mask = FS_DELETE;
+
 	/* Expected to be called before d_delete() */
 	WARN_ON_ONCE(d_is_negative(dentry));
 
-	/* TODO: call fsnotify_dirent() */
+	if (d_is_dir(dentry))
+		mask |= FS_ISDIR;
+
+	fsnotify_dirent(dir, dentry, mask);
 }
 
 /*
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: [RFC][PATCH 1/4] fs: create simple_remove() helper

[Jan Kara]

On Wed 15-05-19 01:18:58, Amir Goldstein wrote:
> There is a common pattern among pseudo filesystems for removing a dentry
> from code paths that are NOT coming from vfs_{unlink,rmdir}, using a
> combination of simple_{unlink,rmdir} and d_delete().
> 
> Create an helper to perform this common operation.  This helper is going
> to be used as a place holder for the new fsnotify_remove() hook.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Looks like a good idea. One comment below:

> +/*
> + * Unlike simple_unlink/rmdir, this helper is NOT called from vfs_unlink/rmdir.
> + * Caller must guaranty that d_parent and d_name are stable.
> + */
> +int simple_remove(struct inode *dir, struct dentry *dentry)
> +{
> +	int ret;
> +
> +	dget(dentry);
> +	if (d_is_dir(dentry))
> +		ret = simple_rmdir(dir, dentry);
> +	else
> +		ret = simple_unlink(dir, dentry);
> +
> +	if (!ret)
> +		d_delete(dentry);
> +	dput(dentry);

This dget() - dput() pair looks fishy. After some research I understand why
it's needed but I think it deserves a comment like:

	/*
	 * 'simple_' operations get dentry reference on create/mkdir and drop
	 * it on unlink/rmdir. So we have to get dentry reference here to
	 * protect d_delete() from accessing freed dentry.
	 */

								Honza

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

Re: [RFC][PATCH 2/4] fsnotify: add empty fsnotify_remove() hook

[Jan Kara]

On Wed 15-05-19 01:18:59, Amir Goldstein wrote:
> 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 an empty hook called fsnotify_remove() and place
> it in the proper VFS call sites.  After all d_delete() call sites
> will be converted to use the new hook, it will replace the old
> fsnotify_nameremove() hook in d_delete().
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Hum, I would just fold this into patch 4. It is not that big and I don't
think it adds to the clarity of the whole series. Rather on contrary it
makes verifying that we didn't miss any conversion harder... Also that way
we don't have to rename the fsnotify hook.

BTW, you seem to have forgotten to provide an empty stub for
!CONFIG_FSNOTIFY case here.

								Honza

> ---
>  fs/libfs.c               |  5 ++++-
>  fs/namei.c               |  2 ++
>  include/linux/fsnotify.h | 13 +++++++++++++
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/libfs.c b/fs/libfs.c
> index 030e67c52b5f..0dd676fc9272 100644
> --- a/fs/libfs.c
> +++ b/fs/libfs.c
> @@ -10,6 +10,7 @@
>  #include <linux/cred.h>
>  #include <linux/mount.h>
>  #include <linux/vfs.h>
> +#include <linux/fsnotify.h>
>  #include <linux/quotaops.h>
>  #include <linux/mutex.h>
>  #include <linux/namei.h>
> @@ -367,8 +368,10 @@ int simple_remove(struct inode *dir, struct dentry *dentry)
>  	else
>  		ret = simple_unlink(dir, dentry);
>  
> -	if (!ret)
> +	if (!ret) {
> +		fsnotify_remove(dir, dentry);
>  		d_delete(dentry);
> +	}
>  	dput(dentry);
>  
>  	return ret;
> diff --git a/fs/namei.c b/fs/namei.c
> index 20831c2fbb34..c9eda9cc5d43 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_remove(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_remove(dir, dentry);
>  			}
>  		}
>  	}
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index 94972e8eb6d1..455dff82595e 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -151,6 +151,19 @@ static inline void fsnotify_vfsmount_delete(struct vfsmount *mnt)
>  	__fsnotify_vfsmount_delete(mnt);
>  }
>  
> +/*
> + * fsnotify_remove - a filename was removed from a directory
> + *
> + * Caller must make sure that dentry->d_name is stable.
> + */
> +static inline void fsnotify_remove(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_inoderemove - an inode is going away
>   */
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC][PATCH 4/4] fsnotify: move fsnotify_nameremove() hook out of d_delete()

[Jan Kara]

On Wed 15-05-19 01:19:01, Amir Goldstein wrote:
> 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 that parent dir and d_name are stable and
> we call the fsnotify_remove() hook before calling d_delete().
> Because of that, fsnotify_remove() does not need the safety measures
> that were in fsnotify_nameremove() to stabilize parent and name.

Looks good, some smaller comments below.

> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/afs/dir_silly.c               |  5 ----
>  fs/btrfs/ioctl.c                 |  4 +++-
>  fs/configfs/dir.c                |  3 +++
>  fs/dcache.c                      |  2 --
>  fs/devpts/inode.c                |  1 +
>  fs/nfs/unlink.c                  |  6 -----
>  fs/notify/fsnotify.c             | 41 --------------------------------
>  include/linux/fsnotify.h         |  7 +++++-
>  include/linux/fsnotify_backend.h |  4 ----
>  9 files changed, 13 insertions(+), 60 deletions(-)
> 
> diff --git a/fs/afs/dir_silly.c b/fs/afs/dir_silly.c
> index f6f89fdab6b2..d3494825d08a 100644
> --- a/fs/afs/dir_silly.c
> +++ b/fs/afs/dir_silly.c
> @@ -57,11 +57,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);
>  	}
>  
>  	_leave(" = %d", ret);

This changes the behavior when rename happens to overwrite a file, doesn't
it? It is more consistent that way and I don't think anybody depends on it
so I agree but it might deserve a comment in the changelog.

> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 591e82ba443c..78566002234a 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -1797,6 +1798,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_remove(d_inode(parent), dentry);
>  	d_delete(dentry);
>  	inode_unlock(d_inode(parent));
>  
> @@ -1925,6 +1927,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_remove(d_inode(root), dentry);
>  	inode_unlock(d_inode(dentry));
>  
>  	d_delete(dentry);

Is his really needed? We have a call chain:
 configfs_detach_group()
   configfs_detach_item()
     configfs_remove_dir()
       remove_dir()
         simple_rmdir()

Ah, but this is the special configfs treatment you were speaking about as
configfs_detach_group() can get called also from vfs_rmdir(). I see. But
please separate this into a special patch with a good changelog.

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

Ditto as for AFS I assume...

> 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 455dff82595e..7f68cb9825dd 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -158,10 +158,15 @@ static inline void fsnotify_vfsmount_delete(struct vfsmount *mnt)
>   */
>  static inline void fsnotify_remove(struct inode *dir, struct dentry *dentry)
>  {
> +	__u32 mask = FS_DELETE;
> +
>  	/* Expected to be called before d_delete() */
>  	WARN_ON_ONCE(d_is_negative(dentry));
>  
> -	/* TODO: call fsnotify_dirent() */
> +	if (d_is_dir(dentry))
> +		mask |= FS_ISDIR;
> +
> +	fsnotify_dirent(dir, dentry, mask);
>  }

With folding patch 2 into this patch, I'd leave fsnotify changes for a
separate patch. I.e., keep fsnotify_nameremove() as is in this patch just
change its callsites and then simplify fsnotify_nameremove() in the next
patch.

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

Re: [RFC][PATCH 0/4] Sort out fsnotify_nameremove() mess

[Jan Kara]

Hi Amir!

On Wed 15-05-19 01:18:57, Amir Goldstein wrote:
> I started out working on your suggestion [1] of annotating
> simple_unlink/rmdir_notify() callers when I realized we could
> do better (IMO).
> 
> Please see this RFC. If you like the idea, I can split patches 3-4
> to per filesystem patches and a final patch to make the switch from
> fsnotify_nameremove() to fsnotify_remove().
> 
> I audited all the d_delete() call sites that will NOT generate
> fsnotify events after these changes and noted to myself why that
> makes sense.  I will include those notes in next posting if this
> works out for you.
> 
> Note that configfs got a special treatment, because its helpers
> that call simple_unlink/rmdir() are called from both vfs_XXX code
> path and non vfs_XXX code path.

Thanks for working on this! I like the series. I would structure it
somewhat differently - see my comments to individual patches - but the end
result looks OK to me. And yes, notes about d_delete() call sites would
make it easier to verify that we didn't miss anything :) so I'd be happy if
you posted them.

								Honza

> 
> Thanks,
> Amir.
> 
> [1] https://lore.kernel.org/linux-fsdevel/20190513163309.GE13297@quack2.suse.cz/
> 
> Amir Goldstein (4):
>   fs: create simple_remove() helper
>   fsnotify: add empty fsnotify_remove() hook
>   fs: convert filesystems to use simple_remove() helper
>   fsnotify: move fsnotify_nameremove() hook out of d_delete()
> 
>  arch/s390/hypfs/inode.c            |  9 ++-----
>  drivers/infiniband/hw/qib/qib_fs.c |  3 +--
>  fs/afs/dir_silly.c                 |  5 ----
>  fs/btrfs/ioctl.c                   |  4 ++-
>  fs/configfs/dir.c                  |  3 +++
>  fs/dcache.c                        |  2 --
>  fs/debugfs/inode.c                 | 20 +++------------
>  fs/devpts/inode.c                  |  1 +
>  fs/libfs.c                         | 25 ++++++++++++++++++
>  fs/namei.c                         |  2 ++
>  fs/nfs/unlink.c                    |  6 -----
>  fs/notify/fsnotify.c               | 41 ------------------------------
>  fs/tracefs/inode.c                 | 23 +++--------------
>  include/linux/fs.h                 |  1 +
>  include/linux/fsnotify.h           | 18 +++++++++++++
>  include/linux/fsnotify_backend.h   |  4 ---
>  net/sunrpc/rpc_pipe.c              | 16 ++----------
>  security/apparmor/apparmorfs.c     |  6 +----
>  18 files changed, 67 insertions(+), 122 deletions(-)
> 
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [RFC][PATCH 4/4] fsnotify: move fsnotify_nameremove() hook out of d_delete()

[Amir Goldstein]

On Wed, May 15, 2019 at 11:24 AM Jan Kara <jack@suse.cz> wrote:
>
> On Wed 15-05-19 01:19:01, Amir Goldstein wrote:
> > 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 that parent dir and d_name are stable and
> > we call the fsnotify_remove() hook before calling d_delete().
> > Because of that, fsnotify_remove() does not need the safety measures
> > that were in fsnotify_nameremove() to stabilize parent and name.
>
> Looks good, some smaller comments below.
>
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > ---
> >  fs/afs/dir_silly.c               |  5 ----
> >  fs/btrfs/ioctl.c                 |  4 +++-
> >  fs/configfs/dir.c                |  3 +++
> >  fs/dcache.c                      |  2 --
> >  fs/devpts/inode.c                |  1 +
> >  fs/nfs/unlink.c                  |  6 -----
> >  fs/notify/fsnotify.c             | 41 --------------------------------
> >  include/linux/fsnotify.h         |  7 +++++-
> >  include/linux/fsnotify_backend.h |  4 ----
> >  9 files changed, 13 insertions(+), 60 deletions(-)
> >
> > diff --git a/fs/afs/dir_silly.c b/fs/afs/dir_silly.c
> > index f6f89fdab6b2..d3494825d08a 100644
> > --- a/fs/afs/dir_silly.c
> > +++ b/fs/afs/dir_silly.c
> > @@ -57,11 +57,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);
> >       }
> >
> >       _leave(" = %d", ret);
>
> This changes the behavior when rename happens to overwrite a file, doesn't
> it? It is more consistent that way and I don't think anybody depends on it
> so I agree but it might deserve a comment in the changelog.

Right. Good spotting. This is very inconsistent...

>
> > diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> > index 591e82ba443c..78566002234a 100644
> > --- a/fs/configfs/dir.c
> > +++ b/fs/configfs/dir.c
> > @@ -1797,6 +1798,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_remove(d_inode(parent), dentry);
> >       d_delete(dentry);
> >       inode_unlock(d_inode(parent));
> >
> > @@ -1925,6 +1927,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_remove(d_inode(root), dentry);
> >       inode_unlock(d_inode(dentry));
> >
> >       d_delete(dentry);
>
> Is his really needed? We have a call chain:
>  configfs_detach_group()
>    configfs_detach_item()
>      configfs_remove_dir()
>        remove_dir()
>          simple_rmdir()
>
> Ah, but this is the special configfs treatment you were speaking about as
> configfs_detach_group() can get called also from vfs_rmdir(). I see. But
> please separate this into a special patch with a good changelog.

OK. I prefer a separate patch per filesystem even for trivial cases like
btrfs, but in order to do that I need to use the empty hook technique.
I will try to convince you in favor of empty hook in reply to your comment.

>
> > 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);
> >  }
>
> Ditto as for AFS I assume...

Yap.

>
> > 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 455dff82595e..7f68cb9825dd 100644
> > --- a/include/linux/fsnotify.h
> > +++ b/include/linux/fsnotify.h
> > @@ -158,10 +158,15 @@ static inline void fsnotify_vfsmount_delete(struct vfsmount *mnt)
> >   */
> >  static inline void fsnotify_remove(struct inode *dir, struct dentry *dentry)
> >  {
> > +     __u32 mask = FS_DELETE;
> > +
> >       /* Expected to be called before d_delete() */
> >       WARN_ON_ONCE(d_is_negative(dentry));
> >
> > -     /* TODO: call fsnotify_dirent() */
> > +     if (d_is_dir(dentry))
> > +             mask |= FS_ISDIR;
> > +
> > +     fsnotify_dirent(dir, dentry, mask);
> >  }
>
> With folding patch 2 into this patch, I'd leave fsnotify changes for a
> separate patch. I.e., keep fsnotify_nameremove() as is in this patch just
> change its callsites and then simplify fsnotify_nameremove() in the next
> patch.
>

I agree we should leave simplifying fsontify hook to last patch, but
I *would* like to add new hook name(s) and discard the old hook, because:
1. I hate the moniker nameremove
2. fsnotify_nameremove() args are incompatible with similar hooks
3. Will allow me to write individual patches for btrfs, devpty, configfs
4. I'd like to suggest fsnotify_rmdir/fsnotify_unlink to pair with
    fsnotify_mkdir/fsnotify_create

- I can start with empty hooks.
- Then add new hooks to all chosen call sites
- Then move fsnotify_nameremove() from d_delete() into
  fsnotify_rmdir/fsnotify_unlink.
- Finally, simply fsnotify_rmdir/fsnotify_unlink to use fsnotify_dirent()
  and kill the complicated fsnotify_nameremove().

Thanks,
Amir.

Re: [RFC][PATCH 4/4] fsnotify: move fsnotify_nameremove() hook out of d_delete()

[Jan Kara]

On Wed 15-05-19 13:56:47, Amir Goldstein wrote:
> On Wed, May 15, 2019 at 11:24 AM Jan Kara <jack@suse.cz> wrote:
> > > 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 455dff82595e..7f68cb9825dd 100644
> > > --- a/include/linux/fsnotify.h
> > > +++ b/include/linux/fsnotify.h
> > > @@ -158,10 +158,15 @@ static inline void fsnotify_vfsmount_delete(struct vfsmount *mnt)
> > >   */
> > >  static inline void fsnotify_remove(struct inode *dir, struct dentry *dentry)
> > >  {
> > > +     __u32 mask = FS_DELETE;
> > > +
> > >       /* Expected to be called before d_delete() */
> > >       WARN_ON_ONCE(d_is_negative(dentry));
> > >
> > > -     /* TODO: call fsnotify_dirent() */
> > > +     if (d_is_dir(dentry))
> > > +             mask |= FS_ISDIR;
> > > +
> > > +     fsnotify_dirent(dir, dentry, mask);
> > >  }
> >
> > With folding patch 2 into this patch, I'd leave fsnotify changes for a
> > separate patch. I.e., keep fsnotify_nameremove() as is in this patch just
> > change its callsites and then simplify fsnotify_nameremove() in the next
> > patch.
> >
> 
> I agree we should leave simplifying fsontify hook to last patch, but
> I *would* like to add new hook name(s) and discard the old hook, because:
> 1. I hate the moniker nameremove
> 2. fsnotify_nameremove() args are incompatible with similar hooks
> 3. Will allow me to write individual patches for btrfs, devpty, configfs
> 4. I'd like to suggest fsnotify_rmdir/fsnotify_unlink to pair with
>     fsnotify_mkdir/fsnotify_create
> 
> - I can start with empty hooks.
> - Then add new hooks to all chosen call sites
> - Then move fsnotify_nameremove() from d_delete() into
>   fsnotify_rmdir/fsnotify_unlink.
> - Finally, simply fsnotify_rmdir/fsnotify_unlink to use fsnotify_dirent()
>   and kill the complicated fsnotify_nameremove().

This sounds OK to me as well.

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