Re: fsnotify path hooks

[Amir Goldstein <amir73il@gmail.com>]

> > I really want to see details on all callers - which mount are
> > you going to use in each case.
>
> The callers are:
> cachefiles, ecryptfs, nfsd, devtmpfs,
> do_truncate(), vfs_utimes() and file_remove_privs()
>
> * cachefiles, ecryptfs, nfsd compose paths from stashed
> mount like this all the time (e.g. for vfs_truncate(), vf_getattr()).
>
> * devtmpfs has the parent path from and also uses it to
> compose child path for vfs_getattr().
>
> * vfs_utimes() and all callers of do_truncate() already have the
> path, just need to pass it through to notify_change()
>

Not sure how I forgot to mention chmod and chown, but obviously
there is no problem with path from those callers.

> >
> >         The thing that is not going to be acceptable is
> > a combination of mount from one filesystem and dentry from
> > another.  In particular, file_remove_privs() is going to be
> > interesting.
> >
> >         Note, BTW, that ftruncate() and file_remove_privs()
> > are different in that respect - the latter hits d_real()
> > (by way of file_dentry()), the former does not.  Which one
> > is correct and if both are, why are their needs different?
>
> Nowadays (>= v4.19) I think the only files whose file_inode() and
> f_path do not agree are the overlayfs "real.file" that can find their
> way to f_mapping and to some vfs helpers and from there to
> filesystem ops and to file_modified() or generic_file_write_iter()
> and to file_remove_privs().
>
> Contrary to that, overlayfs does not call any vfs truncate()
> helper, it calls notify_change() directly (with a composed path).
>
> So what should we do about file_remove_privs()?
> Since I don't think we really need to care about generating an
> event on file_remove_privs(), perhaps it could call __notify_change()
> that does not generate an event

I found more instances of notify_change() in overlayfs copy_up code
that IMO should also use __notify_change() and not report fsnotify
events for restoring attributes on files post copy up.

Like with the case of file_remove_privs(), it is enough IMO that the
listener is able to get an event on the modification event that caused
copy up or remove_privs, no need for an event on the subsystem
internal implementation details.

> and the rest of the callers call this wrapper:
>
> int notify_change(struct path *path, struct iattr *attr,
>                             struct inode **delegated_inode)
> {
>         unsigned int ia_valid;
>         int error = __notify_change(mnt_user_ns(path->mnt), path->dentry,
>                                                     attr, &ia_valid,

Braino here. There is no need to pass ia_valid to helper.
I failed to notice that notify_change updates attr->ia_valid.

Which brings me to the fun question - naming.

Would you like me to follow up on Jan's suggestion to rename:
s/__notify_change/vfs_setattr
s/notify_change/vfs_setattr_notify

I pushed this version (a.k.a. "tollerabe") to:
https://github.com/amir73il/linux/commits/fsnotify_path_hooks

It's only sanity tested.

Thanks,
Amir.