Re: fsnotify path hooks

[Amir Goldstein <amir73il@gmail.com>]

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

So to be clear, you do NOT want additional wrappers like this and
you prefer to have the NULL mnt argument explicit in all callers?

static inline void fsnotify_xattr(struct dentry *dentry)
{
        fsnotify_xattr_mnt(NULL, dentry);
}

For fsnotify_xattr() it does not matter so much, but fsnotify_create/mkdir()
have quite a few callers in special filesystems.

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

I lost you here.
Are you ok with vfs_create() vs. vfs_create_nonotify()?
How do you propose to change fsnotify hooks in vfs_create()?
Did you mean to call the same fsnotify_create() hook in both vfs_create()
and do_mknodat() where the former is called with NULL mnt argument?

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

Sure, just wanted to get your initial feedback before kicking the patch set
into shape, because it could have gone in many different ways, which it did ;)

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

That's what I thought. It is not much different than ACCESS/MODIFY
events.

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

I won't propose to convert all notify_change() callers before getting an
explicit ACK from Al, but I could still use notify_change() as a wrapper
around vfs_setattr_nonotify() and maybe also create an alias vfs_setattr()
and use it in some places.

Thanks,
Amir.