LOR with nfsclient "sillyrename"

[John Baldwin]

On Friday, July 22, 2011 9:11:59 am Kostik Belousov wrote:
> On Fri, Jul 22, 2011 at 08:55:10AM -0400, John Baldwin wrote:
> > On Thursday, July 21, 2011 4:19:59 pm Jeremiah Lott wrote:
> > > We're seeing nfsclient deadlocks with what looks like lock order reversal after removing a "silly rename".  It is fairly rare, but we've 
seen it 
> > happen a few times.  I included relevant back traces from an occurrence.  From what I can see, nfs_inactive() is called with the vnode locked.  
If 
> > there is a silly-rename, it will call vrele() on its parent directory, which can potentially try to lock the parent directory.  Since this is 
the 
> > opposite order of the lock acquisition in lookup, it can deadlock.  This happened in a FreeBSD7 build, but I looked through freebsd head and 
> > didn't see any change that addressed this.  Anyone seen this before?
> > 
> > I haven't seen this before, but your analysis looks correct to me.
> > 
> > Perhaps the best fix would be to defer the actual freeing of the sillyrename
> > to an asynchronous task?  Maybe something like this (untested, uncompiled):
> > 
> > Index: nfsclient/nfsnode.h
> > ===================================================================
> > --- nfsclient/nfsnode.h	(revision 224254)
> > +++ nfsclient/nfsnode.h	(working copy)
> > @@ -36,6 +36,7 @@
> >  #ifndef _NFSCLIENT_NFSNODE_H_
> >  #define _NFSCLIENT_NFSNODE_H_
> >  
> > +#include <sys/_task.h>
> >  #if !defined(_NFSCLIENT_NFS_H_) && !defined(_KERNEL)
> >  #include <nfs/nfs.h>
> >  #endif
> > @@ -45,8 +46,10 @@
> >   * can be removed by nfs_inactive()
> >   */
> >  struct sillyrename {
> > +	struct	task s_task;
> >  	struct	ucred *s_cred;
> >  	struct	vnode *s_dvp;
> > +	struct	vnode *s_vp;
> >  	int	(*s_removeit)(struct sillyrename *sp);
> >  	long	s_namlen;
> >  	char	s_name[32];
> > Index: nfsclient/nfs_vnops.c
> > ===================================================================
> > --- nfsclient/nfs_vnops.c	(revision 224254)
> > +++ nfsclient/nfs_vnops.c	(working copy)
> > @@ -1757,7 +1757,6 @@
> >  {
> >  	/*
> >  	 * Make sure that the directory vnode is still valid.
> > -	 * XXX we should lock sp->s_dvp here.
> >  	 */
> >  	if (sp->s_dvp->v_type == VBAD)
> >  		return (0);
> > @@ -2754,8 +2753,10 @@
> >  		M_NFSREQ, M_WAITOK);
> >  	sp->s_cred = crhold(cnp->cn_cred);
> >  	sp->s_dvp = dvp;
> > +	sp->s_vp = vp;
> >  	sp->s_removeit = nfs_removeit;
> >  	VREF(dvp);
> > +	vhold(vp);
> >  
> >  	/* 
> >  	 * Fudge together a funny name.
> > Index: nfsclient/nfs_node.c
> > ===================================================================
> > --- nfsclient/nfs_node.c	(revision 224254)
> > +++ nfsclient/nfs_node.c	(working copy)
> > @@ -47,6 +47,7 @@
> >  #include <sys/proc.h>
> >  #include <sys/socket.h>
> >  #include <sys/sysctl.h>
> > +#include <sys/taskqueue.h>
> >  #include <sys/vnode.h>
> >  
> >  #include <vm/uma.h>
> > @@ -185,6 +186,26 @@
> >  	return (0);
> >  }
> >  
> > +static void
> > +nfs_freesillyrename(void *arg, int pending)
> > +{
> > +	struct sillyrename *sp;
> > +
> > +	sp = arg;
> > +	vn_lock(sp->s_dvp, LK_SHARED | LK_RETRY);
> I think taking an exclusive lock is somewhat more clean.
> > +	vn_lock(sp->s_vp, LK_EXCLUSIVE | LK_RETRY);
> I believe that you have to verify that at least dvp is not doomed.
> 
> Due to this, I propose to only move the vrele() call to taskqueue.

Fair enough.  It might have been nice to fix the XXX in nfs_removeit() where
it checks the dvp for VBAD without a lock.

-- 
John Baldwin