Re: [PATCH v5 01/17] fsnotify: annotate directory entry modification events

[Jan Kara <jack@suse.cz>]

On Thu 10-01-19 19:04:28, Amir Goldstein wrote:
> "dirent" 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.
> 
> 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 "dirent" event definition. It has no effect on any
> existing backend, because dnotify, inotify and audit always requets the
> child events and fanotify does not get the delete,rename events.
> 
> The fsnotify_dirent() helper is used instead of fsnotify_parent() to
> report a dirent event to dentry->d_parent without FS_EVENT_ON_CHILD
> and regardless if parent has the FS_EVENT_ON_CHILD bit set.
> 
> Unlike fsnotify_parent(), fsnotify_dirent() assumes that dentry->d_name
> and dentry->d_parent are stable. For fsnotify_create()/fsnotify_mkdir(),
> this assumption is abviously correct. For fsnotify_nameremove(), it is
> less trvial, so we use dget_parent() and take_dentry_name_snapshot() to
> grab stable references.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

I've taken the liberty of removing the case analysis from
fsnotify_nameremove() comment. That is bound to become out of date pretty
soon and not very useful for the reader. Otherwise the patch looks good and
I've added it to my tree.

								Honza

> ---
>  include/linux/fsnotify.h | 68 ++++++++++++++++++++++++++++++++++------
>  1 file changed, 59 insertions(+), 9 deletions(-)
> 
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index 2ccb08cb5d6a..116907928c7f 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -17,8 +17,22 @@
>  #include <linux/slab.h>
>  #include <linux/bug.h>
>  
> +/*
> + * Notify this @dir inode about a change in the directory entry @dentry.
> + *
> + * Unlike fsnotify_parent(), the event will be reported regardless of the
> + * FS_EVENT_ON_CHILD mask on the parent inode.
> + */
> +static inline int fsnotify_dirent(struct inode *dir, struct dentry *dentry,
> +				  __u32 mask)
> +{
> +	return fsnotify(dir, mask, d_inode(dentry), FSNOTIFY_EVENT_INODE,
> +			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 +99,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;
>  	const unsigned char *new_name = moved->d_name.name;
>  
>  	if (old_dir == new_dir)
> @@ -128,15 +142,54 @@ static inline void fsnotify_vfsmount_delete(struct vfsmount *mnt)
>  
>  /*
>   * fsnotify_nameremove - a filename was removed from a directory
> + *
> + * Called from d_delete() and nfs_complete_sillyrename().
> + * The latter is called from nfs client ->unlink() ->rmdir() ->rename()
> + * under parent vfs inode lock.
> + *
> + * Most filesystems call d_delete() from ->unlink() ->rmdir() ->rename()
> + * ops under parent vfs inode lock.
> + *
> + * Some pseudo filesystems call d_delete() without parent inode lock.
> + * Those filesystems have no ->rename() op and they do not call
> + * d_move() directly, so d_parent and d_name are stable by definition.
> + * Examples: devpts, efivarfs, rpc_pipefs, functionfs.
> + *
> + * Some clustered filesystems call d_delete() on remote nodes, not under
> + * vfs parent inode lock, but they use cluster distributed locks on local
> + * and remote nodes. Those filesystems call d_delete() under their cluster
> + * lock. Examples:
> + * - in ceph_fill_trace() under CEPH_MDS_R_PARENT_LOCKED
> + * - in ocfs2_dentry_convert_worker() under ocfs2_dentry_lock
> + * But those filesystems also call d_move() under the same cluster lock
> + * (i.e. FS_RENAME_DOES_D_MOVE), so d_parent and d_name are also stable.
> + *
> + * However, to be on the safe side and be reselient to future callers
> + * and out of tree users of d_delete(), we do not assume that d_parent and
> + * d_name are stable and we use dget_parent() and take_dentry_name_snapshot()
> + * to grab stable references.
>   */
>  static inline void fsnotify_nameremove(struct dentry *dentry, int isdir)
>  {
> +	struct dentry *parent;
> +	struct name_snapshot name;
>  	__u32 mask = FS_DELETE;
>  
> +	/* d_delete() of pseudo inode? (e.g. __ns_get_path() playing tricks) */
> +	if (IS_ROOT(dentry))
> +		return;
> +
>  	if (isdir)
>  		mask |= FS_ISDIR;
>  
> -	fsnotify_parent(NULL, dentry, mask);
> +	parent = dget_parent(dentry);
> +	take_dentry_name_snapshot(&name, dentry);
> +
> +	fsnotify(d_inode(parent), mask, d_inode(dentry), FSNOTIFY_EVENT_INODE,
> +		 name.name, 0);
> +
> +	release_dentry_name_snapshot(&name);
> +	dput(parent);
>  }
>  
>  /*
> @@ -155,7 +208,7 @@ static inline void fsnotify_create(struct inode *inode, struct dentry *dentry)
>  {
>  	audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE);
>  
> -	fsnotify(inode, FS_CREATE, dentry->d_inode, FSNOTIFY_EVENT_INODE, dentry->d_name.name, 0);
> +	fsnotify_dirent(inode, dentry, FS_CREATE);
>  }
>  
>  /*
> @@ -176,12 +229,9 @@ static inline void fsnotify_link(struct inode *dir, struct inode *inode, struct
>   */
>  static inline void fsnotify_mkdir(struct inode *inode, struct dentry *dentry)
>  {
> -	__u32 mask = (FS_CREATE | FS_ISDIR);
> -	struct inode *d_inode = dentry->d_inode;
> -
>  	audit_inode_child(inode, dentry, AUDIT_TYPE_CHILD_CREATE);
>  
> -	fsnotify(inode, mask, d_inode, FSNOTIFY_EVENT_INODE, dentry->d_name.name, 0);
> +	fsnotify_dirent(inode, dentry, FS_CREATE | FS_ISDIR);
>  }
>  
>  /*
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR