Re: Bug about devfs?

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);