| Message ID | 20190514221901.29125-5-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Sort out fsnotify_nameremove() mess | expand |
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
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.
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
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) {}
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(-)