On Wed, May 15, 2019 at 11:24 AM Jan Kara <jack@xxxxxxx> 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@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.
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.
