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@xxxxxxxxx>
> ---
> fs/afs/dir_silly.c | 5 ----
> fs/btrfs/ioctl.c | 4 +++-
> fs/configfs/dir.c | 3 +++
> fs/dcache.c | 2 --
> fs/devpts/inode.c | 1 +
> fs/nfs/unlink.c | 6 -----
> fs/notify/fsnotify.c | 41 --------------------------------
> include/linux/fsnotify.h | 7 +++++-
> include/linux/fsnotify_backend.h | 4 ----
> 9 files changed, 13 insertions(+), 60 deletions(-)
>
> diff --git a/fs/afs/dir_silly.c b/fs/afs/dir_silly.c
> index f6f89fdab6b2..d3494825d08a 100644
> --- a/fs/afs/dir_silly.c
> +++ b/fs/afs/dir_silly.c
> @@ -57,11 +57,6 @@ static int afs_do_silly_rename(struct afs_vnode *dvnode, struct afs_vnode *vnode
> if (test_bit(AFS_VNODE_DIR_VALID, &dvnode->flags))
> afs_edit_dir_add(dvnode, &new->d_name,
> &vnode->fid, afs_edit_dir_for_silly_1);
> -
> - /* vfs_unlink and the like do not issue this when a file is
> - * sillyrenamed, so do it here.
> - */
> - fsnotify_nameremove(old, 0);
> }
>
> _leave(" = %d", ret);
This changes the behavior when rename happens to overwrite a file, doesn't
it? It is more consistent that way and I don't think anybody depends on it
so I agree but it might deserve a comment in the changelog.
> diff --git a/fs/configfs/dir.c b/fs/configfs/dir.c
> index 591e82ba443c..78566002234a 100644
> --- a/fs/configfs/dir.c
> +++ b/fs/configfs/dir.c
> @@ -1797,6 +1798,7 @@ void configfs_unregister_group(struct config_group *group)
> configfs_detach_group(&group->cg_item);
> d_inode(dentry)->i_flags |= S_DEAD;
> dont_mount(dentry);
> + fsnotify_remove(d_inode(parent), dentry);
> d_delete(dentry);
> inode_unlock(d_inode(parent));
>
> @@ -1925,6 +1927,7 @@ void configfs_unregister_subsystem(struct configfs_subsystem *subsys)
> configfs_detach_group(&group->cg_item);
> d_inode(dentry)->i_flags |= S_DEAD;
> dont_mount(dentry);
> + fsnotify_remove(d_inode(root), dentry);
> inode_unlock(d_inode(dentry));
>
> d_delete(dentry);
Is his really needed? We have a call chain:
configfs_detach_group()
configfs_detach_item()
configfs_remove_dir()
remove_dir()
simple_rmdir()
Ah, but this is the special configfs treatment you were speaking about as
configfs_detach_group() can get called also from vfs_rmdir(). I see. But
please separate this into a special patch with a good changelog.
> diff --git a/fs/nfs/unlink.c b/fs/nfs/unlink.c
> index 52d533967485..0effeee28352 100644
> --- a/fs/nfs/unlink.c
> +++ b/fs/nfs/unlink.c
> @@ -396,12 +396,6 @@ nfs_complete_sillyrename(struct rpc_task *task, struct nfs_renamedata *data)
> nfs_cancel_async_unlink(dentry);
> return;
> }
> -
> - /*
> - * vfs_unlink and the like do not issue this when a file is
> - * sillyrenamed, so do it here.
> - */
> - fsnotify_nameremove(dentry, 0);
> }
Ditto as for AFS I assume...
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 8c7cbac7183c..5433e37fb0c5 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -107,47 +107,6 @@ void fsnotify_sb_delete(struct super_block *sb)
> fsnotify_clear_marks_by_sb(sb);
> }
>
> -/*
> - * fsnotify_nameremove - a filename was removed from a directory
> - *
> - * This is mostly called under parent vfs inode lock so name and
> - * dentry->d_parent should be stable. However there are some corner cases where
> - * inode lock is not held. So to be on the safe side and be reselient to future
> - * callers and out of tree users of d_delete(), we do not assume that d_parent
> - * and d_name are stable and we use dget_parent() and
> - * take_dentry_name_snapshot() to grab stable references.
> - */
> -void fsnotify_nameremove(struct dentry *dentry, int isdir)
> -{
> - struct dentry *parent;
> - struct name_snapshot name;
> - __u32 mask = FS_DELETE;
> -
> - /* d_delete() of pseudo inode? (e.g. __ns_get_path() playing tricks) */
> - if (IS_ROOT(dentry))
> - return;
> -
> - if (isdir)
> - mask |= FS_ISDIR;
> -
> - parent = dget_parent(dentry);
> - /* Avoid unneeded take_dentry_name_snapshot() */
> - if (!(d_inode(parent)->i_fsnotify_mask & FS_DELETE) &&
> - !(dentry->d_sb->s_fsnotify_mask & FS_DELETE))
> - goto out_dput;
> -
> - take_dentry_name_snapshot(&name, dentry);
> -
> - fsnotify(d_inode(parent), mask, d_inode(dentry), FSNOTIFY_EVENT_INODE,
> - &name.name, 0);
> -
> - release_dentry_name_snapshot(&name);
> -
> -out_dput:
> - dput(parent);
> -}
> -EXPORT_SYMBOL(fsnotify_nameremove);
> -
> /*
> * Given an inode, first check if we care what happens to our children. Inotify
> * and dnotify both tell their parents about events. If we care about any event
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index 455dff82595e..7f68cb9825dd 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -158,10 +158,15 @@ static inline void fsnotify_vfsmount_delete(struct vfsmount *mnt)
> */
> static inline void fsnotify_remove(struct inode *dir, struct dentry *dentry)
> {
> + __u32 mask = FS_DELETE;
> +
> /* Expected to be called before d_delete() */
> WARN_ON_ONCE(d_is_negative(dentry));
>
> - /* TODO: call fsnotify_dirent() */
> + if (d_is_dir(dentry))
> + mask |= FS_ISDIR;
> +
> + fsnotify_dirent(dir, dentry, mask);
> }
With folding patch 2 into this patch, I'd leave fsnotify changes for a
separate patch. I.e., keep fsnotify_nameremove() as is in this patch just
change its callsites and then simplify fsnotify_nameremove() in the next
patch.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
