On Tue, Jan 07, 2014 at 11:30:40AM +0100, J. Hannken-Illjes wrote: > > (also, while this is minor I think I'd rather have vop_mkdir_args_v2 > > and/or vop_mkdir_desc_v2 rather than vop_mkdir2_args and > > vop_mkdir2_desc. The version doesn't need to be part of the operation > > name.) > > Looks very good -- changed vnode_if.sh to create xxx_v2_args. Diff attached.
Would you mind committing the versioning logic right away? It has clear value and it would be nice for it to appear in a separate changeset. > >>> 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. > >> > >> This is not what I'm doing. > > > > It still seems like a good idea to do one op at a time, or at least do > > one op first. > > This is missing any arguments for the second time ...because it's vfs and everything in vfs is delicate? Small incremental changes are safer; if something weird goes wrong, it's easier to deal with the smaller the change was. (If nothing else, it's easier to rever only the part that broke; it's also easier to bisect later.) > and you ignored my > arguments after "This is not what I'm doing.". That's because they aren't really relevant to working incrementally. > >>> 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 > >> > >> Where should it matter? VOP_MKDIR is only used from: In the case of VOP_SYMLINK it definitely matters, as the new symlink isn't filled until after VOP_SYMLINK returns. Since the locking protocol for the containing directory is to unlock it *before* returning, it then becomes possible to see the empty symlink. And that isn't so good. mkdir is less clear, but: > >> external/cddl/osnet/dist/uts/common/fs/zfs/zfs_replay.c:zfs_replay_create() This doesn't appear to do anything with the new dir (right now it just panics...) > >> sys/kern/vfs_syscalls.c:do_sys_mkdirat() This does nothing and is ok. > >> sys/nfs/nfs_serv.c:nfsrv_mkdir() This extracts the nfs file handle for the new dir and calls VOP_GETATTR. If we don't leave the directory locked, these actions are no longer atomic with the directory creation. I don't know whether this is safe or not. My guess is not. In the absence of clear information it seems prudent to not introduce this race. > >> sys/fs/union/union_vnops.c:union_mkdir() This looks ok. > >> 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. > >> sys/rump/librump/rumpvfs/rumpvnode_if.c:RUMP_VOP_MKDIR() this shouldn't matter. > > I have been putting off answering this because I ought to examine all > > these, and I haven't managed to do so yet but figured I ought to > > answer the rest. It looks to me like it would definitely be better to first fix the VOP_MKDIR interface to not release the directory lock within the call. Certainly for VOP_SYMLINK. Therefore, it's probably best to fix all the creation ops. -- David A. Holland dholl...@netbsd.org