Re: [PATCH] [REPOST] fuse: drop dentry on failed revalidate

[Ric Wheeler <rwheeler@redhat.com>]

On 08/01/2013 12:39 PM, Miklos Szeredi wrote:
> On Tue, Jul 30, 2013 at 03:30:45PM -0400, Ric Wheeler wrote:
>> On 07/30/2013 12:16 PM, Miklos Szeredi wrote:
>>>>>    fs/fuse/dir.c | 4 ++++
>>>>>    1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
>>>>> index a1d9047..83c217e 100644
>>>>> --- a/fs/fuse/dir.c
>>>>> +++ b/fs/fuse/dir.c
>>>>> @@ -226,6 +226,10 @@ static int fuse_dentry_revalidate(struct dentry
>>>>> *entry, unsigned int flags)
>>>>>                  if (!err) {
>>>>>                          struct fuse_inode *fi = get_fuse_inode(inode);
>>>>>                          if (outarg.nodeid != get_node_id(inode)) {
>>>>> +                               if (!have_submounts(entry)) {
>>>>> +                                       shrink_dcache_parent(entry);
>>>>> +                                       d_drop(entry);
>>>>> +                               }
>>> Doing d_drop() on a subtree really has problems.  Take this example:
>>>
>>> cd /mnt/foo/bar
>>> mkdir my-mount-point
>>> [->d_revalidate() drops "/mnt/foo"]
>>> mount whatever on ./my-mount-point
>>> cd /
>>>
>>> At this point that mount is not accessible in any way.  The only way
>>> to umount it is to lazy umount the parent mount.  If the parent mount
>>> was root, then that's not a practical thing to do.
>>>
>>> AFAICS nothing prevents this from happening on NFS and root privs are
>>> not required (e.g. mounting a fuse fs on an NFS home dir).
>>>
> Here's a patch that tries to address the mount issue in NFS (and in FUSE in the
> future).  I haven't yet tested it, just interested in whether the concept looks
> OK or not.
>
> Not sure if the ENOENT is the best error.  Also I have a little fear that this
> will cause a regression in some weird setup.  Possibly we should use a new
> dentry flag for unhashing the directory dentry specifically to invalidate it.
>
> Thanks,
> Miklos

Adding in the NFS list for broader review.

I thought that the issue was primarily in FUSE itself, not seen in practice in NFS?

Ric

>
>
>
> diff --git a/fs/dcache.c b/fs/dcache.c
> index 87bdb53..5c51217 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -1103,6 +1103,34 @@ rename_retry:
>   }
>   EXPORT_SYMBOL(have_submounts);
>   
> +static bool __has_unlinked_ancestor(struct dentry *dentry)
> +{
> +	struct dentry *this;
> +
> +	for (this = dentry; !IS_ROOT(this); this = this->d_parent) {
> +		if (d_unhashed(this))
> +			return true;
> +	}
> +	return false;
> +}
> +
> +/*
> + * Called by mount code to check if the mountpoint is reachable (e.g. NFS can
> + * unhash a directory dentry and then the complete subtree can become
> + * unreachable).
> + */
> +bool has_unlinked_ancestor(struct dentry *dentry)
> +{
> +	bool found;
> +
> +	/* Need exclusion wrt. have_submounts() */
> +	write_seqlock(&rename_lock);
> +	found = __has_unlinked_ancestor(dentry);
> +	write_sequnlock(&rename_lock);
> +
> +	return found;
> +}
> +
>   /*
>    * Search the dentry child list of the specified parent,
>    * and move any unused dentries to the end of the unused
> diff --git a/fs/internal.h b/fs/internal.h
> index 7c5f01c..d232355 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -126,6 +126,7 @@ extern int invalidate_inodes(struct super_block *, bool);
>    * dcache.c
>    */
>   extern struct dentry *__d_alloc(struct super_block *, const struct qstr *);
> +extern bool has_unlinked_ancestor(struct dentry *dentry);
>   
>   /*
>    * read_write.c
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 7b1ca9b..bb92a9c 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -634,6 +634,15 @@ static struct mountpoint *new_mountpoint(struct dentry *dentry)
>   	}
>   	dentry->d_flags |= DCACHE_MOUNTED;
>   	spin_unlock(&dentry->d_lock);
> +
> +	if (has_unlinked_ancestor(dentry)) {
> +		spin_lock(&dentry->d_lock);
> +		dentry->d_flags &= ~DCACHE_MOUNTED;
> +		spin_unlock(&dentry->d_lock);
> +		kfree(mp);
> +		return ERR_PTR(-ENOENT);
> +	}
> +
>   	mp->m_dentry = dentry;
>   	mp->m_count = 1;
>   	list_add(&mp->m_hash, chain);