Re: Bug about devfs?

From: Kostik Belousov <kostikbel_at_gmail.com>
Date: Tue, 12 Jul 2011 17:57:53 +0300
On Tue, Jul 12, 2011 at 03:02:44PM +0200, Attilio Rao wrote:
> 2011/7/12 Kostik Belousov <kostikbel_at_gmail.com>:
> > On Tue, Jul 12, 2011 at 07:10:28PM +0900, Kohji Okuno wrote:
> >> Hello,
> >>
> >> I think that devfs has a problem.
> >> I encountered the problem that open("/dev/AAA") returned ENOENT.
> >> Of course, /dev/AAA exists.
> >>
> >> ENOENT was created by the point(***) in devfs_allocv().
> >> I think that the race condition had occurred between process A and
> >> vnlru kernel thread.
> >>
> >> Please check the following.
> >>
> >> If vnlru set VI_DOOMED to vp->v_iflag but vnlru didn't still execute
> >> VOP_RECLAIM(), process A cat get valid vp from de->de_vnode.
> >> But, vget() will return ENOENT, because vp->v_iflag has VI_DOOMED.
> >>
> >> When I set the break point to (***), I checked that de->de_vnode and
> >> vp->v_data were NULL.
> >>
> >>
> >> process A: š š š š š š š š š š š š š švnlru:
> >>
> >> devfs_allocv() {
> >> š š š š š š š š š š š š š š š š š š š vgonel(vp) {
> >> š š... š š š š š š š š š š š š š š š š š š š š š ...
> >> š š š š š š š š š š š š š š š š š š š š vp->v_iflag |= VI_DOOMED;
> >> š mtx_lock(&devfs_de_interlock); š š š š ...
> >> š vp = de->de_vnode;
> >> š if (vp != NULL) { š š š š š š š š š š VI_UNLOCK(vp);
> >> š š š š š š š š š š š š š _____________/ ...
> >> š VI_LOCK(vp); ____________/ š š š š š š šif (VOP_RECLAIM(vp, td))
> >> š mtx_unlock(&devfs_de_interlock); š š š ...
> >> š š... š š š š š š š š š š š š š š š š\ š š š š devfs_reclaim(ap) {
> >> š error = vget(vp,...); š š š š š š š š\
> >> š š... š š š š š š š š š š š š š š š š š\______ š mtx_lock(&devfs_de_interlock);
> >> š if (devfs_allocv_drop_refs(...)) { š š š šde = vp->v_data;
> >> š š ... š š š š š š š š š š š š š š š š š š š š š if (de != NULL) {
> >> š } š š š š š š š š š š š š š š š š š š š š de->de_vnode = NULL;
> >> š else if (error) { š š š š š š š š š š š š vp->v_data = NULL;
> >> š š ... š š š š š š š š š š š š š š š š š š š š š }
> >> š š rturn (error); (***) š š š š š š š š šmtx_unlock(&devfs_de_interlock);
> >> š } š š š š š š š š š š š š š š š š š š š š ...
> >> š š š š š š š š š š š š š š š š š š š š }
> >>
> >>
> >>
> >> I think that devfs_allocv() should be fixed as below.
> >> How do you think?
> >>
> >> devfs_allocv(struct devfs_dirent *de, struct mount *mp, struct vnode **vpp)
> >> {
> >> š š š š int error;
> >> š š š struct vnode *vp;
> >> š š š struct cdev *dev;
> >> š š š struct devfs_mount *dmp;
> >>
> >> š š š dmp = VFSTODEVFS(mp);
> >> +#if 1
> >> + retry:
> >> +#endif
> >> š š š š if (de->de_flags & DE_DOOMED) {
> >>
> >> š š š š š š...
> >>
> >> š š š š mtx_lock(&devfs_de_interlock);
> >> š š š š vp = de->de_vnode;
> >> š š š š if (vp != NULL) {
> >> š š š š š š š š VI_LOCK(vp);
> >> š š š š š š š š mtx_unlock(&devfs_de_interlock);
> >> š š š š š š š š sx_xunlock(&dmp->dm_lock);
> >> š š š š š š š š error = vget(vp, LK_EXCLUSIVE | LK_INTERLOCK, curthread);
> >> š š š š š š š š sx_xlock(&dmp->dm_lock);
> >> š š š š š š š š if (devfs_allocv_drop_refs(0, dmp, de)) {
> >> š š š š š š š š š š š š if (error == 0)
> >> š š š š š š š š š š š š š š š š vput(vp);
> >> š š š š š š š š š š š š return (ENOENT);
> >> š š š š š š š š }
> >> š š š š š š š š else if (error) {
> >> +#if 1
> >> + š š š š š š š š š š if (error == ENOENT)
> >> + š š š š š š š š š š š š š š goto retry;
> >> +#endif
> >> š š š š š š š š š š š sx_xunlock(&dmp->dm_lock);
> >> š š š š š š š š š š š return (error);
> >> š š š š š š š }
> >>
> > Thank you for the report.
> >
> > The proposed change would revert r179247, which also caused some issues.
> > Are you able to reproduce the problem ?
> >
> > Could you try the following patch ? I cannot reproduce your situation,
> > so the patch is untested by me.
> >
> > diff --git a/sys/fs/devfs/devfs_vnops.c b/sys/fs/devfs/devfs_vnops.c
> > index bf6dab8..bbbfff4 100644
> > --- a/sys/fs/devfs/devfs_vnops.c
> > +++ b/sys/fs/devfs/devfs_vnops.c
> > _at__at_ -397,6 +397,7 _at__at_ devfs_allocv(struct devfs_dirent *de, struct mount *mp, int lockmode,
> > š š š š š š š šsx_xunlock(&dmp->dm_lock);
> > š š š š š š š šreturn (ENOENT);
> > š š š š}
> > +loop:
> > š š š šDEVFS_DE_HOLD(de);
> > š š š šDEVFS_DMP_HOLD(dmp);
> > š š š šmtx_lock(&devfs_de_interlock);
> > _at__at_ -412,7 +413,16 _at__at_ devfs_allocv(struct devfs_dirent *de, struct mount *mp, int lockmode,
> > š š š š š š š š š š š š š š š švput(vp);
> > š š š š š š š š š š š šreturn (ENOENT);
> > š š š š š š š š}
> > - š š š š š š š else if (error) {
> > + š š š š š š š else if (error != 0) {
> > + š š š š š š š š š š š if (error == ENOENT) {
> > + š š š š š š š š š š š š š š š mtx_lock(&devfs_de_interlock);
> > + š š š š š š š š š š š š š š š while (de->de_vnode != NULL) {
> > + š š š š š š š š š š š š š š š š š š š msleep(&de->de_vnode,
> > + š š š š š š š š š š š š š š š š š š š š š &devfs_de_interlock, 0, "dvall", 0);
> > + š š š š š š š š š š š š š š š }
> > + š š š š š š š š š š š š š š š mtx_unlock(&devfs_de_interlock);
> > + š š š š š š š š š š š š š š š goto loop;
> > + š š š š š š š š š š š }
> > š š š š š š š š š š š šsx_xunlock(&dmp->dm_lock);
> > š š š š š š š š š š š šreturn (error);
> > š š š š š š š š}
> > _at__at_ -1259,6 +1269,7 _at__at_ devfs_reclaim(struct vop_reclaim_args *ap)
> > š š š šif (de != NULL) {
> > š š š š š š š šde->de_vnode = NULL;
> > š š š š š š š švp->v_data = NULL;
> > + š š š š š š š wakeup(&de->de_vnode);
> > š š š š}
> > š š š šmtx_unlock(&devfs_de_interlock);
> 
> I think that this approach may starve the thread for a while.
> As I told you privately I was able to see on field a livelock because
> of this check. In my case, it was a thread running for 63seconds (at
> least, at that point the watchdog was tripping over).
Feasible explanation was not found at the time, AFAIR. I could believe
that this is possible with r179247 and driver stuck in the close cdevsw
method.

More risky change would be to clear de_vnode early. All devfs code shall
be already safe by checking for VI_DOOMED, and if VI_DOOMED, v_data may
be NULL.

Again, I am unable to test.

diff --git a/sys/fs/devfs/devfs_vnops.c b/sys/fs/devfs/devfs_vnops.c
index bf6dab8..955bd8b 100644
--- a/sys/fs/devfs/devfs_vnops.c
+++ b/sys/fs/devfs/devfs_vnops.c
_at__at_ -397,6 +397,7 _at__at_ devfs_allocv(struct devfs_dirent *de, struct mount *mp, int lockmode,
 		sx_xunlock(&dmp->dm_lock);
 		return (ENOENT);
 	}
+loop:
 	DEVFS_DE_HOLD(de);
 	DEVFS_DMP_HOLD(dmp);
 	mtx_lock(&devfs_de_interlock);
_at__at_ -405,16 +406,21 _at__at_ devfs_allocv(struct devfs_dirent *de, struct mount *mp, int lockmode,
 		VI_LOCK(vp);
 		mtx_unlock(&devfs_de_interlock);
 		sx_xunlock(&dmp->dm_lock);
-		error = vget(vp, lockmode | LK_INTERLOCK, curthread);
+		vget(vp, lockmode | LK_INTERLOCK | LK_RETRY, curthread);
 		sx_xlock(&dmp->dm_lock);
 		if (devfs_allocv_drop_refs(0, dmp, de)) {
-			if (error == 0)
-				vput(vp);
+			vput(vp);
 			return (ENOENT);
 		}
-		else if (error) {
-			sx_xunlock(&dmp->dm_lock);
-			return (error);
+		else if ((vp->v_iflag & VI_DOOMED) != 0) {
+			mtx_lock(&devfs_de_interlock);
+			if (de->de_vnode == vp) {
+				de->de_vnode = NULL;
+				vp->v_data = NULL;
+			}
+			mtx_unlock(&devfs_de_interlock);
+			vput(vp);
+			goto loop;
 		}
 		sx_xunlock(&dmp->dm_lock);
 		*vpp = vp;

Received on Tue Jul 12 2011 - 12:58:02 UTC

This archive was generated by hypermail 2.4.0 : Wed May 19 2021 - 11:40:15 UTC