Date: Fri, 25 Jul 2008 20:00:01 +1000 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Jaakko Heinonen <jh@saunalahti.fi> Cc: freebsd-fs@freebsd.org Subject: Re: birthtime initialization Message-ID: <20080725192315.D27178@delplex.bde.org> In-Reply-To: <20080725072314.GA807@a91-153-120-204.elisa-laajakaista.fi> References: <200806020800.m528038T072838@freefall.freebsd.org> <20080722075718.GA1881@a91-153-120-204.elisa-laajakaista.fi> <20080722215249.K17453@delplex.bde.org> <20080723103424.GA1856@a91-153-120-204.elisa-laajakaista.fi> <20080724000618.Q16961@besplex.bde.org> <20080725072314.GA807@a91-153-120-204.elisa-laajakaista.fi>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080725192315.D27178>