From owner-freebsd-fs@FreeBSD.ORG Fri Jul 25 10:00:19 2008 Return-Path: Delivered-To: freebsd-fs@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 696CF10656BE for ; Fri, 25 Jul 2008 10:00:19 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail04.syd.optusnet.com.au (mail04.syd.optusnet.com.au [211.29.132.185]) by mx1.freebsd.org (Postfix) with ESMTP id 9C16A8FC2A for ; Fri, 25 Jul 2008 10:00:11 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from c220-239-252-11.carlnfd3.nsw.optusnet.com.au (c220-239-252-11.carlnfd3.nsw.optusnet.com.au [220.239.252.11]) by mail04.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id m6PA02Pa019933 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 25 Jul 2008 20:00:08 +1000 Date: Fri, 25 Jul 2008 20:00:01 +1000 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: Jaakko Heinonen In-Reply-To: <20080725072314.GA807@a91-153-120-204.elisa-laajakaista.fi> Message-ID: <20080725192315.D27178@delplex.bde.org> 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> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-fs@freebsd.org Subject: Re: birthtime initialization X-BeenThere: freebsd-fs@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Filesystems List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 25 Jul 2008 10:00:19 -0000 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() (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