Re: Vnode API cleanup pass 2a

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

On Mon, Jan 13, 2014 at 04:06:02PM +0100, J. Hannken-Illjes wrote:
 > [various nfs stuff]

Yeah, I'm not sure what to make of that either.
 
 > The "reference implementation" from Solaris is not atomic:
 > 
 >   VOP_MKDIR(dvp, &vp)
 >   VOP_GETATTR(vp)
 >   makefh(vp)
 >   VN_RELE(vp)
 >   VN_RELE(dvp)

They can't do this kind of thing atomically with their blinkered vfs
locking :-)

One should get the same file handle even if it isn't atomic and that's
probably ok; on the other hand it would be weird if you created a
directory mode 755 and the stat info you got back said 700 because a
chmod raced with the getattr. On the other hand, it's NFS.

 > >>>> sys/fs/union/union_subr.c:union_mkshadow()
 > > 
 > > ...but this doesn't, the caller is trying to create the directory and
 > > also bind it into the onionfs layering atomically. Of course, being
 > > onionfs, it's already only approximate and adding another race to it
 > > may not make things any worse; but it's probably better not to.
 > 
 > Looking closer it makes things better here.  Currently unionfs will
 > lock in the wrong order (uppervp -> upperdvp) while it now does it right.

Oh lovely, I missed that.

 > If the shadow directory we just created would disappear because another
 > thread removed it outside of unionfs we had much more problems than
 > a race here.

Yeah, I think the more likely problem is that it might try to create
it twice and have one of them fail unexpectedly. But maybe it's
already capable of dealing with that.

 > Even though it looks like we don't need creation ops to return with
 > locked directory at the moment it may be better to do this change now
 > so atomic operations like the one seen in NFS server are possible.

Yeah, that's what I was thinking

 > Thus we would end up with:
 > [snip]

Right.

It would probably be better to make this a separate commit; if that's
a nuisance for you, I can do it...

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