Re: [PATCH] fs/dcache: allow __d_obtain_alias() to return unhashed dentries

From: "J. Bruce Fields" <bfields-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org>
To: Nick Piggin <npiggin-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Alexander Viro
	<viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn@public.gmane.org>,
	Nick Piggin <npiggin-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org>,
	linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
	linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Subject: Re: [PATCH] fs/dcache: allow __d_obtain_alias() to return unhashed dentries
Date: Tue, 18 Jan 2011 17:08:17 -0500	[thread overview]
Message-ID: <20110118220817.GF10903@fieldses.org> (raw)
In-Reply-To: <AANLkTikL2CDSWQJ1QH_Y4G-j70Vd=VesNMMnYTmMGHC9-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>

On Wed, Jan 19, 2011 at 09:02:59AM +1100, Nick Piggin wrote:
> On Wed, Jan 19, 2011 at 7:45 AM, J. Bruce Fields <bfields-uC3wQj2KruMpug/h7KTFAQ@public.gmane.orgg> wrote:
> > @@ -1521,6 +1521,28 @@ struct dentry * d_alloc_root(struct inode * root_inode)
> >  }
> >  EXPORT_SYMBOL(d_alloc_root);
> >
> > +static struct dentry * __d_find_any_alias(struct inode *inode)
> > +{
> > +       struct dentry *alias;
> > +
> > +       if (list_empty(&inode->i_dentry))
> > +               return NULL;
> > +       alias = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
> > +       __dget(alias);
> > +       return alias;
> > +}
> > +
> > +static struct dentry * d_find_any_alias(struct inode *inode)
> > +{
> > +       struct dentry *de;
> > +
> > +       spin_lock(&inode->i_lock);
> 
> Yes, i_dentry/d_alias is protected by i_lock, so it looks fine.

OK, thanks; I'm assuming it's OK to re-add your ack in that case.  Al,
could you apply?  (Or if this should go in through an nfsd tree, or some
other way, let me know.)

--b.

From: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date: Fri, 17 Dec 2010 09:05:04 -0500
Subject: [PATCH] fs/dcache: allow __d_obtain_alias() to return unhashed dentries

Without this patch, inodes are not promptly freed on last close of an
unlinked file by an nfs client:

       client$ mount -tnfs4 server:/export/ /mnt/
       client$ tail -f /mnt/FOO
       ...
       server$ df -i /export
       server$ rm /export/FOO
       (^C the tail -f)
       server$ df -i /export
       server$ echo 2 >/proc/sys/vm/drop_caches
       server$ df -i /export

the df's will show that the inode is not freed on the filesystem until
the last step, when it could have been freed after killing the client's
tail -f.  On-disk data won't be deallocated either, leading to possible
spurious ENOSPC.

This occurs because when the client does the close, it arrives in a
compound with a putfh and a close, processed like:

	- putfh: look up the filehandle.  The only alias found for the
	  inode will be DCACHE_UNHASHED alias referenced by the filp
	  this, so it creates a new DCACHE_DISCONECTED dentry and
	  returns that instead.
	- close: closes the existing filp, which is destroyed
	  immediately by dput() since it's DCACHE_UNHASHED.
	- end of the compound: release the reference
	  to the current filehandle, and dput() the new
	  DCACHE_DISCONECTED dentry, which gets put on the
	  unused list instead of being destroyed immediately.

Nick Piggin suggested fixing this by allowing d_obtain_alias to return
the unhashed dentry that is referenced by the filp, instead of making it
create a new dentry.

Leave __d_find_alias() alone to avoid changing behavior of other
callers.

Also nfsd doesn't need all the checks of __d_find_alias(); any dentry,
hashed or unhashed, disconnected or not, should work.

Acked-by: Nick Piggin <npiggin-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org>
Signed-off-by: J. Bruce Fields <bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 fs/dcache.c |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 9f493ee..2849258 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1521,6 +1521,28 @@ struct dentry * d_alloc_root(struct inode * root_inode)
 }
 EXPORT_SYMBOL(d_alloc_root);
 
+static struct dentry * __d_find_any_alias(struct inode *inode)
+{
+	struct dentry *alias;
+
+	if (list_empty(&inode->i_dentry))
+		return NULL;
+	alias = list_first_entry(&inode->i_dentry, struct dentry, d_alias);
+	__dget(alias);
+	return alias;
+}
+
+static struct dentry * d_find_any_alias(struct inode *inode)
+{
+	struct dentry *de;
+
+	spin_lock(&inode->i_lock);
+	de = __d_find_any_alias(inode);
+	spin_unlock(&inode->i_lock);
+	return de;
+}
+
+
 /**
  * d_obtain_alias - find or allocate a dentry for a given inode
  * @inode: inode to allocate the dentry for
@@ -1550,7 +1572,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
 	if (IS_ERR(inode))
 		return ERR_CAST(inode);
 
-	res = d_find_alias(inode);
+	res = d_find_any_alias(inode);
 	if (res)
 		goto out_iput;
 
@@ -1563,7 +1585,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
 
 
 	spin_lock(&inode->i_lock);
-	res = __d_find_alias(inode, 0);
+	res = __d_find_any_alias(inode);
 	if (res) {
 		spin_unlock(&inode->i_lock);
 		dput(tmp);
-- 
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
