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.
--- 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;
--- 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;
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/freebsd-net
To unsubscribe, send any mail to "[email protected]"
