From mboxrd@z Thu Jan 1 00:00:00 1970 From: "J. Bruce Fields" Subject: Re: [PATCH] fs/dcache: allow __d_obtain_alias() to return unhashed dentries Date: Tue, 8 Mar 2011 13:13:20 -0500 Message-ID: <20110308181320.GA15566@fieldses.org> References: <20101213051944.GA8688@amd> <20101214220102.GM24828@fieldses.org> <20101217180022.GB11515@fieldses.org> <20101218020118.GA3179@amd> <20101218161609.GA22150@fieldses.org> <20101227234641.GA22248@fieldses.org> <20110118204509.GA10903@fieldses.org> <20110118220817.GF10903@fieldses.org> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Alexander Viro , Nick Piggin , linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Nick Piggin Return-path: Content-Disposition: inline In-Reply-To: <20110118220817.GF10903-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> Sender: linux-nfs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-fsdevel.vger.kernel.org On Tue, Jan 18, 2011 at 05:08:17PM -0500, J. Bruce Fields wrote: > 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 wrote: > > > @@ -1521,6 +1521,28 @@ struct dentry * d_alloc_root(struct inode = * root_inode) > > > =C2=A0} > > > =C2=A0EXPORT_SYMBOL(d_alloc_root); > > > > > > +static struct dentry * __d_find_any_alias(struct inode *inode) > > > +{ > > > + =C2=A0 =C2=A0 =C2=A0 struct dentry *alias; > > > + > > > + =C2=A0 =C2=A0 =C2=A0 if (list_empty(&inode->i_dentry)) > > > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return NULL; > > > + =C2=A0 =C2=A0 =C2=A0 alias =3D list_first_entry(&inode->i_dentr= y, struct dentry, d_alias); > > > + =C2=A0 =C2=A0 =C2=A0 __dget(alias); > > > + =C2=A0 =C2=A0 =C2=A0 return alias; > > > +} > > > + > > > +static struct dentry * d_find_any_alias(struct inode *inode) > > > +{ > > > + =C2=A0 =C2=A0 =C2=A0 struct dentry *de; > > > + > > > + =C2=A0 =C2=A0 =C2=A0 spin_lock(&inode->i_lock); > >=20 > > Yes, i_dentry/d_alias is protected by i_lock, so it looks fine. >=20 > 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 s= ome > other way, let me know.) Al, do you have this in your queue to look at? Need me to resend? Or should it take some other route? --b. >=20 > --b. >=20 > From: J. Bruce Fields > Date: Fri, 17 Dec 2010 09:05:04 -0500 > Subject: [PATCH] fs/dcache: allow __d_obtain_alias() to return unhash= ed dentries >=20 > Without this patch, inodes are not promptly freed on last close of an > unlinked file by an nfs client: >=20 > =C2=A0 =C2=A0 =C2=A0 =C2=A0client$ mount -tnfs4 server:/export/ /mnt/ > =C2=A0 =C2=A0 =C2=A0 =C2=A0client$ tail -f /mnt/FOO > =C2=A0 =C2=A0 =C2=A0 =C2=A0... > =C2=A0 =C2=A0 =C2=A0 =C2=A0server$ df -i /export > =C2=A0 =C2=A0 =C2=A0 =C2=A0server$ rm /export/FOO > =C2=A0 =C2=A0 =C2=A0 =C2=A0(^C the tail -f) > =C2=A0 =C2=A0 =C2=A0 =C2=A0server$ df -i /export > =C2=A0 =C2=A0 =C2=A0 =C2=A0server$ echo 2 >/proc/sys/vm/drop_caches > =C2=A0 =C2=A0 =C2=A0 =C2=A0server$ df -i /export >=20 > the df's will show that the inode is not freed on the filesystem unti= l > the last step, when it could have been freed after killing the client= 's > tail -f. =C2=A0On-disk data won't be deallocated either, leading to p= ossible > spurious ENOSPC. >=20 > This occurs because when the client does the close, it arrives in a > compound with a putfh and a close, processed like: >=20 > - putfh: look up the filehandle. =C2=A0The 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. >=20 > Nick Piggin suggested fixing this by allowing d_obtain_alias to retur= n > the unhashed dentry that is referenced by the filp, instead of making= it > create a new dentry. >=20 > Leave __d_find_alias() alone to avoid changing behavior of other > callers. >=20 > Also nfsd doesn't need all the checks of __d_find_alias(); any dentry= , > hashed or unhashed, disconnected or not, should work. >=20 > Acked-by: Nick Piggin > Signed-off-by: J. Bruce Fields > --- > fs/dcache.c | 26 ++++++++++++++++++++++++-- > 1 files changed, 24 insertions(+), 2 deletions(-) >=20 > 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 * ro= ot_inode) > } > EXPORT_SYMBOL(d_alloc_root); > =20 > +static struct dentry * __d_find_any_alias(struct inode *inode) > +{ > + struct dentry *alias; > + > + if (list_empty(&inode->i_dentry)) > + return NULL; > + alias =3D 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 =3D __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 *ino= de) > if (IS_ERR(inode)) > return ERR_CAST(inode); > =20 > - res =3D d_find_alias(inode); > + res =3D d_find_any_alias(inode); > if (res) > goto out_iput; > =20 > @@ -1563,7 +1585,7 @@ struct dentry *d_obtain_alias(struct inode *ino= de) > =20 > =20 > spin_lock(&inode->i_lock); > - res =3D __d_find_alias(inode, 0); > + res =3D __d_find_any_alias(inode); > if (res) { > spin_unlock(&inode->i_lock); > dput(tmp); > --=20 > 1.7.1 >=20 -- 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