From owner-freebsd-bugs@FreeBSD.ORG Mon Sep 4 10:40:32 2006 Return-Path: X-Original-To: freebsd-bugs@hub.freebsd.org Delivered-To: freebsd-bugs@hub.freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 41D7416A4DE for ; Mon, 4 Sep 2006 10:40:32 +0000 (UTC) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (freefall.freebsd.org [216.136.204.21]) by mx1.FreeBSD.org (Postfix) with ESMTP id D67F643D53 for ; Mon, 4 Sep 2006 10:40:31 +0000 (GMT) (envelope-from gnats@FreeBSD.org) Received: from freefall.freebsd.org (gnats@localhost [127.0.0.1]) by freefall.freebsd.org (8.13.4/8.13.4) with ESMTP id k84AeVci091026 for ; Mon, 4 Sep 2006 10:40:31 GMT (envelope-from gnats@freefall.freebsd.org) Received: (from gnats@localhost) by freefall.freebsd.org (8.13.4/8.13.4/Submit) id k84AeVMh091025; Mon, 4 Sep 2006 10:40:31 GMT (envelope-from gnats) Date: Mon, 4 Sep 2006 10:40:31 GMT Message-Id: <200609041040.k84AeVMh091025@freefall.freebsd.org> To: freebsd-bugs@FreeBSD.org From: "Alex Unleashed" Cc: Subject: Re: kern/102335: [devfs] sx vnode deadlock X-BeenThere: freebsd-bugs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list Reply-To: Alex Unleashed List-Id: Bug reports List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Sep 2006 10:40:32 -0000 The following reply was made to PR kern/102335; it has been noted by GNATS. From: "Alex Unleashed" To: bug-followup@freebsd.org Cc: Subject: Re: kern/102335: [devfs] sx vnode deadlock Date: Mon, 4 Sep 2006 12:32:17 +0200 ------=_Part_100777_6803680.1157365937363 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline Konstantin Belousov created the following patch against -CURRENT noting that devfs from 6.X should be replaced by -CURRENT soon enough. This patch trades the deadlock for a race at unmount time. Index: devfs.h =================================================================== RCS file: /usr/local/arch/ncvs/src/sys/fs/devfs/devfs.h,v retrieving revision 1.29 diff -u -r1.29 devfs.h --- devfs.h 12 Apr 2006 12:17:29 -0000 1.29 +++ devfs.h 1 Sep 2006 11:00:34 -0000 @@ -163,7 +163,7 @@ void devfs_rules_apply(struct devfs_mount *dm, struct devfs_dirent *de); void devfs_rules_cleanup (struct devfs_mount *dm); int devfs_rules_ioctl(struct devfs_mount *dm, u_long cmd, caddr_t data, struct thread *td); -int devfs_allocv (struct devfs_dirent *de, struct mount *mp, struct vnode **vpp, struct thread *td); +int devfs_allocv (struct devfs_dirent *de, struct mount *mp, struct vnode **vpp, int *dm_lock, struct thread *td); void devfs_delete(struct devfs_mount *dm, struct devfs_dirent *de); void devfs_populate (struct devfs_mount *dm); void devfs_cleanup (struct devfs_mount *dm); Index: devfs_vfsops.c =================================================================== RCS file: /usr/local/arch/ncvs/src/sys/fs/devfs/devfs_vfsops.c,v retrieving revision 1.50 diff -u -r1.50 devfs_vfsops.c --- devfs_vfsops.c 17 Jul 2006 09:07:01 -0000 1.50 +++ devfs_vfsops.c 1 Sep 2006 11:00:34 -0000 @@ -135,9 +135,11 @@ int error; struct vnode *vp; struct devfs_mount *dmp; + int dm_lock; dmp = VFSTODEVFS(mp); - error = devfs_allocv(dmp->dm_rootdir, mp, &vp, td); + dm_lock = 0; + error = devfs_allocv(dmp->dm_rootdir, mp, &vp, &dm_lock, td); if (error) return (error); vp->v_vflag |= VV_ROOT; Index: devfs_vnops.c =================================================================== RCS file: /usr/local/arch/ncvs/src/sys/fs/devfs/devfs_vnops.c,v retrieving revision 1.133 diff -u -r1.133 devfs_vnops.c --- devfs_vnops.c 17 Jul 2006 09:07:01 -0000 1.133 +++ devfs_vnops.c 1 Sep 2006 11:00:34 -0000 @@ -126,20 +126,26 @@ } int -devfs_allocv(struct devfs_dirent *de, struct mount *mp, struct vnode **vpp, struct thread *td) +devfs_allocv(struct devfs_dirent *de, struct mount *mp, struct vnode **vpp, + int *dm_unlock, struct thread *td) { int error; struct vnode *vp; struct cdev *dev; + struct devfs_mount *dmp; KASSERT(td == curthread, ("devfs_allocv: td != curthread")); + dmp = VFSTODEVFS(mp); loop: - mtx_lock(&devfs_de_interlock); vp = de->de_vnode; if (vp != NULL) { VI_LOCK(vp); mtx_unlock(&devfs_de_interlock); + if (*dm_unlock) { + sx_xunlock(&dmp->dm_lock); + *dm_unlock = 0; + } if (vget(vp, LK_EXCLUSIVE | LK_INTERLOCK, td)) goto loop; *vpp = vp; @@ -182,6 +188,10 @@ vp->v_data = de; de->de_vnode = vp; mtx_unlock(&devfs_de_interlock); + if (*dm_unlock) { + sx_xunlock(&dmp->dm_lock); + *dm_unlock = 0; + } vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, td); #ifdef MAC mac_associate_vnode_devfs(mp, de, vp); @@ -456,7 +466,7 @@ } static int -devfs_lookupx(struct vop_lookup_args *ap) +devfs_lookupx(struct vop_lookup_args *ap, int *dm_unlock) { struct componentname *cnp; struct vnode *dvp, **vpp; @@ -507,7 +517,7 @@ de = TAILQ_FIRST(&dd->de_dlist); /* "." */ de = TAILQ_NEXT(de, de_list); /* ".." */ de = de->de_dir; - error = devfs_allocv(de, dvp->v_mount, vpp, td); + error = devfs_allocv(de, dvp->v_mount, vpp, dm_unlock, td); vn_lock(dvp, LK_EXCLUSIVE | LK_RETRY, td); return (error); } @@ -564,7 +574,7 @@ return (0); } } - error = devfs_allocv(de, dvp->v_mount, vpp, td); + error = devfs_allocv(de, dvp->v_mount, vpp, dm_unlock, td); return (error); } @@ -573,11 +583,14 @@ { int j; struct devfs_mount *dmp; + int dm_unlock; dmp = VFSTODEVFS(ap->a_dvp->v_mount); + dm_unlock = 1; sx_xlock(&dmp->dm_lock); - j = devfs_lookupx(ap); - sx_xunlock(&dmp->dm_lock); + j = devfs_lookupx(ap, &dm_unlock); + if (dm_unlock == 1) + sx_xunlock(&dmp->dm_lock); return (j); } @@ -589,6 +602,7 @@ struct thread *td; struct devfs_dirent *dd, *de; struct devfs_mount *dmp; + int dm_unlock; int error; /* @@ -600,6 +614,7 @@ dvp = ap->a_dvp; dmp = VFSTODEVFS(dvp->v_mount); sx_xlock(&dmp->dm_lock); + dm_unlock = 1; cnp = ap->a_cnp; vpp = ap->a_vpp; @@ -620,9 +635,10 @@ if (de == NULL) goto notfound; de->de_flags &= ~DE_WHITEOUT; - error = devfs_allocv(de, dvp->v_mount, vpp, td); + error = devfs_allocv(de, dvp->v_mount, vpp, &dm_unlock, td); notfound: - sx_xunlock(&dmp->dm_lock); + if (dm_unlock == 1) + sx_xunlock(&dmp->dm_lock); return (error); } @@ -1102,6 +1118,7 @@ struct devfs_dirent *de; struct devfs_mount *dmp; struct thread *td; + int dm_unlock; td = ap->a_cnp->cn_thread; KASSERT(td == curthread, ("devfs_symlink: td != curthread")); @@ -1120,12 +1137,14 @@ de->de_symlink = malloc(i, M_DEVFS, M_WAITOK); bcopy(ap->a_target, de->de_symlink, i); sx_xlock(&dmp->dm_lock); + dm_unlock = 1; #ifdef MAC mac_create_devfs_symlink(ap->a_cnp->cn_cred, dmp->dm_mount, dd, de); #endif TAILQ_INSERT_TAIL(&dd->de_dlist, de, de_list); - devfs_allocv(de, ap->a_dvp->v_mount, ap->a_vpp, td); - sx_xunlock(&dmp->dm_lock); + devfs_allocv(de, ap->a_dvp->v_mount, ap->a_vpp, &dm_unlock, td); + if (dm_unlock == 1) + sx_xunlock(&dmp->dm_lock); return (0); } ------=_Part_100777_6803680.1157365937363 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Content-Disposition: inline Konstantin Belousov created the following patch against -CURRENT noting that devfs from 6.X should be replaced by -CURRENT soon enough. This patch trades the deadlock for a race at unmount time.

Index: devfs.h
===================================================================
RCS file: /usr/local/arch/ncvs/src/sys/fs/devfs/devfs.h,v
retrieving revision 1.29
diff -u -r1.29 devfs.h
--- devfs.h     12 Apr 2006 12:17:29 -0000      1.29
+++ devfs.h     1 Sep 2006 11:00:34 -0000
@@ -163,7 +163,7 @@
 void devfs_rules_apply(struct devfs_mount *dm, struct devfs_dirent *de);
 void devfs_rules_cleanup (struct devfs_mount *dm);
 int devfs_rules_ioctl(struct devfs_mount *dm, u_long cmd, caddr_t data, struct thread *td);
-int devfs_allocv (struct devfs_dirent *de, struct mount *mp, struct vnode **vpp, struct thread *td);
+int devfs_allocv (struct devfs_dirent *de, struct mount *mp, struct vnode **vpp, int *dm_lock, struct thread *td);
 void devfs_delete(struct devfs_mount *dm, struct devfs_dirent *de);
 void devfs_populate (struct devfs_mount *dm);
 void devfs_cleanup (struct devfs_mount *dm);
Index: devfs_vfsops.c
===================================================================
RCS file: /usr/local/arch/ncvs/src/sys/fs/devfs/devfs_vfsops.c,v
retrieving revision 1.50
diff -u -r1.50 devfs_vfsops.c
--- devfs_vfsops.c      17 Jul 2006 09:07:01 -0000      1.50
+++ devfs_vfsops.c      1 Sep 2006 11:00:34 -0000
@@ -135,9 +135,11 @@
       int error;
       struct vnode *vp;
       struct devfs_mount *dmp;
+       int dm_lock;

       dmp = VFSTODEVFS(mp);
-       error = devfs_allocv(dmp->dm_rootdir, mp, &vp, td);
+       dm_lock = 0;
+       error = devfs_allocv(dmp->dm_rootdir, mp, &vp, &dm_lock, td);
       if (error)
               return (error);
       vp->v_vflag |= VV_ROOT;
Index: devfs_vnops.c
===================================================================
RCS file: /usr/local/arch/ncvs/src/sys/fs/devfs/devfs_vnops.c,v
retrieving revision 1.133
diff -u -r1.133 devfs_vnops.c
--- devfs_vnops.c       17 Jul 2006 09:07:01 -0000       1.133
+++ devfs_vnops.c       1 Sep 2006 11:00:34 -0000
@@ -126,20 +126,26 @@
 }

 int
-devfs_allocv(struct devfs_dirent *de, struct mount *mp, struct vnode **vpp, struct thread *td)
+devfs_allocv(struct devfs_dirent *de, struct mount *mp, struct vnode **vpp,
+            int *dm_unlock, struct thread *td)
 {
       int error;
       struct vnode *vp;
       struct cdev *dev;
+       struct devfs_mount *dmp;

       KASSERT(td == curthread, ("devfs_allocv: td != curthread"));
+       dmp = VFSTODEVFS(mp);
 loop:
-
       mtx_lock(&devfs_de_interlock);
       vp = de->de_vnode;
       if (vp != NULL) {
               VI_LOCK(vp);
               mtx_unlock(&devfs_de_interlock);
+               if (*dm_unlock) {
+                       sx_xunlock(&dmp->dm_lock);
+                       *dm_unlock = 0;
+               }
               if (vget(vp, LK_EXCLUSIVE | LK_INTERLOCK, td))
                       goto loop;
               *vpp = vp;
@@ -182,6 +188,10 @@
       vp->v_data = de;
       de->de_vnode = vp;
       mtx_unlock(&devfs_de_interlock);
+       if (*dm_unlock) {
+               sx_xunlock(&dmp->dm_lock);
+               *dm_unlock = 0;
+       }
       vn_lock(vp, LK_EXCLUSIVE | LK_RETRY, td);
 #ifdef MAC
       mac_associate_vnode_devfs(mp, de, vp);
@@ -456,7 +466,7 @@
 }

 static int
-devfs_lookupx(struct vop_lookup_args *ap)
+devfs_lookupx(struct vop_lookup_args *ap, int *dm_unlock)
 {
       struct componentname *cnp;
       struct vnode *dvp, **vpp;
@@ -507,7 +517,7 @@
               de = TAILQ_FIRST(&dd->de_dlist);        /* "." */
               de = TAILQ_NEXT(de, de_list);           /* ".." */
               de = de->de_dir;
-               error = devfs_allocv(de, dvp->v_mount, vpp, td);
+               error = devfs_allocv(de, dvp->v_mount, vpp, dm_unlock, td);
               vn_lock(dvp, LK_EXCLUSIVE | LK_RETRY, td);
               return (error);
       }
@@ -564,7 +574,7 @@
                       return (0);
               }
       }
-       error = devfs_allocv(de, dvp->v_mount, vpp, td);
+       error = devfs_allocv(de, dvp->v_mount, vpp, dm_unlock, td);
       return (error);
 }

@@ -573,11 +583,14 @@
 {
       int j;
       struct devfs_mount *dmp;
+       int dm_unlock;

       dmp = VFSTODEVFS(ap->a_dvp->v_mount);
+       dm_unlock = 1;
       sx_xlock(&dmp->dm_lock);
-       j = devfs_lookupx(ap);
-       sx_xunlock(&dmp->dm_lock);
+       j = devfs_lookupx(ap, &dm_unlock);
+       if (dm_unlock == 1)
+               sx_xunlock(&dmp->dm_lock);
       return (j);
 }

@@ -589,6 +602,7 @@
       struct thread *td;
       struct devfs_dirent *dd, *de;
       struct devfs_mount *dmp;
+       int dm_unlock;
       int error;

       /*
@@ -600,6 +614,7 @@
       dvp = ap->a_dvp;
       dmp = VFSTODEVFS(dvp->v_mount);
       sx_xlock(&dmp->dm_lock);
+       dm_unlock = 1;

       cnp = ap->a_cnp;
       vpp = ap->a_vpp;
@@ -620,9 +635,10 @@
       if (de == NULL)
               goto notfound;
       de->de_flags &= ~DE_WHITEOUT;
-       error = devfs_allocv(de, dvp->v_mount, vpp, td);
+       error = devfs_allocv(de, dvp->v_mount, vpp, &dm_unlock, td);
 notfound:
-       sx_xunlock(&dmp->dm_lock);
+       if (dm_unlock == 1)
+               sx_xunlock(&dmp->dm_lock);
       return (error);
 }

@@ -1102,6 +1118,7 @@
       struct devfs_dirent *de;
       struct devfs_mount *dmp;
       struct thread *td;
+       int dm_unlock;

       td = ap->a_cnp->cn_thread;
       KASSERT(td == curthread, ("devfs_symlink: td != curthread"));
@@ -1120,12 +1137,14 @@
       de->de_symlink = malloc(i, M_DEVFS, M_WAITOK);
       bcopy(ap->a_target, de->de_symlink, i);
       sx_xlock(&dmp->dm_lock);
+       dm_unlock = 1;
 #ifdef MAC
       mac_create_devfs_symlink(ap->a_cnp->cn_cred, dmp->dm_mount, dd, de);
 #endif
       TAILQ_INSERT_TAIL(&dd->de_dlist, de, de_list);
-       devfs_allocv(de, ap->a_dvp->v_mount, ap->a_vpp, td);
-       sx_xunlock(&dmp->dm_lock);
+       devfs_allocv(de, ap->a_dvp->v_mount, ap->a_vpp, &dm_unlock, td);
+       if (dm_unlock == 1)
+               sx_xunlock(&dmp->dm_lock);
       return (0);
 }


------=_Part_100777_6803680.1157365937363--