Re: Vnode API cleanup pass 2a

[David Holland <dholland-tech%netbsd.org@localhost>]

On Mon, Dec 30, 2013 at 11:35:48AM +0100, J. Hannken-Illjes wrote:
 > The layered file systems hashlists currently have to work on locked
 > vnodes as the vnode operations returning vnodes return them locked.
 > 
 > This leads to some very dirty hacks -- see layer_node_find() from
 > sys/miscfs/genfs/layer_subr.c for example.
 > 
 > The best solution is to change these vnode operations to return
 > unlocked (and referenced) vnodes.
 > 
 > The attached diff implements this change for operations create, mknod,
 > mkdir and symlink.  It passes the atf test suite and some file system
 > stress tests I use here.
 > 
 > Comments or objections anyone?

My first thought is that I really don't think it's a good idea to
change important semantic properties (particularly locking) of calls
from what's effectively a public API without changing the signatures
too so old code won't compile.

Also, since what you're doing is basically unlocking before return in
every implementation, and then relocking after return at every call
site, it's possible to do one op at a time; it seems like that would
be a good idea in case unexpected problems crop up.

Unlocking and relocking should not itself be a problem since AFAIK
there isn't any reason for those locks to be held in the first place;
however, I think I'd better go review all the call sites just in case.

Plus I think it would be advisable to make this change first, so that
in case we do find a site where it matters we can lock the returned
object before anyone else can see it:

 #
-#% mkdir      dvp     L U U
+#% mkdir      dvp     L L L
 #% mkdir      vpp     - L - 
 #% mkdir      vpp     - L - 
 #

This is not, unfortunately, entirely trivial; I've been putting it off
to combine with namei/componentname-related changes.

-- 
David A. Holland
dholland%netbsd.org@localhost