From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nick Piggin Subject: Re: [PATCH 2/2] fs/dcache: allow __d_obtain_alias() to return unhashed dentries Date: Mon, 20 Dec 2010 01:53:10 +1100 Message-ID: References: <20101115174837.GB10044@fieldses.org> <20101129193248.GA9897@fieldses.org> <20101203223326.GB28763@fieldses.org> <20101213051944.GA8688@amd> <20101214220102.GM24828@fieldses.org> <20101217180022.GB11515@fieldses.org> <20101218020118.GA3179@amd> <20101218161609.GA22150@fieldses.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Nick Piggin , Alexander Viro , linux-nfs-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: "J. Bruce Fields" Return-path: In-Reply-To: <20101218161609.GA22150-uC3wQj2KruNg9hUCZPvPmw@public.gmane.org> Sender: linux-nfs-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-fsdevel.vger.kernel.org On Sun, Dec 19, 2010 at 3:16 AM, J. Bruce Fields = wrote: > 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 >> > On Tue, Dec 14, 2010 at 05:01:02PM -0500, J. Bruce Fields wrote: >> > I think a new __d_find_alias flag would just make it more confusin= g. >> > And it looks like all d_obtain_alias needs is something much simpl= er. >> > 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. OK that makes a lot more sense :) > --b. > > commit 5a911af645aae9992d465d57c397237fb35d1f93 > Author: J. Bruce Fields > Date: =A0 Fri Dec 17 09:05:04 2010 -0500 > > =A0 =A0fs/dcache: allow __d_obtain_alias() to return unhashed dentrie= s > > =A0 =A0Without this patch > > =A0 =A0=A0 =A0 =A0 =A0client$ mount -tnfs4 server:/export/ /mnt/ > =A0 =A0=A0 =A0 =A0 =A0client$ tail -f /mnt/FOO > =A0 =A0=A0 =A0 =A0 =A0... > =A0 =A0=A0 =A0 =A0 =A0server$ df -i /export > =A0 =A0=A0 =A0 =A0 =A0server$ rm /export/FOO > =A0 =A0=A0 =A0 =A0 =A0(^C the tail -f) > =A0 =A0=A0 =A0 =A0 =A0server$ df -i /export > =A0 =A0=A0 =A0 =A0 =A0server$ echo 2 >/proc/sys/vm/drop_caches > =A0 =A0=A0 =A0 =A0 =A0server$ df -i /export > > =A0 =A0the df's will show that the inode is not freed on the filesyst= em until > =A0 =A0the last step, when it could have been freed after killing the= client's > =A0 =A0tail -f. =A0On-disk data won't be deallocated either, leading = to possible > =A0 =A0spurious ENOSPC. > > =A0 =A0This occurs because when the client does the close, it arrives= in a > =A0 =A0compound with a putfh and a close, processed like: > > =A0 =A0=A0 =A0 =A0 =A0- putfh: look up the filehandle. =A0The only al= ias found for the > =A0 =A0=A0 =A0 =A0 =A0 =A0inode will be DCACHE_UNHASHED alias referen= ced by the filp > =A0 =A0=A0 =A0 =A0 =A0 =A0associated with the nfsd open. =A0d_obtain_= alias() doesn't like > =A0 =A0=A0 =A0 =A0 =A0 =A0this, so it creates a new DCACHE_DISCONECTE= D dentry and > =A0 =A0=A0 =A0 =A0 =A0 =A0returns that instead. > > =A0 =A0Nick Piggin suggested fixing this by allowing d_obtain_alias t= o return > =A0 =A0the unhashed dentry that is referenced by the filp, instead of= making it > =A0 =A0create a new dentry. > > =A0 =A0Leave __d_find_alias() alone to avoid changing behavior of oth= er > =A0 =A0callers. > > =A0 =A0Also nfsd doesn't need all the checks of __d_find_alias(); any= dentry, > =A0 =A0hashed or unhashed, disconnected or not, should work. Yeah, because when the call returns, the name could be concurrently unhashed, but the nfsd operation must still work (either handle the rac= e or be unaffected by it) presumably. Therefore: hashed status should not matter here. Acked-by: Nick Piggin > > =A0 =A0Signed-off-by: J. Bruce Fields > > 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, > =A0 =A0 =A0 =A0return dentry_hashtable + (hash & D_HASHMASK); > =A0} > > +static struct dentry * __d_find_any_alias(struct inode *inode) > +{ > + =A0 =A0 =A0 struct dentry *alias; > + > + =A0 =A0 =A0 if (list_empty(&inode->i_dentry)) > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return NULL; > + =A0 =A0 =A0 alias =3D list_first_entry(&inode->i_dentry, struct den= try, d_alias); > + =A0 =A0 =A0 __dget_locked(alias); > + =A0 =A0 =A0 return alias; > +} > + > +static struct dentry * d_find_any_alias(struct inode *inode) > +{ > + =A0 =A0 =A0 struct dentry *de; > + > + =A0 =A0 =A0 spin_lock(&dcache_lock); > + =A0 =A0 =A0 de =3D __d_find_any_alias(inode); > + =A0 =A0 =A0 spin_unlock(&dcache_lock); > + =A0 =A0 =A0 return de; > +} > + > + > =A0/** > =A0* d_obtain_alias - find or allocate a dentry for a given inode > =A0* @inode: inode to allocate the dentry for > @@ -1164,7 +1186,7 @@ struct dentry *d_obtain_alias(struct inode *ino= de) > =A0 =A0 =A0 =A0if (IS_ERR(inode)) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return ERR_CAST(inode); > > - =A0 =A0 =A0 res =3D d_find_alias(inode); > + =A0 =A0 =A0 res =3D d_find_any_alias(inode); > =A0 =A0 =A0 =A0if (res) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto out_iput; > > @@ -1176,7 +1198,7 @@ struct dentry *d_obtain_alias(struct inode *ino= de) > =A0 =A0 =A0 =A0tmp->d_parent =3D tmp; /* make sure dput doesn't croak= */ > > =A0 =A0 =A0 =A0spin_lock(&dcache_lock); > - =A0 =A0 =A0 res =3D __d_find_alias(inode, 0); > + =A0 =A0 =A0 res =3D __d_find_any_alias(inode); > =A0 =A0 =A0 =A0if (res) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0spin_unlock(&dcache_lock); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dput(tmp); > -- 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