birthtime initialization
Bruce Evans
brde at optusnet.com.au
Fri Jul 25 10:00:19 UTC 2008
On Fri, 25 Jul 2008, Jaakko Heinonen wrote:
> On 2008-07-24, Bruce Evans wrote:
>> First, the fields shouldn't be initialized using VATTR_NULL() in
>> VOP_GETATTR().
>
>> Second, VNOVAL is an extremly bogus default value.
>
> Except for va_fsid because there's this check in vn_stat():
>
> if (vap->va_fsid != VNOVAL)
> sb->st_dev = vap->va_fsid;
> else
> sb->st_dev = vp->v_mount->mnt_stat.f_fsid.val[0];
Hmm, this is remarkably broken too. In VOP_GETATTR() for file systems
under sys/fs:
- the following file systems set va_fsid to dev2udev(<the mount point>)
(and thus defeat the better default above):
cd9660, hpfs, msdosfs, ntfs, udf, unionfs
- the following file systems don't seem to set va_fsid (and thus set
st_dev to stack garbage)
- the following file systems set va_fsid to VNOVAL via VATTR_NULL():
fdescfs
- the following file systems set va_fsid to VNOVAL via vattr_null():
devfs, portalfs
- the following file systems set va_fsid to VNOVAL via obscure means:
coda (?)
- the following file systems set va_fsid to mnt_stat.f_fsid.val[0]
directly:
nullfs, nwfs (?), pseudofs, smbfs (?), tmpfs
> What do you think that is a proper default value for va_rdev? Some file
> systems set it to 0 and some to VNOVAL.
Either NODEV or VNOVAL explicitly translated late to NODEV. NODEV is
(dev_t)(-1) (bug: this has parentheses in all the wrong places -- it
should be ((dev_t)-1), so VNOVAL = -1 assigned to va_rdev of type dev_t
equals NODEV and the identity translation works.
>> After deleting the bogus initializations, we're left with va_filerev,
>> va_birthtime and va_flags. Most file systems don't support these, so
>> they could usefully all be handled by defaulting them as in the proposed
>> changes for va_birthtime.
>
> Unfortunately moving initializations to vn_stat() breaks things. For
> example vm_mmap_vnode() uses VOP_GETATTR() to determine which file flags
> are set. Thus moving va_flags initialization to vn_stat() breaks
> mmap.
Oops.
> In theory this could be a potential problem for birthtime too.
It's a bit dangerous, but most callers to VOP_GETATTR() except vn_stat()
only want a couple of fields, and hopefully none want new fields.
Maybe the public interface should be vop_getattr() which sets defaults and
calls VOP_GETATTR(). Does this work with layering? There is negative
point to inlining most VOPs, and for VOP_GETATTR(), no one cares about
the much higher overhead of setting all fields in it when only a couple
are wanted.
Bruce
More information about the freebsd-fs
mailing list