Date: Fri, 22 May 2015 13:09:58 +0100 From: Patrick Welche <pr...@cam.ac.uk>
There are a load of locking PRs involving vnd. I thought I would have a stab at converting vnd to use condvars and mutexes (mutices?) as a first step. I cobbled something together which allows me to [...] with a LOCKDEBUG kernel, so I may have guessed right, but I haven't found any documentation on The Right Way, so please review carefully, assume the worst, and tell me how to do it better! First thought: I'd suggest making a branch for this so you can work on it incrementally without breaking the world, or at least a local Git branch so you can work on separate commits for different issues. Concur with all hannken@'s comments. Small bug: you almost certainly need sc_sclock to be at IPL_BIO, not IPL_NONE, if you want it to replace splbio/splx pairs. Minor nit: diff -pu, please. You converted the VNF_LOCKED/VNF_WANTED flags into a mutex sc_sclock (why not just `sc_lock'?), but the semantics doesn't quite carry over: previously, vndlock would wait interruptibly, and the owner of VNF_LOCKED was allowed to do I/O. But you can't do that with a mutex -- you can't wait interruptibly or do I/O while holding one. Right now you just drop sc_sclock around vndgetdisklabel, and don't take it for DIOCWDINFO &c., but then concurrent callers can stomp all over one another. You will probably have to introduce a new state, e.g. VNF_IO (or maybe just VNF_DISKLABELIO), and make anyone trying to use the disklabel or any other vnd-internal I/O wait for it to complete. - there was some sort of start kernel thread, thread sets variable, meanwhile loop waiting for variable to be set. I removed that. On the other hand I added a MUSTJOIN for the kernel thread exiting side of things. I suggest moving the kthread_join around a little bit. Don't read sc_flags without the lock; instead, grab the thread while under the lock, and kthread_join it outside. I suspect you will need some way for vndset to wait until any pending vndclear is actually complete. I don't see that immediately. Note that you won't hit any of this in testing, no matter how hard you hammer on it, until you add D_MPSAFE to vnd_bdevsw/vnd_cdevsw. - given that vnd is a pseudo-device, do I need an extra lock to care of device creation/destruction? (What am I protecting against? There is an MP safety issue with device_lookup_private: if you detach a driver just after someone does device_lookup_private, nothing causes the detach to block until the caller of device_lookup_private is done. But this is a general issue in autoconf, not a vnd-specific issue. I don't, in my cursory glance, see any obvious MP safety issues in the attachment goo in your patch.