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 [email protected]
