LOR with nfsclient "sillyrename"

[Kostik Belousov]

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.
> +	(void)nfs_vinvalbuf(ap->a_vp, 0, td, 1);
> +	/*
> +	 * Remove the silly file that was rename'd earlier
> +	 */
> +	(sp->s_removeit)(sp);
> +	crfree(sp->s_cred);
> +	vput(sp->s_dvp);
> +	VOP_UNLOCK(sp->s_vp, 0);
> +	vdrop(sp->s_vp);
> +	free((caddr_t)sp, M_NFSREQ);
> +}
> +
>  int
>  nfs_inactive(struct vop_inactive_args *ap)
>  {
> @@ -200,15 +221,9 @@
>  	} else
>  		sp = NULL;
>  	if (sp) {
> +		TASK_INIT(&sp->task, 0, nfs_freesillyrename, sp);
> +		taskqueue_enqueue(taskqueue_thread, &sp->task);
>  		mtx_unlock(&np->n_mtx);
> -		(void)nfs_vinvalbuf(ap->a_vp, 0, td, 1);
> -		/*
> -		 * Remove the silly file that was rename'd earlier
> -		 */
> -		(sp->s_removeit)(sp);
> -		crfree(sp->s_cred);
> -		vrele(sp->s_dvp);
> -		free((caddr_t)sp, M_NFSREQ);
>  		mtx_lock(&np->n_mtx);
>  	}
>  	np->n_flag &= NMODIFIED;
> 
> -- 
> John Baldwin
> _______________________________________________
> freebsd-net at freebsd.org mailing list
> http://lists.freebsd.org/mailman/listinfo/freebsd-net
> To unsubscribe, send any mail to "freebsd-net-unsubscribe at freebsd.org"
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-net/attachments/20110722/1ac71a1c/attachment.pgp