[RFC,4/4] fsnotify: move fsnotify_nameremove() hook out of d_delete()
diff mbox series

Message ID 20190514221901.29125-5-amir73il@gmail.com
State New
Headers show
Series
  • Sort out fsnotify_nameremove() mess
Related show

Commit Message

Amir Goldstein May 14, 2019, 10:19 p.m. UTC
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(-)

Comments

Jan Kara May 15, 2019, 8:24 a.m. UTC | #1
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
Amir Goldstein May 15, 2019, 10:56 a.m. UTC | #2
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.
Jan Kara May 15, 2019, 11:45 a.m. UTC | #3
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

Patch
diff mbox series

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