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

["J. Bruce Fields" <bfields@fieldses.org>]

On Sat, Dec 18, 2010 at 01:01:18PM +1100, Nick Piggin wrote:
> On Fri, Dec 17, 2010 at 01:00:23PM -0500, J. Bruce Fields wrote:
> > From: J. Bruce Fields <bfields@redhat.com>
> > 
> > Without this patch
> > 
> >        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
> >          associated with the nfsd open.  d_obtain_alias() doesn't like
> >          this, so it creates a new DCACHE_DISCONECTED dentry and
> >          returns that instead.
> > 
> > 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.
> > 
> > Signed-off-by: J. Bruce Fields <bfields@redhat.com>
> > ---
> >  fs/dcache.c |   26 ++++++++++++++++++++++++--
> >  1 files changed, 24 insertions(+), 2 deletions(-)
> > 
> > On Tue, Dec 14, 2010 at 05:01:02PM -0500, J. Bruce Fields wrote:
> > > On Mon, Dec 13, 2010 at 04:19:44PM +1100, Nick Piggin wrote:
> > > > Not sure where Al's hiding...
> > > > 
> > > > But I would like to update the comments, and perhaps even a new
> > > > add a new function here (or new flag to __d_find_alias).
> > > > 
> > > > AFAIKS, the callers are OK, however I suppose d_splice_alias and
> > > > d_materialise_unique should not have unlinked inodes at this point,
> > > > so at least a BUG_ON for them might be a good idea?
> > > 
> > > That does sound safer.  I'm pretty confused by the various
> > > __di_splice_alias callers.  I'll go search through them and see if I can
> > > understand better....
> > 
> > I think a new __d_find_alias flag would just make it more confusing.
> > And it looks like all d_obtain_alias needs is something much simpler.
> > This works for me.
> 
> Well I don't really know if this is clearer. By convention,
> __ prefix version should be same as non prefix version but just
> take fewer locks. Why do you have the new d_find_any_alias()?

Argh, apologies; with typo fixed.

--b.

commit 5a911af645aae9992d465d57c397237fb35d1f93
Author: J. Bruce Fields <bfields@redhat.com>
Date:   Fri Dec 17 09:05:04 2010 -0500

    fs/dcache: allow __d_obtain_alias() to return unhashed dentries
    
    Without this patch
    
           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
             associated with the nfsd open.  d_obtain_alias() doesn't like
             this, so it creates a new DCACHE_DISCONECTED dentry and
             returns that instead.
    
    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.
    
    Signed-off-by: J. Bruce Fields <bfields@redhat.com>

diff --git a/fs/dcache.c b/fs/dcache.c
index 5ed93cd..5c014a5 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -1135,6 +1135,28 @@ static inline struct hlist_head *d_hash(struct dentry *parent,
 	return dentry_hashtable + (hash & D_HASHMASK);
 }
 
+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_locked(alias);
+	return alias;
+}
+
+static struct dentry * d_find_any_alias(struct inode *inode)
+{
+	struct dentry *de;
+
+	spin_lock(&dcache_lock);
+	de = __d_find_any_alias(inode);
+	spin_unlock(&dcache_lock);
+	return de;
+}
+
+
 /**
  * d_obtain_alias - find or allocate a dentry for a given inode
  * @inode: inode to allocate the dentry for
@@ -1164,7 +1186,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;
 
@@ -1176,7 +1198,7 @@ struct dentry *d_obtain_alias(struct inode *inode)
 	tmp->d_parent = tmp; /* make sure dput doesn't croak */
 
 	spin_lock(&dcache_lock);
-	res = __d_find_alias(inode, 0);
+	res = __d_find_any_alias(inode);
 	if (res) {
 		spin_unlock(&dcache_lock);
 		dput(tmp);