Date: Fri, 22 Jul 2011 17:11:26 -0400 (EDT) From: Rick Macklem <rmacklem@uoguelph.ca> To: Jeremiah Lott <jlott@averesystems.com> Cc: freebsd-net@freebsd.org, kib@freebsd.org, John Baldwin <jhb@freebsd.org> Subject: Re: LOR with nfsclient "sillyrename" Message-ID: <1931983408.909766.1311369086133.JavaMail.root@erie.cs.uoguelph.ca> In-Reply-To: <44626428-CF14-4B20-AB57-6D4E8F4678AE@averesystems.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --] 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? > > Jeremiah Lott > Avere Systems > Please try the attached patch (which is also at): http://people.freebsd.org/~rmacklem/oldsilly.patch http://people.freebsd.org/~rmacklem/newsilly.patch (for the old and new clients in -current, respectively) - I think oldsilly.patch should apply to the 7.n kernel sources, although you might have to do the edit by hand? The patch is based on what jhb@ posted, with changes as recommended by kib@. Please let me know how testing goes with it, rick ps: Kostik, could you please review this, thanks. [-- Attachment #2 --] --- nfsclient/nfsnode.h.sav 2011-07-22 15:31:11.000000000 -0400 +++ nfsclient/nfsnode.h 2011-07-22 15:32:54.000000000 -0400 @@ -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,6 +46,7 @@ * can be removed by nfs_inactive() */ struct sillyrename { + struct task s_task; struct ucred *s_cred; struct vnode *s_dvp; int (*s_removeit)(struct sillyrename *sp); --- nfsclient/nfs_node.c.sav 2011-07-22 15:33:04.000000000 -0400 +++ nfsclient/nfs_node.c 2011-07-22 16:31:45.000000000 -0400 @@ -47,6 +47,7 @@ __FBSDID("$FreeBSD: head/sys/nfsclient/n #include <sys/proc.h> #include <sys/socket.h> #include <sys/sysctl.h> +#include <sys/taskqueue.h> #include <sys/vnode.h> #include <vm/uma.h> @@ -59,6 +60,8 @@ __FBSDID("$FreeBSD: head/sys/nfsclient/n static uma_zone_t nfsnode_zone; +static void nfs_freesillyrename(void *arg, __unused int pending); + #define TRUE 1 #define FALSE 0 @@ -185,6 +188,20 @@ nfs_nget(struct mount *mntp, nfsfh_t *fh return (0); } +/* + * Do the vrele(sp->s_dvp) as a separate task in order to avoid a + * deadlock because of a LOR when vrele() locks the directory vnode. + */ +static void +nfs_freesillyrename(void *arg, __unused int pending) +{ + struct sillyrename *sp; + + sp = arg; + vrele(sp->s_dvp); + free(sp, M_NFSREQ); +} + int nfs_inactive(struct vop_inactive_args *ap) { @@ -207,8 +224,8 @@ nfs_inactive(struct vop_inactive_args *a */ (sp->s_removeit)(sp); crfree(sp->s_cred); - vrele(sp->s_dvp); - free((caddr_t)sp, M_NFSREQ); + TASK_INIT(&sp->s_task, 0, nfs_freesillyrename, sp); + taskqueue_enqueue(taskqueue_thread, &sp->s_task); mtx_lock(&np->n_mtx); } np->n_flag &= NMODIFIED; [-- Attachment #3 --] --- fs/nfsclient/nfsnode.h.sav2 2011-07-22 15:42:14.000000000 -0400 +++ fs/nfsclient/nfsnode.h 2011-07-22 15:43:25.000000000 -0400 @@ -35,11 +35,14 @@ #ifndef _NFSCLIENT_NFSNODE_H_ #define _NFSCLIENT_NFSNODE_H_ +#include <sys/_task.h> + /* * Silly rename structure that hangs off the nfsnode until the name * can be removed by nfs_inactive() */ struct sillyrename { + struct task s_task; struct ucred *s_cred; struct vnode *s_dvp; long s_namlen; --- fs/nfsclient/nfs_clnode.c.sav2 2011-07-22 15:43:40.000000000 -0400 +++ fs/nfsclient/nfs_clnode.c 2011-07-22 16:32:53.000000000 -0400 @@ -47,6 +47,7 @@ __FBSDID("$FreeBSD: head/sys/fs/nfsclien #include <sys/proc.h> #include <sys/socket.h> #include <sys/sysctl.h> +#include <sys/taskqueue.h> #include <sys/vnode.h> #include <vm/uma.h> @@ -65,6 +66,8 @@ MALLOC_DECLARE(M_NEWNFSREQ); uma_zone_t newnfsnode_zone; +static void nfs_freesillyrename(void *arg, __unused int pending); + void ncl_nhinit(void) { @@ -186,6 +189,20 @@ ncl_nget(struct mount *mntp, u_int8_t *f return (0); } +/* + * Do the vrele(sp->s_dvp) as a separate task in order to avoid a + * deadlock because of a LOR when vrele() locks the directory vnode. + */ +static void +nfs_freesillyrename(void *arg, __unused int pending) +{ + struct sillyrename *sp; + + sp = arg; + vrele(sp->s_dvp); + free(sp, M_NEWNFSREQ); +} + int ncl_inactive(struct vop_inactive_args *ap) { @@ -220,8 +237,8 @@ ncl_inactive(struct vop_inactive_args *a */ ncl_removeit(sp, vp); crfree(sp->s_cred); - vrele(sp->s_dvp); - FREE((caddr_t)sp, M_NEWNFSREQ); + TASK_INIT(&sp->s_task, 0, nfs_freesillyrename, sp); + taskqueue_enqueue(taskqueue_thread, &sp->s_task); mtx_lock(&np->n_mtx); } np->n_flag &= NMODIFIED;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1931983408.909766.1311369086133.JavaMail.root>