Re: [PATCH 1/3] fsnotify: tree-watching support

[Eric Paris <eparis@redhat.com>]

On Fri, 2010-02-19 at 14:46 +0100, Joris Dolderer wrote:
> Add tree-watching support to fsnotify.
> See http://patchwork.kernel.org/patch/79171/ and http://patchwork.kernel.org/patch/79172/ for other patches.
> Signed-off-by: Joris Dolderer <vorstadtkind@googlemail.com>
> ---
>  fs/debugfs/inode.c               |    8 +
>  fs/namei.c                       |   12 +-
>  fs/notify/fsnotify.c             |  189 +++++++++++++++++++++++++++++++--------
>  fs/notify/fsnotify.h             |    1 
>  fs/notify/inode_mark.c           |   46 ++++++++-
>  include/linux/dcache.h           |    3 
>  include/linux/fsnotify.h         |   55 +++++++----
>  include/linux/fsnotify_backend.h |   51 +++++++---
>  8 files changed, 282 insertions(+), 83 deletions(-)
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 037e878..17cd902 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -76,55 +76,165 @@ void __fsnotify_update_child_dentry_flags(struct inode *inode)
>  	spin_unlock(&dcache_lock);
>  }
>  
> -/* Notify this dentry's parent about a child's events. */
> -void __fsnotify_parent(struct dentry *dentry, __u32 mask)
> +/*
> + * Does the work when updating descents of a dentry
> + */
> +static void fsnotify_update_descents(struct dentry *dentry, bool watched)
>  {
> -	struct dentry *parent;
> -	struct inode *p_inode;
> -	bool send = false;
> -	bool should_update_children = false;
> +	struct dentry *child;
> +
> +	list_for_each_entry(child, &dentry->d_subdirs, d_u.d_child) {
> +		if (!child->d_inode)
> +			continue;
> +
> +		spin_lock(&child->d_lock);
> +
> +		if (watched)
> +			child->d_flags |= DCACHE_FSNOTIFY_ANCESTOR_WATCHED;
> +		else
> +			child->d_flags &= ~DCACHE_FSNOTIFY_ANCESTOR_WATCHED;
> +
> +		if (!fsnotify_inode_watches_descents(child->d_inode)) {
> +			fsnotify_update_descents(child, watched);
> +		}
> +
> +		spin_unlock(&child->d_lock);
> +	}
> +}

I haven't tested the code at all, just started to even peek at it and
only made it this far.....

Did you run this with lockdep enabled?  Turn it on, understand and fix
anything it complains about.

Any kind of recursion, especially that which is not tail recursion is
highly frowned upon.

What happens when you do

mkdir /tmp/subdir
ln -s ../ /tmp/subdir/upone

and then put a recursive watch on /tmp?

I'm betting deadlock for 1 of 2 reasons, either you

A) try to take the dentry->d_lock on subdir twice (deadlock)
B) run indefinitely around the /tmp -> /tmp/subdir -> /tmp/subdir/upone
-> /tmp/subidr -> /tmp/subdir/upone -> ... loop.

Maybe I'm missing something, but both of these make this a
non-starter....

Maybe symlinks don't break like that, but I'm betting I could do A with
bind mounts if not B.....

-Eric