Re: PR/38141 CVS commit: src/sys

[Andrew Doran <ad%netbsd.org@localhost>]

The following reply was made to PR kern/38141; it has been noted by GNATS.

From: Andrew Doran <ad%netbsd.org@localhost>
To: gnats-bugs%netbsd.org@localhost, yamt%mwd.biglobe.ne.jp@localhost
Cc: 
Subject: Re: PR/38141 CVS commit: src/sys
Date: Sun, 4 May 2008 01:21:58 +0100

 On Sun, May 04, 2008 at 12:00:12AM +0000, Andrew Doran wrote:
 > The following reply was made to PR kern/38141; it has been noted by GNATS.
 > 
 > From: Andrew Doran <ad%netbsd.org@localhost>
 > To: YAMAMOTO Takashi <yamt%mwd.biglobe.ne.jp@localhost>
 > Cc: gnats-bugs%NetBSD.org@localhost
 > Subject: Re: PR/38141 CVS commit: src/sys
 > Date: Sun, 4 May 2008 00:57:01 +0100
 > 
 >  On Sat, May 03, 2008 at 11:05:16PM +0900, YAMAMOTO Takashi wrote:
 >  > >  Log Message:
 >  > >  PR kern/38141 lookup/vfs_busy acquire rwlock recursively
 >  > >  
 >  > >  Until the code paths are fixed properly, put in place an ugly 
 > workaround
 >  > >  to make it safe to recursively acquire a read lock on a mount.
 >  > 
 >  > unfortunately, recursive lock was not necessary to triger
 >  > a deadlock.  i saw the following while running "build.sh -j128".
 >  > (well, the actual deadlock i saw had 10 or more vnodes involved.
 >  > the following is a simplified version for explanation.)
 >  > 
 >  > LWP1 in vn_open()
 >  >         1. lock a directory vnode which is VV_ROOT.
 >  > LWP2 in lookup()
 >  >         1. vfs_busy(RW_READER)
 >  >         2. call VFS_ROOT, which tries to lock the VV_ROOT vnode which
 >  >            is already locked by LWP1. => block
 >  > LWP3 (syncer) in sync_fsync()
 >  >         1. vfs_trybusy(RW_WRITER) => block
 >  > now, LWP1 again:
 >  >         2. call VOP_CREATE -> getnewvnode -> vfs_busy(RW_READER) => block
 >  
 >  Heh, lockmgr() strikes from beyond the grave. I guess we could make it safe
 >  in the short term by making vfs_busy() an even dumber kind of rwlock based
 >  on atomics, with no preference for writers.
 >  
 >  sched_fsync() could be a problem becase of VOP_LOCK -> vfs_busy(RW_WRITER).
 >  I suppose there is no need for it to be represented as a vnode, and we could
 >  make the syncer look over the list of file systems for ones that need a
 >  periodic VFS_SYNC().
 >  
 >  What do you think?
 
 Actually, it's still not enough. Many of the existing users of vfs_busy()
 would also need to be converted to vfs_trybusy(), for example in sys_fchdir:
 
 chdir: vn_lock -> vfs_busy
 unmount vfs_busy (mnt_unmounter != NULL) -> vn_lock
 
 Andrew