Re: fsnotify path hooks

[Amir Goldstein <amir73il@gmail.com>]

[...]
> > > So yeh, I do think it is manageable. I think the best solution would be
> > > something along the lines of wrappers like the following:
> > >
> > > static inline int vfs_mkdir(...)
> > > {
> > >         int error = __vfs_mkdir_nonotify(...);
> > >         if (!error)
> > >                 fsnotify_mkdir(dir, dentry);
> > >         return error;
> > > }
> > >
> > > And then the few call sites that call the fsnotify_path_ hooks
> > > (i.e. in syscalls and perhaps later in nfsd) will call the
> > > __vfs_xxx_nonotify() variant.
> >
> > Yes, that is OK with me. Or we could have something like:
> >
> > static inline void fsnotify_dirent(struct vfsmount *mnt, struct inode *dir,
> >                                    struct dentry *dentry, __u32 mask)
> > {
> >         if (!mnt) {
> >                 fsnotify(mask, d_inode(dentry), FSNOTIFY_EVENT_INODE, dir,
> >                          &dentry->d_name, NULL, 0);
> >         } else {
> >                 struct path path = {
> >                         .mnt = mnt,
> >                         .dentry = d_find_any_alias(dir)
> >                 };
> >                 fsnotify(mask, d_inode(dentry), FSNOTIFY_EVENT_PATH, &path,
> >                          &dentry->d_name, NULL, 0);
> >         }
> > }
> >
> > static inline void fsnotify_mkdir(struct vfsmount *mnt, struct inode *inode,
> >                                   struct dentry *dentry)
> > {
> >         audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE);
> >
> >         fsnotify_dirent(mnt, inode, dentry, FS_CREATE | FS_ISDIR);
> > }
> >
> > static inline int vfs_mkdir(mnt, ...)
> > {
> >         int error = __vfs_mkdir_nonotify(...);
> >         if (!error)
> >                 fsnotify_mkdir(mnt, dir, dentry);
> > }
> >
>
> I've done something similar to that. I think it's a bit cleaner,
> but we can debate on the details later.
> Pushed POC to branch fsnotify_path_hooks.

FYI, I tried your suggested approach above for fsnotify_xattr(),
but I think I prefer to use an explicit flavor fsnotify_xattr_mnt()
and a wrapper fsnotify_xattr().
Pushed WIP to fsnotify_path_hooks branch. It also contains
some unstashed "fix" patches to tidy up the previous hooks.

I ran into another hurdle with fsnotify_xattr() -
vfs_setxattr() is too large to duplicate a _nonotify() variant IMO.
OTOH, I cannot lift fsnotify_xattr() up to callers without moving
the fsnotify hook outside the inode lock.

This was not a problem with the directory entry path hooks.
This is also not going to be a problem with fsnotify_change(),
because notify_change() is called with inode locked.

Do you think that calling fsnotify_xattr() under inode lock is important?
Should I refactor a helper vfs_setxattr_notify() that takes a boolean
arg for optionally calling fsnotify_xattr()?
Do you have another idea how to deal with that hook?

With notify_change() I have a different silly problem with using the
refactoring method - the name notify_change_nonotify() is unacceptable.
We may consider __ATTR_NONOTIFY ia_valid flag as the method to
use instead of refactoring in this case, just because we can and
because it creates less clutter.

What do you think?

Thanks,
Amir.