[PATCH v2 00/14] Sort out fsnotify_nameremove() mess

[PATCH v2 00/14] Sort out fsnotify_nameremove() mess

[Amir Goldstein]

Jan,

What do you think will be the best merge strategy for this patch series?

How about sending first 3 prep patches to Linus for applying at the end
of v5.2 merge window, so individual maintainers can pick up per fs
patches for the v5.3 development cycle?

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

drivers/usb/gadget/function/f_fs.c:ffs_epfiles_destroy() - cleanup? (*)
fs/ceph/dir.c:ceph_unlink() - from vfs_unlink()
fs/ceph/inode.c:ceph_fill_trace() - invalidate (**)
fs/configfs/dir.c:detach_groups() - 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() - invalidate (**)
fs/ocfs2/dlmglue.c:ocfs2_dentry_convert_worker() - invalidate? (**)
fs/reiserfs/xattr.c:xattr_{unlink,rmdir}() - hidden xattr inode

(*) There are 2 "cleanup" use cases:
  - Create;init;delte 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 v1:
- Split up per filesystem patches
- New hook names fsnotify_{unlink,rmdir}()
- Simplify fsnotify code in separate final patch

Amir Goldstein (14):
  ASoC: rename functions that pollute the simple_xxx namespace
  fs: create simple_remove() helper
  fsnotify: add empty fsnotify_{unlink,rmdir}() hooks
  fs: convert hypfs to use simple_remove() helper
  fs: convert qibfs/ipathfs to use simple_remove() helper
  fs: convert debugfs to use simple_remove() helper
  fs: convert tracefs to use simple_remove() helper
  fs: convert rpc_pipefs to use simple_remove() helper
  fs: convert apparmorfs to use simple_remove() helper
  fsnotify: call fsnotify_rmdir() hook from btrfs
  fsnotify: call fsnotify_rmdir() hook from configfs
  fsnotify: call fsnotify_unlink() hook from devpts
  fsnotify: move fsnotify_nameremove() hook out of d_delete()
  fsnotify: get rid of fsnotify_nameremove()

 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                         | 32 +++++++++++++++++++++++
 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           | 26 +++++++++++++++++++
 include/linux/fsnotify_backend.h   |  4 ---
 net/sunrpc/rpc_pipe.c              | 16 ++----------
 security/apparmor/apparmorfs.c     |  6 +----
 sound/soc/generic/simple-card.c    |  8 +++---
 19 files changed, 86 insertions(+), 126 deletions(-)

-- 
2.17.1

[PATCH v2 01/14] ASoC: rename functions that pollute the simple_xxx namespace

[Amir Goldstein]

include/linux/fs.h defines a bunch of simple fs helpers, (e.g.
simple_rename) and we intend to add an fs helper named simple_remove.

Rename the ASoC driver static functions, so they will not collide with
the upcoming fs helper function name.

Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 sound/soc/generic/simple-card.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 9b568f578bcd..d16e894fce2b 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -607,7 +607,7 @@ static int simple_soc_probe(struct snd_soc_card *card)
 	return 0;
 }
 
-static int simple_probe(struct platform_device *pdev)
+static int asoc_simple_probe(struct platform_device *pdev)
 {
 	struct asoc_simple_priv *priv;
 	struct device *dev = &pdev->dev;
@@ -705,7 +705,7 @@ static int simple_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int simple_remove(struct platform_device *pdev)
+static int asoc_simple_remove(struct platform_device *pdev)
 {
 	struct snd_soc_card *card = platform_get_drvdata(pdev);
 
@@ -726,8 +726,8 @@ static struct platform_driver asoc_simple_card = {
 		.pm = &snd_soc_pm_ops,
 		.of_match_table = simple_of_match,
 	},
-	.probe = simple_probe,
-	.remove = simple_remove,
+	.probe = asoc_simple_probe,
+	.remove = asoc_simple_remove,
 };
 
 module_platform_driver(asoc_simple_card);
-- 
2.17.1

Re: [PATCH v2 01/14] ASoC: rename functions that pollute the simple_xxx namespace

[Kuninori Morimoto]

Hi

> include/linux/fs.h defines a bunch of simple fs helpers, (e.g.
> simple_rename) and we intend to add an fs helper named simple_remove.
> 
> Rename the ASoC driver static functions, so they will not collide with
> the upcoming fs helper function name.
> 
> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Cc: Mark Brown <broonie@kernel.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---

Acked-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Thank you for your help !!
Best regards
---
Kuninori Morimoto

Applied "ASoC: rename functions that pollute the simple_xxx namespace" to the asoc tree

[Mark Brown]

The patch

   ASoC: rename functions that pollute the simple_xxx namespace

has been applied to the asoc tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-5.3

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From b0a821daf0d04e5a8ae99829e24f2fe538f25763 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@gmail.com>
Date: Thu, 16 May 2019 13:26:28 +0300
Subject: [PATCH] ASoC: rename functions that pollute the simple_xxx namespace

include/linux/fs.h defines a bunch of simple fs helpers, (e.g.
simple_rename) and we intend to add an fs helper named simple_remove.

Rename the ASoC driver static functions, so they will not collide with
the upcoming fs helper function name.

Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Cc: Mark Brown <broonie@kernel.org>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 sound/soc/generic/simple-card.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 9b568f578bcd..d16e894fce2b 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -607,7 +607,7 @@ static int simple_soc_probe(struct snd_soc_card *card)
 	return 0;
 }
 
-static int simple_probe(struct platform_device *pdev)
+static int asoc_simple_probe(struct platform_device *pdev)
 {
 	struct asoc_simple_priv *priv;
 	struct device *dev = &pdev->dev;
@@ -705,7 +705,7 @@ static int simple_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int simple_remove(struct platform_device *pdev)
+static int asoc_simple_remove(struct platform_device *pdev)
 {
 	struct snd_soc_card *card = platform_get_drvdata(pdev);
 
@@ -726,8 +726,8 @@ static struct platform_driver asoc_simple_card = {
 		.pm = &snd_soc_pm_ops,
 		.of_match_table = simple_of_match,
 	},
-	.probe = simple_probe,
-	.remove = simple_remove,
+	.probe = asoc_simple_probe,
+	.remove = asoc_simple_remove,
 };
 
 module_platform_driver(asoc_simple_card);
-- 
2.20.1

[PATCH v2 02/14] 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_{unlink,rmdir} hooks.

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

diff --git a/fs/libfs.c b/fs/libfs.c
index 4b59b1816efb..ca1132f1d5c6 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -353,6 +353,33 @@ 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;
+
+	/*
+	 * 'simple_' operations get a 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 a freed dentry.
+	 */
+	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

Fwd: [PATCH v2 02/14] fs: create simple_remove() helper

[Amir Goldstein]

Dear pseudo fs maintainer,

Today you received a patch from me titled
"fs: convert <your>fs to use simple_remove() helper"

The patch that you received depends on this patch from my series
and this patch is obviously needed for the context of your review.

Please review the use of the helper as a cleanup/refactoring
patch for your fs that should not change logic regardless of the wider
scope of my series.

For complete context and motivation, please see the rest of the series at:
https://lore.kernel.org/linux-fsdevel/20190516102641.6574-1-amir73il@gmail.com/

Thanks,
Amir.

---------- Forwarded message ---------
From: Amir Goldstein <amir73il@gmail.com>
Date: Thu, May 16, 2019 at 1:26 PM
Subject: [PATCH v2 02/14] fs: create simple_remove() helper
To: Jan Kara <jack@suse.cz>
Cc: Matthew Bobrowski <mbobrowski@mbobrowski.org>,
<linux-fsdevel@vger.kernel.org>


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_{unlink,rmdir} hooks.

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

diff --git a/fs/libfs.c b/fs/libfs.c
index 4b59b1816efb..ca1132f1d5c6 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -353,6 +353,33 @@ 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;
+
+       /*
+        * 'simple_' operations get a 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 a freed dentry.
+        */
+       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

Re: [PATCH v2 02/14] fs: create simple_remove() helper

[Al Viro]

On Thu, May 16, 2019 at 01:26:29PM +0300, 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_{unlink,rmdir} hooks.

This is the wrong approach.  What we have is a bunch of places trying
to implement recursive removal of a subtree.  They are broken, each in
its own way, and I'm not talking about fsnotify crap - there are
much more unpleasant issues there.

Trying to untangle that mess is not going to be made easier by mandating
the calls of that helper of yours - if anything, it makes the whole
thing harder to massage.

It needs to be dealt with, no arguments here, but that's not a good
starting point for that...  I've taken several stabs at that, never
got anywhere satisfactory with those ;-/  I'll try to dig out the
notes/existing attempts at patch series; if you are willing to participate
in discussing those and sorting the whole thing out, you are very welcome;
just ping me in a couple of days.

Re: [PATCH v2 02/14] fs: create simple_remove() helper

[Amir Goldstein]

On Thu, May 16, 2019 at 8:07 PM Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> On Thu, May 16, 2019 at 01:26:29PM +0300, 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_{unlink,rmdir} hooks.
>
> This is the wrong approach.  What we have is a bunch of places trying
> to implement recursive removal of a subtree.  They are broken, each in
> its own way, and I'm not talking about fsnotify crap - there are
> much more unpleasant issues there.
>
> Trying to untangle that mess is not going to be made easier by mandating
> the calls of that helper of yours - if anything, it makes the whole
> thing harder to massage.
>
> It needs to be dealt with, no arguments here, but that's not a good
> starting point for that...  I've taken several stabs at that, never
> got anywhere satisfactory with those ;-/  I'll try to dig out the
> notes/existing attempts at patch series; if you are willing to participate
> in discussing those and sorting the whole thing out, you are very welcome;
> just ping me in a couple of days.

Will do.

Thanks,
Amir.

[PATCH v2 03/14] 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/libfs.c               | 11 ++++++++---
 fs/namei.c               |  2 ++
 include/linux/fsnotify.h | 26 ++++++++++++++++++++++++++
 3 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index ca1132f1d5c6..4db61ca8cc94 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,11 +368,15 @@ int simple_remove(struct inode *dir, struct dentry *dentry)
 	 * protect d_delete() from accessing a freed dentry.
 	 */
 	dget(dentry);
-	if (d_is_dir(dentry))
+	if (d_is_dir(dentry)) {
 		ret = simple_rmdir(dir, dentry);
-	else
+		if (!ret)
+			fsnotify_rmdir(dir, dentry);
+	} else {
 		ret = simple_unlink(dir, dentry);
-
+		if (!ret)
+			fsnotify_unlink(dir, dentry);
+	}
 	if (!ret)
 		d_delete(dentry);
 	dput(dentry);
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

Fwd: [PATCH v2 03/14] fsnotify: add empty fsnotify_{unlink,rmdir}() hooks

[Amir Goldstein]

Christoph,

From my patch series, this is really the only patch that is a
dependency to the patch you received earlier today:
"fsnotify: call fsnotify_rmdir() hook from configfs"

What you should know for wider context is that currently
users can sign up for inotify notifications on configfs.
If they do that with current kernel they will get notified when
directory is removed not via vfs_rmdir() or via
configfs_unregister_{group,subsystem}().

For reasons outside the scope of configfs, we intend to stop
calling fsnotify hook from d_delete(). In order to preserve existing
behavior (whether some users depends on it or not I do not know),
we add explicit fsnotify hooks in configfs_unregister_{group,subsystem}().

For most pseduo filesystems that use simple_rmdir(), I chose a different
approach of calling a new helper simple_remove(), where the new fsnotify
hook is placed. For configfs, I could not use the same technique because
configfs_remove_dir() is also called from execution path of vfs_rmdir(), where
the new fsnotify hook is already called.

Hope this context is sufficient for you to review the configfs patch and provide
an ACK, so Jan or Al can carry the configfs patch.

If you like me to post you the entire series, I can do that.

Thanks,
Amir.

---------- Forwarded message ---------
From: Amir Goldstein <amir73il@gmail.com>
Date: Thu, May 16, 2019 at 1:26 PM
Subject: [PATCH v2 03/14] fsnotify: add empty fsnotify_{unlink,rmdir}() hooks
To: Jan Kara <jack@suse.cz>
Cc: Matthew Bobrowski <mbobrowski@mbobrowski.org>,
<linux-fsdevel@vger.kernel.org>


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/libfs.c               | 11 ++++++++---
 fs/namei.c               |  2 ++
 include/linux/fsnotify.h | 26 ++++++++++++++++++++++++++
 3 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/fs/libfs.c b/fs/libfs.c
index ca1132f1d5c6..4db61ca8cc94 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,11 +368,15 @@ int simple_remove(struct inode *dir, struct
dentry *dentry)
         * protect d_delete() from accessing a freed dentry.
         */
        dget(dentry);
-       if (d_is_dir(dentry))
+       if (d_is_dir(dentry)) {
                ret = simple_rmdir(dir, dentry);
-       else
+               if (!ret)
+                       fsnotify_rmdir(dir, dentry);
+       } else {
                ret = simple_unlink(dir, dentry);
-
+               if (!ret)
+                       fsnotify_unlink(dir, dentry);
+       }
        if (!ret)
                d_delete(dentry);
        dput(dentry);
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 v2 04/14] fs: convert hypfs to use simple_remove() helper

[Amir Goldstein]

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

Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 arch/s390/hypfs/inode.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 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));
 }
-- 
2.17.1

[PATCH v2 05/14] fs: convert qibfs/ipathfs to use simple_remove() helper

[Amir Goldstein]

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

Cc: Doug Ledford <dledford@redhat.com>
Cc: Jason Gunthorpe <jgg@mellanox.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 drivers/infiniband/hw/qib/qib_fs.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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

[PATCH v2 06/14] fs: convert debugfs to use simple_remove() helper

[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 | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

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

Re: [PATCH v2 06/14] fs: convert debugfs to use simple_remove() helper

[Greg Kroah-Hartman]

On Thu, May 16, 2019 at 01:26:33PM +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 | 20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)
> 
> 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);

What happened to this call?  Why no unlinking anymore?

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

Can't dentry be gone here?  This doesn't seem to match the same pattern
as before.

What am I missing?

confused,

greg k-h

Re: [PATCH v2 06/14] fs: convert debugfs to use simple_remove() helper

[Amir Goldstein]

On Thu, May 16, 2019 at 1:35 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, May 16, 2019 at 01:26:33PM +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 | 20 ++++----------------
> >  1 file changed, 4 insertions(+), 16 deletions(-)
> >
> > 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);
>
> What happened to this call?  Why no unlinking anymore?
>
> > -
> >       /*
> >        * 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))
>
> Can't dentry be gone here?  This doesn't seem to match the same pattern
> as before.
>
> What am I missing?
>

The grammatical change __debugfs_remove_file() => __debugfs_file_removed()
After change, the helper only does the post delete stuff.
simple_unlink() is now done inside simple_remove().
This debugfs patch depends on a patch that adds the simple_remove() helper.
sorry for not mentioning this explicitly.

Thanks,
Amir.

Re: [PATCH v2 06/14] fs: convert debugfs to use simple_remove() helper

[Jan Kara]

On Thu 16-05-19 13:44:51, Amir Goldstein wrote:
> On Thu, May 16, 2019 at 1:35 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, May 16, 2019 at 01:26:33PM +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 | 20 ++++----------------
> > >  1 file changed, 4 insertions(+), 16 deletions(-)
> > >
> > > 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);
> >
> > What happened to this call?  Why no unlinking anymore?
> >
> > > -
> > >       /*
> > >        * 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))
> >
> > Can't dentry be gone here?  This doesn't seem to match the same pattern
> > as before.
> >
> > What am I missing?
> >
> 
> The grammatical change __debugfs_remove_file() => __debugfs_file_removed()
> After change, the helper only does the post delete stuff.
> simple_unlink() is now done inside simple_remove().
> This debugfs patch depends on a patch that adds the simple_remove() helper.
> sorry for not mentioning this explicitly.

Right. But Greg is right that simple_remove() may have dropped last dentry
reference and thus you now pass freed dentry to d_is_reg() and
__debugfs_file_removed()?

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

Re: [PATCH v2 06/14] fs: convert debugfs to use simple_remove() helper

[Amir Goldstein]

On Thu, May 16, 2019 at 3:02 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 16-05-19 13:44:51, Amir Goldstein wrote:
> > On Thu, May 16, 2019 at 1:35 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, May 16, 2019 at 01:26:33PM +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 | 20 ++++----------------
> > > >  1 file changed, 4 insertions(+), 16 deletions(-)
> > > >
> > > > 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);
> > >
> > > What happened to this call?  Why no unlinking anymore?
> > >
> > > > -
> > > >       /*
> > > >        * 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))
> > >
> > > Can't dentry be gone here?  This doesn't seem to match the same pattern
> > > as before.
> > >
> > > What am I missing?
> > >
> >
> > The grammatical change __debugfs_remove_file() => __debugfs_file_removed()
> > After change, the helper only does the post delete stuff.
> > simple_unlink() is now done inside simple_remove().
> > This debugfs patch depends on a patch that adds the simple_remove() helper.
> > sorry for not mentioning this explicitly.
>
> Right. But Greg is right that simple_remove() may have dropped last dentry
> reference and thus you now pass freed dentry to d_is_reg() and
> __debugfs_file_removed()?
>

It seem so. Good spotting!

Thanks,
Amir.

Re: [PATCH v2 06/14] fs: convert debugfs to use simple_remove() helper

[Greg Kroah-Hartman]

On Thu, May 16, 2019 at 03:09:20PM +0300, Amir Goldstein wrote:
> On Thu, May 16, 2019 at 3:02 PM Jan Kara <jack@suse.cz> wrote:
> >
> > On Thu 16-05-19 13:44:51, Amir Goldstein wrote:
> > > On Thu, May 16, 2019 at 1:35 PM Greg Kroah-Hartman
> > > <gregkh@linuxfoundation.org> wrote:
> > > >
> > > > On Thu, May 16, 2019 at 01:26:33PM +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 | 20 ++++----------------
> > > > >  1 file changed, 4 insertions(+), 16 deletions(-)
> > > > >
> > > > > 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);
> > > >
> > > > What happened to this call?  Why no unlinking anymore?
> > > >
> > > > > -
> > > > >       /*
> > > > >        * 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))
> > > >
> > > > Can't dentry be gone here?  This doesn't seem to match the same pattern
> > > > as before.
> > > >
> > > > What am I missing?
> > > >
> > >
> > > The grammatical change __debugfs_remove_file() => __debugfs_file_removed()
> > > After change, the helper only does the post delete stuff.
> > > simple_unlink() is now done inside simple_remove().
> > > This debugfs patch depends on a patch that adds the simple_remove() helper.
> > > sorry for not mentioning this explicitly.
> >
> > Right. But Greg is right that simple_remove() may have dropped last dentry
> > reference and thus you now pass freed dentry to d_is_reg() and
> > __debugfs_file_removed()?
> >
> 
> It seem so. Good spotting!

Yes, that's what I was trying to say.  I don't think this conversion is
correct, so you might either have to rework your simple_rmdir(), or this
patch, to make it work properly.

thanks,

greg k-h

Re: [PATCH v2 06/14] fs: convert debugfs to use simple_remove() helper

[Amir Goldstein]

On Thu, May 16, 2019 at 6:28 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, May 16, 2019 at 03:09:20PM +0300, Amir Goldstein wrote:
> > On Thu, May 16, 2019 at 3:02 PM Jan Kara <jack@suse.cz> wrote:
> > >
> > > On Thu 16-05-19 13:44:51, Amir Goldstein wrote:
> > > > On Thu, May 16, 2019 at 1:35 PM Greg Kroah-Hartman
> > > > <gregkh@linuxfoundation.org> wrote:
> > > > >
> > > > > On Thu, May 16, 2019 at 01:26:33PM +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 | 20 ++++----------------
> > > > > >  1 file changed, 4 insertions(+), 16 deletions(-)
> > > > > >
> > > > > > 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);
> > > > >
> > > > > What happened to this call?  Why no unlinking anymore?
> > > > >
> > > > > > -
> > > > > >       /*
> > > > > >        * 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))
> > > > >
> > > > > Can't dentry be gone here?  This doesn't seem to match the same pattern
> > > > > as before.
> > > > >
> > > > > What am I missing?
> > > > >
> > > >
> > > > The grammatical change __debugfs_remove_file() => __debugfs_file_removed()
> > > > After change, the helper only does the post delete stuff.
> > > > simple_unlink() is now done inside simple_remove().
> > > > This debugfs patch depends on a patch that adds the simple_remove() helper.
> > > > sorry for not mentioning this explicitly.
> > >
> > > Right. But Greg is right that simple_remove() may have dropped last dentry
> > > reference and thus you now pass freed dentry to d_is_reg() and
> > > __debugfs_file_removed()?
> > >
> >
> > It seem so. Good spotting!
>
> Yes, that's what I was trying to say.  I don't think this conversion is
> correct, so you might either have to rework your simple_rmdir(), or this
> patch, to make it work properly.
>

To fix the correctness issue we will keep dget(dentry)/dput(dentry)
in place both in __debugfs_remove() and in simple_remove(), i.e:

               dget(dentry);
               ret = simple_remove(d_inode(parent), dentry);
               if (d_is_reg(dentry))
                       __debugfs_file_removed(dentry);
               dput(dentry);

Will this have addressed your concern?

BTW, I forwarded you the dependency patch that is needed for the
context of this review.

Thanks,
Amir.

Re: [PATCH v2 06/14] fs: convert debugfs to use simple_remove() helper

[Greg Kroah-Hartman]

On Thu, May 16, 2019 at 06:38:50PM +0300, Amir Goldstein wrote:
> On Thu, May 16, 2019 at 6:28 PM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> >
> > On Thu, May 16, 2019 at 03:09:20PM +0300, Amir Goldstein wrote:
> > > On Thu, May 16, 2019 at 3:02 PM Jan Kara <jack@suse.cz> wrote:
> > > >
> > > > On Thu 16-05-19 13:44:51, Amir Goldstein wrote:
> > > > > On Thu, May 16, 2019 at 1:35 PM Greg Kroah-Hartman
> > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > >
> > > > > > On Thu, May 16, 2019 at 01:26:33PM +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 | 20 ++++----------------
> > > > > > >  1 file changed, 4 insertions(+), 16 deletions(-)
> > > > > > >
> > > > > > > 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);
> > > > > >
> > > > > > What happened to this call?  Why no unlinking anymore?
> > > > > >
> > > > > > > -
> > > > > > >       /*
> > > > > > >        * 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))
> > > > > >
> > > > > > Can't dentry be gone here?  This doesn't seem to match the same pattern
> > > > > > as before.
> > > > > >
> > > > > > What am I missing?
> > > > > >
> > > > >
> > > > > The grammatical change __debugfs_remove_file() => __debugfs_file_removed()
> > > > > After change, the helper only does the post delete stuff.
> > > > > simple_unlink() is now done inside simple_remove().
> > > > > This debugfs patch depends on a patch that adds the simple_remove() helper.
> > > > > sorry for not mentioning this explicitly.
> > > >
> > > > Right. But Greg is right that simple_remove() may have dropped last dentry
> > > > reference and thus you now pass freed dentry to d_is_reg() and
> > > > __debugfs_file_removed()?
> > > >
> > >
> > > It seem so. Good spotting!
> >
> > Yes, that's what I was trying to say.  I don't think this conversion is
> > correct, so you might either have to rework your simple_rmdir(), or this
> > patch, to make it work properly.
> >
> 
> To fix the correctness issue we will keep dget(dentry)/dput(dentry)
> in place both in __debugfs_remove() and in simple_remove(), i.e:
> 
>                dget(dentry);
>                ret = simple_remove(d_inode(parent), dentry);
>                if (d_is_reg(dentry))
>                        __debugfs_file_removed(dentry);
>                dput(dentry);
> 
> Will this have addressed your concern?

Shouldn't you check for !d_is_reg before calling simple_remove()?
> 
> BTW, I forwarded you the dependency patch that is needed for the
> context of this review.

I had dug it out of the original series when I reviewed this :)

thanks,

greg k-h

Re: [PATCH v2 06/14] fs: convert debugfs to use simple_remove() helper

[Amir Goldstein]

On Thu, May 16, 2019 at 7:52 PM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Thu, May 16, 2019 at 06:38:50PM +0300, Amir Goldstein wrote:
> > On Thu, May 16, 2019 at 6:28 PM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > >
> > > On Thu, May 16, 2019 at 03:09:20PM +0300, Amir Goldstein wrote:
> > > > On Thu, May 16, 2019 at 3:02 PM Jan Kara <jack@suse.cz> wrote:
> > > > >
> > > > > On Thu 16-05-19 13:44:51, Amir Goldstein wrote:
> > > > > > On Thu, May 16, 2019 at 1:35 PM Greg Kroah-Hartman
> > > > > > <gregkh@linuxfoundation.org> wrote:
> > > > > > >
> > > > > > > On Thu, May 16, 2019 at 01:26:33PM +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 | 20 ++++----------------
> > > > > > > >  1 file changed, 4 insertions(+), 16 deletions(-)
> > > > > > > >
> > > > > > > > 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);
> > > > > > >
> > > > > > > What happened to this call?  Why no unlinking anymore?
> > > > > > >
> > > > > > > > -
> > > > > > > >       /*
> > > > > > > >        * 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))
> > > > > > >
> > > > > > > Can't dentry be gone here?  This doesn't seem to match the same pattern
> > > > > > > as before.
> > > > > > >
> > > > > > > What am I missing?
> > > > > > >
> > > > > >
> > > > > > The grammatical change __debugfs_remove_file() => __debugfs_file_removed()
> > > > > > After change, the helper only does the post delete stuff.
> > > > > > simple_unlink() is now done inside simple_remove().
> > > > > > This debugfs patch depends on a patch that adds the simple_remove() helper.
> > > > > > sorry for not mentioning this explicitly.
> > > > >
> > > > > Right. But Greg is right that simple_remove() may have dropped last dentry
> > > > > reference and thus you now pass freed dentry to d_is_reg() and
> > > > > __debugfs_file_removed()?
> > > > >
> > > >
> > > > It seem so. Good spotting!
> > >
> > > Yes, that's what I was trying to say.  I don't think this conversion is
> > > correct, so you might either have to rework your simple_rmdir(), or this
> > > patch, to make it work properly.
> > >
> >
> > To fix the correctness issue we will keep dget(dentry)/dput(dentry)
> > in place both in __debugfs_remove() and in simple_remove(), i.e:
> >
> >                dget(dentry);
> >                ret = simple_remove(d_inode(parent), dentry);
> >                if (d_is_reg(dentry))
> >                        __debugfs_file_removed(dentry);
> >                dput(dentry);
> >
> > Will this have addressed your concern?
>
> Shouldn't you check for !d_is_reg before calling simple_remove()?

Current code does simple_unlink() or simple_rmdir() for !d_is_reg
and simple_unlink() + stuff for d_is_reg.

New helper does simple_unlink() or simple_rmdir() as appropriate
for all cases, so all we are left is with + stuff only for d_is_reg.

But anyway, Al is calling this patch set off.

So will continue this discussion on another thread some other time.

Thanks,
Amir.

[PATCH v2 07/14] fs: convert tracefs to use simple_remove() helper

[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 | 23 ++++-------------------
 1 file changed, 4 insertions(+), 19 deletions(-)

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;
 }
 
 /**
-- 
2.17.1

Re: [PATCH v2 07/14] fs: convert tracefs to use simple_remove() helper

[Steven Rostedt]

On Thu, 16 May 2019 13:26:34 +0300
Amir Goldstein <amir73il@gmail.com> wrote:

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

I added this and the patch you forwarded to me (thank you for doing
that), and ran some smoke tests against it. It didn't trigger any
regressions in those tests.

Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> 
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/tracefs/inode.c | 23 ++++-------------------
>  1 file changed, 4 insertions(+), 19 deletions(-)
> 
> 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;
>  }
>  
>  /**

[PATCH v2 08/14] fs: convert rpc_pipefs to use simple_remove() helper

[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 | 16 ++--------------
 1 file changed, 2 insertions(+), 14 deletions(-)

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

[PATCH v2 09/14] fs: convert apparmorfs to use simple_remove() helper

[Amir Goldstein]

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

Cc: John Johansen <john.johansen@canonical.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 security/apparmor/apparmorfs.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

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

[PATCH v2 10/14] fsnotify: call fsnotify_rmdir() hook from btrfs

[Amir Goldstein]

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

Cc: Chris Mason <clm@fb.com>
Cc: Josef Bacik <josef@toxicpanda.com>
Cc: David Sterba <dsterba@suse.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.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

Re: [PATCH v2 10/14] fsnotify: call fsnotify_rmdir() hook from btrfs

[David Sterba]

On Thu, May 16, 2019 at 01:26:37PM +0300, Amir Goldstein wrote:
> This will allow generating fsnotify delete events after the
> fsnotify_nameremove() hook is removed from d_delete().
> 
> Cc: Chris Mason <clm@fb.com>
> Cc: Josef Bacik <josef@toxicpanda.com>
> Cc: David Sterba <dsterba@suse.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.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);

Ok, this is the only instance of d_delete.

Acked-by: David Sterba <dsterba@suse.com>

[PATCH v2 11/14] fsnotify: call fsnotify_rmdir() hook from configfs

[Amir Goldstein]

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

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 591e82ba443c..2356008029f7 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_rmdir(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_rmdir(d_inode(root), dentry);
 	inode_unlock(d_inode(dentry));
 
 	d_delete(dentry);
-- 
2.17.1

Re: [PATCH v2 11/14] fsnotify: call fsnotify_rmdir() hook from configfs

[Christoph Hellwig]

On Thu, May 16, 2019 at 01:26:38PM +0300, Amir Goldstein wrote:
> This will allow generating fsnotify delete events after the
> fsnotify_nameremove() hook is removed from d_delete().

This seems to be missing 13 patches of context without it isn't
reviewable.  If you decide to Cc someone either make sure they get all
the patches or just don't bother to start with.

Re: [PATCH v2 11/14] fsnotify: call fsnotify_rmdir() hook from configfs

[Amir Goldstein]

On Thu, May 16, 2019 at 3:33 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Thu, May 16, 2019 at 01:26:38PM +0300, Amir Goldstein wrote:
> > This will allow generating fsnotify delete events after the
> > fsnotify_nameremove() hook is removed from d_delete().
>
> This seems to be missing 13 patches of context without it isn't
> reviewable.  If you decide to Cc someone either make sure they get all
> the patches or just don't bother to start with.

You are right. I wanted to avoid spamming all fs maintainers which all
individual fs patches, but should have CC'ed all on the cover letter and
the dependency patches 2-3:
https://lore.kernel.org/linux-fsdevel/20190516122506.GF13274@quack2.suse.cz/

I will go a head and manually forward those messages to all affected
maintainers.

Thanks,
Amir.

[PATCH v2 12/14] fsnotify: call fsnotify_unlink() hook from devpts

[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 v2 13/14] 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 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/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 v2 14/14] 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 v2 00/14] Sort out fsnotify_nameremove() mess

[Amir Goldstein]

On Thu, May 16, 2019 at 1:26 PM Amir Goldstein <amir73il@gmail.com> wrote:
>
> Jan,
>
> What do you think will be the best merge strategy for this patch series?
>
> How about sending first 3 prep patches to Linus for applying at the end
> of v5.2 merge window, so individual maintainers can pick up per fs
> patches for the v5.3 development cycle?

Doh! I was going to CC Al on the entire series and forgot.

Al,

We could definitely use your input on this series in general and about
merge strategy in particular.
If you would agree to take the entire series through your tree, that could
make things considerably easier.
After all it, this patch set is more a vfs work than it is an fsnotify work.

Thanks,
Amir.

>
> The following d_delete() call sites have been audited and will no longer
> generate fsnotify event after this series:
>
> drivers/usb/gadget/function/f_fs.c:ffs_epfiles_destroy() - cleanup? (*)
> fs/ceph/dir.c:ceph_unlink() - from vfs_unlink()
> fs/ceph/inode.c:ceph_fill_trace() - invalidate (**)
> fs/configfs/dir.c:detach_groups() - 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() - invalidate (**)
> fs/ocfs2/dlmglue.c:ocfs2_dentry_convert_worker() - invalidate? (**)
> fs/reiserfs/xattr.c:xattr_{unlink,rmdir}() - hidden xattr inode
>
> (*) There are 2 "cleanup" use cases:
>   - Create;init;delte 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 v1:
> - Split up per filesystem patches
> - New hook names fsnotify_{unlink,rmdir}()
> - Simplify fsnotify code in separate final patch
>
> Amir Goldstein (14):
>   ASoC: rename functions that pollute the simple_xxx namespace
>   fs: create simple_remove() helper
>   fsnotify: add empty fsnotify_{unlink,rmdir}() hooks
>   fs: convert hypfs to use simple_remove() helper
>   fs: convert qibfs/ipathfs to use simple_remove() helper
>   fs: convert debugfs to use simple_remove() helper
>   fs: convert tracefs to use simple_remove() helper
>   fs: convert rpc_pipefs to use simple_remove() helper
>   fs: convert apparmorfs to use simple_remove() helper
>   fsnotify: call fsnotify_rmdir() hook from btrfs
>   fsnotify: call fsnotify_rmdir() hook from configfs
>   fsnotify: call fsnotify_unlink() hook from devpts
>   fsnotify: move fsnotify_nameremove() hook out of d_delete()
>   fsnotify: get rid of fsnotify_nameremove()
>
>  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                         | 32 +++++++++++++++++++++++
>  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           | 26 +++++++++++++++++++
>  include/linux/fsnotify_backend.h   |  4 ---
>  net/sunrpc/rpc_pipe.c              | 16 ++----------
>  security/apparmor/apparmorfs.c     |  6 +----
>  sound/soc/generic/simple-card.c    |  8 +++---
>  19 files changed, 86 insertions(+), 126 deletions(-)
>
> --
> 2.17.1
>

Re: [PATCH v2 00/14] Sort out fsnotify_nameremove() mess

[Jan Kara]

Hi,

On Thu 16-05-19 13:26:27, Amir Goldstein wrote:
> What do you think will be the best merge strategy for this patch series?
> 
> How about sending first 3 prep patches to Linus for applying at the end
> of v5.2 merge window, so individual maintainers can pick up per fs
> patches for the v5.3 development cycle?

I don't think we'll make it for rc1. But those three cleanup patches would
be OK for rc2 as well. But overall patches are not very intrusive so I'm
also OK with pushing the patches myself once we get acks from respective
maintainers if Al will be too busy and won't react.

> The following d_delete() call sites have been audited and will no longer
> generate fsnotify event after this series:
> 
> drivers/usb/gadget/function/f_fs.c:ffs_epfiles_destroy() - cleanup? (*)

Not really but creation events are not generated either.

> fs/ceph/dir.c:ceph_unlink() - from vfs_unlink()
> fs/ceph/inode.c:ceph_fill_trace() - invalidate (**)

There's one more 'invalidate' case in fs/ceph/inode.c.

> fs/configfs/dir.c:detach_groups() - cleanup (*)

Why is this a cleanup? detach_groups() is used also from
configfs_detach_group() which gets called from configfs_rmdir() which is
real deletion.

> 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() - invalidate (**)

More a cleanup case I'd say...

> fs/ocfs2/dlmglue.c:ocfs2_dentry_convert_worker() - invalidate? (**)

This is really invalidate.

> fs/reiserfs/xattr.c:xattr_{unlink,rmdir}() - hidden xattr inode
> 
> (*) There are 2 "cleanup" use cases:
>   - Create;init;delte 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. Maybe for other reviewers it would be more convincing to take
sanitized output of 'git grep "[^a-z_]d_delete("' and annotate how each
callsite is handled - i.e., "cleanup, invalidate, simple_remove, vfs,
manual - patch X". But I'm now reasonably convinced we didn't forget
anything :).

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

Re: [PATCH v2 00/14] Sort out fsnotify_nameremove() mess

[Amir Goldstein]

On Thu, May 16, 2019 at 3:25 PM Jan Kara <jack@suse.cz> wrote:
>
> Hi,
>
> On Thu 16-05-19 13:26:27, Amir Goldstein wrote:
> > What do you think will be the best merge strategy for this patch series?
> >
> > How about sending first 3 prep patches to Linus for applying at the end
> > of v5.2 merge window, so individual maintainers can pick up per fs
> > patches for the v5.3 development cycle?
>
> I don't think we'll make it for rc1. But those three cleanup patches would
> be OK for rc2 as well. But overall patches are not very intrusive so I'm
> also OK with pushing the patches myself once we get acks from respective
> maintainers if Al will be too busy and won't react.
>
> > The following d_delete() call sites have been audited and will no longer
> > generate fsnotify event after this series:
> >
> > drivers/usb/gadget/function/f_fs.c:ffs_epfiles_destroy() - cleanup? (*)
>
> Not really but creation events are not generated either.
>
> > fs/ceph/dir.c:ceph_unlink() - from vfs_unlink()
> > fs/ceph/inode.c:ceph_fill_trace() - invalidate (**)
>
> There's one more 'invalidate' case in fs/ceph/inode.c.
>
> > fs/configfs/dir.c:detach_groups() - cleanup (*)
>
> Why is this a cleanup? detach_groups() is used also from
> configfs_detach_group() which gets called from configfs_rmdir() which is
> real deletion.

True, configfs is a special case where both cleanup and real deletion
use the same helper. configfs_detach_group() is either called for cleanup
or from vfs_rmdir->configfs_rmdir()/configfs_unregister_{group,subsystem}()
the latter 3 cases have new fsnotify hooks.

>
> > 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() - invalidate (**)
>
> More a cleanup case I'd say...
>
> > fs/ocfs2/dlmglue.c:ocfs2_dentry_convert_worker() - invalidate? (**)
>
> This is really invalidate.
>
> > fs/reiserfs/xattr.c:xattr_{unlink,rmdir}() - hidden xattr inode
> >
> > (*) There are 2 "cleanup" use cases:
> >   - Create;init;delte 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. Maybe for other reviewers it would be more convincing to take
> sanitized output of 'git grep "[^a-z_]d_delete("' and annotate how each
> callsite is handled - i.e., "cleanup, invalidate, simple_remove, vfs,
> manual - patch X". But I'm now reasonably convinced we didn't forget
> anything :).
>
>                                                                 Honza
> --
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: [PATCH v2 00/14] Sort out fsnotify_nameremove() mess

[Al Viro]

On Thu, May 16, 2019 at 04:56:20PM +0300, Amir Goldstein wrote:

> > Why is this a cleanup? detach_groups() is used also from
> > configfs_detach_group() which gets called from configfs_rmdir() which is
> > real deletion.
> 
> True, configfs is a special case where both cleanup and real deletion
> use the same helper. configfs_detach_group() is either called for cleanup
> or from vfs_rmdir->configfs_rmdir()/configfs_unregister_{group,subsystem}()
> the latter 3 cases have new fsnotify hooks.

FWIW, I've an old series on configfs, from the "deal with kernel-side rm -rf
properly" pile.

I'll try to resurrect and post it.  A _lot_ of locking crap in there is
due to the bad idea of having the subtree being built reachable from
root as we are putting it together; massaging it to the form when we
build a subtree and move it in place only when we are past the last
failure makes for much simpler logics, for obvious reasons.  The massage
is not trivial, though.

In general, the problem with kernel-side recursive removals is that
a bunch of places are trying to cobble local solutions for the problem
that is actually a missing primitive.  And fucking it up in all kinds
of ways.  We definitely want a unified primitive for that; the question
is what kind of API would be kludge-free for callers.

I've a pile of notes and half-assed patch series in that direction;
I'll dig it out and try to get something coherent out of it.