From owner-freebsd-fs@FreeBSD.ORG Wed Jul 23 15:42:54 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 B8E8E1065678 for ; Wed, 23 Jul 2008 15:42:54 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail01.syd.optusnet.com.au (mail01.syd.optusnet.com.au [211.29.132.182]) by mx1.freebsd.org (Postfix) with ESMTP id 50F208FC0C for ; Wed, 23 Jul 2008 15:42:53 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from besplex.bde.org (c220-239-252-11.carlnfd3.nsw.optusnet.com.au [220.239.252.11]) by mail01.syd.optusnet.com.au (8.13.1/8.13.1) with ESMTP id m6NFgpMJ023245 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Thu, 24 Jul 2008 01:42:51 +1000 Date: Thu, 24 Jul 2008 01:42:51 +1000 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Jaakko Heinonen In-Reply-To: <20080723103424.GA1856@a91-153-120-204.elisa-laajakaista.fi> Message-ID: <20080724000618.Q16961@besplex.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> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: freebsd-fs@freebsd.org, ighighi@gmail.com 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: Wed, 23 Jul 2008 15:42:54 -0000 On Wed, 23 Jul 2008, Jaakko Heinonen wrote: > On 2008-07-22, Bruce Evans wrote: >>> + VATTR_NULL(vap); >> >> I want to initialize va_birthtime to { -1, 0 } here only. Don't >> initialize the whole vattr here. VOP_GETTATR() is supposed to initalize >> everything, but doesn't for va_birthtime. If there any other fields >> that VOP_GETTATR() doesn't initialize, then these should be searched >> for and fixed instead of setting them to the garbage value given by >> vattr_null. > > At least xfs gets it wrong for several fields. > > /* > * Fields with no direct equivalent in XFS > * leave initialized by VATTR_NULL > */ > #if 0 > vap->va_filerev = 0; > vap->va_birthtime = va.va_ctime; > vap->va_vaflags = 0; > vap->va_flags = 0; > vap->va_spare = 0; > #endif That's amazingly bad. First, the fields shouldn't be initialized using VATTR_NULL() in VOP_GETATTR(). Doing so breaks the preinitialization that we want to add (maybe also layering). This bug seems to be present in only the following file systems: fdescfs, mqfs, pseudofs, tmpfs, xfs The uninitialized fields should give stack garbage. Second, VNOVAL is an extremly bogus default value. For va_flags, it gives all flags set, so ls -lo output would be weird (and wrong since the flags aren't actually there). Third, va_vaflags and va_spare aren't part of the VOP_GETATTR() API. va_vaflags is an input parameter for VOP_SETATTR(). va_spare is just spare (unused). VATTR_NULL() initializes va_vaflags to 0, not VNOVAL (as is required for the usual case in VOP_SETTATR()), and it knows better than to initialize unused fields (it also doesn't initialize unnamed padding -- stack garbage in this is OK since vattr is never copied directly to userland). 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. >>> Index: sys/ufs/ufs/ufs_vnops.c >>> ... >>> Index: sys/fs/msdosfs/msdosfs_vnops.c >>> ... >>> Index: sys/nfsclient/nfs_subs.c >> >> There are a probably more file systems that have missing or slightly >> incorrect (all zero) settings of va_birthtime. > > Many file systems misses settings of va_birthtime. That's the reason why > I initialized it in vn_stat(). I have seen four types of > initializations: > > 1) Support and set birthtime. (UFS2, tmpfs, msdosfs (not all > variants of msdosfs support birthtime), nfs4?) > > 2) Set birthtime to zero. (UFS1, nfs (nfs zeroes the vattr structure)) Zeroing it is almost as bad as VATTR_NULL()ing it. > 3) Initialize vattr with VATTR_NULL() but not birthtime explicitly. Thus > tv_sec and tv_nsec are set to -1 (VNOVAL). (devfs, xfs, portalfs, > pseudofs) > > 4) Not initialize birthtime at all. Those would be fixed by initializing the > birthtime in vn_stat(). (cd9660, hpfs, ntfs, smbfs, udf, ext2fs, > reiserfs) > I couldn't test but I suspect that also coda belongs to this group. > > So I see two ways to fix: > > - initialize birthtime in vn_stat() and add/fix explicit setting for group 2 > and 3 file systems or > - add explicit initialization to all file systems missing it > (groups 3 and 4) and fix group 2 to initialize birthtime to correct value (3) and (4) are only different due to bugs. I want to initialize va_birthtime and maybe a couple of other fields in vn_stat(), and depend on this and not initialize to the same or a worse value in case (3). This requires removing VATTR_NULL() or zeroing in some cases and checking that everything is still initialized. All old fields should be handled by explicit initialization as in ffs1, and all new fields should have defaults. > I have updated the patch per your comments and checked more file > systems. I have verified that with this patch these file systems return > correct birthtime values (real birthtime or {-1, 0} if not supported): > > UFS2, UFS1, cd9660, nfs, ext2fs, smbfs, reiserfs, xfs, ntfs, devfs, > procfs, linprocfs, tmpfs, msdosfs, portalfs, udf. I don't want the case (3). Otherwise good. > > For pseudofs I set birthtime to current time. I don't like this. birthtime should be <= all other file times. If a file system doesn't support birthtime, then it could also set birthtime = mtime, but that isn't useful either. Better set it to -1 as in ffs1 (exept ffs1 set it to 0). > > %%% > Index: sys/kern/vfs_vnops.c > =================================================================== > --- sys/kern/vfs_vnops.c (revision 180729) > +++ sys/kern/vfs_vnops.c (working copy) > @@ -703,6 +703,13 @@ vn_stat(vp, sb, active_cred, file_cred, > #endif > > vap = &vattr; > + > + /* > + * Not all file systems initialize birthtime. > + */ Change to something like: /* * Initialize defaults for new and/or unusual fields, so that file * systems which don't support these fields don't need to know * about them. */ > + vap->va_birthtime.tv_sec = -1; > + vap->va_birthtime.tv_nsec = 0; > + > error = VOP_GETATTR(vp, vap, active_cred, td); > if (error) > return (error); > Index: sys/ufs/ufs/ufs_vnops.c > =================================================================== > --- sys/ufs/ufs/ufs_vnops.c (revision 180729) > +++ sys/ufs/ufs/ufs_vnops.c (working copy) > @@ -410,7 +410,7 @@ ufs_getattr(ap) > vap->va_mtime.tv_nsec = ip->i_din1->di_mtimensec; > vap->va_ctime.tv_sec = ip->i_din1->di_ctime; > vap->va_ctime.tv_nsec = ip->i_din1->di_ctimensec; > - vap->va_birthtime.tv_sec = 0; > + vap->va_birthtime.tv_sec = -1; > vap->va_birthtime.tv_nsec = 0; > vap->va_bytes = dbtob((u_quad_t)ip->i_din1->di_blocks); > } else { Can just delete birthtime references here. Unless I've missed a bzero. > Index: sys/nfsclient/nfs_subs.c > =================================================================== > --- sys/nfsclient/nfs_subs.c (revision 180729) > +++ sys/nfsclient/nfs_subs.c (working copy) > @@ -628,6 +628,8 @@ nfs_loadattrcache(struct vnode **vpp, st > vap->va_rdev = rdev; > mtime_save = vap->va_mtime; > vap->va_mtime = mtime; > + vap->va_birthtime.tv_sec = -1; > + vap->va_birthtime.tv_nsec = 0; > vap->va_fsid = vp->v_mount->mnt_stat.f_fsid.val[0]; > if (v3) { > vap->va_nlink = fxdr_unsigned(u_short, fp->fa_nlink); Need to remove the zeroing and check other fields before defaulting birthtime here. > Index: sys/fs/pseudofs/pseudofs_vnops.c > =================================================================== > --- sys/fs/pseudofs/pseudofs_vnops.c (revision 180729) > +++ sys/fs/pseudofs/pseudofs_vnops.c (working copy) > @@ -200,7 +200,7 @@ pfs_getattr(struct vop_getattr_args *va) > vap->va_fsid = vn->v_mount->mnt_stat.f_fsid.val[0]; > vap->va_nlink = 1; > nanotime(&vap->va_ctime); > - vap->va_atime = vap->va_mtime = vap->va_ctime; > + vap->va_atime = vap->va_mtime = vap->va_birthtime = vap->va_ctime; > > switch (pn->pn_type) { > case pfstype_procdir: I don't understand why it doesn't have _any_ persistent storage for times. > Index: sys/fs/portalfs/portal_vnops.c > =================================================================== > --- sys/fs/portalfs/portal_vnops.c (revision 180729) > +++ sys/fs/portalfs/portal_vnops.c (working copy) > @@ -462,6 +462,8 @@ portal_getattr(ap) > nanotime(&vap->va_atime); > vap->va_mtime = vap->va_atime; > vap->va_ctime = vap->va_mtime; > + vap->va_birthtime.tv_sec = -1; > + vap->va_birthtime.tv_nsec = 0; > vap->va_gen = 0; > vap->va_flags = 0; > vap->va_rdev = 0; This uses both bzero() and vattr_null(). Oops, I only grepped for use of VATTR_NULL() when I looked for bogus initializations above. VATTR_NULL() is the public interface and vattr_null() is an implementation detail. Add the following file systems to the list of file systems with bogusly initialized vattr's in VOP_GETATTR(): devfs, portalfs These both misuse bzero() and vattr_null(). There are no other misuses of vattr_null(). > Index: sys/fs/devfs/devfs_vnops.c > =================================================================== > --- sys/fs/devfs/devfs_vnops.c (revision 180729) > +++ sys/fs/devfs/devfs_vnops.c (working copy) > @@ -543,6 +543,8 @@ devfs_getattr(struct vop_getattr_args *a > > vap->va_rdev = cdev2priv(dev)->cdp_inode; > } > + vap->va_birthtime.tv_sec = -1; > + vap->va_birthtime.tv_nsec = 0; > vap->va_gen = 0; > vap->va_flags = 0; > vap->va_nlink = de->de_links; See above. > Index: sys/gnu/fs/xfs/FreeBSD/xfs_vnops.c > =================================================================== > --- sys/gnu/fs/xfs/FreeBSD/xfs_vnops.c (revision 180729) > +++ sys/gnu/fs/xfs/FreeBSD/xfs_vnops.c (working copy) > @@ -263,6 +263,8 @@ _xfs_getattr( > vap->va_atime = va.va_atime; > vap->va_mtime = va.va_mtime; > vap->va_ctime = va.va_ctime; > + vap->va_birthtime.tv_sec = -1; > + vap->va_birthtime.tv_nsec = 0; > vap->va_gen = va.va_gen; > vap->va_rdev = va.va_rdev; > vap->va_bytes = (va.va_nblocks << BBSHIFT); See above (need to do somethign about the VATTR_NULL()). > %%% Grepping for va_.*flags in only sys/fs/ shows the following problems in VOP_SETATTR(): - coda: sets va_vaflags in a macro but never uses va_vaflags (needed for layering?) - ntfs: sets va_flags to ip->i_flag -- nonsense -- i_flag is an internal flag that has nothing to do with va_flags - nwfs: sets va_vaflags in nwfs_attr_cacheenter(), but I think nothing uses this setting. - smbfs: sets va_vaflags in smbfs_attrcachelookup() ... - tmpfs: sets va_vaflags and also va_spare. and the following non-problems: - all except msdosfs set va_flags to 0, so defaulting va_flags to 0 and deleting most settings of it would work well. Bruce