Re: [PATCH -V2 05/13] fsnotify: parent event notification

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 27 Mar 2009 16:05:32 -0400
Eric Paris <eparis@redhat.com> wrote:

> inotify and dnotify both use a similar parent notification mechanism.  We
> add a generic parent notification mechanism to fsnotify for both of these
> to use.  This new machanism also adds the dentry flag optimization which
> exists for inotify to dnotify.
> 
>
> ...
>
>  /*
> + * Given an inode, first check if we care what happens to out children.  Inotify

"our"

> + * and dnotify both tell their parents about events.  If we care about any event
> + * on a child we run all of our children and set a dentry flag saying that the
> + * parent cares.  Thus when an event happens on a child it can quickly tell if
> + * if there is a need to find a parent and send the event to the parent.
> + */
> +static inline void fsnotify_update_dentry_child_flags(struct inode *inode)
> +{
> +	struct dentry *alias;
> +	int watched;
> +
> +	if (!S_ISDIR(inode->i_mode))
> +		return;
> +
> +	/* determine if the children should tell inode about their events */
> +	watched = fsnotify_inode_watches_children(inode);
> +
> +	spin_lock(&dcache_lock);
> +	/* run all of the dentries associated with this inode.  Since this is a
> +	 * directory, there damn well better only be one item on this list */
> +	list_for_each_entry(alias, &inode->i_dentry, d_alias) {
> +		struct dentry *child;
> +
> +		/* run all of the children of the original inode and fix their
> +		 * d_flags to indicate parental interest (their parent is the
> +		 * original inode) */
> +		list_for_each_entry(child, &alias->d_subdirs, d_u.d_child) {
> +			if (!child->d_inode)
> +				continue;
> +
> +			spin_lock(&child->d_lock);
> +			if (watched)
> +				child->d_flags |= DCACHE_FSNOTIFY_PARENT_WATCHED;
> +			else
> +				child->d_flags &= ~DCACHE_FSNOTIFY_PARENT_WATCHED;
> +			spin_unlock(&child->d_lock);
> +		}
> +	}
> +	spin_unlock(&dcache_lock);
> +}

Huge function, three callsites, way too large to inline!

afacit all these DCACHE_FSNOTIFY_PARENT_WATCHED bits are left set
without suitable locks being held.  What prevents different threads of
control from setting and clearing them under each others' feet?

The comment should be updated to answer this, please.

>
> ...
>
> +static inline void fsnotify_d_instantiate(struct dentry *dentry, struct inode *inode)
>  {
> -	inotify_d_instantiate(entry, inode);
> +	__fsnotify_d_instantiate(dentry, inode);
> +
> +	/* call the legacy inotify shit */
> +	inotify_d_instantiate(dentry, inode);
> +}
> +
> +/* Notify this dentry's parent about a child's events. */
> +static inline void fsnotify_parent(struct dentry *dentry, __u32 mask)
> +{
> +	struct dentry *parent;
> +	struct inode *p_inode;
> +	char send = 0;
> +
> +	if (!(dentry->d_flags | DCACHE_FSNOTIFY_PARENT_WATCHED))
> +		return;
> +
> +	/* we are notifying a parent so come up with the new mask which
> +	 * specifies these are events which came from a child. */
> +	mask |= FS_EVENT_ON_CHILD;
> +
> +	spin_lock(&dentry->d_lock);
> +	parent = dentry->d_parent;
> +	p_inode = parent->d_inode;
> +
> +	if (p_inode->i_fsnotify_mask & mask) {
> +		dget(parent);
> +		send = 1;
> +	}
> +
> +	spin_unlock(&dentry->d_lock);
> +
> +	if (send) {
> +		fsnotify(p_inode, mask, dentry->d_inode, FSNOTIFY_EVENT_INODE);
> +		dput(parent);
> +	}
>  }

I hereby revoke your inlining license.

>
> ...
>
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -45,8 +45,17 @@
>  #define FS_DN_RENAME		0x10000000ul	/* file renamed */
>  #define FS_DN_MULTISHOT		0x20000000ul	/* dnotify multishot */
>  
> +/* this inode cares about things that happen to it's children.  Always set for

"its"

> + * dnotify and inotify.  never set for fanotify */

You might want to go through the comments and start sentences with
capital latters :(

>  #define FS_EVENT_ON_CHILD	0x08000000ul
>  
> +/* this is a list of all events that may get sent to a parernt based on fs event
> + * happening to inodes inside that directory */
>
> ...
>