Re: vnd locking

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

   Date: Fri, 22 May 2015 13:09:58 +0100
   From: Patrick Welche <prlw1%cam.ac.uk@localhost>

   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.