Hi Alexander,
Your comments are certainly welcome but some/many of them I happen to be
aware of. Therefore, your corrections should be directed to the
author of the paper. I cc'd him now, in case he finds time update the
document. I am not the author of it, nor was I a major contributor to it,
I only contributed a few bits and pieces - I just try to encourage
knowledgeable people like yourself to help Neil keeping it uptodate.
Thank you,
------
Tigran A. Aivazian | http://www.sco.com
Escalations Research Group | tel: +44-(0)1923-813796
Santa Cruz Operation Ltd | http://www.ocston.org/~tigran
On Wed, 8 Mar 2000, Alexander Viro wrote:
> 1) Vnodes as a thing from "commercial Unices"? Funny, that. SVR[45]-based?
> Even funnier. They originated in SunOS (== BSD derivative) way before SVR4
> happened. And I wouldn't call 4.4BSD and its kin commercial (except BSDI)
> or based on Missed'em'V codebase.
> 2) Files != file descriptors. Both concepts exist in every UNIX kernel since
> early PDP-7 days. Descriptors belong to process and refer to channels (==files).
> However, they _must_ be different from files, simply because you need an object
> created upon each open() and shared across fork(). Otherwise you either have
> lseek() in one process affecting everybody else (if we would keep the position
> with inode) or you can't redirect output from scripts (if we would keep it with
> process). Think a bit and you'll see why - it became obvious very soon after
> adding fork() to the PDP-7 kernel. Again, it's not Linux-specific - it's basic
> (and user-visible) UNIX stuff.
>
> 3) files _always_ have inodes. They may be not on-disk ones, but for VFS
> they are real, honest inodes. No matter what had created a file - pipe(2) and
> socket(2) are creating inodes just fine.
>
> 4) dcache is a forest, not a tree. Linkage from mount(2) is there, but it's
> independent from the rest of structure. 99% of operations act on a single
> tree and crossing the boundary is done in very few places.
>
> 5) read_super() is a kinda messy. Code in super.c looks like a half-arsed
> attempt to prepare for superblocks being kept across umount()/mount().
> It's obviously Wrong Thing(tm), since it's completely incompatible with
> modules. Actually all work with reference-counting must be done in VFS.
> To be fixed before 2.4...
>
> 6) unions in superblocks (and inodes) are nasty. Use separately-allocated
> private parts of either and keep pointer to them in ->u.generic_sbp and
> ->u.generic_ip, resp. It's less critical for superblocks, but for inodes
> it is pretty serious.
>
> 7) "The one interesting usage of the field [->s_type] is in
> fs/nfsd/vfs.c:nfsd_lookup() where it is used to make sure that a
> proc or nfs type file-system is never accessed via NFS."
> That's a bug. Thanks for pointing out. knfsd is not a place for such tests.
>
> 8) Ob ->s_vfs_rename_sem: the only place where it is of interest is in
> fs/namei.c::vfs_rename_dir(). And I mean it - anybody else trying to use it/
> look at it/whatever is asking for trouble. Maybe
> ->s_touch_it_and_you_will_get_another_arsehole_sem would be a better name...
>
> 9) ->read_inode() and iget(): the former should not be there. The latter is
> a convenience, but it's only for filesystem itself. And it's not mandatory -
> check ncpfs in 2.3.0 for example of how it should not be used. See also the
> entry for "blivet" in Jargon File... As for nfsfh.c - right, we need an
> analog of VOP_FHTOVP(), but it should return dentry instead of inode. Work
> in progress...
>
> 10) ->put_inode() and special-casing for ->i_count==1. Don't. There is
> ->clear_inode(), use it.
>
> 11) ->notify_change() - now ->setattr() in inode_operations.
>
> 12) ->put_super() - see comment on module reference counters. Code should
> never tell "you can drop me now" - that's a work for caller. Again, currently
> it's done that way, but it will be fixed.
>
> 13) ->statfs() - it assumes that result is in user memory. Bad idea, since
> we have to play with setfs() in all callers that want the data in kernel
> and we have to use copy_to_user() in a lot of places. Potential interface
> change: always pass it the kernel buffer and copy data to user in sys_statfs().
>
> 14) ->f_list - one more use is in tty layer (per-tty list).
>
> 15) ->f_dentry - _always_ non-NULL for opened files. Never change it.
> NB: if you are scanning one of the file lists you may find files with NULL
> ->f_dentry - they are in process of getting the fsck out of there. Just
> skip and pretend that you hadn't seen them.
>
> 16) ->f_count - never touch it by hands, use standard functions for that.
>
> 17) readahead is a mess. It's in a bad need of cleanup. Unfortunately, there
> are problems blocking such work.
>
> 18) ->revalidate() and ->check_media_change() in file_operations - RIP. They
> had no business being there.
>
>
