Re: fsnotify path hooks

[Jan Kara <jack@suse.cz>]

On Tue 06-04-21 21:49:13, Amir Goldstein wrote:
> [...]
> > > > 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.

What's in fsnotify_path_hooks branch looks good to me wrt xattr hooks. What
I somewhat dislike about e.g. the fsnotify_create() approach you took is
that there are separate hooks fsnotify_create() and fsnotify_create_path()
which expose what is IMO an internal fsnotify detail of what are different
event types. I'd say it is more natural (from VFS POV) to have just a
single hook and fill in as much information as available... Also from
outside view, it is unclear that e.g. vfs_create() will generate some types
of fsnotify events but not all while e.g. do_mknodat() will generate all
fsnotify events. That's why I'm not sure whether a helper like vfs_create()
in your tree is the right abstraction since generating one type of fsnotify
event while not generating another type should be a very conscious decision
of the implementor - basically if you have no other option.

That all being said, this is just an internal API so we are free to tweak
it in the future if we get things wrong. So I'm not pushing hard for my
proposal but I wanted to raise my concerns. Also I think Al Viro might have
his opinion on this so you should probably CC him when posting the series...

> 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?

I think having the event generated outside of i_rwsem is fine. The only
reason why I think it could possibly matter is due to reordering of events
on the same inode but that order is uncertain anyway.

> 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?

Hmm, notify_change() is an inconsistent name anyway (for historical
reasons). Consistent name would be vfs_setattr(). And
vfs_setattr_nonotify() would be a fine name as well. What do you think?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR