Re: fsnotify path hooks

[Amir Goldstein <amir73il@gmail.com>]

On Fri, Apr 9, 2021 at 1:08 PM Jan Kara <jack@suse.cz> wrote:
>
> On Thu 08-04-21 18:11:31, Amir Goldstein wrote:
> > > > 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.
>
> Yes, I prefer explicit NULL mnt argument to make it obvious we are going to
> miss something in this case. I agree it's going to be somewhat bigger churn
> but it isn't that bad (10 + 6 callers).
>

I don't mind the churn so much, but for clarity of what we are missing, I'd
prefer to use fsnotify_inode_create() vs. fsnotify_path_create(), which is
exactly the difference between the two flavors - the type or args passed.
BTW, there is a precedence to that convention with security_{inode,path}
hooks, but in that case, both hooks are called.

> > > outside view, it is ctunclear 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.
>
> Sorry, I was probably too philosophical here ;)
>
> > Are you ok with vfs_create() vs. vfs_create_nonotify()?
>
> I'm OK with vfs_create_nonotify(). I have a problem with vfs_create()
> because it generates inode + fs events but does not generate mount events
> which is just strange (although I appreciate the technical reason behind
> it :).
>
> > How do you propose to change fsnotify hooks in vfs_create()?
>
> So either pass 'mnt' to vfs_create() - as we discussed, this may be
> actually acceptable these days due to idmapped mounts work - and generate
> all events there, or make vfs_create() not generate any fsnotify events and
> create new vfs_create_notify() which will take the 'mnt' and generate
> events. Either is fine with me and more consistent than what you currently
> propose. Thoughts?
>

I'm good with vfs_create_notify(). This definitely forces me to submit the
s/notify_change/vfs_setattr_notify conversion ;-)
I will take a swing at it.

But we are actually going in cycles around the solution that we all want,
but fear of rejection. It's time to try and solicit direct feedback from Al.

Al,

would you be ok with passing mnt arg to vfs_create() and friends,
so that we can pass that to fsnotify_create() (and friends) in order to
be able to report FAN_CREATE events to FAN_MARK_MOUNT listeners?

The watched mount context is relevant to syscalls/io_uring and nfsd
and less relevant to callers using private mount clones like overlayfs,
but it doesn't hurt to pass the private mount.

Thanks,
Amir.