Re: [PATCH v2 2/5] fsnotify: annotate filename events

[Jan Kara <jack@suse.cz>]

On Wed 14-11-18 19:43:41, Amir Goldstein wrote:
> Filename events are referring to events that modify directory entries,
> such as create,delete,rename. Those events should always be reported
> on a watched directory, regardless if FS_EVENT_ON_CHILD is set
> on the watch mask.

OK, I find 'directory modification events' clearer than 'filename events'.
But I can live with your name since I don't really have a better
alternative :). Just please define these events in terms of all FS_<foo>
events that are involved so that everyone is on the same page which events
you mean.

> fsnotify_nameremove() and fsnotify_move() were modified to no longer
> set the FS_EVENT_ON_CHILD event bit. This is a semantic change to
> align with the filename event definition. It has no effect on any
> existing backend, because dnotify and inotify always requets the
> child events and fanotify does not get the delete,rename events.

You keep forgetting about audit ;) But in this case it is fine as it always
sets FS_EVENT_ON_CHILD as well.

> The fsnotify_filename() helper is used to report all the filename
> events. It gets a reference on parent dentry and passes it as the
> data for the event along with the filename.
> 
> fsnotify_filename() is different from fsnotify_parent().
> fsnotify_parent() is intended to report any events that happened on
> child inodes when FS_EVENT_ON_CHILD is requested.
> fsnotify_filename() is intended to report only filename events,
> such as create,mkdir,link. Those events must always be reported
> on a watched directory, regardless if FS_EVENT_ON_CHILD was requested.
> 
> fsnotify_d_name() is a helper for the common case where the
> filename to pass is dentry->d_name.name.
> 
> It is safe to use these helpers with negative or not instantiated
> dentries, such as the case with fsnotify_link() and
> fsnotify_nameremove().
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  include/linux/fsnotify.h | 40 +++++++++++++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index 9dadc0bcd7a9..d00ec5838d6e 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -17,8 +17,31 @@
>  #include <linux/slab.h>
>  #include <linux/bug.h>
>  
> +/*
> + * Notify this parent about a filename event (create,delete,rename).
> + * Unlike fsnotify_parent(), the event will be reported regardless of the
> + * FS_EVENT_ON_CHILD mask on the parent inode
> + */

How about specifying this as 'Notify directory 'parent' about a change of
some of its directory entries'? That way you avoid using 'filename' event
in the description which is not defined.

> +static inline int fsnotify_filename(struct dentry *parent, __u32 mask,
> +				    const unsigned char *file_name, u32 cookie)

And how about calling the function fsnotify_dir_change()?

> +{
> +	return fsnotify(d_inode(parent), mask, parent, FSNOTIFY_EVENT_DENTRY,
> +			file_name, cookie);
> +}
> +
> +/*
> + * Call fsnotify_filename() with parent and d_name of this dentry.
> + * Safe to call with negative dentry, e.g. from fsnotify_nameremove()
> + */
> +static inline int fsnotify_d_name(struct dentry *dentry, __u32 mask)

Maybe call this fsnotify_dir_dentry_change()?

> +{
> +	return fsnotify_filename(dentry->d_parent, mask,
> +				 dentry->d_name.name, 0);
> +}
> +
>  /* Notify this dentry's parent about a child's events. */
> -static inline int fsnotify_parent(const struct path *path, struct dentry *dentry, __u32 mask)
> +static inline int fsnotify_parent(const struct path *path,
> +				  struct dentry *dentry, __u32 mask)
>  {
>  	if (!dentry)
>  		dentry = path->dentry;
> @@ -85,8 +108,8 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
>  {
>  	struct inode *source = moved->d_inode;
>  	u32 fs_cookie = fsnotify_get_cookie();
> -	__u32 old_dir_mask = (FS_EVENT_ON_CHILD | FS_MOVED_FROM);
> -	__u32 new_dir_mask = (FS_EVENT_ON_CHILD | FS_MOVED_TO);
> +	__u32 old_dir_mask = FS_MOVED_FROM;
> +	__u32 new_dir_mask = FS_MOVED_TO;

You can just inline these two masks now. There's no point for the variable
anymore.

> @@ -99,8 +122,7 @@ static inline void fsnotify_move(struct inode *old_dir, struct inode *new_dir,
>  
>  	fsnotify(old_dir, old_dir_mask, source, FSNOTIFY_EVENT_INODE, old_name,
>  		 fs_cookie);
> -	fsnotify(new_dir, new_dir_mask, moved, FSNOTIFY_EVENT_DENTRY, new_name,
> -		 fs_cookie);
> +	fsnotify_filename(moved->d_parent, new_dir_mask, new_name, fs_cookie);

The same comment as for the patch 1 here. You should justify that
moved->d_parent is actually the same as new_dir and the dentry cannot
change under us. The same holds for all the calls of fsnotify_d_name()
where the directory inode originally passed in gets replaced with
d_inode(dentry->d_parent).

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