birthtime initialization

[Bruce Evans]

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